Fix the conversion and e2e

Signed-off-by: Nont <[email protected]>

Author: Nont

pkg/webhook/managedresource/managedresource_validating_webhook.go

@@ -11,7 +11,7 @@ import (
 	"net/http"
 
 	admissionv1 "k8s.io/api/admission/v1"
-	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
@@ -51,29 +51,41 @@ type managedResourceValidator struct {
 // Handle denies the resource admission if the request target object has a label or annotation key "fleet.azure.com".
 func (v *managedResourceValidator) Handle(_ context.Context, req admission.Request) admission.Response {
 	namespacedName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace}
+	klog.V(1).InfoS("handling resource", "operation", req.Operation, "subResource", req.SubResource, "namespacedName", namespacedName)
+
+	var objs []runtime.RawExtension
 	switch req.Operation {
-	case admissionv1.Create, admissionv1.Update, admissionv1.Delete:
-		klog.V(1).InfoS("handling resource", "operation", req.Operation, "subResource", req.SubResource, "namespacedName", namespacedName)
-		for _, obj := range []runtime.Object{req.OldObject.Object, req.Object.Object} {
-			labels, annotations, err := getLabelsAndAnnotations(obj)
-			if err != nil {
-				return admission.Errored(http.StatusInternalServerError, err)
-			}
-			if (managedByArm(labels) || managedByArm(annotations)) && !validation.IsAdminGroupUserOrWhiteListedUser(v.whiteListedUsers, req.UserInfo) {
-				klog.V(2).InfoS(deniedResource, "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
-				return admission.Denied(fmt.Sprintf(resourceDeniedFormat, req.Kind, req.Name, req.Namespace))
-			}
+	case admissionv1.Create:
+		objs = append(objs, req.Object)
+	case admissionv1.Update:
+		objs = append(objs, req.Object, req.OldObject)
+	case admissionv1.Delete:
+		objs = append(objs, req.OldObject)
+	}
+	for _, obj := range objs {
+		labels, annotations, err := getLabelsAndAnnotations(obj)
+		if err != nil {
+			return admission.Errored(http.StatusInternalServerError, err)
+		}
+		if (managedByArm(labels) || managedByArm(annotations)) && !validation.IsAdminGroupUserOrWhiteListedUser(v.whiteListedUsers, req.UserInfo) {
+			klog.V(2).InfoS(deniedResource, "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
+			return admission.Denied(fmt.Sprintf(resourceDeniedFormat, req.Kind, req.Name, req.Namespace))
 		}
 	}
 	return admission.Allowed("")
 }
 
-func getLabelsAndAnnotations(obj runtime.Object) (map[string]string, map[string]string, error) {
-	accessor, err := meta.Accessor(obj)
+func getLabelsAndAnnotations(raw runtime.RawExtension) (map[string]string, map[string]string, error) {
+	var obj runtime.Object
+	if err := runtime.Convert_runtime_RawExtension_To_runtime_Object(&raw, &obj, nil); err != nil {
+		return nil, nil, err
+	}
+	o, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
 	if err != nil {
 		return nil, nil, err
 	}
-	return accessor.GetLabels(), accessor.GetAnnotations(), nil
+	u := unstructured.Unstructured{Object: o}
+	return u.GetLabels(), u.GetAnnotations(), nil
 }
 
 func managedByArm(m map[string]string) bool {

pkg/webhook/managedresource/managedresource_validating_webhook_test.go

@@ -20,7 +20,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
-func Test_managedResourceValidzaator_Handle(t *testing.T) {
+func Test_managedResourceValidator_Handle(t *testing.T) {
 	const fleet1p = "fleet1p"
 	validator := &managedResourceValidator{
 		whiteListedUsers: []string{fleet1p},
@@ -49,9 +49,9 @@ func Test_managedResourceValidzaator_Handle(t *testing.T) {
 		{
 			name:         "denied - error on getLabelsAndAnnotations failure",
 			operation:    admissionv1.Create,
-			expectedResp: admission.Errored(http.StatusInternalServerError, fmt.Errorf("object does not implement the Object interfaces")),
+			expectedResp: admission.Errored(http.StatusInternalServerError, fmt.Errorf("error decoding string from json: unexpected trailing data at offset 9")),
 			modReq: func(req *admission.Request) {
-				req.Object = runtime.RawExtension{Object: nil}
+				req.Object = runtime.RawExtension{Raw: []byte(`"invalid"}`)} // Invalid object without labels or annotations
 			},
 		},
 		{
@@ -125,24 +125,19 @@ func Test_managedResourceValidzaator_Handle(t *testing.T) {
 func Test_getLabelsAndAnnotations(t *testing.T) {
 	tests := []struct {
 		name            string
-		obj             runtime.Object
+		obj             runtime.RawExtension
 		wantLabels      map[string]string
 		wantAnnotations map[string]string
 		expectError     bool
 	}{
-		{
-			name:            "nil object - error",
-			obj:             nil,
-			wantLabels:      nil,
-			wantAnnotations: nil,
-			expectError:     true,
-		},
 		{
 			name: "object with labels and annotations",
-			obj: &metav1.PartialObjectMetadata{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels:      map[string]string{"foo": "bar"},
-					Annotations: map[string]string{"baz": "qux"},
+			obj: runtime.RawExtension{
+				Object: &metav1.PartialObjectMetadata{
+					ObjectMeta: metav1.ObjectMeta{
+						Labels:      map[string]string{"foo": "bar"},
+						Annotations: map[string]string{"baz": "qux"},
+					},
 				},
 			},
 			wantLabels:      map[string]string{"foo": "bar"},
@@ -151,8 +146,10 @@ func Test_getLabelsAndAnnotations(t *testing.T) {
 		},
 		{
 			name: "object with no labels or annotations",
-			obj: &metav1.PartialObjectMetadata{
-				ObjectMeta: metav1.ObjectMeta{},
+			obj: runtime.RawExtension{
+				Object: &metav1.PartialObjectMetadata{
+					ObjectMeta: metav1.ObjectMeta{},
+				},
 			},
 			wantLabels:      nil,
 			wantAnnotations: nil,

test/e2e/utils_test.go

@@ -709,18 +709,20 @@ func cleanupWorkResources() {
 	cleanWorkResourcesOnCluster(hubCluster)
 }
 
-func createManagedNamespace() corev1.Namespace {
-	ns := corev1.Namespace{
+func managedNamespace() corev1.Namespace {
+	return corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: fmt.Sprintf(managedNamespaceTemplate, GinkgoParallelProcess()),
 			Labels: map[string]string{
 				managedresource.ManagedByArmKey: managedresource.ManagedByArmValue,
 			},
 		},
 	}
+}
 
+func createManagedNamespace() {
+	ns := managedNamespace()
 	Expect(hubClient.Create(ctx, &ns)).To(Succeed(), "Failed to create namespace %s", ns.Name)
-	return ns
 }
 
 func cleanWorkResourcesOnCluster(cluster *framework.Cluster) {
@@ -833,6 +835,7 @@ func checkIfRemovedWorkResourcesFromMemberClustersConsistently(clusters []*frame
 		Consistently(workResourcesRemovedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to remove work resources from member cluster %s consistently", memberCluster.ClusterName)
 	}
 }
+
 func checkNamespaceExistsWithOwnerRefOnMemberCluster(nsName, crpName string) {
 	Consistently(func() error {
 		ns := &corev1.Namespace{}

test/e2e/webhook_test.go

@@ -1302,7 +1302,7 @@ var _ = Describe("webhook tests for operations on ARM managed resources", Ordere
 
 	AfterAll(func() {
 		By("deleting the managed namespace")
-		ns := createManagedNamespace()
+		ns := managedNamespace()
 		Expect(hubClient.Delete(ctx, &ns)).Should(SatisfyAny(Succeed(), utils.NotFoundMatcher{}), "Failed to delete the managed namespace")
 		Eventually(
 			func() error {
@@ -1315,53 +1315,58 @@ var _ = Describe("webhook tests for operations on ARM managed resources", Ordere
 
 	It("should deny create a managed resource namespace", func() {
 		Eventually(func(g Gomega) error {
-			createNs := createManagedNamespace()
-			createNs.Name = fmt.Sprintf("test-create-managed-ns-%d", GinkgoParallelProcess())
-			err := impersonateHubClient.Update(ctx, &createNs)
+			createNs := managedNamespace()
+			createNs.Name = "this-will-be-denied"
+			err := impersonateHubClient.Create(ctx, &createNs)
 			if k8sErrors.IsConflict(err) {
 				return err
 			}
 			var statusErr *k8sErrors.StatusError
 			g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create managed namespace call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
-			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("the operation on the managed resource type * is not allowed"))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(".*the operation on the managed resource type .* is not allowed"))
 			return nil
 		}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
 	})
 
 	It("should deny update a managed resource namespace", func() {
 		Eventually(func(g Gomega) error {
-			updateNamespace := createManagedNamespace()
+			updateNamespace := managedNamespace()
 			updateNamespace.Labels["foo"] = "NotManaged"
 			err := impersonateHubClient.Update(ctx, &updateNamespace)
 			if k8sErrors.IsConflict(err) {
 				return err
 			}
 			var statusErr *k8sErrors.StatusError
 			g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update managed namespace call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
-			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("the operation on the managed resource type * is not allowed"))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(".*the operation on the managed resource type .* is not allowed"))
 			return nil
 		}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
 	})
 
 	It("should deny delete a managed resource namespace", func() {
 		Eventually(func(g Gomega) error {
-			created := createManagedNamespace()
+			created := managedNamespace()
 			err := impersonateHubClient.Delete(ctx, &created)
 			if k8sErrors.IsConflict(err) {
 				return err
 			}
 			var statusErr *k8sErrors.StatusError
 			g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Delete managed namespace call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
-			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("the operation on the managed resource type * is not allowed"))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(".*the operation on the managed resource type .* is not allowed"))
 			return nil
 		}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
 	})
 
-	It("should allow create an unmanaged resource namespace", func() {
+	It("should allow create/update/delete an unmanaged resource namespace", func() {
 		Eventually(func(g Gomega) error {
-			creating := createManagedNamespace()
-			delete(creating.Labels, managedresource.ManagedByArmKey)
-			return impersonateHubClient.Create(ctx, &creating)
+			unmanaged := managedNamespace()
+			unmanaged.Name = "this-should-be-allowed"
+			delete(unmanaged.Labels, managedresource.ManagedByArmKey)
+			g.Expect(impersonateHubClient.Create(ctx, &unmanaged)).Should(SatisfyAny(Succeed(), utils.NotFoundMatcher{}), "Failed to create the unmanaged namespace")
+			unmanaged.Labels["foo"] = "NotManaged"
+			g.Expect(impersonateHubClient.Update(ctx, &unmanaged)).Should(SatisfyAny(Succeed(), utils.NotFoundMatcher{}), "Failed to update the unmanaged namespace")
+			g.Expect(impersonateHubClient.Delete(ctx, &unmanaged)).Should(SatisfyAny(Succeed(), utils.NotFoundMatcher{}), "Failed to delete the unmanaged namespace")
+			return nil
 		}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
 	})
 })