feat: Role & Role binding webhook

apis/placement/v1beta1/commons.go

@@ -13,16 +13,16 @@ import (
 type ClusterState string
 
 const (
-	// Unprefixed labels/annotations are reserved for end-users
-	// we will add a placement.karavel.io to designate these labels/annotations as official fleet labels/annotations.
+	// FleetPrefix placement.karavel.io will be used to designate these labels/annotations as official fleet labels/annotations.
 	// See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#label-selector-and-annotation-conventions
-	fleetPrefix = "placement.karavel.io/"
+	// Non-prefixed labels/annotations are reserved for end-users
+	FleetPrefix = "placement.karavel.io/"
 
 	// CRPTrackingLabel is the label that points to the cluster resource policy that creates a resource binding.
-	CRPTrackingLabel = fleetPrefix + "parentCRP"
+	CRPTrackingLabel = FleetPrefix + "parentCRP"
 
 	// IsLatestSnapshotLabel tells if the snapshot is the latest one.
-	IsLatestSnapshotLabel = fleetPrefix + "isLatestSnapshot"
+	IsLatestSnapshotLabel = FleetPrefix + "isLatestSnapshot"
 )
 
 const (

apis/placement/v1beta1/policysnapshot_types.go

@@ -12,13 +12,13 @@ import (
 
 const (
 	// PolicyIndexLabel is the label that indicate the policy snapshot index of a cluster policy.
-	PolicyIndexLabel = fleetPrefix + "policyIndex"
+	PolicyIndexLabel = FleetPrefix + "policyIndex"
 
 	// PolicySnapshotNameFmt is clusterPolicySnapshot name format: {CRPName}-{PolicySnapshotIndex}.
 	PolicySnapshotNameFmt = "%s-%d"
 
 	// NumberOfClustersAnnotation is the annotation that indicates how many clusters should be selected for selectN placement type.
-	NumberOfClustersAnnotation = fleetPrefix + "numberOfClusters"
+	NumberOfClustersAnnotation = FleetPrefix + "numberOfClusters"
 )
 
 // +genclient

apis/placement/v1beta1/resourcesnapshot_types.go

@@ -13,14 +13,14 @@ import (
 
 const (
 	// ResourceIndexLabel is the label that indicate the resource snapshot index of a cluster resource snapshot.
-	ResourceIndexLabel = fleetPrefix + "resourceIndex"
+	ResourceIndexLabel = FleetPrefix + "resourceIndex"
 
 	// ResourceGroupHashAnnotation is the annotation that contains the value of the sha-256 hash
 	// value of all the snapshots belong to the same snapshot index.
-	ResourceGroupHashAnnotation = fleetPrefix + "resourceHash"
+	ResourceGroupHashAnnotation = FleetPrefix + "resourceHash"
 
 	// NumberOfResourceSnapshotsAnnotation is the annotation that contains the total number of resource snapshots.
-	NumberOfResourceSnapshotsAnnotation = fleetPrefix + "numberOfResourceSnapshots"
+	NumberOfResourceSnapshotsAnnotation = FleetPrefix + "numberOfResourceSnapshots"
 )
 
 // +genclient

pkg/controllers/membercluster/membercluster_controller.go

@@ -29,13 +29,15 @@ import (
 	workv1alpha1 "sigs.k8s.io/work-api/pkg/apis/v1alpha1"
 
 	"go.goms.io/fleet/apis"
+	fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
 	fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
 	"go.goms.io/fleet/pkg/metrics"
 	"go.goms.io/fleet/pkg/utils"
 )
 
 const (
 	eventReasonNamespaceCreated       = "NamespaceCreated"
+	eventReasonNamespaceUpdated       = "NamespaceUpdated"
 	eventReasonRoleCreated            = "RoleCreated"
 	eventReasonRoleUpdated            = "RoleUpdated"
 	eventReasonRoleBindingCreated     = "RoleBindingCreated"
@@ -47,6 +49,9 @@ const (
 	reasonMemberClusterJoined         = "MemberClusterJoined"
 	reasonMemberClusterLeft           = "MemberClusterLeft"
 	reasonMemberClusterUnknown        = "MemberClusterUnknown"
+
+	FleetResourceLabelKey = fleetv1beta1.FleetPrefix + "isFleetResource"
+	FleetNamespaceValue   = "fleet-namespace"
 )
 
 // Reconciler reconciles a MemberCluster object
@@ -232,6 +237,7 @@ func (r *Reconciler) syncNamespace(ctx context.Context, mc *fleetv1alpha1.Member
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            namespaceName,
 			OwnerReferences: []metav1.OwnerReference{*toOwnerReference(mc)},
+			Labels:          map[string]string{FleetResourceLabelKey: FleetNamespaceValue},
 		},
 	}
 
@@ -251,8 +257,17 @@ func (r *Reconciler) syncNamespace(ctx context.Context, mc *fleetv1alpha1.Member
 		return namespaceName, nil
 	}
 
-	// TODO: Update namespace if currentNS != expectedNS.
-
+	// Updates namespace if currentNS != expectedNS.
+	if cmp.Equal(currentNS.Labels, expectedNS.Labels) {
+		return namespaceName, nil
+	}
+	currentNS.Labels = expectedNS.Labels
+	klog.V(2).InfoS("updating namespace", "memberCluster", klog.KObj(mc), "namespace", namespaceName)
+	if err := r.Client.Update(ctx, &currentNS, client.FieldOwner(utils.MCControllerFieldManagerName)); err != nil {
+		return "", fmt.Errorf("failed to update namespace %s: %w", namespaceName, err)
+	}
+	r.recorder.Event(mc, corev1.EventTypeNormal, eventReasonNamespaceUpdated, "Namespace was updated")
+	klog.V(2).InfoS("updated namespace", "memberCluster", klog.KObj(mc), "namespace", namespaceName)
 	return namespaceName, nil
 }
 
@@ -292,7 +307,7 @@ func (r *Reconciler) syncRole(ctx context.Context, mc *fleetv1alpha1.MemberClust
 	currentRole.Rules = expectedRole.Rules
 	klog.V(2).InfoS("updating role", "memberCluster", klog.KObj(mc), "role", roleName)
 	if err := r.Client.Update(ctx, &currentRole, client.FieldOwner(utils.MCControllerFieldManagerName)); err != nil {
-		return "", fmt.Errorf("failed to update role %s with rules %+v: %w", roleName, currentRole.Rules, err)
+		return "", fmt.Errorf("failed to update role %s: %w", roleName, err)
 	}
 	r.recorder.Event(mc, corev1.EventTypeNormal, eventReasonRoleUpdated, "role was updated")
 	klog.V(2).InfoS("updated role", "memberCluster", klog.KObj(mc), "role", roleName)

pkg/controllers/membercluster/membercluster_controller_test.go

@@ -42,13 +42,16 @@ func TestSyncNamespace(t *testing.T) {
 		wantedEvent         string
 		wantedError         string
 	}{
-		"namespace exists": {
+		"namespace exists but no diff": {
 			r: &Reconciler{
 				Client: &test.MockClient{
 					MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
 						o := obj.(*corev1.Namespace)
 						*o = corev1.Namespace{
-							ObjectMeta: metav1.ObjectMeta{Name: namespace1},
+							ObjectMeta: metav1.ObjectMeta{
+								Name:   namespace1,
+								Labels: map[string]string{FleetResourceLabelKey: FleetNamespaceValue},
+							},
 						}
 						return nil
 					},
@@ -58,6 +61,29 @@ func TestSyncNamespace(t *testing.T) {
 			wantedNamespaceName: namespace1,
 			wantedError:         "",
 		},
+		"namespace exists but with diff": {
+			r: &Reconciler{
+				Client: &test.MockClient{
+					MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
+						o := obj.(*corev1.Namespace)
+						*o = corev1.Namespace{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: namespace1,
+							},
+						}
+						return nil
+					},
+					MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+						return nil
+					},
+				},
+				recorder: utils.NewFakeRecorder(1),
+			},
+			memberCluster:       &fleetv1alpha1.MemberCluster{ObjectMeta: metav1.ObjectMeta{Name: "mc1"}},
+			wantedNamespaceName: namespace1,
+			wantedEvent:         utils.GetEventString(&fleetv1alpha1.MemberCluster{ObjectMeta: metav1.ObjectMeta{Name: "mc1"}}, corev1.EventTypeNormal, eventReasonNamespaceUpdated, "Namespace was updated"),
+			wantedError:         "",
+		},
 		"namespace doesn't exist": {
 			r: &Reconciler{
 				Client: &test.MockClient{
@@ -102,6 +128,27 @@ func TestSyncNamespace(t *testing.T) {
 			wantedNamespaceName: "",
 			wantedError:         "namespace cannot be retrieved",
 		},
+		"namespace update error": {
+			r: &Reconciler{
+				Client: &test.MockClient{
+					MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
+						o := obj.(*corev1.Namespace)
+						*o = corev1.Namespace{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: namespace1,
+							},
+						}
+						return nil
+					},
+					MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+						return errors.New("namespace cannot be updated")
+					},
+				},
+			},
+			memberCluster:       &fleetv1alpha1.MemberCluster{ObjectMeta: metav1.ObjectMeta{Name: "mc1"}},
+			wantedNamespaceName: "",
+			wantedError:         "namespace cannot be updated",
+		},
 	}
 
 	for testName, tt := range tests {

pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go

@@ -7,6 +7,7 @@ import (
 	"regexp"
 
 	admissionv1 "k8s.io/api/admission/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -26,6 +27,8 @@ const (
 	groupMatch        = `^[^.]*\.(.*)`
 	crdKind           = "CustomResourceDefinition"
 	memberClusterKind = "MemberCluster"
+	roleKind          = "Role"
+	roleBindingKind   = "RoleBinding"
 )
 
 // Add registers the webhook for K8s bulit-in object types.
@@ -51,6 +54,12 @@ func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Reque
 		case createMemberClusterGVK():
 			klog.V(2).InfoS("handling Member cluster resource", "GVK", createMemberClusterGVK())
 			response = v.handleMemberCluster(ctx, req)
+		case createRoleGVK():
+			klog.V(2).InfoS("handling Role resource", "GVK", createRoleGVK())
+			response = v.handleRole(req)
+		case createRoleBindingGVK():
+			klog.V(2).InfoS("handling Role binding resource", "GVK", createRoleBindingGVK())
+			response = v.handleRoleBinding(req)
 		default:
 			klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.Kind.String())
 			response = admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify resource with GVK: %s", req.UserInfo.Username, req.UserInfo.Groups, req.Kind.String()))
@@ -67,7 +76,7 @@ func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Resp
 
 	// This regex works because every CRD name in kubernetes follows this pattern <plural>.<group>.
 	group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1]
-	if validation.CheckCRDGroup(group) && !validation.ValidateUserForCRD(v.whiteListedUsers, req.UserInfo) {
+	if validation.CheckCRDGroup(group) && !validation.IsMasterGroupUserOrWhiteListedUser(v.whiteListedUsers, req.UserInfo) {
 		return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
 	}
 	return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
@@ -86,6 +95,24 @@ func (v *fleetResourceValidator) handleMemberCluster(ctx context.Context, req ad
 	return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify member cluster: %s", req.UserInfo.Username, req.UserInfo.Groups, mc.Name))
 }
 
+func (v *fleetResourceValidator) handleRole(req admission.Request) admission.Response {
+	var role rbacv1.Role
+	if err := v.decodeRequestObject(req, &role); err != nil {
+		return admission.Errored(http.StatusBadRequest, err)
+	}
+
+	return validation.ValidateUserForResource(v.whiteListedUsers, req.UserInfo, role.Kind, role.Name)
+}
+
+func (v *fleetResourceValidator) handleRoleBinding(req admission.Request) admission.Response {
+	var rb rbacv1.RoleBinding
+	if err := v.decodeRequestObject(req, &rb); err != nil {
+		return admission.Errored(http.StatusBadRequest, err)
+	}
+
+	return validation.ValidateUserForResource(v.whiteListedUsers, req.UserInfo, rb.Kind, rb.Name)
+}
+
 func (v *fleetResourceValidator) decodeRequestObject(req admission.Request, obj runtime.Object) error {
 	if req.Operation == admissionv1.Delete {
 		// req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762.
@@ -122,3 +149,19 @@ func createMemberClusterGVK() metav1.GroupVersionKind {
 		Kind:    memberClusterKind,
 	}
 }
+
+func createRoleGVK() metav1.GroupVersionKind {
+	return metav1.GroupVersionKind{
+		Group:   rbacv1.SchemeGroupVersion.Group,
+		Version: rbacv1.SchemeGroupVersion.Version,
+		Kind:    roleKind,
+	}
+}
+
+func createRoleBindingGVK() metav1.GroupVersionKind {
+	return metav1.GroupVersionKind{
+		Group:   rbacv1.SchemeGroupVersion.Group,
+		Version: rbacv1.SchemeGroupVersion.Version,
+		Kind:    roleBindingKind,
+	}
+}

pkg/webhook/validation/uservalidation.go

@@ -2,11 +2,13 @@ package validation
 
 import (
 	"context"
+	"fmt"
 
 	authenticationv1 "k8s.io/api/authentication/v1"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/strings/slices"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
 )
@@ -15,14 +17,9 @@ const (
 	mastersGroup = "system:masters"
 )
 
-// ValidateUserForCRD checks to see if user is authenticated to make a request to modify fleet CRDs.
-func ValidateUserForCRD(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
-	return isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo)
-}
-
-// ValidateUserForFleetCR checks to see if user is authenticated to make a request to modify Fleet CRs.
+// ValidateUserForFleetCR checks to see if user is allowed to make a request to modify fleet CRs.
 func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
-	if isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) {
+	if IsMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) {
 		return true
 	}
 	var memberClusterList fleetv1alpha1.MemberClusterList
@@ -38,6 +35,15 @@ func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteList
 	return slices.Contains(identities, userInfo.Username)
 }
 
-func isMasterGroupUserOrWhiteListedUser(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
+// ValidateUserForResource checks to see if user is allowed to modify argued fleet resource.
+func ValidateUserForResource(whiteListedUsers []string, userInfo authenticationv1.UserInfo, resKind, resName string) admission.Response {
+	if !IsMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) {
+		return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet resource %s: %s", userInfo.Username, userInfo.Groups, resKind, resName))
+	}
+	klog.V(2).InfoS("user in groups is allowed to modify fleet resource", "user", userInfo.Username, "groups", userInfo.Groups, resKind, resName)
+	return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify fleet resource %s: %s", userInfo.Username, userInfo.Groups, resKind, resName))
+}
+
+func IsMasterGroupUserOrWhiteListedUser(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool {
 	return slices.Contains(whiteListedUsers, userInfo.Username) || slices.Contains(userInfo.Groups, mastersGroup)
 }

pkg/webhook/validation/uservalidation_test.go

@@ -2,6 +2,7 @@ package validation
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/crossplane/crossplane-runtime/pkg/test"
@@ -10,44 +11,54 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
 	"go.goms.io/fleet/pkg/utils"
 )
 
-func TestValidateUserForCRD(t *testing.T) {
+func TestValidateUserForResource(t *testing.T) {
 	testCases := map[string]struct {
 		userInfo         v1.UserInfo
 		whiteListedUsers []string
-		wantResult       bool
+		resKind          string
+		resName          string
+		wantResponse     admission.Response
 	}{
 		"allow user in system:masters group": {
 			userInfo: v1.UserInfo{
 				Username: "test-user",
 				Groups:   []string{"system:masters"},
 			},
-			wantResult: true,
+			resKind:      "Role",
+			resName:      "test-role",
+			wantResponse: admission.Allowed(fmt.Sprintf("user: test-user in groups: [system:masters] is allowed to modify fleet resource Role: test-role")),
 		},
 		"allow white listed user not in system:masters group": {
 			userInfo: v1.UserInfo{
 				Username: "test-user",
+				Groups:   []string{"test-group"},
 			},
+			resKind:          "RoleBinding",
+			resName:          "test-role-binding",
 			whiteListedUsers: []string{"test-user"},
-			wantResult:       true,
+			wantResponse:     admission.Allowed(fmt.Sprintf("user: test-user in groups: [test-group] is allowed to modify fleet resource RoleBinding: test-role-binding")),
 		},
 		"fail to validate user with invalid username, groups": {
 			userInfo: v1.UserInfo{
 				Username: "test-user",
 				Groups:   []string{"test-group"},
 			},
-			wantResult: false,
+			resKind:      "Role",
+			resName:      "test-role",
+			wantResponse: admission.Denied(fmt.Sprintf("failed to validate user: test-user in groups: [test-group] to modify fleet resource Role: test-role")),
 		},
 	}
 
 	for testName, testCase := range testCases {
 		t.Run(testName, func(t *testing.T) {
-			gotResult := ValidateUserForCRD(testCase.whiteListedUsers, testCase.userInfo)
-			assert.Equal(t, testCase.wantResult, gotResult, utils.TestCaseMsg, testName)
+			gotResult := ValidateUserForResource(testCase.whiteListedUsers, testCase.userInfo, testCase.resKind, testCase.resName)
+			assert.Equal(t, testCase.wantResponse, gotResult, utils.TestCaseMsg, testName)
 		})
 	}
 }