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

Reverse DNS records for named headless services #25

Merged
merged 2 commits into from Jan 13, 2017
Merged

Reverse DNS records for named headless services #25

merged 2 commits into from Jan 13, 2017

Conversation

@sadlil
Copy link
Contributor

@sadlil sadlil commented Jan 11, 2017

Add reverse DNS records for pods' IPs to named headless services' with Pod Hosts FQDN (<podHostName>.<serviceName>.<namespace>.svc.cluster.local).

Original PR: kubernetes/kubernetes#38376

Resolves Issues

Spec:
Records for a Named Headless Service
Type: PTR
f a ready endpoint exists with IP equal to the IP of exists, and the endpoint has a non-empty hostname with value , then a PTR record with ...svc. will be returned.

@thockin @bowei @smarterclayton @harryge00

@thockin
Copy link
Member

@thockin thockin commented Jan 11, 2017

There is now a specification in this repo - can you also update that, please?

@sadlil
Copy link
Contributor Author

@sadlil sadlil commented Jan 11, 2017

@thockin i think 2.4.3 already says what this PR does.

@@ -323,6 +320,76 @@ func (kd *KubeDNS) handleEndpointAdd(obj interface{}) {
}
}

func (kd *KubeDNS) handleEndpointUpdate(oldObj, newObj interface{}) {
if o, ok := oldObj.(*v1.Endpoints); ok {
Copy link
Member

@bowei bowei Jan 11, 2017

Choose a reason for hiding this comment

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

short circuit these and log error and return if the type cast does not work. For longer scoped variables, I prefer more than one letter names.

func (kd *KubeDNS) handleEndpointUpdate(oldObj, newObj interface{}) {
  oldEndpoints, ok ;= oldObj.(*v1...)
  newEndpoints, ok := newObj.(*v1...)
  if oldEndpoints == nil { 
    glog.Errorf("oldObj was not of type v1.Endpoints)
    // etc
  }
// etc

}

func (kd *KubeDNS) handleEndpointDelete(obj interface{}) {
if e, ok := obj.(*v1.Endpoints); ok {
Copy link
Member

@bowei bowei Jan 11, 2017

Choose a reason for hiding this comment

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

short circuit log error and return (see comment above)

address := &e.Subsets[idx].Addresses[subIdx]
endpointIP := address.IP
if _, has := getHostname(address); has {
delete(kd.reverseRecordMap, endpointIP)
Copy link
Member

@bowei bowei Jan 11, 2017

Choose a reason for hiding this comment

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

Do you need to grab the lock to do this update?

// in new endpoint, or
// the addresses that are no longer named.
for k := range oldAddressMap {
delete(kd.reverseRecordMap, k)
Copy link
Member

@bowei bowei Jan 11, 2017

Choose a reason for hiding this comment

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

I think the mutex needs to be held for the deletion

@sadlil
Copy link
Contributor Author

@sadlil sadlil commented Jan 12, 2017

@bowei updated with locking and short circuit.

@reAsOn2010
Copy link

@reAsOn2010 reAsOn2010 commented Jan 12, 2017

@sadlil I used this PR for my HBase deployment, which is strongly relied on reverse DNS lookup. It worked as what I wanted. Hoping for merge and many thanks~

@thockin
Copy link
Member

@thockin thockin commented Jan 13, 2017

bowei
bowei approved these changes Jan 13, 2017
Copy link
Member

@bowei bowei left a comment

LGTM -- I will push a new dot release for you, and you can submit a PR against the main repo to integrate

@bowei bowei merged commit 3700f98 into kubernetes:master Jan 13, 2017
2 checks passed
@bowei
Copy link
Member

@bowei bowei commented Jan 13, 2017

@sadlil -- you will have to open a PR integrating kube-dns:1.11.0. Remember the release note.

@sadlil
Copy link
Contributor Author

@sadlil sadlil commented Jan 15, 2017

@tamalsaha tamalsaha deleted the reverse-dns branch Jan 15, 2017
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Jan 17, 2017
Automatic merge from submit-queue (batch tested with PRs 39911, 40002, 39969, 40012, 40009)

kubeadm: upgrade kube-dns to 1.11.0.

**What this PR does / why we need it**: See kubernetes/dns#25

**Which issue this PR fixes**: fixes kubernetes/kubeadm#121

**Special notes for your reviewer**: /cc @luxas
I know this is not the template solution you are looking for but seems to me it's important enough to do this now because of the issues it fixes.
Tested manually and it works.

`NONE`
func (kd *KubeDNS) handleEndpointDelete(obj interface{}) {
endpoints, ok := obj.(*v1.Endpoints)
if !ok {
glog.Errorf("obj type assertion failed! Expected 'v1.Endpoints', got %T", obj)
Copy link
Member

@kargakis kargakis Jan 17, 2017

Choose a reason for hiding this comment

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

You can get a tombstone which you could cast again and get the endpoint that was deleted out of it. See the ds delete resource handler for example: https://github.com/kubernetes/kubernetes/blob/f7305e6f4375a34cb6e2f47b6e164527370e8871/pkg/controller/daemon/daemoncontroller.go#L174

Copy link
Contributor Author

@sadlil sadlil Jan 17, 2017

Choose a reason for hiding this comment

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

The dns package didn't use the tombstone pattern. I tried to follow what was done here, and modified to short circuit after bowei told.
If you like i can send a pr implementing the tombstone pattern.

Copy link
Member

@kargakis kargakis Jan 17, 2017

Choose a reason for hiding this comment

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

@bowei @thockin is there any specific reason not to use tombstones?

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Jan 17, 2017
Automatic merge from submit-queue

Use kube-dns:1.11.0

Use [kube-dns:1.11.0](https://github.com/kubernetes/dns/releases/tag/1.11.0)

With: kubernetes/dns#25
Fixes #26752
Fixes #33470

@bowei @thockin
@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented May 29, 2017

How does this handle multiple endpoints all reporting the same IP with different hostnames? I did not see that in the code, but multiple services can represent the same pod, and there's nothing that prevents a caller from setting their own endpoints that have hostnames and use the pod. For example, a controller that copied endpoints to identify which pod is the "master" could easily have the hostname and IP the same as another service, and thus result in ordering of updates changing what is in the cache.

@thockin
Copy link
Member

@thockin thockin commented May 30, 2017

Oh snap! THIS was why we didn't do it in the first place - it requires us to confirm with the pod, and we don't want DNS watching every Pod.

We can really only have one reverse record per IP. Without checking that this is the same Service that pod elected as Subdomain, we can't do this safely.

Do we need to revert this? Sad because it will work sometimes...

@sadlil
Copy link
Contributor Author

@sadlil sadlil commented May 30, 2017

@thockin, @smarterclayton this problem can be avoided if we do a get Pod only when address.Hostname is set, and check for the Pod.Spec.Subdomain == svc.Name. Is this reasonable?
I can have a PR ready for this.

@sadlil sadlil mentioned this pull request May 30, 2017
@sadlil
Copy link
Contributor Author

@sadlil sadlil commented May 30, 2017

@thockin @smarterclayton i have created a sample PR #101 with address.Hostname is set, and check for the Pod.Spec.Subdomain == svc.Name solution.
To further reduce cost we may enforce a annotation over Headless service. service.beta.kubernetes.io/ptr: true. PTR records will only be generated if the annotation exists.

Let me know what you think.

@thockin
Copy link
Member

@thockin thockin commented Jul 7, 2017

@thockin thockin mentioned this pull request Jul 7, 2017
@chrislovecnm
Copy link
Member

@chrislovecnm chrislovecnm commented Sep 8, 2017

We are running into a nasty PTR issue.

Create package let's call it alpha. It will create pods named alpha-0.alpha.default.cluster.local. The default naming for a headless service and statefulset.

  1. Use a statefulset on hostnetwork, and nodeport
  2. Use a headless service with said set
  3. Use a nodeport service with set set

Basically one pod on one node all running on 9042. Dns is both in nodeport and headless services.

Now uninstall alpha. Update the manifest and rename the stateful set as beta. Install beta.

Now do reverse ip lookup, the PTR record is set to alpha-0 old pod name. Lions tigers and bears of my. This was a nasty one ;(

Basically with the manifest that I am running with 1.7.4 PTR records are getting set once and then never updated.

@thockin
Copy link
Member

@thockin thockin commented Sep 11, 2017

Yeah, I'm afraid PTR as implemented is pretty broken. I'm not sure what the right fix is. The proposed fix is pretty gnarly...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants