From be01da8c3535db550a28f4bd308fbab3d1bf6573 Mon Sep 17 00:00:00 2001 From: Ahmet Alp Balkan Date: Sun, 21 Jul 2019 20:40:34 -0700 Subject: [PATCH 1/2] pkg/validation: add selector validation - 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 --- .../testdata/testindex/plugins/bar.yaml | 6 + pkg/index/validation/validate.go | 40 +++++- pkg/index/validation/validate_test.go | 115 +++++++++++++----- 3 files changed, 128 insertions(+), 33 deletions(-) diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml index c9b5dbf8..362f1742 100644 --- a/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml +++ b/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml @@ -24,9 +24,15 @@ spec: uri: https://example.com sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef bin: kubectl-bar + selector: + matchLabels: + os: windows - files: - from: "*" uri: https://example.com sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef bin: kubectl-bar + selector: + matchLabels: + os: linux shortDescription: "exists" diff --git a/pkg/index/validation/validate.go b/pkg/index/validation/validate.go index 1a3d9e13..0467d4d2 100644 --- a/pkg/index/validation/validate.go +++ b/pkg/index/validation/validate.go @@ -20,6 +20,7 @@ import ( "strings" "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/krew/pkg/constants" "sigs.k8s.io/krew/pkg/index" @@ -88,15 +89,15 @@ func ValidatePlugin(name string, p index.Plugin) error { return errors.Wrap(err, "failed to parse plugin version") } for _, pl := range p.Spec.Platforms { - if err := ValidatePlatform(pl); err != nil { + if err := validatePlatform(pl); err != nil { return errors.Wrapf(err, "platform (%+v) is badly constructed", pl) } } return nil } -// ValidatePlatform checks Platform for structural validity. -func ValidatePlatform(p index.Platform) error { +// validatePlatform checks Platform for structural validity. +func validatePlatform(p index.Platform) error { if p.URI == "" { return errors.New("URI has to be set") } @@ -112,5 +113,38 @@ func ValidatePlatform(p index.Platform) error { if len(p.Files) == 0 { return errors.New("can't have a plugin without specifying file operations") } + if err := validateSelector(p.Selector); err != nil { + return errors.Wrap(err, "invalid platform selector") + } + return nil +} + +// validateSelector checks if the platform selector uses supported keys and is not empty or nil. +func validateSelector(sel *metav1.LabelSelector) error { + if sel == nil { + return errors.New("nil selector is not supported") + } + + // check for unsupported keys + keys := []string{} + for k := range sel.MatchLabels { + keys = append(keys, k) + } + for _, expr := range sel.MatchExpressions { + keys = append(keys, expr.Key) + } + for _, key := range keys { + if key != "os" && key != "arch" { + return errors.Errorf("key %q not supported", key) + } + } + + if sel.MatchLabels != nil && len(sel.MatchLabels) == 0 { + return errors.New("matchLabels specified but empty") + } + if sel.MatchExpressions != nil && len(sel.MatchExpressions) == 0 { + return errors.New("matchExpressions specified but empty") + } + return nil } diff --git a/pkg/index/validation/validate_test.go b/pkg/index/validation/validate_test.go index 7d7a2a86..ac2a6772 100644 --- a/pkg/index/validation/validate_test.go +++ b/pkg/index/validation/validate_test.go @@ -25,47 +25,36 @@ import ( ) func Test_IsSafePluginName(t *testing.T) { - type args struct { - name string - } tests := []struct { - name string - args args - want bool + name string + pluginName string + want bool }{ { - name: "secure name", - args: args{ - name: "foo-bar", - }, - want: true, + name: "secure name", + pluginName: "foo-bar", + want: true, }, { - name: "insecure path name", - args: args{ - name: "/foo-bar", - }, - want: false, + name: "insecure path name", + pluginName: "/foo-bar", + want: false, }, { - name: "relative name", - args: args{ - name: "..foo-bar", - }, - want: false, + name: "relative name", + pluginName: "..foo-bar", + want: false, }, { - name: "bad windows name", - args: args{ - name: "nul", - }, - want: false, + name: "bad windows name", + pluginName: "nul", + want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := IsSafePluginName(tt.args.name); got != tt.want { - t.Errorf("IsSafePluginName() = %v, want %v", got, tt.want) + if got := IsSafePluginName(tt.pluginName); got != tt.want { + t.Errorf("IsSafePluginName(%s) = %v, want %v", tt.pluginName, got, tt.want) } }) } @@ -204,14 +193,80 @@ func TestValidatePlatform(t *testing.T) { platform: testutil.NewPlatform().WithBin("").V(), wantErr: true, }, + { + name: "invalid platform selector", + platform: testutil.NewPlatform().WithSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{"unsupported-field": "orange"}}).V(), + wantErr: true, + }, // TODO(ahmetb): add test case "bin field outside the plugin installation directory" // by testing .WithBin("foo/../../../malicious-file"). // It appears like currently we're allowing this. } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := ValidatePlatform(tt.platform); (err != nil) != tt.wantErr { - t.Errorf("ValidatePlatform() error = %v, wantErr %v", err, tt.wantErr) + if err := validatePlatform(tt.platform); (err != nil) != tt.wantErr { + t.Errorf("validatePlatform() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func Test_validateSelector(t *testing.T) { + var tests = []struct { + name string + sel *metav1.LabelSelector + wantErr bool + }{ + { + name: "nil selector", + sel: nil, + wantErr: true, + }, + { + name: "valid matchLabels", + sel: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "foo", "arch": "bar"}}, + wantErr: false, + }, + { + name: "valid matchExpressions", + sel: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "os", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"apple", "orange"}, + }}}, + wantErr: false, + }, + { + name: "empty matchLabels", + sel: &metav1.LabelSelector{MatchLabels: map[string]string{}}, + wantErr: true, + }, + { + name: "empty matchExpressions", + sel: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{}}, + wantErr: true, + }, + { + name: "unsupported key in matchLabels", + sel: &metav1.LabelSelector{MatchLabels: map[string]string{"unsupported-key": "value"}}, + wantErr: true, + }, + { + name: "unsupported key in matchExpressions", + sel: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "unsupported-key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"apple", "orange"}}}}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validateSelector(tt.sel); (err != nil) != tt.wantErr { + t.Errorf("validateSelector() error = %v, wantErr %v", err, tt.wantErr) } }) } From 1ec2910867a3c9cdca23893b0a82ae10816262b7 Mon Sep 17 00:00:00 2001 From: Ahmet Alp Balkan Date: Mon, 22 Jul 2019 10:00:32 -0700 Subject: [PATCH 2/2] Address code review comments Signed-off-by: Ahmet Alp Balkan --- pkg/index/validation/validate.go | 3 +++ pkg/index/validation/validate_test.go | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/pkg/index/validation/validate.go b/pkg/index/validation/validate.go index 0467d4d2..113f4cb2 100644 --- a/pkg/index/validation/validate.go +++ b/pkg/index/validation/validate.go @@ -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 { + return errors.New("empty selector is not supported") + } // check for unsupported keys keys := []string{} diff --git a/pkg/index/validation/validate_test.go b/pkg/index/validation/validate_test.go index ac2a6772..ff45dcc6 100644 --- a/pkg/index/validation/validate_test.go +++ b/pkg/index/validation/validate_test.go @@ -223,6 +223,11 @@ func Test_validateSelector(t *testing.T) { sel: nil, wantErr: true, }, + { + name: "empty (wildcard) selector", + sel: &metav1.LabelSelector{}, + wantErr: true, + }, { name: "valid matchLabels", sel: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "foo", "arch": "bar"}},