Avoid unnecessary node replacements when TLS bootstrapping is enabled #639

danielfm · 2017-05-10T19:49:15Z

Simplifies the code by re-using the same encryption/caching/fingerprinting logic both to cert files and TLS bootstrapping / auth tokens, and keeps the TLS bootstrap token in its own file instead of storing it directly in ./credentials/tokens.csv.

Concretely, instead of using the ./credentials/tokens.csv file to store the TLS bootstrap token (and re-encrypting it every time, causing the bug in #633), we now store the bootstrap token in its own file, thus not causing unnecessary node pool replacement at every kube-aws update.

TODO

Test creation/update workflow with TLS bootstrapping disabled, without custom auth tokens
Test creation/update workflow with TLS bootstrapping disabled, with custom auth tokens
Test creation/update workflow with TLS bootstrapping enabled, without custom auth tokens
Test creation/update workflow with TLS bootstrapping enabled, with custom auth tokens
Address review comments and perform the last round of tests

Actions Required

If this get merged, we must not forget to inform users of the experimental TLS Bootstrap feature that the following actions are required:

Run kube-aws render stack to update controller/worker user data templates

Move the bootstrap token from ./credentials/tokens.csv to its own file ./credentials/kubelet-tls-bootstrap-token

# Remove the following line from ./credentials/tokens.csv, and move <token>
# (with no leading/trailing blank chars and lines) to it's own file
# ./credentials/kubelet-tls-bootstrap-token
<token>,kubelet-bootstrap,10001,system:kubelet-bootstrap

Run kube-aws update to update the cluster. This operation will cause controllers and workers to be replaced

Fixes #633.

The logic contained in this file will be re-used so we can also handle encryption of non-TLS assets.

Doing so, it allows us to re-use the same logic to handle encryption and caching of non-TLS resources.

codecov-io · 2017-05-10T19:59:49Z

Codecov Report

Merging #639 into master will decrease coverage by 0.84%.
The diff coverage is 76.33%.

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
- Coverage   38.26%   37.42%   -0.85%     
==========================================
  Files          51       50       -1     
  Lines        3316     3177     -139     
==========================================
- Hits         1269     1189      -80     
+ Misses       1845     1812      -33     
+ Partials      202      176      -26

Impacted Files	Coverage Δ
core/controlplane/config/credential.go	`57.14% <57.14%> (-3.09%)`	⬇️
core/controlplane/config/config.go	`56.42% <60%> (+0.44%)`	⬆️
core/controlplane/config/encrypted_assets.go	`73.09% <77.7%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f22f2cf...7d07d24. Read the comment docs.

This commit gets rid of the `token_config.go` file that were used to read/cache/fingerprint the auth tokens file, and instead re-use the same logic for reading/caching/fingerprinting TLS cert files. This commit also changes the way the auth tokens file is assembled; instead of storing the TLS bootstrap token in this file - and extracting/encrypting it in every update (which cause the issue reported in #633), we store the TLS bootstrap token separately in its own file (which are cached and fingerprinted like the other ones) and assemble the final auth tokens file in `cloud-config-controller`, during the asset decryption step.

Ensures that tests that rely on the TLS bootstrapping feature run with a dummy bootstrap token in order to bypass validation.

danielfm · 2017-05-15T13:24:51Z

This PR is ready for review.

mumoshu · 2017-05-16T02:27:31Z

core/root/render/credentials.go

@@ -25,9 +25,9 @@ func NewCredentialsRenderer(c *config.Cluster) CredentialsRenderer {
 	}
 }

-func (r credentialsRendererImpl) RenderTLSCerts(renderCredentialsOpts config.CredentialsOptions) error {
+func (r credentialsRendererImpl) RenderCerts(renderCredentialsOpts config.CredentialsOptions) error {


Perhaps RenderCredentials should be a better name here as it generates not only certs but also tls-bootstrap token?

mumoshu · 2017-05-16T02:41:23Z

core/controlplane/config/templates/cloud-config-controller

+           echo $(cat $authDir/tls-bootstrap.token),kubelet-bootstrap,10001,system:kubelet-bootstrap >> $authDir/computed-tokens.csv
+           {{- end }}
+           {{- if .AssetsConfig.HasAuthTokens }}
+           cat $authDir/tokens.csv >> $authDir/computed-tokens.csv


nit but my preference is first write a work file like tokens.csv to /var/run/coreos/tokens.csv and keep the final one named $authDir/tokens.csv rather than computed-tokens.csv
so that someone peeked into $authDir finds only tokens.csv and don't get confused like which one is the "final" tokens file passed to apiserver, computed-tokens.csv or tokens.csv?

mumoshu · 2017-05-16T02:42:06Z

core/controlplane/config/templates/cloud-config-controller

@@ -1326,7 +1340,7 @@ write_files:
        - hostPath:
            path: /usr/share/ca-certificates
          name: ssl-certs-host
-        {{if .AuthTokensConfig.HasTokens}}
+        {{if .AssetsConfig.HasAnyAuthTokens}}


BTW I like the function naming 👍

mumoshu · 2017-05-16T02:49:16Z

core/controlplane/config/templates/cloud-config-worker

           echo injecting token into the kubelet bootstrap kubeconfig file
-           bootstrap_token=$(cat /etc/kubernetes/auth/kubelet-bootstrap.token);
+           bootstrap_token=$(cat /etc/kubernetes/auth/tls-bootstrap.token);


Just for my education but why did you rename it from kubelet- to tls-?
I believe you tried to match it against other relevant symbol names(TLSBootstrap, HasTLSBootstrapToken).
However, the naming in kubelet is kubelet-bootstrap rather than tls-bootstrap.
I guess you assumed (like me) that it is originally meant for kubelet-(tls-)bootstrap hence named tls-bootstrap?

Yes, my reasoning was to make it match with the symbols in Go code, and what this feature is called in upstream Kubernetes documentation, but I think I forgot to replace a few other names in templates.

What do you think it's the right thing to do here? Keep the old kublet-bootstrap names throughout the templates, or replace them all by tls-bootstrap (or even kubelet-tls-bootstrap)?

Thanks for the clarification!

Yes. Renaming to tls-bootstrap makes more sense for me.

Also, I began to slightly prefer kubelet-tls-bootstrap because it produces no ambiguity even in the context of kube-aws. "kube-aws tls bootstrap" vs "kube-aws kubelet tls bootstrap" - the latter seems to better represent that it is for kubelet for me.

mumoshu · 2017-05-16T03:03:49Z

core/controlplane/config/templates/cloud-config-controller

+           echo > $authDir/computed-tokens.csv
+
+           {{- if .AssetsConfig.HasTLSBootstrapToken }}
+           echo $(cat $authDir/tls-bootstrap.token),kubelet-bootstrap,10001,system:kubelet-bootstrap >> $authDir/computed-tokens.csv


Sorry for being late in commenting something like this but

my preference after read through code is, the token file named tls-bootstrap-token rather than tls-bootstrap.token so that we can be consistent in the word token contained in basenames of both files tls-bootstrap-token and tokens.csv

It can be named like tls-bootstra-token.txt but I feels that it is too much 😃 and we already have various txt files without the .txt extentions anyway

mumoshu · 2017-05-16T03:06:27Z

@danielfm Thanks as always for your continuous efforts on this area 🙇

I've added several comments but LGTM overall.
Would you mind addressing these?

Anyway IMHO, what the most important thing in this PR is the code reduced by 500 lines!
Great job 👍

danielfm · 2017-05-16T13:44:04Z

@mumoshu sure, I'll try to address these ASAP.

The idea is to document that these files are temporary; their only purpose is to be used to generate the final $authDir/tokens.csv file.

mumoshu · 2017-05-18T06:26:43Z

@danielfm Thanks a lot for your efforts! LGTM 👍

* kubernetes-incubator/master: Fix "install-kube-system" script when "clusterAutoscaler" is disabled. Remove obsolete etcd locking logic Re: cluster-autoscaler support Make `go test` timeout longer enough for Travis Fixes kubernetes-retired#667 NVIDIA driver installation support on GPU instances (kubernetes-retired#645) Make kubelet flags more consistent Fix taint being assigned as labels Avoid unnecessary node replacements when TLS bootstrapping is enabled (kubernetes-retired#639) Update Kubernetes dashboard to v1.6.1. Update calico to v2.2.1. Fix typo in help message

…kubernetes-retired#639) Simplifies the code by re-using the same encryption/caching/fingerprinting logic both to cert files and TLS bootstrapping / auth tokens, and keeps the TLS bootstrap token in its own file instead of storing it directly in `./credentials/tokens.csv`. Concretely, instead of using the `./credentials/tokens.csv` file to store the TLS bootstrap token (and re-encrypting it every time, causing the bug in kubernetes-retired#633), we now store the bootstrap token in its own file, thus not causing unnecessary node pool replacement at every `kube-aws update`. TODO - [x] Test creation/update workflow with TLS bootstrapping disabled, without custom auth tokens - [x] Test creation/update workflow with TLS bootstrapping disabled, with custom auth tokens - [x] Test creation/update workflow with TLS bootstrapping enabled, without custom auth tokens - [x] Test creation/update workflow with TLS bootstrapping enabled, with custom auth tokens - [x] Address review comments and perform the last round of tests ## Actions Required If this get merged, we must not forget to inform users of the experimental TLS Bootstrap feature that the following actions are required: 1. Run `kube-aws render stack` to update controller/worker user data templates 2. Move the bootstrap token from `./credentials/tokens.csv` to its own file `./credentials/kubelet-tls-bootstrap-token` ``` # Remove the following line from ./credentials/tokens.csv, and move <token> # (with no leading/trailing blank chars and lines) to it's own file # ./credentials/kubelet-tls-bootstrap-token <token>,kubelet-bootstrap,10001,system:kubelet-bootstrap ``` 3. Run `kube-aws update` to update the cluster. This operation will cause controllers and workers to be replaced Fixes kubernetes-retired#633. Changelog * Rename files The logic contained in this file will be re-used so we can also handle encryption of non-TLS assets. * Rename functions and struct members to not mention TLS Doing so, it allows us to re-use the same logic to handle encryption and caching of non-TLS resources. * Reuse caching/encryption logic for the TLS bootstrap and auth tokens This commit gets rid of the `token_config.go` file that were used to read/cache/fingerprint the auth tokens file, and instead re-use the same logic for reading/caching/fingerprinting TLS cert files. This commit also changes the way the auth tokens file is assembled; instead of storing the TLS bootstrap token in this file - and extracting/encrypting it in every update (which cause the issue reported in kubernetes-retired#633), we store the TLS bootstrap token separately in its own file (which are cached and fingerprinted like the other ones) and assemble the final auth tokens file in `cloud-config-controller`, during the asset decryption step. * Avoid create/update cluster with TLS bootstrap enabled without token * Fix tests Ensures that tests that rely on the TLS bootstrapping feature run with a dummy bootstrap token in order to bypass validation. * Match conditional used in asset decryption script for consistency * Auto-generates the TLS bootstrap token if necessary * Rename function to a more appropriate name * Rename TLS token to make it clear it's related to the Kubelet * Suffix auth token and Kubelet TLS bootstrap files with `.tmp` The idea is to document that these files are temporary; their only purpose is to be used to generate the final $authDir/tokens.csv file.

Daniel Fernandes Martins added 2 commits May 9, 2017 15:26

Rename files

9d2cb2e

The logic contained in this file will be re-used so we can also handle encryption of non-TLS assets.

Rename functions and struct members to not mention TLS

503d04d

Doing so, it allows us to re-use the same logic to handle encryption and caching of non-TLS resources.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2017

Daniel Fernandes Martins added 4 commits May 10, 2017 18:16

Avoid create/update cluster with TLS bootstrap enabled without token

bcb0d34

Fix tests

d344dec

Ensures that tests that rely on the TLS bootstrapping feature run with a dummy bootstrap token in order to bypass validation.

Match conditional used in asset decryption script for consistency

1a1b3fe

mumoshu modified the milestones: backlog, wip, tbd May 12, 2017

mumoshu added this to To be reminded in v0.9.7 May 12, 2017

mumoshu modified the milestones: tbd, v0.9.7-rc.<tbd> May 12, 2017

danielfm changed the title ~~TLS bootstrap encrypted token cache~~ Fix unnecessary node replacements when TLS bootstrapping is enabled May 12, 2017

Auto-generates the TLS bootstrap token if necessary

eb25c9b

mumoshu self-requested a review May 16, 2017 02:21

mumoshu reviewed May 16, 2017

View reviewed changes

Rename function to a more appropriate name

00e5961

Daniel Fernandes Martins added 3 commits May 16, 2017 11:26

Rename TLS token file

5d51d5d

Rename TLS token to make it clear it's related to the Kubelet

d1c8e42

Suffix auth token and Kubelet TLS bootstrap files with .tmp

7d07d24

The idea is to document that these files are temporary; their only purpose is to be used to generate the final $authDir/tokens.csv file.

danielfm changed the title ~~Fix unnecessary node replacements when TLS bootstrapping is enabled~~ Avoid unnecessary node replacements when TLS bootstrapping is enabled May 17, 2017

mumoshu merged commit 1e2ac13 into kubernetes-retired:master May 18, 2017

mumoshu modified the milestones: v0.9.7-rc.1, v0.9.7-rc.<tbd> May 18, 2017

danielfm deleted the tls-bootstrap-encrypted-token-cache branch January 5, 2018 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary node replacements when TLS bootstrapping is enabled #639

Avoid unnecessary node replacements when TLS bootstrapping is enabled #639

danielfm commented May 10, 2017 •

edited

codecov-io commented May 10, 2017 •

edited

danielfm commented May 15, 2017

mumoshu May 16, 2017

mumoshu May 16, 2017

mumoshu May 16, 2017

mumoshu May 16, 2017

danielfm May 16, 2017 •

edited

mumoshu May 16, 2017

danielfm May 17, 2017

mumoshu May 16, 2017

mumoshu commented May 16, 2017

danielfm commented May 16, 2017

mumoshu commented May 18, 2017

Avoid unnecessary node replacements when TLS bootstrapping is enabled #639

Avoid unnecessary node replacements when TLS bootstrapping is enabled #639

Conversation

danielfm commented May 10, 2017 • edited

Actions Required

codecov-io commented May 10, 2017 • edited

Codecov Report

danielfm commented May 15, 2017

mumoshu May 16, 2017

Choose a reason for hiding this comment

mumoshu May 16, 2017

Choose a reason for hiding this comment

mumoshu May 16, 2017

Choose a reason for hiding this comment

mumoshu May 16, 2017

Choose a reason for hiding this comment

danielfm May 16, 2017 • edited

Choose a reason for hiding this comment

mumoshu May 16, 2017

Choose a reason for hiding this comment

danielfm May 17, 2017

Choose a reason for hiding this comment

mumoshu May 16, 2017

Choose a reason for hiding this comment

mumoshu commented May 16, 2017

danielfm commented May 16, 2017

mumoshu commented May 18, 2017

danielfm commented May 10, 2017 •

edited

codecov-io commented May 10, 2017 •

edited

danielfm May 16, 2017 •

edited