From ade7f7708a21029264d95ecb5b5d481327d150e6 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 4 Jul 2024 12:00:12 +0100 Subject: [PATCH] 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. --- api/v1alpha6/openstackcluster_conversion.go | 5 +- api/v1alpha6/openstackmachine_conversion.go | 1 - api/v1alpha6/types_conversion.go | 2 + api/v1alpha7/openstackcluster_conversion.go | 5 +- api/v1alpha7/types_conversion.go | 2 + .../apivalidations/openstackcluster_test.go | 81 +++++++++++++------ .../apivalidations/openstackmachine_test.go | 55 +++++++++++++ test/e2e/suites/apivalidations/suite_test.go | 18 +++++ 8 files changed, 141 insertions(+), 28 deletions(-) diff --git a/api/v1alpha6/openstackcluster_conversion.go b/api/v1alpha6/openstackcluster_conversion.go index 3bbb3f97d8..28108d1648 100644 --- a/api/v1alpha6/openstackcluster_conversion.go +++ b/api/v1alpha6/openstackcluster_conversion.go @@ -364,7 +364,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i } out.CloudName = in.IdentityRef.CloudName - out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name} + out.IdentityRef = &OpenStackIdentityReference{} + if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil { + return err + } if in.APIServerLoadBalancer != nil { if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil { diff --git a/api/v1alpha6/openstackmachine_conversion.go b/api/v1alpha6/openstackmachine_conversion.go index aed2d3b7d4..47a757bd7d 100644 --- a/api/v1alpha6/openstackmachine_conversion.go +++ b/api/v1alpha6/openstackmachine_conversion.go @@ -373,7 +373,6 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *i } if in.IdentityRef != nil { - out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name} out.CloudName = in.IdentityRef.CloudName } diff --git a/api/v1alpha6/types_conversion.go b/api/v1alpha6/types_conversion.go index 041ce9eb3a..30c8618cc8 100644 --- a/api/v1alpha6/types_conversion.go +++ b/api/v1alpha6/types_conversion.go @@ -534,6 +534,8 @@ func Convert_v1alpha6_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error { out.Name = in.Name + // Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required + out.Kind = "Secret" return nil } diff --git a/api/v1alpha7/openstackcluster_conversion.go b/api/v1alpha7/openstackcluster_conversion.go index 6b4a82a9bf..cfdea6e20d 100644 --- a/api/v1alpha7/openstackcluster_conversion.go +++ b/api/v1alpha7/openstackcluster_conversion.go @@ -361,7 +361,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *i } out.CloudName = in.IdentityRef.CloudName - out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name} + out.IdentityRef = &OpenStackIdentityReference{} + if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil { + return err + } if in.APIServerLoadBalancer != nil { if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha7_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil { diff --git a/api/v1alpha7/types_conversion.go b/api/v1alpha7/types_conversion.go index 806e145806..db640e1e95 100644 --- a/api/v1alpha7/types_conversion.go +++ b/api/v1alpha7/types_conversion.go @@ -536,6 +536,8 @@ func Convert_v1alpha7_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error { out.Name = in.Name + // Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required + out.Kind = "Secret" return nil } diff --git a/test/e2e/suites/apivalidations/openstackcluster_test.go b/test/e2e/suites/apivalidations/openstackcluster_test.go index 80320e167c..76bcbb40bf 100644 --- a/test/e2e/suites/apivalidations/openstackcluster_test.go +++ b/test/e2e/suites/apivalidations/openstackcluster_test.go @@ -19,6 +19,7 @@ package apivalidations import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -33,16 +34,6 @@ import ( var _ = Describe("OpenStackCluster API validations", func() { var namespace *corev1.Namespace - create := func(obj client.Object) error { - err := k8sClient.Create(ctx, obj) - if err == nil { - DeferCleanup(func() error { - return k8sClient.Delete(ctx, obj) - }) - } - return err - } - BeforeEach(func() { namespace = createNamespace() }) @@ -58,12 +49,12 @@ var _ = Describe("OpenStackCluster API validations", func() { }) It("should allow the smallest permissible cluster spec", func() { - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should only allow controlPlaneEndpoint to be set once", func() { By("Creating a bare cluster") - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") By("Setting the control plane endpoint") cluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{ @@ -79,12 +70,12 @@ var _ = Describe("OpenStackCluster API validations", func() { It("should allow an empty managed security groups definition", func() { cluster.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should default enabled to true if APIServerLoadBalancer is specified without enabled=true", func() { cluster.Spec.APIServerLoadBalancer = &infrav1.APIServerLoadBalancer{} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the cluster and check the defaulting fetchedCluster := &infrav1.OpenStackCluster{} @@ -95,7 +86,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }) It("should not default APIServerLoadBalancer if it is not specifid", func() { - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the cluster and check the defaulting fetchedCluster := &infrav1.OpenStackCluster{} @@ -116,19 +107,19 @@ var _ = Describe("OpenStackCluster API validations", func() { }, }, } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should not allow bastion.enabled=true without a spec", func() { cluster.Spec.Bastion = &infrav1.Bastion{ Enabled: ptr.To(true), } - Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") }) It("should not allow an empty Bastion", func() { cluster.Spec.Bastion = &infrav1.Bastion{} - Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") }) It("should default bastion.enabled=true", func() { @@ -141,7 +132,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }, }, } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should not succeed") // Fetch the cluster and check the defaulting fetchedCluster := &infrav1.OpenStackCluster{} @@ -162,7 +153,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }, FloatingIP: ptr.To("10.0.0.0"), } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should not allow non-IPv4 as bastion floating IP", func() { @@ -176,7 +167,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }, FloatingIP: ptr.To("foobar"), } - Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") }) }) @@ -196,7 +187,7 @@ var _ = Describe("OpenStackCluster API validations", func() { Kind: "FakeKind", Name: "identity-ref", } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -218,7 +209,7 @@ var _ = Describe("OpenStackCluster API validations", func() { It("should not enable an explicitly disabled bastion when converting to v1beta1", func() { cluster.Spec.Bastion = &infrav1alpha7.Bastion{Enabled: false} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -233,6 +224,26 @@ var _ = Describe("OpenStackCluster API validations", func() { Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed") Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled") }) + + It("should downgrade cleanly from infrav1", func() { + infrav1Cluster := &infrav1.OpenStackCluster{} + infrav1Cluster.Namespace = namespace.Name + infrav1Cluster.GenerateName = clusterNamePrefix + infrav1Cluster.Spec.IdentityRef.CloudName = "test-cloud" + infrav1Cluster.Spec.IdentityRef.Name = "test-credentials" + Expect(createObj(infrav1Cluster)).To(Succeed(), "infrav1 OpenStackCluster creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + cluster := &infrav1alpha7.OpenStackCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed") + + setObjectGVK(cluster) + cluster.ManagedFields = nil + Expect(k8sClient.Patch(ctx, cluster, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(cluster, 4)) + }) }) Context("v1alpha6", func() { @@ -251,7 +262,7 @@ var _ = Describe("OpenStackCluster API validations", func() { Kind: "FakeKind", Name: "identity-ref", } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -273,7 +284,7 @@ var _ = Describe("OpenStackCluster API validations", func() { It("should not enable an explicitly disabled bastion when converting to v1beta1", func() { cluster.Spec.Bastion = &infrav1alpha6.Bastion{Enabled: false} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -288,5 +299,25 @@ var _ = Describe("OpenStackCluster API validations", func() { Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed") Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled") }) + + It("should downgrade cleanly from infrav1", func() { + infrav1Cluster := &infrav1.OpenStackCluster{} + infrav1Cluster.Namespace = namespace.Name + infrav1Cluster.GenerateName = clusterNamePrefix + infrav1Cluster.Spec.IdentityRef.CloudName = "test-cloud" + infrav1Cluster.Spec.IdentityRef.Name = "test-credentials" + Expect(createObj(infrav1Cluster)).To(Succeed(), "infrav1 OpenStackCluster creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + cluster := &infrav1alpha6.OpenStackCluster{} //nolint:staticcheck + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed") + + setObjectGVK(cluster) + cluster.ManagedFields = nil + Expect(k8sClient.Patch(ctx, cluster, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(cluster, 4)) + }) }) }) diff --git a/test/e2e/suites/apivalidations/openstackmachine_test.go b/test/e2e/suites/apivalidations/openstackmachine_test.go index 568a82b8d6..7e428bb374 100644 --- a/test/e2e/suites/apivalidations/openstackmachine_test.go +++ b/test/e2e/suites/apivalidations/openstackmachine_test.go @@ -21,9 +21,14 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + infrav1alpha6 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha6" + infrav1alpha7 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" ) @@ -278,4 +283,54 @@ var _ = Describe("OpenStackMachine API validations", func() { Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "Creating a machine with an additional block device with an empty name availability zone should fail") }) }) + + Context("v1alpha7", func() { + It("should downgrade cleanly from infrav1", func() { + infrav1Machine := &infrav1.OpenStackMachine{} + infrav1Machine.Namespace = namespace.Name + infrav1Machine.GenerateName = clusterNamePrefix + infrav1Machine.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{ + CloudName: "test-cloud", + Name: "test-credentials", + } + infrav1Machine.Spec.Image.ID = ptr.To("de9872ee-0c2c-44ed-9414-90163c8b0e0d") + Expect(createObj(infrav1Machine)).To(Succeed(), "infrav1 OpenStackMachine creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + machine := &infrav1alpha7.OpenStackMachine{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Machine.Name, Namespace: infrav1Machine.Namespace}, machine)).To(Succeed(), "OpenStackMachine fetch should succeed") + + setObjectGVK(machine) + machine.ManagedFields = nil + Expect(k8sClient.Patch(ctx, machine, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(machine, 4)) + }) + }) + + Context("v1alpha6", func() { + It("should downgrade cleanly from infrav1", func() { + infrav1Machine := &infrav1.OpenStackMachine{} + infrav1Machine.Namespace = namespace.Name + infrav1Machine.GenerateName = clusterNamePrefix + infrav1Machine.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{ + CloudName: "test-cloud", + Name: "test-credentials", + } + infrav1Machine.Spec.Image.ID = ptr.To("de9872ee-0c2c-44ed-9414-90163c8b0e0d") + Expect(createObj(infrav1Machine)).To(Succeed(), "infrav1 OpenStackMachine creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + machine := &infrav1alpha6.OpenStackMachine{} //nolint:staticcheck + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Machine.Name, Namespace: infrav1Machine.Namespace}, machine)).To(Succeed(), "OpenStackMachine fetch should succeed") + + setObjectGVK(machine) + machine.ManagedFields = nil + Expect(k8sClient.Patch(ctx, machine, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(machine, 4)) + }) + }) }) diff --git a/test/e2e/suites/apivalidations/suite_test.go b/test/e2e/suites/apivalidations/suite_test.go index beac7d11aa..c8cca42d73 100644 --- a/test/e2e/suites/apivalidations/suite_test.go +++ b/test/e2e/suites/apivalidations/suite_test.go @@ -165,3 +165,21 @@ func createNamespace() *corev1.Namespace { By(fmt.Sprintf("Using namespace %s", namespace.Name)) return &namespace } + +func createObj(obj client.Object) error { + err := k8sClient.Create(ctx, obj) + if err == nil { + DeferCleanup(func() error { + return k8sClient.Delete(ctx, obj) + }) + } + return err +} + +func setObjectGVK(obj runtime.Object) { + gvk, unversioned, err := testScheme.ObjectKinds(obj) + Expect(unversioned).To(BeFalse(), "Object is considered unversioned") + Expect(err).ToNot(HaveOccurred(), "Error fetching gvk for Object") + Expect(gvk).To(HaveLen(1), "Object should have only one gvk") + obj.GetObjectKind().SetGroupVersionKind(gvk[0]) +}