-
Notifications
You must be signed in to change notification settings - Fork 206
Replace forked net parsers with regexp hacks #330
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship 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 |
/hold |
"regexp" | ||
) | ||
|
||
var matchZeros = regexp.MustCompile(`(^|:)0*(0|[1-9][0-9]*)\.0*(0|[1-9][0-9]*)\.0*(0|[1-9][0-9]*)\.0*(0|[1-9][0-9]*)((/)0*(0|[1-9][0-9]*))?$`) |
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 like the idea of being able to not just tolerate an IP using old style parsing, but actually transform it into the acceptable version, so that we could conceivably scrub existing data and stop accepting leading 0s via the API at some point in the future.
as nits, if we were going to do this for real, I'd want to split the IP and CIDR cases so we match exactly what we intend to for each scenario, and probably try to factor out the matcher for the leading-zero number to make the overall structure easier to see:
var (
num = `0*(0|[1-9][0-9]*)`
dot = `\.`
leadingZeroIP = regexp.MustCompile(`(^|:)` + num + dot + num + dot + num + dot + num + `$`)
leadingZeroIPReplace = `$1$2.$3.$4.$5`
leadingZeroCIDR = regexp.MustCompile(`(^|:)` + num + dot + num + dot + num + dot + num + `/` + num + `$`)
leadingZeroCIDRReplace = `$1$2.$3.$4.$5/$6`
)
I'd also consider whether we wanted to use these cleaners externally to detect a field was relying on this (so we could warn) or to do the transform (so we could normalize or clean up existing or incoming data)
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.
to detect a field was relying on this (so we could warn)
KEP-4858 already took care of all of this (for non-CRD types). kubernetes/kubernetes#122550 split IP/CIDR validation into IsValid{IP,CIDR}
for new fields vs IsValid{IP,CIDR}ForLegacyField
, for older fields, and kubernetes/kubernetes#128786 added warnings when you're using a value with 0s in a legacy field.
As for transform/clean-up, that's been an ongoing discussion, most recently in #328 (comment) (which is the proximate cause of me filing this PR).
Proof-of-concept, since I keep mentioning we could do this if we wanted. I'm not sure we do want to, but...
Note that we only use the regexps if the initial attempt at parsing fails, so we only take the regexp hit on invalid or legacy inputs.
/kind cleanup
/cc @aojea @thockin