Skip to content

Commit 3321655

Browse files
bnallapetabnallapeta
committed
worked on PR comments: change in logic validation
Signed-off-by: Bharath Nallapeta <[email protected]>
1 parent f7dfd16 commit 3321655

File tree

4 files changed

+125
-155
lines changed

4 files changed

+125
-155
lines changed

‎controllers/openstackcluster_controller.go

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

499499
// If the bastion is found but the spec has changed, we need to delete it and reconcile.
500-
bastionServerSpec := bastionToOpenStackServerSpec(openStackCluster)
500+
bastionServerSpec, err := bastionToOpenStackServerSpec(openStackCluster)
501+
if err != nil {
502+
return nil, true, err
503+
}
501504
if !bastionNotFound && server != nil && !apiequality.Semantic.DeepEqual(bastionServerSpec, &server.Spec) {
502505
scope.Logger().Info("Bastion spec has changed, re-creating the OpenStackServer object")
503506
if err := r.deleteBastion(ctx, scope, cluster, openStackCluster); err != nil {
@@ -543,7 +546,10 @@ func (r *OpenStackClusterReconciler) getBastionServer(ctx context.Context, openS
543546
// createBastionServer creates the OpenStackServer object for the bastion server.
544547
// It returns the OpenStackServer object and an error if any.
545548
func (r *OpenStackClusterReconciler) createBastionServer(ctx context.Context, openStackCluster *infrav1.OpenStackCluster, cluster *clusterv1.Cluster) (*infrav1alpha1.OpenStackServer, error) {
546-
bastionServerSpec := bastionToOpenStackServerSpec(openStackCluster)
549+
bastionServerSpec, err := bastionToOpenStackServerSpec(openStackCluster)
550+
if err != nil {
551+
return nil, err
552+
}
547553
bastionServer := &infrav1alpha1.OpenStackServer{
548554
ObjectMeta: metav1.ObjectMeta{
549555
Labels: map[string]string{
@@ -571,7 +577,7 @@ func (r *OpenStackClusterReconciler) createBastionServer(ctx context.Context, op
571577

572578
// bastionToOpenStackServerSpec converts the OpenStackMachineSpec for the bastion to an OpenStackServerSpec.
573579
// It returns the OpenStackServerSpec and an error if any.
574-
func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) *infrav1alpha1.OpenStackServerSpec {
580+
func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) (*infrav1alpha1.OpenStackServerSpec, error) {
575581
bastion := openStackCluster.Spec.Bastion
576582
if bastion == nil {
577583
bastion = &infrav1.Bastion{}
@@ -586,9 +592,12 @@ func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) *i
586592
if bastion.AvailabilityZone != nil {
587593
az = *bastion.AvailabilityZone
588594
}
589-
openStackServerSpec := openStackMachineSpecToOpenStackServerSpec(bastion.Spec, openStackCluster.Spec.IdentityRef, compute.InstanceTags(bastion.Spec, openStackCluster), az, nil, getBastionSecurityGroupID(openStackCluster), openStackCluster.Status.Network.ID)
595+
openStackServerSpec, err := openStackMachineSpecToOpenStackServerSpec(bastion.Spec, openStackCluster.Spec.IdentityRef, compute.InstanceTags(bastion.Spec, openStackCluster), az, nil, getBastionSecurityGroupID(openStackCluster), openStackCluster.Status.Network)
596+
if err != nil {
597+
return nil, err
598+
}
590599

591-
return openStackServerSpec
600+
return openStackServerSpec, nil
592601
}
593602

594603
func bastionName(clusterResourceName string) string {

‎controllers/openstackmachine_controller.go

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,18 @@ func (r *OpenStackMachineReconciler) getMachineServer(ctx context.Context, openS
479479

480480
// openStackMachineSpecToOpenStackServerSpec converts an OpenStackMachineSpec to an OpenStackServerSpec.
481481
// It returns the OpenStackServerSpec object and an error if there is any.
482-
func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.OpenStackMachineSpec, identityRef infrav1.OpenStackIdentityReference, tags []string, failureDomain string, userDataRef *corev1.LocalObjectReference, defaultSecGroup *string, defaultNetworkID string) *infrav1alpha1.OpenStackServerSpec {
482+
func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.OpenStackMachineSpec, identityRef infrav1.OpenStackIdentityReference, tags []string, failureDomain string, userDataRef *corev1.LocalObjectReference, defaultSecGroup *string, clusterNetwork *infrav1.NetworkStatusWithSubnets) (*infrav1alpha1.OpenStackServerSpec, error) {
483+
// Determine default network ID if the cluster status exposes one.
484+
var defaultNetworkID string
485+
if clusterNetwork != nil {
486+
defaultNetworkID = clusterNetwork.ID
487+
}
488+
489+
// If no cluster network is available AND the machine spec did not define any ports with a network, we cannot choose a network.
490+
if defaultNetworkID == "" && len(openStackMachineSpec.Ports) == 0 {
491+
return nil, capoerrors.Terminal(infrav1.InvalidMachineSpecReason, "no network configured: cluster network is missing and machine spec does not define ports with a network")
492+
}
493+
483494
openStackServerSpec := &infrav1alpha1.OpenStackServerSpec{
484495
AdditionalBlockDevices: openStackMachineSpec.AdditionalBlockDevices,
485496
ConfigDrive: openStackMachineSpec.ConfigDrive,
@@ -535,7 +546,7 @@ func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.Ope
535546
}
536547
openStackServerSpec.Ports = serverPorts
537548

538-
return openStackServerSpec
549+
return openStackServerSpec, nil
539550
}
540551

541552
// reconcileMachineServer reconciles the OpenStackServer object for the OpenStackMachine.
@@ -584,18 +595,10 @@ func (r *OpenStackMachineReconciler) getOrCreateMachineServer(ctx context.Contex
584595
}
585596
return openStackCluster.Spec.IdentityRef
586597
}()
587-
// Determine default network ID if the cluster status exposes one.
588-
var defaultNetworkID string
589-
if openStackCluster.Status.Network != nil {
590-
defaultNetworkID = openStackCluster.Status.Network.ID
591-
}
592-
593-
// If no cluster network is available AND the machine spec did not define any ports with a network, we cannot choose a network.
594-
if defaultNetworkID == "" && len(openStackMachine.Spec.Ports) == 0 {
595-
return nil, capoerrors.Terminal(infrav1.InvalidMachineSpecReason, "no network configured: cluster network is missing and machine spec does not define ports with a network")
598+
machineServerSpec, err := openStackMachineSpecToOpenStackServerSpec(&openStackMachine.Spec, identityRef, compute.InstanceTags(&openStackMachine.Spec, openStackCluster), failureDomain, userDataRef, getManagedSecurityGroup(openStackCluster, machine), openStackCluster.Status.Network)
599+
if err != nil {
600+
return nil, err
596601
}
597-
598-
machineServerSpec := openStackMachineSpecToOpenStackServerSpec(&openStackMachine.Spec, identityRef, compute.InstanceTags(&openStackMachine.Spec, openStackCluster), failureDomain, userDataRef, getManagedSecurityGroup(openStackCluster, machine), defaultNetworkID)
599602
machineServer = &infrav1alpha1.OpenStackServer{
600603
ObjectMeta: metav1.ObjectMeta{
601604
Labels: map[string]string{

‎controllers/openstackmachine_controller_test.go

Lines changed: 95 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -77,24 +77,25 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
7777
},
7878
}
7979
portOptsWithAdditionalSecurityGroup := []infrav1.PortOpts{
80-
{
81-
Network: &infrav1.NetworkParam{
82-
ID: ptr.To(openStackCluster.Status.Network.ID),
83-
},
84-
SecurityGroups: []infrav1.SecurityGroupParam{
85-
{
86-
ID: ptr.To(extraSecurityGroupUUID),
87-
},
88-
},
89-
},
90-
}
80+
{
81+
Network: &infrav1.NetworkParam{
82+
ID: ptr.To(openStackCluster.Status.Network.ID),
83+
},
84+
SecurityGroups: []infrav1.SecurityGroupParam{
85+
{
86+
ID: ptr.To(extraSecurityGroupUUID),
87+
},
88+
},
89+
},
90+
}
9191
image := infrav1.ImageParam{Filter: &infrav1.ImageFilter{Name: ptr.To("my-image")}}
9292
tags := []string{"tag1", "tag2"}
9393
userData := &corev1.LocalObjectReference{Name: "server-data-secret"}
9494
tests := []struct {
95-
name string
96-
spec *infrav1.OpenStackMachineSpec
97-
want *infrav1alpha1.OpenStackServerSpec
95+
name string
96+
spec *infrav1.OpenStackMachineSpec
97+
want *infrav1alpha1.OpenStackServerSpec
98+
wantErr bool
9899
}{
99100
{
100101
name: "Test a minimum OpenStackMachineSpec to OpenStackServerSpec conversion",
@@ -155,68 +156,86 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
155156
},
156157
},
157158
{
158-
name: "Test an OpenStackMachineSpec to OpenStackServerSpec conversion with flavorID specified but not flavor",
159-
spec: &infrav1.OpenStackMachineSpec{
160-
FlavorID: ptr.To(flavorUUID),
161-
Image: image,
162-
SSHKeyName: sshKeyName,
163-
},
164-
want: &infrav1alpha1.OpenStackServerSpec{
165-
FlavorID: ptr.To(flavorUUID),
166-
IdentityRef: identityRef,
167-
Image: image,
168-
SSHKeyName: sshKeyName,
169-
Ports: portOpts,
170-
Tags: tags,
171-
UserDataRef: userData,
172-
},
173-
},
174-
{
175-
name: "Cluster network nil, machine defines port network and overrides SG",
176-
spec: &infrav1.OpenStackMachineSpec{
177-
Ports: []infrav1.PortOpts{{
178-
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
179-
}},
180-
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
181-
},
182-
want: &infrav1alpha1.OpenStackServerSpec{
183-
IdentityRef: identityRef,
184-
Ports: []infrav1.PortOpts{{
185-
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
186-
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
187-
}},
188-
Tags: tags,
189-
UserDataRef: userData,
190-
},
191-
},
192-
{
193-
name: "Cluster network nil, machine defines port network and falls back to cluster SG",
194-
spec: &infrav1.OpenStackMachineSpec{
195-
Ports: []infrav1.PortOpts{{
196-
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
197-
}},
198-
},
199-
want: &infrav1alpha1.OpenStackServerSpec{
200-
IdentityRef: identityRef,
201-
Ports: []infrav1.PortOpts{{
202-
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
203-
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(workerSecurityGroupUUID)}},
204-
}},
205-
Tags: tags,
206-
UserDataRef: userData,
207-
},
208-
},
159+
name: "Test an OpenStackMachineSpec to OpenStackServerSpec conversion with flavorID specified but not flavor",
160+
spec: &infrav1.OpenStackMachineSpec{
161+
FlavorID: ptr.To(flavorUUID),
162+
Image: image,
163+
SSHKeyName: sshKeyName,
164+
},
165+
want: &infrav1alpha1.OpenStackServerSpec{
166+
FlavorID: ptr.To(flavorUUID),
167+
IdentityRef: identityRef,
168+
Image: image,
169+
SSHKeyName: sshKeyName,
170+
Ports: portOpts,
171+
Tags: tags,
172+
UserDataRef: userData,
173+
},
174+
},
175+
{
176+
name: "Cluster network nil, machine defines port network and overrides SG",
177+
spec: &infrav1.OpenStackMachineSpec{
178+
Ports: []infrav1.PortOpts{{
179+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
180+
}},
181+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
182+
},
183+
want: &infrav1alpha1.OpenStackServerSpec{
184+
IdentityRef: identityRef,
185+
Ports: []infrav1.PortOpts{{
186+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
187+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
188+
}},
189+
Tags: tags,
190+
UserDataRef: userData,
191+
},
192+
},
193+
{
194+
name: "Cluster network nil, machine defines port network and falls back to cluster SG",
195+
spec: &infrav1.OpenStackMachineSpec{
196+
Ports: []infrav1.PortOpts{{
197+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
198+
}},
199+
},
200+
want: &infrav1alpha1.OpenStackServerSpec{
201+
IdentityRef: identityRef,
202+
Ports: []infrav1.PortOpts{{
203+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
204+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(workerSecurityGroupUUID)}},
205+
}},
206+
Tags: tags,
207+
UserDataRef: userData,
208+
},
209+
},
210+
{
211+
name: "Error case: no cluster network and no machine ports",
212+
spec: &infrav1.OpenStackMachineSpec{
213+
Flavor: ptr.To(flavorName),
214+
Image: image,
215+
SSHKeyName: sshKeyName,
216+
// No ports defined
217+
},
218+
want: nil,
219+
wantErr: true,
220+
},
209221
}
210222
for i := range tests {
211223
tt := tests[i]
212224
t.Run(tt.name, func(t *testing.T) {
213-
defaultNetID := ""
214-
if openStackCluster.Status.Network != nil {
215-
defaultNetID = openStackCluster.Status.Network.ID
225+
// For the error test case, simulate no cluster network
226+
var clusterNetwork *infrav1.NetworkStatusWithSubnets
227+
if tt.wantErr && tt.name == "Error case: no cluster network and no machine ports" {
228+
clusterNetwork = nil // Simulate nil cluster network for HCP scenario
229+
} else {
230+
clusterNetwork = openStackCluster.Status.Network
216231
}
217232

218-
spec := openStackMachineSpecToOpenStackServerSpec(tt.spec, identityRef, tags, "", userData, &openStackCluster.Status.WorkerSecurityGroup.ID, defaultNetID)
219-
if !reflect.DeepEqual(spec, tt.want) {
233+
spec, err := openStackMachineSpecToOpenStackServerSpec(tt.spec, identityRef, tags, "", userData, &openStackCluster.Status.WorkerSecurityGroup.ID, clusterNetwork)
234+
if (err != nil) != tt.wantErr {
235+
t.Errorf("openStackMachineSpecToOpenStackServerSpec() error = %v, wantErr %v", err, tt.wantErr)
236+
return
237+
}
238+
if !tt.wantErr && !reflect.DeepEqual(spec, tt.want) {
220239
t.Errorf("openStackMachineSpecToOpenStackServerSpec() got = %+v, want %+v", spec, tt.want)
221240
}
222241
})
@@ -332,82 +351,23 @@ func TestOpenStackMachineSpecToOpenStackServerSpec_NilNetworkCases(t *testing.T)
332351
managedSecurityGroupID = &tt.openStackCluster.Status.WorkerSecurityGroup.ID
333352
}
334353

335-
spec := openStackMachineSpecToOpenStackServerSpec(
354+
spec, err := openStackMachineSpecToOpenStackServerSpec(
336355
tt.spec,
337356
identityRef,
338357
tags,
339358
"", // failureDomain
340359
userData,
341360
managedSecurityGroupID,
342-
defaultNetworkID,
361+
tt.openStackCluster.Status.Network,
343362
)
363+
if err != nil {
364+
t.Errorf("Unexpected error: %v", err)
365+
return
366+
}
344367

345368
if !reflect.DeepEqual(spec.Ports, tt.expectedPorts) {
346369
t.Errorf("Expected ports = %+v, got %+v", tt.expectedPorts, spec.Ports)
347370
}
348371
})
349372
}
350373
}
351-
352-
func TestValidateNetworkConfiguration(t *testing.T) {
353-
tests := []struct {
354-
name string
355-
clusterNetworkID string
356-
machinePortsCount int
357-
expectError bool
358-
expectedErrorMsg string
359-
description string
360-
}{
361-
{
362-
name: "Valid: cluster has network, machine has no explicit ports",
363-
clusterNetworkID: networkUUID,
364-
machinePortsCount: 0,
365-
expectError: false,
366-
description: "Should succeed when cluster provides default network",
367-
},
368-
{
369-
name: "Valid: no cluster network, machine defines explicit ports",
370-
clusterNetworkID: "",
371-
machinePortsCount: 1,
372-
expectError: false,
373-
description: "Should succeed when machine defines its own networking",
374-
},
375-
{
376-
name: "Invalid: no cluster network, no machine ports",
377-
clusterNetworkID: "",
378-
machinePortsCount: 0,
379-
expectError: true,
380-
expectedErrorMsg: "no network configured: cluster network is missing and machine spec does not define ports with a network",
381-
description: "Should fail with terminal error when no networking is configured anywhere",
382-
},
383-
{
384-
name: "Valid: cluster network and machine ports both defined",
385-
clusterNetworkID: networkUUID,
386-
machinePortsCount: 2,
387-
expectError: false,
388-
description: "Should succeed when both cluster and machine define networking",
389-
},
390-
}
391-
392-
for _, tt := range tests {
393-
t.Run(tt.name, func(t *testing.T) {
394-
// Simulate the validation logic from the controller
395-
hasClusterNetwork := tt.clusterNetworkID != ""
396-
hasMachinePorts := tt.machinePortsCount > 0
397-
398-
shouldFail := !hasClusterNetwork && !hasMachinePorts
399-
400-
if shouldFail != tt.expectError {
401-
t.Errorf("Expected error: %v, but validation result: %v", tt.expectError, shouldFail)
402-
}
403-
404-
if tt.expectError && shouldFail {
405-
// In the real controller, this would be a terminal error
406-
actualError := "no network configured: cluster network is missing and machine spec does not define ports with a network"
407-
if actualError != tt.expectedErrorMsg {
408-
t.Errorf("Expected error message: %q, got: %q", tt.expectedErrorMsg, actualError)
409-
}
410-
}
411-
})
412-
}
413-
}

‎docs/book/src/development/development.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -562,5 +562,3 @@ kubectl get openstackservers
562562
```
563563

564564
This object is immutable and is created by the controller when a machine or a bastion is created. The `OpenStackServer` object is deleted when the machine or the bastion is deleted.
565-
566-

0 commit comments

Comments
 (0)