Validations of managed labels/annotations

See kubernetes-retired#47. This adds webhook validation of managed labels and annotations. Integration tests should be added for this new feature, that included verifying validations.

Tested: Added unit tests for validations. Ran integration tests, but no additions there.

Author: erikgb

internal/hierarchyconfig/validator.go

@@ -7,14 +7,16 @@ import (
 	"strings"
 
 	"github.com/go-logr/logr"
-	k8sadm "k8s.io/api/admission/v1"
 	authnv1 "k8s.io/api/authentication/v1"
 	authzv1 "k8s.io/api/authorization/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
@@ -129,6 +131,38 @@ func (v *Validator) handle(ctx context.Context, log logr.Logger, req *request) a
 		return webhooks.Deny(metav1.StatusReasonForbidden, reason)
 	}
 
+	allErrs := field.ErrorList{}
+
+	fldPath := field.NewPath("spec", "labels")
+	for _, l := range req.hc.Spec.Labels {
+		if err := validateMetaKey(l.Key, fldPath); err != nil {
+			allErrs = append(allErrs, err)
+		} else if !config.IsManagedLabel(l.Key) { // Only validate managed if key is valid
+			fldErr := field.Invalid(fldPath, l.Key, "not a managed label and cannot be configured")
+			allErrs = append(allErrs, fldErr)
+		}
+		if err := validateLabelValue(l.Key, l.Value, fldPath); err != nil {
+			allErrs = append(allErrs, err)
+		}
+	}
+
+	fldPath = field.NewPath("spec", "annotations")
+	for _, a := range req.hc.Spec.Annotations {
+		if err := validateMetaKey(a.Key, fldPath); err != nil {
+			allErrs = append(allErrs, err)
+		} else if !config.IsManagedAnnotation(a.Key) { // Only validate managed if key is valid
+			fldErr := field.Invalid(fldPath, a.Key, "not a managed annotation and cannot be configured")
+			allErrs = append(allErrs, fldErr)
+		}
+		// No validation for annotation values; only limitation seems to be the total length (256Kb) of all annotations
+	}
+
+	if len(allErrs) > 0 {
+		gk := schema.GroupKind{Group: api.GroupVersion.Group, Kind: "HierarchyConfiguration"}
+		err := apierrors.NewInvalid(gk, req.hc.Name, allErrs)
+		return webhooks.DenyFromAPIError(err)
+	}
+
 	// Do all checks that require holding the in-memory lock. Generate a list of server checks we
 	// should perform once the lock is released.
 	serverChecks, resp := v.checkForest(log, req.hc)
@@ -514,14 +548,20 @@ func (r *realClient) IsAdmin(ctx context.Context, ui *authnv1.UserInfo, nnm stri
 	return sar.Status.Allowed, err
 }
 
-// allow is a replacement for controller-runtime's admission.Allowed() that allows you to set the
-// message (human-readable) as opposed to the reason (machine-readable).
+func validateMetaKey(k string, path *field.Path) *field.Error {
+	if errs := validation.IsQualifiedName(k); len(errs) != 0 {
+		return field.Invalid(path, k, strings.Join(errs, "; "))
+	}
+	return nil
+}
+
+func validateLabelValue(k, v string, path *field.Path) *field.Error {
+	if errs := validation.IsValidLabelValue(v); len(errs) != 0 {
+		return field.Invalid(path.Key(k), v, strings.Join(errs, "; "))
+	}
+	return nil
+}
+
 func allow(msg string) admission.Response {
-	return admission.Response{AdmissionResponse: k8sadm.AdmissionResponse{
-		Allowed: true,
-		Result: &metav1.Status{
-			Code:    0,
-			Message: msg,
-		},
-	}}
+	return webhooks.Allow(msg)
 }

internal/hierarchyconfig/validator_test.go

@@ -17,6 +17,101 @@ import (
 	"sigs.k8s.io/hierarchical-namespaces/internal/objects"
 )
 
+func TestManagedMetaPermissions(t *testing.T) {
+	f := foresttest.Create("-") // a
+	h := &Validator{Forest: f}
+	l := zap.New()
+
+	tests := []struct {
+		name        string
+		labels      []api.MetaKVP
+		annotations []api.MetaKVP
+		allowed     bool
+	}{
+		{name: "managed label", labels: []api.MetaKVP{{Key: "label.com/team"}}, allowed: true},
+		{name: "unmanaged label", labels: []api.MetaKVP{{Key: "kubernetes.io/metadata.name"}}},
+		{name: "managed annotation", annotations: []api.MetaKVP{{Key: "annot.com/log-index"}}, allowed: true},
+		{name: "unmanaged annotation", annotations: []api.MetaKVP{{Key: "openshift.io/sa.scc.uid-range"}}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup
+			g := NewWithT(t)
+			// In this test we only allow labels/annotations with the defined prefixes
+			err := config.SetManagedMeta([]string{"label\\.com/.*"}, []string{"annot\\.com/.*"})
+			g.Expect(err).ToNot(HaveOccurred())
+
+			hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{}}
+			hc.ObjectMeta.Name = api.Singleton
+			hc.ObjectMeta.Namespace = "a"
+			hc.Spec.Labels = tc.labels
+			hc.Spec.Annotations = tc.annotations
+			req := &request{hc: hc}
+
+			got := h.handle(context.Background(), l, req)
+
+			logResult(t, got.AdmissionResponse.Result)
+			g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allowed))
+		})
+	}
+}
+
+func TestManagedMetaSyntax(t *testing.T) {
+	f := foresttest.Create("-") // a
+	h := &Validator{Forest: f}
+	l := zap.New()
+
+	tests := []struct {
+		name        string
+		labels      []api.MetaKVP
+		annotations []api.MetaKVP
+		allowed     bool
+	}{
+		{name: "ok: prefixed label key", labels: []api.MetaKVP{{Key: "foo.bar/team", Value: "v"}}, allowed: true},
+		{name: "ok: bare label key", labels: []api.MetaKVP{{Key: "team", Value: "v"}}, allowed: true},
+		{name: "invalid: label prefix key", labels: []api.MetaKVP{{Key: "foo;bar/team", Value: "v"}}},
+		{name: "invalid: label name key", labels: []api.MetaKVP{{Key: "foo.bar/-team", Value: "v"}}},
+		{name: "invalid: empty label key", labels: []api.MetaKVP{{Key: "", Value: "v"}}},
+
+		{name: "ok: label value", labels: []api.MetaKVP{{Key: "k", Value: "foo"}}, allowed: true},
+		{name: "ok: empty label value", labels: []api.MetaKVP{{Key: "k", Value: ""}}, allowed: true},
+		{name: "ok: label value special char", labels: []api.MetaKVP{{Key: "k", Value: "f-oo"}}, allowed: true},
+		{name: "invalid: label value", labels: []api.MetaKVP{{Key: "k", Value: "-foo"}}},
+
+		{name: "ok: prefixed annotation key", annotations: []api.MetaKVP{{Key: "foo.bar/team", Value: "v"}}, allowed: true},
+		{name: "ok: bare annotation key", annotations: []api.MetaKVP{{Key: "team", Value: "v"}}, allowed: true},
+		{name: "invalid: annotation prefix key", annotations: []api.MetaKVP{{Key: "foo;bar/team", Value: "v"}}},
+		{name: "invalid: annotation name key", annotations: []api.MetaKVP{{Key: "foo.bar/-team", Value: "v"}}},
+		{name: "invalid: empty annotation key", annotations: []api.MetaKVP{{Key: "", Value: "v"}}},
+
+		{name: "ok: annotation value", annotations: []api.MetaKVP{{Key: "k", Value: "foo"}}, allowed: true},
+		{name: "ok: empty annotation value", annotations: []api.MetaKVP{{Key: "k", Value: ""}}, allowed: true},
+		{name: "ok: special annotation value", annotations: []api.MetaKVP{{Key: "k", Value: ";$+:;/*'\""}}, allowed: true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup
+			g := NewWithT(t)
+			// For this test we accept any label or annotation not starting with 'h'
+			// to allow almost anything - except the hnc.x-k8s.io labels/annotations
+			err := config.SetManagedMeta([]string{"[^h].*"}, []string{"[^h].*"})
+			g.Expect(err).ToNot(HaveOccurred())
+
+			hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{}}
+			hc.ObjectMeta.Name = api.Singleton
+			hc.ObjectMeta.Namespace = "a"
+			hc.Spec.Labels = tc.labels
+			hc.Spec.Annotations = tc.annotations
+			req := &request{hc: hc}
+
+			got := h.handle(context.Background(), l, req)
+
+			logResult(t, got.AdmissionResponse.Result)
+			g.Expect(got.AdmissionResponse.Allowed).Should(Equal(tc.allowed))
+		})
+	}
+}
+
 func TestStructure(t *testing.T) {
 	f := foresttest.Create("-a-") // a <- b; c
 	h := &Validator{Forest: f}