Skip to content
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

node.Name/minion.Name should not be used as an address #2462

Closed
anguslees opened this issue Nov 19, 2014 · 18 comments
Closed

node.Name/minion.Name should not be used as an address #2462

anguslees opened this issue Nov 19, 2014 · 18 comments
Assignees
Labels
area/nodecontroller priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Milestone

Comments

@anguslees
Copy link
Member

According to my git grepping, the cloudprovider.Instances().IPAddress() function is only used when finding pod hosts (ie: pod.getInstanceIP()), not when finding minion addresses (ie: minion.ResourceLocation()) for healthchecking, etc.

The result is that my (openstack) cloudprovider is called to find minions and returns a list of "names". These names are neither literal IPs nor DNS names, so minion.ResourceLocation turns them into URLs that later fail to resolve, and the new minions never pass health checks.

I suggest minion.ResourceLocation should use code very much like pod.getInstanceIP to do name->address mapping via the cloudprovider plugin. This would presumably involve moving getInstanceIP out into a common library/singleton somewhere so both callers can benefit from a shared cache (see getInstanceIP).

@bgrant0607
Copy link
Member

/cc @ddysher

@ddysher
Copy link
Contributor

ddysher commented Nov 20, 2014

Thanks for the suggestion! We currently assume minion.Name can be resolved.. and we also do not respect the HostIP field, even if we say so (not 100% sure about this). I'll take a look.

// Queried from cloud provider, if available.
HostIP string `json:"hostIP,omitempty" yaml:"hostIP,omitempty"`

@goltermann goltermann added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Dec 3, 2014
@anguslees
Copy link
Member Author

I have a fix for this buried in #3171

I don't know that I've caught all places where the Name==endpoint assumption existed, but basic functionality seems to be working for me.

@brendandburns
Copy link
Contributor

We really need to have both external and internal IP in the cloud provider
interface. Currently for GCE it returns the external address which doesn't
work in a number of contexts.

--brendan

On Mon, Dec 29, 2014 at 9:50 PM, Angus Lees notifications@github.com
wrote:

I have a fix for this buried in #3171
#3171

I don't know that I've caught all places where the Name==endpoint
assumption existed, but basic functionality seems to be working for me.


Reply to this email directly or view it on GitHub
#2462 (comment)
.

@davidopp davidopp added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Feb 17, 2015
@roberthbailey roberthbailey added this to the v1.0 milestone Mar 4, 2015
@alex-mohr
Copy link
Contributor

Dup of now-fixed #4384

@anguslees
Copy link
Member Author

Alas, the bug is not yet fixed. We now have both internal and external addresses from the cloud provider, but those addresses are not being used in many places. For example, see code like: https://github.com/GoogleCloudPlatform/kubernetes/blob/a8f2cee8c5418676ee33a311fad57d6821d3d29a/pkg/registry/minion/rest.go#L147

@piosz piosz reopened this Mar 20, 2015
@ddysher
Copy link
Contributor

ddysher commented Mar 27, 2015

correct, it's not used right now. #4434 is one step toward the goal. Replacing node.Name with different ip address involves more than just a field replace though.

@ddysher ddysher removed their assignment Mar 27, 2015
@alex-mohr
Copy link
Contributor

CJ, I think you're closest to which IP address is being used? Also, hardening master<->kubelet communication might fix this as part of cert validation and proxy functions?

@alex-mohr
Copy link
Contributor

And can you please update title with actual remaining issues?

@roberthbailey roberthbailey added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Apr 27, 2015
@cjcullen cjcullen changed the title cloudprovider IPAddress() is not consulted for minion addresses node.Name/minion.Name should not be used as an address Apr 30, 2015
@anguslees
Copy link
Member Author

Btw (and apologies if this is obvious), when changing Name -> IP in URLs, we need to also use net.JoinHostPort() because some of those IP literals might be IPv6 and need special quoting. A simple git grep 'https\?://%s' shows several incorrect examples.

@cjcullen
Copy link
Member

Definitely was not obvious to me. Thank you for pointing that out.

@roberthbailey
Copy link
Contributor

We would also like an indicator of which node addresses are signed by the certificate on the node, so that we will be able to establish a secured TLS connection when contacting the node.

@roberthbailey
Copy link
Contributor

The code in master.go is part of the /validate endpoint, which @fabioy is ripping out as part of #7092.

@bgrant0607
Copy link
Member

cc @derekwaynecarr

@cjcullen
Copy link
Member

cjcullen commented Jun 3, 2015

#9155 removed the last place (that I could find) where we were using node.Name as an address.

@cjcullen cjcullen closed this as completed Jun 3, 2015
@bgrant0607 bgrant0607 reopened this Jun 4, 2015
@bgrant0607
Copy link
Member

#9155 was reverted.

Also see #9267

@alex-mohr
Copy link
Contributor

Closing #2462 via #9451

justinsb added a commit to justinsb/kubernetes that referenced this issue Sep 29, 2016
Most of our communications from apiserver -> nodes used
nodutil.GetNodeHostIP, but a few places didn't - and this
meant that the node name needed to be resolvable _and_ we needed
to populate valid IP addresses.

Fix the last few places that used the NodeName.

Issue kubernetes#18525
Issue kubernetes#9451
Issue kubernetes#9728
Issue kubernetes#17643
Issue kubernetes#11543
Issue kubernetes#22063
Issue kubernetes#2462
Issue kubernetes#22109
Issue kubernetes#22770
Issue kubernetes#32286
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/nodecontroller priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Projects
None yet
Development

No branches or pull requests

10 participants