Skip to content

Commit d56a003

Browse files
mtuliomtulio
committed
CORS-3479: seting subnet config when not BYO-VPC deployment
Nodes must be correctly distributed among zones in the default deployment, Manage VPC and no zones is set in the install-config.yaml. Additionally, when the zones is set, the control plane nodes must follow the user-defined configuration, placing nodes correctly in the zones, considering the "subnet internet facing", when the PublicIP is set to the machine. This change changes the behavior not letting CAPA decide which zones will be placed when no subnets are set, keeping the terraform parity which is distributing control plane nodes between zones, preventing nodes in the same zone when there are available.
1 parent 1cb86cb commit d56a003

File tree

3 files changed

+282
-13
lines changed

3 files changed

+282
-13
lines changed

‎pkg/asset/machines/aws/awsmachines.go‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,24 +46,29 @@ func GenerateMachines(clusterID string, in *MachineInput) ([]*asset.RuntimeFile,
4646
var result []*asset.RuntimeFile
4747

4848
for idx := int64(0); idx < total; idx++ {
49-
var subnet *capa.AWSResourceReference
50-
// By not setting subnets for the machine, we let CAPA choose one for us
49+
subnet := &capa.AWSResourceReference{}
50+
zone := mpool.Zones[int(idx)%len(mpool.Zones)]
51+
52+
// BYO VPC deployments when subnet IDs are set on install-config.yaml
5153
if len(in.Subnets) > 0 {
52-
zone := mpool.Zones[int(idx)%len(mpool.Zones)]
5354
subnetID, ok := in.Subnets[zone]
5455
if len(in.Subnets) > 0 && !ok {
5556
return nil, fmt.Errorf("no subnet for zone %s", zone)
5657
}
57-
subnet = &capa.AWSResourceReference{}
5858
if subnetID == "" {
59-
subnet.Filters = []capa.Filter{
60-
{
61-
Name: "tag:Name",
62-
Values: []string{fmt.Sprintf("%s-subnet-private-%s", clusterID, zone)},
63-
},
64-
}
65-
} else {
66-
subnet.ID = ptr.To(subnetID)
59+
return nil, fmt.Errorf("invalid subnet ID for zone %s", zone)
60+
}
61+
subnet.ID = ptr.To(subnetID)
62+
} else {
63+
subnetInternetScope := "private"
64+
if in.PublicIP {
65+
subnetInternetScope = "public"
66+
}
67+
subnet.Filters = []capa.Filter{
68+
{
69+
Name: "tag:Name",
70+
Values: []string{fmt.Sprintf("%s-subnet-%s-%s", clusterID, subnetInternetScope, zone)},
71+
},
6772
}
6873
}
6974

‎pkg/asset/machines/aws/awsmachines_test.go‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
// Package aws generates Machine objects for aws.
2+
package aws
3+
4+
import (
5+
"fmt"
6+
"strings"
7+
"testing"
8+
9+
"github.com/google/go-cmp/cmp"
10+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
"k8s.io/utils/ptr"
12+
capa "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
13+
14+
"github.com/openshift/installer/pkg/asset"
15+
"github.com/openshift/installer/pkg/types"
16+
awstypes "github.com/openshift/installer/pkg/types/aws"
17+
)
18+
19+
var stubMachineInputManagedVpc = &MachineInput{
20+
Role: "master",
21+
Pool: &types.MachinePool{
22+
Name: "master",
23+
Replicas: ptr.To(int64(3)),
24+
Platform: types.MachinePoolPlatform{
25+
AWS: &awstypes.MachinePool{
26+
Zones: []string{"A", "B", "C"},
27+
},
28+
},
29+
},
30+
Subnets: make(map[string]string, 0),
31+
Tags: capa.Tags{},
32+
PublicIP: false,
33+
Ignition: &capa.Ignition{},
34+
}
35+
36+
func stubDeepCopyMachineInput(in *MachineInput) *MachineInput {
37+
out := &MachineInput{
38+
Role: in.Role,
39+
PublicIP: in.PublicIP,
40+
}
41+
if in.Pool != nil {
42+
out.Pool = &types.MachinePool{}
43+
*out.Pool = *in.Pool
44+
}
45+
if len(in.Subnets) > 0 {
46+
out.Subnets = make(map[string]string, len(in.Subnets))
47+
for k, v := range in.Subnets {
48+
out.Subnets[k] = v
49+
}
50+
}
51+
if len(in.Tags) > 0 {
52+
out.Tags = in.Tags.DeepCopy()
53+
}
54+
if in.Ignition != nil {
55+
out.Ignition = in.Ignition.DeepCopy()
56+
}
57+
return out
58+
}
59+
60+
func stubGetMachineManagedVpc() *MachineInput {
61+
return stubDeepCopyMachineInput(stubMachineInputManagedVpc)
62+
}
63+
64+
func TestGenerateMachines(t *testing.T) {
65+
stubClusterID := "vpc-zr2-m2"
66+
tests := []struct {
67+
name string
68+
clusterID string
69+
input *MachineInput
70+
want []*asset.RuntimeFile
71+
wantInfraFiles []*asset.RuntimeFile
72+
wantErr string
73+
}{
74+
{
75+
name: "topology ha, managed vpc, default zones region, 2 zones A and B, 3 machines should be in A, B and A private subnet",
76+
clusterID: stubClusterID,
77+
input: func() *MachineInput {
78+
in := stubGetMachineManagedVpc()
79+
in.Pool.Platform.AWS.Zones = []string{"A", "B"}
80+
return in
81+
}(),
82+
// generate 3 AWSMachine manifests for control plane nodes in two zones
83+
wantInfraFiles: func() []*asset.RuntimeFile {
84+
machineZoneMap := map[int]string{0: "A", 1: "B", 2: "A"}
85+
infraMachineFiles := []*asset.RuntimeFile{}
86+
for mid := 0; mid < 3; mid++ {
87+
machineName := fmt.Sprintf("%s-%s-%d", stubClusterID, "master", mid)
88+
machineZone := machineZoneMap[mid]
89+
machine := &capa.AWSMachine{
90+
TypeMeta: metav1.TypeMeta{
91+
APIVersion: "infrastructure.cluster.x-k8s.io/v1beta2",
92+
Kind: "AWSMachine",
93+
},
94+
ObjectMeta: metav1.ObjectMeta{
95+
Name: machineName,
96+
Labels: map[string]string{"cluster.x-k8s.io/control-plane": ""},
97+
},
98+
Spec: capa.AWSMachineSpec{
99+
InstanceMetadataOptions: &capa.InstanceMetadataOptions{
100+
HTTPEndpoint: capa.InstanceMetadataEndpointStateEnabled,
101+
},
102+
AMI: capa.AMIReference{
103+
ID: ptr.To(""),
104+
},
105+
IAMInstanceProfile: fmt.Sprintf("%s-%s-profile", stubClusterID, "master"),
106+
PublicIP: ptr.To(false),
107+
Subnet: &capa.AWSResourceReference{
108+
Filters: []capa.Filter{{Name: "tag:Name", Values: []string{
109+
fmt.Sprintf("%s-subnet-private-%s", stubClusterID, machineZone),
110+
}}},
111+
},
112+
SSHKeyName: ptr.To(""),
113+
RootVolume: &capa.Volume{
114+
Encrypted: ptr.To(true),
115+
},
116+
UncompressedUserData: ptr.To(true),
117+
Ignition: &capa.Ignition{},
118+
},
119+
}
120+
infraMachineFiles = append(infraMachineFiles, &asset.RuntimeFile{
121+
File: asset.File{Filename: fmt.Sprintf("10_inframachine_%s.yaml", machineName)},
122+
Object: machine,
123+
})
124+
}
125+
return infraMachineFiles
126+
}(),
127+
},
128+
{
129+
name: "topology ha, byo vpc, two zones subnets A and B, 3 machines should be in A, B and A private subnets",
130+
clusterID: stubClusterID,
131+
input: func() *MachineInput {
132+
in := stubGetMachineManagedVpc()
133+
in.Pool.Platform.AWS.Zones = []string{"A", "B"}
134+
in.Subnets = map[string]string{"A": "subnet-id-A", "B": "subnet-id-B"}
135+
return in
136+
}(),
137+
// generate 3 AWSMachine manifests for control plane nodes in two subnets/zones
138+
wantInfraFiles: func() []*asset.RuntimeFile {
139+
machineZoneMap := map[int]string{0: "subnet-id-A", 1: "subnet-id-B", 2: "subnet-id-A"}
140+
infraMachineFiles := []*asset.RuntimeFile{}
141+
for mid := 0; mid < 3; mid++ {
142+
machineName := fmt.Sprintf("%s-%s-%d", stubClusterID, "master", mid)
143+
machineSubnet := machineZoneMap[mid]
144+
machine := &capa.AWSMachine{
145+
TypeMeta: metav1.TypeMeta{
146+
APIVersion: "infrastructure.cluster.x-k8s.io/v1beta2",
147+
Kind: "AWSMachine",
148+
},
149+
ObjectMeta: metav1.ObjectMeta{
150+
Name: machineName,
151+
Labels: map[string]string{"cluster.x-k8s.io/control-plane": ""},
152+
},
153+
Spec: capa.AWSMachineSpec{
154+
InstanceMetadataOptions: &capa.InstanceMetadataOptions{
155+
HTTPEndpoint: capa.InstanceMetadataEndpointStateEnabled,
156+
},
157+
AMI: capa.AMIReference{
158+
ID: ptr.To(""),
159+
},
160+
IAMInstanceProfile: fmt.Sprintf("%s-%s-profile", stubClusterID, "master"),
161+
PublicIP: ptr.To(false),
162+
Subnet: &capa.AWSResourceReference{
163+
ID: ptr.To(machineSubnet),
164+
},
165+
SSHKeyName: ptr.To(""),
166+
RootVolume: &capa.Volume{
167+
Encrypted: ptr.To(true),
168+
},
169+
UncompressedUserData: ptr.To(true),
170+
Ignition: &capa.Ignition{},
171+
},
172+
}
173+
infraMachineFiles = append(infraMachineFiles, &asset.RuntimeFile{
174+
File: asset.File{Filename: fmt.Sprintf("10_inframachine_%s.yaml", machineName)},
175+
Object: machine,
176+
})
177+
}
178+
return infraMachineFiles
179+
}(),
180+
},
181+
// Error's scenarios
182+
{
183+
name: "error topology ha, byo vpc, no subnet for zones",
184+
clusterID: stubClusterID,
185+
input: func() *MachineInput {
186+
in := stubGetMachineManagedVpc()
187+
in.Pool.Platform.AWS.Zones = []string{"A", "B"}
188+
in.Subnets = map[string]string{"C": "subnet-id-C", "D": "subnet-id-D"}
189+
return in
190+
}(),
191+
wantErr: `no subnet for zone A`,
192+
},
193+
{
194+
name: "error topology ha, managed vpc, empty subnet zone",
195+
clusterID: stubClusterID,
196+
input: func() *MachineInput {
197+
in := stubGetMachineManagedVpc()
198+
in.Pool.Platform.AWS.Zones = []string{"A", "B"}
199+
in.Subnets = map[string]string{"A": "subnet-id-A", "B": ""}
200+
return in
201+
}(),
202+
wantErr: `invalid subnet ID for zone B`,
203+
},
204+
// TODO: add more use cases.
205+
// {
206+
// name: "managed vpc, default zones region, 5 zones A to E, 3 machines should be in A, B and C private subnet",
207+
// },
208+
// {
209+
// name: "managed vpc, default zones region, 5 zones A to E, 3 machines should be in A, B and C private subnet",
210+
// },
211+
// {
212+
// name: "byo vpc, 2 zones subnets A and B, 3 machines' subnet should be in A, B and A",
213+
// },
214+
// {
215+
// name: "managed vpc, default zones region, 5 zones A to E, bootstrap should be in zone A public subnet",
216+
// },
217+
// {
218+
// name: "topology ha, managed vpc, two zones subnets A and B, bootstrap node using public subnet A",
219+
// },
220+
}
221+
for _, tt := range tests {
222+
t.Run(tt.name, func(t *testing.T) {
223+
files, err := GenerateMachines(tt.clusterID, tt.input)
224+
if err != nil {
225+
if len(tt.wantErr) > 0 {
226+
if got := err.Error(); !cmp.Equal(got, tt.wantErr) {
227+
t.Errorf("GenerateMachines() unexpected error message: %v", cmp.Diff(got, tt.wantErr))
228+
}
229+
return
230+
}
231+
t.Errorf("GenerateMachines() unexpected error: %v", err)
232+
return
233+
}
234+
// TODO: support the CAPA v1beta1.Machine manifest check.
235+
// Support only comparing manifest file for CAPA v1beta2.AWSMachine.
236+
if len(tt.wantInfraFiles) > 0 {
237+
got := []*asset.RuntimeFile{}
238+
for _, file := range files {
239+
if !strings.HasPrefix(file.Filename, "10_inframachine") {
240+
continue
241+
}
242+
got = append(got, file)
243+
}
244+
if !cmp.Equal(got, tt.wantInfraFiles) {
245+
t.Errorf("GenerateMachines() Got unexpected results:\n%v", cmp.Diff(got, tt.wantInfraFiles))
246+
}
247+
}
248+
})
249+
}
250+
}

‎pkg/asset/machines/clusterapi.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,28 @@ func (c *ClusterAPI) Generate(dependencies asset.Parents) error {
9191
switch ic.Platform.Name() {
9292
case awstypes.Name:
9393
subnets := map[string]string{}
94+
bootstrapSubnets := map[string]string{}
9495
if len(ic.Platform.AWS.Subnets) > 0 {
96+
// fetch private subnets to master nodes.
9597
subnetMeta, err := installConfig.AWS.PrivateSubnets(ctx)
9698
if err != nil {
9799
return err
98100
}
99101
for id, subnet := range subnetMeta {
100102
subnets[subnet.Zone.Name] = id
101103
}
104+
// fetch public subnets for bootstrap, when exists, otherwise use private.
105+
if installConfig.Config.Publish == types.ExternalPublishingStrategy {
106+
subnetMeta, err := installConfig.AWS.PublicSubnets(ctx)
107+
if err != nil {
108+
return err
109+
}
110+
for id, subnet := range subnetMeta {
111+
bootstrapSubnets[subnet.Zone.Name] = id
112+
}
113+
} else {
114+
bootstrapSubnets = subnets
115+
}
102116
}
103117

104118
mpool := defaultAWSMachinePoolPlatform("master")
@@ -177,7 +191,7 @@ func (c *ClusterAPI) Generate(dependencies asset.Parents) error {
177191
pool.Platform.AWS = &mpool
178192
bootstrapAWSMachine, err := aws.GenerateMachines(clusterID.InfraID, &aws.MachineInput{
179193
Role: "bootstrap",
180-
Subnets: nil, // let CAPA pick one
194+
Subnets: bootstrapSubnets,
181195
Pool: &pool,
182196
Tags: tags,
183197
PublicIP: installConfig.Config.Publish == types.ExternalPublishingStrategy,

0 commit comments

Comments
 (0)