fix: pass IPv6 DNS resolvers correctly

[lucasl0st]

Proposed changes

Problem: nginx expects IPv6 DNS resolvers to be passed with [] brackets:
invalid port in resolver \"2606:4700:4700::1111\" in /etc/nginx/stream-conf.d/stream.conf
But passing resolvers with brackets is not possible:
gatewayClassNp="&{Source:0xc0014382c0 ErrMsgs:[spec.dnsResolver.addresses[0].value: Invalid value: \"[2606:4700:4700::1111]\": must be a valid IP address] Valid:false}"

Solution: detect IPv6 resolvers and add brackets in the config templating.

Testing: deployed on an IPv6 capable cluster and configured DNS resolvers using the helm chart, deployed a gateway and http route pointing to an ExternalName service (which requires configuring a DNS resolver).

Fixes #4369

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Fixed an issue regarding configuring IPv6 DNS resolvers for ExternalName services

[nginx-bot]

Hi @lucasl0st!

Thanks for opening this pull request!
Be sure to check out our Contributing Guidelines while you wait for someone on the team to review this.

[sjberman]

@lucasl0st Thanks for finding and fixing this! Would you mind creating an associated issue and linking it in the PR description to Close? Helps us keep track of things.

[sjberman]

Based on what I read in the bug you created, does the NGF validation need to be updated as well?

[lucasl0st]

Based on what I read in the bug you created, does the NGF validation need to be updated as well?

Could you tell me what exactly you mean by NGF validation?
The part that fails when passing an IPv6 address with brackets is here:

nginx-gateway-fabric/internal/controller/state/graph/nginxproxy.go

Line 358 in b50193d

if errs := k8svalidation.IsValidIP(addrPath.Child("value"), addr.Value); len(errs) > 0 {

It does not accept IPv6 addresses within [] brackets, but I'm unsure if that's what you're referring to here

[lucasl0st]

And just to be sure, anything I should do regarding the failed linter in the pipeline? https://github.com/nginx/nginx-gateway-fabric/actions/runs/19806475453/job/56824105560

Runs fine locally for me

[sjberman]

It does not accept IPv6 addresses within [] brackets, but I'm unsure if that's what you're referring to here

Is the intention to be able to define an Ipv6 address with brackets in the API? If port is specified, then would it make sense to do it that way? I figured we would want to loosen the validation if so.

[lucasl0st]

My intention with this PR is to support IPv6 resolvers the same way IPv4 resolvers are currently supported.
This means that within the helm chart or configuration IPv6 resolvers are configured by a user without brackets, the nginx config templating handles the job of adding the brackets required by nginx for IPv6 (the fix in this PR).

The current implementation/validation would not allow for custom ports (to the best of my knowledge), for IPv6 as well as IPv4.
Nginx itself does seem to support custom ports: https://nginx.org/en/docs/http/ngx_http_core_module.html#resolver
So I guess it would be up to you to decide if you want to support this.

[sjberman]

Ah, I see that ports was not the original intention. So I think the validation is fine for now as long as a basic IPv6 address works.

[sjberman]

I think the linting/dependency issue is probably on our end.

[lucasl0st]

Regarding the failed pipeline, I think the reason mine is failing and others are not is because I am on a fork.
The step "Configure GOPROXY" in Checks and Variables says No Artifactory secrets available - using direct GOPROXY.
Other PRs say Development mode - using dev Artifactory.

So something seems to be wrong when using GOPROXY=direct 🤔

[lucasl0st]

Running

go clean -modcache
GOPROXY=direct go mod tidy

inside the tests directory reproduces the issue locally

[sjberman]

GOPROXY=direct go mod tidy

Aha, yes it does. Nice.

[sjberman]

A manual change of the checksum could fix this, however that worries me for two reasons:

1. the dependency may have been tampered with, which is why this checksum verification exists
2. it probably won't match our dev artifactory version, causing all upstream PRs to likely fail.

I've opened tsenart/vegeta#739 to see about this. In the meantime, in order to get this fix merged in, I'll open a PR in the upstream repo and add you as the commit author.

[lucasl0st]

This seems to be the diff between the version in sum.golang.org and direct:
https://gist.github.com/lucasl0st/48d239ab265ce7ddab50bb7b3c960bc6

GOPROXY=https://proxy.golang.org GOSUMDB=sum.golang.org \
  GOPATH=/tmp/gopath-old \
  go mod download github.com/tsenart/vegeta/v12@v12.13.0

GOPROXY=direct GONOSUMDB=github.com/tsenart/vegeta \
  GOPATH=/tmp/gopath-new \
  go mod download github.com/tsenart/vegeta/v12@v12.13.0

diff -ru /tmp/gopath-old/pkg/mod/github.com/tsenart/vegeta/v12@v12.13.0 /tmp/gopath-new/pkg/mod/github.com/tsenart/vegeta/v12@v12.13.0

Seems to be this commit: tsenart/vegeta@507c4f0

[sjberman]

That commit did go into the most recent release, so I'm wondering how the checksums ended up differing.

[lucasl0st]

They probably force pushed the tag

[sjberman]

Well I'm not sure what just happened, but the pipeline ran and passed those checks now. Seemed timed perfectly with when I opened the PR in the main repo...

[sjberman]

Oh interesting, it seems to be running while referencing my other PR, maybe due to the shared commit. I'm just going to reopen that one.