From 6899c0fb33d25d52be14fb046830591a646bbb89 Mon Sep 17 00:00:00 2001 From: Richard Case Date: Thu, 4 Nov 2021 08:26:24 +0000 Subject: [PATCH] fix: support more than 1 eks addon When the EKS addons functionality was added to CAPA there was only 1 addon available (vpc) and so it was never tested with multiple addons. Now there are multiple addons there are issues. The unit and e2e tests only tested 1 addons. On investigation the issue was caused by implicit memory aliasing in a loop. This change fixes the loop and also expands the unit and e2e tests to include multiple eks addons Signed-off-by: Richard Case --- .gitignore | 1 + pkg/cloud/services/eks/addons.go | 3 +- pkg/eks/addons/plan.go | 18 +-- pkg/eks/addons/plan_test.go | 114 ++++++++++++++++-- pkg/eks/addons/procedures.go | 28 ++++- test/e2e/data/e2e_eks_conf.yaml | 5 +- ...late-eks-control-plane-only-withaddon.yaml | 6 + test/e2e/suites/managed/addon.go | 2 +- test/e2e/suites/managed/addon_helpers.go | 12 +- test/e2e/suites/managed/eks_test.go | 27 ++++- 10 files changed, 180 insertions(+), 36 deletions(-) diff --git a/.gitignore b/.gitignore index 24ec630d9f..84740d8e71 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,7 @@ bin # envfiles .env envfile +.envrc # kubeconfigs kind.kubeconfig diff --git a/pkg/cloud/services/eks/addons.go b/pkg/cloud/services/eks/addons.go index 3acd88ef0e..67b3a60c7b 100644 --- a/pkg/cloud/services/eks/addons.go +++ b/pkg/cloud/services/eks/addons.go @@ -191,7 +191,8 @@ func (s *Service) listAddons(eksClusterName string) ([]*string, error) { func (s *Service) translateAPIToAddon(addons []ekscontrolplanev1.Addon) []*eksaddons.EKSAddon { converted := []*eksaddons.EKSAddon{} - for _, addon := range addons { + for i := range addons { + addon := addons[i] convertedAddon := &eksaddons.EKSAddon{ Name: &addon.Name, Version: &addon.Version, diff --git a/pkg/eks/addons/plan.go b/pkg/eks/addons/plan.go index a3bca806ef..a8ec24cf4a 100644 --- a/pkg/eks/addons/plan.go +++ b/pkg/eks/addons/plan.go @@ -49,12 +49,13 @@ func (a *plan) Create(ctx context.Context) ([]planner.Procedure, error) { procedures := []planner.Procedure{} // Handle create and update - for _, desired := range a.desiredAddons { + for i := range a.desiredAddons { + desired := a.desiredAddons[i] installed := a.getInstalled(*desired.Name) if installed == nil { // Need to add the addon procedures = append(procedures, &CreateAddonProcedure{plan: a, name: *desired.Name}) - procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name}) + procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name, includeDegraded: true}) } else { // Check if its just the tags that need updating diffTags := desired.Tags.Difference(installed.Tags) @@ -64,16 +65,17 @@ func (a *plan) Create(ctx context.Context) ([]planner.Procedure, error) { // Check if we also need to update the addon if !desired.IsEqual(installed, false) { procedures = append(procedures, &UpdateAddonProcedure{plan: a, name: *installed.Name}) - procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name}) + procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name, includeDegraded: true}) } else if *installed.Status != eks.AddonStatusActive { // If the desired and installed are the same make sure its active - procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name}) + procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name, includeDegraded: true}) } } } // look for deletions & unchanged - for _, installed := range a.installedAddons { + for i := range a.installedAddons { + installed := a.installedAddons[i] desired := a.getDesired(*installed.Name) if desired == nil { if *installed.Status != eks.AddonStatusDeleting { @@ -87,7 +89,8 @@ func (a *plan) Create(ctx context.Context) ([]planner.Procedure, error) { } func (a *plan) getInstalled(name string) *EKSAddon { - for _, installed := range a.installedAddons { + for i := range a.installedAddons { + installed := a.installedAddons[i] if *installed.Name == name { return installed } @@ -97,7 +100,8 @@ func (a *plan) getInstalled(name string) *EKSAddon { } func (a *plan) getDesired(name string) *EKSAddon { - for _, desired := range a.desiredAddons { + for i := range a.desiredAddons { + desired := a.desiredAddons[i] if *desired.Name == name { return desired } diff --git a/pkg/eks/addons/plan_test.go b/pkg/eks/addons/plan_test.go index 548b839842..52f08c9805 100644 --- a/pkg/eks/addons/plan_test.go +++ b/pkg/eks/addons/plan_test.go @@ -82,13 +82,85 @@ func TestEKSAddonPlan(t *testing.T) { }, }, nil) - m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{ + out := &eks.DescribeAddonOutput{ + Addon: &eks.Addon{ + Status: aws.String(eks.AddonStatusActive), + }, + } + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ AddonName: aws.String(addon1Name), ClusterName: aws.String(clusterName), - })).Return(nil) + })).Return(out, nil) + }, + desiredAddons: []*EKSAddon{ + createDesiredAddon(addon1Name, addon1version), + }, + expectCreateError: false, + expectDoError: false, + }, + { + name: "no installed and 2 desired", + expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) { + m. + CreateAddon(gomock.Eq(&eks.CreateAddonInput{ + AddonName: aws.String(addon1Name), + AddonVersion: aws.String(addon1version), + ClusterName: aws.String(clusterName), + ResolveConflicts: aws.String(eks.ResolveConflictsOverwrite), + Tags: convertTags(createTags()), + })). + Return(&eks.CreateAddonOutput{ + Addon: &eks.Addon{ + AddonArn: aws.String(addonARN), + AddonName: aws.String(addon1Name), + AddonVersion: aws.String(addon1version), + ClusterName: aws.String(clusterName), + CreatedAt: &created, + ModifiedAt: &created, + Status: aws.String(addonStatusCreating), + Tags: convertTags(createTags()), + }, + }, nil) + + out := &eks.DescribeAddonOutput{ + Addon: &eks.Addon{ + Status: aws.String(eks.AddonStatusActive), + }, + } + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ + AddonName: aws.String(addon1Name), + ClusterName: aws.String(clusterName), + })).Return(out, nil) + + m. + CreateAddon(gomock.Eq(&eks.CreateAddonInput{ + AddonName: aws.String("addon2"), + AddonVersion: aws.String(addon1version), + ClusterName: aws.String(clusterName), + ResolveConflicts: aws.String(eks.ResolveConflictsOverwrite), + Tags: convertTags(createTags()), + })). + Return(&eks.CreateAddonOutput{ + Addon: &eks.Addon{ + AddonArn: aws.String(addonARN), + AddonName: aws.String("addon2"), + AddonVersion: aws.String(addon1version), + ClusterName: aws.String(clusterName), + CreatedAt: &created, + ModifiedAt: &created, + Status: aws.String(addonStatusCreating), + Tags: convertTags(createTags()), + }, + }, nil) + + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ + AddonName: aws.String("addon2"), + ClusterName: aws.String(clusterName), + })).Return(out, nil) }, desiredAddons: []*EKSAddon{ createDesiredAddon(addon1Name, addon1version), + createDesiredAddon("addon2", addon1version), }, expectCreateError: false, expectDoError: false, @@ -110,10 +182,15 @@ func TestEKSAddonPlan(t *testing.T) { { name: "1 installed and 1 desired - both same and installed is creating", expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) { - m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{ + out := &eks.DescribeAddonOutput{ + Addon: &eks.Addon{ + Status: aws.String(eks.AddonStatusActive), + }, + } + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ AddonName: aws.String(addon1Name), ClusterName: aws.String(clusterName), - })).Return(nil) + })).Return(out, nil) }, desiredAddons: []*EKSAddon{ createDesiredAddon(addon1Name, addon1version), @@ -142,10 +219,16 @@ func TestEKSAddonPlan(t *testing.T) { Type: aws.String(eks.UpdateTypeVersionUpdate), }, }, nil) - m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{ + + out := &eks.DescribeAddonOutput{ + Addon: &eks.Addon{ + Status: aws.String(eks.AddonStatusActive), + }, + } + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ AddonName: aws.String(addon1Name), ClusterName: aws.String(clusterName), - })).Return(nil) + })).Return(out, nil) }, desiredAddons: []*EKSAddon{ createDesiredAddon(addon1Name, addon1Upgrade), @@ -159,10 +242,15 @@ func TestEKSAddonPlan(t *testing.T) { { name: "1 installed and 1 desired - version upgrade in progress", expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) { - m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{ + out := &eks.DescribeAddonOutput{ + Addon: &eks.Addon{ + Status: aws.String(eks.AddonStatusActive), + }, + } + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ AddonName: aws.String(addon1Name), ClusterName: aws.String(clusterName), - })).Return(nil) + })).Return(out, nil) }, desiredAddons: []*EKSAddon{ createDesiredAddon(addon1Name, addon1Upgrade), @@ -217,10 +305,15 @@ func TestEKSAddonPlan(t *testing.T) { }, }, nil) - m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{ + out := &eks.DescribeAddonOutput{ + Addon: &eks.Addon{ + Status: aws.String(eks.AddonStatusActive), + }, + } + m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{ AddonName: aws.String(addon1Name), ClusterName: aws.String(clusterName), - })).Return(nil) + })).Return(out, nil) }, desiredAddons: []*EKSAddon{ createDesiredAddonExtraTag(addon1Name, addon1Upgrade), @@ -251,7 +344,6 @@ func TestEKSAddonPlan(t *testing.T) { Tags: convertTags(createTags()), }, }, nil) - m.WaitUntilAddonDeleted(gomock.Eq(&eks.DescribeAddonInput{ AddonName: aws.String(addon1Name), ClusterName: aws.String(clusterName), diff --git a/pkg/eks/addons/procedures.go b/pkg/eks/addons/procedures.go index a199cf7ec5..e76974a972 100644 --- a/pkg/eks/addons/procedures.go +++ b/pkg/eks/addons/procedures.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/eks" + "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/wait" ) var ( @@ -166,10 +167,12 @@ func (p *CreateAddonProcedure) Name() string { } // WaitAddonActiveProcedure is a procedure that will wait for an EKS addon -// to be active in a cluster +// to be active in a cluster. Abd optionally include the degraded state. +// Note: addons may be degraded until there are worker nodes. type WaitAddonActiveProcedure struct { - plan *plan - name string + plan *plan + name string + includeDegraded bool } // Do implements the logic for the procedure @@ -179,8 +182,23 @@ func (p *WaitAddonActiveProcedure) Do(ctx context.Context) error { ClusterName: aws.String(p.plan.clusterName), } - if err := p.plan.eksClient.WaitUntilAddonActive(input); err != nil { - return fmt.Errorf("waiting for addon %s to be active: %w", p.name, err) + if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) { + out, describeErr := p.plan.eksClient.DescribeAddon(input) + if describeErr != nil { + return false, describeErr + } + + if *out.Addon.Status == eks.AddonStatusActive { + return true, nil + } + + if p.includeDegraded && *out.Addon.Status == eks.AddonStatusDegraded { + return true, nil + } + + return false, nil + }); err != nil { + return fmt.Errorf("failed waiting for addon %s to be ready: %w", p.name, err) } return nil diff --git a/test/e2e/data/e2e_eks_conf.yaml b/test/e2e/data/e2e_eks_conf.yaml index 28cad3d099..6e8827d5ba 100644 --- a/test/e2e/data/e2e_eks_conf.yaml +++ b/test/e2e/data/e2e_eks_conf.yaml @@ -119,7 +119,10 @@ variables: EXP_EKS: "true" EXP_EKS_IAM: "false" EXP_EKS_ADD_ROLES: "false" - VPC_ADDON_VERSION: "v1.6.3-eksbuild.1" + VPC_ADDON_VERSION: "v1.8.0-eksbuild.1" + COREDNS_ADDON_VERSION: "v1.8.3-eksbuild.1" + CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "1.21.2" + AUTO_CONTROLLER_IDENTITY_CREATOR: "false" intervals: default/wait-cluster: ["30m", "10s"] diff --git a/test/e2e/data/eks/cluster-template-eks-control-plane-only-withaddon.yaml b/test/e2e/data/eks/cluster-template-eks-control-plane-only-withaddon.yaml index b3b216b6c2..68c19be3b7 100644 --- a/test/e2e/data/eks/cluster-template-eks-control-plane-only-withaddon.yaml +++ b/test/e2e/data/eks/cluster-template-eks-control-plane-only-withaddon.yaml @@ -28,3 +28,9 @@ spec: - name: "vpc-cni" version: "${VPC_ADDON_VERSION}" conflictResolution: "overwrite" + - name: "coredns" + version: "${COREDNS_ADDON_VERSION}" + conflictResolution: "overwrite" + identityRef: + kind: AWSClusterStaticIdentity + name: e2e-account diff --git a/test/e2e/suites/managed/addon.go b/test/e2e/suites/managed/addon.go index d4fdabe705..33d5c5f345 100644 --- a/test/e2e/suites/managed/addon.go +++ b/test/e2e/suites/managed/addon.go @@ -75,6 +75,6 @@ func CheckAddonExistsSpec(ctx context.Context, inputGetter func() CheckAddonExis AWSSession: input.AWSSession, AddonName: input.AddonName, AddonVersion: input.AddonVersion, - AddonStatus: eks.AddonStatusActive, + AddonStatus: []string{eks.AddonStatusActive, eks.AddonStatusDegraded}, }, input.E2EConfig.GetIntervals("", "wait-addon-status")...) } diff --git a/test/e2e/suites/managed/addon_helpers.go b/test/e2e/suites/managed/addon_helpers.go index 4492fd8f03..fc46ee89fb 100644 --- a/test/e2e/suites/managed/addon_helpers.go +++ b/test/e2e/suites/managed/addon_helpers.go @@ -35,7 +35,7 @@ type waitForEKSAddonToHaveStatusInput struct { AWSSession client.ConfigProvider AddonName string AddonVersion string - AddonStatus string + AddonStatus []string } func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHaveStatusInput, intervals ...interface{}) { @@ -57,11 +57,15 @@ func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHav return false, err } - if *installedAddon.Status != input.AddonStatus { - return false, nil + for i := range input.AddonStatus { + wantedStatus := input.AddonStatus[i] + + if wantedStatus == *installedAddon.Status { + return true, nil + } } - return true, nil + return false, nil }, intervals...).Should(BeTrue()) } diff --git a/test/e2e/suites/managed/eks_test.go b/test/e2e/suites/managed/eks_test.go index 575b4a19cf..6c03f0c9b3 100644 --- a/test/e2e/suites/managed/eks_test.go +++ b/test/e2e/suites/managed/eks_test.go @@ -35,12 +35,14 @@ import ( // General EKS e2e test var _ = Describe("EKS cluster tests", func() { var ( - namespace *corev1.Namespace - ctx context.Context - specName = "eks-nodes" - clusterName string - cniAddonName = "vpc-cni" - cniAddonVersion = "v1.6.3-eksbuild.1" + namespace *corev1.Namespace + ctx context.Context + specName = "eks-nodes" + clusterName string + cniAddonName = "vpc-cni" + cniAddonVersion = "v1.8.0-eksbuild.1" + corednsAddonName = "coredns" + corednsAddonVersion = "v1.8.3-eksbuild.1" ) shared.ConditionalIt(runGeneralTests, "should create a cluster and add nodes", func() { @@ -85,6 +87,19 @@ var _ = Describe("EKS cluster tests", func() { } }) + By("should have the Coredns addon installed") + CheckAddonExistsSpec(ctx, func() CheckAddonExistsSpecInput { + return CheckAddonExistsSpecInput{ + E2EConfig: e2eCtx.E2EConfig, + BootstrapClusterProxy: e2eCtx.Environment.BootstrapClusterProxy, + AWSSession: e2eCtx.BootstrapUserAWSSession, + Namespace: namespace, + ClusterName: clusterName, + AddonName: corednsAddonName, + AddonVersion: corednsAddonVersion, + } + }) + By("should create a MachineDeployment") MachineDeploymentSpec(ctx, func() MachineDeploymentSpecInput { return MachineDeploymentSpecInput{