enhance DNS service configuration with TCP/UDP node ports#146
enhance DNS service configuration with TCP/UDP node ports#146alexrashed merged 3 commits intomainfrom
Conversation
0858481 to
14c3001
Compare
cloutierMat
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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 <79954947+cloutierMat@users.noreply.github.com>
Co-authored-by: Mathieu Cloutier <79954947+cloutierMat@users.noreply.github.com>
alexrashed
left a comment
There was a problem hiding this comment.
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