Node addresses #4434

ddysher · 2015-02-13T21:25:48Z

Deprecate Node.HostIP, use Node.Addresses.

This is mostly the api side of change. We need to update cloudprovider, etc. to differentiate different addresses.

I've aggressively removed HostIP from v1beta1/beta2. It causes troubles for conversions. Node is generally created from node controller, so I'd expect the change to have minimal user impact. If this is not desirable, we can leave it there and do the conversion of Node.HostIP <-> Node.Addresses.

@bgrant0607 @smarterclayton

bgrant0607 · 2015-02-14T06:09:13Z

pkg/api/types.go

 	// NodePhase is the current lifecycle phase of the node.
 	Phase NodePhase `json:"phase,omitempty"`
 	// Conditions is an array of current node conditions.
 	Conditions []NodeCondition `json:"conditions,omitempty"`
+	// Queried from cloud provider, if available.
+	Addresses []NodeAddress `json:"addresses,omitempty"`


Everywhere else in the API, we use "IP" rather than "Address". We should stick to that convention.

Is this convention strictly enforced? Anything breaks if we change to address?

The reason to change ip to address is apparent, we want to support hostname too. We can change back to IPs and use a separate new list, but that seems a little redundant. See below comment.

It's not enforced, but I'd like as much consistency as possible in v1beta3.

At the moment, is it possible for this list to contain hostnames? Besides, I don't think "Address" is appropriate for a hostname.

Normally I would use "Host", but that doesn't work here. Maybe "Address" is the lesser evil.

@thockin Any preference re. the term we use to refer to something that may be either an IP address or hostname?

yeah, the list can contain hostnames, and that's the intent.

How about AlternateEndpoints -- so, including port?

We've discussed allowing hosts in Endpoints, in #4370.

I want Nodes to actually populate an Endpoints object, as discussed in #4440.

Endpoints will need to be able to carry various information (e.g., internal vs. external), as discussed in #4482 and elsewhere.

Why Alternative in the Name? Do we have a primary Endpoint or Address?

Endpoints is more or less a concept in our service world now, do we want to overload it here? It's a little confusing, plus I don't know how useful it would be to have port. Whoever consumes the list should know the port, like api-server proxy.

I think what we need here is a list of equivalent address (external ip, internal ip, hostname). If the direction is to produce Endpoints in Node, then I'll need to take a deep look at how it might work.

Ok, Addresses is fine, at least for now. I don't have a better alternative.

BTW, I do want to replace ResourceLocation with node and pod endpoints, but the endpoints wouldn't appear here in that form, regardless.

ddysher · 2015-02-17T22:26:41Z

Thanks for the review. I'll update the code after we agree with the API change.

googlebot · 2015-02-17T22:26:43Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

justinsb · 2015-02-26T22:13:50Z

Thanks for making this patch. I've used it as the basis for my AWS work (justinsb@0f43e6a), so hopefully we can get this merged soon :-)

I agree with the comments. One thought on the legacy host IP enum: perhaps we should not add that to the public API. We can still populate the HostIP for v1beta1 and v1beta2 by looking for the "best" amongst the addresses returned: I would guess hostname > external ip > internal ip. That seems to be at least as good as the current state of things.

justinsb · 2015-03-03T17:07:07Z

I'd love to see this patch make progress ... the rebases are proving painful!

@bgrant0607 If we make the proposed changes, are you happy with the idea of a list of "NodeAddress" objects with a enum-Kind and a string Value? And we can remove HostIP from v3? We can populate HostIP by simply taking the first address value when there is no "LegacyHostIp" enum value (because the cloud providers will initially just return a single address), until we rework the cloud providers to populate Addresses fully. I do think we should avoid returning that value to clients though, as we don't want them depending on it.

@ddysher Can I help in any way? I'm happy to fork your PR and go through the iterations if you'd rather I did that, but I don't want to "steal your PR" unless you want me to :-)

ddysher · 2015-03-03T17:25:43Z

@justinlindh I'll work on it today :) Sorry I was distracted by something else.

Personally, I think NodeAddress with enum-Kind and string Value makes sense for now. I'll see what @bgrant0607 have in mind. We will remove HostIP from v3. Agreed the legacy ip probably shouldn't appear in public API, but i'm a little concerned to just use the 'first address' as we may have different assumption everywhere. I'll revisit t the PR.

justinsb · 2015-03-03T19:02:37Z

Thanks - I really appreciate it. If there is anything I can do to help, just ping me (I'm @justinsb BTW)

googlebot · 2015-03-03T21:08:16Z

CLAs look good, thanks!

ddysher · 2015-03-03T21:24:20Z

doh. I know you are @justinsb. I'm surpirsed github cant inference name for me.

Some changes:

Removed LegacyHostIP from public IP. Only internal version needs it.
During conversion from external to internal version, if HostIP is set, then we add a new entry in NodeAddresses; if not, just ignore it (in which case user should be using api.NodeInternalIP, etc)
During conversion from internal to external version, set HostIP to NodeAddress[api.NodeLegacyIP] if any.
Before we drop suport for v1beta1/2, pod cache will pick up LegacyHostIP first, then internal ip, then external ip.

bgrant0607 · 2015-03-03T21:34:18Z

pkg/api/v1beta1/types.go

+
+type NodeAddress struct {
+	Type  NodeAddressType `json:"type" description:"type of the node address, e.g. external ip, internal ip, hostname, etc"`
+	Value string          `json:"value" description:"string representation of the address"`


I'd use Address rather than Value.

bgrant0607 · 2015-03-03T21:47:04Z

Pretty close. Please address comments.

bgrant0607 · 2015-03-03T23:27:05Z

Please fix the build failure.

ddysher · 2015-03-04T00:00:09Z

Rebased. merge error from #4994 (method name conflict).

bgrant0607 · 2015-03-05T06:31:50Z

LGTM

Node addresses

saad-ali assigned bgrant0607 Feb 13, 2015

bgrant0607 reviewed Feb 14, 2015
View reviewed changes

smarterclayton mentioned this pull request Feb 18, 2015

buildlogs: dont redirect to host but to IP instead openshift/origin#1057

Closed

ddysher mentioned this pull request Feb 19, 2015

IPAddresses reported by the Cloud Providers are ignored #4384

Closed

googlebot added the cla: no label Mar 3, 2015

ddysher force-pushed the node-hostip branch from 7a2dc9c to 2ab2497 Compare March 3, 2015 21:08

googlebot added cla: yes and removed cla: no labels Mar 3, 2015

ddysher force-pushed the node-hostip branch 2 times, most recently from ce87fed to 48d0929 Compare March 3, 2015 21:23

bgrant0607 reviewed Mar 3, 2015
View reviewed changes

ddysher force-pushed the node-hostip branch from 48d0929 to 6783e47 Compare March 3, 2015 23:20

ddysher force-pushed the node-hostip branch from 6783e47 to 75caffc Compare March 3, 2015 23:36

ddysher force-pushed the node-hostip branch from 75caffc to d690ab9 Compare March 4, 2015 18:12

Node addresses

9b0af8d

ddysher force-pushed the node-hostip branch from d690ab9 to 9b0af8d Compare March 4, 2015 21:47

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2015

bgrant0607 added a commit that referenced this pull request Mar 5, 2015

Merge pull request #4434 from ddysher/node-hostip

9fcb48c

Node addresses

bgrant0607 merged commit 9fcb48c into kubernetes:master Mar 5, 2015

ddysher mentioned this pull request Mar 27, 2015

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

Closed

ddysher deleted the node-hostip branch March 13, 2016 13:36

anguslees mentioned this pull request May 18, 2020

KEP-1664: Better Support for Dual-Stack Node Addresses kubernetes/enhancements#1665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node addresses #4434

Node addresses #4434

ddysher commented Feb 13, 2015

bgrant0607 Feb 14, 2015

ddysher Feb 17, 2015

bgrant0607 Feb 19, 2015

bgrant0607 Feb 19, 2015

ddysher Feb 19, 2015

bgrant0607 Feb 20, 2015

ddysher Feb 24, 2015

bgrant0607 Mar 3, 2015

bgrant0607 Mar 3, 2015

ddysher commented Feb 17, 2015

googlebot commented Feb 17, 2015

justinsb commented Feb 26, 2015

justinsb commented Mar 3, 2015

ddysher commented Mar 3, 2015

justinsb commented Mar 3, 2015

googlebot commented Mar 3, 2015

ddysher commented Mar 3, 2015

bgrant0607 Mar 3, 2015

bgrant0607 commented Mar 3, 2015

bgrant0607 commented Mar 3, 2015

ddysher commented Mar 4, 2015

bgrant0607 commented Mar 5, 2015

Node addresses #4434

Node addresses #4434

Conversation

ddysher commented Feb 13, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher commented Feb 17, 2015

googlebot commented Feb 17, 2015

justinsb commented Feb 26, 2015

justinsb commented Mar 3, 2015

ddysher commented Mar 3, 2015

justinsb commented Mar 3, 2015

googlebot commented Mar 3, 2015

ddysher commented Mar 3, 2015

Choose a reason for hiding this comment

bgrant0607 commented Mar 3, 2015

bgrant0607 commented Mar 3, 2015

ddysher commented Mar 4, 2015

bgrant0607 commented Mar 5, 2015