Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow nodegroup creation after a cluster subnet is deleted #7714

Merged
merged 3 commits into from
Apr 24, 2024
Merged

Allow nodegroup creation after a cluster subnet is deleted #7714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2ea191
Preserve eksctl commands correctness when user deletes subnets
TiberiuGC Apr 18, 2024
bcd0222
update error when subnet availability validation fails
TiberiuGC Apr 22, 2024
7d69372
address PR comments
TiberiuGC Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions pkg/actions/nodegroup/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
168 changes: 156 additions & 12 deletions pkg/actions/nodegroup/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"))
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
{
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/cfn/outputs/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading