Merge pull request #2139 from shiftstack/issue2136-main

🐛 Fix down-conversion of IdentityRef

api/v1alpha6/openstackcluster_conversion.go

@@ -365,7 +365,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 {

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
 	}
 

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
 }
 

api/v1alpha7/openstackcluster_conversion.go

@@ -362,7 +362,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 {

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
 }
 

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"
@@ -32,16 +33,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()
 	})
@@ -57,12 +48,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{
@@ -78,12 +69,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{}
@@ -94,7 +85,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{}
@@ -115,19 +106,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() {
@@ -140,7 +131,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{}
@@ -161,7 +152,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() {
@@ -175,7 +166,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")
 		})
 	})
 
@@ -195,7 +186,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{}
@@ -217,7 +208,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{}
@@ -232,5 +223,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 := &infrav1alpha7.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))
+		})
 	})
 })

test/e2e/suites/apivalidations/openstackmachine_test.go

@@ -21,9 +21,13 @@ 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"
 
+	infrav1alpha7 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7"
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
 )
 
@@ -332,4 +336,29 @@ 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 v1alpha7 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{} //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))
+		})
+	})
 })

test/e2e/suites/apivalidations/suite_test.go

@@ -161,3 +161,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])
+}