diff --git a/pkg/actions/nodegroup/create.go b/pkg/actions/nodegroup/create.go index b587152082..0134da7ed2 100644 --- a/pkg/actions/nodegroup/create.go +++ b/pkg/actions/nodegroup/create.go @@ -136,6 +136,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 } @@ -423,3 +427,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 +} diff --git a/pkg/actions/nodegroup/create_test.go b/pkg/actions/nodegroup/create_test.go index 19765ae44c..f734b0738a 100644 --- a/pkg/actions/nodegroup/create_test.go +++ b/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) { diff --git a/pkg/cfn/outputs/api.go b/pkg/cfn/outputs/api.go index f2c44ed9a7..a156f8da0f 100644 --- a/pkg/cfn/outputs/api.go +++ b/pkg/cfn/outputs/api.go @@ -112,10 +112,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 diff --git a/pkg/vpc/vpc.go b/pkg/vpc/vpc.go index 5c015cac3b..11218be9f3 100644 --- a/pkg/vpc/vpc.go +++ b/pkg/vpc/vpc.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "fmt" "net" + "slices" "strings" "github.com/aws/aws-sdk-go-v2/aws" @@ -300,7 +301,47 @@ func UseFromClusterStack(ctx context.Context, provider api.ClusterProvider, stac return strings.Split(v, ",") } importSubnetsFromIDList := func(subnetMapping api.AZSubnetMapping, value string) error { - return ImportSubnetsFromIDList(ctx, provider.EC2(), spec, subnetMapping, splitOutputValue(value)) + var ( + vpcSubnets []string + stackSubnets []string + toBeImported []string + ) + // collect VPC subnets as returned by CFN stack outputs + stackSubnets = splitOutputValue(value) + + // collect VPC subnets as returned by EC2 API + ec2API := provider.EC2() + output, err := ec2API.DescribeSubnets(ctx, &ec2.DescribeSubnetsInput{ + Filters: []ec2types.Filter{ + { + Name: aws.String("vpc-id"), + Values: []string{spec.VPC.ID}, + }, + }, + }) + if err != nil { + return err + } + for _, o := range output.Subnets { + vpcSubnets = append(vpcSubnets, *o.SubnetId) + } + + // if a subnet is present on the stack outputs, but actually missing from VPC + // e.g. it was manually deleted by the user using AWS CLI/Console + // than log a warning and don't import it into cluster spec + stackDriftFound := false + for _, ssID := range stackSubnets { + if !slices.Contains(vpcSubnets, ssID) { + stackDriftFound = true + logger.Warning("%s was found on cluster cloudformation stack outputs, but has been removed from VPC %s outside of eksctl", ssID, spec.VPC.ID) + continue + } + toBeImported = append(toBeImported, ssID) + } + if stackDriftFound { + logger.Warning("VPC %s contains the following subnets: %s", spec.VPC.ID, strings.Join(vpcSubnets, ",")) + } + return ImportSubnetsFromIDList(ctx, provider.EC2(), spec, subnetMapping, toBeImported) } optionalCollectors := map[string]outputs.Collector{ diff --git a/pkg/vpc/vpc_test.go b/pkg/vpc/vpc_test.go index 63970f803b..e4090852e4 100644 --- a/pkg/vpc/vpc_test.go +++ b/pkg/vpc/vpc_test.go @@ -500,6 +500,15 @@ var _ = Describe("VPC", func() { mockEC2: func(ec2Mock *mocksv2.EC2) { ec2Mock.On("DescribeSubnets", Anything, Anything).Return(func(_ context.Context, input *ec2.DescribeSubnetsInput, _ ...func(options *ec2.Options)) *ec2.DescribeSubnetsOutput { + if len(input.Filters) > 0 { + return &ec2.DescribeSubnetsOutput{ + Subnets: []ec2types.Subnet{ + { + SubnetId: aws.String("subnet-1"), + }, + }, + } + } return &ec2.DescribeSubnetsOutput{ Subnets: []ec2types.Subnet{ { @@ -582,6 +591,18 @@ var _ = Describe("VPC", func() { mockEC2: func(ec2Mock *mocksv2.EC2) { ec2Mock.On("DescribeSubnets", Anything, Anything).Return(func(_ context.Context, input *ec2.DescribeSubnetsInput, _ ...func(options *ec2.Options)) *ec2.DescribeSubnetsOutput { + if len(input.Filters) > 0 { + return &ec2.DescribeSubnetsOutput{ + Subnets: []ec2types.Subnet{ + { + SubnetId: aws.String("subnet-1"), + }, + { + SubnetId: aws.String("subnet-lz1"), + }, + }, + } + } subnet := ec2types.Subnet{ SubnetId: aws.String(input.SubnetIds[0]), VpcId: aws.String("vpc-123"), @@ -653,6 +674,18 @@ var _ = Describe("VPC", func() { }, mockEC2: func(ec2Mock *mocksv2.EC2) { ec2Mock.On("DescribeSubnets", Anything, Anything).Return(func(_ context.Context, input *ec2.DescribeSubnetsInput, _ ...func(options *ec2.Options)) *ec2.DescribeSubnetsOutput { + if len(input.Filters) > 0 { + return &ec2.DescribeSubnetsOutput{ + Subnets: []ec2types.Subnet{ + { + SubnetId: aws.String("subnet-public-1"), + }, + { + SubnetId: aws.String("subnet-private-1"), + }, + }, + } + } subnet := ec2types.Subnet{ SubnetId: aws.String(input.SubnetIds[0]), VpcId: aws.String("vpc-123"),