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

mikedanese · 2018-03-29T02:11:18Z

The cloudprovider is right, and only cloudprovider addresses can be verified centrally, so don't add any extra when we have them.

k8s-ci-robot · 2018-03-29T02:11:19Z

@mikedanese: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ericchiang · 2018-03-29T16:40:54Z

pkg/kubelet/kubelet.go

+		// If we successfully discovered addresses from the cloudprovider, use
+		// those. Otherwise, use the configured addressed if provided. Otherwise,
+		// make a best guess.
+		if len(ips) != 0 || len(names) != 0 {


How could these ever not have len() == 0? The var block above creates zero length slices.

Maybe

if len(cloudIPs) !=0 || len(cloudNames) != 0 {

Ya that's right. I'll add a test :)

Hmm, this function seems to be untested.

ericchiang · 2018-03-29T16:43:18Z

pkg/kubelet/kubelet.go

+		if len(ips) != 0 || len(names) != 0 {
+			ips = cloudIPs
+			names = cloudNames
+		} else if cfgAddress := net.ParseIP(kubeCfg.Address); cfgAddress != nil && !cfgAddress.IsUnspecified() {


I would have expected the explicitly configured address to override the cloud provider discovered ones. What's the motivation for having discovered addresses override the explicit ones?

Ya, explicitly configured addresses should probably come first.

ericchiang

/lgtm

… the cloudprovider The cloudprovider is right, and only cloudprovider addresses can be verified centrally, so don't add any extra.

tallclair · 2018-03-31T01:29:23Z

/lgtm

k8s-ci-robot · 2018-03-31T01:29:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, mikedanese, tallclair

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/kubelet/OWNERS~~ [tallclair]

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-03-31T02:16:02Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-03-31T03:13:04Z

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

liggitt · 2018-06-28T14:35:54Z

this doesn't work in combination with the way the kubelet reports addresses, which will fall back reporting a locally detected/overridden hostname address if the cloud provider doesn't specify one:

kubernetes/pkg/kubelet/kubelet_node_status.go

Lines 532 to 546 in ee2e11a

    
           // Only add a NodeHostName address if the cloudprovider did not specify one 
        
           // (we assume the cloudprovider knows best) 
        
           var addressNodeHostName *v1.NodeAddress 
        
           for i := range nodeAddresses { 
        
           	if nodeAddresses[i].Type == v1.NodeHostName { 
        
           		addressNodeHostName = &nodeAddresses[i] 
        
           		break 
        
           	} 
        
           } 
        
           if addressNodeHostName == nil { 
        
           	hostnameAddress := v1.NodeAddress{Type: v1.NodeHostName, Address: kl.GetHostname()} 
        
           	nodeAddresses = append(nodeAddresses, hostnameAddress) 
        
           } else { 
        
           	glog.V(2).Infof("Using Node Hostname from cloudprovider: %q", addressNodeHostName.Address) 
        
           }

the kubelet needs to request a cert valid for the addresses it will report. it might be fine to omit the locally detected hostname in both places, but omitting only in one causes problems

mikedanese · 2018-06-28T14:42:04Z

cc @awly

liggitt · 2018-06-28T14:52:06Z

opened #65585 to see what happens if we apply the same logic to node status

liggitt · 2018-06-28T15:16:43Z

pkg/kubelet/kubelet.go

+
+		// If the address was explicitly configured, use that. Otherwise, try to
+		// discover addresses from the cloudprovider. Otherwise, make a best guess.
+		if cfgAddress := net.ParseIP(kubeCfg.Address); cfgAddress != nil && !cfgAddress.IsUnspecified() {


this doesn't seem to make sense... isn't hostnameOverride an "explicit configuration" as well? why would --node-ip trump the cloud provider, and not --hostname-override? I could see making the cloud provider completely authoritative, but the ordering here doesn't make sense to me

edit: retracing this, this address isn't --node-ip, it's --address. that makes even less sense to have it override the cloud-provider-specified addresses.

kubelet node status jumps through a ton of hoops to derive an IP address to report:

// 1) Use nodeIP if set // 2) If the user has specified an IP to HostnameOverride, use it // 3) Lookup the IP from node name by DNS and use the first valid IPv4 address. // If the node does not have a valid IPv4 address, use the first valid IPv6 address. // 4) Try to get the IP from the network interface used as default gateway if kl.nodeIP != nil { ipAddr = kl.nodeIP } else if addr := net.ParseIP(kl.hostname); addr != nil { ipAddr = addr } else { var addrs []net.IP addrs, _ = net.LookupIP(node.Name) for _, addr := range addrs { if err = kl.nodeIPValidator(addr); err == nil { if addr.To4() != nil { ipAddr = addr break } if addr.To16() != nil && ipAddr == nil { ipAddr = addr } } } if ipAddr == nil { ipAddr, err = utilnet.ChooseHostInterface() } }

unfortunately, we have to match that here when requesting certs

Updated #65585 to make this match as well

long-term (hopefully in 1.12), we want to switch the serving cert request to react to changes in the node status, so that the node always has a valid cert (or a pending csr) for the addresses in the node status. that lets us contain the crazy address-computing code to one place, and also drive node addresses via an external cloud provider

long-term (hopefully in 1.12)

turns out it was easier than expected. see #65594

@mikedanese

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Revert "certs: only append locally discovered addresses when we got none from the cloudprovider" This reverts commit 7354bbe. #61869 caused a mismatch between the requested CSR and the addresses in node status. Instead of computing addresses in two places, the cert manager should derive its CSR request from the addresses in node status. This would enable the kubelet to react to address changes, as well as be driven by an external cloud provider. /cc @mikedanese ```release-note NONE ```

mikedanese assigned ericchiang and tallclair Mar 29, 2018

k8s-ci-robot requested review from dashpole and sjpotter March 29, 2018 02:11

mikedanese changed the title ~~certs: only append locally discovered addresses when we got none from the cloudprovider~~ certs: only append locally discovered addresses when we get none from the cloudprovider Mar 29, 2018

mikedanese force-pushed the serverauth1 branch from 077341f to 0f75de9 Compare March 29, 2018 02:29

ericchiang reviewed Mar 29, 2018

View reviewed changes

mikedanese force-pushed the serverauth1 branch from 0f75de9 to 0b80a33 Compare March 29, 2018 17:16

ericchiang approved these changes Mar 29, 2018

View reviewed changes

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

mikedanese force-pushed the serverauth1 branch from 0b80a33 to 0a4240a Compare March 30, 2018 04:03

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

certs: only append locally discovered addresses when we got none from…

7354bbe

… the cloudprovider The cloudprovider is right, and only cloudprovider addresses can be verified centrally, so don't add any extra.

mikedanese force-pushed the serverauth1 branch from 0a4240a to 7354bbe Compare March 30, 2018 16:22

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

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

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

k8s-github-robot merged commit 9c67d76 into kubernetes:master Mar 31, 2018

mikedanese deleted the serverauth1 branch March 31, 2018 03:24

liggitt mentioned this pull request Jun 28, 2018

Treat cloud provider addresses as authoritative #65585

Closed

liggitt reviewed Jun 28, 2018

View reviewed changes

liggitt mentioned this pull request Jun 28, 2018

Revert "certs: only append locally discovered addresses when we got none from the cloudprovider" #65587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mikedanese commented Mar 29, 2018

k8s-ci-robot commented Mar 29, 2018

ericchiang Mar 29, 2018 •

edited

Loading

ericchiang Mar 29, 2018

mikedanese Mar 29, 2018

mikedanese Mar 29, 2018

ericchiang Mar 29, 2018

mikedanese Mar 29, 2018

ericchiang left a comment

tallclair commented Mar 31, 2018

k8s-ci-robot commented Mar 31, 2018

k8s-github-robot commented Mar 31, 2018

k8s-github-robot commented Mar 31, 2018

liggitt commented Jun 28, 2018

mikedanese commented Jun 28, 2018

liggitt commented Jun 28, 2018

liggitt Jun 28, 2018 •

edited

Loading

liggitt Jun 28, 2018

liggitt Jun 28, 2018

liggitt Jun 28, 2018

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

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

Conversation

mikedanese commented Mar 29, 2018

k8s-ci-robot commented Mar 29, 2018

ericchiang Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang left a comment

Choose a reason for hiding this comment

tallclair commented Mar 31, 2018

k8s-ci-robot commented Mar 31, 2018

k8s-github-robot commented Mar 31, 2018

k8s-github-robot commented Mar 31, 2018

liggitt commented Jun 28, 2018

mikedanese commented Jun 28, 2018

liggitt commented Jun 28, 2018

liggitt Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang Mar 29, 2018 •

edited

Loading

liggitt Jun 28, 2018 •

edited

Loading