Allow nodegroup creation after a cluster subnet is deleted (eksctl-io#7714)

* Preserve eksctl commands correctness when user deletes subnets

* update error when subnet availability validation fails

* address PR comments

Author: TiberiuGC

pkg/actions/nodegroup/create.go

@@ -135,6 +135,10 @@ func (m *Manager) Create(ctx context.Context, options CreateOpts, nodegroupFilte
 		}
 	}
 
+	if err := validateSubnetsAvailability(cfg); err != nil {
+		return err
+	}
+
 	if err := vpc.ValidateLegacySubnetsForNodeGroups(ctx, cfg, ctl.AWSProvider); err != nil {
 		return err
 	}
@@ -404,3 +408,52 @@ func validateSecurityGroup(ctx context.Context, ec2API awsapi.EC2, securityGroup
 	}
 	return hasDefaultEgressRule, nil
 }
+
+func validateSubnetsAvailability(spec *api.ClusterConfig) error {
+	validateSubnetsAvailabilityForNg := func(np api.NodePool) error {
+		ng := np.BaseNodeGroup()
+		subnetTypeForPrivateNetworking := map[bool]string{
+			true:  "private",
+			false: "public",
+		}
+		unavailableSubnetsErr := func(subnetLocation string) error {
+			return fmt.Errorf("all %[1]s subnets from %[2]s, that the cluster was originally created on, have been deleted; to create %[1]s nodegroups within %[2]s please manually set valid %[1]s subnets via nodeGroup.SubnetIDs",
+				subnetTypeForPrivateNetworking[ng.PrivateNetworking], subnetLocation)
+		}
+
+		// don't check private networking compatibility for:
+		// self-managed nodegroups on local zones
+		if nodeGroup, ok := np.(*api.NodeGroup); (ok && len(nodeGroup.LocalZones) > 0) ||
+			// nodegroups on outposts
+			(ng.OutpostARN != "" || spec.IsControlPlaneOnOutposts()) ||
+			// nodegroups on user specified subnets
+			len(ng.Subnets) > 0 {
+			return nil
+		}
+		shouldCheckAcrossAllAZs := true
+		for _, az := range ng.AvailabilityZones {
+			shouldCheckAcrossAllAZs = false
+			if _, ok := spec.VPC.Subnets.Private[az]; !ok && ng.PrivateNetworking {
+				return unavailableSubnetsErr(az)
+			}
+			if _, ok := spec.VPC.Subnets.Public[az]; !ok && !ng.PrivateNetworking {
+				return unavailableSubnetsErr(az)
+			}
+		}
+		if shouldCheckAcrossAllAZs {
+			if ng.PrivateNetworking && len(spec.VPC.Subnets.Private) == 0 {
+				return unavailableSubnetsErr(spec.VPC.ID)
+			}
+			if !ng.PrivateNetworking && len(spec.VPC.Subnets.Public) == 0 {
+				return unavailableSubnetsErr(spec.VPC.ID)
+			}
+		}
+		return nil
+	}
+	for _, np := range nodes.ToNodePools(spec) {
+		if err := validateSubnetsAvailabilityForNg(np); err != nil {
+			return err
+		}
+	}
+	return nil
+}

pkg/actions/nodegroup/create_test.go

@@ -62,6 +62,11 @@ type expectedCalls struct {
 	clientset            *fake.Clientset
 }
 
+type vpcSubnets struct {
+	publicIDs  []string
+	privateIDs []string
+}
+
 //counterfeiter:generate -o fakes/fake_nodegroup_task_creator.go . nodeGroupTaskCreator
 type nodeGroupTaskCreator interface {
 	NewUnmanagedNodeGroupTask(context.Context, []*api.NodeGroup, bool, bool, bool, vpc.Importer) *tasks.TaskTree
@@ -253,6 +258,51 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 		},
 		expectedErr: errors.Wrap(errors.New("shared node security group missing, to fix this run 'eksctl update cluster --name=my-cluster --region='"), "cluster compatibility check failed")}),
 
+	Entry("fails when nodegroup uses privateNetworking:true and there's no private subnet within vpc", ngEntry{
+		updateClusterConfig: func(c *api.ClusterConfig) {
+			c.NodeGroups[0].PrivateNetworking = true
+		},
+		mockCalls: func(m mockCalls) {
+			mockProviderWithVPCSubnets(m.mockProvider, &vpcSubnets{
+				publicIDs: []string{"subnet-public-1", "subnet-public-2"},
+			})
+		},
+		expectedErr: fmt.Errorf("all private subnets from vpc-1, that the cluster was originally created on, have been deleted; to create private nodegroups within vpc-1 please manually set valid private subnets via nodeGroup.SubnetIDs"),
+	}),
+	Entry("fails when nodegroup uses privateNetworking:false and there's no public subnet within vpc", ngEntry{
+		mockCalls: func(m mockCalls) {
+			mockProviderWithVPCSubnets(m.mockProvider, &vpcSubnets{
+				publicIDs: []string{"subnet-private-1", "subnet-private-2"},
+			})
+		},
+		expectedErr: fmt.Errorf("all public subnets from vpc-1, that the cluster was originally created on, have been deleted; to create public nodegroups within vpc-1 please manually set valid public subnets via nodeGroup.SubnetIDs"),
+	}),
+	Entry("fails when nodegroup uses privateNetworking:true and there's no private subnet within az", ngEntry{
+		updateClusterConfig: func(c *api.ClusterConfig) {
+			c.NodeGroups[0].PrivateNetworking = true
+			c.NodeGroups[0].AvailabilityZones = []string{"us-west-2b"}
+		},
+		mockCalls: func(m mockCalls) {
+			mockProviderWithVPCSubnets(m.mockProvider, &vpcSubnets{
+				publicIDs:  []string{"subnet-public-1", "subnet-public-2"},
+				privateIDs: []string{"subnet-private-1"},
+			})
+		},
+		expectedErr: fmt.Errorf("all private subnets from us-west-2b, that the cluster was originally created on, have been deleted; to create private nodegroups within us-west-2b please manually set valid private subnets via nodeGroup.SubnetIDs"),
+	}),
+	Entry("fails when nodegroup uses privateNetworking:false and there's no private subnet within az", ngEntry{
+		updateClusterConfig: func(c *api.ClusterConfig) {
+			c.NodeGroups[0].AvailabilityZones = []string{"us-west-2a", "us-west-2b"}
+		},
+		mockCalls: func(m mockCalls) {
+			mockProviderWithVPCSubnets(m.mockProvider, &vpcSubnets{
+				publicIDs:  []string{"subnet-public-2"},
+				privateIDs: []string{"subnet-private-1", "subnet-private-2"},
+			})
+		},
+		expectedErr: fmt.Errorf("all public subnets from us-west-2a, that the cluster was originally created on, have been deleted; to create public nodegroups within us-west-2a please manually set valid public subnets via nodeGroup.SubnetIDs"),
+	}),
+
 	Entry("fails when existing local ng stacks in config file is not listed", ngEntry{
 		mockCalls: func(m mockCalls) {
 			m.nodeGroupFilter.SetOnlyLocalReturns(errors.New("err"))
@@ -435,7 +485,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 			m.kubeProvider.NewRawClientReturns(nil, &kubernetes.APIServerUnreachableError{
 				Err: errors.New("timeout"),
 			})
-			mockProviderWithConfig(m.mockProvider, defaultOutput, &ekstypes.VpcConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, &ekstypes.VpcConfigResponse{
 				ClusterSecurityGroupId: aws.String("csg-1234"),
 				EndpointPublicAccess:   false,
 				EndpointPrivateAccess:  true,
@@ -499,7 +549,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 			UpdateAuthConfigMap: api.Disabled(),
 		},
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeApi,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -519,7 +569,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 			UpdateAuthConfigMap: api.Enabled(),
 		},
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeApi,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -536,7 +586,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 
 	Entry("creates nodegroup using access entries when authenticationMode is API_AND_CONFIG_MAP and updateAuthConfigMap is not supplied", ngEntry{
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeApiAndConfigMap,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -553,7 +603,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 
 	Entry("creates nodegroup using aws-auth ConfigMap when authenticationMode is CONFIG_MAP and updateAuthConfigMap is true", ngEntry{
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeConfigMap,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -567,7 +617,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 
 	Entry("creates nodegroup using aws-auth ConfigMap when authenticationMode is CONFIG_MAP and updateAuthConfigMap is not supplied", ngEntry{
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeConfigMap,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -581,7 +631,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 
 	Entry("creates nodegroup but does not use either aws-auth ConfigMap or access entries when authenticationMode is API_AND_CONFIG_MAP and updateAuthConfigMap is false", ngEntry{
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeApiAndConfigMap,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -602,7 +652,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 
 	Entry("creates nodegroup but does not use either aws-auth ConfigMap or access entries when authenticationMode is CONFIG_MAP and updateAuthConfigMap is false", ngEntry{
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeConfigMap,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -623,7 +673,7 @@ var _ = DescribeTable("Create", func(t ngEntry) {
 
 	Entry("authorizes nodegroups using aws-auth ConfigMap when authenticationMode is API_AND_CONFIG_MAP and updateAuthConfigMap is true", ngEntry{
 		mockCalls: func(m mockCalls) {
-			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, &ekstypes.AccessConfigResponse{
+			mockProviderWithConfig(m.mockProvider, defaultOutput, nil, nil, nil, &ekstypes.AccessConfigResponse{
 				AuthenticationMode: ekstypes.AuthenticationModeApiAndConfigMap,
 			})
 			defaultProviderMocks(m.mockProvider, defaultOutput)
@@ -734,6 +784,14 @@ var defaultOutput = []cftypes.Output{
 		OutputKey:   aws.String("SharedNodeSecurityGroup"),
 		OutputValue: aws.String("sg-1"),
 	},
+	{
+		OutputKey:   aws.String("SubnetsPublic"),
+		OutputValue: aws.String("subnet-public-1,subnet-public-2"),
+	},
+	{
+		OutputKey:   aws.String("SubnetsPrivate"),
+		OutputValue: aws.String("subnet-private-1,subnet-private-2"),
+	},
 }
 
 func getIAMIdentities(clientset kubernetes.Interface) []iam.Identity {
@@ -766,14 +824,18 @@ func expectedCallsForAWSAuth(e expectedCalls) {
 }
 
 func defaultProviderMocks(p *mockprovider.MockProvider, output []cftypes.Output) {
-	mockProviderWithConfig(p, output, nil, nil, nil)
+	mockProviderWithConfig(p, output, nil, nil, nil, nil)
 }
 
 func mockProviderWithOutpostConfig(p *mockprovider.MockProvider, describeStacksOutput []cftypes.Output, outpostConfig *ekstypes.OutpostConfigResponse) {
-	mockProviderWithConfig(p, describeStacksOutput, nil, outpostConfig, nil)
+	mockProviderWithConfig(p, describeStacksOutput, nil, nil, outpostConfig, nil)
+}
+
+func mockProviderWithVPCSubnets(p *mockprovider.MockProvider, subnets *vpcSubnets) {
+	mockProviderWithConfig(p, defaultOutput, subnets, nil, nil, nil)
 }
 
-func mockProviderWithConfig(p *mockprovider.MockProvider, describeStacksOutput []cftypes.Output, vpcConfigRes *ekstypes.VpcConfigResponse, outpostConfig *ekstypes.OutpostConfigResponse, accessConfig *ekstypes.AccessConfigResponse) {
+func mockProviderWithConfig(p *mockprovider.MockProvider, describeStacksOutput []cftypes.Output, subnets *vpcSubnets, vpcConfigRes *ekstypes.VpcConfigResponse, outpostConfig *ekstypes.OutpostConfigResponse, accessConfig *ekstypes.AccessConfigResponse) {
 	p.MockCloudFormation().On("ListStacks", mock.Anything, mock.Anything).Return(&cloudformation.ListStacksOutput{
 		StackSummaries: []cftypes.StackSummary{
 			{
@@ -854,6 +916,88 @@ func mockProviderWithConfig(p *mockprovider.MockProvider, describeStacksOutput [
 				},
 			},
 		}, nil)
+
+	if subnets == nil {
+		subnets = &vpcSubnets{
+			publicIDs:  []string{"subnet-public-1", "subnet-public-2"},
+			privateIDs: []string{"subnet-private-1", "subnet-private-2"},
+		}
+	}
+
+	subnetPublic1 := ec2types.Subnet{
+		SubnetId:            aws.String("subnet-public-1"),
+		CidrBlock:           aws.String("192.168.64.0/20"),
+		AvailabilityZone:    aws.String("us-west-2a"),
+		VpcId:               aws.String("vpc-1"),
+		MapPublicIpOnLaunch: aws.Bool(true),
+	}
+	subnetPrivate1 := ec2types.Subnet{
+		SubnetId:            aws.String("subnet-private-1"),
+		CidrBlock:           aws.String("192.168.128.0/20"),
+		AvailabilityZone:    aws.String("us-west-2a"),
+		VpcId:               aws.String("vpc-1"),
+		MapPublicIpOnLaunch: aws.Bool(false),
+	}
+	subnetPublic2 := ec2types.Subnet{
+		SubnetId:            aws.String("subnet-public-2"),
+		CidrBlock:           aws.String("192.168.80.0/20"),
+		AvailabilityZone:    aws.String("us-west-2b"),
+		VpcId:               aws.String("vpc-1"),
+		MapPublicIpOnLaunch: aws.Bool(true),
+	}
+	subnetPrivate2 := ec2types.Subnet{
+		SubnetId:            aws.String("subnet-private-2"),
+		CidrBlock:           aws.String("192.168.32.0/20"),
+		AvailabilityZone:    aws.String("us-west-2b"),
+		VpcId:               aws.String("vpc-1"),
+		MapPublicIpOnLaunch: aws.Bool(false),
+	}
+
+	subnetsForID := map[string]ec2types.Subnet{
+		"subnet-public-1":  subnetPublic1,
+		"subnet-private-1": subnetPrivate1,
+		"subnet-public-2":  subnetPublic2,
+		"subnet-private-2": subnetPrivate2,
+	}
+
+	mockDescribeSubnets := func(mp *mockprovider.MockProvider, vpcID string, subnetIDs []string) {
+		var subnets []ec2types.Subnet
+		for _, id := range subnetIDs {
+			subnets = append(subnets, subnetsForID[id])
+		}
+		if vpcID == "" {
+			mp.MockEC2().On("DescribeSubnets", mock.Anything, &ec2.DescribeSubnetsInput{
+				SubnetIds: subnetIDs,
+			}).Return(&ec2.DescribeSubnetsOutput{Subnets: subnets}, nil)
+			return
+		}
+		mp.MockEC2().On("DescribeSubnets", mock.Anything, &ec2.DescribeSubnetsInput{
+			Filters: []ec2types.Filter{
+				{
+					Name:   aws.String("vpc-id"),
+					Values: []string{vpcID},
+				},
+			},
+		}).Return(&ec2.DescribeSubnetsOutput{Subnets: subnets}, nil)
+	}
+
+	mockDescribeSubnets(p, "", subnets.publicIDs)
+	mockDescribeSubnets(p, "", subnets.privateIDs)
+	mockDescribeSubnets(p, "vpc-1", append(subnets.publicIDs, subnets.privateIDs...))
+
+	p.MockEC2().On("DescribeVpcs", mock.Anything, mock.Anything).Return(&ec2.DescribeVpcsOutput{
+		Vpcs: []ec2types.Vpc{
+			{
+				CidrBlock: aws.String("192.168.0.0/20"),
+				VpcId:     aws.String("vpc-1"),
+				CidrBlockAssociationSet: []ec2types.VpcCidrBlockAssociation{
+					{
+						CidrBlock: aws.String("192.168.0.0/20"),
+					},
+				},
+			},
+		},
+	}, nil)
 }
 
 func mockProviderForUnownedCluster(p *mockprovider.MockProvider, k *eksfakes.FakeKubeProvider, extraSGRules ...ec2types.SecurityGroupRule) {

pkg/cfn/outputs/api.go

@@ -108,10 +108,10 @@ func Exists(stack types.Stack, key string) bool {
 
 // Collect the outputs of a stack using required and optional CollectorSets
 func Collect(stack types.Stack, required, optional map[string]Collector) error {
-	if err := NewCollectorSet(optional).doCollect(false, stack); err != nil {
+	if err := NewCollectorSet(required).doCollect(true, stack); err != nil {
 		return err
 	}
-	return NewCollectorSet(required).doCollect(true, stack)
+	return NewCollectorSet(optional).doCollect(false, stack)
 }
 
 // MustCollect will error if any of the outputs are missing