-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Implement TLS bootstrap for kubelet using --bootstrap-auth-token
#30094
Conversation
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
cc @kubernetes/sig-cluster-lifecycle |
Thanks! |
@@ -76,6 +76,11 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) { | |||
fs.StringVar(&s.TLSPrivateKeyFile, "tls-private-key-file", s.TLSPrivateKeyFile, "File containing x509 private key matching --tls-cert-file.") | |||
fs.StringVar(&s.CertDirectory, "cert-dir", s.CertDirectory, "The directory where the TLS certs are located (by default /var/run/kubernetes). "+ | |||
"If --tls-cert-file and --tls-private-key-file are provided, this flag will be ignored.") | |||
fs.StringVar(&s.BootstrapAuthToken, "bootstrap-auth-token", s.BootstrapAuthToken, ""+ | |||
"A string token that is used to get the issued certificate from API server. "+ | |||
"If --tls-cert-file, --tls-private-key-file are provided, then kubelet will use this token get a x509 cert from the API server"+ |
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.
Missing a space after server"
and key."
"If --tls-cert-file and .." (assuming it really is and).
We can throw in a "respectively" at the end, though it's clear either way.
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.
@euank will do
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.
What if these additional flags are not specified?
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.
Defaulting happens and they still get written to files, I had the same question #30094 (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.
This sentence is missing a "to" between "token" and "get"
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.
Since --tls-cert-file, --tls-private-key-file are defaulted "If --tls-cert-file, --tls-private-key-file are provided, then kubelet will" doesn't seem accurate since the kubelet will even if they aren't provided.
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.
@mikedanese Yes, let me rephrase it.
08dd84e
to
83494d4
Compare
ips = []netIP{nodeIP} | ||
} | ||
|
||
req, err := utilcertificates.NewCertificateRequest(s.TLSPrivateKeyFile, &pkix.Name{CommonName: "kubelet"}, []string{hostname}, ips) |
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.
The common name is what ends up being treated as the "user" IIRC. I also believe that if you configure --kubelet-certificate-authority
in the api-server - connections to the node (by nodeName) will validate against the CN.
Maybe we should have common name default to hostname/Nodename instead?
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.
Sorry, see that the hostname is added as SAN. But the CN="user" still stands. So I guess a question of if we would want to be able to have AuthZ on a node-by-node basis .. or if all nodes being identified by a single CN is sufficient.
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 think this is an open question, but personally I'd expect all nodes to have identical (and limited) access to the API.
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 think that we should go with separate users with identical access. Can we go with kubelet-$(hostname or hostname-override)
? Further down we may not want identical access (e.g. we will probably want to limit access to only bound pods).
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.
@mikedanese @gtank @euank @aaronlevy
kubelet-$(hostname/hostname-override)
looks like a middle ground.
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.
When requesting a serving cert, the CN should probably match the hostname. I don't think the CN mapping to username applies here… this won't be used as a client cert
I think 83494d4 should be dropped. I don't know why that's happening. It's making this PR difficult to review. |
Which one? I modified the PR, the commit you referenced gets overridden now. @mikedanese |
7af7be8
to
072a86c
Compare
certPath = defaultKubeletClientCertificateFile | ||
} | ||
if keyPath == "" { | ||
keyPath = defaultKubeletClientKeyFile |
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.
filepath.Abs(filepath.Join(certDir, "kubelet-client.key"))
@yifan-gu PR needs rebase |
return fmt.Errorf("unable to get cert from API server: %v", err) | ||
} | ||
|
||
// Marshal and write the kubeconfig to disk. |
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 think I would build a struct based on the restclient.Config the bootstrap kubeconfig resolved to, rather than including the entire bootstrap config (we don't want info from anything other than the current context, and we don't want the bootstrap credentials making it into the written kubeconfig). also, write using clientcmd.WriteToFile
(see https://github.com/liggitt/kubernetes/blob/tls-bootstrap/cmd/kubelet/app/bootstrap.go#L104-L136 for struct building and marshaling)
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.
Oh yeah, we shouldn't include the bootstrap token in the kubeconfig. But I assume the boot strap kubeconfig only has minimum context (current context).
08bcc1f
to
4bb4bd6
Compare
Labelling this PR as size/XL |
GCE e2e build/test failed for commit 08bcc1f59ff5bf17ba04220808c6440e531877dc. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
GCE e2e build/test failed for commit 4bb4bd6. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
GCE e2e build/test failed for commit 07a5c60. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
07a5c60
to
da67613
Compare
@yifan-gu PR needs rebase |
Labelling this PR as size/L |
GCE e2e build/test failed for commit da67613. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
Let's move dicussion to #30922 This PR page doesn't load correctly now, the commits are not the same as my |
GCE e2e build/test failed for commit da67613. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
Automatic merge from submit-queue Implement TLS bootstrap for kubelet using `--experimental-bootstrap-kubeconfig` (2nd take) Ref kubernetes/enhancements#43 (comment) cc @gtank @philips @mikedanese @aaronlevy @liggitt @deads2k @errordeveloper @justinsb Continue on the older PR #30094 as there are too many comments on that one and it's not loadable now.
Ref kubernetes/enhancements#43 (comment)
cc @gtank @philips @mikedanese @aaronlevy
cc @kubernetes/sig-node
Small guides on how to test this with a local cluster(thanks @gtank for help): https://gist.github.com/yifan-gu/df96e562643e49f74939fdd0bc63533a
This change is