-
Notifications
You must be signed in to change notification settings - Fork 321
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
NET-10763 - Allow DNS Proxy to configured with an ACL token when manageSystemACLs is false #4300
Conversation
@@ -1,5 +1,4 @@ | |||
{{- if (or (and (ne (.Values.dns.proxy.enabled | toString) "-") .Values.dns.proxy.enabled) (and (eq (.Values.dns.proxy.enabled | toString) "-") .Values.global.enabled)) }} | |||
{{- if not .Values.connectInject.enabled }}{{ fail "connectInject.enabled must be true" }}{{ end -}} |
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 not have been a requirement.
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.
Shouldn't we have the opposite requirement? Is dns-proxy safe to use with connect-inject enabled?
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 can't think of any reason why it would be unsafe. I think ultimately services would use the local dataplane or agent, but I can't think of a reason why it would be unsafe and require us to prevent it being configured.
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.
The only scenario I can think of where I'm not sure is with tproxy on. I'm not sure how we handle DNS in that case and if it's even allowed to resolve DNS at all because all traffic will be handled through envoy and virtual ips but yes probably not a big deal.
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.
@jmurret Thank you for adding this. I had a question about the deployment requirement and a suggestion to document the type of permission the token needed.
@@ -1,5 +1,4 @@ | |||
{{- if (or (and (ne (.Values.dns.proxy.enabled | toString) "-") .Values.dns.proxy.enabled) (and (eq (.Values.dns.proxy.enabled | toString) "-") .Values.global.enabled)) }} | |||
{{- if not .Values.connectInject.enabled }}{{ fail "connectInject.enabled must be true" }}{{ end -}} |
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.
Shouldn't we have the opposite requirement? Is dns-proxy safe to use with connect-inject enabled?
charts/consul/values.yaml
Outdated
# Refers to a Kubernetes secret that you have created that contains | ||
# an ACL token for your Consul cluster which allows the dns proxy the correct | ||
# permissions. This is only needed if ACLs are managed manually within the Consul cluster, i.e. `global.acls.manageSystemACLs` is `false`. |
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.
Maybe we can add what are the correct permissions that dns-proxy needs in that case?
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.
LGTM!
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.
Suggestions to align the description in the Helm chart that is used for the documentation.
Approving so you're not blocked. Please let me know if you have any questions or need any additional review.
Co-authored-by: Jeff Boruszak <104028618+boruszak@users.noreply.github.com>
Changes proposed in this PR
How I've tested this PR
CI & unit tests
How I expect reviewers to test this PR
👀
Checklist