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
Make CIDR validation consistent #123174
Make CIDR validation consistent #123174
Conversation
Move apivalidation.ValidateCIDR to apimachinery, and rename it and change its return value to match the other functions. Also, add unit tests. (Also, while updating NetworkPolicy validation for the API change, fix a variable name that implied that IPBlock.Except[] is IP-valued rather than CIDR-valued.)
/triage accepted |
In preparation for rewriting LoadBalancerSourceRanges validation, add/update the existing unit tests to cover some of the more exciting edge cases of the existing validation code: - The values in service.Spec.LoadBalancerSourceRanges are allowed to have arbitrary whitespace around them. - The annotation must be unset for non-LoadBalancer services, but for LoadBalancer services, "set but empty" and "whitespace-only" are treated the same as "unset". - The annotation value is only validated if the field is not set. Also fix some of the existing tests to be more precise about what they are testing. Also fix the CIDR values to actually be valid. Sigh.
Inline the LoadBalancerSourceRanges parsing to make it more obvious what it's requiring (and more importantly, *not* requiring), and change it to use IsValidCIDR as well.
f530895
to
593b1c6
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/lgtm |
LGTM label has been added. Git tree hash: 463208e40547983e42024f624e51912d9f0a3543
|
in: "2001:DB8::/64", | ||
}, | ||
|
||
// BAD VALUES WE CURRENTLY CONSIDER GOOD |
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.
:)
Epic! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, danwinship, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
/sig network
/sig api-machinery
What this PR does / why we need it:
Followup to #122931, another subset of #122550.
This cleans up validation of CIDRs, moving the validation function from
pkg/apis/core/validation
tok8s.io/apimachinery/pkg/util/validation
, and then rewriting the LoadBalancerSourceRanges validation to use it, rather than using a random function with crazy semantics. (Of course, we have to preserve the crazy semantics for backward-compatibility, but now they're explicit inpkg/apis/core/validation
rather than being hidden in the helper function.)As with #122931, this does not change any validation results, it just paves the way for making CIDR validation more strict later.
Does this PR introduce a user-facing change?