Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion pkg/registration/register/aws_irsa/aws_irsa.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package aws_irsa
import (
"context"
"fmt"
operatorv1 "open-cluster-management.io/api/operator/v1"

"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/events"
Expand All @@ -22,7 +23,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 {
Expand Down Expand Up @@ -95,6 +98,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{}
}
4 changes: 4 additions & 0 deletions pkg/registration/register/csr/csr.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/registration/register/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package register

import (
"context"

"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/events"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -71,6 +70,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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could understand the intension. But I wonder it is suitable for an interface since it seems quite specific to a certain implementation. I am thinking whether it makes more sense to have a method like

ClusterDecorator(cluster *v1.ManagedCluster) *v1.ManagedCluster

and call it in the creatingCluster controller.

I can try to create an example PR to show how it looks like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qiujian16 We are incorporating the changes from #752 into our changes. We'll let you know once we are ready for another around of reviews. Thanks for creating the example PR to demonstrate the changes.

}

// Approvers is the inteface that each driver should implement on hub side. The hub controller will use this driver
Expand Down
20 changes: 4 additions & 16 deletions pkg/registration/spoke/spokeagent.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +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"
"open-cluster-management.io/ocm/pkg/features"
Expand Down Expand Up @@ -191,24 +189,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
Expand Down
14 changes: 14 additions & 0 deletions pkg/registration/spoke/spokeagent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down
69 changes: 69 additions & 0 deletions test/integration/registration/clusterannotations_aws_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package registration_test

import (
"fmt"
operatorv1 "open-cluster-management.io/api/operator/v1"
"path"
"time"

"github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"

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())

})
})
10 changes: 10 additions & 0 deletions test/integration/registration/clusterannotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package registration_test

import (
"fmt"
operatorv1 "open-cluster-management.io/api/operator/v1"
"open-cluster-management.io/ocm/pkg/registration/register/aws_irsa"
"path"
"time"

Expand Down Expand Up @@ -49,6 +51,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"])
}
Expand Down
118 changes: 118 additions & 0 deletions test/integration/registration/spokecluster_aws_joining_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
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()
})

})
Loading