diff --git a/pkg/cfn/manager/api.go b/pkg/cfn/manager/api.go index f00b132ff0..570db1959f 100644 --- a/pkg/cfn/manager/api.go +++ b/pkg/cfn/manager/api.go @@ -544,11 +544,34 @@ func nonTransitionalReadyStackStatuses() []types.StackStatus { return []types.StackStatus{ types.StackStatusCreateComplete, types.StackStatusUpdateComplete, - types.StackStatusRollbackComplete, + // Note: ROLLBACK_COMPLETE is deliberately excluded. It means the stack's + // CREATE failed and was rolled back, so no resources exist and CloudFormation + // refuses to update it; grouping it with healthy terminal states caused + // broken stacks to be treated as healthy existing nodegroups (see + // https://github.com/eksctl-io/eksctl/issues/8712). UPDATE_ROLLBACK_COMPLETE + // is genuinely healthy (an update rolled back to a known-good state) and + // remains included. types.StackStatusUpdateRollbackComplete, } } +// StackStatusIsNotOperational reports whether the stack is in a terminal failed or +// rolled-back state (e.g. ROLLBACK_COMPLETE) in which it cannot serve the resources +// it represents. Such stacks must not be treated as healthy existing nodegroups: a +// nodegroup whose stack is in ROLLBACK_COMPLETE was never created successfully, and +// CloudFormation refuses to update it, so it has to be deleted and recreated. +func StackStatusIsNotOperational(s *Stack) bool { + switch s.StackStatus { + case types.StackStatusCreateFailed, + types.StackStatusRollbackComplete, + types.StackStatusRollbackFailed, + types.StackStatusUpdateRollbackFailed, + types.StackStatusDeleteFailed: + return true + } + return false +} + func allNonDeletedStackStatuses() []types.StackStatus { return []types.StackStatus{ types.StackStatusCreateInProgress, diff --git a/pkg/cfn/manager/api_test.go b/pkg/cfn/manager/api_test.go index d6e3b6d574..005a67dcff 100644 --- a/pkg/cfn/manager/api_test.go +++ b/pkg/cfn/manager/api_test.go @@ -22,6 +22,32 @@ import ( ) var _ = Describe("StackCollection", func() { + Context("StackStatusIsNotOperational", func() { + It("returns true for failed and rolled-back states", func() { + for _, status := range []types.StackStatus{ + types.StackStatusCreateFailed, + types.StackStatusRollbackComplete, + types.StackStatusRollbackFailed, + types.StackStatusUpdateRollbackFailed, + types.StackStatusDeleteFailed, + } { + Expect(StackStatusIsNotOperational(&Stack{StackStatus: status})).To(BeTrue(), "expected %q to be not operational", status) + } + }) + + It("returns false for healthy and transitional states", func() { + for _, status := range []types.StackStatus{ + types.StackStatusCreateComplete, + types.StackStatusUpdateComplete, + types.StackStatusUpdateRollbackComplete, + types.StackStatusCreateInProgress, + types.StackStatusRollbackInProgress, + } { + Expect(StackStatusIsNotOperational(&Stack{StackStatus: status})).To(BeFalse(), "expected %q to be operational", status) + } + }) + }) + Context("PropagateManagedNodeGroupTagsToASG", func() { var ( asgName string diff --git a/pkg/ctl/cmdutils/filter/nodegroup_filter.go b/pkg/ctl/cmdutils/filter/nodegroup_filter.go index 94e4ef39fc..1aa8c0aa7d 100644 --- a/pkg/ctl/cmdutils/filter/nodegroup_filter.go +++ b/pkg/ctl/cmdutils/filter/nodegroup_filter.go @@ -2,6 +2,7 @@ package filter import ( "context" + "fmt" "strings" "github.com/aws/aws-sdk-go-v2/service/eks" @@ -35,6 +36,7 @@ type NodeGroupFilter struct { onlyRemote bool localNodegroups sets.Set[string] remoteNodegroups sets.Set[string] + nodeGroupStacks []manager.NodeGroupStack } // NewNodeGroupFilter creates a new NodeGroupFilter struct @@ -80,11 +82,33 @@ func (f *NodeGroupFilter) AppendIncludeNames(names ...string) { func (f *NodeGroupFilter) SetOnlyLocal(ctx context.Context, eksAPI awsapi.EKS, lister StackLister, clusterConfig *api.ClusterConfig) error { f.onlyLocal = true + // Capture the nodegroups the user actually asked for *before* loading, since + // loadLocalAndRemoteNodegroups appends remote-only nodegroups to the config. + configNodeGroupNames := sets.New(clusterConfig.GetAllNodeGroupNames()...) + err := f.loadLocalAndRemoteNodegroups(ctx, eksAPI, lister, clusterConfig) if err != nil { return err } + // Stacks stuck in a failed or rolled-back state (e.g. ROLLBACK_COMPLETE) cannot + // be used and would otherwise be silently treated as healthy existing nodegroups, + // causing `create nodegroup` to do nothing. Fail fast when a nodegroup in the + // user's config already has such a stack. + var notOperationalNodeGroups []string + for _, s := range f.nodeGroupStacks { + if configNodeGroupNames.Has(s.NodeGroupName) && s.Stack != nil && manager.StackStatusIsNotOperational(s.Stack) { + notOperationalNodeGroups = append(notOperationalNodeGroups, s.NodeGroupName) + } + } + if len(notOperationalNodeGroups) > 0 { + return fmt.Errorf( + "nodegroup(s) %q have a CloudFormation stack in a failed or rolled-back state (e.g. ROLLBACK_COMPLETE) and cannot be used; "+ + "delete the failed stack(s) first with 'eksctl delete nodegroup --cluster %s --name %s' and then retry", + strings.Join(notOperationalNodeGroups, ", "), clusterConfig.Metadata.Name, notOperationalNodeGroups[0], + ) + } + // Remote ones will be excluded if f.remoteNodegroups.Len() > 0 { logger.Info("%d existing %s(s) (%s) will be excluded", f.remoteNodegroups.Len(), "nodegroup", strings.Join(sets.List(f.remoteNodegroups), ",")) @@ -126,6 +150,7 @@ func (f *NodeGroupFilter) loadLocalAndRemoteNodegroups(ctx context.Context, eksA if err != nil { return err } + f.nodeGroupStacks = nodeGroupsWithStacks for _, s := range nodeGroupsWithStacks { f.remoteNodegroups.Insert(s.NodeGroupName) } diff --git a/pkg/ctl/cmdutils/filter/nodegroup_filter_test.go b/pkg/ctl/cmdutils/filter/nodegroup_filter_test.go index b7e99a7997..7cd5b66356 100644 --- a/pkg/ctl/cmdutils/filter/nodegroup_filter_test.go +++ b/pkg/ctl/cmdutils/filter/nodegroup_filter_test.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/cloudformation/types" "github.com/aws/aws-sdk-go-v2/service/eks" "github.com/stretchr/testify/mock" @@ -117,6 +118,60 @@ var _ = Describe("nodegroup filter", func() { Expect(excluded.HasAll("test-ng1a", "test-ng2a", "test-ng3a", "test-ng2b", "test-ng3b")).To(BeTrue()) }) + It("should error when a nodegroup stack is in a failed or rolled-back state", func() { + mockLister := &mockStackLister{ + nodesResult: []manager.NodeGroupStack{ + { + NodeGroupName: "test-ng1a", + Stack: &manager.Stack{ + StackStatus: types.StackStatusRollbackComplete, + }, + }, + { + NodeGroupName: "test-ng2a", + Stack: &manager.Stack{ + StackStatus: types.StackStatusCreateComplete, + }, + }, + }, + } + err := filter.SetOnlyLocal(context.Background(), mockProvider.EKS(), mockLister, cfg) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("test-ng1a")) + Expect(err.Error()).To(ContainSubstring("failed or rolled-back")) + Expect(err.Error()).To(ContainSubstring("delete nodegroup")) + }) + + It("should allow healthy nodegroup stacks", func() { + mockLister := &mockStackLister{ + nodesResult: []manager.NodeGroupStack{ + { + NodeGroupName: "test-ng1a", + Stack: &manager.Stack{ + StackStatus: types.StackStatusUpdateRollbackComplete, + }, + }, + }, + } + err := filter.SetOnlyLocal(context.Background(), mockProvider.EKS(), mockLister, cfg) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should ignore failed or rolled-back stacks for nodegroups not in the config", func() { + mockLister := &mockStackLister{ + nodesResult: []manager.NodeGroupStack{ + { + NodeGroupName: "remote-ng-not-in-config", + Stack: &manager.Stack{ + StackStatus: types.StackStatusRollbackComplete, + }, + }, + }, + } + err := filter.SetOnlyLocal(context.Background(), mockProvider.EKS(), mockLister, cfg) + Expect(err).NotTo(HaveOccurred()) + }) + It("should match only local nodegroups with exclude and include rules", func() { err := filter.AppendIncludeGlobs(getNodeGroupNames(cfg), "test-ng?a", "test-ng?b") Expect(err).NotTo(HaveOccurred()) diff --git a/pkg/eks/nodegroup_service.go b/pkg/eks/nodegroup_service.go index 3a8d5e8572..fd6e8c543d 100644 --- a/pkg/eks/nodegroup_service.go +++ b/pkg/eks/nodegroup_service.go @@ -290,7 +290,12 @@ func ValidateExistingNodeGroupsForCompatibility(ctx context.Context, cfg *api.Cl logger.Info("checking security group configuration for all nodegroups") var incompatibleNodeGroups []string + var notOperationalNodeGroups []string for ng, info := range infoByNodeGroup { + if manager.StackStatusIsNotOperational(info.Stack) { + notOperationalNodeGroups = append(notOperationalNodeGroups, ng) + continue + } if stackManager.StackStatusIsNotTransitional(info.Stack) { isCompatible, err := isNodeGroupCompatible(ng, info) if err != nil { @@ -305,8 +310,15 @@ func ValidateExistingNodeGroupsForCompatibility(ctx context.Context, cfg *api.Cl } } + if len(notOperationalNodeGroups) > 0 { + logger.Critical("found nodegroup(s) (%s) whose CloudFormation stack is in a failed or rolled-back state (e.g. ROLLBACK_COMPLETE) and cannot be used", + strings.Join(notOperationalNodeGroups, ", ")) + logger.Critical("these nodegroups were never created successfully; delete and recreate them, e.g. 'eksctl delete nodegroup --cluster %s --name <%s>'", + cfg.Metadata.Name, notOperationalNodeGroups[0]) + } + if len(incompatibleNodeGroups) == 0 { - logger.Info("all nodegroups have up-to-date cloudformation templates") + logger.Info("all nodegroups have compatible shared security group configuration") return nil }