pkg/validation: add selector validation #288
Conversation
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
cncf-cla: yes
- unexport validatePlatform as it's not used outside pkg/validation. - introduce validateSelector that validates selectors for: - unsupported keys - nil selector - empty selector (both for matchLabels and matchExpressions) as these situations are not supported, such as no plugin is valid for all platforms supported by krew. Signed-off-by: Ahmet Alp Balkan <[email protected]>
ahmetb
force-pushed
the
nil-platforms-and-selectors
branch
from
fb5e874 to
be01da8
Compare
Codecov Report
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
+ Coverage 54.84% 55.62% +0.78%
==========================================
Files 19 19
Lines 877 897 +20
==========================================
+ Hits 481 499 +18
- Misses 342 343 +1
- Partials 54 55 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks pretty good.
I've found one test-case, of which I'm not sure if you want it to pass or not. Judging from your PR description
empty selector (both for matchLabels and matchExpressions)
as these situations are not supported, such as no plugin is valid for all
platforms supported by krew.
My new test-case should not pass, but it does.
I think this wildcard case should be forbidden as well. There may be valid cases, where the same installation instruction applies to all platforms (say, a shell script with an .exe ending -- not pretty, but valid). However, in most cases, this indicates that the plugin author has not put enough thought into this. So let's forbid this now and if we find that this is too restrictive, we can still allow that case in the future.
| sel: nil, | ||
| wantErr: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
This test does not pass, and should be added:
{
name: "wildcard selector",
sel: &metav1.LabelSelector{},
wantErr: true,
},There was a problem hiding this comment.
That's a great catch. Added a commit.
Signed-off-by: Ahmet Alp Balkan <[email protected]>
| @@ -124,6 +124,9 @@ func validateSelector(sel *metav1.LabelSelector) error { | |||
| if sel == nil { | |||
| return errors.New("nil selector is not supported") | |||
| } | |||
| if sel.MatchLabels == nil && len(sel.MatchExpressions) == 0 { | |||
There was a problem hiding this comment.
To be on the safe side, this should be
if len(sel.MatchLabels) == 0 && len(sel.MatchExpressions) == 0 {Otherwise, sel.MatchLabels could be non-nil, but empty. And that would match everything, right?
There was a problem hiding this comment.
we have a check for that case below.
I wanted to distinguish error messages for these:
selector:
selector:
matchLabels:
which means different things.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
as these situations are not supported, such as no plugin is valid for all
platforms supported by krew.
Fixes #282, fixes #287.