Skip to content

Commit c600665

Browse files
richardcaserichardcase
committed
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 <[email protected]>
1 parent 5aabdb1 commit c600665

File tree

10 files changed

+178
-39
lines changed

10 files changed

+178
-39
lines changed

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ bin
2222
# envfiles
2323
.env
2424
envfile
25+
.envrc
2526

2627
# kubeconfigs
2728
kind.kubeconfig

‎pkg/cloud/services/eks/addons.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,8 @@ func (s *Service) listAddons(eksClusterName string) ([]*string, error) {
191191
func (s *Service) translateAPIToAddon(addons []ekscontrolplanev1.Addon) []*eksaddons.EKSAddon {
192192
converted := []*eksaddons.EKSAddon{}
193193

194-
for _, addon := range addons {
194+
for i := range addons {
195+
addon := addons[i]
195196
convertedAddon := &eksaddons.EKSAddon{
196197
Name: &addon.Name,
197198
Version: &addon.Version,

‎pkg/eks/addons/plan.go‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,13 @@ func (a *plan) Create(ctx context.Context) ([]planner.Procedure, error) {
4949
procedures := []planner.Procedure{}
5050

5151
// Handle create and update
52-
for _, desired := range a.desiredAddons {
52+
for i := range a.desiredAddons {
53+
desired := a.desiredAddons[i]
5354
installed := a.getInstalled(*desired.Name)
5455
if installed == nil {
5556
// Need to add the addon
5657
procedures = append(procedures, &CreateAddonProcedure{plan: a, name: *desired.Name})
57-
procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name})
58+
procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name, includeDegraded: true})
5859
} else {
5960
// Check if its just the tags that need updating
6061
diffTags := desired.Tags.Difference(installed.Tags)
@@ -64,16 +65,17 @@ func (a *plan) Create(ctx context.Context) ([]planner.Procedure, error) {
6465
// Check if we also need to update the addon
6566
if !desired.IsEqual(installed, false) {
6667
procedures = append(procedures, &UpdateAddonProcedure{plan: a, name: *installed.Name})
67-
procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name})
68+
procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name, includeDegraded: true})
6869
} else if *installed.Status != eks.AddonStatusActive {
6970
// If the desired and installed are the same make sure its active
70-
procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name})
71+
procedures = append(procedures, &WaitAddonActiveProcedure{plan: a, name: *desired.Name, includeDegraded: true})
7172
}
7273
}
7374
}
7475

7576
// look for deletions & unchanged
76-
for _, installed := range a.installedAddons {
77+
for i := range a.installedAddons {
78+
installed := a.installedAddons[i]
7779
desired := a.getDesired(*installed.Name)
7880
if desired == nil {
7981
if *installed.Status != eks.AddonStatusDeleting {
@@ -87,7 +89,8 @@ func (a *plan) Create(ctx context.Context) ([]planner.Procedure, error) {
8789
}
8890

8991
func (a *plan) getInstalled(name string) *EKSAddon {
90-
for _, installed := range a.installedAddons {
92+
for i := range a.installedAddons {
93+
installed := a.installedAddons[i]
9194
if *installed.Name == name {
9295
return installed
9396
}
@@ -97,7 +100,8 @@ func (a *plan) getInstalled(name string) *EKSAddon {
97100
}
98101

99102
func (a *plan) getDesired(name string) *EKSAddon {
100-
for _, desired := range a.desiredAddons {
103+
for i := range a.desiredAddons {
104+
desired := a.desiredAddons[i]
101105
if *desired.Name == name {
102106
return desired
103107
}

‎pkg/eks/addons/plan_test.go‎

Lines changed: 103 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -82,13 +82,85 @@ func TestEKSAddonPlan(t *testing.T) {
8282
},
8383
}, nil)
8484

85-
m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{
85+
out := &eks.DescribeAddonOutput{
86+
Addon: &eks.Addon{
87+
Status: aws.String(eks.AddonStatusActive),
88+
},
89+
}
90+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
8691
AddonName: aws.String(addon1Name),
8792
ClusterName: aws.String(clusterName),
88-
})).Return(nil)
93+
})).Return(out, nil)
94+
},
95+
desiredAddons: []*EKSAddon{
96+
createDesiredAddon(addon1Name, addon1version),
97+
},
98+
expectCreateError: false,
99+
expectDoError: false,
100+
},
101+
{
102+
name: "no installed and 2 desired",
103+
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
104+
m.
105+
CreateAddon(gomock.Eq(&eks.CreateAddonInput{
106+
AddonName: aws.String(addon1Name),
107+
AddonVersion: aws.String(addon1version),
108+
ClusterName: aws.String(clusterName),
109+
ResolveConflicts: aws.String(eks.ResolveConflictsOverwrite),
110+
Tags: convertTags(createTags()),
111+
})).
112+
Return(&eks.CreateAddonOutput{
113+
Addon: &eks.Addon{
114+
AddonArn: aws.String(addonARN),
115+
AddonName: aws.String(addon1Name),
116+
AddonVersion: aws.String(addon1version),
117+
ClusterName: aws.String(clusterName),
118+
CreatedAt: &created,
119+
ModifiedAt: &created,
120+
Status: aws.String(addonStatusCreating),
121+
Tags: convertTags(createTags()),
122+
},
123+
}, nil)
124+
125+
out := &eks.DescribeAddonOutput{
126+
Addon: &eks.Addon{
127+
Status: aws.String(eks.AddonStatusActive),
128+
},
129+
}
130+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
131+
AddonName: aws.String(addon1Name),
132+
ClusterName: aws.String(clusterName),
133+
})).Return(out, nil)
134+
135+
m.
136+
CreateAddon(gomock.Eq(&eks.CreateAddonInput{
137+
AddonName: aws.String("addon2"),
138+
AddonVersion: aws.String(addon1version),
139+
ClusterName: aws.String(clusterName),
140+
ResolveConflicts: aws.String(eks.ResolveConflictsOverwrite),
141+
Tags: convertTags(createTags()),
142+
})).
143+
Return(&eks.CreateAddonOutput{
144+
Addon: &eks.Addon{
145+
AddonArn: aws.String(addonARN),
146+
AddonName: aws.String("addon2"),
147+
AddonVersion: aws.String(addon1version),
148+
ClusterName: aws.String(clusterName),
149+
CreatedAt: &created,
150+
ModifiedAt: &created,
151+
Status: aws.String(addonStatusCreating),
152+
Tags: convertTags(createTags()),
153+
},
154+
}, nil)
155+
156+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
157+
AddonName: aws.String("addon2"),
158+
ClusterName: aws.String(clusterName),
159+
})).Return(out, nil)
89160
},
90161
desiredAddons: []*EKSAddon{
91162
createDesiredAddon(addon1Name, addon1version),
163+
createDesiredAddon("addon2", addon1version),
92164
},
93165
expectCreateError: false,
94166
expectDoError: false,
@@ -110,10 +182,15 @@ func TestEKSAddonPlan(t *testing.T) {
110182
{
111183
name: "1 installed and 1 desired - both same and installed is creating",
112184
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
113-
m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{
185+
out := &eks.DescribeAddonOutput{
186+
Addon: &eks.Addon{
187+
Status: aws.String(eks.AddonStatusActive),
188+
},
189+
}
190+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
114191
AddonName: aws.String(addon1Name),
115192
ClusterName: aws.String(clusterName),
116-
})).Return(nil)
193+
})).Return(out, nil)
117194
},
118195
desiredAddons: []*EKSAddon{
119196
createDesiredAddon(addon1Name, addon1version),
@@ -142,10 +219,16 @@ func TestEKSAddonPlan(t *testing.T) {
142219
Type: aws.String(eks.UpdateTypeVersionUpdate),
143220
},
144221
}, nil)
145-
m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{
222+
223+
out := &eks.DescribeAddonOutput{
224+
Addon: &eks.Addon{
225+
Status: aws.String(eks.AddonStatusActive),
226+
},
227+
}
228+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
146229
AddonName: aws.String(addon1Name),
147230
ClusterName: aws.String(clusterName),
148-
})).Return(nil)
231+
})).Return(out, nil)
149232
},
150233
desiredAddons: []*EKSAddon{
151234
createDesiredAddon(addon1Name, addon1Upgrade),
@@ -159,10 +242,15 @@ func TestEKSAddonPlan(t *testing.T) {
159242
{
160243
name: "1 installed and 1 desired - version upgrade in progress",
161244
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
162-
m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{
245+
out := &eks.DescribeAddonOutput{
246+
Addon: &eks.Addon{
247+
Status: aws.String(eks.AddonStatusActive),
248+
},
249+
}
250+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
163251
AddonName: aws.String(addon1Name),
164252
ClusterName: aws.String(clusterName),
165-
})).Return(nil)
253+
})).Return(out, nil)
166254
},
167255
desiredAddons: []*EKSAddon{
168256
createDesiredAddon(addon1Name, addon1Upgrade),
@@ -217,10 +305,15 @@ func TestEKSAddonPlan(t *testing.T) {
217305
},
218306
}, nil)
219307

220-
m.WaitUntilAddonActive(gomock.Eq(&eks.DescribeAddonInput{
308+
out := &eks.DescribeAddonOutput{
309+
Addon: &eks.Addon{
310+
Status: aws.String(eks.AddonStatusActive),
311+
},
312+
}
313+
m.DescribeAddon(gomock.Eq(&eks.DescribeAddonInput{
221314
AddonName: aws.String(addon1Name),
222315
ClusterName: aws.String(clusterName),
223-
})).Return(nil)
316+
})).Return(out, nil)
224317
},
225318
desiredAddons: []*EKSAddon{
226319
createDesiredAddonExtraTag(addon1Name, addon1Upgrade),
@@ -251,7 +344,6 @@ func TestEKSAddonPlan(t *testing.T) {
251344
Tags: convertTags(createTags()),
252345
},
253346
}, nil)
254-
255347
m.WaitUntilAddonDeleted(gomock.Eq(&eks.DescribeAddonInput{
256348
AddonName: aws.String(addon1Name),
257349
ClusterName: aws.String(clusterName),

‎pkg/eks/addons/procedures.go‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323

2424
"github.com/aws/aws-sdk-go/aws"
2525
"github.com/aws/aws-sdk-go/service/eks"
26+
"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/wait"
2627
)
2728

2829
var (
@@ -168,10 +169,12 @@ func (p *CreateAddonProcedure) Name() string {
168169
}
169170

170171
// WaitAddonActiveProcedure is a procedure that will wait for an EKS addon
171-
// to be active in a cluster.
172+
// to be active in a cluster. Abd optionally include the degraded state.
173+
// Note: addons may be degraded until there are worker nodes.
172174
type WaitAddonActiveProcedure struct {
173-
plan *plan
174-
name string
175+
plan *plan
176+
name string
177+
includeDegraded bool
175178
}
176179

177180
// Do implements the logic for the procedure.
@@ -181,8 +184,23 @@ func (p *WaitAddonActiveProcedure) Do(ctx context.Context) error {
181184
ClusterName: aws.String(p.plan.clusterName),
182185
}
183186

184-
if err := p.plan.eksClient.WaitUntilAddonActive(input); err != nil {
185-
return fmt.Errorf("waiting for addon %s to be active: %w", p.name, err)
187+
if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
188+
out, describeErr := p.plan.eksClient.DescribeAddon(input)
189+
if describeErr != nil {
190+
return false, describeErr
191+
}
192+
193+
if *out.Addon.Status == eks.AddonStatusActive {
194+
return true, nil
195+
}
196+
197+
if p.includeDegraded && *out.Addon.Status == eks.AddonStatusDegraded {
198+
return true, nil
199+
}
200+
201+
return false, nil
202+
}); err != nil {
203+
return fmt.Errorf("failed waiting for addon %s to be ready: %w", p.name, err)
186204
}
187205

188206
return nil

‎test/e2e/data/e2e_eks_conf.yaml‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,16 +87,17 @@ providers:
8787

8888

8989
variables:
90-
KUBERNETES_VERSION: "v1.19.8"
90+
KUBERNETES_VERSION: "1.21.2"
9191
CNI: "../../data/cni/calico_eks.yaml"
9292
EXP_MACHINE_POOL: "true"
9393
EXP_CLUSTER_RESOURCE_SET: "true"
9494
AWS_NODE_MACHINE_TYPE: t3.large
9595
AWS_SSH_KEY_NAME: "cluster-api-provider-aws-sigs-k8s-io"
9696
EXP_EKS_IAM: "false"
9797
EXP_EKS_ADD_ROLES: "false"
98-
VPC_ADDON_VERSION: "v1.6.3-eksbuild.1"
99-
CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "v1.19.8"
98+
VPC_ADDON_VERSION: "v1.8.0-eksbuild.1"
99+
COREDNS_ADDON_VERSION: "v1.8.3-eksbuild.1"
100+
CONFORMANCE_CI_ARTIFACTS_KUBERNETES_VERSION: "1.21.2"
100101
AUTO_CONTROLLER_IDENTITY_CREATOR: "false"
101102

102103
intervals:

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ spec:
2828
- name: "vpc-cni"
2929
version: "${VPC_ADDON_VERSION}"
3030
conflictResolution: "overwrite"
31+
- name: "coredns"
32+
version: "${COREDNS_ADDON_VERSION}"
33+
conflictResolution: "overwrite"
3134
identityRef:
3235
kind: AWSClusterStaticIdentity
3336
name: e2e-account

‎test/e2e/suites/managed/addon.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,6 @@ func CheckAddonExistsSpec(ctx context.Context, inputGetter func() CheckAddonExis
7575
AWSSession: input.AWSSession,
7676
AddonName: input.AddonName,
7777
AddonVersion: input.AddonVersion,
78-
AddonStatus: eks.AddonStatusActive,
78+
AddonStatus: []string{eks.AddonStatusActive, eks.AddonStatusDegraded},
7979
}, input.E2EConfig.GetIntervals("", "wait-addon-status")...)
8080
}

‎test/e2e/suites/managed/addon_helpers.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ type waitForEKSAddonToHaveStatusInput struct {
3535
AWSSession client.ConfigProvider
3636
AddonName string
3737
AddonVersion string
38-
AddonStatus string
38+
AddonStatus []string
3939
}
4040

4141
func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHaveStatusInput, intervals ...interface{}) {
@@ -45,7 +45,7 @@ func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHav
4545
Expect(input.AddonVersion).ShouldNot(HaveLen(0), "Invalid argument. input.AddonVersion can't be empty")
4646
Expect(input.AddonStatus).ShouldNot(HaveLen(0), "Invalid argument. input.AddonStatus can't be empty")
4747

48-
ginkgo.By(fmt.Sprintf("Ensuring EKS addon %s has status %s for EKS cluster %s", input.AddonName, input.AddonStatus, input.ControlPlane.Spec.EKSClusterName))
48+
ginkgo.By(fmt.Sprintf("Ensuring EKS addon %s has status in %q for EKS cluster %s", input.AddonName, input.AddonStatus, input.ControlPlane.Spec.EKSClusterName))
4949

5050
Eventually(func() (bool, error) {
5151
installedAddon, err := getEKSClusterAddon(input.ControlPlane.Spec.EKSClusterName, input.AddonName, input.AWSSession)
@@ -57,11 +57,15 @@ func waitForEKSAddonToHaveStatus(ctx context.Context, input waitForEKSAddonToHav
5757
return false, err
5858
}
5959

60-
if *installedAddon.Status != input.AddonStatus {
61-
return false, nil
60+
for i := range input.AddonStatus {
61+
wantedStatus := input.AddonStatus[i]
62+
63+
if wantedStatus == *installedAddon.Status {
64+
return true, nil
65+
}
6266
}
6367

64-
return true, nil
68+
return false, nil
6569

6670
}, intervals...).Should(BeTrue())
6771
}

0 commit comments

Comments
 (0)