-
Notifications
You must be signed in to change notification settings - Fork 453
Add --token-env flag to install command
#6755
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: s3rj1k <[email protected]>
twz123
left a comment
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.
Can you mention this somewhere in the docs, too? /cc #5253
| return fmt.Errorf("invalid node config: %w", errors.Join(errs...)) | ||
| } | ||
|
|
||
| // Convert --token-env to --token-file |
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.
Personally, I think this is a very unintuitive thing to do. By nature, an environment variable is ephemeral and belongs to the current process only. Capturing it in a file is not something I'd want to happen with it. Moreover, this creates a discrepancy between the arguments passed to the install controller/worker command and the arguments passed to controller/worker command, which we have managed to avoid so far. If folks need to persist the token, then they can just use --token-file without surprises.
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.
Are you fine with keeping temporary bootstrap token inside init system file instead of keeping pointing it to a file?
And no, there are use cases that do not allow just using "--token-file without surprises"
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.
@makhov pointed out that there's a straight forward solution which wouldn't require any special-handling at all here: We could simply use k0s install -e K0S_JOIN_TOKEN controller [...] and be done with it. That would store the token somewhere in the init system config, and the behavior would be much more obvious. /cc #6763
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.
yes, I am working on this RN, ETA PR today
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.
alternative PR #6766
| } | ||
|
|
||
| func loadKubeconfigFromTokenFile(path string) (*clientcmdapi.Config, error) { | ||
| func loadKubeconfigFromToken(path string) (*clientcmdapi.Config, error) { |
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.
It's still reading a token file, so we could keep the old name.
| flagset.StringVar(&workerOpts.WorkerProfile, "profile", defaultWorkerProfile, "worker profile to use on the node") | ||
| flagset.BoolVar(&workerOpts.CloudProvider, "enable-cloud-provider", false, "Whether or not to enable cloud provider support in kubelet") | ||
| flagset.StringVar(&workerOpts.TokenFile, "token-file", "", "Path to the file containing join-token.") | ||
| flagset.StringVar(&workerOpts.TokenEnv, "token-env", "", "Environment variable name containing the join-token.") |
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'd rather not add an additional flag for this. Instead, I'd simply check for K0S_JOIN_TOKEN if the token isn't provided as an argument or a file.
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.
Hiding flag means, extra document section about secret environ values
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.
Yes, we need to document that.
|
Turning this to draft, #6766 should supersede this PR |
Adds new flags to improve k0s token management experience for both worker and controller:
--token-env: Allows specifying the join token via an environment variable name instead of passing it via fileRelated: #6727