-
Notifications
You must be signed in to change notification settings - Fork 368
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add more validation of IPPool CRD #3570
Conversation
d9e3c60
to
d35af17
Compare
Codecov Report
@@ Coverage Diff @@
## main #3570 +/- ##
==========================================
- Coverage 63.94% 58.10% -5.85%
==========================================
Files 278 278
Lines 27838 27846 +8
==========================================
- Hits 17802 16180 -1622
- Misses 8110 9877 +1767
+ Partials 1926 1789 -137
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except a small comment.
pkg/controller/ipam/validate.go
Outdated
return false, fmt.Sprintf( | ||
"Range is invalid. IP version of range %s-%s differs from Pool IP version", r.Start, r.End) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Validate IP version and prefix length fields. Signed-off-by: Jianjun Shen <shenj@vmware.com>
d35af17
to
1808fe8
Compare
@@ -152,7 +176,7 @@ func TestEgressControllerValidateExternalIPPool(t *testing.T) { | |||
}, | |||
SubnetInfo: crdv1alpha2.SubnetInfo{ | |||
Gateway: "10:2400::01", | |||
PrefixLength: 64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why changing this? the new prefix /24 seems more unrealistic in ipv6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check to prefix length must <32 for IPv4, so this case will fail for that reason (which is covered by a earlier test case already) if the length is still 64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thanks for explanation.
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -152,7 +176,7 @@ func TestEgressControllerValidateExternalIPPool(t *testing.T) { | |||
}, | |||
SubnetInfo: crdv1alpha2.SubnetInfo{ | |||
Gateway: "10:2400::01", | |||
PrefixLength: 64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thanks for explanation.
/test-flexible-ipam-e2e |
Validate IP version and prefix length fields. Signed-off-by: Jianjun Shen <shenj@vmware.com>
Validate IP version and prefix length fields.
Signed-off-by: Jianjun Shen shenj@vmware.com