From 7ddf8869197dcddb72a94a749a276fc7802bc8bd Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Mon, 31 Oct 2022 14:34:06 -0700 Subject: [PATCH 1/9] Added implementation for ReplicaSet validation webhook. --- pkg/webhook/add_replicaset.go | 14 +++++ .../replicaset_validating_webhook.go | 55 +++++++++++++++++++ pkg/webhook/webhook.go | 35 ++++++++++-- 3 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 pkg/webhook/add_replicaset.go create mode 100644 pkg/webhook/replicaset/replicaset_validating_webhook.go diff --git a/pkg/webhook/add_replicaset.go b/pkg/webhook/add_replicaset.go new file mode 100644 index 000000000..9d1e37e90 --- /dev/null +++ b/pkg/webhook/add_replicaset.go @@ -0,0 +1,14 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package webhook + +import ( + "go.goms.io/fleet/pkg/webhook/replicaset" +) + +func init() { + AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add) +} diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go new file mode 100644 index 000000000..3cf9f2563 --- /dev/null +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -0,0 +1,55 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package replicaset + +import ( + "context" + "fmt" + "net/http" + + admissionv1 "k8s.io/api/admission/v1" + v1 "k8s.io/api/apps/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +const ( + // ValidationPath is the webhook service path which admission requests are routed to for validating ReplicaSet resources. + ValidationPath = "/validate-apps-v1-replicaset" +) + +// Add registers the webhook for K8s bulit-in object types. +func Add(mgr manager.Manager) error { + hookServer := mgr.GetWebhookServer() + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &replicaSetValidator{Client: mgr.GetClient()}}) + return nil +} + +type replicaSetValidator struct { + Client client.Client + decoder *admission.Decoder +} + +// Handle replicaSetValidator denies all creation requests. +func (v *replicaSetValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + if req.Operation == admissionv1.Create { + rs := &v1.ReplicaSet{} + err := v.decoder.Decode(req, rs) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + return admission.Denied(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster", rs.Namespace, rs.Name)) + } + return admission.Allowed("") +} + +// InjectDecoder injects the decoder. +func (v *replicaSetValidator) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 9868f86f1..2c8570bc2 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -21,6 +21,7 @@ import ( "time" admv1 "k8s.io/api/admissionregistration/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,6 +33,7 @@ import ( "go.goms.io/fleet/cmd/hubagent/options" "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" "go.goms.io/fleet/pkg/webhook/pod" + "go.goms.io/fleet/pkg/webhook/replicaset" ) const ( @@ -82,7 +84,7 @@ func NewWebhookConfig(mgr manager.Manager, port int, clientConnectionType *optio } caPEM, err := w.genCertificate(certDir) if err != nil { - return nil, err // TODO + return nil, err } w.caPEM = caPEM return &w, err @@ -97,9 +99,9 @@ func (w *Config) Start(ctx context.Context) error { return nil } -// createFleetWebhookConfiguration creates the ValidatingWebhookConfiguration object for the webhook +// createFleetWebhookConfiguration creates the ValidatingWebhookConfiguration object for the webhook. func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { - failPolicy := admv1.Fail // reject request if the webhook doesn't work + failPolicy := admv1.Fail sideEffortsNone := admv1.SideEffectClassNone namespacedScope := admv1.NamespacedScope clusterScope := admv1.ClusterScope @@ -155,6 +157,27 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { }, }, }, + { + Name: "fleet.replicaset.validating", + ClientConfig: w.createClientConfig(appsv1.ReplicaSet{}), + FailurePolicy: &failPolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: []string{"v1", "v1beta1"}, + + Rules: []admv1.RuleWithOperations{ + { + Operations: []admv1.OperationType{ + admv1.Create, + }, + Rule: admv1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"replicaset"}, + Scope: &namespacedScope, + }, + }, + }, + }, }, } @@ -178,6 +201,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { return nil } +// createClientConfig generates the client configuration with either service ref or URL for the argued interface. func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookClientConfig { serviceRef := admv1.ServiceReference{ Namespace: w.serviceNamespace, @@ -192,6 +216,9 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC case fleetv1alpha1.ClusterResourcePlacement: serviceEndpoint = w.serviceURL + clusterresourceplacement.ValidationPath serviceRef.Path = pointer.String(clusterresourceplacement.ValidationPath) + case appsv1.ReplicaSet: + serviceEndpoint = w.serviceURL + replicaset.ValidationPath + serviceRef.Path = pointer.String(replicaset.ValidationPath) } config := admv1.WebhookClientConfig{ @@ -206,7 +233,7 @@ func (w *Config) createClientConfig(webhookInterface interface{}) admv1.WebhookC return config } -// genCertificate generates the serving cerficiate for the webhook server +// genCertificate generates the serving cerficiate for the webhook server. func (w *Config) genCertificate(certDir string) ([]byte, error) { caPEM, certPEM, keyPEM, err := w.genSelfSignedCert() if err != nil { From 772531e9e4620c8cd8f708fdebac873556237d13 Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Tue, 1 Nov 2022 10:44:44 -0700 Subject: [PATCH 2/9] Added e2e test for replicaset creation. --- pkg/webhook/webhook.go | 3 +-- test/e2e/webhook_test.go | 57 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 2c8570bc2..e0d7198a8 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -163,7 +163,6 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { FailurePolicy: &failPolicy, SideEffects: &sideEffortsNone, AdmissionReviewVersions: []string{"v1", "v1beta1"}, - Rules: []admv1.RuleWithOperations{ { Operations: []admv1.OperationType{ @@ -172,7 +171,7 @@ func (w *Config) createFleetWebhookConfiguration(ctx context.Context) error { Rule: admv1.Rule{ APIGroups: []string{"apps"}, APIVersions: []string{"v1"}, - Resources: []string{"replicaset"}, + Resources: []string{"replicasets"}, Scope: &namespacedScope, }, }, diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 191b4696f..89fac2cdc 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -14,9 +14,11 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" admv1 "k8s.io/api/admissionregistration/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1" @@ -496,4 +498,59 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(regexp.QuoteMeta(fmt.Sprintf("the resource is not found in schema (please retry) or it is not a cluster scoped resource: %s", invalidGVK)))) }) }) + Context("ReplicaSet validation webhook", func() { + It("should deny create operations on ReplicaSets", func() { + rs := &appsv1.ReplicaSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicaSet", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: utils.RandStr(), + Namespace: "default", + }, + Spec: appsv1.ReplicaSetSpec{ + Replicas: pointer.Int32(1), + MinReadySeconds: 1, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"web"}, + }, + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "web"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "nginx", + Image: "nginx", + Ports: []corev1.ContainerPort{ + { + Name: "http", + Protocol: corev1.ProtocolTCP, + ContainerPort: 80, + }, + }, + }, + }, + }, + }, + }, + } + + By("attempting to create a ReplicaSet") + err := HubCluster.KubeClient.Create(ctx, rs) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create ReplicaSet call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) + Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(`admission webhook "fleet.replicaset.validating" denied the request`)) + Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster", rs.Namespace, rs.Name))) + }) + }) }) From 92dacaee6b3d74f417ed5c911e2fe5f29c23aef4 Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Tue, 1 Nov 2022 10:48:50 -0700 Subject: [PATCH 3/9] Style --- .../replicaset/replicaset_validating_webhook.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go index 3cf9f2563..453f8099d 100644 --- a/pkg/webhook/replicaset/replicaset_validating_webhook.go +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -23,6 +23,11 @@ const ( ValidationPath = "/validate-apps-v1-replicaset" ) +type replicaSetValidator struct { + Client client.Client + decoder *admission.Decoder +} + // Add registers the webhook for K8s bulit-in object types. func Add(mgr manager.Manager) error { hookServer := mgr.GetWebhookServer() @@ -30,11 +35,6 @@ func Add(mgr manager.Manager) error { return nil } -type replicaSetValidator struct { - Client client.Client - decoder *admission.Decoder -} - // Handle replicaSetValidator denies all creation requests. func (v *replicaSetValidator) Handle(ctx context.Context, req admission.Request) admission.Response { if req.Operation == admissionv1.Create { From a33ef1595b058a3698cc046c2a52e4c3d30d27c4 Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Tue, 1 Nov 2022 11:58:57 -0700 Subject: [PATCH 4/9] Simplified if statement. --- pkg/webhook/replicaset/replicaset_validating_webhook.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go index 453f8099d..940dd89c1 100644 --- a/pkg/webhook/replicaset/replicaset_validating_webhook.go +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -39,8 +39,7 @@ func Add(mgr manager.Manager) error { func (v *replicaSetValidator) Handle(ctx context.Context, req admission.Request) admission.Response { if req.Operation == admissionv1.Create { rs := &v1.ReplicaSet{} - err := v.decoder.Decode(req, rs) - if err != nil { + if err := v.decoder.Decode(req, rs); err != nil { return admission.Errored(http.StatusBadRequest, err) } return admission.Denied(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster", rs.Namespace, rs.Name)) From 238a556cd06518006b3dc74ac9268a928b9f536b Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Fri, 4 Nov 2022 10:45:06 -0700 Subject: [PATCH 5/9] Made system reserved namespaces a common constant. Added test. --- pkg/utils/common.go | 6 ++ pkg/webhook/pod/pod_validating_webhook.go | 4 +- .../replicaset_validating_webhook.go | 6 +- test/e2e/e2e_test.go | 2 +- test/e2e/webhook_test.go | 68 ++++++++++++++++--- 5 files changed, 72 insertions(+), 14 deletions(-) diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 73a07f62c..3179e20a6 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -63,6 +63,12 @@ const ( LastWorkUpdateTimeAnnotationKey = "work.fleet.azure.com/last-update-time" ) +// System reserved namespaces. +const ( + FleetSysNamespace = "fleet-system" + K8sSysNamespace = "kube-system" +) + var ( FleetRule = rbacv1.PolicyRule{ Verbs: []string{"*"}, diff --git a/pkg/webhook/pod/pod_validating_webhook.go b/pkg/webhook/pod/pod_validating_webhook.go index a16d491f1..a6cbcd1ac 100644 --- a/pkg/webhook/pod/pod_validating_webhook.go +++ b/pkg/webhook/pod/pod_validating_webhook.go @@ -16,6 +16,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "go.goms.io/fleet/pkg/utils" ) const ( @@ -43,7 +45,7 @@ func (v *podValidator) Handle(ctx context.Context, req admission.Request) admiss if err != nil { return admission.Errored(http.StatusBadRequest, err) } - if pod.Namespace != "kube-system" && pod.Namespace != "fleet-system" { + if pod.Namespace != utils.K8sSysNamespace && pod.Namespace != utils.FleetSysNamespace { return admission.Denied(fmt.Sprintf("Pod %s/%s creation is disallowed in the fleet hub cluster", pod.Namespace, pod.Name)) } } diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go index 940dd89c1..2c5305189 100644 --- a/pkg/webhook/replicaset/replicaset_validating_webhook.go +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -16,6 +16,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "go.goms.io/fleet/pkg/utils" ) const ( @@ -42,7 +44,9 @@ func (v *replicaSetValidator) Handle(ctx context.Context, req admission.Request) if err := v.decoder.Decode(req, rs); err != nil { return admission.Errored(http.StatusBadRequest, err) } - return admission.Denied(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster", rs.Namespace, rs.Name)) + if rs.Namespace != utils.FleetSysNamespace && rs.Namespace != utils.K8sSysNamespace { + return admission.Denied(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster. Namespace must either be %s or %s", rs.Namespace, rs.Name, utils.FleetSysNamespace, utils.K8sSysNamespace)) + } } return admission.Allowed("") } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index a723f01a1..6575591cc 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -174,7 +174,7 @@ var _ = BeforeSuite(func() { identity := rbacv1.Subject{ Name: "member-agent-sa", Kind: "ServiceAccount", - Namespace: "fleet-system", + Namespace: utils.FleetSysNamespace, } mc = &v1alpha1.MemberCluster{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 89fac2cdc..42984dcef 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -26,22 +26,17 @@ import ( testUtils "go.goms.io/fleet/test/e2e/utils" ) -const ( - kubeSystemNs = "kube-system" - fleetSystemNs = "fleet-system" -) - var ( - whitelistedNamespaces = []corev1.Namespace{ - {ObjectMeta: metav1.ObjectMeta{Name: kubeSystemNs}}, - {ObjectMeta: metav1.ObjectMeta{Name: fleetSystemNs}}, + reservedSystemNamespaces = []corev1.Namespace{ + {ObjectMeta: metav1.ObjectMeta{Name: utils.K8sSysNamespace}}, + {ObjectMeta: metav1.ObjectMeta{Name: utils.FleetSysNamespace}}, } ) var _ = Describe("Fleet's Hub cluster webhook tests", func() { Context("Pod validation webhook", func() { It("should admit operations on Pods within whitelisted namespaces", func() { - for _, ns := range whitelistedNamespaces { + for _, ns := range reservedSystemNamespaces { objKey := client.ObjectKey{Name: utils.RandStr(), Namespace: ns.ObjectMeta.Name} nginxPod := &corev1.Pod{ TypeMeta: metav1.TypeMeta{ @@ -499,7 +494,58 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }) }) Context("ReplicaSet validation webhook", func() { - It("should deny create operations on ReplicaSets", func() { + It("should admit operation CREATE on ReplicaSets in system reserved namespaces", func() { + for _, ns := range reservedSystemNamespaces { + rs := &appsv1.ReplicaSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "ReplicaSet", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: utils.RandStr(), + Namespace: ns.Name, + }, + Spec: appsv1.ReplicaSetSpec{ + Replicas: pointer.Int32(1), + MinReadySeconds: 1, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"web"}, + }, + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "web"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "nginx", + Image: "nginx", + Ports: []corev1.ContainerPort{ + { + Name: "http", + Protocol: corev1.ProtocolTCP, + ContainerPort: 80, + }, + }, + }, + }, + }, + }, + }, + } + + By(fmt.Sprintf("expecting admission of operation CREATE in system reserved namespace %s", ns.ObjectMeta.Name)) + Expect(HubCluster.KubeClient.Create(ctx, rs)).Should(Succeed()) + } + }) + It("should deny CREATE operation on ReplicaSets in a non-system reserved namespace", func() { rs := &appsv1.ReplicaSet{ TypeMeta: metav1.TypeMeta{ Kind: "ReplicaSet", @@ -545,7 +591,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By("attempting to create a ReplicaSet") + By("expecting denial of operation CREATE") err := HubCluster.KubeClient.Create(ctx, rs) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create ReplicaSet call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) From 699084b74843471e73012c879e025e0913276187 Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Wed, 9 Nov 2022 14:03:08 -0800 Subject: [PATCH 6/9] Created reserved namespace check, added / replaced relevant usages. --- pkg/utils/common.go | 28 +++++++++---------- pkg/webhook/pod/pod_validating_webhook.go | 2 +- .../replicaset_validating_webhook.go | 4 +-- test/e2e/e2e_test.go | 2 +- test/e2e/webhook_test.go | 26 ++++++++--------- 5 files changed, 30 insertions(+), 32 deletions(-) diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 3179e20a6..eaea50fb6 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -29,12 +29,12 @@ import ( ) const ( - kubePrefix = "kube-" - fleetPrefix = "fleet-" - FleetSystemNamespace = fleetPrefix + "system" - NamespaceNameFormat = fleetPrefix + "member-%s" - RoleNameFormat = fleetPrefix + "role-%s" - RoleBindingNameFormat = fleetPrefix + "rolebinding-%s" + KubePrefix = "kube-" + FleetPrefix = "fleet-" + FleetSystemNamespace = FleetPrefix + "system" + NamespaceNameFormat = FleetPrefix + "member-%s" + RoleNameFormat = FleetPrefix + "role-%s" + RoleBindingNameFormat = FleetPrefix + "rolebinding-%s" ) const ( @@ -63,12 +63,6 @@ const ( LastWorkUpdateTimeAnnotationKey = "work.fleet.azure.com/last-update-time" ) -// System reserved namespaces. -const ( - FleetSysNamespace = "fleet-system" - K8sSysNamespace = "kube-system" -) - var ( FleetRule = rbacv1.PolicyRule{ Verbs: []string{"*"}, @@ -248,10 +242,14 @@ func ShouldPropagateObj(informerManager informer.Manager, uObj *unstructured.Uns return true, nil } -// ShouldPropagateNamespace decides if we should propagate the resources in the namespace +// IsReservedNamespace indicates if an argued namespace is reserved. +func IsReservedNamespace(namespace string) bool { + return strings.HasPrefix(namespace, FleetPrefix) || strings.HasPrefix(namespace, KubePrefix) +} + +// ShouldPropagateNamespace decides if we should propagate the resources in the namespace. func ShouldPropagateNamespace(namespace string, skippedNamespaces map[string]bool) bool { - // special case for namespace have the reserved prefix - if strings.HasPrefix(namespace, fleetPrefix) || strings.HasPrefix(namespace, kubePrefix) { + if IsReservedNamespace(namespace) { return false } diff --git a/pkg/webhook/pod/pod_validating_webhook.go b/pkg/webhook/pod/pod_validating_webhook.go index a6cbcd1ac..b878bc43f 100644 --- a/pkg/webhook/pod/pod_validating_webhook.go +++ b/pkg/webhook/pod/pod_validating_webhook.go @@ -45,7 +45,7 @@ func (v *podValidator) Handle(ctx context.Context, req admission.Request) admiss if err != nil { return admission.Errored(http.StatusBadRequest, err) } - if pod.Namespace != utils.K8sSysNamespace && pod.Namespace != utils.FleetSysNamespace { + if !utils.IsReservedNamespace(pod.Namespace) { return admission.Denied(fmt.Sprintf("Pod %s/%s creation is disallowed in the fleet hub cluster", pod.Namespace, pod.Name)) } } diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go index 2c5305189..f94f4ab90 100644 --- a/pkg/webhook/replicaset/replicaset_validating_webhook.go +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -44,8 +44,8 @@ func (v *replicaSetValidator) Handle(ctx context.Context, req admission.Request) if err := v.decoder.Decode(req, rs); err != nil { return admission.Errored(http.StatusBadRequest, err) } - if rs.Namespace != utils.FleetSysNamespace && rs.Namespace != utils.K8sSysNamespace { - return admission.Denied(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster. Namespace must either be %s or %s", rs.Namespace, rs.Name, utils.FleetSysNamespace, utils.K8sSysNamespace)) + if !utils.IsReservedNamespace(rs.Namespace) { + return admission.Denied(fmt.Sprintf("ReplicaSet %s/%s creation is disallowed in the fleet hub cluster.", rs.Namespace, rs.Name)) } } return admission.Allowed("") diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 6575591cc..e924bfde6 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -174,7 +174,7 @@ var _ = BeforeSuite(func() { identity := rbacv1.Subject{ Name: "member-agent-sa", Kind: "ServiceAccount", - Namespace: utils.FleetSysNamespace, + Namespace: utils.FleetSystemNamespace, } mc = &v1alpha1.MemberCluster{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 42984dcef..40c997930 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -27,17 +27,17 @@ import ( ) var ( - reservedSystemNamespaces = []corev1.Namespace{ - {ObjectMeta: metav1.ObjectMeta{Name: utils.K8sSysNamespace}}, - {ObjectMeta: metav1.ObjectMeta{Name: utils.FleetSysNamespace}}, + reservedNamespaces = []corev1.Namespace{ + {ObjectMeta: metav1.ObjectMeta{Name: "kube-system"}}, + {ObjectMeta: metav1.ObjectMeta{Name: utils.FleetSystemNamespace}}, } ) var _ = Describe("Fleet's Hub cluster webhook tests", func() { Context("Pod validation webhook", func() { - It("should admit operations on Pods within whitelisted namespaces", func() { - for _, ns := range reservedSystemNamespaces { - objKey := client.ObjectKey{Name: utils.RandStr(), Namespace: ns.ObjectMeta.Name} + It("should admit operations on Pods within reserved namespaces", func() { + for _, reservedNamespace := range reservedNamespaces { + objKey := client.ObjectKey{Name: utils.RandStr(), Namespace: reservedNamespace.Name} nginxPod := &corev1.Pod{ TypeMeta: metav1.TypeMeta{ Kind: "Pod", @@ -64,10 +64,10 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By(fmt.Sprintf("expecting admission of operation CREATE of Pod in whitelisted namespace %s", ns.ObjectMeta.Name)) + By(fmt.Sprintf("expecting admission of operation CREATE of Pod in reserved namespace %s", reservedNamespace.Name)) Expect(HubCluster.KubeClient.Create(ctx, nginxPod)).Should(Succeed()) - By(fmt.Sprintf("expecting admission of operation UPDATE of Pod in whitelisted namespace %s", ns.ObjectMeta.Name)) + By(fmt.Sprintf("expecting admission of operation UPDATE of Pod in reserved namespace %s", reservedNamespace.Name)) var podV2 *corev1.Pod Eventually(func() error { var currentPod corev1.Pod @@ -77,11 +77,11 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { return HubCluster.KubeClient.Update(ctx, podV2) }, testUtils.PollTimeout, testUtils.PollInterval).Should(Succeed()) - By(fmt.Sprintf("expecting admission of operation DELETE of Pod in whitelisted namespace %s", ns.ObjectMeta.Name)) + By(fmt.Sprintf("expecting admission of operation DELETE of Pod in reserved namespace %s", reservedNamespace.Name)) Expect(HubCluster.KubeClient.Delete(ctx, nginxPod)).Should(Succeed()) } }) - It("should deny create operation on Pods within any non-whitelisted namespaces", func() { + It("should deny create operation on Pods within any non-reserved namespace", func() { rndNs := corev1.Namespace{ TypeMeta: metav1.TypeMeta{ Kind: "namespace", @@ -119,7 +119,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By(fmt.Sprintf("expecting denial of operation %s of Pod in non-whitelisted namespace %s", admv1.Create, rndNs.Name)) + By(fmt.Sprintf("expecting denial of operation %s of Pod in non-reserved namespace %s", admv1.Create, rndNs.Name)) err := HubCluster.KubeClient.Create(ctx, nginxPod) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create Pod call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) @@ -495,7 +495,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }) Context("ReplicaSet validation webhook", func() { It("should admit operation CREATE on ReplicaSets in system reserved namespaces", func() { - for _, ns := range reservedSystemNamespaces { + for _, ns := range reservedNamespaces { rs := &appsv1.ReplicaSet{ TypeMeta: metav1.TypeMeta{ Kind: "ReplicaSet", @@ -541,7 +541,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By(fmt.Sprintf("expecting admission of operation CREATE in system reserved namespace %s", ns.ObjectMeta.Name)) + By(fmt.Sprintf("expecting admission of operation CREATE in system reserved namespace %s", ns.Name)) Expect(HubCluster.KubeClient.Create(ctx, rs)).Should(Succeed()) } }) From 1810448a7871e65cad293baa371bf2fdf0fa0339 Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Thu, 10 Nov 2022 10:02:11 -0800 Subject: [PATCH 7/9] Attention to detail - messages formats should match. Fully qualify names. --- test/e2e/webhook_test.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 4f29d9a01..4ed093f0d 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -147,7 +147,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, }, } - By("expecting admission of operation CREATE of a valid CRP") + By("expecting admission of operation CREATE of a valid ClusterResourcePlacement") Expect(HubCluster.KubeClient.Create(ctx, &validCRP)).Should(Succeed()) // Get the created CRP @@ -163,12 +163,12 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, testUtils.PollTimeout, testUtils.PollInterval).Should(Succeed()) }) AfterEach(func() { - By("expecting admission of operation DELETE") + By("expecting admission of operation DELETE of ClusterResourcePlacement") Expect(HubCluster.KubeClient.Delete(ctx, &createdCRP)).Should(Succeed()) }) It("should admit write operations for valid ClusterResourcePlacement resources", func() { // create & delete write operations are handled within the BeforeEach & AfterEach functions. - By("expecting admission of operation UPDATE with a valid CRP") + By("expecting admission of operation UPDATE with a valid ClusterResourcePlacement") createdCRP.Spec.ResourceSelectors[0].Name = utils.RandStr() Expect(HubCluster.KubeClient.Update(ctx, &createdCRP)).Should(Succeed()) }) @@ -196,7 +196,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By("expecting denial of operation CREATE with the invalid CRP") + By("expecting denial of operation CREATE with the invalid ClusterResourcePlacement") err := HubCluster.KubeClient.Create(ctx, &invalidCRP) Expect(err).Should(HaveOccurred()) var statusErr *k8sErrors.StatusError @@ -204,7 +204,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(`admission webhook "fleet.clusterresourceplacement.validating" denied the request`)) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("the labelSelector and name fields are mutually exclusive")) - By("expecting denial of operation UPDATE with the invalid CRP") + By("expecting denial of operation UPDATE with the invalid ClusterResourcePlacement") createdCRP.Spec = invalidCRP.Spec err = HubCluster.KubeClient.Update(ctx, &createdCRP) Expect(err).Should(HaveOccurred()) @@ -247,14 +247,14 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By("expecting denial of operation CREATE") + By("expecting denial of operation CREATE of ClusterResourcePlacement") err := HubCluster.KubeClient.Create(ctx, &invalidCRP) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create ClusterResourcePlacement call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(`admission webhook "fleet.clusterresourceplacement.validating" denied the request`)) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(regexp.QuoteMeta(fmt.Sprintf("the labelSelector in cluster selector %+v is invalid:", invalidCRP.Spec.Policy.Affinity.ClusterAffinity.ClusterSelectorTerms[0])))) - By("expecting denial of operation UPDATE") + By("expecting denial of operation UPDATE of ClusterResourcePlacement") createdCRP.Spec = invalidCRP.Spec err = HubCluster.KubeClient.Update(ctx, &createdCRP) Expect(err).Should(HaveOccurred()) @@ -293,14 +293,14 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By("expecting denial of operation CREATE with the invalid CRP") + By("expecting denial of operation CREATE with the invalid ClusterResourcePlacement") err := HubCluster.KubeClient.Create(ctx, &invalidCRP) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create ClusterResourcePlacement call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(`admission webhook "fleet.clusterresourceplacement.validating" denied the request`)) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(regexp.QuoteMeta(fmt.Sprintf("the labelSelector in resource selector %+v is invalid:", invalidCRP.Spec.ResourceSelectors[0])))) - By("expecting denial of operation UPDATE with the invalid CRP") + By("expecting denial of operation UPDATE with the invalid ClusterResourcePlacement") createdCRP.Spec = invalidCRP.Spec err = HubCluster.KubeClient.Update(ctx, &createdCRP) Expect(err).Should(HaveOccurred()) @@ -336,14 +336,14 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { Kind: invalidCRP.Spec.ResourceSelectors[0].Kind, } - By("expecting denial of operation CREATE with the invalid CRP") + By("expecting denial of operation CREATE with the invalid ClusterResourcePlacement") err := HubCluster.KubeClient.Create(ctx, &invalidCRP) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create ClusterResourcePlacement call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(`admission webhook "fleet.clusterresourceplacement.validating" denied the request`)) Expect(statusErr.ErrStatus.Message).Should(MatchRegexp(regexp.QuoteMeta(fmt.Sprintf("the resource is not found in schema (please retry) or it is not a cluster scoped resource: %s", invalidGVK)))) - By("expecting denial of operation UPDATE with the invalid CRP") + By("expecting denial of operation UPDATE with the invalid ClusterResourcePlacement") createdCRP.Spec = invalidCRP.Spec err = HubCluster.KubeClient.Update(ctx, &createdCRP) Expect(err).Should(HaveOccurred()) @@ -353,7 +353,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }) }) Context("ReplicaSet validation webhook", func() { - It("should admit operation CREATE on ReplicaSets in system reserved namespaces", func() { + It("should admit operation CREATE on ReplicaSets in reserved namespaces", func() { for _, ns := range reservedNamespaces { rs := &appsv1.ReplicaSet{ TypeMeta: metav1.TypeMeta{ @@ -400,11 +400,11 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By(fmt.Sprintf("expecting admission of operation CREATE in system reserved namespace %s", ns.Name)) + By(fmt.Sprintf("expecting admission of operation CREATE of ReplicaSet in reserved namespace %s", ns.Name)) Expect(HubCluster.KubeClient.Create(ctx, rs)).Should(Succeed()) } }) - It("should deny CREATE operation on ReplicaSets in a non-system reserved namespace", func() { + It("should deny CREATE operation on ReplicaSets in a non-reserved namespace", func() { rs := &appsv1.ReplicaSet{ TypeMeta: metav1.TypeMeta{ Kind: "ReplicaSet", @@ -450,7 +450,7 @@ var _ = Describe("Fleet's Hub cluster webhook tests", func() { }, } - By("expecting denial of operation CREATE") + By("expecting denial of operation CREATE of ReplicaSet") err := HubCluster.KubeClient.Create(ctx, rs) var statusErr *k8sErrors.StatusError Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create ReplicaSet call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) From f2bcd896e578452d14ff7b91281343ffb620c2f7 Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Thu, 10 Nov 2022 13:24:00 -0800 Subject: [PATCH 8/9] MemberCluster namespace included. Reverted change to const access. --- pkg/utils/common.go | 14 +++++++------- test/e2e/e2e_test.go | 20 +++++++++++++++++--- test/e2e/webhook_test.go | 7 ++++--- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/pkg/utils/common.go b/pkg/utils/common.go index eaea50fb6..2a8d915c5 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -29,12 +29,12 @@ import ( ) const ( - KubePrefix = "kube-" - FleetPrefix = "fleet-" - FleetSystemNamespace = FleetPrefix + "system" - NamespaceNameFormat = FleetPrefix + "member-%s" - RoleNameFormat = FleetPrefix + "role-%s" - RoleBindingNameFormat = FleetPrefix + "rolebinding-%s" + kubePrefix = "kube-" + fleetPrefix = "fleet-" + FleetSystemNamespace = fleetPrefix + "system" + NamespaceNameFormat = fleetPrefix + "member-%s" + RoleNameFormat = fleetPrefix + "role-%s" + RoleBindingNameFormat = fleetPrefix + "rolebinding-%s" ) const ( @@ -244,7 +244,7 @@ func ShouldPropagateObj(informerManager informer.Manager, uObj *unstructured.Uns // IsReservedNamespace indicates if an argued namespace is reserved. func IsReservedNamespace(namespace string) bool { - return strings.HasPrefix(namespace, FleetPrefix) || strings.HasPrefix(namespace, KubePrefix) + return strings.HasPrefix(namespace, fleetPrefix) || strings.HasPrefix(namespace, kubePrefix) } // ShouldPropagateNamespace decides if we should propagate the resources in the namespace. diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index e924bfde6..0fd779097 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -43,17 +43,31 @@ var ( imc *v1alpha1.InternalMemberCluster ctx context.Context - // This namespace will store Member cluster-related CRs, such as v1alpha1.MemberCluster + // The fleet-system namespace. + fleetSystemNamespace = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("fleet-system"), + }, + } + + // The kube-system namespace + kubeSystemNamespace = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("kube-system"), + }, + } + + // This namespace will store Member cluster-related CRs, such as v1alpha1.MemberCluster. memberNamespace = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(utils.NamespaceNameFormat, MemberCluster.ClusterName), + Name: fmt.Sprintf("fleet-member-%s", MemberCluster.ClusterName), }, } // This namespace in HubCluster will store v1alpha1.Work to simulate Work-related features in Hub Cluster. workNamespace = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf(utils.NamespaceNameFormat, MemberCluster.ClusterName), + Name: fmt.Sprintf("fleet-member-%s", MemberCluster.ClusterName), }, } diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index 4ed093f0d..db771032c 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -26,9 +26,10 @@ import ( ) var ( - reservedNamespaces = []corev1.Namespace{ - {ObjectMeta: metav1.ObjectMeta{Name: "kube-system"}}, - {ObjectMeta: metav1.ObjectMeta{Name: utils.FleetSystemNamespace}}, + reservedNamespaces = []*corev1.Namespace{ + fleetSystemNamespace, + kubeSystemNamespace, + memberNamespace, } ) From ad9ee164ddca469002a36ce58f6992c9d5a0580b Mon Sep 17 00:00:00 2001 From: Sean Hobbs Date: Thu, 10 Nov 2022 17:13:53 -0800 Subject: [PATCH 9/9] Removed accidental sprintf. --- test/e2e/e2e_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 0fd779097..e66757bf5 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -46,14 +46,14 @@ var ( // The fleet-system namespace. fleetSystemNamespace = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("fleet-system"), + Name: "fleet-system", }, } // The kube-system namespace kubeSystemNamespace = &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("kube-system"), + Name: "kube-system", }, }