xds/rbac: use net.SplitHostPort in IP matchers to handle ip:port addresses#8992
Open
instantraaamen wants to merge 3 commits intogrpc:masterfrom
Open
xds/rbac: use net.SplitHostPort in IP matchers to handle ip:port addresses#8992instantraaamen wants to merge 3 commits intogrpc:masterfrom
instantraaamen wants to merge 3 commits intogrpc:masterfrom
Conversation
…esses remoteIPMatcher.match() and localIPMatcher.match() call netip.ParseAddr() on net.Addr.String(), which returns "ip:port" format in production (*net.TCPAddr). Since netip.ParseAddr() only accepts bare IP addresses, the parse always fails, causing all IP-based RBAC rules to silently not match. Use net.SplitHostPort() to extract the host before parsing, consistent with how rbac_engine.go already handles address parsing for port matching. Add test cases using *net.TCPAddr to verify correct behavior with ip:port format addresses. Fixes grpc#8991 Made-with: Cursor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8992 +/- ##
==========================================
- Coverage 83.04% 82.94% -0.11%
==========================================
Files 411 411
Lines 32892 32900 +8
==========================================
- Hits 27316 27288 -28
- Misses 4181 4206 +25
- Partials 1395 1406 +11
🚀 New features to boost your workflow:
|
Replace net.ParseIP (disallowed by vet.sh) with netip.MustParseAddr(...).AsSlice() in ip_matcher_test.go. Made-with: Cursor
Made-with: Cursor
Author
|
I hadn’t noticed #8990 when I opened this PR — sorry about the duplication. For context, I had shared this privately yesterday and was later advised to open a public PR here, which is why I submitted #8992 today. One thing I tried to add here is test coverage for the ip:port case with *net.TCPAddr. Happy to rework or consolidate this however maintainers prefer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
remoteIPMatcher.match()andlocalIPMatcher.match()ininternal/xds/rbac/matchers.gocallnetip.ParseAddr()on the string representation ofnet.Addr, which includes the port number in production (e.g."10.0.0.5:8080"). Sincenetip.ParseAddr()only accepts bare IP addresses, the parse always fails on real TCP connections, causing all IP-based RBAC rules (DirectRemoteIp,RemoteIp,DestinationIp) to silently not match.This fix uses
net.SplitHostPort()to extract the host before callingnetip.ParseAddr(), with a fallback for addresses that don't include a port. This is consistent with how the same file already handles address parsing for port matching inrbac_engine.go(line 232).Fixes #8991
Root Cause
In production,
peer.Peer.Addris a*net.TCPAddrwhoseString()returns"ip:port". The existing unit tests use a customaddrstruct that returns only the bare IP, so the bug was not caught.Changes
internal/xds/rbac/matchers.go: Addnet.SplitHostPort()beforenetip.ParseAddr()in bothremoteIPMatcher.match()andlocalIPMatcher.match()internal/xds/rbac/ip_matcher_test.go: Add test cases using*net.TCPAddrto verify correct behavior withip:portformat addressesTesting
All existing tests continue to pass. New test cases added for both
remoteIPMatcherandlocalIPMatcherusing*net.TCPAddr(production behavior):RELEASE NOTES: