From fc792f3816ed1e2f2d0609a5c0ac69da32ed2ab9 Mon Sep 17 00:00:00 2001 From: xuezhaojun Date: Sun, 9 Apr 2023 20:39:36 +0800 Subject: [PATCH] Fix: deny accept request when namespace is terminating. (#310) Signed-off-by: xuezhaojun --- deploy/webhook/clusterrole.yaml | 3 + pkg/webhook/v1/managedcluster_validating.go | 50 ++++++-- .../v1/managedcluster_validating_test.go | 114 +++++++++++++++++- test/e2e/webhook_test.go | 108 +++++++++++++++++ 4 files changed, 265 insertions(+), 10 deletions(-) diff --git a/deploy/webhook/clusterrole.yaml b/deploy/webhook/clusterrole.yaml index 06e1f03cb..b13080fcd 100644 --- a/deploy/webhook/clusterrole.yaml +++ b/deploy/webhook/clusterrole.yaml @@ -7,6 +7,9 @@ rules: - apiGroups: [""] resources: ["configmaps"] verbs: ["get", "list", "watch"] +- apiGroups: [""] + resources: ["namespaces"] + verbs: ["get"] # Allow managedcluster admission to create subjectaccessreviews - apiGroups: ["authorization.k8s.io"] resources: ["subjectaccessreviews"] diff --git a/pkg/webhook/v1/managedcluster_validating.go b/pkg/webhook/v1/managedcluster_validating.go index 5f69b90de..a4e89e70e 100644 --- a/pkg/webhook/v1/managedcluster_validating.go +++ b/pkg/webhook/v1/managedcluster_validating.go @@ -19,6 +19,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + corev1 "k8s.io/api/core/v1" ) var _ webhook.CustomValidator = &ManagedClusterWebhook{} @@ -33,17 +35,22 @@ func (r *ManagedClusterWebhook) ValidateCreate(ctx context.Context, obj runtime. if err != nil { return apierrors.NewBadRequest(err.Error()) } + //Validate if Spec.ManagedClusterClientConfigs is Valid HTTPS URL err = r.validateManagedClusterObj(*managedCluster) if err != nil { return err } - // the HubAcceptsClient field is changed, we need to check the request user whether - // has been allowed to change the HubAcceptsClient field with SubjectAccessReview api + // the HubAcceptsClient field is changed, we need to: + // 1. check whether cluster namespace is terminating. + // 2. check the request user whether has been allowed to change the HubAcceptsClient field with + // SubjectAccessReview api. if managedCluster.Spec.HubAcceptsClient { - err := r.allowUpdateAcceptField(managedCluster.Name, req.UserInfo) - if err != nil { + if err := r.validateAcceptByClusterNamespace(managedCluster.Name); err != nil { + return err + } + if err := r.allowUpdateAcceptField(managedCluster.Name, req.UserInfo); err != nil { return err } } @@ -78,12 +85,16 @@ func (r *ManagedClusterWebhook) ValidateUpdate(ctx context.Context, oldObj, newO return err } - // the HubAcceptsClient field is changed, we need to check the request user whether - // has been allowed to change the HubAcceptsClient field with SubjectAccessReview api + // the HubAcceptsClient field is changed, we need to: + // 1. check whether cluster namespace is terminating. + // 2. check the request user whether has been allowed to change the HubAcceptsClient field with + // SubjectAccessReview api. if managedCluster.Spec.HubAcceptsClient != oldManagedCluster.Spec.HubAcceptsClient { if managedCluster.Spec.HubAcceptsClient { - err := r.allowUpdateAcceptField(managedCluster.Name, req.UserInfo) - if err != nil { + if err := r.validateAcceptByClusterNamespace(managedCluster.Name); err != nil { + return err + } + if err := r.allowUpdateAcceptField(managedCluster.Name, req.UserInfo); err != nil { return err } } @@ -173,6 +184,29 @@ func (r *ManagedClusterWebhook) allowUpdateAcceptField(clusterName string, userI return nil } +// validateClusterNamespace checks the cluster namespace, if the namespace is terminating, reject the accept request. +func (r *ManagedClusterWebhook) validateAcceptByClusterNamespace(clusterName string) error { + clusterNamespace, err := r.kubeClient.CoreV1().Namespaces().Get(context.TODO(), clusterName, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return nil + } + if err != nil { + return apierrors.NewForbidden( + v1.Resource("managedclusters/accept"), + clusterName, + err, + ) + } + if clusterNamespace.Status.Phase == corev1.NamespaceTerminating { + return apierrors.NewForbidden( + v1.Resource("managedclusters/accept"), + clusterName, + fmt.Errorf("cluster namespace %q is terminating", clusterName), + ) + } + return nil +} + // allowSetClusterSetLabel checks whether a request user has been authorized to set clusterset label func (r *ManagedClusterWebhook) allowSetClusterSetLabel(userInfo authenticationv1.UserInfo, originalClusterSet, newClusterSet string) error { if originalClusterSet == newClusterSet { diff --git a/pkg/webhook/v1/managedcluster_validating_test.go b/pkg/webhook/v1/managedcluster_validating_test.go index 3c94171ae..424a249b7 100644 --- a/pkg/webhook/v1/managedcluster_validating_test.go +++ b/pkg/webhook/v1/managedcluster_validating_test.go @@ -15,12 +15,15 @@ import ( clienttesting "k8s.io/client-go/testing" v1 "open-cluster-management.io/api/cluster/v1" "open-cluster-management.io/api/cluster/v1beta1" + + corev1 "k8s.io/api/core/v1" ) func TestValidateCreate(t *testing.T) { cases := []struct { name string cluster *v1.ManagedCluster + preObjs []runtime.Object expectedError bool allowUpdateAcceptField bool allowClusterset bool @@ -61,6 +64,52 @@ func TestValidateCreate(t *testing.T) { }, }, }, + { + name: "validate cluster namespace, namespace is active", + expectedError: false, + allowUpdateAcceptField: true, + cluster: &v1.ManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Spec: v1.ManagedClusterSpec{ + HubAcceptsClient: true, + }, + }, + preObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Status: corev1.NamespaceStatus{ + Phase: corev1.NamespaceActive, + }, + }, + }, + }, + { + name: "validate cluster namespace, namespace is terminating", + expectedError: true, + allowUpdateAcceptField: true, + cluster: &v1.ManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Spec: v1.ManagedClusterSpec{ + HubAcceptsClient: true, + }, + }, + preObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Status: corev1.NamespaceStatus{ + Phase: corev1.NamespaceTerminating, + }, + }, + }, + }, { name: "validate setting clusterset label", expectedError: false, @@ -136,7 +185,7 @@ func TestValidateCreate(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - kubeClient := kubefake.NewSimpleClientset() + kubeClient := kubefake.NewSimpleClientset(c.preObjs...) kubeClient.PrependReactor( "create", "subjectaccessreviews", @@ -194,6 +243,7 @@ func TestValidateUpdate(t *testing.T) { name string cluster *v1.ManagedCluster oldCluster *v1.ManagedCluster + preObjs []runtime.Object expectedError bool allowUpdateAcceptField bool allowClusterset bool @@ -244,6 +294,66 @@ func TestValidateUpdate(t *testing.T) { }, }, }, + { + name: "validate update ManagedCluster when namespace is terminating", + expectedError: true, + allowUpdateAcceptField: true, + cluster: &v1.ManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Spec: v1.ManagedClusterSpec{ + HubAcceptsClient: true, + }, + }, + oldCluster: &v1.ManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Spec: v1.ManagedClusterSpec{ + HubAcceptsClient: false, + }, + }, + preObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Status: corev1.NamespaceStatus{ + Phase: corev1.NamespaceTerminating, + }, + }}, + }, + { + name: "validate update ManagedCluster when namespace is active", + expectedError: false, + allowUpdateAcceptField: true, + cluster: &v1.ManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Spec: v1.ManagedClusterSpec{ + HubAcceptsClient: true, + }, + }, + oldCluster: &v1.ManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Spec: v1.ManagedClusterSpec{ + HubAcceptsClient: false, + }, + }, + preObjs: []runtime.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "set-1", + }, + Status: corev1.NamespaceStatus{ + Phase: corev1.NamespaceActive, + }, + }}, + }, { name: "validate updating an accepted ManagedCluster with permission", expectedError: false, @@ -443,7 +553,7 @@ func TestValidateUpdate(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - kubeClient := kubefake.NewSimpleClientset() + kubeClient := kubefake.NewSimpleClientset(c.preObjs...) kubeClient.PrependReactor( "create", "subjectaccessreviews", diff --git a/test/e2e/webhook_test.go b/test/e2e/webhook_test.go index b37ec0cd1..5575805e0 100644 --- a/test/e2e/webhook_test.go +++ b/test/e2e/webhook_test.go @@ -239,6 +239,62 @@ var _ = ginkgo.Describe("Admission webhook", func() { gomega.Expect(cleanupClusterClient(saNamespace, sa)).ToNot(gomega.HaveOccurred()) }) + ginkgo.It("Should forbid the request when creating a managed cluster with a termaniting namespace", func() { + var err error + + sa := fmt.Sprintf("webhook-sa-%s", rand.String(6)) + clusterName := fmt.Sprintf("webhook-spoke-%s", rand.String(6)) + + authorizedClient, err := buildClusterClient(saNamespace, sa, []rbacv1.PolicyRule{ + { + APIGroups: []string{"cluster.open-cluster-management.io"}, + Resources: []string{"managedclusters"}, + Verbs: []string{"create", "get", "update"}, + }, + { + APIGroups: []string{"cluster.open-cluster-management.io"}, + Resources: []string{"managedclustersets/join"}, + Verbs: []string{"create"}, + }, + { + APIGroups: []string{"register.open-cluster-management.io"}, + Resources: []string{"managedclusters/accept"}, + ResourceNames: []string{clusterName}, + Verbs: []string{"update"}, + }, + }, nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // create a namespace, add a finilizer to it, and delete it + _, err = hubClient.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Finalizers: []string{ + "open-cluster-mangement.io/finalizer", + }, + }, + }, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // delete the namespace + err = hubClient.CoreV1().Namespaces().Delete(context.TODO(), clusterName, metav1.DeleteOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // create a managed cluster, should be denied + managedCluster := newManagedCluster(clusterName, true, validURL) + _, err = authorizedClient.ClusterV1().ManagedClusters().Create(context.TODO(), managedCluster, metav1.CreateOptions{}) + gomega.Expect(errors.IsForbidden(err)).Should(gomega.BeTrue()) + + // remove the finalizer to truely delete the namespace + ns, err := hubClient.CoreV1().Namespaces().Get(context.TODO(), clusterName, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + ns.Finalizers = []string{} + _, err = hubClient.CoreV1().Namespaces().Update(context.TODO(), ns, metav1.UpdateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + gomega.Expect(cleanupClusterClient(saNamespace, sa)).ToNot(gomega.HaveOccurred()) + }) + ginkgo.It("Should accept the request when creating an accepted managed cluster by authorized user", func() { sa := fmt.Sprintf("webhook-sa-%s", rand.String(6)) clusterName := fmt.Sprintf("webhook-spoke-%s", rand.String(6)) @@ -505,6 +561,58 @@ var _ = ginkgo.Describe("Admission webhook", func() { gomega.Expect(cleanupClusterClient(saNamespace, sa)).ToNot(gomega.HaveOccurred()) }) + ginkgo.It("Should forbid the request when updating a managed cluster with a terminating namespace", func() { + sa := fmt.Sprintf("webhook-sa-%s", rand.String(6)) + var err error + authorizedClient, err := buildClusterClient(saNamespace, sa, []rbacv1.PolicyRule{ + { + APIGroups: []string{"cluster.open-cluster-management.io"}, + Resources: []string{"managedclusters"}, + Verbs: []string{"create", "get", "update"}, + }, + { + APIGroups: []string{"cluster.open-cluster-management.io"}, + Resources: []string{"managedclustersets/join"}, + Verbs: []string{"create"}, + }, + { + APIGroups: []string{"register.open-cluster-management.io"}, + Resources: []string{"managedclusters/accept"}, + ResourceNames: []string{clusterName}, + Verbs: []string{"update"}, + }, + }, nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // create a namespace, add a finilizer to it, and delete it + _, err = hubClient.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Finalizers: []string{ + "open-cluster-mangement.io/finalizer", + }, + }, + }, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // delete the namespace + err = hubClient.CoreV1().Namespaces().Delete(context.TODO(), clusterName, metav1.DeleteOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // update the HubAcceptsClient field to true + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + managedCluster, err := authorizedClient.ClusterV1().ManagedClusters().Get(context.TODO(), clusterName, metav1.GetOptions{}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + managedCluster.Spec.HubAcceptsClient = true + _, err = authorizedClient.ClusterV1().ManagedClusters().Update(context.TODO(), managedCluster, metav1.UpdateOptions{}) + return err + }) + gomega.Expect(errors.IsForbidden(err)).To(gomega.BeTrue()) + + gomega.Expect(cleanupClusterClient(saNamespace, sa)).ToNot(gomega.HaveOccurred()) + }) + ginkgo.It("Should accept the request when updating the clusterset of a managed cluster by authorized user", func() { clusterSetName := fmt.Sprintf("webhook-spoke-%s", rand.String(6)) ginkgo.By(fmt.Sprintf("create a managed cluster set %q", clusterSetName))