Merge pull request #2914 from richardcase/eks_addons_bug

fix: multiple addons not being reconciled

.gitignore

@@ -22,6 +22,7 @@ bin
 # envfiles
 .env
 envfile
+.envrc
 
 # kubeconfigs
 kind.kubeconfig

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,

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
 		}

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

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 (
@@ -168,10 +169,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.
@@ -181,8 +184,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

test/e2e/data/e2e_eks_conf.yaml

@@ -172,16 +172,17 @@ providers:
         targetName: "cluster-template-eks-managedmachinepool.yaml"
 
 variables:
-  KUBERNETES_VERSION: "v1.19.8"
+  KUBERNETES_VERSION: "1.21.2"
   CNI: "../../data/cni/calico_eks.yaml"
   EXP_MACHINE_POOL: "true"
   EXP_CLUSTER_RESOURCE_SET: "true"
   AWS_NODE_MACHINE_TYPE: t3.large
   AWS_SSH_KEY_NAME: "cluster-api-provider-aws-sigs-k8s-io"
   EXP_EKS_IAM: "false"
   EXP_EKS_ADD_ROLES: "false"
-  VPC_ADDON_VERSION: "v1.6.3-eksbuild.1"
-  CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "v1.19.8"
+  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:

test/e2e/data/eks/cluster-template-eks-control-plane-only-withaddon.yaml

@@ -28,6 +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

test/e2e/suites/managed/addon.go

@@ -1,3 +1,4 @@
+//go:build e2e
 // +build e2e
 
 /*
@@ -75,6 +76,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")...)
 }

test/e2e/suites/managed/addon_helpers.go

@@ -1,3 +1,4 @@
+//go:build e2e
 // +build e2e
 
 /*
@@ -35,7 +36,7 @@ type waitForEKSAddonToHaveStatusInput struct {
 	AWSSession   client.ConfigProvider
 	AddonName    string
 	AddonVersion string
-	AddonStatus  string
+	AddonStatus  []string
 }
 
 func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHaveStatusInput, intervals ...interface{}) {
@@ -45,7 +46,7 @@ func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHav
 	Expect(input.AddonVersion).ShouldNot(HaveLen(0), "Invalid argument. input.AddonVersion can't be empty")
 	Expect(input.AddonStatus).ShouldNot(HaveLen(0), "Invalid argument. input.AddonStatus can't be empty")
 
-	ginkgo.By(fmt.Sprintf("Ensuring EKS addon %s has status %s for EKS cluster %s", input.AddonName, input.AddonStatus, input.ControlPlane.Spec.EKSClusterName))
+	ginkgo.By(fmt.Sprintf("Ensuring EKS addon %s has status in %q for EKS cluster %s", input.AddonName, input.AddonStatus, input.ControlPlane.Spec.EKSClusterName))
 
 	Eventually(func() (bool, error) {
 		installedAddon, err := getEKSClusterAddon(input.ControlPlane.Spec.EKSClusterName, input.AddonName, input.AWSSession)
@@ -57,11 +58,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())
 }