support CIDR and IPv6 #772
Conversation
Signed-off-by: Prakhar-Shankar <prakharshankar247@gmail.com>
Prakhar-Shankar
requested review from
ispeakc0de,
ksatchit and
uditgaurav
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for CIDR blocks and IPv6 addresses in the pod network partition functionality. Previously, the system was incorrectly appending "/32" to all destination IPs regardless of their format.
- Introduces
normalizeIPOrCIDR()function to properly handle IPv4, IPv6, and CIDR formats - Replaces the previous IP processing logic with proper validation and normalization
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| network-policy.go | Implements new IP/CIDR normalization logic and refactors setExceptIPs method |
| network-policy_cidr_test.go | Adds unit tests for CIDR handling and IP normalization functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| bad := []string{"", "foo", "10.0.0.0/33", "2001:db8::/129"} | ||
| for _, in := range bad { | ||
| if _, err := normalizeIPOrCIDR(in); err == nil && in != "" { |
There was a problem hiding this comment.
The condition err == nil && in != "" is incorrect. For the empty string case, you want to expect no error since line 209 returns "", nil for empty strings. The condition should be err == nil to properly test that invalid inputs like "foo", "10.0.0.0/33", etc. return errors.
| if _, err := normalizeIPOrCIDR(in); err == nil && in != "" { | |
| if _, err := normalizeIPOrCIDR(in); err == nil { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I have changed the test file, now it shows no error for empty string.
| return nil | ||
| } | ||
|
|
||
| // It supports both CIDR block and IPv6 now |
There was a problem hiding this comment.
This comment is incomplete and doesn't clearly describe what the function does. It should be expanded to: "normalizeIPOrCIDR validates and normalizes IP addresses or CIDR blocks, adding appropriate subnet masks (/32 for IPv4, /128 for IPv6) to plain IP addresses."
| // It supports both CIDR block and IPv6 now | |
| // normalizeIPOrCIDR validates and normalizes IP addresses or CIDR blocks, | |
| // adding appropriate subnet masks (/32 for IPv4, /128 for IPv6) to plain IP addresses. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I have added these comments.
|
Hey @Prakhar-Shankar thanks for the PR 🙏, can you please address the Copilot comments? |
Signed-off-by: Prakhar-Shankar <prakharshankar247@gmail.com>
Thanks for reviewing, I have changed the test file so that it won't take empty string as an error. I have also added the recommended comments. |
Problem:
Earlier the program was adding "/32" to every destination IP be it CIDR or IPv6.
Solution:
Introduced normalizeIPOrCIDR() to validate and normalize IPs:
fixes #717
Note - I have also added a test file to check everything is working fine.
Checklist:
breaking-changestagrequires-upgradetag