Replace forked net parsers with regexp hacks#330
Replace forked net parsers with regexp hacks#330danwinship wants to merge 1 commit intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
kind/cleanup
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold |
k8s-ci-robot
added
do-not-merge/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.
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.
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).
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
added
size/L
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
/close I did this because I thought Antonio wanted to get rid of the forked parsers, but he says he'd rather have those than regexp hacks. |
|
@danwinship: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
mandatory xkcd XD 
|
5 participants
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