-
Notifications
You must be signed in to change notification settings - Fork 108
concord-cli: support for remote secrets, configurable secrets providers #1143
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
base: master
Are you sure you want to change the base?
Conversation
2d370c7
to
7b00145
Compare
cli/src/main/java/com/walmartlabs/concord/cli/runner/secrets/RemoteSecretsProvider.java
Show resolved
Hide resolved
cli/src/main/java/com/walmartlabs/concord/cli/runner/secrets/CliSecretService.java
Show resolved
Hide resolved
var remote = cliConfig.secrets().remote(); | ||
if (remote.enabled()) { | ||
var provider = new RemoteSecretsProvider(workDir, remote.baseUrl(), remote.apiKey()); | ||
providers.add(new SecretsProviderRef("remote", provider, remote.writable())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a littttle bit concerned this opens a door to a big potential oopsie. One should be very considerate of their handling of secrets from "real" environments to start with, but this could easily end up with a "I got out of a pickle with the CLI pointed at prod real quick on Monday and forgot to turn off remote secrets...Tuesday got interesting 😬"
That could be addressed with an extra setting for the remote config(s), or any config since the same situation is possible--just fewer hoops to jump through (e.g. creating specific secrets locally for secrets.local
vs secrets.remote
unlocking any available secret in the system). Something like secrets.remote.warnOnAccess: true
that gives a prompt/warning that it's about to access a remote secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a valid point. I will see if we can add a prompt there.
Or maybe providers should be configured for specific "contexts" and the CLI can require something like `--context prod" to use a non-default context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benbroadaway how about now -- see the example in the PR description.
Users will need to explicitly say concord run --context myNonDefaultContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Config example:
In future, the feature can be extended to support multiple providers of the same type.