diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index e18d51dd0..f38e8f302 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -171,7 +171,7 @@ func main() { if opts.EnableWebhook { whiteListedUsers := strings.Split(opts.WhiteListedUsers, ",") - if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs); err != nil { + if err := SetupWebhook(mgr, options.WebhookClientConnectionType(opts.WebhookClientConnectionType), opts.WebhookServiceName, whiteListedUsers, opts.EnableGuardRail, opts.EnableV1Beta1APIs, opts.DenyModifyMemberClusterLabels); err != nil { klog.ErrorS(err, "unable to set up webhook") exitWithErrorFunc() } @@ -203,9 +203,9 @@ func main() { } // SetupWebhook generates the webhook cert and then set up the webhook configurator. -func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool) error { +func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.WebhookClientConnectionType, webhookServiceName string, whiteListedUsers []string, enableGuardRail, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error { // Generate self-signed key and crt files in FleetWebhookCertDir for the webhook server to start. - w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail) + w, err := webhook.NewWebhookConfig(mgr, webhookServiceName, FleetWebhookPort, &webhookClientConnectionType, FleetWebhookCertDir, enableGuardRail, denyModifyMemberClusterLabels) if err != nil { klog.ErrorS(err, "fail to generate WebhookConfig") return err @@ -214,7 +214,7 @@ func SetupWebhook(mgr manager.Manager, webhookClientConnectionType options.Webho klog.ErrorS(err, "unable to add WebhookConfig") return err } - if err = webhook.AddToManager(mgr, whiteListedUsers, isFleetV1Beta1API); err != nil { + if err = webhook.AddToManager(mgr, whiteListedUsers, isFleetV1Beta1API, denyModifyMemberClusterLabels); err != nil { klog.ErrorS(err, "unable to register webhooks to the manager") return err } diff --git a/cmd/hubagent/options/options.go b/cmd/hubagent/options/options.go index 0015acbf1..41432131e 100644 --- a/cmd/hubagent/options/options.go +++ b/cmd/hubagent/options/options.go @@ -98,6 +98,8 @@ type Options struct { EnableStagedUpdateRunAPIs bool // EnableEvictionAPIs enables to agents to watch the eviction and placement disruption budget CRs. EnableEvictionAPIs bool + // DenyModifyMemberClusterLabels indicates if the member cluster labels cannot be modified by groups (excluding system:masters) + DenyModifyMemberClusterLabels bool } // NewOptions builds an empty options. @@ -158,6 +160,7 @@ func (o *Options) AddFlags(flags *flag.FlagSet) { flags.DurationVar(&o.ForceDeleteWaitTime.Duration, "force-delete-wait-time", 15*time.Minute, "The duration the hub agent waits before force deleting a member cluster.") flags.BoolVar(&o.EnableStagedUpdateRunAPIs, "enable-staged-update-run-apis", true, "If set, the agents will watch for the ClusterStagedUpdateRun APIs.") flags.BoolVar(&o.EnableEvictionAPIs, "enable-eviction-apis", true, "If set, the agents will watch for the Eviction and PlacementDisruptionBudget APIs.") + flags.BoolVar(&o.DenyModifyMemberClusterLabels, "deny-modify-member-cluster-labels", false, "If set, users not in the system:masters cannot modify member cluster labels.") o.RateLimiterOpts.AddFlags(flags) } diff --git a/cmd/hubagent/options/validation_test.go b/cmd/hubagent/options/validation_test.go index e099c6ab9..b4cf80b9c 100644 --- a/cmd/hubagent/options/validation_test.go +++ b/cmd/hubagent/options/validation_test.go @@ -17,10 +17,12 @@ limitations under the License. package options import ( + "flag" "testing" "time" "github.com/google/go-cmp/cmp" + "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" ) @@ -102,3 +104,13 @@ func TestValidateControllerManagerConfiguration(t *testing.T) { }) } } + +func TestAddFlags(t *testing.T) { + g := gomega.NewWithT(t) + opts := NewOptions() + + flags := flag.NewFlagSet("deny-modify-member-cluster-labels", flag.ExitOnError) + opts.AddFlags(flags) + + g.Expect(opts.DenyModifyMemberClusterLabels).To(gomega.BeFalse(), "deny-modify-member-cluster-labels should be false by default") +} diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go index 52e8236ab..b49705776 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go @@ -35,23 +35,25 @@ const ( ) // Add registers the webhook for K8s built-in object types. -func Add(mgr manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool) error { +func Add(mgr manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error { hookServer := mgr.GetWebhookServer() handler := &fleetResourceValidator{ - client: mgr.GetClient(), - whiteListedUsers: whiteListedUsers, - isFleetV1Beta1API: isFleetV1Beta1API, - decoder: admission.NewDecoder(mgr.GetScheme()), + client: mgr.GetClient(), + whiteListedUsers: whiteListedUsers, + isFleetV1Beta1API: isFleetV1Beta1API, + decoder: admission.NewDecoder(mgr.GetScheme()), + denyModifyMemberClusterLabels: denyModifyMemberClusterLabels, } hookServer.Register(ValidationPath, &webhook.Admission{Handler: handler}) return nil } type fleetResourceValidator struct { - client client.Client - whiteListedUsers []string - isFleetV1Beta1API bool - decoder webhook.AdmissionDecoder + client client.Client + whiteListedUsers []string + isFleetV1Beta1API bool + decoder webhook.AdmissionDecoder + denyModifyMemberClusterLabels bool } // Handle receives the request then allows/denies the request to modify fleet resources. @@ -133,7 +135,7 @@ func (v *fleetResourceValidator) handleMemberCluster(req admission.Request) admi } isFleetMC := utils.IsFleetAnnotationPresent(oldMC.Annotations) if isFleetMC { - return validation.ValidateFleetMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers) + return validation.ValidateFleetMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers, v.denyModifyMemberClusterLabels) } return validation.ValidatedUpstreamMemberClusterUpdate(currentMC, oldMC, req, v.whiteListedUsers) } diff --git a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go index 21feac935..8b48ee258 100644 --- a/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go +++ b/pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go @@ -634,6 +634,189 @@ func TestHandleMemberCluster(t *testing.T) { }, wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "aks-support", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Delete, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})), }, + + "allow label modification by system:masters when flag is set to true": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + Object: runtime.RawExtension{ + Raw: func() []byte { + updatedMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(updatedMC) + return raw + }(), + }, + OldObject: runtime.RawExtension{ + Raw: fleetMCObjectBytes, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "mastersUser", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceValidator: fleetResourceValidator{ + decoder: decoder, + denyModifyMemberClusterLabels: true, + }, + wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "mastersUser", utils.GenerateGroupString([]string{"system:masters"}), admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})), + }, + "deny label modification by non system:masters user when flag is set to true": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + Object: runtime.RawExtension{ + Raw: func() []byte { + updatedMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(updatedMC) + return raw + }(), + }, + OldObject: runtime.RawExtension{ + Raw: func() []byte { + oldMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(oldMC) + return raw + }(), + }, + UserInfo: authenticationv1.UserInfo{ + Username: "nonSystemMastersUser", + Groups: []string{"system:authenticated"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceValidator: fleetResourceValidator{ + decoder: decoder, + denyModifyMemberClusterLabels: true, + }, + wantResponse: admission.Denied(fmt.Sprintf(validation.DeniedModifyMemberClusterLabels)), + }, + "deny label modification by fleet agent when flag is set to true": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + Object: runtime.RawExtension{ + Raw: func() []byte { + updatedMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(updatedMC) + return raw + }(), + }, + OldObject: runtime.RawExtension{ + Raw: func() []byte { + oldMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(oldMC) + return raw + }(), + }, + UserInfo: authenticationv1.UserInfo{ + Username: "system:serviceaccount:fleet-system:hub-agent-sa", + Groups: []string{"system:serviceaccounts"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceValidator: fleetResourceValidator{ + decoder: decoder, + denyModifyMemberClusterLabels: true, + }, + wantResponse: admission.Denied(fmt.Sprintf(validation.DeniedModifyMemberClusterLabels)), + }, + "allow label modification by non system:masters resources when flag is set to false": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + Object: runtime.RawExtension{ + Raw: func() []byte { + updatedMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(updatedMC) + return raw + }(), + }, + OldObject: runtime.RawExtension{ + Raw: func() []byte { + oldMC := &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + } + raw, _ := json.Marshal(oldMC) + return raw + }(), + }, + UserInfo: authenticationv1.UserInfo{ + Username: "nonMastersUser", + Groups: []string{"system:authenticated"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceValidator: fleetResourceValidator{ + decoder: decoder, + denyModifyMemberClusterLabels: false, + }, + wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, + "nonMastersUser", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &utils.MCMetaGVK, "", + types.NamespacedName{Name: "test-mc"})), + }, } for testName, testCase := range testCases { diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 1b40cafef..334d9b716 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -32,10 +32,11 @@ const ( aksSupportUser = "aks-support" serviceAccountFmt = "system:serviceaccount:fleet-system:%s" - allowedModifyResource = "user in groups is allowed to modify resource" - deniedModifyResource = "user in groups is not allowed to modify resource" - deniedAddFleetAnnotation = "no user is allowed to add a fleet pre-fixed annotation to an upstream member cluster" - deniedRemoveFleetAnnotation = "no user is allowed to remove all fleet pre-fixed annotations from a fleet member cluster" + allowedModifyResource = "user in groups is allowed to modify resource" + deniedModifyResource = "user in groups is not allowed to modify resource" + deniedAddFleetAnnotation = "no user is allowed to add a fleet pre-fixed annotation to an upstream member cluster" + deniedRemoveFleetAnnotation = "no user is allowed to remove all fleet pre-fixed annotations from a fleet member cluster" + DeniedModifyMemberClusterLabels = "users are not allowed to modify labels through hub cluster directly" ResourceAllowedFormat = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v" ResourceDeniedFormat = "user: '%s' in '%s' is not allowed to %s resource %+v/%s: %+v" @@ -62,7 +63,7 @@ func ValidateUserForFleetCRD(req admission.Request, whiteListedUsers []string, g func ValidateUserForResource(req admission.Request, whiteListedUsers []string) admission.Response { namespacedName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace} userInfo := req.UserInfo - if isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isNodeGroupUser(userInfo) || isAKSSupportUser(userInfo) { + if isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) || isUserAuthenticatedServiceAccount(userInfo) || isUserKubeScheduler(userInfo) || isUserKubeControllerManager(userInfo) || isUserInGroup(userInfo, nodeGroup) || isAKSSupportUser(userInfo) { klog.V(3).InfoS(allowedModifyResource, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName) return admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, userInfo.Username, utils.GenerateGroupString(userInfo.Groups), req.Operation, req.RequestKind, req.SubResource, namespacedName)) } @@ -93,7 +94,7 @@ func ValidateV1Alpha1MemberClusterUpdate(currentMC, oldMC fleetv1alpha1.MemberCl } // ValidateFleetMemberClusterUpdate checks to see if user had updated the fleet member cluster resource and allows/denies the request. -func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberCluster, req admission.Request, whiteListedUsers []string) admission.Response { +func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberCluster, req admission.Request, whiteListedUsers []string, denyModifyMemberClusterLabels bool) admission.Response { namespacedName := types.NamespacedName{Name: currentMC.GetName()} userInfo := req.UserInfo if areAllFleetAnnotationsRemoved(currentMC.Annotations, oldMC.Annotations) { @@ -107,6 +108,17 @@ func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberClus if err != nil { return admission.Denied(err.Error()) } + + // Users are no longer allowed to modify labels of fleet member cluster through webhook. + // This will be disabled until member labels are accessible through CLI + if denyModifyMemberClusterLabels { + isLabelUpdated := isMapFieldUpdated(currentMC.GetLabels(), oldMC.GetLabels()) + if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) { + klog.V(2).InfoS(DeniedModifyMemberClusterLabels, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName) + return admission.Denied(DeniedModifyMemberClusterLabels) + } + } + isAnnotationUpdated := isFleetAnnotationUpdated(currentMC.Annotations, oldMC.Annotations) if isObjUpdated || isAnnotationUpdated { return ValidateUserForResource(req, whiteListedUsers) @@ -162,9 +174,9 @@ func isAKSSupportUser(userInfo authenticationv1.UserInfo) bool { return userInfo.Username == aksSupportUser } -// isNodeGroupUser returns true if user belongs to system:nodes group. -func isNodeGroupUser(userInfo authenticationv1.UserInfo) bool { - return slices.Contains(userInfo.Groups, nodeGroup) +// isUserInGroup returns true if user belongs to the specified groupName. +func isUserInGroup(userInfo authenticationv1.UserInfo, groupName string) bool { + return slices.Contains(userInfo.Groups, groupName) } // isMemberClusterMapFieldUpdated return true if member cluster label is updated. diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go index 8e7cbdc7e..7bcfa3919 100644 --- a/pkg/webhook/validation/uservalidation_test.go +++ b/pkg/webhook/validation/uservalidation_test.go @@ -8,9 +8,11 @@ import ( "github.com/stretchr/testify/assert" admissionv1 "k8s.io/api/admission/v1" authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" "github.com/kubefleet-dev/kubefleet/pkg/utils" ) @@ -182,3 +184,156 @@ func TestValidateUserForResource(t *testing.T) { }) } } + +func TestValidateFleetMemberClusterUpdate(t *testing.T) { + testCases := map[string]struct { + denyModifyMemberClusterLabels bool + oldMC *clusterv1beta1.MemberCluster + newMC *clusterv1beta1.MemberCluster + req admission.Request + whiteListedUsers []string + wantResponse admission.Response + }{ + "allow label modification by system:masters resources when flag is set to true": { + denyModifyMemberClusterLabels: true, + oldMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + newMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + UserInfo: authenticationv1.UserInfo{ + Username: "mastersUser", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "mastersUser", utils.GenerateGroupString([]string{"system:masters"}), + admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})), + }, + "deny label modification by non-system:masters resource when flag is set to true": { + denyModifyMemberClusterLabels: true, + oldMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + newMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + UserInfo: authenticationv1.UserInfo{ + Username: "nonSystemMastersUser", + Groups: []string{"system:authenticated"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + wantResponse: admission.Denied(DeniedModifyMemberClusterLabels), + }, + "deny label modification by fleet agent when flag is set to true": { + denyModifyMemberClusterLabels: true, + oldMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + newMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key2": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + UserInfo: authenticationv1.UserInfo{ + Username: "system:serviceaccount:fleet-system:hub-agent-sa", + Groups: []string{"system:serviceaccounts"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + wantResponse: admission.Denied(DeniedModifyMemberClusterLabels), + }, + "allow label modification by non system:masters resource when flag is set to false": { + denyModifyMemberClusterLabels: false, + oldMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value1"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + newMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"key1": "value2"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + UserInfo: authenticationv1.UserInfo{ + Username: "nonSystemMastersUser", + Groups: []string{"system:authenticated"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "nonSystemMastersUser", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})), + }, + } + + for testName, testCase := range testCases { + t.Run(testName, func(t *testing.T) { + gotResult := ValidateFleetMemberClusterUpdate(*testCase.newMC, *testCase.oldMC, testCase.req, testCase.whiteListedUsers, testCase.denyModifyMemberClusterLabels) + assert.Equal(t, testCase.wantResponse, gotResult, utils.TestCaseMsg, testName) + }) + } +} diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 5d81e24e7..fac970d01 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -128,16 +128,16 @@ var ( ) var AddToManagerFuncs []func(manager.Manager) error -var AddToManagerFleetResourceValidator func(manager.Manager, []string, bool) error +var AddToManagerFleetResourceValidator func(manager.Manager, []string, bool, bool) error // AddToManager adds all Controllers to the Manager -func AddToManager(m manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool) error { +func AddToManager(m manager.Manager, whiteListedUsers []string, isFleetV1Beta1API bool, denyModifyMemberClusterLabels bool) error { for _, f := range AddToManagerFuncs { if err := f(m); err != nil { return err } } - return AddToManagerFleetResourceValidator(m, whiteListedUsers, isFleetV1Beta1API) + return AddToManagerFleetResourceValidator(m, whiteListedUsers, isFleetV1Beta1API, denyModifyMemberClusterLabels) } type Config struct { @@ -155,22 +155,25 @@ type Config struct { clientConnectionType *options.WebhookClientConnectionType enableGuardRail bool + + denyModifyMemberClusterLabels bool } -func NewWebhookConfig(mgr manager.Manager, webhookServiceName string, port int32, clientConnectionType *options.WebhookClientConnectionType, certDir string, enableGuardRail bool) (*Config, error) { +func NewWebhookConfig(mgr manager.Manager, webhookServiceName string, port int32, clientConnectionType *options.WebhookClientConnectionType, certDir string, enableGuardRail bool, denyModifyMemberClusterLabels bool) (*Config, error) { // We assume the Pod namespace should be passed to env through downward API in the Pod spec. namespace := os.Getenv("POD_NAMESPACE") if namespace == "" { return nil, errors.New("fail to obtain Pod namespace from POD_NAMESPACE") } w := Config{ - mgr: mgr, - servicePort: port, - serviceNamespace: namespace, - serviceName: webhookServiceName, - serviceURL: fmt.Sprintf("https://%s.%s.svc.cluster.local:%d", webhookServiceName, namespace, port), - clientConnectionType: clientConnectionType, - enableGuardRail: enableGuardRail, + mgr: mgr, + servicePort: port, + serviceNamespace: namespace, + serviceName: webhookServiceName, + serviceURL: fmt.Sprintf("https://%s.%s.svc.cluster.local:%d", webhookServiceName, namespace, port), + clientConnectionType: clientConnectionType, + enableGuardRail: enableGuardRail, + denyModifyMemberClusterLabels: denyModifyMemberClusterLabels, } caPEM, err := w.genCertificate(certDir) if err != nil { diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index f299e7754..ff8567606 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -3,7 +3,10 @@ package webhook import ( "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" + "sigs.k8s.io/controller-runtime/pkg/manager" "github.com/kubefleet-dev/kubefleet/cmd/hubagent/options" "github.com/kubefleet-dev/kubefleet/pkg/utils" @@ -58,3 +61,63 @@ func TestBuildFleetGuardRailValidatingWebhooks(t *testing.T) { }) } } + +func TestNewWebhookConfig(t *testing.T) { + tests := []struct { + name string + mgr manager.Manager + webhookServiceName string + port int32 + clientConnectionType *options.WebhookClientConnectionType + certDir string + enableGuardRail bool + denyModifyMemberClusterLabels bool + want *Config + wantErr bool + }{ + { + name: "valid input", + mgr: nil, + webhookServiceName: "test-webhook", + port: 8080, + clientConnectionType: nil, + certDir: "/tmp/cert", + enableGuardRail: true, + denyModifyMemberClusterLabels: true, + want: &Config{ + serviceNamespace: "test-namespace", + serviceName: "test-webhook", + servicePort: 8080, + clientConnectionType: nil, + enableGuardRail: true, + denyModifyMemberClusterLabels: true, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("POD_NAMESPACE", "test-namespace") + defer t.Setenv("POD_NAMESPACE", "") + got, err := NewWebhookConfig(tt.mgr, tt.webhookServiceName, tt.port, tt.clientConnectionType, tt.certDir, tt.enableGuardRail, tt.denyModifyMemberClusterLabels) + if (err != nil) != tt.wantErr { + t.Errorf("NewWebhookConfig() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got == nil || tt.want == nil { + if got != tt.want { + t.Errorf("NewWebhookConfig() = %v, want %v", got, tt.want) + } + return + } + + opts := []cmp.Option{ + cmpopts.IgnoreFields(Config{}, "caPEM"), + } + opts = append(opts, cmpopts.IgnoreUnexported(Config{})) + if diff := cmp.Diff(tt.want, got, opts...); diff != "" { + t.Errorf("NewWebhookConfig() mismatch (-want +got):\n%s", diff) + } + }) + } +}