Skip to content

Commit a424a7f

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #8347 from mtulio/CORS-3479-fix-control-plane-placement
CORS-2895: capa/machines: fix zone placement for control planes
2 parents 18b9aa2 + d56a003 commit a424a7f

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)