Skip to content

Commit

Permalink
Cleanup subnets with invalid AZs before importing VPC from CFN stack (e…
Browse files Browse the repository at this point in the history 
…ksctl-io#6935)

* Cleanup subnets with invalid AZs when importing cluster VPC from stack

* add integration test

* remove unnecessary log line
  • Loading branch information
@TiberiuGC
TiberiuGC authored Aug 30, 2023
1 parent 1d2a3cd commit 1807c5e
Show file tree
Hide file tree
Showing 3 changed files with 265 additions and 12 deletions.
63 changes: 57 additions & 6 deletions integration/tests/crud/creategetdelete_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,19 +285,32 @@ var _ = Describe("(Integration) Create, Get, Scale & Delete", func() {

Context("can add a nodegroup into a new subnet", func() {
var (
subnet *types.Subnet
nodegroupName string
subnet *types.Subnet
nodegroupNameCLI string
nodegroupNameConfigFile string
subnetName string
)
BeforeEach(func() {
nodegroupName = "test-extra-nodegroup"
nodegroupNameCLI = "test-extra-nodegroup-cli"
nodegroupNameConfigFile = "text-extra-nodegroup-config-file"
subnetName = "new-subnet"
})
AfterEach(func() {
cmd := params.EksctlDeleteCmd.WithArgs(
"nodegroup",
"--verbose", "4",
"--cluster", params.ClusterName,
"--wait",
nodegroupName,
nodegroupNameCLI,
)
Expect(cmd).To(RunSuccessfully())

cmd = params.EksctlDeleteCmd.WithArgs(
"nodegroup",
"--verbose", "4",
"--cluster", params.ClusterName,
"--wait",
nodegroupNameConfigFile,
)
Expect(cmd).To(RunSuccessfully())
config := NewConfig(params.Region)
Expand Down Expand Up @@ -343,6 +356,7 @@ var _ = Describe("(Integration) Create, Get, Scale & Delete", func() {
tags = append(tags, t)
}
}
// create a new subnet in that given vpc and zone.
output, err := ec2.CreateSubnet(context.Background(), &awsec2.CreateSubnetInput{
AvailabilityZone: aws.String("us-west-2a"),
CidrBlock: aws.String(cidr),
Expand Down Expand Up @@ -380,17 +394,54 @@ var _ = Describe("(Integration) Create, Get, Scale & Delete", func() {
})
Expect(err).NotTo(HaveOccurred(), routput)

// create a new subnet in that given vpc and zone.
// create a nodegroup into the new subnet via CLI
cmd := params.EksctlCreateCmd.WithArgs(
"nodegroup",
"--timeout", nodegroupTimeout,
"--cluster", params.ClusterName,
"--nodes", "1",
"--node-type", "p2.xlarge",
"--subnet-ids", *subnet.SubnetId,
nodegroupName,
nodegroupNameCLI,
)
Expect(cmd).To(RunSuccessfully())

// create a nodegroup into the new subnet via config file
clusterConfig := makeClusterConfig()
clusterConfig.VPC = &api.ClusterVPC{
Network: api.Network{
ID: *s.VpcId,
},
Subnets: &api.ClusterSubnets{
Public: api.AZSubnetMapping{
subnetName: api.AZSubnetSpec{
ID: *subnet.SubnetId,
},
},
},
}
clusterConfig.NodeGroups = []*api.NodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Name: nodegroupNameConfigFile,
ScalingConfig: &api.ScalingConfig{
DesiredCapacity: aws.Int(1),
},
Subnets: []string{subnetName},
},
},
}

cmd = params.EksctlCreateCmd.
WithArgs(
"nodegroup",
"--config-file", "-",
"--verbose", "4",
"--timeout", nodegroupTimeout,
).
WithoutArg("--region", params.Region).
WithStdin(clusterutils.Reader(clusterConfig))
Expect(cmd).To(RunSuccessfully())
})
})

Expand Down
15 changes: 14 additions & 1 deletion pkg/vpc/vpc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,12 @@ func UseFromClusterStack(ctx context.Context, provider api.ClusterProvider, stac
}
}

return outputs.Collect(*stack, requiredCollectors, optionalCollectors)
if err := outputs.Collect(*stack, requiredCollectors, optionalCollectors); err != nil {
return err
}
// to clean up invalid subnets based on AZ after importing valid subnets from stack
cleanupSubnets(spec)
return nil
}

// MakeExtendedSubnetAliasFunc returns a function for creating an alias for a subnet that was added as part of extending
Expand Down Expand Up @@ -647,6 +652,14 @@ func cleanupSubnets(spec *api.ClusterConfig) {
cleanup := func(subnets *api.AZSubnetMapping) {
for name, subnet := range *subnets {
if _, ok := availabilityZones[subnet.AZ]; !ok {
// since we're removing the subnet with invalid AZ from spec, we want to reference it by ID in any subsequent nodegroup creation task
for _, node := range nodes.ToNodePools(spec) {
for i, subnetRef := range node.BaseNodeGroup().Subnets {
if subnetRef == name {
node.BaseNodeGroup().Subnets[i] = subnet.ID
}
}
}
delete(*subnets, name)
}
}
Expand Down
199 changes: 194 additions & 5 deletions pkg/vpc/vpc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,15 @@ func newFakeClusterWithEndpoints(private, public bool, name string) *ekstypes.Cl
return cluster
}

func clusterConfigWithSubnets(publicSubnets, privateSubnets api.AZSubnetMapping) *api.ClusterConfig {
cfg := api.NewClusterConfig()
cfg.VPC.Subnets = &api.ClusterSubnets{
Private: privateSubnets,
Public: publicSubnets,
}
return cfg
}

var _ = Describe("VPC", func() {
Describe("SplitInto16", func() {
It("splits the block into 16", func() {
Expand Down Expand Up @@ -597,6 +606,113 @@ var _ = Describe("VPC", func() {
}, nil)
},
}),

Entry("cluster spec provided - subnets with invalid AZs", useFromClusterCase{
cfg: clusterConfigWithSubnets(
api.AZSubnetMapping{
"public-1": {
ID: "subnet-public-1",
},
// public-2 will not be on the stack,
// hence fetching config from stack
// won't resolve the AZ for this subnet
"public-2": {
ID: "subnet-public-2",
},
},
api.AZSubnetMapping{
"private-1": {
ID: "subnet-private-1",
},
// private-2 will not be on the stack,
// hence fetching config from stack
// won't resolve the AZ for this subnet
"private-2": {
ID: "subnet-private-2",
},
}),
stack: &cfntypes.Stack{
Outputs: []cfntypes.Output{
{
OutputKey: aws.String("VPC"),
OutputValue: aws.String("vpc-123"),
},
{
OutputKey: aws.String("SecurityGroup"),
OutputValue: aws.String("sg-123"),
},
{
OutputKey: aws.String("SubnetsPublic"),
OutputValue: aws.String("subnet-public-1"),
},
{
OutputKey: aws.String("SubnetsPrivate"),
OutputValue: aws.String("subnet-private-1"),
},
},
},
mockEC2: func(ec2Mock *mocksv2.EC2) {
ec2Mock.On("DescribeSubnets", Anything, Anything).Return(func(_ context.Context, input *ec2.DescribeSubnetsInput, _ ...func(options *ec2.Options)) *ec2.DescribeSubnetsOutput {
subnet := ec2types.Subnet{
SubnetId: aws.String(input.SubnetIds[0]),
VpcId: aws.String("vpc-123"),
}
if input.SubnetIds[0] == "subnet-public-1" {
subnet.AvailabilityZone = aws.String("us-west-2a")
subnet.CidrBlock = aws.String("192.168.0.0/20")
} else {
subnet.AvailabilityZone = aws.String("us-west-2a")
subnet.CidrBlock = aws.String("192.168.0.16/20")
}
return &ec2.DescribeSubnetsOutput{
Subnets: []ec2types.Subnet{subnet},
}
}, nil).On("DescribeVpcs", Anything, Anything).Return(&ec2.DescribeVpcsOutput{
Vpcs: []ec2types.Vpc{
{
VpcId: aws.String("vpc-123"),
CidrBlock: aws.String("192.168.0.0/20"),
},
},
}, nil)
},
expectedVPC: &api.ClusterVPC{
Network: api.Network{
ID: "vpc-123",
CIDR: ipnet.MustParseCIDR("192.168.0.0/20"),
},
SecurityGroup: "sg-123",
Subnets: &api.ClusterSubnets{
Public: api.AZSubnetMapping{
"public-1": api.AZSubnetSpec{
ID: "subnet-public-1",
AZ: "us-west-2a",
CIDR: ipnet.MustParseCIDR("192.168.0.0/20"),
},
},
Private: api.AZSubnetMapping{
"private-1": api.AZSubnetSpec{
ID: "subnet-private-1",
AZ: "us-west-2a",
CIDR: ipnet.MustParseCIDR("192.168.0.16/20"),
},
},
},
LocalZoneSubnets: &api.ClusterSubnets{
Private: api.NewAZSubnetMapping(),
Public: api.NewAZSubnetMapping(),
},
ManageSharedNodeSecurityGroupRules: aws.Bool(true),
AutoAllocateIPv6: aws.Bool(false),
NAT: &api.ClusterNAT{
Gateway: aws.String("Single"),
},
ClusterEndpoints: &api.ClusterEndpoints{
PublicAccess: aws.Bool(true),
PrivateAccess: aws.Bool(true),
},
},
}),
)

DescribeTable("importVPC",
Expand Down Expand Up @@ -875,10 +991,54 @@ var _ = Describe("VPC", func() {
AvailabilityZones: []string{"az1", "az2", "az3"},
}

cfgWithAllAZAndNGs := &api.ClusterConfig{
NodeGroups: []*api.NodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Subnets: []string{"invalid id"},
},
},
},
ManagedNodeGroups: []*api.ManagedNodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Subnets: []string{"invalid id"},
},
},
},
VPC: &api.ClusterVPC{
Subnets: &api.ClusterSubnets{
Private: api.AZSubnetMappingFromMap(map[string]api.AZSubnetSpec{
"az1": {
ID: "private1",
},
"az2": {
ID: "private2",
},
"az3": {
ID: "private3",
},
}),
Public: api.AZSubnetMappingFromMap(map[string]api.AZSubnetSpec{
"az1": {
ID: "public1",
},
"az2": {
ID: "public2",
},
"az3": {
ID: "public3",
},
}),
},
},
AvailabilityZones: []string{"az1", "az2", "az3"},
}

DescribeTable("clean up the subnets details in spec if given AZ is invalid",
func(e cleanupSubnetsCase) {
cleanupSubnets(e.cfg)
Expect(e.cfg).To(Equal(cfgWithAllAZ))
Expect(e.cfg).To(Equal(e.want))
},

Entry("All AZs are valid", cleanupSubnetsCase{
Expand Down Expand Up @@ -911,10 +1071,25 @@ var _ = Describe("VPC", func() {
},
AvailabilityZones: []string{"az1", "az2", "az3"},
},
want: cfgWithAllAZ,
}),

Entry("Private subnet with invalid AZ", cleanupSubnetsCase{
cfg: &api.ClusterConfig{
NodeGroups: []*api.NodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Subnets: []string{"invalid AZ"},
},
},
},
ManagedNodeGroups: []*api.ManagedNodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Subnets: []string{"invalid AZ"},
},
},
},
VPC: &api.ClusterVPC{
Subnets: &api.ClusterSubnets{
Private: api.AZSubnetMappingFromMap(map[string]api.AZSubnetSpec{
Expand All @@ -928,7 +1103,7 @@ var _ = Describe("VPC", func() {
ID: "private3",
},
"invalid AZ": {
ID: "invalid private id",
ID: "invalid id",
},
}),
Public: api.AZSubnetMappingFromMap(map[string]api.AZSubnetSpec{
Expand All @@ -946,10 +1121,24 @@ var _ = Describe("VPC", func() {
},
AvailabilityZones: []string{"az1", "az2", "az3"},
},
want: cfgWithAllAZ,
want: cfgWithAllAZAndNGs,
}),
Entry("Public subnet with invalid AZ", cleanupSubnetsCase{
cfg: &api.ClusterConfig{
NodeGroups: []*api.NodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Subnets: []string{"invalid AZ"},
},
},
},
ManagedNodeGroups: []*api.ManagedNodeGroup{
{
NodeGroupBase: &api.NodeGroupBase{
Subnets: []string{"invalid AZ"},
},
},
},
VPC: &api.ClusterVPC{
Subnets: &api.ClusterSubnets{
Private: api.AZSubnetMappingFromMap(map[string]api.AZSubnetSpec{
Expand All @@ -974,14 +1163,14 @@ var _ = Describe("VPC", func() {
ID: "public3",
},
"invalid AZ": {
ID: "invalid public id",
ID: "invalid id",
},
}),
},
},
AvailabilityZones: []string{"az1", "az2", "az3"},
},
want: cfgWithAllAZ,
want: cfgWithAllAZAndNGs,
}),
)

Expand Down

0 comments on commit 1807c5e

Please sign in to comment.