Remove special support for ExternalName services by briansmith · Pull Request #764 · linkerd/linkerd2

briansmith · 2018-04-13T23:28:24Z

After this was implemented we found that ExternalName services are
represented in DNS as CNAMEs, which means that the proxy's DNS
fallback logic can be used instead of doing DNS in the control
plane. Besides simplifying the controller, this will also increase
fidelity with the proxied pods' DNS configuration (improve
transparency).

Signed-off-by: Brian Smith brian@briansmith.org

After this was implemented we found that ExternalName services are represented in DNS as CNAMEs, which means that the proxy's DNS fallback logic can be used instead of doing DNS in the control plane. Besides simplifying the controller, this will also increase fidelity with the proxied pods' DNS configuration (improve transparency). Signed-off-by: Brian Smith <brian@briansmith.org>

briansmith · 2018-04-13T23:30:07Z

@klingerf In Slack discussion @olix0r mentioned the idea of using an ExternalName service to alias another service within the cluster, to test this. In the test framework we have, where can I find an example of creating a service completely internal to the cluster? The egress_test.go tests all reference services that make requests outside the cluster to httpbin.org.

klingerf · 2018-04-13T23:55:39Z

@briansmith Checkout the smoke test config that we use to verify that inject is working: https://github.com/runconduit/conduit/blob/master/test/testdata/smoke_test.yaml

Signed-off-by: Brian Smith <brian@briansmith.org>

klingerf

Nice cleanup! I just had one question about removing support for resolving IPs.

klingerf · 2018-04-16T17:15:25Z

-		}
 	})

 	//TODO: Resolve name using DNS similar to Kubernetes' ClusterFirst


Would you mind removing this TODO as well as part of this branch?

Would you mind removing this TODO as well as part of this branch?

I am going to remove it, and other similar things, in a separate PR.

You're removing the original comment from which this comment was copied (line 51, k8s_resolver.go) as part of this branch. I think it makes sense to also remove this comment simultaneously.

klingerf · 2018-04-16T17:40:29Z

 	log.Infof("Adding k8s name resolver")

-	return []streamingDestinationResolver{ipResolver, k8sResolver}, nil
+	return []streamingDestinationResolver{k8sResolver}, nil


Did you mean to remove the echoIpV4Resolver as part of this branch? That seems unrelated to removing external name support. I'm not sure what the proxy uses the ip resolver for, but functionally, if the path to be resolved is an ipv4 address, this resolver sends it back as an Add event. On master, it looks like this:

$ bin/go-run controller/script/destination-client -path 1.2.3.4:80 INFO[0000] Add: INFO[0000] metric_labels: map[] INFO[0000] - 1.2.3.4:80 - map[] INFO[0000]

Whereas on this branch, I get:

$ bin/go-run controller/script/destination-client -path 1.2.3.4:80 FATA[0000] rpc error: code = Unknown desc = resolver [&{k8sDNSZoneLabels:[] endpointsWatcher:0xc4201b0420}] found error resolving host [1.2.3.4] port[80]: DNS name cannot only contain digits and hyphens: 1.2.3.4

Did you mean to remove the echoIpV4Resolver as part of this branch?

I should have removed it in an earlier branch. The proxy never sends IP addresses to the controller any more. I think even in 0.3.1 it was that way.

Ok, in that case can you also remove the echoIpV4Resolver struct and associated methods and tests? I think that controller/destination/resolver_test.go can go away entirely.

Ok, in that case can you also remove the echoIpV4Resolver struct and associated methods and tests? I think that controller/destination/resolver_test.go can go away entirely.

Done.

Signed-off-by: Brian Smith <brian@briansmith.org>

klingerf

⭐️ Great, thanks for updating.

olix0r · 2018-04-17T17:18:56Z

I believe there may be a correctness issue with this branch. I'll reproduce the issue and describe here.

olix0r · 2018-04-17T17:29:02Z

:; bin/root-tag 
git-b227bd20

Clean up

:; kubectl delete ns emojivoto conduit bot && watch kubectl get ns

Install conduit and emojivoto

...
:; bin/conduit install -v $(bin/root-tag) |kubectl apply -f -
...
:; bin/conduit inject -v $(bin/root-tag) ../emojivoto.yml |kubectl apply -f -     
...

Install two clients: one that uses an ingress.

:; cat ../bot{,-ingress}.yml |bin/conduit inject -v $(bin/root-tag) - |kubectl apply -f -
namespace "bot" created
deployment "voter" created
namespace "bot" configured
service "emojivoto" created
deployment "absentee-voter" created

Confirm that the normal voter is able to contact emojivoto directly:

:; kubectl -n bot exec -c vote-bot voter-75859db8f9-sgg9l  -i -t bash
root@voter-75859db8f9-sgg9l:/# curl -vso /dev/null web-svc.emojivoto.svc.cluster.local
* Rebuilt URL to: web-svc.emojivoto.svc.cluster.local/
* Hostname was NOT found in DNS cache
*   Trying 10.111.130.124...
* Connected to web-svc.emojivoto.svc.cluster.local (10.111.130.124) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.38.0
> Host: web-svc.emojivoto.svc.cluster.local
> Accept: */*
> 
< HTTP/1.1 200 OK
< content-type: text/html
< date: Tue, 17 Apr 2018 17:23:56 GMT
< content-length: 560
< 
{ [data not shown]
* Connection #0 to host web-svc.emojivoto.svc.cluster.local left intact

Confirm the absentee-voter pod is able to resolve the ExternalName, emojivoto.

:; kubectl -n bot exec -c vote-bot absentee-voter-84c6dbc745-4mhwk  -i -t bash
root@absentee-voter-84c6dbc745-4mhwk:/# dig +showsearch emojivoto

; <<>> DiG 9.9.5-9+deb8u15-Debian <<>> +showsearch emojivoto
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 16995
;; flags: qr aa rd ra; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;emojivoto.bot.svc.cluster.local. IN	A

;; ANSWER SECTION:
emojivoto.bot.svc.cluster.local. 30 IN	CNAME	web-svc.emojivoto.svc.cluster.local.
web-svc.emojivoto.svc.cluster.local. 30	IN A	10.111.130.124

;; Query time: 6 msec
;; SERVER: 10.96.0.10#53(10.96.0.10)
;; WHEN: Tue Apr 17 17:25:34 UTC 2018
;; MSG SIZE  rcvd: 97

The absentee-voter pod is not able to contact emojivoto:

root@absentee-voter-84c6dbc745-4mhwk:/# curl -vso /dev/null emojivoto
* Rebuilt URL to: emojivoto/
* Hostname was NOT found in DNS cache
*   Trying 10.111.130.124...
* Connected to emojivoto (10.111.130.124) port 80 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.38.0
> Host: emojivoto
> Accept: */*
> 
< HTTP/1.1 500 Internal Server Error
< content-length: 0
< Date: Tue, 17 Apr 2018 17:26:59 GMT
< 
* Connection #0 to host emojivoto left intact

Configs are here

olix0r

appears to cause some connectivity issues in a test i ran

briansmith · 2018-04-19T22:52:18Z

Thanks @olix0r for providing detailed testing instructions. Sorry I didn't indicate when I submitted the PR that it wasn't yet working.

The problem is that the abstract_ns/ns_dns_tokio/domain crates don't support CNAME entries. There are other limitations of those crates, e.g. they don't support /etc/hosts either.

I have a PR that switches the DNS resolution in the proxy from the ns_dns_tokio crate to Trust-DNS. However, that PR can't be submitted until some bugs I found in Trust-DNS are fixed. I am working with the Trust-DNS maintainer on those now; I'm planning to review the PRs for the fixes when they are ready. Then I will submit a PR to switch to Trust-DNS and then we can continue with this PR.

trust-dns-resolver is a more complete implementation. In particular, it supports CNAMES correctly, which is needed for PR #764. It also supports /etc/hosts, which will help with issue #62. Use the 0.8.2 pre-release since it hasn't been released yet. It was created at our request. Signed-off-by: Brian Smith <brian@briansmith.org>

Signed-off-by: Brian Smith <brian@briansmith.org>

briansmith · 2018-04-25T21:35:06Z

@olix0r Please review (approve) this again. I verified that your example works now, with no changes to this PR, after #834 was merged.

olix0r

thanks!

briansmith self-assigned this Apr 13, 2018

briansmith requested review from klingerf and olix0r April 13, 2018 23:28

olix0r added the review label Apr 13, 2018

Update Go dependency stuff

79a6d2a

Signed-off-by: Brian Smith <brian@briansmith.org>

briansmith force-pushed the b/remove-externalname-support branch from 6bc5e7a to 79a6d2a Compare April 14, 2018 01:07

Fix unit tests

7a8b1c8

Signed-off-by: Brian Smith <brian@briansmith.org>

klingerf reviewed Apr 16, 2018

View reviewed changes

klingerf mentioned this pull request Apr 16, 2018

Destination service does not watch for changes to ExternalName-type services #571

Closed

briansmith added 2 commits April 16, 2018 13:15

Remove echoIpV4Resolver.

ed84088

Signed-off-by: Brian Smith <brian@briansmith.org>

Remove obsolete TODO.

b227bd2

Signed-off-by: Brian Smith <brian@briansmith.org>

klingerf approved these changes Apr 16, 2018

View reviewed changes

olix0r requested changes Apr 17, 2018

View reviewed changes

briansmith mentioned this pull request Apr 20, 2018

added ipv6parser and changed ip inspection logic #729

Closed

briansmith mentioned this pull request Apr 23, 2018

Proxy: Use trust-dns-resolver for DNS. #834

Merged

briansmith added 2 commits April 25, 2018 10:05

Merge branch 'master' into b/remove-externalname-support

5d58e4d

Signed-off-by: Brian Smith <brian@briansmith.org>

Merge branch 'master' into b/remove-externalname-support

aa82089

Signed-off-by: Brian Smith <brian@briansmith.org>

olix0r approved these changes Apr 25, 2018

View reviewed changes

briansmith merged commit c5d2dab into master Apr 25, 2018

briansmith deleted the b/remove-externalname-support branch April 25, 2018 21:53

olix0r removed the review label Apr 25, 2018

This was referenced Apr 25, 2018

Release Notes for 0.4.1 release. #839

Merged

Destinations service keeps complaining about external name not being valid IP #643

Closed

Conversation

briansmith commented Apr 13, 2018

Uh oh!

briansmith commented Apr 13, 2018

Uh oh!

klingerf commented Apr 13, 2018

Uh oh!

klingerf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

briansmith Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

klingerf Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

klingerf Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

briansmith Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

klingerf Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansmith Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

olix0r commented Apr 17, 2018

Uh oh!

olix0r commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

briansmith commented Apr 19, 2018

Uh oh!

briansmith commented Apr 25, 2018

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

klingerf left a comment •

edited

Loading

klingerf Apr 16, 2018 •

edited

Loading

olix0r commented Apr 17, 2018 •

edited

Loading