the cluster-admin should be respected for the propagated appsub (#391)

Signed-off-by: Xiangjing Li <xiangli@redhat.com>
open-cluster-management-io · Feb 21, 2024 · dd8f192 · dd8f192
1 parent 3975885
commit dd8f192
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 5 deletions.
diff --git 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
@@ -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) {