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

Add a Hostname field to the EndpointAddress object. #11838

Closed
wants to merge 1 commit into from

Conversation

brendandburns
Copy link
Contributor

This will be used for DNS CName services, and also using DNS to lookup addresses for Headless services.

Related to #4447

@thockin @bgrant0607 @lavalamp

This will be used for DNS CName services.
@k8s-bot
Copy link

k8s-bot commented Jul 24, 2015

GCE e2e build/test passed for commit cde577e.

IP string `json:"ip" description:"IP address of the endpoint"`

// Hostname for this endpoint, you can specify IP or Hostname, but not both.
Hostname string `json:"hostname" description:"Hostname for this endpoint"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can sorta see what you're trying to do here, but the details seem very murky. Do you anticipate the endpoints controller ever setting this field? If so, how? Don't we need to solve external DNS before we'll have something reasonable to put here? If not, should this really go here?

Is Hostname the right field name? How about "DNSName" or "DNS"?

Why not combine this with the IP field? Do we really need two fields?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding one field vs two, this is a question we need to solve in a bunch of places. We were originally going to make IP fields use a net.IP-derived type for strong validation, but we backed off when we realized some places really do want host or IP. So in my mind this is still an open question. This sets precedent - we either use twin fields all over or we document these all as being "ip or hostname" and stick with 'string'. I have not put much thought into this, but I guess it doesn't matter much either way.

@bgrant0607 for API call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we set both IP and Hostname, or just OneOf? If OneOf, then this is a breaking API change. If we overloaded the existing field, that would also be a breaking API change.

We do have both IP and Hostname in LoadBalancerIngress. If users won't typically care about the difference and we wouldn't provide both, then a single Host field would be better. However, if we would provide both IP and DNS name and/or clients would need to use them differently or in different situations, then two fields would be best.

With respect to the use of "Host" in the API: We use Host in a number of places: HostPort, HostIP, HostNetwork, HostPath, Host (in HTTPGetAction and EventSource), and Hostname. More often than Node (not counting the Node API itself): NodeSelector, NodeName, NodePort. Users have complained about the inconsistency. However, I agree that Hostname makes sense in this case, and is consistent with the other use in the API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we set both IP and Hostname, or just OneOf?

In most places we really want one-of. Yeah, making a previously required field suddenly be optional will potentially screw up clients who expect a field to be populated. Changing it to be a hostname or IP is probably less likely to do damage, but it is sketchy to change.

LoadBalancerIngress was thrown together quickly.

HostPort and HostNet are named in equivalence with Docker. One could argue it's a matter of perspective - Node is what we call a machine from outside that machine, Host is what we call it from within a container. That's a stretch.

In this context though "hostname" (and HTTPGet.Host) means the name-or-IP of a host somewhere, and has nothing to do with kubernetes nodes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs omitempty. Otherwise this field is required.

@thockin
Copy link
Member

thockin commented Jul 25, 2015

This needs validation to enforce the one-of property, if we keep twin fields.

This must never have been run at all because current validation checks if !util.IsValidIPv4(address.IP) {

We need to define the use case for this:

  • Do we expect to be able to mix hostnames and IPs in a single Endpoints?
  • Is it OK if .hostname expands to multiple A records? Or do we expect this to simply be a 1:1 name import. That sort of overlaps with my hoped-for extensions to make naming a first-class thing - could this whole feature be replaced by a Name object which provides an internal alias to an external thing?
  • Do we cache the hostname-to-IP lookup? Nothing in here defeats the cache, if there is one in Go's library.
  • What is our DNS server supposed to do with these?

Does kube-proxy just work with this? I just took a look, and I guess it does. This will break when we switch to pure iptables, which is O(soon) @BenTheElder

Needs some sort of e2e testing

@bgrant0607
Copy link
Member

@brendandburns What exactly did you intend to use this for?

@BenTheElder
Copy link
Member

@thockin BTW: we have a few things to sort out with the iptables version but it's been mostly working fine seemingly for a while now. I have one possible problem reported here and am still working on this (deterministic rule names and reconciling against iptables-save).

I have summer finals for my two classes Monday and Tuesday so I am and will be studying, but Tuesday afternoon I will be back to resolving the remaining issues. I'll keep an eye of this, and when/if it has been clarified & merged I'll update kube-proxy to handle it.

That said, looking at it now: I would also like to know as you asked if we are going to allow mixing hostnames and IPs in single Endpoints, if the records will be 1:1, etc, so I know what kind of changes will need to be made to endpoint tracking and rule generation.

Further, will we allow updating of hostname records in the DNS service and if so: if the DNS service is updated will there be an endpoints event, or will we need to periodically re-resolve hostnames in the proxy or...?

@bgrant0607
Copy link
Member

That reminds me: We'd need to resolve the hostnames to ensure they didn't resolve to link-local addresses.

@thockin
Copy link
Member

thockin commented Jul 27, 2015

If we intend to enforce non-link-localness of these, we'll need to query
upstream DNS, expand THAT into the Endpoints object, note the TTL and
schedule a refresh (in endpoint controller, I guess) - lather rinse
repeat. And if ever the upstream DNS changes to be a link-local name, fail
in an obscure way. Yick.

On Sun, Jul 26, 2015 at 8:08 PM, Brian Grant notifications@github.com
wrote:

That reminds me: We'd need to resolve the hostnames to ensure they didn't
resolve to link-local addresses.


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

@lavalamp lavalamp assigned thockin and unassigned lavalamp Jul 27, 2015
@lavalamp
Copy link
Member

@thockin can you take this one?

@thockin
Copy link
Member

thockin commented Jul 27, 2015

ACK

On Mon, Jul 27, 2015 at 2:43 PM, Daniel Smith notifications@github.com
wrote:

@thockin https://github.com/thockin can you take this one?


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

@brendandburns
Copy link
Contributor Author

@bgrant0607 I want to enable CName based services.

So that I can have a headless service that maps to a hostname, and results in a CName in the Kube DNS server.

I can lay that in at a higher level, if we'd prefer, by introducing a new "DNSDelegate" ServiceType.

That is less breaking, I guess.

--brendan

@bgrant0607
Copy link
Member

You want a Kubernetes service to target a public CNAME, or you want a public CNAME to target a Kubernetes service?

A "headless" service is one with no ClusterIP. It exposes the pod IPs directly.

An imported service is one with no selector that targets a manually populated Endpoints resource.

@brendandburns
Copy link
Contributor Author

I guess it's "imported service" then. (I can never seem to keep the different names straight)

I want a service that targets a public CNAME. As a concrete example of this, AWS ELB load balancers are only exposed as hostnames, never as reliable IP addresses.

--brendan

@brendandburns
Copy link
Contributor Author

@thockin @bgrant0607
Friendly ping on this.

@bgrant0607
Copy link
Member

If the target were specified as a hostname, but the endpoints controller did the DNS lookup and updated the IP field, then clients that expected the IP field would still work (except for maybe the short duration where that field would be unset).

Do we need to support hostnames that resolve to multiple IP addresses? Or would it be acceptable for clients not aware of the hostname field to just target one IP?

@brendandburns
Copy link
Contributor Author

I think we can actually do this on admissions control, so IP will never not be set.

And yes, only using a single IP for hosts with multiple IP seems reasonable to me.

@bgrant0607
Copy link
Member

I think what we agreed then is that if Hostname is set, it will overwrite IP with a resolved address, regardless whether that field was set or not. Can we do this resolution synchronously upon creation? Then consumers could rely on IP to always be set.

@brendandburns
Copy link
Contributor Author

@bgrant0607
yes, that's why I proposed doing it in Admissions Control, that should be synchronous on creation, right?

@bgrant0607
Copy link
Member

Sorry, I somehow missed that comment.

Admission control is not the right place for this. That's where optional, pluggable policy goes.

Resolution would need to go into the Endpoints registry.

@brendandburns
Copy link
Contributor Author

@bgrant0607
Ok, I can add that to the endpoints registry. Do you want me to do that as part of this PR?

@smarterclayton
Copy link
Contributor

What timeout would we use for synchronous resolution? Has the potential to increase latency of endpoint updates significantly. Does it only update when IP is unset?

@brendandburns
Copy link
Contributor Author

@smarterclayton, yeah it would only update when IP is unset. And it wouldn't attempt to re-resolve, it's really just to make legacy tools continue to work (ish) future tools should be re-tooled to prefer hostname.

@smarterclayton
Copy link
Contributor

Sgtm. What about resolution timeout?

On Aug 4, 2015, at 6:34 PM, Brendan Burns notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton, yeah it would only
update when IP is unset. And it wouldn't attempt to re-resolve, it's really
just to make legacy tools continue to work (ish) future tools should be
re-tooled to prefer hostname.


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

@bgrant0607
Copy link
Member

Seems like the timeout would have to be less than the overall API timeout so that we'd have the opportunity to fail the API request in the case that resolution failed.

@bgrant0607
Copy link
Member

Go ahead and put the implementation in this PR as a separate commit. We don't have a good way of splitting API changes across multiple PRs such that we can ensure the changes aren't visible and aren't persisted to etcd. That's discussed here: #4855 (comment)

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 27, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 1, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/S

@thockin
Copy link
Member

thockin commented Sep 2, 2015

I didn't follow the final negotiation on this, but I'm not keen on never re-resolving. I'm not sure where this PR stands

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2015
@smarterclayton
Copy link
Contributor

I really wish we could implement this as a distinct object - ExternalDNSEndpoints type that a controller reads.

@smarterclayton
Copy link
Contributor

I'd prefer to implement #13748 which is think is strictly superior to this proposal (in that it has a wider solution space).

@bgrant0607
Copy link
Member

ref #13358

@k8s-bot
Copy link

k8s-bot commented Sep 23, 2015

GCE e2e build/test failed for commit cde577e.

@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 9, 2015
@thuey
Copy link

thuey commented Oct 16, 2015

Are there any updates on this Pull Request? We really need it or a similar solution to be able to point a mysql service at an AWS RDS instance. The IP address changes, so we can't just set the IP address of the endpoint. We have to use the DNS name. I tried to follow the conversation, but I apologize that a lot of it was over my head. If this is infeasible for some reason, is there a good work around?

@thockin
Copy link
Member

thockin commented Oct 16, 2015

I think the status is that we should either have a way of exposing arbitrary CNAMEs into kube-dns or we should allow a new form of Service that does not have a selector but instead a hostname, which kube-dns would act on by creating a CNAME record. Or both.

The alias-service seems simple but its another step down an ugly path of making Service more complicated and polymorphic.

Neither solution is in for v1.1, sadly.

@smarterclayton
Copy link
Contributor

As soon as we expose arbitrary Cnames we have to worry about collisions and
name "stealing". The joys of security...

On Oct 16, 2015, at 1:49 PM, Tim Hockin notifications@github.com wrote:

I think the status is that we should either have a way of exposing
arbitrary CNAMEs into kube-dns or we should allow a new form of Service
that does not have a selector but instead a hostname, which kube-dns would
act on by creating a CNAME record. Or both.

The alias-service seems simple but its another step down an ugly path of
making Service more complicated and polymorphic.

Neither solution is in for v1.1, sadly.


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

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e build/test failed for commit cde577e.

@brendandburns
Copy link
Contributor Author

@thockin I agree with @smarterclayton can we please re-activate this PR?

@brendandburns
Copy link
Contributor Author

and I'm also ok with implementing this in the style that @smarterclayton suggests, by having kube-dns load in a CNAME for the service, rather than resolving this at create time.

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit cde577e.

@mward29
Copy link

mward29 commented Jan 16, 2016

+1 As an end user of Kubernetes we are eagerly awaiting something like what was proposed. We use AWS resources that are often represented by a DNS entry. My first thought was adding this manually to an endpoint as the author suggests but I'm game for any solution that works.

@thockin
Copy link
Member

thockin commented Jan 17, 2016

Closing this in favor of #13748 - we don't need 2 places to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet