Skip to content

Conversation

@nik-localstack
Copy link
Contributor

@nik-localstack nik-localstack commented Oct 29, 2025

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

  • Convert service.dnsService from boolean to object
    Previous: dnsService: true/false
    New: dnsService: { enabled: true, nodePort: 31053 }

related to UNC-69

@nik-localstack nik-localstack self-assigned this Oct 29, 2025
@nik-localstack nik-localstack requested a review from a team October 29, 2025 09:47
@nik-localstack nik-localstack marked this pull request as ready for review October 29, 2025 09:47
@nik-localstack nik-localstack changed the title enhance DNS service configuration with separate TCP/UDP node ports enhance DNS service configuration with TCP/UDP node ports Oct 30, 2025
Copy link
Contributor

@cloutierMat cloutierMat left a 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?

Copy link
Contributor

@cloutierMat cloutierMat left a 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?

Comment on lines 143 to 146
nodePortTcp: 31053
## @param service.dnsService.nodePortUdp specifies the nodePort to map the UDP DNS service to
##
nodePortUdp: 31053
Copy link
Contributor

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?

Copy link
Contributor Author

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.
Copy link
Contributor

@cloutierMat cloutierMat left a 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! 🙏

nik-localstack and others added 2 commits November 5, 2025 11:13
Copy link
Member

@alexrashed alexrashed left a 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. 🤩

@alexrashed alexrashed merged commit fa19558 into main Nov 5, 2025
2 checks passed
@alexrashed alexrashed deleted the unc-69-expose-dns-port branch November 5, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants