Skip to content

Commit

Permalink
fix: support more than 1 eks addon
Browse files Browse the repository at this point in the history 
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 <richard@weave.works>
  • Loading branch information
@richardcase
richardcase committed Nov 16, 2021
1 parent 25399d6 commit 6899c0f
Show file tree
Hide file tree
Showing 10 changed files with 180 additions and 36 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Expand Up @@ -22,6 +22,7 @@ bin
# envfiles
.env
envfile
.envrc

# kubeconfigs
kind.kubeconfig
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloud/services/eks/addons.go
View file
Expand Up @@ -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,
Expand Down
18 changes: 11 additions & 7 deletions pkg/eks/addons/plan.go
View file
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
114 changes: 103 additions & 11 deletions pkg/eks/addons/plan_test.go
View file
Expand Up @@ -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,
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
28 changes: 23 additions & 5 deletions pkg/eks/addons/procedures.go
View file
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
5 changes: 4 additions & 1 deletion test/e2e/data/e2e_eks_conf.yaml
View file
Expand Up @@ -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"]
Expand Down
6 changes: 6 additions & 0 deletions test/e2e/data/eks/cluster-template-eks-control-plane-only-withaddon.yaml
View file
Expand Up @@ -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
2 changes: 1 addition & 1 deletion test/e2e/suites/managed/addon.go
View file
Expand Up @@ -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")...)
}
12 changes: 8 additions & 4 deletions test/e2e/suites/managed/addon_helpers.go
View file
Expand Up @@ -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{}) {
Expand All @@ -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())
}
27 changes: 21 additions & 6 deletions test/e2e/suites/managed/eks_test.go
View file
Expand Up @@ -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() {
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit 6899c0f

Please sign in to comment.