Fork Go's ParseIP and ParseCIDR #207
Conversation
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
approved
There was a problem hiding this comment.
Do you envision trying to transition API validation to require new objects to pass the stricter stdlib methods over time?
ALL KUBERNETES CODE should use these instead of the standard equivalents. They are identical in every way except the parsing of leading zeros.
That's not going to stick (in this repo or in kubernetes/kubernetes) unless we have some sort of static analysis ensuring that happens. Any ideas for that (starting in this repo would be a good first step).
| var parseIPTests = []struct { | ||
| in string | ||
| out IP | ||
| }{ | ||
| {"127.0.1.2", IPv4(127, 0, 1, 2)}, | ||
| {"127.0.0.1", IPv4(127, 0, 0, 1)}, | ||
| {"127.001.002.003", IPv4(127, 1, 2, 3)}, // see https://issue.k8s.io/100895 |
There was a problem hiding this comment.
one that demonstrates it parses as decimal as well?
{"127.010.020.030", IPv4(127, 10, 20, 30)}, // see https://issue.k8s.io/100895
There was a problem hiding this comment.
good call. Also will test 007 vs 008 to prove octal's edge
|
lgtm, just a couple nits/questions and the painful question of how we ensure consistent use |
This is from go ed01ceaf4838cd67fd802df481769fa9ae9d0440 See kubernetes/kubernetes#100895
Also add a few test cases See kubernetes/kubernetes#100895
| var parseIPTests = []struct { | ||
| in string | ||
| out IP | ||
| }{ | ||
| {"127.0.1.2", IPv4(127, 0, 1, 2)}, | ||
| {"127.0.0.1", IPv4(127, 0, 0, 1)}, | ||
| {"127.001.002.003", IPv4(127, 1, 2, 3)}, // see https://issue.k8s.io/100895 |
There was a problem hiding this comment.
good call. Also will test 007 vs 008 to prove octal's edge
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, 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 |
|
side-benefit of a different name is that a static checker could be easier to write this is about as dumb a check as I can imagine but would catch a lot in k/k |
/kind bug
What this PR does / why we need it:
This forks Go's
ParseIP()andParseCIDR()functions and reverts https://go-review.googlesource.com/c/go/+/325829/ , which changed the parsing to error on leading zeros. We don't know if anyone has stored objects with those leading zeros, so this represents an unknown break for us.Which issue(s) this PR fixes:
xref kubernetes/kubernetes#100895
Release note:
cc @liggitt