-
Notifications
You must be signed in to change notification settings - Fork 468
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
fix pod hostname to be podname for dns srv records #232
Conversation
/assign @thockin |
/assign @bowei |
I don't think this is right - TargetRef is almost always Pod, this would break all the functionality around pod.spec.hostname ? |
@thockin any other suggestions ? If hostname is not empty we use hostname else use podname , would that help ? I am open to suggestions as i am not aware of how the hostname is used but its completely broken when create a deployment with three instances, all of them get the same hostname and i cant resolve their ips using their dns names. |
@thockin addressed comments PTAL |
This does not seem to address the problem:
You don't handle that case any differently than before? |
@thockin that is correct. I improved the situation from before in the case, where if hostname is not specified, we will use the pod name, but if hostname is still there in the pod, we will use that. Earlier all pods of the same deployment would get the same dns entries, and they would resolve to just one of the pods IP, if they had a hostname specified and if no hostname was specified, they wont get any dns entry. |
Sorry, I am very confused as to which problem you are solving now. A headless service with 3 backends will get 3 responses to the service's DNS name. This is correct behavior.
If you add a hostname to the pod, you still get 3
Can you clarify what you're trying to solve again? |
@thockin curious was you service pointing to a Deployment or a StatefulSet. From the hostname it seems you were indeed pointing to Deployment, but just want to confirm. I am using kube-dns
Let me know if that clarifies. I should have put all this in the description though. |
Ahh, #116 is all about not reporting multiple IPs when the same hostname is used, not about using the pod's name. so this doesn't fix #116 at all. It changes behavior to use the pod name rather than an anonymous name. What is the use case for that? If the intent is to fix #116, we need to handle duplicates explicitly |
@thockin in #116 one of the example , the author of the issue mentions is the following, which says that the when resolving the headless service, only one A record is returned. This PR fixes that problem.
Also see kubernetes/kubernetes#47992 and the similar changes in coredns here https://github.com/coredns/coredns/pull/1190/files
Can you elaborate ? the pod names are always different. Do you want to explicitly check if the pod names are not colliding ? |
You're not handling the case of hostname being set at all. You're also not handling bogus The bugs effectively say 2 things:
As far as I can see you are only addressing point 1, right? That seems like the less important issue of the two. |
@thockin yes i am only addressing 1 so far. I am happy to update the dns specification. @johnbelamaric how is this supported in CoreDns. This fixes dns resolution of pod name and reverse lookups. Regarding not handling bogus TargetRef.Name collisions, i am having a hard time thinking about the case when TargetRef.Name will collide ? Even though a Service can span across deployment objects, within a namespace they will never collide. Can a Service selector span across namespaces ? Also yes a Service can span across Pods created manually which can lead to collision, but no one uses Pods without a workload controller For handling case 2(hostname specified), i am not sure why the hostname was introduced at all ? It seems it would only help when the pod is created on its own without using ReplicaSet or Deployment which is hardly ever the case. Should we instead , use the dashed form of ip address as the name of the A record when hostname is specified or even always . CoreDns in this PR does exactly that https://github.com/coredns/coredns/pull/1190/files. Any other suggestions ? |
@thockin i am happy to discuss on hangout if that will help us move faster . |
Ping @thockin |
I believe this works in CoreDNS when you specify the option Headless service with
Headless service with
With
Finally, with
For the endpoint names, the spec just says it is the
So, So, rather than updating |
Pod names are not supposed to be resolvable. Making them reverse-resolvable is not a good thing, IMO. What is the use-case? The pod name is non-deterministic outside of a StatefulSet or manual control, so forward lookups are unlikely to be helpful. I understand the desire for reverse lookups. What if we simply returned the Service name in this case? E.g. given:
if hostname and domainname were set:
if subdomain was set, but hostname was not:
for completeness, if neither subdomain nor hostname were set:
(and even this we could maybe fix by returning
Remember that Endpoints should not be "trusted" too far. Users can manually set Endpoints in some cases.
Don't think about polite users. Think about trouble makers.
StatefulSet TL;DR: If a pod specifies Before we review code, we should get agreement on a change to the DNS spec. |
I'm still very game to fix the bugs here, but throwing a hold on it for now. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@krmayankk thoughts on what's next for this? |
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@fejta-bot: Closed this PR. In response to this:
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. |
Fixes #116 and kubernetes/kubernetes#47992
Verified this works now