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/kubernetes: Handle multiple local IPs and bind #3208

Merged
merged 8 commits into from Sep 5, 2019

Conversation

@chrisohaver
Copy link
Member

commented Aug 26, 2019

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

Handle NS responses for following cases:

  • CoreDNS is bound to more than one local address.
  • CoreDNS is bound to a specific address/addresses with the bind plugin: Only use the bound IPs.

example: CoreDNS bound to two IP addresses

;; ANSWER SECTION:
cluster.local.		5	IN	NS	ns.dns.cluster.local.

;; ADDITIONAL SECTION:
ns.dns.cluster.local.	5	IN	AAAA	1::2:3:4:5
ns.dns.cluster.local.	5	IN	A	10.84.16.183

Previously, we would always only use the first non-loopback address seen on the local system.
This would fail in cases where:

  • CoreDNS is using the bind plugin to bind to an IP other than the first non-loopback address. CoreDNS would then answer with an IP that it is not bound to.
  • In a Kubernetes cluster, if CoreDNS runs in a dual stack pod, the first IP may not be targeted by a service (or each IP could be targeted by different services), in which case CoreDNS would answer incompletely, or with the Pod IP instead of the Service IP.

2. Which issues (if any) are related?

#3160
Also related to dual stack support in k8s.

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

no

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

no

@chrisohaver chrisohaver requested a review from johnbelamaric Aug 26, 2019
@@ -372,6 +372,8 @@ func NS(ctx context.Context, b ServiceBackend, zone string, state request.Reques
// ... and reset
state.Req.Question[0].Name = old

seen := map[string]bool{}

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 26, 2019

Author Member

Added seen to avoid answer duplication in cases where a single answer has multiple additional records.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 26, 2019

Codecov Report

Merging #3208 into master will decrease coverage by 0.03%.
The diff coverage is 19.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3208      +/-   ##
==========================================
- Coverage   55.77%   55.73%   -0.04%     
==========================================
  Files         206      206              
  Lines       10419    10435      +16     
==========================================
+ Hits         5811     5816       +5     
- Misses       4188     4200      +12     
+ Partials      420      419       -1
Impacted Files Coverage Δ
plugin/kubernetes/kubernetes.go 66.8% <ø> (+0.68%) ⬆️
plugin/kubernetes/setup.go 66.32% <0%> (-1.58%) ⬇️
plugin/backend_lookup.go 0% <0%> (ø) ⬆️
plugin/kubernetes/local.go 0% <0%> (ø) ⬆️
plugin/kubernetes/ns.go 88.23% <100%> (+0.48%) ⬆️
plugin/clouddns/clouddns.go 85.45% <0%> (+2.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 73391f6...1901720. Read the comment docs.

Copy link
Member

left a comment

/lgtm

}
}
return nil
return ips
}

// LocalNodeName is exclusively used in federation plugin, will be deprecated later.

This comment has been minimized.

Copy link
@johnbelamaric

johnbelamaric Aug 27, 2019

Member

is this comment true?

At least partly ...

"LocalNodeName is exclusively used in federation plugin": True, only used by FederationsFunc in the federation plugin.

"... will be deprecated later": Maybe - depends on what "remove" means in #3041. I thought it means deprecate/remove... but maybe it just means to move it to an external repo (since thats all that was done, and the issue was closed).

@chrisohaver chrisohaver merged commit 630d3d6 into coredns:master Sep 5, 2019
4 checks passed
4 checks passed
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 55.73% (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
3 participants
You can’t perform that action at this time.