wasmerio / wasmer

🚀 The leading Wasm Runtime supporting WASIX and WASI
https://wasmer.io
MIT License
19.07k stars 819 forks source link

CLI Secrets management: Re-consider upsert logic #4936

Open theduke opened 4 months ago

theduke commented 4 months ago

Should this kind of logic really be in the CLI? That makes it hard to update.

This also opens the door to race conditions, so it's not actually a guarantee that create actually creates.

I feel like the backend should handle this. Eg with a "createNew" argument to the mutation.

_Originally posted by @theduke in https://github.com/wasmerio/wasmer/pull/4930#discussion_r1679282016_

theduke commented 4 months ago

@xdoardo @ayys

syrusakbary commented 4 months ago

This also opens the door to race conditions

What race conditions? I see upsert being less prone to race conditions than the other one (basically because it will not fail if it already exists while the other strategy will require always two checks - and doesn't guarantee that other client doesn't do an operation in between). But perhaps I'm missing something

theduke commented 4 months ago

@syrusakbary

So the current implementation of the "secret create" command does a check if a secret with the same name already exists. Doing that in CLI is only a best effort sanity check, it's not a guarantee of any kind. Only the backend can ensure that it is actually a "create" operation, not an update. Currently if someone else were to create a secret in the meantime the previous value would be overwritten due to the upsert.

Supporting a "create new" operation makes sense, because it prevents overwriting a secret, but if we want that to actually have guaranteed semantics that has to be done in the backend.

It's a marginal concern, because secrets will be updated rarely. It's just a question of if we actually want to ensure the behaviour.

A side concern is that putting such logic in the CLI makes it hard to update. The backend can be modified at any time without requiring users to update the CLI.