✨ (WIP) Extract the logic to add cluster annotations to the driver interface and add unit and integration tests

pkg/registration/register/aws_irsa/aws_irsa.go

@@ -13,6 +13,8 @@ import (
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/klog/v2"
 
+	operatorv1 "open-cluster-management.io/api/operator/v1"
+
 	"open-cluster-management.io/ocm/pkg/registration/register"
 )
 
@@ -22,7 +24,9 @@ const (
 	// TLSKeyFile is the name of tls key file in kubeconfigSecret
 	TLSKeyFile = "tls.key"
 	// TLSCertFile is the name of the tls cert file in kubeconfigSecret
-	TLSCertFile = "tls.crt"
+	TLSCertFile                 = "tls.crt"
+	ManagedClusterArn           = "managed-cluster-arn"
+	ManagedClusterIAMRoleSuffix = "managed-cluster-iam-role-suffix"
 )
 
 type AWSIRSADriver struct {
@@ -95,6 +99,15 @@ func (c *AWSIRSADriver) IsHubKubeConfigValid(ctx context.Context, secretOption r
 	return true, nil
 }
 
+func (c *AWSIRSADriver) AddClusterAnnotations(clusterAnnotations map[string]string, managedClusterArn string, managedClusterRoleSuffix string) {
+	if clusterAnnotations == nil {
+		clusterAnnotations = map[string]string{}
+	}
+
+	clusterAnnotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+ManagedClusterArn] = managedClusterArn
+	clusterAnnotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+ManagedClusterIAMRoleSuffix] = managedClusterRoleSuffix
+}
+
 func NewAWSIRSADriver() register.RegisterDriver {
 	return &AWSIRSADriver{}
 }

pkg/registration/register/csr/csr.go

@@ -266,6 +266,10 @@ func (c *CSRDriver) IsHubKubeConfigValid(ctx context.Context, secretOption regis
 	return isCertificateValid(logger, certData, nil)
 }
 
+// AddClusterAnnotations noop for CSR driver
+func (c *CSRDriver) AddClusterAnnotations(clusterAnnotations map[string]string, managedClusterArn string, managedClusterRoleSuffix string) {
+}
+
 func NewCSRDriver() register.RegisterDriver {
 	return &CSRDriver{}
 }

pkg/registration/register/interface.go

@@ -71,6 +71,9 @@ type RegisterDriver interface {
 	// InformerHandler returns informer of the related object. If no object needs to be watched, the func could
 	// return nil, nil.
 	InformerHandler(option any) (cache.SharedIndexInformer, factory.EventFilterFunc)
+
+	// AddClusterAnnotations adds cluster annotations for non-CSR drivers
+	AddClusterAnnotations(clusterAnnotations map[string]string, managedClusterArn string, managedClusterRoleSuffix string)
 }
 
 // Approvers is the inteface that each driver should implement on hub side. The hub controller will use this driver

pkg/registration/register/secret_controller_test.go

@@ -167,6 +167,9 @@ type fakeDriver struct {
 	cond   *metav1.Condition
 }
 
+func (f *fakeDriver) AddClusterAnnotations(clusterAnnotations map[string]string, managedClusterArn string, managedClusterRoleSuffix string) {
+}
+
 func newFakeDriver(secret *corev1.Secret, cond *metav1.Condition, err error) *fakeDriver {
 	return &fakeDriver{
 		secret: secret,

pkg/registration/spoke/spokeagent.go

@@ -26,7 +26,6 @@ import (
 	clusterv1informers "open-cluster-management.io/api/client/cluster/informers/externalversions"
 	clusterv1 "open-cluster-management.io/api/cluster/v1"
 	ocmfeature "open-cluster-management.io/api/feature"
-	operatorv1 "open-cluster-management.io/api/operator/v1"
 
 	"open-cluster-management.io/ocm/pkg/common/helpers"
 	commonoptions "open-cluster-management.io/ocm/pkg/common/options"
@@ -191,24 +190,14 @@ func (o *SpokeAgentConfig) RunSpokeAgentWithSpokeInformers(ctx context.Context,
 
 	// initiate registration driver
 	var registerDriver register.RegisterDriver
-	if o.registrationOption.RegistrationAuth == AwsIrsaAuthType {
-		// TODO: may consider add additional validations
-		if o.registrationOption.HubClusterArn != "" {
-			registerDriver = awsIrsa.NewAWSIRSADriver()
-			if o.registrationOption.ClusterAnnotations == nil {
-				o.registrationOption.ClusterAnnotations = map[string]string{}
-			}
-			o.registrationOption.ClusterAnnotations[operatorv1.ClusterAnnotationsKeyPrefix+"/managed-cluster-arn"] = o.registrationOption.ManagedClusterArn
-			o.registrationOption.ClusterAnnotations[operatorv1.ClusterAnnotationsKeyPrefix+"/managed-cluster-iam-role-suffix"] =
-				o.registrationOption.ManagedClusterRoleSuffix
-
-		} else {
-			panic("A valid EKS Hub Cluster ARN is required with awsirsa based authentication")
-		}
+	var registrationOption = o.registrationOption
+	if registrationOption.RegistrationAuth == AwsIrsaAuthType {
+		registerDriver = awsIrsa.NewAWSIRSADriver()
 	} else {
 		registerDriver = csr.NewCSRDriver()
 	}
 
+	registerDriver.AddClusterAnnotations(registrationOption.ClusterAnnotations, registrationOption.ManagedClusterArn, registrationOption.ManagedClusterRoleSuffix)
 	o.driver = registerDriver
 
 	// get spoke cluster CA bundle

pkg/registration/spoke/spokeagent_test.go

@@ -45,6 +45,10 @@ func init() {
 func TestValidate(t *testing.T) {
 	defaultCompletedOptions := NewSpokeAgentOptions()
 	defaultCompletedOptions.BootstrapKubeconfig = "/spoke/bootstrap/kubeconfig"
+	awsCompletedOptionsHubArnMissing := *defaultCompletedOptions
+	awsCompletedOptionsHubArnMissing.RegistrationAuth = AwsIrsaAuthType
+	awsDefaultCompletedOptions := awsCompletedOptionsHubArnMissing
+	awsDefaultCompletedOptions.HubClusterArn = "arn:aws:eks:us-west-2:123456789012:cluster/hub-cluster1"
 
 	cases := []struct {
 		name        string
@@ -78,6 +82,16 @@ func TestValidate(t *testing.T) {
 			options:     defaultCompletedOptions,
 			expectedErr: "",
 		},
+		{
+			name:        "default completed options for aws flow",
+			options:     &awsDefaultCompletedOptions,
+			expectedErr: "",
+		},
+		{
+			name:        "default completed options without HubClusterArn for aws flow",
+			options:     &awsCompletedOptionsHubArnMissing,
+			expectedErr: "EksHubClusterArn cannot be empty if RegistrationAuth is awsirsa",
+		},
 		{
 			name: "default completed options",
 			options: &SpokeAgentOptions{

test/integration/registration/clusterannotations_aws_test.go

@@ -0,0 +1,72 @@
+package registration_test
+
+import (
+	"fmt"
+	"path"
+	"time"
+
+	"github.com/onsi/ginkgo/v2"
+	"github.com/onsi/gomega"
+
+	operatorv1 "open-cluster-management.io/api/operator/v1"
+
+	commonoptions "open-cluster-management.io/ocm/pkg/common/options"
+	"open-cluster-management.io/ocm/pkg/registration/register/aws_irsa"
+	"open-cluster-management.io/ocm/pkg/registration/spoke"
+	"open-cluster-management.io/ocm/test/integration/util"
+)
+
+var _ = ginkgo.Describe("Cluster Annotations for aws", func() {
+	ginkgo.It("Cluster Annotations for aws flow should be created on the managed cluster", func() {
+		managedClusterName := "clusterannotations-spokecluster-aws"
+		//#nosec G101
+		hubKubeconfigSecret := "clusterannotations-hub-kubeconfig-secret"
+		hubKubeconfigDir := path.Join(util.TestDir, "clusterannotations", "hub-kubeconfig")
+
+		managedClusterArn := "arn:aws:eks:us-west-2:123456789012:cluster/managed-cluster1"
+		managedClusterRoleSuffix := "7f8141296c75f2871e3d030f85c35692"
+		hubClusterArn := "arn:aws:eks:us-west-2:123456789012:cluster/hub-cluster1"
+		agentOptions := &spoke.SpokeAgentOptions{
+			RegistrationAuth:         spoke.AwsIrsaAuthType,
+			HubClusterArn:            hubClusterArn,
+			ManagedClusterArn:        managedClusterArn,
+			ManagedClusterRoleSuffix: managedClusterRoleSuffix,
+			BootstrapKubeconfig:      bootstrapKubeConfigFile,
+			HubKubeconfigSecret:      hubKubeconfigSecret,
+			ClusterHealthCheckPeriod: 1 * time.Minute,
+			ClusterAnnotations: map[string]string{
+				"agent.open-cluster-management.io/foo": "bar",
+				"foo":                                  "bar", // this annotation should be filtered out
+			},
+		}
+
+		commOptions := commonoptions.NewAgentOptions()
+		commOptions.HubKubeconfigDir = hubKubeconfigDir
+		commOptions.SpokeClusterName = managedClusterName
+
+		// run registration agent
+		cancel := runAgent("rotationtest", agentOptions, commOptions, spokeCfg)
+		defer cancel()
+
+		// after bootstrap the spokecluster and csr should be created
+		gomega.Eventually(func() error {
+			mc, err := util.GetManagedCluster(clusterClient, managedClusterName)
+			if err != nil {
+				return err
+			}
+
+			if mc.Annotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterArn] != managedClusterArn {
+				return fmt.Errorf("expected annotation "+operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterArn+" to be "+
+					""+managedClusterArn+", got %s", mc.Annotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterArn])
+			}
+
+			if mc.Annotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterIAMRoleSuffix] != managedClusterRoleSuffix {
+				return fmt.Errorf("expected annotation "+operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterIAMRoleSuffix+" "+
+					"to be "+managedClusterRoleSuffix+", got %s", mc.Annotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterIAMRoleSuffix])
+			}
+
+			return nil
+		}, eventuallyTimeout, eventuallyInterval).Should(gomega.Succeed())
+
+	})
+})

test/integration/registration/clusterannotations_test.go

@@ -8,7 +8,10 @@ import (
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
 
+	operatorv1 "open-cluster-management.io/api/operator/v1"
+
 	commonoptions "open-cluster-management.io/ocm/pkg/common/options"
+	"open-cluster-management.io/ocm/pkg/registration/register/aws_irsa"
 	"open-cluster-management.io/ocm/pkg/registration/spoke"
 	"open-cluster-management.io/ocm/test/integration/util"
 )
@@ -49,6 +52,14 @@ var _ = ginkgo.Describe("Cluster Annotations", func() {
 				return fmt.Errorf("unexpected annotations %v", mc.Annotations)
 			}
 
+			if _, ok := mc.Annotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterArn]; ok {
+				return fmt.Errorf("unexpected annotations %v", mc.Annotations)
+			}
+
+			if _, ok := mc.Annotations[operatorv1.ClusterAnnotationsKeyPrefix+"/"+aws_irsa.ManagedClusterIAMRoleSuffix]; ok {
+				return fmt.Errorf("unexpected annotations %v", mc.Annotations)
+			}
+
 			if mc.Annotations["agent.open-cluster-management.io/foo"] != "bar" {
 				return fmt.Errorf("expected annotation agent.open-cluster-management.io/foo to be bar, got %s", mc.Annotations["agent.open-cluster-management.io/foo"])
 			}

test/integration/registration/spokecluster_aws_joining_test.go

@@ -0,0 +1,119 @@
+package registration_test
+
+import (
+	"fmt"
+	"path"
+	"time"
+
+	"github.com/onsi/ginkgo/v2"
+	"github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/util/rand"
+
+	commonoptions "open-cluster-management.io/ocm/pkg/common/options"
+	"open-cluster-management.io/ocm/pkg/registration/spoke"
+	"open-cluster-management.io/ocm/test/integration/util"
+)
+
+var _ = ginkgo.Describe("Joining Process for aws flow", func() {
+	var bootstrapKubeconfig string
+	var managedClusterName string
+	var hubKubeconfigSecret string
+	var hubKubeconfigDir string
+
+	ginkgo.BeforeEach(func() {
+		postfix := rand.String(5)
+		managedClusterName = fmt.Sprintf("joiningtest-managedcluster-%s", postfix)
+		hubKubeconfigSecret = fmt.Sprintf("joiningtest-hub-kubeconfig-secret-%s", postfix)
+		hubKubeconfigDir = path.Join(util.TestDir, fmt.Sprintf("joiningtest-%s", postfix), "hub-kubeconfig")
+	})
+
+	assertJoiningSucceed := func() {
+		ginkgo.It("managedcluster should join successfully for aws flow", func() {
+			var err error
+
+			managedClusterArn := "arn:aws:eks:us-west-2:123456789012:cluster/managed-cluster1"
+			managedClusterRoleSuffix := "7f8141296c75f2871e3d030f85c35692"
+			hubClusterArn := "arn:aws:eks:us-west-2:123456789012:cluster/hub-cluster1"
+
+			// run registration agent
+			agentOptions := &spoke.SpokeAgentOptions{
+				RegistrationAuth:         spoke.AwsIrsaAuthType,
+				HubClusterArn:            hubClusterArn,
+				ManagedClusterArn:        managedClusterArn,
+				ManagedClusterRoleSuffix: managedClusterRoleSuffix,
+				BootstrapKubeconfig:      bootstrapKubeconfig,
+				HubKubeconfigSecret:      hubKubeconfigSecret,
+				ClusterHealthCheckPeriod: 1 * time.Minute,
+			}
+			commOptions := commonoptions.NewAgentOptions()
+			commOptions.HubKubeconfigDir = hubKubeconfigDir
+			commOptions.SpokeClusterName = managedClusterName
+
+			cancel := runAgent("joiningtest", agentOptions, commOptions, spokeCfg)
+			defer cancel()
+
+			// the ManagedCluster CR should be created after bootstrap
+			gomega.Eventually(func() error {
+				if _, err := util.GetManagedCluster(clusterClient, managedClusterName); err != nil {
+					return err
+				}
+				return nil
+			}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			// the csr should not be created for aws flow after bootstrap
+			gomega.Eventually(func() error {
+				if _, err := util.FindUnapprovedSpokeCSR(kubeClient, managedClusterName); err != nil {
+					return err
+				}
+				return nil
+			}, eventuallyTimeout, eventuallyInterval).Should(gomega.HaveOccurred())
+
+			// simulate hub cluster admin to accept the managedcluster
+			err = util.AcceptManagedCluster(clusterClient, managedClusterName)
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+			err = authn.ApproveSpokeClusterCSR(kubeClient, managedClusterName, time.Hour*24)
+			gomega.Expect(err).To(gomega.HaveOccurred())
+
+			// the hub kubeconfig secret should be filled after the ManagedCluster is accepted
+			// TODO: Revisit while implementing slice 3
+			//gomega.Eventually(func() error {
+			//	secret, err := util.GetFilledHubKubeConfigSecret(kubeClient, testNamespace, hubKubeconfigSecret)
+			//	if err != nil {
+			//		return err
+			//	}
+			//
+			//	// check if the proxyURL is set correctly
+			//	proxyURL, err := getProxyURLFromKubeconfigData(secret.Data["kubeconfig"])
+			//	if err != nil {
+			//		return err
+			//	}
+			//	if proxyURL != expectedProxyURL {
+			//		return fmt.Errorf("expected proxy url %q, but got %q", expectedProxyURL, proxyURL)
+			//	}
+			//	return nil
+			//}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+
+			// the spoke cluster should have joined condition finally
+			// TODO: Revisit while implementing slice 3
+			//gomega.Eventually(func() error {
+			//	spokeCluster, err := util.GetManagedCluster(clusterClient, managedClusterName)
+			//	if err != nil {
+			//		return err
+			//	}
+			//	if !meta.IsStatusConditionTrue(spokeCluster.Status.Conditions, clusterv1.ManagedClusterConditionJoined) {
+			//		return fmt.Errorf("cluster should be joined")
+			//	}
+			//	return nil
+			//}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
+		})
+	}
+
+	ginkgo.Context("without proxy", func() {
+		ginkgo.BeforeEach(func() {
+			bootstrapKubeconfig = bootstrapKubeConfigFile
+		})
+		assertJoiningSucceed()
+	})
+
+})