Merge pull request #112 from yue9944882/adding-spoke-cluster-common-group

Defining a common user-group to match all spoke-clusters

pkg/hub/csr/controller.go

@@ -15,16 +15,17 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	certificatesinformers "k8s.io/client-go/informers/certificates/v1beta1"
 	"k8s.io/client-go/kubernetes"
 	certificateslisters "k8s.io/client-go/listers/certificates/v1beta1"
 	"k8s.io/klog/v2"
 
 	"github.com/open-cluster-management/registration/pkg/helpers"
+	"github.com/open-cluster-management/registration/pkg/hub/user"
 )
 
 const (
-	subjectPrefix         = "system:open-cluster-management:"
 	spokeClusterNameLabel = "open-cluster-management.io/cluster-name"
 )
 
@@ -158,16 +159,20 @@ func isSpokeClusterClientCertRenewal(csr *certificatesv1beta1.CertificateSigning
 		return false
 	}
 
-	if len(x509cr.Subject.Organization) != 1 {
+	requestingOrgs := sets.NewString(x509cr.Subject.Organization...)
+	if requestingOrgs.Has(user.ManagedClustersGroup) { // optional common group for backward-compatibility
+		requestingOrgs.Delete(user.ManagedClustersGroup)
+	}
+	if requestingOrgs.Len() != 1 {
 		return false
 	}
 
-	organization := x509cr.Subject.Organization[0]
-	if organization != fmt.Sprintf("%s%s", subjectPrefix, spokeClusterName) {
+	expectedPerClusterOrg := fmt.Sprintf("%s%s", user.SubjectPrefix, spokeClusterName)
+	if !requestingOrgs.Has(expectedPerClusterOrg) {
 		return false
 	}
 
-	if !strings.HasPrefix(x509cr.Subject.CommonName, organization) {
+	if !strings.HasPrefix(x509cr.Subject.CommonName, expectedPerClusterOrg) {
 		return false
 	}
 

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 {

pkg/hub/user/doc.go

@@ -0,0 +1,2 @@
+// Package user contains common definition works for kubernetes certificates
+package user

pkg/hub/user/identity.go

@@ -0,0 +1,8 @@
+package user
+
+const (
+	// SubjectPrefix is a prefix for marking open-cluster-management users
+	SubjectPrefix = "system:open-cluster-management:"
+	// ManagedClustersGroup is a common group for all spoke clusters
+	ManagedClustersGroup = SubjectPrefix + "managed-clusters"
+)

pkg/spoke/hubclientcert/certificate.go

@@ -12,6 +12,8 @@ import (
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 	certutil "k8s.io/client-go/util/cert"
 	"k8s.io/klog/v2"
+
+	"github.com/open-cluster-management/registration/pkg/hub/user"
 )
 
 // HasValidKubeconfig checks if there exists a valid kubeconfig in the given secret
@@ -185,10 +187,10 @@ func GetClusterAgentNamesFromCertificate(certData []byte) (clusterName, agentNam
 	}
 
 	for _, cert := range certs {
-		if ok := strings.HasPrefix(cert.Subject.CommonName, subjectPrefix); !ok {
+		if ok := strings.HasPrefix(cert.Subject.CommonName, user.SubjectPrefix); !ok {
 			continue
 		}
-		names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, subjectPrefix), ":")
+		names := strings.Split(strings.TrimPrefix(cert.Subject.CommonName, user.SubjectPrefix), ":")
 		if len(names) != 2 {
 			continue
 		}

pkg/spoke/hubclientcert/controller.go

@@ -28,6 +28,8 @@ import (
 	certutil "k8s.io/client-go/util/cert"
 	"k8s.io/client-go/util/keyutil"
 	"k8s.io/klog/v2"
+
+	"github.com/open-cluster-management/registration/pkg/hub/user"
 )
 
 const (
@@ -38,7 +40,6 @@ const (
 	// TLSCertFile is the name of the tls cert file in kubeconfigSecret
 	TLSCertFile = "tls.crt"
 
-	subjectPrefix         = "system:open-cluster-management:"
 	clusterNameAnnotation = "open-cluster-management.io/cluster-name"
 	ClusterNameFile       = "cluster-name"
 	AgentNameFile         = "agent-name"
@@ -181,7 +182,7 @@ func (c *ClientCertForHubController) sync(ctx context.Context, syncCtx factory.S
 	// create a csr to request new client certificate if
 	// a. there is no valid client certificate issued for the current cluster/agent
 	// b. client certificate exists and has less than 20% of its life remaining
-	if hasValidKubeconfig(secret, fmt.Sprintf("%s%s:%s", subjectPrefix, c.clusterName, c.agentName)) {
+	if hasValidKubeconfig(secret, fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName)) {
 		notBefore, notAfter, err := getCertValidityPeriod(secret)
 		if err != nil {
 			return err
@@ -284,8 +285,11 @@ func (c *ClientCertForHubController) syncCSR(secret *corev1.Secret) (map[string]
 
 func (c *ClientCertForHubController) createCSR() (string, error) {
 	subject := &pkix.Name{
-		Organization: []string{fmt.Sprintf("%s%s", subjectPrefix, c.clusterName)},
-		CommonName:   fmt.Sprintf("%s%s:%s", subjectPrefix, c.clusterName, c.agentName),
+		Organization: []string{
+			fmt.Sprintf("%s%s", user.SubjectPrefix, c.clusterName),
+			user.ManagedClustersGroup,
+		},
+		CommonName: fmt.Sprintf("%s%s:%s", user.SubjectPrefix, c.clusterName, c.agentName),
 	}
 
 	privateKey, err := keyutil.ParsePrivateKeyPEM(c.keyData)

pkg/spoke/hubclientcert/controller_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	testinghelpers "github.com/open-cluster-management/registration/pkg/helpers/testing"
+	"github.com/open-cluster-management/registration/pkg/hub/user"
 
 	certificates "k8s.io/api/certificates/v1beta1"
 	corev1 "k8s.io/api/core/v1"
@@ -26,7 +27,7 @@ const (
 	testCSRName    = "testcsr"
 )
 
-var commonName = fmt.Sprintf("%s%s:%s", subjectPrefix, testinghelpers.TestManagedClusterName, testAgentName)
+var commonName = fmt.Sprintf("%s%s:%s", user.SubjectPrefix, testinghelpers.TestManagedClusterName, testAgentName)
 
 func TestSync(t *testing.T) {
 	cases := []struct {