From 842d4bb15e48952cc52afd290c324cc135440510 Mon Sep 17 00:00:00 2001
From: yue9944882 <291271447@qq.com>
Date: Wed, 27 Jan 2021 20:37:06 +0800
Subject: [PATCH] addressing review comments

Signed-off-by: yue9944882 <291271447@qq.com>
---
 pkg/hub/csr/controller.go             |  4 ++--
 pkg/hub/csr/controller_test.go        | 31 ++++++++++++++++++++++++---
 pkg/hub/user/identity.go              |  4 ++--
 pkg/spoke/hubclientcert/controller.go |  2 +-
 4 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/pkg/hub/csr/controller.go b/pkg/hub/csr/controller.go
index 33ab4eaa3..e547789e5 100644
--- a/pkg/hub/csr/controller.go
+++ b/pkg/hub/csr/controller.go
@@ -160,8 +160,8 @@ func isSpokeClusterClientCertRenewal(csr *certificatesv1beta1.CertificateSigning
 	}
 
 	requestingOrgs := sets.NewString(x509cr.Subject.Organization...)
-	if requestingOrgs.Has(user.SpokeClustersGroup) { // optional common group for backward-compatibility
-		requestingOrgs.Delete(user.SpokeClustersGroup)
+	if requestingOrgs.Has(user.ManagedClustersGroup) { // optional common group for backward-compatibility
+		requestingOrgs.Delete(user.ManagedClustersGroup)
 	}
 	if requestingOrgs.Len() != 1 {
 		return false
diff --git a/pkg/hub/csr/controller_test.go b/pkg/hub/csr/controller_test.go
index e417ba3f0..990072264 100644
--- a/pkg/hub/csr/controller_test.go
+++ b/pkg/hub/csr/controller_test.go
@@ -6,12 +6,14 @@ import (
 	"time"
 
 	testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing"
+	"github.com/open-cluster-management/registration/pkg/hub/user"
 
 	"github.com/openshift/library-go/pkg/operator/events/eventstesting"
 
 	authorizationv1 "k8s.io/api/authorization/v1"
 	certificatesv1beta1 "k8s.io/api/certificates/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/informers"
 	kubefake "k8s.io/client-go/kubernetes/fake"
 	clienttesting "k8s.io/client-go/testing"
@@ -23,9 +25,9 @@ var (
 		Name:         "testcsr",
 		Labels:       map[string]string{"open-cluster-management.io/cluster-name": "managedcluster1"},
 		SignerName:   &signerName,
-		CN:           "system:open-cluster-management:managedcluster1:spokeagent1",
-		Orgs:         []string{"system:open-cluster-management:managedcluster1"},
-		Username:     "system:open-cluster-management:managedcluster1:spokeagent1",
+		CN:           user.SubjectPrefix + "managedcluster1:spokeagent1",
+		Orgs:         []string{user.SubjectPrefix + "managedcluster1", user.ManagedClustersGroup},
+		Username:     user.SubjectPrefix + "managedcluster1:spokeagent1",
 		ReqBlockType: "CERTIFICATE REQUEST",
 	}
 )
@@ -96,6 +98,29 @@ func TestSync(t *testing.T) {
 				testinghelpers.AssertCSRCondition(t, actual.(*certificatesv1beta1.CertificateSigningRequest).Status.Conditions, expectedCondition)
 			},
 		},
+		{
+			name: "allow an auto approving csr w/o ManagedClusterGroup for backward-compatibility",
+			startingCSRs: []runtime.Object{testinghelpers.NewCSR(testinghelpers.CSRHolder{
+				Name:         validCSR.Name,
+				Labels:       validCSR.Labels,
+				SignerName:   validCSR.SignerName,
+				CN:           validCSR.CN,
+				Orgs:         sets.NewString(validCSR.Orgs...).Delete(user.ManagedClustersGroup).List(),
+				Username:     validCSR.Username,
+				ReqBlockType: validCSR.ReqBlockType,
+			})},
+			autoApprovingAllowed: true,
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				expectedCondition := certificatesv1beta1.CertificateSigningRequestCondition{
+					Type:    certificatesv1beta1.CertificateApproved,
+					Reason:  "AutoApprovedByHubCSRApprovingController",
+					Message: "Auto approving Managed cluster agent certificate after SubjectAccessReview.",
+				}
+				testinghelpers.AssertActions(t, actions, "create", "update")
+				actual := actions[1].(clienttesting.UpdateActionImpl).Object
+				testinghelpers.AssertCSRCondition(t, actual.(*certificatesv1beta1.CertificateSigningRequest).Status.Conditions, expectedCondition)
+			},
+		},
 	}
 
 	for _, c := range cases {
diff --git a/pkg/hub/user/identity.go b/pkg/hub/user/identity.go
index 964e75b5b..83f6554c7 100644
--- a/pkg/hub/user/identity.go
+++ b/pkg/hub/user/identity.go
@@ -3,6 +3,6 @@ package user
 const (
 	// SubjectPrefix is a prefix for marking open-cluster-management users
 	SubjectPrefix = "system:open-cluster-management:"
-	// SpokeClustersGroup is a common group for all spoke clusters
-	SpokeClustersGroup = SubjectPrefix + "spoke-clusters"
+	// ManagedClustersGroup is a common group for all spoke clusters
+	ManagedClustersGroup = SubjectPrefix + "managed-clusters"
 )
diff --git a/pkg/spoke/hubclientcert/controller.go b/pkg/spoke/hubclientcert/controller.go
index 79dff3e55..1b3a5f116 100644
--- a/pkg/spoke/hubclientcert/controller.go
+++ b/pkg/spoke/hubclientcert/controller.go
@@ -287,7 +287,7 @@ func (c *ClientCertForHubController) createCSR() (string, error) {
 	subject := &pkix.Name{
 		Organization: []string{
 			fmt.Sprintf("%s%s", user.SubjectPrefix, c.clusterName),
-			user.SpokeClustersGroup,
+			user.ManagedClustersGroup,
 		},
 		CommonName: fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName),
 	}