Skip to content

Commit ad24391

Browse files
jfpucheujfpucheu
committed
fix: address reviewer feedback from bnallapeta
- Move reconcileAllowedAddressPairs before imageID nil check so it is never skipped by the early return in reconcileNormal - Add missing RBAC marker for openstackservers (get;list;watch) - Add webhook test cases for allowedAddressPairs mutability: - changing only AAP is allowed - changing AAP + another field is rejected - adding a port is rejected - removing a port is rejected
1 parent aa84436 commit ad24391

3 files changed

Lines changed: 156 additions & 5 deletions

File tree

‎controllers/openstackmachinetemplate_controller.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ type OpenStackMachineTemplateReconciler struct {
7373
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates,verbs=get;list;watch
7474
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates/status,verbs=get;update;patch
7575
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,verbs=get;list;watch;patch
76+
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackservers,verbs=get;list;watch
7677
// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets,verbs=get;list;watch
7778
// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines,verbs=get;list;watch
7879

@@ -218,6 +219,12 @@ func (r *OpenStackMachineTemplateReconciler) reconcileNormal(ctx context.Context
218219
openStackMachineTemplate.Status.Capacity[corev1.ResourceStorage] = *resource.NewQuantity(storageBytes, resource.BinarySI)
219220
}
220221

222+
// reconcileAllowedAddressPairs is called independently of the image/flavor logic so that
223+
// it is never skipped by an early return (e.g. when imageID is not yet resolvable).
224+
if err := r.reconcileAllowedAddressPairs(ctx, scope, clusterName, openStackMachineTemplate); err != nil {
225+
return err
226+
}
227+
221228
imageID, err := computeService.GetImageID(ctx, r.Client, openStackMachineTemplate.Namespace, openStackMachineTemplate.Spec.Template.Spec.Image)
222229
if err != nil {
223230
return err
@@ -241,10 +248,6 @@ func (r *OpenStackMachineTemplateReconciler) reconcileNormal(ctx context.Context
241248
}
242249
}
243250

244-
if err := r.reconcileAllowedAddressPairs(ctx, scope, clusterName, openStackMachineTemplate); err != nil {
245-
return err
246-
}
247-
248251
return nil
249252
}
250253

‎controllers/openstackmachinetemplate_controller_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ import (
4242
"sigs.k8s.io/controller-runtime/pkg/client/fake"
4343
"sigs.k8s.io/controller-runtime/pkg/reconcile"
4444

45-
infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta2"
4645
infrav1alpha1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha1"
46+
infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta2"
4747
mockclient "sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock"
4848
"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
4949
)

‎pkg/webhooks/openstackmachine_webhook_test.go‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,154 @@ func TestOpenStackMachine_ValidateUpdate(t *testing.T) {
199199
},
200200
wantErr: false,
201201
},
202+
{
203+
name: "Changing only allowedAddressPairs is allowed",
204+
oldMachine: &infrav1.OpenStackMachine{
205+
Spec: infrav1.OpenStackMachineSpec{
206+
Flavor: ptr.To("m1.small"),
207+
Image: infrav1.ImageParam{
208+
Filter: &infrav1.ImageFilter{
209+
Name: ptr.To("ubuntu"),
210+
},
211+
},
212+
Ports: []infrav1.PortOpts{
213+
{
214+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
215+
AllowedAddressPairs: []infrav1.AddressPair{
216+
{IPAddress: "192.168.1.100"},
217+
},
218+
},
219+
},
220+
},
221+
},
222+
},
223+
newMachine: &infrav1.OpenStackMachine{
224+
Spec: infrav1.OpenStackMachineSpec{
225+
Flavor: ptr.To("m1.small"),
226+
Image: infrav1.ImageParam{
227+
Filter: &infrav1.ImageFilter{
228+
Name: ptr.To("ubuntu"),
229+
},
230+
},
231+
Ports: []infrav1.PortOpts{
232+
{
233+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
234+
AllowedAddressPairs: []infrav1.AddressPair{
235+
{IPAddress: "192.168.1.100"},
236+
{IPAddress: "192.168.1.200"},
237+
},
238+
},
239+
},
240+
},
241+
},
242+
},
243+
wantErr: false,
244+
},
245+
{
246+
name: "Changing allowedAddressPairs and another field is rejected",
247+
oldMachine: &infrav1.OpenStackMachine{
248+
Spec: infrav1.OpenStackMachineSpec{
249+
Flavor: ptr.To("m1.small"),
250+
Image: infrav1.ImageParam{
251+
Filter: &infrav1.ImageFilter{
252+
Name: ptr.To("ubuntu"),
253+
},
254+
},
255+
Ports: []infrav1.PortOpts{
256+
{
257+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
258+
AllowedAddressPairs: []infrav1.AddressPair{
259+
{IPAddress: "192.168.1.100"},
260+
},
261+
},
262+
},
263+
},
264+
},
265+
},
266+
newMachine: &infrav1.OpenStackMachine{
267+
Spec: infrav1.OpenStackMachineSpec{
268+
Flavor: ptr.To("m1.large"),
269+
Image: infrav1.ImageParam{
270+
Filter: &infrav1.ImageFilter{
271+
Name: ptr.To("ubuntu"),
272+
},
273+
},
274+
Ports: []infrav1.PortOpts{
275+
{
276+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
277+
AllowedAddressPairs: []infrav1.AddressPair{
278+
{IPAddress: "192.168.1.100"},
279+
{IPAddress: "192.168.1.200"},
280+
},
281+
},
282+
},
283+
},
284+
},
285+
},
286+
wantErr: true,
287+
},
288+
{
289+
name: "Adding a port is rejected",
290+
oldMachine: &infrav1.OpenStackMachine{
291+
Spec: infrav1.OpenStackMachineSpec{
292+
Flavor: ptr.To("m1.small"),
293+
Image: infrav1.ImageParam{
294+
Filter: &infrav1.ImageFilter{
295+
Name: ptr.To("ubuntu"),
296+
},
297+
},
298+
Ports: []infrav1.PortOpts{
299+
{Network: &infrav1.NetworkParam{ID: ptr.To("net-1")}},
300+
},
301+
},
302+
},
303+
newMachine: &infrav1.OpenStackMachine{
304+
Spec: infrav1.OpenStackMachineSpec{
305+
Flavor: ptr.To("m1.small"),
306+
Image: infrav1.ImageParam{
307+
Filter: &infrav1.ImageFilter{
308+
Name: ptr.To("ubuntu"),
309+
},
310+
},
311+
Ports: []infrav1.PortOpts{
312+
{Network: &infrav1.NetworkParam{ID: ptr.To("net-1")}},
313+
{Network: &infrav1.NetworkParam{ID: ptr.To("net-2")}},
314+
},
315+
},
316+
},
317+
wantErr: true,
318+
},
319+
{
320+
name: "Removing a port is rejected",
321+
oldMachine: &infrav1.OpenStackMachine{
322+
Spec: infrav1.OpenStackMachineSpec{
323+
Flavor: ptr.To("m1.small"),
324+
Image: infrav1.ImageParam{
325+
Filter: &infrav1.ImageFilter{
326+
Name: ptr.To("ubuntu"),
327+
},
328+
},
329+
Ports: []infrav1.PortOpts{
330+
{Network: &infrav1.NetworkParam{ID: ptr.To("net-1")}},
331+
{Network: &infrav1.NetworkParam{ID: ptr.To("net-2")}},
332+
},
333+
},
334+
},
335+
newMachine: &infrav1.OpenStackMachine{
336+
Spec: infrav1.OpenStackMachineSpec{
337+
Flavor: ptr.To("m1.small"),
338+
Image: infrav1.ImageParam{
339+
Filter: &infrav1.ImageFilter{
340+
Name: ptr.To("ubuntu"),
341+
},
342+
},
343+
Ports: []infrav1.PortOpts{
344+
{Network: &infrav1.NetworkParam{ID: ptr.To("net-1")}},
345+
},
346+
},
347+
},
348+
wantErr: true,
349+
},
202350
}
203351
for _, tt := range tests {
204352
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)