-
Notifications
You must be signed in to change notification settings - Fork 316
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
Automate DNS redirection to Consul DNS #833
Conversation
demo.yaml
Outdated
app: frontend | ||
annotations: | ||
"consul.hashicorp.com/connect-inject": "true" | ||
"consul.hashicorp.com/transparent-proxy-exclude-outbound-cidrs": "151.101.2.133,151.101.66.133,151.101.130.133,151.101.194.133" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will let us apk update
&& apk add
stuff
Force delete pods that are still running after `helm uninstall` so the next installation can begin immediately.
5475539
to
462098b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work Ashwin and Nitya! I've left some comments/questions inline.
bf6b45a
to
f29938f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looking but wanted to get feedback on my question about namespaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good but wondering about how we're finding the ClusterIP.
Also for acceptance tests what are we thinking? Is this gonna be a separate PR?
|
||
var consulDNSIP string | ||
if dnsEnabled { | ||
for _, e := range os.Environ() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this kinda confusing. I think it could use a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
@lkysow Acceptance tests are a great question. The approach I was considering for acceptance testing this change was indirectly using the changes the will be added for transparent proxy for x-partition communication. I will update the fixtures to support transparent proxying the upstream which should test the usage of Consul DNS we ultimately care about. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can clean up the env var things now?
@@ -138,6 +138,10 @@ type Handler struct { | |||
// from mesh services. | |||
EnableConsulDNS bool | |||
|
|||
// ReleasePrefix is the prefix used for the installation which is used to determine the Service | |||
// name of the Consul DNS service. | |||
ReleasePrefix string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use ResourcePrefix
to match server-acl-init, create-federation-job and others
258eb41
to
ac6faa4
Compare
@test "connectInject/Deployment: -release-prefix unset by default" { | ||
cd `chart_dir` | ||
local actual=$(helm template \ | ||
-s templates/connect-inject-deployment.yaml \ | ||
--set 'connectInject.enabled=true' \ | ||
. | tee /dev/stderr | | ||
yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-release-prefix=RELEASE-NAME-consul")' | tee /dev/stderr) | ||
[ "${actual}" = "false" ] | ||
} | ||
|
||
@test "connectInject/Deployment: -release-prefix is true if dns.enabled=true and dns.enableRedirection=true" { | ||
cd `chart_dir` | ||
local actual=$(helm template \ | ||
-s templates/connect-inject-deployment.yaml \ | ||
--set 'connectInject.enabled=true' \ | ||
--set 'dns.enableRedirection=true' \ | ||
. | tee /dev/stderr | | ||
yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-release-prefix=RELEASE-NAME-consul")' | tee /dev/stderr) | ||
[ "${actual}" = "true" ] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need these now
// If Consul DNS is enabled, we find the environment variable that has the value | ||
// of the ClusterIP of the Consul DNS Service. constructDNSServiceHostName returns | ||
// the name of the env variable whose value is the ClusterIP of the Consul DNS Service. | ||
consulDNSClusterIP = os.Getenv(h.constructDNSServiceHostName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should error out if the env var is not found
705b8b9
to
7a4ab00
Compare
f5dc220
to
71b60f1
Compare
Tested with this branch and consul branch `dns-redirect-to-consul`.
Co-authored-by: Luke Kysow <1034429+lkysow@users.noreply.github.com>
afaf55f
to
ff61a47
Compare
ff61a47
to
521fb79
Compare
Changes proposed in this PR:
Corresponding consul PR: hashicorp/consul#11480
How I've tested this PR:
k apply -f demo.yaml #included in this PR for testing, remove before merging
Then, exec onto the frontend pod, and
and verify it returns a pod ip for backend. We can't curl
backend.service.consul
because envoy only knows to direct traffic based on VIPs not pod ipsHow I expect reviewers to test this PR:
Checklist: