Sitelet https://github.com/hakman/kops/commit/90e0b45e07ea010519cd040de000ed73fb651c6a
Skip to content

Commit 90e0b45

Browse files
hakmanhakman
committed
aws: allow shared subnets to omit the zone
1 parent b5fe278 commit 90e0b45

7 files changed

Lines changed: 169 additions & 13 deletions

File tree

‎pkg/model/resources/nodeup.go‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -448,11 +448,7 @@ func buildEnvironmentVariables(cluster *kops.Cluster, ig *kops.InstanceGroup) (m
448448
if err != nil {
449449
return nil, err
450450
}
451-
if region == "" {
452-
klog.Warningf("unable to determine cluster region")
453-
} else {
454-
env["AWS_REGION"] = region
455-
}
451+
env["AWS_REGION"] = region
456452
}
457453

458454
if cluster.GetCloudProvider() == kops.CloudProviderAzure {

‎tests/integration/update_cluster/private-shared-subnet/in-v1alpha2.yaml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ spec:
4141
id: subnet-abcdef
4242
name: utility-us-test-1a
4343
type: Utility
44-
zone: us-test-1a
4544

4645
---
4746

‎upup/pkg/fi/cloudup/awsup/aws_utils.go‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,19 @@ func ValidateRegion(ctx context.Context, region string) error {
9292
func FindRegion(cluster *kops.Cluster) (string, error) {
9393
region := ""
9494

95-
nodeZones := make(map[string]bool)
9695
for _, subnet := range cluster.Spec.Networking.Subnets {
96+
if subnet.Zone == "" {
97+
// The zone of a subnet specified by ID is looked up from the cloud later.
98+
if subnet.ID == "" {
99+
return "", fmt.Errorf("subnet %q must specify a zone or the ID of an existing subnet", subnet.Name)
100+
}
101+
continue
102+
}
103+
97104
if len(subnet.Zone) <= 2 {
98105
return "", fmt.Errorf("invalid AWS zone: %q in subnet %q", subnet.Zone, subnet.Name)
99106
}
100107

101-
nodeZones[subnet.Zone] = true
102-
103108
zoneRegion := subnet.Zone[:len(subnet.Zone)-1]
104109
if region != "" && zoneRegion != region {
105110
return "", fmt.Errorf("error Clusters cannot span multiple regions (found zone %q, but region is %q)", subnet.Zone, region)
@@ -108,6 +113,10 @@ func FindRegion(cluster *kops.Cluster) (string, error) {
108113
region = zoneRegion
109114
}
110115

116+
if region == "" {
117+
return "", fmt.Errorf("could not determine cluster region: no subnet specifies a zone")
118+
}
119+
111120
return region, nil
112121
}
113122

‎upup/pkg/fi/cloudup/awsup/aws_utils_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,57 @@ func TestFindRegion(t *testing.T) {
6666
t.Fatalf("unexpected region for zone: %q vs %q", expected, region)
6767
}
6868
}
69+
70+
testCases := []struct {
71+
name string
72+
subnets []kops.ClusterSubnetSpec
73+
expected string
74+
expectError bool
75+
}{
76+
{
77+
name: "ignores subnets specified by ID without a zone",
78+
subnets: []kops.ClusterSubnetSpec{
79+
{Name: "subnet-a", ID: "subnet-12345678"},
80+
{Name: "subnet-b", Zone: "us-east-2b"},
81+
},
82+
expected: "us-east-2",
83+
},
84+
{
85+
name: "errors when no subnet specifies a zone",
86+
subnets: []kops.ClusterSubnetSpec{
87+
{Name: "subnet-a", ID: "subnet-12345678"},
88+
},
89+
expectError: true,
90+
},
91+
{
92+
name: "errors when a subnet has neither zone nor ID",
93+
subnets: []kops.ClusterSubnetSpec{
94+
{Name: "subnet-a"},
95+
{Name: "subnet-b", Zone: "us-east-2b"},
96+
},
97+
expectError: true,
98+
},
99+
}
100+
for _, tc := range testCases {
101+
t.Run(tc.name, func(t *testing.T) {
102+
c := &kops.Cluster{}
103+
c.Spec.Networking.Subnets = tc.subnets
104+
105+
region, err := FindRegion(c)
106+
if tc.expectError {
107+
if err == nil {
108+
t.Fatalf("expected error finding region, got %q", region)
109+
}
110+
return
111+
}
112+
if err != nil {
113+
t.Fatalf("unexpected error finding region: %v", err)
114+
}
115+
if region != tc.expected {
116+
t.Fatalf("unexpected region: %q vs %q", region, tc.expected)
117+
}
118+
})
119+
}
69120
}
70121

71122
func TestEC2TagSpecification(t *testing.T) {

‎upup/pkg/fi/cloudup/subnets.go‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,19 @@ func assignCIDRsToSubnets(c *kops.Cluster, cloud fi.Cloud) error {
4747
// TODO: We probably could query for the existing subnets & allocate appropriately
4848
// for now we'll require users to set CIDRs themselves
4949

50-
if allSubnetsHaveCIDRs(c) {
50+
// On AWS, subnets specified by ID may omit the zone; we look it up from the cloud so that the
51+
// rest of kOps can rely on the zone being set.
52+
needZones := false
53+
if c.GetCloudProvider() == kops.CloudProviderAWS {
54+
for _, subnet := range c.Spec.Networking.Subnets {
55+
if subnet.ID != "" && subnet.Zone == "" {
56+
needZones = true
57+
break
58+
}
59+
}
60+
}
61+
62+
if allSubnetsHaveCIDRs(c) && !needZones {
5163
klog.V(4).Infof("All subnets have CIDRs; skipping assignment logic")
5264
return nil
5365
}
@@ -75,21 +87,33 @@ func assignCIDRsToSubnets(c *kops.Cluster, cloud fi.Cloud) error {
7587
}
7688
if subnet.CIDR == "" {
7789
subnet.CIDR = cloudSubnet.CIDR
78-
if subnet.CIDR == "" {
90+
// IPv6-only private subnets do not have an IPv4 CIDR
91+
if subnet.CIDR == "" && (subnet.IPv6CIDR == "" || subnet.Type != kops.SubnetTypePrivate) {
7992
return fmt.Errorf("Subnet %q did not have CIDR", subnet.ID)
8093
}
8194
} else if subnet.CIDR != cloudSubnet.CIDR {
8295
return fmt.Errorf("Subnet %q has configured CIDR %q, but the actual CIDR found was %q", subnet.ID, subnet.CIDR, cloudSubnet.CIDR)
8396
}
8497

85-
if subnet.Zone != cloudSubnet.Zone {
98+
if needZones && subnet.Zone == "" {
99+
subnet.Zone = cloudSubnet.Zone
100+
} else if subnet.Zone != cloudSubnet.Zone {
86101
return fmt.Errorf("Subnet %q has configured Zone %q, but the actual Zone found was %q", subnet.ID, subnet.Zone, cloudSubnet.Zone)
87102
}
88103

89104
}
90105
}
91106
}
92107

108+
if needZones {
109+
for i := range c.Spec.Networking.Subnets {
110+
subnet := &c.Spec.Networking.Subnets[i]
111+
if subnet.ID != "" && subnet.Zone == "" {
112+
return fmt.Errorf("could not determine the zone of subnet %q; specify the zone in the cluster spec", subnet.Name)
113+
}
114+
}
115+
}
116+
93117
if allSubnetsHaveCIDRs(c) {
94118
klog.V(4).Infof("All subnets have CIDRs; skipping assignment logic")
95119
return nil

‎upup/pkg/fi/cloudup/subnets_test.go‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ import (
2121
"reflect"
2222
"testing"
2323

24+
"github.com/aws/aws-sdk-go-v2/aws"
25+
"github.com/aws/aws-sdk-go-v2/service/ec2"
26+
"k8s.io/kops/cloudmock/aws/mockec2"
2427
"k8s.io/kops/pkg/apis/kops"
28+
"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
2529
)
2630

2731
func Test_AssignSubnets(t *testing.T) {
@@ -147,3 +151,73 @@ func Test_AssignSubnets(t *testing.T) {
147151
})
148152
}
149153
}
154+
155+
func Test_AssignSubnetZones(t *testing.T) {
156+
buildCloud := func() *awsup.MockAWSCloud {
157+
cloud := awsup.BuildMockAWSCloud("us-test-1", "abc")
158+
mockEC2 := &mockec2.MockEC2{}
159+
cloud.MockEC2 = mockEC2
160+
161+
mockEC2.CreateVpcWithId(&ec2.CreateVpcInput{
162+
CidrBlock: aws.String("172.20.0.0/16"),
163+
}, "vpc-12345678")
164+
mockEC2.CreateSubnetWithId(&ec2.CreateSubnetInput{
165+
VpcId: aws.String("vpc-12345678"),
166+
AvailabilityZone: aws.String("us-test-1a"),
167+
CidrBlock: aws.String("172.20.32.0/19"),
168+
}, "subnet-a")
169+
mockEC2.CreateSubnetWithId(&ec2.CreateSubnetInput{
170+
VpcId: aws.String("vpc-12345678"),
171+
AvailabilityZone: aws.String("us-test-1b"),
172+
Ipv6CidrBlock: aws.String("2001:db8::/64"),
173+
}, "subnet-b")
174+
return cloud
175+
}
176+
177+
buildCluster := func(subnets []kops.ClusterSubnetSpec) *kops.Cluster {
178+
c := &kops.Cluster{}
179+
c.Spec.CloudProvider.AWS = &kops.AWSSpec{}
180+
c.Spec.Networking.NetworkID = "vpc-12345678"
181+
c.Spec.Networking.NetworkCIDR = "172.20.0.0/16"
182+
c.Spec.Networking.Subnets = subnets
183+
return c
184+
}
185+
186+
t.Run("zone is backfilled from the cloud", func(t *testing.T) {
187+
c := buildCluster([]kops.ClusterSubnetSpec{
188+
{Name: "a", ID: "subnet-a", CIDR: "172.20.32.0/19", Type: kops.SubnetTypePublic},
189+
})
190+
191+
if err := assignCIDRsToSubnets(c, buildCloud()); err != nil {
192+
t.Fatalf("unexpected error: %v", err)
193+
}
194+
if zone := c.Spec.Networking.Subnets[0].Zone; zone != "us-test-1a" {
195+
t.Fatalf("unexpected zone: %q", zone)
196+
}
197+
})
198+
199+
t.Run("IPv6-only private subnets do not require an IPv4 CIDR", func(t *testing.T) {
200+
c := buildCluster([]kops.ClusterSubnetSpec{
201+
{Name: "a", ID: "subnet-a", CIDR: "172.20.32.0/19", Type: kops.SubnetTypePublic},
202+
{Name: "b", ID: "subnet-b", IPv6CIDR: "2001:db8::/64", Zone: "us-test-1b", Type: kops.SubnetTypePrivate},
203+
})
204+
205+
if err := assignCIDRsToSubnets(c, buildCloud()); err != nil {
206+
t.Fatalf("unexpected error: %v", err)
207+
}
208+
if zone := c.Spec.Networking.Subnets[0].Zone; zone != "us-test-1a" {
209+
t.Fatalf("unexpected zone: %q", zone)
210+
}
211+
})
212+
213+
t.Run("errors when the zone cannot be determined", func(t *testing.T) {
214+
c := buildCluster([]kops.ClusterSubnetSpec{
215+
{Name: "a", ID: "subnet-a", CIDR: "172.20.32.0/19", Type: kops.SubnetTypePublic},
216+
})
217+
c.Spec.Networking.NetworkID = ""
218+
219+
if err := assignCIDRsToSubnets(c, buildCloud()); err == nil {
220+
t.Fatalf("expected error assigning zones")
221+
}
222+
})
223+
}

‎upup/pkg/fi/cloudup/utils.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,10 @@ func BuildCloud(cluster *kops.Cluster) (fi.Cloud, error) {
9393

9494
var zoneNames []string
9595
for _, subnet := range cluster.Spec.Networking.Subnets {
96-
zoneNames = append(zoneNames, subnet.Zone)
96+
// The zone of a subnet specified by ID is looked up from the cloud later.
97+
if subnet.Zone != "" {
98+
zoneNames = append(zoneNames, subnet.Zone)
99+
}
97100
}
98101
err = awsup.ValidateZones(zoneNames, awsCloud)
99102
if err != nil {

0 commit comments

Comments
 (0)