Derive kubelet serving certificate CSR template from node status addresses #65594

liggitt · 2018-06-28T18:45:15Z

xref kubernetes/enhancements#267
fixes #55633

Builds on #65587

Makes the cloud provider authoritative when recording node status addresses
Makes the node status addresses authoritative for the kube-apiserver determining how to speak to a kubelet (stops paying attention to the hostname label when determining how to reach a kubelet, which was only done to support kubelets < 1.5)
Updates kubelet certificate rotation to be driven from node status
- Avoids needing to compute node addresses a second time, and differently, in order to request serving certificates.
- Allows the kubelet to react to changes in its status addresses by updating its serving certificate
- Allows the kubelet to be driven by external cloud providers recording node addresses on the node status

test procedure:

# setup
export FEATURE_GATES=RotateKubeletServerCertificate=true
export KUBELET_FLAGS="--rotate-server-certificates=true --cloud-provider=external"

# cleanup from previous runs
sudo rm -fr /var/lib/kubelet/pki/

# startup
hack/local-up-cluster.sh

# wait for a node to register, verify it didn't set addresses
kubectl get nodes 
kubectl get node/127.0.0.1 -o jsonpath={.status.addresses}

# verify the kubelet server isn't available, and that it didn't populate a serving certificate
curl --cacert _output/certs/server-ca.crt -v https://localhost:10250/pods
ls -la /var/lib/kubelet/pki

# set an address on the node
curl -X PATCH http://localhost:8080/api/v1/nodes/127.0.0.1/status \
  -H "Content-Type: application/merge-patch+json" \
  --data '{"status":{"addresses":[{"type":"Hostname","address":"localhost"}]}}'

# verify a csr was submitted with the right SAN, and approve it
kubectl describe csr
kubectl certificate approve csr-...

# verify the kubelet connection uses a cert that is properly signed and valid for the specified hostname, but NOT the IP
curl --cacert _output/certs/server-ca.crt -v https://localhost:10250/pods
curl --cacert _output/certs/server-ca.crt -v https://127.0.0.1:10250/pods
ls -la /var/lib/kubelet/pki

# set an hostname and IP address on the node
curl -X PATCH http://localhost:8080/api/v1/nodes/127.0.0.1/status \
  -H "Content-Type: application/merge-patch+json" \
  --data '{"status":{"addresses":[{"type":"Hostname","address":"localhost"},{"type":"InternalIP","address":"127.0.0.1"}]}}'

# verify a csr was submitted with the right SAN, and approve it
kubectl describe csr
kubectl certificate approve csr-...

# verify the kubelet connection uses a cert that is properly signed and valid for the specified hostname AND IP
curl --cacert _output/certs/server-ca.crt -v https://localhost:10250/pods
curl --cacert _output/certs/server-ca.crt -v https://127.0.0.1:10250/pods
ls -la /var/lib/kubelet/pki

* kubelets that specify `--cloud-provider` now only report addresses in Node status as determined by the cloud provider (unless `--hostname-override` is used to force reporting of the specified hostname)
* kubelet serving certificate rotation now reacts to changes in reported node addresses, and will request certificates for addresses set by an external cloud provider

liggitt · 2018-06-28T19:02:16Z

@kubernetes/sig-node-pr-reviews @kubernetes/sig-auth-pr-reviews
/assign @mikedanese @awly

rphillips · 2018-06-28T19:44:46Z

pkg/kubelet/certificate/kubelet.go

+			}
+		}
+	}
+


@liggitt since we are using reflect to verify a deep equal later in the code, we should sort the dns and IP addresses just in case they get returned in a different order at some point.

good point, done

liggitt · 2018-06-28T19:53:24Z

/test pull-kubernetes-e2e-gke

liggitt · 2018-06-28T19:53:39Z

/retest

rphillips · 2018-06-28T20:18:22Z

/lgtm

liggitt · 2018-06-28T21:45:35Z

/test pull-kubernetes-e2e-gke

smarterclayton · 2018-06-29T00:30:08Z

pkg/kubelet/certificate/kubelet.go

+
+		switch address.Type {
+		case v1.NodeHostName:
+			if ip := net.ParseIP(address.Address); ip != nil {


Hrm. Is there any scenario where the node hostname can like an IP, but is not actually an IP and would not be appropriate to interpret as an IP?

Is there any scenario where the node hostname can like an IP, but is not actually an IP and would not be appropriate to interpret as an IP?

No? A hostname that parses as an IP is an IP... SNI wouldn't treat it as a DNS name, cert verification would require an IP SAN (c.f. https://golang.org/src/crypto/x509/verify.go?s=28297:28349#L914)

smarterclayton · 2018-06-29T00:31:54Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

+		wait.PollImmediateInfinite(time.Second, func() (bool, error) {
+			lastTemplate = m.getTemplate()
+			if lastTemplate == nil {
+				glog.Infof("Waiting for certificate template")


Why not v(2)? Because it hangs? If so, might want a slightly longer message.

Yeah, wanted visibility

liggitt · 2018-07-04T02:46:45Z

/retest

liggitt · 2018-07-04T16:36:06Z

/retest

liggitt · 2018-07-05T03:21:50Z

/retest

awly

LGTM, but @mikedanese should take another look

awly · 2018-07-09T17:16:39Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

 	m := manager{
 		certSigningRequestClient: config.CertificateSigningRequestClient,
-		template:                 config.Template,
+		getTemplate:              getTemplate,
+		dynamicTemplate:          dynamicTemplate,


This can be config.GetTemplate != nil and local dynamicTemplate var can be removed

~~config.GetTemplate and config.Template are mutually exclusive. dynamicTemplate is used to determine whether we need to monitor getTemplate() for changes~~

edit: misread, can update

mikedanese · 2018-07-09T22:12:24Z

This fixed GKE alpha cluster creation.

/lgtm

…esses

…t connections

k8s-ci-robot · 2018-07-10T18:34:28Z

New changes are detected. LGTM label has been removed.

liggitt · 2018-07-10T18:35:00Z

fixed #65594 (comment), rebased, no other changes, relabeling
/assign @smarterclayton
for approval of change to pkg/util/node/node.go (dropped use of the hostname label when computing node connection URLs... was for <1.5 compatibility)

liggitt · 2018-07-11T00:47:23Z

/retest

smarterclayton · 2018-07-11T20:44:29Z

/lgtm

k8s-ci-robot · 2018-07-11T20:44:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese, rphillips, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/client-go/util/certificate/OWNERS~~ [liggitt,mikedanese,smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-07-12T05:25:05Z

Automatic merge from submit-queue (batch tested with PRs 65052, 65594). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from awly and smarterclayton June 28, 2018 18:45

liggitt force-pushed the node-csr-addresses-2 branch from f6337f8 to fb44c6a Compare June 28, 2018 18:53

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 28, 2018

k8s-ci-robot assigned awly and mikedanese Jun 28, 2018

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jun 28, 2018

rphillips reviewed Jun 28, 2018

View reviewed changes

liggitt force-pushed the node-csr-addresses-2 branch from bf9603c to 2595403 Compare June 28, 2018 20:05

k8s-ci-robot assigned rphillips Jun 28, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2018

liggitt mentioned this pull request Jun 28, 2018

certs: only append locally discovered addresses when we get none from the cloudprovider #61869

Merged

liggitt force-pushed the node-csr-addresses-2 branch from 2595403 to d4fe5b5 Compare June 28, 2018 22:44

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2018

liggitt force-pushed the node-csr-addresses-2 branch from d4fe5b5 to 85298b5 Compare June 28, 2018 23:56

smarterclayton reviewed Jun 29, 2018

View reviewed changes

awly reviewed Jul 9, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2018

liggitt added 3 commits July 10, 2018 14:33

Derive kubelet serving certificate CSR template from node status addr…

db9d3c2

…esses

Make cloud provider authoritative for node status address reporting

7828e5d

Make node status addresses authoritative for kube-apiserver -> kubele…

7af8c6a

…t connections

liggitt force-pushed the node-csr-addresses-2 branch from b9ff9a9 to 7af8c6a Compare July 10, 2018 18:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2018

k8s-ci-robot assigned smarterclayton Jul 10, 2018

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2018

k8s-github-robot merged commit 337dfe0 into kubernetes:master Jul 12, 2018

liggitt deleted the node-csr-addresses-2 branch July 20, 2018 03:08

klausenbusk mentioned this pull request Aug 17, 2018

hack/quickstart: enable setting kubelet cloud provider separately from controller manager kubernetes-retired/bootkube#804

Closed

liggitt mentioned this pull request Aug 28, 2018

Report Hostname node address for vsphere #67922

Merged

WanLinghao mentioned this pull request Dec 9, 2018

Clean unused function in pkg/kubelet/kubelet.go #71889

Merged

andrewsykim mentioned this pull request Mar 8, 2019

kubelet: lookup node address for external provider if none is set #75229

Merged

micahhausler mentioned this pull request Apr 11, 2019

Too many certificate files being generated for EKS Optimized AMI v.12 awslabs/amazon-eks-ami#244

Closed

wongma7 mentioned this pull request Jul 10, 2019

Fix cloud reported hostname being overridden if nodeIP set #80003

Merged

cleanshavenalex mentioned this pull request Sep 5, 2019

--hostname-override ignored when --cloud-provider is specified #54482

Open

aojea mentioned this pull request Jun 3, 2024

document kubelet --node-ip and external cloud provider node addresses logic #125300

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive kubelet serving certificate CSR template from node status addresses #65594

Derive kubelet serving certificate CSR template from node status addresses #65594

liggitt commented Jun 28, 2018 •

edited

Loading

liggitt commented Jun 28, 2018

rphillips Jun 28, 2018

liggitt Jun 28, 2018

liggitt commented Jun 28, 2018

liggitt commented Jun 28, 2018

rphillips commented Jun 28, 2018

liggitt commented Jun 28, 2018

smarterclayton Jun 29, 2018

liggitt Jun 29, 2018

smarterclayton Jun 29, 2018

liggitt Jun 29, 2018

liggitt commented Jul 4, 2018

liggitt commented Jul 4, 2018

liggitt commented Jul 5, 2018

awly left a comment

awly Jul 9, 2018

liggitt Jul 9, 2018 •

edited

Loading

mikedanese commented Jul 9, 2018

k8s-ci-robot commented Jul 10, 2018

liggitt commented Jul 10, 2018 •

edited

Loading

liggitt commented Jul 11, 2018

smarterclayton commented Jul 11, 2018

k8s-ci-robot commented Jul 11, 2018

k8s-github-robot commented Jul 12, 2018

Derive kubelet serving certificate CSR template from node status addresses #65594

Derive kubelet serving certificate CSR template from node status addresses #65594

Conversation

liggitt commented Jun 28, 2018 • edited Loading

liggitt commented Jun 28, 2018

rphillips Jun 28, 2018

Choose a reason for hiding this comment

liggitt Jun 28, 2018

Choose a reason for hiding this comment

liggitt commented Jun 28, 2018

liggitt commented Jun 28, 2018

rphillips commented Jun 28, 2018

liggitt commented Jun 28, 2018

smarterclayton Jun 29, 2018

Choose a reason for hiding this comment

liggitt Jun 29, 2018

Choose a reason for hiding this comment

smarterclayton Jun 29, 2018

Choose a reason for hiding this comment

liggitt Jun 29, 2018

Choose a reason for hiding this comment

liggitt commented Jul 4, 2018

liggitt commented Jul 4, 2018

liggitt commented Jul 5, 2018

awly left a comment

Choose a reason for hiding this comment

awly Jul 9, 2018

Choose a reason for hiding this comment

liggitt Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

mikedanese commented Jul 9, 2018

k8s-ci-robot commented Jul 10, 2018

liggitt commented Jul 10, 2018 • edited Loading

liggitt commented Jul 11, 2018

smarterclayton commented Jul 11, 2018

k8s-ci-robot commented Jul 11, 2018

k8s-github-robot commented Jul 12, 2018

liggitt commented Jun 28, 2018 •

edited

Loading

liggitt Jul 9, 2018 •

edited

Loading

liggitt commented Jul 10, 2018 •

edited

Loading