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.

Sign up for GitHub

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

Jump to bottom

Use nodeutil.GetHostIP consistently when talking to nodes #33718

Merged
merged 2 commits into from
Oct 10, 2016
Merged

Use nodeutil.GetHostIP consistently when talking to nodes #33718

merged 2 commits into from
Oct 10, 2016

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Sep 29, 2016 •
edited by liggitt
Loading

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.

The apiserver now uses addresses reported by the kubelet in the Node object's status for apiserver->kubelet communications, rather than the name of the Node object.

This change is Reviewable

@justinsb
Refactor: separate KubeletClient & ConnectionInfoGetter concepts
7381e9a 
KubeletClient implements ConnectionInfoGetter, but it is not a complete
implementation: it does not set the kubelet port from the node record,
for example.

By renaming the method so that it does not implement the interface, we
are able to cleanly see where the "raw" GetConnectionInfo is used (it is
correct) and also have go type-checking enforce this for us.
@justinsb justinsb changed the title Use nodeutil.GetHostIP consistently when talking to nodes WIP: Use nodeutil.GetHostIP consistently when talking to nodes Sep 29, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 29, 2016
@justinsb justinsb changed the title WIP: Use nodeutil.GetHostIP consistently when talking to nodes Use nodeutil.GetHostIP consistently when talking to nodes Sep 29, 2016
@justinsb
Copy link
Member Author

The first commit is #29011 , which was LGTM-ed but needed a rebase.

This works in my testing on AWS and passes e2e above (though we are still using NodeNames that are resolvable, so this may not be complete - but still a step in the right direction IMO!)

@justinsb
Copy link
Member Author

(removed WIP)

@justinsb
Use nodeutil.GetHostIP consistently when talking to nodes
8fe884a 
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
@justinsb justinsb mentioned this pull request Sep 29, 2016
Closed
@justinsb
Copy link
Member Author

Issue #18525
Issue #9451
Issue #9728
Issue #17643
Issue #11543
Issue #22063
Issue #2462
Issue #22109
Issue #22770
Issue #32286

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 8fe884a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@justinsb
Copy link
Member Author

@k8s-bot gke e2e test this

@k8s-github-robot k8s-github-robot mentioned this pull request Sep 29, 2016
Closed
@justinsb justinsb mentioned this pull request Sep 30, 2016
Merged
@liggitt
Copy link
Member

liggitt commented Sep 30, 2016 •
edited
Loading

switching from DNS to IP for master->kubelet connections has implications for deployments that validate the kubelet's TLS serving certs. I am not aware of merge-queue tests that run with the master started with a --kubelet-certificate-authority flag, so don't assume that green tests mean this will not break existing deployments.

This adds the requirement that the kubelet's serving certificate be valid for the node IP, which may or may not be feasible for deployments, depending on how they provision the certificates.

I also don't agree that "Most of our communications from apiserver -> nodes used nodeutil.GetNodeHostIP"... only the node proxy API (/proxy/nodes/node/...) uses that. Other APIs (notably pod logs/exec/attach) use the nodeName as DNS, and are far more widely used.

I'd like to hold merging this until we resolve whether DNS or IP should be preferred, and how we want to give control to deployers if either method does not work for them (no viable DNS or no serving certs valid for IP)

c.f. #25532 for using a reported hostname to connect via DNS instead of assuming the node name is a resolvable DNS name

@justinsb
Copy link
Member Author

justinsb commented Oct 4, 2016

We have decided though: #22063 (comment)

@bgrant0607 can you confirm?

@liggitt
Copy link
Member

liggitt commented Oct 4, 2016 •
edited
Loading

We have decided though: #22063 (comment)

@bgrant0607 can you confirm?

I don't disagree that decoupling the resolution from the nodeName is desired. I'm just looking to keep continuity for clusters that are currently working for people using pod logs/exec/attach, while giving deployers the tools they need to decouple as appropriate to their environment. #25532 seems closer to achieving that goal.

@bgrant0607
Copy link
Member

ref #2462

@bgrant0607
Copy link
Member

@liggitt This and #25532 don't seem incompatible. Applying type checking to nodeName seems useful in order to avoid future regressions. We have time before cutting 1.5.

cc @cjcullen

This was referenced Oct 6, 2016
Closed
Merged
@bgrant0607 bgrant0607 assigned liggitt and unassigned wojtek-t Oct 7, 2016
@bgrant0607
Copy link
Member

@liggitt What changes does this PR need in order to be merged?

@liggitt
Copy link
Member

liggitt commented Oct 8, 2016 •
edited
Loading

@liggitt What changes does this PR need in order to be merged?

if there's general agreement to record more types of addresses in node status (#25532 or #34259 or similar) and to allow selecting which address types to use for apiserver->kubelet communications (#34259 or similar) by 1.5, this change to temporarily require connecting using node IP seems ok as-is

@liggitt liggitt added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 10, 2016
@liggitt
Copy link
Member

liggitt commented Oct 10, 2016

LGTM, will follow up with #25532 and #34259

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ff20b17 into kubernetes:master Oct 10, 2016
This was referenced Oct 10, 2016
Merged
Merged
@kkirsche
Copy link

With this PR merged, does limitation 3 on https://github.com/kubernetes/kubernetes.github.io/blob/master/docs/getting-started-guides/kubeadm.md#limitations need to be removed?

@resouer resouer mentioned this pull request Oct 13, 2016
Closed
@resouer
Copy link
Contributor

resouer commented Oct 13, 2016

@liggitt Have we cherry picked this into latest release? Some people are asking for this.

ref: kubernetes/website#1453

@liggitt
Copy link
Member

liggitt commented Oct 13, 2016

@liggitt Have we cherry picked this into latest release? Some people are asking for this.

No, picking just this into 1.4 could break clusters that are currently working with secured node names as DNS names. More changes are needed in the kubelet and apiserver to allow both DNS-based and IP-based modes of operation.

@luxas
Copy link
Member

luxas commented Oct 15, 2016

With this PR merged, does limitation 3 on https://github.com/kubernetes/kubernetes.github.io/blob/master/docs/getting-started-guides/kubeadm.md#limitations need to be removed?

ping @justinsb Does this PR fix that issue?

@kkirsche We can't remove it until v1.5 is released (which has this patch)

@kkirsche
Copy link

@luxas noted. Thank you for answering and clarifying that. I wasn't sure if hot fix / cherry picking was ever done for that type of thing. Thanks for everyone'a hard work!

k8s-github-robot pushed a commit that referenced this pull request Oct 18, 2016
Merge pull request #34474 from liggitt/connection-info-refactor
f39e86c 
Automatic merge from submit-queue

Remove static kubelet client, refactor ConnectionInfoGetter

Follow up to #33718

* Collapses the multi-valued return to a `ConnectionInfo` struct
* Removes the "raw" connection info method and interface, since it was only used in a single non-test location (by the "real" connection info method)
* Disentangles the node REST object from being a ConnectionInfoProvider itself by extracting an implementation of ConnectionInfoProvider that takes a node (using a provided NodeGetter) and determines ConnectionInfo
* Plumbs the KubeletClientConfig to the point where we construct the helper object that combines the config and the node lookup. I anticipate adding a preference order for choosing an address type in #34259
This was referenced Oct 25, 2016
Merged
Closed
@justinsb justinsb mentioned this pull request Oct 28, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Nov 1, 2016
Merge pull request #35497 from liggitt/node-address
d461262 
Automatic merge from submit-queue

Allow apiserver to choose preferred kubelet address type

Follow up to #33718 to stay compatible with clusters using DNS names for master->node communications. Adds the `--kubelet-preferred-address-types` apiserver flag for clusters that prefer a different node address type.

```release-note
The apiserver can now select which type of kubelet-reported address to use for master->node communications, using the --kubelet-preferred-address-types flag.
```
@davidchua davidchua mentioned this pull request Nov 1, 2016
Closed
@roeyazroel
Copy link

Hi,

Which version include GetNodeHostIP fix?
I'm using the following:

Client Version: version.Info{Major:"1", Minor:"4", GitVersion:"v1.4.5", GitCommit:"5a0a696437ad35c133c0c8493f7e9d22b0f9b81b", GitTreeState:"clean", BuildDate:"2016-10-29T01:38:40Z", GoVersion:"go1.6.3", Compiler:"gc", Platform:"linux/amd64"}

Server Version: version.Info{Major:"1", Minor:"4", GitVersion:"v1.4.5", GitCommit:"5a0a696437ad35c133c0c8493f7e9d22b0f9b81b", GitTreeState:"clean", BuildDate:"2016-10-29T01:32:42Z", GoVersion:"go1.6.3", Compiler:"gc", Platform:"linux/amd64"}

Thanks,
Roey

@dkoshkin
Copy link
Contributor

dkoshkin commented Nov 9, 2016

Looks like this was merged in v1.5.0-alpha.1 https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG.md#v150-alpha1, I'm using that version and no longer see this issue.

@colemickens colemickens mentioned this pull request Nov 16, 2016
Closed
@abrarshivani abrarshivani mentioned this pull request Dec 22, 2016
Open
1 task
@colemickens colemickens mentioned this pull request Feb 18, 2017
Closed
@spiffxp spiffxp mentioned this pull request Jun 15, 2017
Closed
@benmoss benmoss mentioned this pull request Jun 20, 2019
Closed
@Sicaine Sicaine mentioned this pull request May 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet