Skip to content

Commit ac41654

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2635 from bnallapeta/hcp-2380
🐛 Fix panic when OpenStackCluster.Status.Network is nil in HCP scenarios
2 parents d278b9b + c07dc18 commit ac41654

File tree

5 files changed

+530
-14
lines changed

5 files changed

+530
-14
lines changed

‎controllers/openstackcluster_controller.go‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,10 @@ func (r *OpenStackClusterReconciler) reconcileBastionServer(ctx context.Context,
499499
}
500500

501501
// If the bastion is found but the spec has changed, we need to delete it and reconcile.
502-
bastionServerSpec := bastionToOpenStackServerSpec(openStackCluster)
502+
bastionServerSpec, err := bastionToOpenStackServerSpec(openStackCluster)
503+
if err != nil {
504+
return nil, true, err
505+
}
503506
if !bastionNotFound && server != nil && !apiequality.Semantic.DeepEqual(bastionServerSpec, &server.Spec) {
504507
scope.Logger().Info("Bastion spec has changed, re-creating the OpenStackServer object")
505508
if err := r.deleteBastion(ctx, scope, cluster, openStackCluster); err != nil {
@@ -545,7 +548,10 @@ func (r *OpenStackClusterReconciler) getBastionServer(ctx context.Context, openS
545548
// createBastionServer creates the OpenStackServer object for the bastion server.
546549
// It returns the OpenStackServer object and an error if any.
547550
func (r *OpenStackClusterReconciler) createBastionServer(ctx context.Context, openStackCluster *infrav1.OpenStackCluster, cluster *clusterv1.Cluster) (*infrav1alpha1.OpenStackServer, error) {
548-
bastionServerSpec := bastionToOpenStackServerSpec(openStackCluster)
551+
bastionServerSpec, err := bastionToOpenStackServerSpec(openStackCluster)
552+
if err != nil {
553+
return nil, err
554+
}
549555
bastionServer := &infrav1alpha1.OpenStackServer{
550556
ObjectMeta: metav1.ObjectMeta{
551557
Labels: map[string]string{
@@ -573,7 +579,7 @@ func (r *OpenStackClusterReconciler) createBastionServer(ctx context.Context, op
573579

574580
// bastionToOpenStackServerSpec converts the OpenStackMachineSpec for the bastion to an OpenStackServerSpec.
575581
// It returns the OpenStackServerSpec and an error if any.
576-
func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) *infrav1alpha1.OpenStackServerSpec {
582+
func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) (*infrav1alpha1.OpenStackServerSpec, error) {
577583
bastion := openStackCluster.Spec.Bastion
578584
if bastion == nil {
579585
bastion = &infrav1.Bastion{}
@@ -588,9 +594,12 @@ func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) *i
588594
if bastion.AvailabilityZone != nil {
589595
az = *bastion.AvailabilityZone
590596
}
591-
openStackServerSpec := openStackMachineSpecToOpenStackServerSpec(bastion.Spec, openStackCluster.Spec.IdentityRef, compute.InstanceTags(bastion.Spec, openStackCluster), az, nil, getBastionSecurityGroupID(openStackCluster), openStackCluster.Status.Network.ID)
597+
openStackServerSpec, err := openStackMachineSpecToOpenStackServerSpec(bastion.Spec, openStackCluster.Spec.IdentityRef, compute.InstanceTags(bastion.Spec, openStackCluster), az, nil, getBastionSecurityGroupID(openStackCluster), openStackCluster.Status.Network)
598+
if err != nil {
599+
return nil, err
600+
}
592601

593-
return openStackServerSpec
602+
return openStackServerSpec, nil
594603
}
595604

596605
func bastionName(clusterResourceName string) string {

‎controllers/openstackmachine_controller.go‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,18 @@ func (r *OpenStackMachineReconciler) getMachineServer(ctx context.Context, openS
480480

481481
// openStackMachineSpecToOpenStackServerSpec converts an OpenStackMachineSpec to an OpenStackServerSpec.
482482
// It returns the OpenStackServerSpec object and an error if there is any.
483-
func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.OpenStackMachineSpec, identityRef infrav1.OpenStackIdentityReference, tags []string, failureDomain string, userDataRef *corev1.LocalObjectReference, defaultSecGroup *string, defaultNetworkID string) *infrav1alpha1.OpenStackServerSpec {
483+
func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.OpenStackMachineSpec, identityRef infrav1.OpenStackIdentityReference, tags []string, failureDomain string, userDataRef *corev1.LocalObjectReference, defaultSecGroup *string, clusterNetwork *infrav1.NetworkStatusWithSubnets) (*infrav1alpha1.OpenStackServerSpec, error) {
484+
// Determine default network ID if the cluster status exposes one.
485+
var defaultNetworkID string
486+
if clusterNetwork != nil {
487+
defaultNetworkID = clusterNetwork.ID
488+
}
489+
490+
// If no cluster network is available AND the machine spec did not define any ports with a network, we cannot choose a network.
491+
if defaultNetworkID == "" && len(openStackMachineSpec.Ports) == 0 {
492+
return nil, capoerrors.Terminal(infrav1.InvalidMachineSpecReason, "no network configured: cluster network is missing and machine spec does not define ports with a network")
493+
}
494+
484495
openStackServerSpec := &infrav1alpha1.OpenStackServerSpec{
485496
AdditionalBlockDevices: openStackMachineSpec.AdditionalBlockDevices,
486497
ConfigDrive: openStackMachineSpec.ConfigDrive,
@@ -522,7 +533,8 @@ func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.Ope
522533
serverPorts = make([]infrav1.PortOpts, 1)
523534
}
524535
for i := range serverPorts {
525-
if serverPorts[i].Network == nil {
536+
// Only inject the default network when we actually have an ID.
537+
if serverPorts[i].Network == nil && defaultNetworkID != "" {
526538
serverPorts[i].Network = &infrav1.NetworkParam{
527539
ID: &defaultNetworkID,
528540
}
@@ -540,7 +552,7 @@ func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.Ope
540552
}
541553
openStackServerSpec.Ports = serverPorts
542554

543-
return openStackServerSpec
555+
return openStackServerSpec, nil
544556
}
545557

546558
// reconcileMachineServer reconciles the OpenStackServer object for the OpenStackMachine.
@@ -589,7 +601,10 @@ func (r *OpenStackMachineReconciler) getOrCreateMachineServer(ctx context.Contex
589601
}
590602
return openStackCluster.Spec.IdentityRef
591603
}()
592-
machineServerSpec := openStackMachineSpecToOpenStackServerSpec(&openStackMachine.Spec, identityRef, compute.InstanceTags(&openStackMachine.Spec, openStackCluster), failureDomain, userDataRef, getManagedSecurityGroup(openStackCluster, machine), openStackCluster.Status.Network.ID)
604+
machineServerSpec, err := openStackMachineSpecToOpenStackServerSpec(&openStackMachine.Spec, identityRef, compute.InstanceTags(&openStackMachine.Spec, openStackCluster), failureDomain, userDataRef, getManagedSecurityGroup(openStackCluster, machine), openStackCluster.Status.Network)
605+
if err != nil {
606+
return nil, err
607+
}
593608
machineServer = &infrav1alpha1.OpenStackServer{
594609
ObjectMeta: metav1.ObjectMeta{
595610
Labels: map[string]string{

‎controllers/openstackmachine_controller_test.go‎

Lines changed: 89 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,11 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
9595
tags := []string{"tag1", "tag2"}
9696
userData := &corev1.LocalObjectReference{Name: "server-data-secret"}
9797
tests := []struct {
98-
name string
99-
spec *infrav1.OpenStackMachineSpec
100-
want *infrav1alpha1.OpenStackServerSpec
98+
name string
99+
spec *infrav1.OpenStackMachineSpec
100+
clusterNetwork *infrav1.NetworkStatusWithSubnets
101+
want *infrav1alpha1.OpenStackServerSpec
102+
wantErr bool
101103
}{
102104
{
103105
name: "Test a minimum OpenStackMachineSpec to OpenStackServerSpec conversion",
@@ -106,6 +108,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
106108
Image: image,
107109
SSHKeyName: sshKeyName,
108110
},
111+
clusterNetwork: openStackCluster.Status.Network,
109112
want: &infrav1alpha1.OpenStackServerSpec{
110113
Flavor: ptr.To(flavorName),
111114
IdentityRef: identityRef,
@@ -128,6 +131,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
128131
},
129132
},
130133
},
134+
clusterNetwork: openStackCluster.Status.Network,
131135
want: &infrav1alpha1.OpenStackServerSpec{
132136
Flavor: ptr.To(flavorName),
133137
IdentityRef: identityRef,
@@ -146,6 +150,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
146150
Image: image,
147151
SSHKeyName: sshKeyName,
148152
},
153+
clusterNetwork: openStackCluster.Status.Network,
149154
want: &infrav1alpha1.OpenStackServerSpec{
150155
Flavor: ptr.To(flavorName),
151156
FlavorID: ptr.To(flavorUUID),
@@ -164,6 +169,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
164169
Image: image,
165170
SSHKeyName: sshKeyName,
166171
},
172+
clusterNetwork: openStackCluster.Status.Network,
167173
want: &infrav1alpha1.OpenStackServerSpec{
168174
FlavorID: ptr.To(flavorUUID),
169175
IdentityRef: identityRef,
@@ -174,12 +180,90 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
174180
UserDataRef: userData,
175181
},
176182
},
183+
{
184+
name: "Cluster network nil, machine defines port network and overrides SG",
185+
spec: &infrav1.OpenStackMachineSpec{
186+
Ports: []infrav1.PortOpts{{
187+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
188+
}},
189+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
190+
},
191+
clusterNetwork: nil,
192+
want: &infrav1alpha1.OpenStackServerSpec{
193+
IdentityRef: identityRef,
194+
Ports: []infrav1.PortOpts{{
195+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
196+
SecurityGroups: []infrav1.SecurityGroupParam{
197+
{ID: ptr.To(workerSecurityGroupUUID)},
198+
{ID: ptr.To(extraSecurityGroupUUID)},
199+
},
200+
}},
201+
Tags: tags,
202+
UserDataRef: userData,
203+
},
204+
},
205+
{
206+
name: "Cluster network nil, machine defines port network and falls back to cluster SG",
207+
spec: &infrav1.OpenStackMachineSpec{
208+
Ports: []infrav1.PortOpts{{
209+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
210+
}},
211+
},
212+
clusterNetwork: nil,
213+
want: &infrav1alpha1.OpenStackServerSpec{
214+
IdentityRef: identityRef,
215+
Ports: []infrav1.PortOpts{{
216+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
217+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(workerSecurityGroupUUID)}},
218+
}},
219+
Tags: tags,
220+
UserDataRef: userData,
221+
},
222+
},
223+
{
224+
name: "Error case: no cluster network and no machine ports",
225+
spec: &infrav1.OpenStackMachineSpec{
226+
Flavor: ptr.To(flavorName),
227+
Image: image,
228+
SSHKeyName: sshKeyName,
229+
// No ports defined
230+
},
231+
clusterNetwork: nil,
232+
want: nil,
233+
wantErr: true,
234+
},
235+
{
236+
name: "Empty cluster network ID, machine defines explicit ports",
237+
spec: &infrav1.OpenStackMachineSpec{
238+
Flavor: ptr.To(flavorName),
239+
Image: image,
240+
Ports: []infrav1.PortOpts{{
241+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
242+
}},
243+
},
244+
clusterNetwork: &infrav1.NetworkStatusWithSubnets{NetworkStatus: infrav1.NetworkStatus{ID: ""}},
245+
want: &infrav1alpha1.OpenStackServerSpec{
246+
Flavor: ptr.To(flavorName),
247+
IdentityRef: identityRef,
248+
Image: image,
249+
Ports: []infrav1.PortOpts{{
250+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
251+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(workerSecurityGroupUUID)}},
252+
}},
253+
Tags: tags,
254+
UserDataRef: userData,
255+
},
256+
},
177257
}
178258
for i := range tests {
179259
tt := tests[i]
180260
t.Run(tt.name, func(t *testing.T) {
181-
spec := openStackMachineSpecToOpenStackServerSpec(tt.spec, identityRef, tags, "", userData, &openStackCluster.Status.WorkerSecurityGroup.ID, openStackCluster.Status.Network.ID)
182-
if !reflect.DeepEqual(spec, tt.want) {
261+
spec, err := openStackMachineSpecToOpenStackServerSpec(tt.spec, identityRef, tags, "", userData, &openStackCluster.Status.WorkerSecurityGroup.ID, tt.clusterNetwork)
262+
if (err != nil) != tt.wantErr {
263+
t.Errorf("openStackMachineSpecToOpenStackServerSpec() error = %v, wantErr %v", err, tt.wantErr)
264+
return
265+
}
266+
if !tt.wantErr && !reflect.DeepEqual(spec, tt.want) {
183267
t.Errorf("openStackMachineSpecToOpenStackServerSpec() got = %+v, want %+v", spec, tt.want)
184268
}
185269
})

‎docs/book/src/SUMMARY.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
- [Configuration](clusteropenstack/configuration.md)
66
- [Topics](./topics/index.md)
77
- [external cloud provider](./topics/external-cloud-provider.md)
8+
- [hosted control plane](./topics/hosted-control-plane.md)
89
- [move from bootstrap](./topics/mover.md)
910
- [trouble shooting](./topics/troubleshooting.md)
1011
- [CRD Changes](./topics/crd-changes/index.md)

0 commit comments

Comments
 (0)