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

fix bug with delete externalName service #210

Merged

Conversation

sergeylanzman
Copy link
Contributor

On delete service from namespace kube-dns get event EndpointAdd.
Func addDNSUsingEndpoints generate RecordsForHeadlessService in cache to service type externalName.
After this delete from a cache, it's deleted an empty record, but a record of Entries for externalName stuck forever.
Solution: don't generate the empty record for externalName

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 8, 2018
@BenTheElder
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 9, 2018
@sergeylanzman
Copy link
Contributor Author

@BenTheElder travis-ci broked in the master branch.
Fix in #206.

@sergeylanzman
Copy link
Contributor Author

@BenTheElder Any updated?

@BenTheElder
Copy link
Member

@sergeylanzman I'm not the right person to review this PR, but Prow tests are passing at least. To get Travis to re-run I think you can close / reopen it. We have /retest and /test foo for Prow but I don't think Travis allows anyone besides the repo owner to reschedule testing directly.

@BenTheElder
Copy link
Member

/assign @MrHohn

@sergeylanzman
Copy link
Contributor Author

/retest

Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, this makes sense. According to the specification and doc on kubernetes.io, endpoints controller does not create Endpoints records for externalName service (users might be able to be that should have no effect) and such service only gets a CNAME record.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2018
@MrHohn MrHohn merged commit 22ca58c into kubernetes:master Mar 25, 2018
@sergeylanzman
Copy link
Contributor Author

@MrHohn what about release 1.14.9? I need this fix, today I can't delete externalname service

@MrHohn
Copy link
Member

MrHohn commented Mar 25, 2018

@sergeylanzman We are planing on releasing 1.14.9 ~next week.

While I agree we shouldn't generate unnecessary record for extenalName service, it is not quite clear to me how you have hit this. "On delete service from namespace kube-dns get event EndpointAdd" sounds a bit odd, deleting a service shouldn't have any endpoint to be created, or do you have some custom logic that triggers this?

@sergeylanzman
Copy link
Contributor Author

sergeylanzman commented Mar 25, 2018

Step to reproduce

  1. Create deployment + 1 pod
  2. Create clusterIP service with a selector for pod deployment
  3. Delete clusterIP service
  4. Create externalName service with the same name as clusterIP service(replace service)
  5. Delete externaName service and create again clusterIP

I continue to receive DNS resolve with externalName CName value

@MrHohn
Copy link
Member

MrHohn commented Mar 25, 2018

@sergeylanzman Interesting, sounds like the endpoints controller was tricked and thought the yet to be deleted endpoint belongs to the externalName service. Did you create the externalName service with the same selector (which should have no effect)?

@sergeylanzman
Copy link
Contributor Author

@MrHohn I create extername service with kubectl create, without selector, but it's create with selector from clusterIP(maybe cache in apiserver)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants