Skip to content
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

dns: Support link local IPv6 addresses #7889

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Dec 2, 2024

Fixes: #7882 #3272

Link local IPv6 addresses have the format [IPv6-address%interface]:port, notice the extra %interface part which is called the zone. Unlike link local IPv4 addresses, link local IPv6 addresses are not globally unique, instead they can be re-used for each network interface. As a result, the interface is required to disambiguate link local IPv6 addresses (see wikipedia).

Since net.ParseIP doesn't support such addresses, the PR replaces usages of net.ParseIP with the new ipnet.PareAddr which support IPv6 addresses with zones and also returns parsing errors.

RELEASE NOTES:

  • dns: Add support for link local IPv6 addresses.

@arjan-bal arjan-bal added the Type: Feature New features or improvements in behavior label Dec 2, 2024
@arjan-bal arjan-bal added this to the 1.69 Release milestone Dec 2, 2024
@arjan-bal arjan-bal requested review from easwars and dfawley December 2, 2024 15:05
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (4c07bca) to head (e17a20c).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
internal/resolver/dns/dns_resolver.go 73.33% 2 Missing and 2 partials ⚠️
internal/testutils/fakegrpclb/server.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7889      +/-   ##
==========================================
+ Coverage   81.84%   81.93%   +0.09%     
==========================================
  Files         377      377              
  Lines       38120    38117       -3     
==========================================
+ Hits        31201    31233      +32     
+ Misses       5603     5573      -30     
+ Partials     1316     1311       -5     
Files with missing lines Coverage Δ
balancer/pickfirst/pickfirstleaf/pickfirstleaf.go 87.44% <100.00%> (-0.36%) ⬇️
internal/xds/rbac/matchers.go 74.90% <100.00%> (+0.20%) ⬆️
internal/testutils/fakegrpclb/server.go 53.03% <50.00%> (ø)
internal/resolver/dns/dns_resolver.go 88.34% <73.33%> (-1.13%) ⬇️

... and 16 files with indirect coverage changes

@dfawley dfawley assigned arjan-bal and unassigned dfawley Dec 2, 2024
@dfawley
Copy link
Member

dfawley commented Dec 2, 2024

As mentioned offline:

Should we audit all uses of net.ParseIP and change them (and maybe have vet.sh forbid its use)?

@arjan-bal
Copy link
Contributor Author

As mentioned offline:

Should we audit all uses of net.ParseIP and change them (and maybe have vet.sh forbid its use)?

I've replaced all usages of net.ParseIP and added a line in vet.sh to catch usages in the future. PTAL.

@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Dec 2, 2024
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! Glad to see almost all the other uses were just in tests.

@@ -344,7 +345,8 @@ func newRemoteIPMatcher(cidrRange *v3corepb.CidrRange) (*remoteIPMatcher, error)
}

func (sim *remoteIPMatcher) match(data *rpcData) bool {
return sim.ipNet.Contains(net.IP(net.ParseIP(data.peerInfo.Addr.String())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this old redundant net.IP cast was about, but now it's actually necessary. :)

@dfawley dfawley assigned arjan-bal and unassigned dfawley Dec 2, 2024
scripts/vet.sh Outdated
@@ -89,6 +89,10 @@ git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.
git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credential
s/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel('

# Disallow usage of net.ParseIP in favour of ipnet.ParseAddr as the former
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: s/ipnet/netip/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@easwars
Copy link
Contributor

easwars commented Dec 3, 2024

Would this be taken care of as well with this change: #3272?

@easwars easwars removed their assignment Dec 3, 2024
@arjan-bal arjan-bal merged commit 00272e8 into grpc:master Dec 3, 2024
15 checks passed
@arjan-bal arjan-bal deleted the link-local-ipv6 branch December 3, 2024 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NewClient doesn't handle link-local IPv6 addresses
3 participants