Skip to content

Commit 0343b92

Browse files
mdboothmdbooth
committed
Fix down-conversion of IdentityRef
We were not setting IdentityRef.Kind when down-converting an object with no previous version annotation, which results in a validation error.
1 parent ffe572e commit 0343b92

File tree

8 files changed

+93
-26
lines changed

8 files changed

+93
-26
lines changed

‎api/v1alpha6/openstackcluster_conversion.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i
365365
}
366366

367367
out.CloudName = in.IdentityRef.CloudName
368-
out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
368+
out.IdentityRef = &OpenStackIdentityReference{}
369+
if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil {
370+
return err
371+
}
369372

370373
if in.APIServerLoadBalancer != nil {
371374
if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil {

‎api/v1alpha6/openstackmachine_conversion.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,6 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *i
373373
}
374374

375375
if in.IdentityRef != nil {
376-
out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
377376
out.CloudName = in.IdentityRef.CloudName
378377
}
379378

‎api/v1alpha6/types_conversion.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,8 @@ func Convert_v1alpha6_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef
534534

535535
func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error {
536536
out.Name = in.Name
537+
// Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required
538+
out.Kind = "Secret"
537539
return nil
538540
}
539541

‎api/v1alpha7/openstackcluster_conversion.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -362,7 +362,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *i
362362
}
363363

364364
out.CloudName = in.IdentityRef.CloudName
365-
out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name}
365+
out.IdentityRef = &OpenStackIdentityReference{}
366+
if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil {
367+
return err
368+
}
366369

367370
if in.APIServerLoadBalancer != nil {
368371
if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha7_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil {

‎api/v1alpha7/types_conversion.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,8 @@ func Convert_v1alpha7_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef
536536

537537
func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error {
538538
out.Name = in.Name
539+
// Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required
540+
out.Kind = "Secret"
539541
return nil
540542
}
541543

‎test/e2e/suites/apivalidations/openstackcluster_test.go‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package apivalidations
1919
import (
2020
. "github.com/onsi/ginkgo/v2"
2121
. "github.com/onsi/gomega"
22+
"github.com/onsi/gomega/format"
2223
corev1 "k8s.io/api/core/v1"
2324
"k8s.io/apimachinery/pkg/types"
2425
"k8s.io/utils/ptr"
@@ -32,16 +33,6 @@ import (
3233
var _ = Describe("OpenStackCluster API validations", func() {
3334
var namespace *corev1.Namespace
3435

35-
create := func(obj client.Object) error {
36-
err := k8sClient.Create(ctx, obj)
37-
if err == nil {
38-
DeferCleanup(func() error {
39-
return k8sClient.Delete(ctx, obj)
40-
})
41-
}
42-
return err
43-
}
44-
4536
BeforeEach(func() {
4637
namespace = createNamespace()
4738
})
@@ -57,12 +48,12 @@ var _ = Describe("OpenStackCluster API validations", func() {
5748
})
5849

5950
It("should allow the smallest permissible cluster spec", func() {
60-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
51+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
6152
})
6253

6354
It("should only allow controlPlaneEndpoint to be set once", func() {
6455
By("Creating a bare cluster")
65-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
56+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
6657

6758
By("Setting the control plane endpoint")
6859
cluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{
@@ -78,12 +69,12 @@ var _ = Describe("OpenStackCluster API validations", func() {
7869

7970
It("should allow an empty managed security groups definition", func() {
8071
cluster.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{}
81-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
72+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
8273
})
8374

8475
It("should default enabled to true if APIServerLoadBalancer is specified without enabled=true", func() {
8576
cluster.Spec.APIServerLoadBalancer = &infrav1.APIServerLoadBalancer{}
86-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
77+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
8778

8879
// Fetch the cluster and check the defaulting
8980
fetchedCluster := &infrav1.OpenStackCluster{}
@@ -94,7 +85,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
9485
})
9586

9687
It("should not default APIServerLoadBalancer if it is not specifid", func() {
97-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
88+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
9889

9990
// Fetch the cluster and check the defaulting
10091
fetchedCluster := &infrav1.OpenStackCluster{}
@@ -115,19 +106,19 @@ var _ = Describe("OpenStackCluster API validations", func() {
115106
},
116107
},
117108
}
118-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
109+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
119110
})
120111

121112
It("should not allow bastion.enabled=true without a spec", func() {
122113
cluster.Spec.Bastion = &infrav1.Bastion{
123114
Enabled: ptr.To(true),
124115
}
125-
Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
116+
Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
126117
})
127118

128119
It("should not allow an empty Bastion", func() {
129120
cluster.Spec.Bastion = &infrav1.Bastion{}
130-
Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
121+
Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
131122
})
132123

133124
It("should default bastion.enabled=true", func() {
@@ -140,7 +131,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
140131
},
141132
},
142133
}
143-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should not succeed")
134+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should not succeed")
144135

145136
// Fetch the cluster and check the defaulting
146137
fetchedCluster := &infrav1.OpenStackCluster{}
@@ -161,7 +152,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
161152
},
162153
FloatingIP: ptr.To("10.0.0.0"),
163154
}
164-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
155+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
165156
})
166157

167158
It("should not allow non-IPv4 as bastion floating IP", func() {
@@ -175,7 +166,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
175166
},
176167
FloatingIP: ptr.To("foobar"),
177168
}
178-
Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
169+
Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
179170
})
180171
})
181172

@@ -195,7 +186,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
195186
Kind: "FakeKind",
196187
Name: "identity-ref",
197188
}
198-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
189+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
199190

200191
// Fetch the infrav1 version of the cluster
201192
infrav1Cluster := &infrav1.OpenStackCluster{}
@@ -217,7 +208,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
217208

218209
It("should not enable an explicitly disabled bastion when converting to v1beta1", func() {
219210
cluster.Spec.Bastion = &infrav1alpha7.Bastion{Enabled: false}
220-
Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
211+
Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
221212

222213
// Fetch the infrav1 version of the cluster
223214
infrav1Cluster := &infrav1.OpenStackCluster{}
@@ -232,5 +223,25 @@ var _ = Describe("OpenStackCluster API validations", func() {
232223
Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed")
233224
Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled")
234225
})
226+
227+
It("should downgrade cleanly from infrav1", func() {
228+
infrav1Cluster := &infrav1.OpenStackCluster{}
229+
infrav1Cluster.Namespace = namespace.Name
230+
infrav1Cluster.GenerateName = clusterNamePrefix
231+
infrav1Cluster.Spec.IdentityRef.CloudName = "test-cloud"
232+
infrav1Cluster.Spec.IdentityRef.Name = "test-credentials"
233+
Expect(createObj(infrav1Cluster)).To(Succeed(), "infrav1 OpenStackCluster creation should succeed")
234+
235+
// Just fetching the object as v1alpha6 doesn't trigger
236+
// validation failure, so we first fetch it and then
237+
// patch the object with identical contents. The patch
238+
// triggers a validation failure.
239+
cluster := &infrav1alpha7.OpenStackCluster{} //nolint: staticcheck
240+
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed")
241+
242+
setObjectGVK(cluster)
243+
cluster.ManagedFields = nil
244+
Expect(k8sClient.Patch(ctx, cluster, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(cluster, 4))
245+
})
235246
})
236247
})

‎test/e2e/suites/apivalidations/openstackmachine_test.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,13 @@ import (
2121

2222
. "github.com/onsi/ginkgo/v2"
2323
. "github.com/onsi/gomega"
24+
"github.com/onsi/gomega/format"
2425
corev1 "k8s.io/api/core/v1"
26+
"k8s.io/apimachinery/pkg/types"
2527
"k8s.io/utils/ptr"
28+
"sigs.k8s.io/controller-runtime/pkg/client"
2629

30+
infrav1alpha7 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7"
2731
infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
2832
)
2933

@@ -332,4 +336,29 @@ var _ = Describe("OpenStackMachine API validations", func() {
332336
Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "Creating a machine with an additional block device with an empty name availability zone should fail")
333337
})
334338
})
339+
340+
Context("v1alpha7", func() {
341+
It("should downgrade cleanly from infrav1", func() {
342+
infrav1Machine := &infrav1.OpenStackMachine{}
343+
infrav1Machine.Namespace = namespace.Name
344+
infrav1Machine.GenerateName = clusterNamePrefix
345+
infrav1Machine.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{
346+
CloudName: "test-cloud",
347+
Name: "test-credentials",
348+
}
349+
infrav1Machine.Spec.Image.ID = ptr.To("de9872ee-0c2c-44ed-9414-90163c8b0e0d")
350+
Expect(createObj(infrav1Machine)).To(Succeed(), "infrav1 OpenStackMachine creation should succeed")
351+
352+
// Just fetching the object as v1alpha7 doesn't trigger
353+
// validation failure, so we first fetch it and then
354+
// patch the object with identical contents. The patch
355+
// triggers a validation failure.
356+
machine := &infrav1alpha7.OpenStackMachine{} //nolint: staticcheck
357+
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Machine.Name, Namespace: infrav1Machine.Namespace}, machine)).To(Succeed(), "OpenStackMachine fetch should succeed")
358+
359+
setObjectGVK(machine)
360+
machine.ManagedFields = nil
361+
Expect(k8sClient.Patch(ctx, machine, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(machine, 4))
362+
})
363+
})
335364
})

‎test/e2e/suites/apivalidations/suite_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,3 +161,21 @@ func createNamespace() *corev1.Namespace {
161161
By(fmt.Sprintf("Using namespace %s", namespace.Name))
162162
return &namespace
163163
}
164+
165+
func createObj(obj client.Object) error {
166+
err := k8sClient.Create(ctx, obj)
167+
if err == nil {
168+
DeferCleanup(func() error {
169+
return k8sClient.Delete(ctx, obj)
170+
})
171+
}
172+
return err
173+
}
174+
175+
func setObjectGVK(obj runtime.Object) {
176+
gvk, unversioned, err := testScheme.ObjectKinds(obj)
177+
Expect(unversioned).To(BeFalse(), "Object is considered unversioned")
178+
Expect(err).ToNot(HaveOccurred(), "Error fetching gvk for Object")
179+
Expect(gvk).To(HaveLen(1), "Object should have only one gvk")
180+
obj.GetObjectKind().SetGroupVersionKind(gvk[0])
181+
}

0 commit comments

Comments
 (0)