Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate all VaultsSecrets handlers to RawHandler #822

Open
8 tasks
aryanjassal opened this issue Oct 8, 2024 · 2 comments
Open
8 tasks

Migrate all VaultsSecrets handlers to RawHandler #822

aryanjassal opened this issue Oct 8, 2024 · 2 comments
Assignees
Labels
development Standard development

Comments

@aryanjassal
Copy link
Contributor

Specification

Most, if not all, VaultsSecrets handlers transfer binary data. Doing so using plain JSON isn't the most efficient, as the required bandwidth can be as high as 200% of actual data size, making regular JSON extremely inefficient for transferring binary data.

This is solved by the RawHandler, which is able to stream raw bytes of data directly, bypassing the large overhead. However, it also has many issues, like not supporting error serialisation by default. As such, any errors thrown within the context will hit the transport layer, causing a read 0 error.

To solve this, each handler needs to implement its own method of error serialisation and deserialisation. The handler side needs to serialise the errors so they are passed through as binary data, and the client side needs to deserialise the data back into the original error. Serialising and deserialising all possible errors is not feasible, and should not be done either. Only create serialisation and deserialisation handlers for relevant errors that are expected, and raise a generic error for all other, unexpected errors.

Additional context

Tasks

  1. Migrate the following to use RawHandlers
    • VaultsSecretsEnv (check note 1)
    • VaultsSecretsGet
    • VaultsSecretsMkdir
    • VaultsSecretsNew
    • VaultsSecretsNewDir (check note 2)
    • VaultsSecretsRemove
    • VaultsSecretsRename (check note 3)
    • VaultsSecretsWrite

Notes

  1. The VaultsSecretsEnv is an interesting case. It basically performs the same as VaultsSecretsList. The source code was actually copied from the old list handler, but was never updated. Requirements need to be clarified and the command needs to be updated before we can figure out if we need to convert this handler too, or if another handler suits it better.
  2. The VaultsSecretsNewDir will be rendered obsolete with the introduction of VaultsSecretsCopy and VaultsSecretsRename. This list will be updated then, but for the time being, it is reflecting status quo.
  3. This command will be changed to act as the mv command on unix. Then, it will need to be a RawHandler.
  4. The following VaultsSecrets handlers were excluded for the given reasons:
    • VaultsSecretsStat: The data it returns can be very nicely serialised into JSON. So, instead of converting command to use RawHandler, we can simply convert it to DuplexHandler to handle performing stat on multiple files instead of doing it on a single file with UnaryHandler.
    • The VaultsSecretsList handler currently sends over only the file path and the file path. This is easily serialisable by JSON without significant performance/bandwidth issues. Even in the future, we would only need to pass in the stat alongside the output. This shouldn't require the use of RawHandler, so it will be omitted from the conversion unless a reason comes up.
@aryanjassal aryanjassal added the development Standard development label Oct 8, 2024
@aryanjassal aryanjassal self-assigned this Oct 8, 2024
Copy link

linear bot commented Oct 8, 2024

@CMCDragonkai
Copy link
Member

This needs to be weighed up against making the RPC too unique to our usecases and making it more difficult to use in other clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

No branches or pull requests

2 participants