From dd8f1924b0e06cf72c57623b8a2611e7a95d878a Mon Sep 17 00:00:00 2001 From: Xiangjing Li <55890329+xiangjingli@users.noreply.github.com> Date: Wed, 21 Feb 2024 10:19:39 -0500 Subject: [PATCH] the cluster-admin should be respected for the propagated appsub (#391) Signed-off-by: Xiangjing Li --- pkg/utils/gitrepo.go | 15 ++++++++++++--- pkg/utils/gitrepo_test.go | 22 ++++++++++++++++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/pkg/utils/gitrepo.go b/pkg/utils/gitrepo.go index e7d235b2..0e2ab9dc 100644 --- a/pkg/utils/gitrepo.go +++ b/pkg/utils/gitrepo.go @@ -934,9 +934,12 @@ func IsClusterAdmin(client client.Client, sub *appv1.Subscription, eventRecorder } } - // If subscription has cluster-admin:true and propagated from hub and cannot find the webhook, we know we are - // on the managed cluster so trust the annotations to decide that the subscription is from subscription-admin. - // But the subscription can also be propagated to the self-managed hub cluster. + // When the appsub has hosting-subscription annotation, the cluster-admin: true annotation is always respected, including 3 cases: + // 1: the appsub is on the managed cluster propagated by a hub appsub, there is no ocm-mutating-webhook found in the current cluster + // 2: the appsub is on the hub containing -local name suffix, the appsub is propagated by a hub appsub. + // 3: the appsub is on the hub not containing -local name suffix, the appsub is propagated by a hub sub of sub. + isHubSubOfHubSub := isSubPropagatedFromHub && doesWebhookExist && !strings.HasSuffix(sub.GetName(), "-local") + if isClusterAdminAnnotationTrue && isSubPropagatedFromHub { if !doesWebhookExist || // not on the hub cluster (doesWebhookExist && strings.HasSuffix(sub.GetName(), "-local")) { // on the hub cluster and the subscription has -local suffix @@ -945,6 +948,12 @@ func IsClusterAdmin(client client.Client, sub *appv1.Subscription, eventRecorder "Role was elevated to cluster admin for subscription "+sub.Name, nil) } + isClusterAdmin = true + } else if isHubSubOfHubSub { // hub appsub of a hub appsub + if eventRecorder != nil { + eventRecorder.RecordEvent(sub, "RoleElevation", + "Role was elevated to cluster admin for hub subscription of hub subscription "+sub.Name, nil) + } isClusterAdmin = true } } else if isUserSubAdmin { diff --git a/pkg/utils/gitrepo_test.go b/pkg/utils/gitrepo_test.go index d3815b06..67fe36eb 100644 --- a/pkg/utils/gitrepo_test.go +++ b/pkg/utils/gitrepo_test.go @@ -646,6 +646,9 @@ func TestIsClusterAdminLocal(t *testing.T) { // The mutation webhook does not exist. + // Don't specify hosting-subscription annotation + // Don't specify cluster-admin annotation + // In this case, IsClusterAdmin is expected to return false subscriptionYAML := `apiVersion: apps.open-cluster-management.io/v1 kind: Subscription metadata: @@ -662,6 +665,9 @@ spec: g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeFalse()) + // specify hosting-subscription annotation + // Don't specify cluster-admin annotation + // In this case, IsClusterAdmin is expected to return false subscriptionYAML = `apiVersion: apps.open-cluster-management.io/v1 kind: Subscription metadata: @@ -680,6 +686,9 @@ spec: g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeFalse()) + // specify hosting-subscription annotation + // specify cluster-admin annotation to be false + // In this case, IsClusterAdmin is expected to return false subscriptionYAML = `apiVersion: apps.open-cluster-management.io/v1 kind: Subscription metadata: @@ -699,6 +708,9 @@ spec: g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeFalse()) + // specify hosting-subscription annotation + // specify cluster-admin annotation to be true + // In this case, IsClusterAdmin is expected to return true subscriptionYAML = `apiVersion: apps.open-cluster-management.io/v1 kind: Subscription metadata: @@ -718,6 +730,9 @@ spec: g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeTrue()) + // Don't specify hosting-subscription annotation + // specify cluster-admin annotation to be true + // In this case, IsClusterAdmin is expected to return false subscriptionYAML = `apiVersion: apps.open-cluster-management.io/v1 kind: Subscription metadata: @@ -735,7 +750,10 @@ spec: g.Expect(err).NotTo(gomega.HaveOccurred()) g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeFalse()) - + // specify ocm-mutating-webhook webhook to indicate the appsub is on hub + // specify hosting-subscription annotation + // specify cluster-admin annotation to be true + // In this case, IsClusterAdmin is expected to return true webhookYAML := `apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: @@ -780,7 +798,7 @@ metadata: err = yaml.Unmarshal([]byte(subscriptionYAML), &subscription) g.Expect(err).NotTo(gomega.HaveOccurred()) - g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeFalse()) + g.Expect(IsClusterAdmin(c, subscription, nil)).To(gomega.BeTrue()) } func TestIsClusterAdminRemote(t *testing.T) {