Skip to content

grpclb: replace net.IP with netip.Addr#8918

Merged
arjan-bal merged 3 commits intogrpc:masterfrom
veeceey:fix/issue-8884-netip-migration
Mar 17, 2026
Merged

grpclb: replace net.IP with netip.Addr#8918
arjan-bal merged 3 commits intogrpc:masterfrom
veeceey:fix/issue-8884-netip-migration

Conversation

@veeceey
Copy link
Contributor

@veeceey veeceey commented Feb 20, 2026

Contributes to #8884

This PR replaces deprecated net.IP usage with the modern netip.Addr API in two packages:

  • grpclb: Replace net.IP(s.IpAddress).String() with netip.AddrFromSlice() in processServerList, adding proper error handling for invalid IP addresses instead of silently producing "?" from net.IP.String().

Note: x509.Certificate.IPAddresses fields in test files (internal/credentials/xds/handshake_info_test.go, security/advancedtls/crl_test.go) are typed as []net.IP by the standard library, so those cannot be migrated. The xds/server and xds/xdsclient packages are being addressed in #8909.

RELEASE NOTES: N/A

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 20, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Feb 20, 2026

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.40%. Comparing base (c1a9239) to head (48210b8).
⚠️ Report is 34 commits behind head on master.

Files with missing lines Patch % Lines
balancer/grpclb/grpclb_remote_balancer.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8918      +/-   ##
==========================================
+ Coverage   80.40%   83.40%   +3.00%     
==========================================
  Files         416      410       -6     
  Lines       33495    32602     -893     
==========================================
+ Hits        26930    27191     +261     
+ Misses       4682     4035     -647     
+ Partials     1883     1376     -507     
Files with missing lines Coverage Δ
balancer/grpclb/grpclb_remote_balancer.go 83.06% <60.00%> (-1.37%) ⬇️

... and 79 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Pranjali-2501
Copy link
Contributor

You need to sign the CLA first: https://github.com/grpc/grpc-go?tab=contributing-ov-file#legal-requirements.

This comment will guide.

Replace legacy net.IP usage with the modern net/netip package:
- grpclb: use netip.AddrFromSlice instead of net.IP cast for parsing
  server list IP addresses, with proper error handling for invalid IPs.
- channelz: use netip.AddrFromSlice and net.TCPAddr.AddrPort() for
  converting addresses in the channelz protoconv layer.

Contributes to grpc#8884.
@veeceey veeceey force-pushed the fix/issue-8884-netip-migration branch from 7ad583b to 3bd1c99 Compare February 23, 2026 02:30
@veeceey
Copy link
Contributor Author

veeceey commented Feb 23, 2026

Thanks for the heads up! The CLA is already signed — the EasyCLA check is passing now (you can see the ✅ in the bot comment above).

I've also updated the PR description to follow the repo's contributing guidelines.

@veeceey
Copy link
Contributor Author

veeceey commented Feb 24, 2026

The 'Validate PR' check is failing because this PR is missing a Type: label. Could a maintainer please add the Type: Internal Cleanup label? The changes are a refactor replacing deprecated net.IP usage with the modern netip.Addr API.

case "tcp":
// Note zone info is discarded through the conversion.
return &channelzpb.Address{Address: &channelzpb.Address_TcpipAddress{TcpipAddress: &channelzpb.Address_TcpIpAddress{IpAddress: a.(*net.TCPAddr).IP, Port: int32(a.(*net.TCPAddr).Port)}}}
tcpAddr := a.(*net.TCPAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can do that in a single line: addrPort := a.(*net.TCPAddr).AddrPort()

Copy link
Contributor

@Pranjali-2501 Pranjali-2501 left a comment

Choose a reason for hiding this comment

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

LGTM.
Assigning it to @arjan-bal for second review.

Comment on lines +87 to +92
if !ok {
if lb.logger.V(2) {
lb.logger.Infof("Server list entry:|%d|, failed to parse IP address: %x", i, s.IpAddress)
}
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing behaviour of net.IP(s.IpAddress).String() when receiving invalid input is to return "?" + hexString(ip). This results in the load balancing policy attempting to create a subchannel with an invalid address string, which ultimately fails. The original authors may not have realized that IP address parsing behaves this way for malformed inputs.

Simply ignoring the address could cause a behavioral change. Since grpclb is no longer actively developed, I prefer maintaining the existing behaviour to avoid the risk of regressions, even if it isn't optimal. @veeceey, could you please assign ipStr as fmt.Sprintf("? %x", s.IpAddress)?

Comment on lines +66 to +67
if addr, ok := netip.AddrFromSlice(a.(*net.IPAddr).IP); ok {
return &channelzpb.Address{Address: &channelzpb.Address_TcpipAddress{TcpipAddress: &channelzpb.Address_TcpIpAddress{IpAddress: addr.AsSlice()}}}
Copy link
Contributor

@arjan-bal arjan-bal Feb 27, 2026

Choose a reason for hiding this comment

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

It looks like we’re taking these bytes on a mandatory scenic tour. We are converting net.IP (which is already a []byte) to netip.Addr only to convert it back to []byte. This introduces unnecessary parsing overhead and a fallible operation. Can we avoid this?

The ideal solution would be to change the code that sets the concrete type behind the net.Addr interface not be a *net.IPAddr. However, this seems to be coming from the net.Conn produced by the std library. So we can't do anything here.

- channelz: remove unnecessary net.IP -> netip.Addr -> []byte round-trip
  in the "ip" and "ip+net" cases; use the IP byte slice directly
- channelz: collapse tcpAddr intermediate variable per nit
- grpclb: restore original fallback behaviour for malformed IP addresses
  by using fmt.Sprintf("? %x", ...) instead of silently skipping entries
@github-actions
Copy link

github-actions bot commented Mar 5, 2026

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added stale and removed stale labels Mar 5, 2026
@mbissa mbissa modified the milestones: 1.80 Release, 1.81 Release Mar 6, 2026
@veeceey
Copy link
Contributor Author

veeceey commented Mar 10, 2026

Hey, sorry for the delay on this — totally missed the label change.

@arjan-bal thanks for the thorough review, appreciate the detailed feedback on both files.

For the grpclb change: makes total sense to preserve the existing behavior for invalid IPs. I'll update it to fall back to fmt.Sprintf("? %x", s.IpAddress) instead of skipping the entry. Since grpclb isn't actively developed, maintaining backwards compat is the right call.

For the channelz socket.go comment about the round-trip conversion: yeah, fair point — converting net.IP to netip.Addr and back to []byte is pointless there since the input is already a []byte. I'll leave those cases as-is (keep using the original .IP directly) and only apply the netip migration where it actually provides value (like the TCPAddr case where we use AddrPort()).

Also @Pranjali-2501, good catch on the single-line simplification with .AddrPort() — I'll fold that in too.

Will push the updates shortly.

@veeceey
Copy link
Contributor Author

veeceey commented Mar 12, 2026

addressed the round-trip concern - reverted socket.go back to direct field access instead of going through AddrPort(). let me know if this looks better!

@veeceey
Copy link
Contributor Author

veeceey commented Mar 15, 2026

Fair points @arjan-bal. You're right about the unnecessary round-trip — converting net.IP to netip.Addr just to get bytes back is pointless overhead. I'll simplify that.

And agreed on preserving the existing behavior for malformed inputs rather than silently dropping addresses. I'll keep the fallback path as-is and just clean up the conversion logic. Will push an update.

Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@arjan-bal arjan-bal changed the title grpclb, channelz: replace net.IP with netip.Addr grpclb: replace net.IP with netip.Addr Mar 17, 2026
@arjan-bal arjan-bal merged commit 36e13de into grpc:master Mar 17, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants