New Plugin: extkubernetes - Expose External IPs of Load Balancers #2337

chrisohaver · 2018-11-26T14:34:35Z

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

Adds a new plugin (extkubernetes) that exposes external ip addresses of loadBalancer services
Starts to moves some code common with kubernetes to pkg/kubernetes (e.g. API connection, watch).

"Connection sharing" with the kubernetes plugin is not implemented (that is, if both plugins are active, they do not share the same API connection). I think the general mode of deployment would be to serve external IPs from a separate instance of CoreDNS, not from the internal cluster DNS, so connection sharing would seldom be a factor. But if there is a demand to serve the data from the same instance, then we can implement connection sharing later.

No coredns/ci test yet. It will be complicated to implement, but doable. It will involve setting up virtual routers and load balancers (metalLB) in the framework of the test. I'll start on this only if it looks like the plugin will be accepted.

2. Which issues (if any) are related?

#1851
kubernetes/dns#242

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

plugin/pkg/kubernetes/kubernetes.go

plugin/pkg/kubernetes/watch.go

codecov-io · 2018-11-26T15:23:06Z

Codecov Report

Merging #2337 into master will decrease coverage by 0.38%.
The diff coverage is 49.18%.

@@            Coverage Diff             @@
##           master    #2337      +/-   ##
==========================================
- Coverage   56.45%   56.07%   -0.39%     
==========================================
  Files         203      216      +13     
  Lines       10157    10702     +545     
==========================================
+ Hits         5734     6001     +267     
- Misses       3986     4238     +252     
- Partials      437      463      +26

Impacted Files	Coverage Δ
plugin/kubernetes/watch.go	`0% <ø> (-43.48%)`	⬇️
plugin/pkg/kubernetes/controller.go	`0% <0%> (ø)`
plugin/extkubernetes/watch.go	`0% <0%> (ø)`
plugin/extkubernetes/apiproxy.go	`0% <0%> (ø)`
plugin/extkubernetes/health.go	`0% <0%> (ø)`
plugin/kubernetes/autopath.go	`0% <0%> (ø)`	⬆️
plugin/pkg/kubernetes/xfr.go	`0% <0%> (ø)`
plugin/kubernetes/xfr.go	`90.82% <100%> (+2.49%)`	⬆️
plugin/kubernetes/parse.go	`91.11% <100%> (ø)`	⬆️
plugin/kubernetes/setup.go	`70.15% <100%> (ø)`	⬆️
... and 26 more

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 2032586...d0f9446. Read the comment docs.

chrisohaver · 2018-11-27T14:13:59Z

FYI, @johnbelamaric

chrisohaver · 2018-11-28T16:10:55Z

I should clarify that the state of this PR is that it is functional. I've tested it using minikube + metalLB, and it works as expected. Hence I am not marking this as a work in progress.
There is still some code duplication, more code could be moved to pkg/kubernetes.

Awaiting feedback and general acceptance of the approach before continuing.

miekg · 2018-11-28T21:36:40Z

This almost duplicates the kubernetes plugin. Please figure out a way to not do that and not fill up pkg/* with kubernetes stuff

miekg · 2018-11-28T21:29:44Z

plugin.cfg

@@ -44,6 +44,7 @@ hosts:hosts
 route53:route53
 federation:federation
 kubernetes:kubernetes
+extkubernetes:extkubernetes


don't really like the name, should we just go for external, or externelkubernetes or ... something?

miekg · 2018-11-28T21:30:50Z

plugin/extkubernetes/README.md

+
+## Description
+
+Creates A/AAAA, SRV, and PTR records for the External IPs of each LoadBalancer type Service in a Kubernetes cluster.


Please full sentences. The extkubernernetes plugin ...

miekg · 2018-11-28T21:31:37Z

plugin/extkubernetes/README.md

+}
+```
+
+* `resyncperiod` specifies the Kubernetes API refresh period to be **DURATION**.


Just reference the kubernetes plugin here? Removes the need to duplicate all the info here.

miekg · 2018-11-28T21:32:59Z

plugin/extkubernetes/apiproxy.go

+func (p *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	upstream := p.Select()
+	network := "tcp"
+	address := upstream.Name


address is only used in the dial just below? No need to put this in a new var

miekg · 2018-11-28T21:33:26Z

plugin/extkubernetes/apiproxy.go

+		http.Error(w, fmt.Sprintf("Unable to establish connection to upstream %s://%s: %s", network, address, err), 500)
+		return
+	}
+	hj, ok := w.(http.Hijacker)


why is this needed?

miekg · 2018-11-28T21:33:54Z

plugin/extkubernetes/handler.go

+	"context"
+
+	"github.com/coredns/coredns/plugin"
+	"github.com/coredns/coredns/request"


miekg · 2018-11-28T21:34:32Z

plugin/extkubernetes/handler.go

+
+// ServeDNS implements the plugin.Handler interface.
+func (k Kubernetes) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+	opt := plugin.Options{}


where is opt used? Please move this closer

miekg · 2018-11-28T21:35:16Z

plugin/extkubernetes/handler_test.go

+	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+var dnsTestCases = []test.Case{


is this just duplicating stuff?

miekg · 2018-11-28T21:45:06Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] New Plugin: e..." ]

Closed #2337.

Note, Chris: I'm very much in favor of having this, but can we do it in much less than 3000 lines added?

fturib · 2018-11-28T22:08:31Z

from @johnbelamaric:

For this reason I think it makes sense to be a different plugin.

from @miekg :

This almost duplicates the kubernetes plugin. Please figure out a way to not do that and not fill up pkg/* with kubernetes stuff

I guess there is too much constraints here.

If we want to make 2 separates plugins, then we'd better have common code (throuhg lib), and also maybe share the same connection to K8s API.

I am not sure why the number of lines make a difference: I would think the important is to have code that with good design, and maintainable, and UT (which adds line for sure).

miekg · 2018-11-28T22:11:45Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] New Plugin: e..." ]

I am not sure why the number of lines make a difference: I would think the important is to have code that with good design, and maintainable, and UT (which adds line for sure).

It's not CLOC, but if duplicated, we need something to keep things in sync (automatically) /Miek

…

-- Miek Gieben

fturib · 2018-11-28T22:18:25Z

I do not find the meaning for CLOC (https://acronyms.thefreedictionary.com/CLOC). Can you translate ?

but if duplicated, we need something to keep things in sync

I guess we do not want to duplicate but rather have a common library, no ?
and the common part, packaged as a lib, is moved to plugin/pkg ... Is it not the purpose of this folder ?

miekg · 2018-11-28T22:28:15Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] New Plugin: e..." ]

I do not find the meaning for CLOC (https://acronyms.thefreedictionary.com/CLOC). Can you translate ? > but if duplicated, we need something to keep things in sync I guess we do not want to duplicate but rather have a common library, no ? and the common part, packaged as a lib, is moved to plugin/pkg ... Is it not the purpose of this folder ?

yes, but everything within limits of course; if the entire plugin lives in pkg/ you have to ask what you're doing. Seems doing this in the kubernets plugins seems more sensible? (don't know what I said before, but 3000 cloc for a little feature seems to be pushing it)

chrisohaver requested a review from miekg November 26, 2018 14:34

stickler-ci reviewed Nov 26, 2018

View reviewed changes

chrisohaver added 4 commits November 26, 2018 09:55

packagize k8s api con

8cd9a77

add extkubernetes plugin

c038e23

oops got readmes swapped

84d4d53

rebase with SOA TTL changes from master

170412d

chrisohaver force-pushed the k8sextcopy branch from e6a4163 to 170412d Compare November 26, 2018 15:05

chrisohaver added 2 commits November 26, 2018 10:11

stickler

f61585a

fix federation object ref

dd1d893

chrisohaver added 2 commits November 27, 2018 09:16

remove docs reference to pod records

d0f9446

minor changes to readme

f0dc777

miekg reviewed Nov 28, 2018

View reviewed changes

chrisohaver closed this Nov 28, 2018

chrisohaver mentioned this pull request Nov 29, 2018

Kubernetes Expose External Load Balancer IPs #2349

Closed

miekg mentioned this pull request Dec 8, 2018

Add new plugin: external - resolve k8s ingress and LB address with external names #2379

Merged

chrisohaver deleted the k8sextcopy branch January 9, 2021 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Plugin: extkubernetes - Expose External IPs of Load Balancers #2337

New Plugin: extkubernetes - Expose External IPs of Load Balancers #2337

chrisohaver commented Nov 26, 2018 •

edited

Loading

codecov-io commented Nov 26, 2018 •

edited

Loading

chrisohaver commented Nov 27, 2018

chrisohaver commented Nov 28, 2018

miekg commented Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg Nov 28, 2018

miekg commented Nov 28, 2018 via email

fturib commented Nov 28, 2018

miekg commented Nov 28, 2018 via email

fturib commented Nov 28, 2018

miekg commented Nov 28, 2018 via email


		## Description

		Creates A/AAAA, SRV, and PTR records for the External IPs of each LoadBalancer type Service in a Kubernetes cluster.

New Plugin: extkubernetes - Expose External IPs of Load Balancers #2337

New Plugin: extkubernetes - Expose External IPs of Load Balancers #2337

Conversation

chrisohaver commented Nov 26, 2018 • edited Loading

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

2. Which issues (if any) are related?

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

codecov-io commented Nov 26, 2018 • edited Loading

Codecov Report

chrisohaver commented Nov 27, 2018

chrisohaver commented Nov 28, 2018

miekg commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miekg commented Nov 28, 2018 via email

fturib commented Nov 28, 2018

miekg commented Nov 28, 2018 via email

fturib commented Nov 28, 2018

miekg commented Nov 28, 2018 via email

chrisohaver commented Nov 26, 2018 •

edited

Loading

codecov-io commented Nov 26, 2018 •

edited

Loading