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

plugin/k8s_external/kubernetes: handle NS records #3160

Merged
merged 6 commits into from Aug 23, 2019

Conversation

@chrisohaver
Copy link
Member

commented Aug 20, 2019

1. Why is this pull request needed and what does it do?

Fixes #3064:

  • don't SERVFAIL when querying NS cluster.local. in k8s_external
  • reply with a record for ns1.dns.cluster.local. in k8s_external

While reworking nsAddr() to fix #3064, I made the following related fixes/improvements which apply to both k8s_external and kubernetes:

  • optimization to use svc object indexes (previously iterated entire service list, which is expensive)
  • handle the ipv6 case (where coredns service is ipv6 - previously ipv4 was assumed)
  • handle case where more than one k8s service targets the coredns endpoint
  • handle corner case when a headless k8s service points to the coredns endpoint

2. Which issues (if any) are related?

#3064

3. Which documentation changes (if any) need to be made?

None

4. Does this introduce a backward incompatible change or deprecation?

No

@corbot

This comment has been minimized.

Copy link

commented Aug 20, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked rajansandeep (via plugin/kubernetes/OWNERS) for a review.
Note this is not an exclusive request. Anyone is free to provide a review of this pull request.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@corbot corbot bot requested a review from rajansandeep Aug 20, 2019

@chrisohaver chrisohaver requested a review from miekg Aug 20, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Aug 20, 2019

Codecov Report

Merging #3160 into master will increase coverage by 0.03%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3160      +/-   ##
==========================================
+ Coverage   55.72%   55.76%   +0.03%     
==========================================
  Files         207      207              
  Lines       10397    10424      +27     
==========================================
+ Hits         5794     5813      +19     
- Misses       4184     4192       +8     
  Partials      419      419
Impacted Files Coverage Δ
plugin/kubernetes/external.go 53.48% <0%> (+1.21%) ⬆️
plugin/kubernetes/local.go 0% <0%> (ø) ⬆️
plugin/kubernetes/ns.go 87.75% <86.36%> (-2.87%) ⬇️
plugin/kubernetes/kubernetes.go 66.11% <88.88%> (+0.45%) ⬆️
plugin/file/reload.go 68.57% <0%> (-5.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f47fc8...d85e0be. Read the comment docs.

@chrisohaver chrisohaver force-pushed the chrisohaver:k8sextns branch from fe983e2 to a82a76f Aug 21, 2019

@chrisohaver chrisohaver requested a review from johnbelamaric Aug 22, 2019

)

rr := new(dns.A)
// Find the CoreDNS Endpoint
localIP := k.interfaceAddrsFunc()

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Aug 22, 2019

Member

FYI, I just looked, and this always returns the IPv4 address. Not something for this PR, but perhaps it needs to go on the k8s IPv6 support list? It's probably going to need to return both...or all addresses? (right now it's just one).

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 23, 2019

Author Member

Yeah - it should return all addresses of all interfaces, and also check for the bind plugin, if present. I was thinking to fixing this in a separate PR. This PR getting a bit large, but I can fix it in this PR if thats preferable.

I missed the IPV6 coredns pod ip bit, thanks! I think I should fix that in this PR, since I have other ipv6 handling in this PR.

// targeting the CoreDNS Pod.
// If CoreDNS is running outside of the Kubernetes cluster: k.nsAddrs() will return the first non-loopback IP
// address seen on the local system it is running on. This could be the wrong answer if coredns is using the *bind*
// plugin to bind to a different IP address.

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Aug 22, 2019

Member

Huh. Is there anything we can do about this? Maybe we need to get it from the server object? Can we get to that here?

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 23, 2019

Author Member

I think we could do it in the onStartUp event.

}
return svcs
}

func (APIConnTest) EpIndexReverse(string) []*object.Endpoints {
func (APIConnTest) EpIndexReverse(ip string) []*object.Endpoints {
if ip != "127.0.0.1" {

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Aug 22, 2019

Member

it's kind of confusing that we use loopback here, given that in general this wouldn't be an endpoint IP. unless I am not understanding something.

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 23, 2019

Author Member

Yeah - i think the test could stub in a different IP to be clearer.

@johnbelamaric

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Ok, looks good. Please fix the bind and multiple IP issue in another PR.

/lgtm

@corbot
corbot bot approved these changes Aug 23, 2019
Copy link

left a comment

LGTM by johnbelamaric

@chrisohaver

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@miekg - would you like to give this a once over or OK to merge?

@johnbelamaric, I do have the bind/multiple IP case (almost) working, but will submit that in a separate PR.

@miekg miekg merged commit 338d148 into coredns:master Aug 23, 2019

4 checks passed

ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 55.76% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.