-
Notifications
You must be signed in to change notification settings - Fork 72
enhance DNS service configuration with TCP/UDP node ports #146
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
Conversation
0858481 to
14c3001
Compare
cloutierMat
left 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.
Thank you for digging here. I like the changes and being able to expose our dns server.
But since this is a breaking change to the values file, I would like to make sure it also fixes the issue in the pipeline before we merge anything. Are we able to use it in your PR K8s tests: Run non-validated tests without releasing?
14c3001 to
8af393a
Compare
cloutierMat
left 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.
Hey @nik-localstack, sorry for not raising this point earlier, but since we found a solution to exposing to nodePort on the same port, do you think that there is value in configuring different ports for dns resolution for TCP/UDP?
charts/localstack/values.yaml
Outdated
| nodePortTcp: 31053 | ||
| ## @param service.dnsService.nodePortUdp specifies the nodePort to map the UDP DNS service to | ||
| ## | ||
| nodePortUdp: 31053 |
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.
Do we actually need both? Is there a use case for tcp dns to be served on a different port as udp dns? This is not really a pattern in LocalStack. Maybe a single nodePort is sufficient?
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 changed it to a single nodeport
This change resolves template errors and provides more granular control over DNS service exposure in Kubernetes environments.
8af393a to
3647f92
Compare
cloutierMat
left 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.
Thanks @nik-localstack for the work to make LocalStack DNS server to work with LS on K8s 🚀
I only added a couple extra nits for better documentation. Otherwise, the changes look good. Thank you for the effort on making it a non-breaking change and improvement for a simpler interface for our users! 🙏
Co-authored-by: Mathieu Cloutier <[email protected]>
Co-authored-by: Mathieu Cloutier <[email protected]>
alexrashed
left 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.
Nice! Thanks for the great iterations, @nik-localstack and @cloutierMat!
I just did some tests, and when using the "old" way of defining the dns (simple true or false) I do get the following warnings:
coalesce.go:298: warning: cannot overwrite table with non table for localstack.service.dnsService (map[enabled:false nodePort:31053])
coalesce.go:298: warning: cannot overwrite table with non table for localstack.service.dnsService (map[enabled:false nodePort:31053])
However, it works perfectly fine. So these warnings can be seen as deprecation warnings I would say. LGTM!
I'll directly move forward and merge this PR and push a new release. 🤩
Motivation
This PR enhances the DNS service configuration by converting the dnsService field from a simple boolean to a structured configuration object, enabling more granular control over DNS port exposure.
Changes
Previous:
dnsService: true/falseNew:
dnsService: { enabled: true, nodePort: 31053 }related to UNC-69