-
Notifications
You must be signed in to change notification settings - Fork 737
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
fix: over-restrictive validation of wildcard match patterns #3310
Conversation
…erns Signed-off-by: Ben Couture <Benjamin.Couture@LibertyMutual.com>
Signed-off-by: Ben Couture <bencouture@protonmail.com>
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.
1 small comment about tests
{ | ||
name: "no wildcard, only hypens at suffix and prefix", | ||
w: Wildcard("-kube-"), | ||
candidate: "test-kube-test", | ||
matches: false, | ||
}, | ||
{ | ||
name: "wild card at suffix and prefix, multiple hyphens", |
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.
Is there a reason tests were removed? Pre-existing tests should still pass with this change?
Thanks for the PR! It looks like |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3310 +/- ##
==========================================
- Coverage 54.49% 54.31% -0.18%
==========================================
Files 134 134
Lines 12329 10258 -2071
==========================================
- Hits 6719 5572 -1147
+ Misses 5116 4180 -936
- Partials 494 506 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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, thanks for the PR!
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
Thanks for the PR!
…icy-agent#3310) Signed-off-by: Ben Couture <Benjamin.Couture@LibertyMutual.com> Signed-off-by: Ben Couture <bencouture@protonmail.com> Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>
What this PR does / why we need it:
validating regex update
The regex used for field validation in the CRDs for wildcard match patterns is:
^(\*|\*-)?[a-z0-9]([-:a-z0-9]*[a-z0-9])?(\*|-\*)?$
It looks to me like this is pretty much just the same regex that the Kubernetes apimachinery package uses for validating resource names, just with
(\*|\*-)?
tacked on the front and(\*|-\*)?
tacked on the end.Apimachinery regex:
const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
https://github.com/kubernetes/kubernetes/blob/466d9378dbb0a185df9680657f5cd96d5e5aab57/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L177
The problem is that that regex is designed to validate a full, valid resource name, but by definition a wildcard match only includes a partial resource name. So the validating regex is saying "you must provide a valid DNS name with an optional wildcard symbol at the beginning or end" when what should really be expected is "you must provide a partial valid DNS name". The validation should just check that no invalid characters are supplied, but other than that I don't think it should restrict the value.
As a specific example of how the regex is over-restrictive, dns1123 allows multiple hyphens in a row.
unusual--------name
is a valid resource name, and Kubernetes will accept that. If I try to use Gatekeeper to match on*--*
, the Gatekeeper resource is rejected for failed field validation, even though it's still a perfectly valid wildcard match.field documentation update
I just updated the language explaining the match fields to explicitly say that you can use a prefix and suffix match at the same time. That wasn't initially clear to me from the existing documentation.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged): noneFixes #
Special notes for your reviewer:
The regex has a colon (
:
) in it as a valid character. Does anybody know why? That's not actually a valid DNS1123 character, according to the RFC, and Kubernetes rejects resources with a:
in the name. I initially was going to remove it, but was concerned that it might be for some edge case I'm not aware of. The test suite still passed when I removed it, so there aren't any tests that rely on it. I would propose removing it if nobody can think of a reason why it's there.