pkg/validation: add selector validation (#288)

* 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 <[email protected]> * Address code review comments Signed-off-by: Ahmet Alp Balkan <[email protected]>
kubernetes-sigs · Jul 22, 2019 · 53019d6 · 53019d6
1 parent 009e0ea
commit 53019d6
Show file tree

Hide file tree

Showing 3 changed files with 136 additions and 33 deletions.
diff --git 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
@@ -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,41 @@ 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")
+	}
+	if sel.MatchLabels == nil && len(sel.MatchExpressions) == 0 {
+		return errors.New("empty 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
@@ -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)
 			}
 		})
 	}
@@ -210,14 +199,85 @@ 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:    "empty (wildcard) selector",
+			sel:     &metav1.LabelSelector{},
+			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)
 			}
 		})
 	}