controller: resolve external services with public DNS #360

seanmonstar · 2018-02-14T22:27:21Z

This is a pretty basic stab at providing DNS support for external services (see #155). Notably:

This doesn't use any DNS library at the moment, just net.LookupHost. This means it was much easier to implement, but that it doesn't observe TTLs, and so the interval it checks for updates is rather arbitrary.
I tried to follow what I saw in the Endpoints stuff to keep only 1 querying thing per host, even if multiple proxies are all asking for it.
Hopefully the informer.run() part is sufficiently separate that we could later add in usage of dns or coredns or something.

Closes #155

adleong

Looks reasonable to me so far

adleong · 2018-02-14T22:37:33Z

controller/destination/egress.go

+	informer.mutex.Lock()
+	defer informer.mutex.Unlock()
+
+	for l in range(informer.listeners) {


if I remember correctly, this might be giving you the index instead of the item....

adleong · 2018-02-14T22:38:14Z

controller/destination/egress.go

+
+	for l in range(informer.listeners) {
+		if l == listener {
+			//todo


something to watch out for: you probably want to stop the informer and remove it from the map if the list of listeners becomes empty.

Yes, that's what the stopCh is meant as well. In Unsubscribe, I expect that if the list has become empty, to shutdown that informer. Still looking up super basic things like removing an element from an array :D

seanmonstar · 2018-02-15T02:33:30Z

I wanted to start trying to write tests for this file, but have hit some snags in how best to do that. I'd love to create some testListener, and be able to spell out a bunch of arrays that would be called on informer.update(addrs), and that with each of those calls, be able to test testListener.Update(add, remove) was called with the right arrays. Can I easily smash common.TcpAddresss into strings and compare for testing purposes?

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

adleong · 2018-02-15T21:02:57Z

in util/util.go there is func AddressToString(addr *pb.TcpAddress) string {
Does that help?

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar · 2018-02-17T00:00:29Z

Woo, unit tests pass, and theres a relcon test passing with this also.

So, uh, needs a review. Also, there's some implementation details that still have questions:

The port is just always assumed to be 80. Is there a time when that's incorrect?
This picks an arbitrary interval of 10 seconds. Should it be more? Less?

adleong

This looks really great. A few comments below.

adleong · 2018-02-17T00:10:14Z

controller/destination/egress.go

@@ -0,0 +1,186 @@
+package destination


I'd consider naming this file dns_watcher.go since the contents of the file make no reference to the term egress

adleong · 2018-02-17T00:11:54Z

controller/destination/egress.go

+}
+
+func (i* informer) update(addrs []string) {
+	// diff with current set, send any updates


There is a diff function in endpoints.go that could be factored out into util.go and then re-used here.

adleong · 2018-02-17T00:14:01Z

controller/destination/egress.go

+			}
+			additions = append(additions, common.TcpAddress{
+				Ip: ip,
+				Port: 80, //magical


I believe the host can optionally include a port. We should split on : and use the specified port if it exists. Otherwise, we should use the default port for the protocol. This probably requires a change to the destination protobuf to start sending the protocol.

Alternatively, we can punt on this and default to 80 for now, but this will not work for HTTPS egress calls.

briansmith · 2018-02-19T23:16:12Z

See #392.

olix0r · 2018-02-20T00:47:21Z

@briansmith thanks for pointing this out. that is, indeed, an interesting wrinkle.

If I understand correctly, the following would work for us, at least for this release:

proxy tries to resolve all names through the controller. the controller does NOT attempt DNS resolution.
if the controller cannot resolve the name through the kubernetes API, the proxy simply uses the original dst addr, which should be already resolved through DNS by the application

Would this be correct behavior for 0.3?

@seanmonstar If so, I imagine this isn't all that hard to implement in the next day. (I can help on the proxy work, if needed).

adleong · 2018-02-20T01:00:37Z

I don't think we want to wait for a destination service round-trip to route each request with DNS resolution. Should the proxy cache that a particular authority is not resolvable by the destination service? For how long? Or should the destination API be expanded to include a state in the response stream which indicates that the destination service could not resolve the authority?

…

On Mon, Feb 19, 2018 at 4:47 PM, Oliver Gould ***@***.***> wrote: @briansmith <https://github.com/briansmith> thanks for pointing this out. that is, indeed, an interesting wrinkle. If I understand correctly, the following would work for us, at least for this release: 1. proxy to tries to resolve all names through the controller. the controller does NOT attempt to DNS resolution. 2. if the controller cannot resolve the name through the kubernetes API, the proxy simply uses the original dst addr, which should be already resolved through DNS by the application Would this be correct behavior for 0.3? If so, I imagine this isn't *all* that hard to implement in the next day. (I can help on the proxy work, if needed). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#360 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADy6InY6VLmn1AurtcKahaEwFTwSIYeIks5tWhYbgaJpZM4SGElO> .

briansmith · 2018-02-20T01:08:44Z

proxy to tries to resolve all names through the controller. the controller does NOT attempt to DNS resolution.
if the controller cannot resolve the name through the kubernetes API, the proxy simply uses the original dst addr, which should be already resolved through DNS by the application

The controller would have to return a distinct indicator that means "don't rely on me for this name." I don't know if it currently does that; it might need to be modified to do so.

Would this be correct behavior for 0.3?

It would be more correct than the current behavior and safer than what's proposed in this PR.

It wouldn't be completely correct because we'd still resolve metadata as metadata.$namespace.svc.$zone. However, resolving that requires doing a DNS lookup for metadata specifically and I don't think that's something that can be implemented well in a day.

I suggest a slight variation of your suggested implementation, by making the decision in the proxy instead of in the Destination service: If default_zone.is_none() and the name is in the form $a.$b.svc, or if !default_zone.is_none() and the name is in the form $a.$b.svc.$default_zone, for some a and some b, then use the Destination service. Otherwise, use the IP given. That way, we don't even ping the Destination service for such names.

Either your way or this more efficient way would allow external services to work.

Regardless, in 0.4 we'll need to properly handle names like metadata and localhost by actually using DNS and other configuration information to decide whether we should append $namespace.svc and/or $default_zone to the name and resolve it through the Destination service.

briansmith · 2018-02-20T01:11:21Z

Should the proxy cache that a particular authority is not resolvable by the
destination service? For how long?

Or should the destination API be expanded to include a state in the
response stream which indicates that the destination service could not
resolve the authority?

I think this is all addressed by my suggestion above in #360 (comment).

seanmonstar · 2018-02-20T21:21:58Z

Closing in favor of #397

olix0r added the review/ready Issue has a reviewable PR label Feb 14, 2018

seanmonstar force-pushed the controller-public-dns branch from 3ab34d2 to d03cd80 Compare February 14, 2018 22:38

adleong reviewed Feb 14, 2018

View reviewed changes

seanmonstar force-pushed the controller-public-dns branch 12 times, most recently from 39b7789 to 36babcd Compare February 15, 2018 02:25

seanmonstar mentioned this pull request Feb 15, 2018

Destinations service doesn't discover services outside the local cluster #155

Closed

seanmonstar force-pushed the controller-public-dns branch from 36babcd to 307341d Compare February 15, 2018 18:12

controller[wip]: public dns

1ba2e29

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar force-pushed the controller-public-dns branch from 307341d to 1ba2e29 Compare February 15, 2018 20:56

seanmonstar force-pushed the controller-public-dns branch 4 times, most recently from 24ffc5b to 79aa55b Compare February 16, 2018 22:56

add a bunch of update test cases

ea01a76

Signed-off-by: Sean McArthur <sean@seanmonstar.com>

seanmonstar force-pushed the controller-public-dns branch from 79aa55b to ea01a76 Compare February 16, 2018 23:45

adleong reviewed Feb 17, 2018

View reviewed changes

siggy assigned seanmonstar Feb 19, 2018

klingerf added this to the Conduit 0.3 milestone Feb 19, 2018

briansmith mentioned this pull request Feb 20, 2018

Support custom dnsPolicy and dnsConfig #62

Closed

klingerf removed this from the Conduit 0.3 milestone Feb 20, 2018

seanmonstar mentioned this pull request Feb 20, 2018

proxy: use original dst if authority doesnt look like local service #397

Merged

seanmonstar closed this Feb 20, 2018

seanmonstar removed the review/ready Issue has a reviewable PR label Feb 20, 2018

klingerf mentioned this pull request Mar 1, 2018

Handle ExternalName-type svcs in destination service #490

Merged

olix0r deleted the controller-public-dns branch April 30, 2018 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: resolve external services with public DNS #360

controller: resolve external services with public DNS #360

seanmonstar commented Feb 14, 2018 •

edited

Loading

adleong left a comment

adleong Feb 14, 2018

adleong Feb 14, 2018

seanmonstar Feb 14, 2018

seanmonstar commented Feb 15, 2018 •

edited

Loading

adleong commented Feb 15, 2018

seanmonstar commented Feb 17, 2018

adleong left a comment

adleong Feb 17, 2018

adleong Feb 17, 2018

adleong Feb 17, 2018

adleong Feb 17, 2018

briansmith commented Feb 19, 2018

olix0r commented Feb 20, 2018 •

edited

Loading

adleong commented Feb 20, 2018 via email

briansmith commented Feb 20, 2018

briansmith commented Feb 20, 2018 •

edited

Loading

seanmonstar commented Feb 20, 2018

controller: resolve external services with public DNS #360

controller: resolve external services with public DNS #360

Conversation

seanmonstar commented Feb 14, 2018 • edited Loading

adleong left a comment

Choose a reason for hiding this comment

adleong Feb 14, 2018

Choose a reason for hiding this comment

adleong Feb 14, 2018

Choose a reason for hiding this comment

seanmonstar Feb 14, 2018

Choose a reason for hiding this comment

seanmonstar commented Feb 15, 2018 • edited Loading

adleong commented Feb 15, 2018

seanmonstar commented Feb 17, 2018

adleong left a comment

Choose a reason for hiding this comment

adleong Feb 17, 2018

Choose a reason for hiding this comment

adleong Feb 17, 2018

Choose a reason for hiding this comment

adleong Feb 17, 2018

Choose a reason for hiding this comment

adleong Feb 17, 2018

Choose a reason for hiding this comment

briansmith commented Feb 19, 2018

olix0r commented Feb 20, 2018 • edited Loading

adleong commented Feb 20, 2018 via email

briansmith commented Feb 20, 2018

briansmith commented Feb 20, 2018 • edited Loading

seanmonstar commented Feb 20, 2018

seanmonstar commented Feb 14, 2018 •

edited

Loading

seanmonstar commented Feb 15, 2018 •

edited

Loading

olix0r commented Feb 20, 2018 •

edited

Loading

briansmith commented Feb 20, 2018 •

edited

Loading