From db386b31f046189e5ae12bb687266ec3291b158a Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Thu, 22 Jun 2023 12:17:21 -0700 Subject: [PATCH 1/5] Add label to role, rolebindings & namespace --- apis/placement/v1beta1/commons.go | 10 ++-- .../placement/v1beta1/policysnapshot_types.go | 4 +- .../v1beta1/resourcesnapshot_types.go | 6 +-- .../membercluster/membercluster_controller.go | 21 ++++++-- .../membercluster_controller_test.go | 51 ++++++++++++++++++- 5 files changed, 77 insertions(+), 15 deletions(-) diff --git a/apis/placement/v1beta1/commons.go b/apis/placement/v1beta1/commons.go index 9b59e1646..8ca7b1097 100644 --- a/apis/placement/v1beta1/commons.go +++ b/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 ( diff --git a/apis/placement/v1beta1/policysnapshot_types.go b/apis/placement/v1beta1/policysnapshot_types.go index 7ff78402e..4c2938550 100644 --- a/apis/placement/v1beta1/policysnapshot_types.go +++ b/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 diff --git a/apis/placement/v1beta1/resourcesnapshot_types.go b/apis/placement/v1beta1/resourcesnapshot_types.go index 7caf14d42..e3a66517e 100644 --- a/apis/placement/v1beta1/resourcesnapshot_types.go +++ b/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 diff --git a/pkg/controllers/membercluster/membercluster_controller.go b/pkg/controllers/membercluster/membercluster_controller.go index 9292ac1c3..e34f10554 100644 --- a/pkg/controllers/membercluster/membercluster_controller.go +++ b/pkg/controllers/membercluster/membercluster_controller.go @@ -29,6 +29,7 @@ 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" @@ -36,6 +37,7 @@ import ( 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, ¤tNS, 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, ¤tRole, 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) diff --git a/pkg/controllers/membercluster/membercluster_controller_test.go b/pkg/controllers/membercluster/membercluster_controller_test.go index 1c1b83a01..2d073cd6d 100644 --- a/pkg/controllers/membercluster/membercluster_controller_test.go +++ b/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 { From 1f121bf8c351334f3bbf2b0b1159a9f45a2a88a3 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 21 Jun 2023 13:07:14 -0700 Subject: [PATCH 2/5] Fleet Role webhook --- .../fleetresourcehandler_webhook.go | 26 ++++++++++++++++++ pkg/webhook/validation/uservalidation.go | 12 +++++++-- pkg/webhook/webhook.go | 27 +++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index bc9650b03..30b366197 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/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,7 @@ const ( groupMatch = `^[^.]*\.(.*)` crdKind = "CustomResourceDefinition" memberClusterKind = "MemberCluster" + roleKind = "Role" ) // Add registers the webhook for K8s bulit-in object types. @@ -51,6 +53,9 @@ 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 resources", "GVK", createRoleGVK()) + response = v.handleRole(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())) @@ -86,6 +91,19 @@ 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) + } + + if validation.ValidateUserForRole(v.whiteListedUsers, req.UserInfo) { + return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet Role: %s", req.UserInfo.Username, req.UserInfo.Groups, role.Name)) + } + + return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify fleet Role: %s", req.UserInfo.Username, req.UserInfo.Groups, role.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 +140,11 @@ func createMemberClusterGVK() metav1.GroupVersionKind { Kind: memberClusterKind, } } + +func createRoleGVK() metav1.GroupVersionKind { + return metav1.GroupVersionKind{ + Group: rbacv1.SchemeGroupVersion.Group, + Version: rbacv1.SchemeGroupVersion.Version, + Kind: roleKind, + } +} diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 41a88d99d..03bcfcee1 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -15,12 +15,20 @@ const ( mastersGroup = "system:masters" ) -// ValidateUserForCRD checks to see if user is authenticated to make a request to modify fleet CRDs. +// ValidateUserForCRD checks to see if user is allowed 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. +// ValidateUserForRole checks to see if user is allowed to make a request to modify fleet Role. +func ValidateUserForRole(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { + if slices.Contains(userInfo.Groups, mastersGroup) { + return true + } + return slices.Contains(whiteListedUsers, userInfo.Username) +} + +// 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) { return true diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index ee99349dc..3b8212758 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -24,6 +24,7 @@ import ( admv1beta1 "k8s.io/api/admissionregistration/v1beta1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -50,6 +51,7 @@ const ( memberClusterResourceName = "memberclusters" replicaSetResourceName = "replicasets" podResourceName = "pods" + roleResourceName = "roles" ) var ( @@ -234,6 +236,28 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { }, }, }, + { + Name: "fleet.roles.validating", + ClientConfig: w.createClientConfig(rbacv1.Role{}), + FailurePolicy: &failPolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: admissionReviewVersions, + Rules: []admv1.RuleWithOperations{ + { + Operations: []admv1.OperationType{ + admv1.Create, + admv1.Update, + admv1.Delete, + }, + Rule: admv1.Rule{ + APIGroups: []string{rbacv1.SchemeGroupVersion.Group}, + APIVersions: []string{rbacv1.SchemeGroupVersion.Version}, + Resources: []string{roleResourceName}, + Scope: &namespacedScope, + }, + }, + }, + }, }, } @@ -287,6 +311,9 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC case fleetv1alpha1.MemberCluster: serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) + case rbacv1.Role: + serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath + serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) } config := admv1.WebhookClientConfig{ From f2878d010acbaab9e93d0a0d3d5a87ceb8ad7423 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 23 Jun 2023 16:29:58 -0700 Subject: [PATCH 3/5] working Role webhook --- .../fleetresourcehandler_webhook.go | 6 +++--- pkg/webhook/validation/uservalidation.go | 17 ++--------------- pkg/webhook/webhook.go | 10 ++++++++++ 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 30b366197..5310f0a16 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -72,7 +72,7 @@ func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Resp // This regex works because every CRD name in kubernetes follows this pattern .. 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)) @@ -97,10 +97,10 @@ func (v *fleetResourceValidator) handleRole(req admission.Request) admission.Res return admission.Errored(http.StatusBadRequest, err) } - if validation.ValidateUserForRole(v.whiteListedUsers, req.UserInfo) { + if !validation.IsMasterGroupUserOrWhiteListedUser(v.whiteListedUsers, req.UserInfo) { return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet Role: %s", req.UserInfo.Username, req.UserInfo.Groups, role.Name)) } - + klog.V(2).InfoS("user in groups is allowed to modify fleet Role", "user", req.UserInfo.Username, "groups", req.UserInfo.Groups) return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify fleet Role: %s", req.UserInfo.Username, req.UserInfo.Groups, role.Name)) } diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 03bcfcee1..63e59f429 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -15,22 +15,9 @@ const ( mastersGroup = "system:masters" ) -// ValidateUserForCRD checks to see if user is allowed to make a request to modify fleet CRDs. -func ValidateUserForCRD(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { - return isMasterGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) -} - -// ValidateUserForRole checks to see if user is allowed to make a request to modify fleet Role. -func ValidateUserForRole(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { - if slices.Contains(userInfo.Groups, mastersGroup) { - return true - } - return slices.Contains(whiteListedUsers, userInfo.Username) -} - // 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 @@ -46,6 +33,6 @@ func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteList return slices.Contains(identities, userInfo.Username) } -func isMasterGroupUserOrWhiteListedUser(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { +func IsMasterGroupUserOrWhiteListedUser(whiteListedUsers []string, userInfo authenticationv1.UserInfo) bool { return slices.Contains(whiteListedUsers, userInfo.Username) || slices.Contains(userInfo.Groups, mastersGroup) } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 3b8212758..3140a23e5 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -35,6 +35,7 @@ import ( fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" "go.goms.io/fleet/cmd/hubagent/options" + "go.goms.io/fleet/pkg/controllers/membercluster" "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" "go.goms.io/fleet/pkg/webhook/fleetresourcehandler" "go.goms.io/fleet/pkg/webhook/pod" @@ -257,6 +258,15 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { }, }, }, + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: membercluster.FleetResourceLabelKey, + Operator: metav1.LabelSelectorOpIn, + Values: []string{membercluster.FleetNamespaceValue}, + }, + }, + }, }, }, } From a3761a6037723ee59acf561a5866ae2498906c02 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 23 Jun 2023 19:27:26 -0700 Subject: [PATCH 4/5] role binding webhook --- .../membercluster/membercluster_controller.go | 6 +- .../membercluster_controller_test.go | 2 +- .../fleetresourcehandler_webhook.go | 27 ++++- pkg/webhook/validation/uservalidation.go | 11 ++ pkg/webhook/webhook.go | 109 ++++++++---------- 5 files changed, 88 insertions(+), 67 deletions(-) diff --git a/pkg/controllers/membercluster/membercluster_controller.go b/pkg/controllers/membercluster/membercluster_controller.go index e34f10554..c4250540e 100644 --- a/pkg/controllers/membercluster/membercluster_controller.go +++ b/pkg/controllers/membercluster/membercluster_controller.go @@ -50,8 +50,8 @@ const ( reasonMemberClusterLeft = "MemberClusterLeft" reasonMemberClusterUnknown = "MemberClusterUnknown" - fleetResourceLabelKey = fleetv1beta1.FleetPrefix + "isFleetResource" - fleetNamespaceValue = "fleet-namespace" + FleetResourceLabelKey = fleetv1beta1.FleetPrefix + "isFleetResource" + FleetNamespaceValue = "fleet-namespace" ) // Reconciler reconciles a MemberCluster object @@ -237,7 +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}, + Labels: map[string]string{FleetResourceLabelKey: FleetNamespaceValue}, }, } diff --git a/pkg/controllers/membercluster/membercluster_controller_test.go b/pkg/controllers/membercluster/membercluster_controller_test.go index 2d073cd6d..447e4e2e4 100644 --- a/pkg/controllers/membercluster/membercluster_controller_test.go +++ b/pkg/controllers/membercluster/membercluster_controller_test.go @@ -50,7 +50,7 @@ func TestSyncNamespace(t *testing.T) { *o = corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: namespace1, - Labels: map[string]string{fleetResourceLabelKey: fleetNamespaceValue}, + Labels: map[string]string{FleetResourceLabelKey: FleetNamespaceValue}, }, } return nil diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 5310f0a16..66f04fa3f 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -28,6 +28,7 @@ const ( crdKind = "CustomResourceDefinition" memberClusterKind = "MemberCluster" roleKind = "Role" + roleBindingKind = "RoleBinding" ) // Add registers the webhook for K8s bulit-in object types. @@ -54,8 +55,11 @@ func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Reque klog.V(2).InfoS("handling Member cluster resource", "GVK", createMemberClusterGVK()) response = v.handleMemberCluster(ctx, req) case createRoleGVK(): - klog.V(2).InfoS("handling Role resources", "GVK", 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())) @@ -97,11 +101,16 @@ func (v *fleetResourceValidator) handleRole(req admission.Request) admission.Res return admission.Errored(http.StatusBadRequest, err) } - if !validation.IsMasterGroupUserOrWhiteListedUser(v.whiteListedUsers, req.UserInfo) { - return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet Role: %s", req.UserInfo.Username, req.UserInfo.Groups, role.Name)) + 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) } - klog.V(2).InfoS("user in groups is allowed to modify fleet Role", "user", req.UserInfo.Username, "groups", req.UserInfo.Groups) - return admission.Allowed(fmt.Sprintf("user: %s in groups: %v is allowed to modify fleet Role: %s", req.UserInfo.Username, req.UserInfo.Groups, role.Name)) + + return validation.ValidateUserForResource(v.whiteListedUsers, req.UserInfo, rb.Kind, rb.Name) } func (v *fleetResourceValidator) decodeRequestObject(req admission.Request, obj runtime.Object) error { @@ -148,3 +157,11 @@ func createRoleGVK() metav1.GroupVersionKind { Kind: roleKind, } } + +func createRoleBindingGVK() metav1.GroupVersionKind { + return metav1.GroupVersionKind{ + Group: rbacv1.SchemeGroupVersion.Group, + Version: rbacv1.SchemeGroupVersion.Version, + Kind: roleBindingKind, + } +} diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 63e59f429..0a734b746 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/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" ) @@ -33,6 +35,15 @@ func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteList return slices.Contains(identities, userInfo.Username) } +// 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 %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 %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) } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 3140a23e5..faa60baf8 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -53,6 +53,7 @@ const ( replicaSetResourceName = "replicasets" podResourceName = "pods" roleResourceName = "roles" + roleBindingResourceName = "rolebindings" ) var ( @@ -122,6 +123,22 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { namespacedScope := admv1.NamespacedScope clusterScope := admv1.ClusterScope + namespaceSelector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: membercluster.FleetResourceLabelKey, + Operator: metav1.LabelSelectorOpIn, + Values: []string{membercluster.FleetNamespaceValue}, + }, + }, + } + + CUDOperations := []admv1.OperationType{ + admv1.Create, + admv1.Update, + admv1.Delete, + } + whCfg := admv1.ValidatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ Name: FleetWebhookCfgName, @@ -142,12 +159,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { Operations: []admv1.OperationType{ admv1.Create, }, - Rule: admv1.Rule{ - APIGroups: []string{corev1.SchemeGroupVersion.Group}, - APIVersions: []string{corev1.SchemeGroupVersion.Version}, - Resources: []string{podResourceName}, - Scope: &namespacedScope, - }, + Rule: createRule([]string{corev1.SchemeGroupVersion.Group}, []string{corev1.SchemeGroupVersion.Version}, []string{podResourceName}, &namespacedScope), }, }, }, @@ -164,12 +176,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { admv1.Create, admv1.Update, }, - Rule: admv1.Rule{ - APIGroups: []string{fleetv1alpha1.GroupVersion.Group}, - APIVersions: []string{fleetv1alpha1.GroupVersion.Version}, - Resources: []string{fleetv1alpha1.ClusterResourcePlacementResource}, - Scope: &clusterScope, - }, + Rule: createRule([]string{fleetv1alpha1.GroupVersion.Group}, []string{fleetv1alpha1.GroupVersion.Version}, []string{fleetv1alpha1.ClusterResourcePlacementResource}, &clusterScope), }, }, }, @@ -184,12 +191,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { Operations: []admv1.OperationType{ admv1.Create, }, - Rule: admv1.Rule{ - APIGroups: []string{appsv1.SchemeGroupVersion.Group}, - APIVersions: []string{appsv1.SchemeGroupVersion.Version}, - Resources: []string{replicaSetResourceName}, - Scope: &namespacedScope, - }, + Rule: createRule([]string{appsv1.SchemeGroupVersion.Group}, []string{appsv1.SchemeGroupVersion.Version}, []string{replicaSetResourceName}, &namespacedScope), }, }, }, @@ -201,17 +203,8 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { - Operations: []admv1.OperationType{ - admv1.Create, - admv1.Update, - admv1.Delete, - }, - Rule: admv1.Rule{ - APIGroups: []string{v1.SchemeGroupVersion.Group}, - APIVersions: []string{v1.SchemeGroupVersion.Version}, - Resources: []string{crdResourceName}, - Scope: &clusterScope, - }, + Operations: CUDOperations, + Rule: createRule([]string{v1.SchemeGroupVersion.Group}, []string{v1.SchemeGroupVersion.Version}, []string{crdResourceName}, &clusterScope), }, }, }, @@ -223,17 +216,8 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { - Operations: []admv1.OperationType{ - admv1.Create, - admv1.Update, - admv1.Delete, - }, - Rule: admv1.Rule{ - APIGroups: []string{fleetv1alpha1.GroupVersion.Group}, - APIVersions: []string{fleetv1alpha1.GroupVersion.Version}, - Resources: []string{memberClusterResourceName}, - Scope: &clusterScope, - }, + Operations: CUDOperations, + Rule: createRule([]string{fleetv1alpha1.GroupVersion.Group}, []string{fleetv1alpha1.GroupVersion.Version}, []string{memberClusterResourceName}, &clusterScope), }, }, }, @@ -245,28 +229,25 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { AdmissionReviewVersions: admissionReviewVersions, Rules: []admv1.RuleWithOperations{ { - Operations: []admv1.OperationType{ - admv1.Create, - admv1.Update, - admv1.Delete, - }, - Rule: admv1.Rule{ - APIGroups: []string{rbacv1.SchemeGroupVersion.Group}, - APIVersions: []string{rbacv1.SchemeGroupVersion.Version}, - Resources: []string{roleResourceName}, - Scope: &namespacedScope, - }, + Operations: CUDOperations, + Rule: createRule([]string{rbacv1.SchemeGroupVersion.Group}, []string{rbacv1.SchemeGroupVersion.Version}, []string{roleResourceName}, &namespacedScope), }, }, - NamespaceSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: membercluster.FleetResourceLabelKey, - Operator: metav1.LabelSelectorOpIn, - Values: []string{membercluster.FleetNamespaceValue}, - }, + NamespaceSelector: namespaceSelector, + }, + { + Name: "fleet.rolesbindings.validating", + ClientConfig: w.createClientConfig(rbacv1.RoleBinding{}), + FailurePolicy: &failPolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: admissionReviewVersions, + Rules: []admv1.RuleWithOperations{ + { + Operations: CUDOperations, + Rule: createRule([]string{rbacv1.SchemeGroupVersion.Group}, []string{rbacv1.SchemeGroupVersion.Version}, []string{roleBindingResourceName}, &namespacedScope), }, }, + NamespaceSelector: namespaceSelector, }, }, } @@ -324,6 +305,9 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC case rbacv1.Role: serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) + case rbacv1.RoleBinding: + serviceEndpoint = w.serviceURL + fleetresourcehandler.ValidationPath + serviceRef.Path = pointer.String(fleetresourcehandler.ValidationPath) } config := admv1.WebhookClientConfig{ @@ -511,3 +495,12 @@ func bindWebhookConfigToFleetSystem(ctx context.Context, k8Client client.Client, validatingWebhookConfig.OwnerReferences = []metav1.OwnerReference{ownerRef} return nil } + +func createRule(apiGroups, apiResources, resources []string, scopeType *admv1.ScopeType) admv1.Rule { + return admv1.Rule{ + APIGroups: apiGroups, + APIVersions: apiResources, + Resources: resources, + Scope: scopeType, + } +} From 87909b28b5d33d25ea04383514c3989c88d0a6db Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Mon, 26 Jun 2023 12:40:32 -0700 Subject: [PATCH 5/5] fix UT --- pkg/webhook/validation/uservalidation.go | 4 +-- pkg/webhook/validation/uservalidation_test.go | 25 +++++++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 0a734b746..0cb84c30c 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -38,10 +38,10 @@ func ValidateUserForFleetCR(ctx context.Context, client client.Client, whiteList // 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 %s: %s", userInfo.Username, userInfo.Groups, resKind, resName)) + 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 %s: %s", userInfo.Username, 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 { diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go index 0ecaf05ab..effa99b8a 100644 --- a/pkg/webhook/validation/uservalidation_test.go +++ b/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) }) } }