janus_cli: automatically discover datastore keys
#448
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
divergentdave
approved these changes
Aug 30, 2022
janus_server/src/bin/janus_cli.rs
Outdated
| long, | ||
| env = "SECRETS_K8S_NAMESPACE", | ||
| takes_value = true, | ||
| long_help = "Kubernetes namespace where datastore keyis stored. Required if \ |
There was a problem hiding this comment.
nit: typo
Suggested change
| long_help = "Kubernetes namespace where datastore keyis stored. Required if \ | |
| long_help = "Kubernetes namespace where the datastore key is stored. Required if \ |
tgeoghegan
force-pushed
the
timg/janus-cli-secrets
branch
from
51c3485 to
18433ef
Compare
janus_server/src/bin/janus_cli.rs
Outdated
Comment on lines
64
to
106
| let k8s_client = kube::Client::try_default() | ||
| .await | ||
| .context("couldn't connect to Kubernetes environment")?; |
There was a problem hiding this comment.
With janus_cli decode-dap-message on the way, we should avoid constructing this client unless it is required by the subcommand, so that write-schema and decode-dap-message can be used on systems without kubeconfig files. once_cell::unsync::Lazy would be a convenient way to do so without duplication.
There was a problem hiding this comment.
+1, we should maintain that we don't access/connect to dependencies that are not used for the particular subcommand.
branlwyd
approved these changes
Aug 31, 2022
janus_server/src/bin/janus_cli.rs
Outdated
Comment on lines
64
to
106
| let k8s_client = kube::Client::try_default() | ||
| .await | ||
| .context("couldn't connect to Kubernetes environment")?; |
There was a problem hiding this comment.
+1, we should maintain that we don't access/connect to dependencies that are not used for the particular subcommand.
|
Downgrading to draft while I re-work the argument handling here and in #450 |
tgeoghegan
force-pushed
the
timg/janus-cli-secrets
branch
3 times, most recently
from
2ca4a34 to
411c870
Compare
tgeoghegan
changed the base branch from
main
to
timg/protocol-message-decoder
|
divergentdave
requested changes
Sep 1, 2022
janus_server/src/bin/janus_cli.rs
Outdated
Comment on lines
104
to
107
| let kube_client = kube::Client::try_default() | ||
| .await | ||
| .context("couldn't connect to Kubernetes environment")?; | ||
|
|
There was a problem hiding this comment.
We need to defer this, and only run it inside the commands that require it. Otherwise, we may get the following error when it's not necessary.
Error: couldn't connect to Kubernetes environment
Caused by:
0: Failed to infer configuration: failed to infer config: in-cluster: (failed to read the default namespace: No such file or directory (os error 2)), kubeconfig: (failed to read kubeconfig from '"/root/.kube/config"': No such file or directory (os error 2))
1: failed to infer config: in-cluster: (failed to read the default namespace: No such file or directory (os error 2)), kubeconfig: (failed to read kubeconfig from '"/root/.kube/config"': No such file or directory (os error 2))
2: failed to read kubeconfig from '"/root/.kube/config"': No such file or directory (os error 2)
3: No such file or directory (os error 2)
There was a problem hiding this comment.
Whoops, I was sure I had caught this, but I think I got mixed up while rebasing.
divergentdave
approved these changes
Sep 1, 2022
In order to provision tasks, `janus_cli` needs the datastore key, used encrypt some database rows. This key can be provided with `--datastore-keys`, but since `janus_cli` has to talk to a Kubernetes cluster anyway, it can also fetch the secret value from Kubernetes secrets, which is fewer steps for an operator (no need to fetch the secret before running `janus_cli`) and less risk of exposing the datastore key to an operator workstation (or worse, an operator's clipboard). Resolves #409
Add a doctest for `janus_server::config::CommonConfig` with a minimal example. This is principally useful so I can copy-paste this into a config file for `janus_cli`.
When `janus_cli` provisions a task, it first deletes any task with the provided `TaskId`, and ignores the error if it is "Not Found". But if the `delete_task` method is annotated with `#[tracing::instrument(err)]`, then you get a big, scary level=ERROR line printed in RED to the console when `janus_cli` is run, even though the error is benign. Remove the annotation to avoid operators incorrectly thinking something has gone wrong.
tgeoghegan
force-pushed
the
timg/janus-cli-secrets
branch
from
14d1f6a to
e1348ed
Compare
|
Another day, another PR merged to the wrong target. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to provision tasks,
janus_clineeds the datastore key, usedencrypt some database rows. This key can be provided with
--datastore-keys, but sincejanus_clihas to talk to a Kubernetescluster anyway, it can also fetch the secret value from Kubernetes
secrets, which is fewer steps for an operator (no need to fetch the
secret before running
janus_cli) and less risk of exposing thedatastore key to an operator workstation (or worse, an operator's
clipboard).