Merge pull request #2011 from shiftstack/issue2009

🐛 Fix idempotent restore when setting ControlPlaneEndpoint

Author: k8s-ci-robot

api/v1alpha6/openstackcluster_conversion.go

@@ -72,6 +72,18 @@ var v1alpha6OpenStackClusterRestorer = conversion.RestorerFor[*OpenStackCluster]
 			return &c.Spec
 		},
 		restorev1alpha6ClusterSpec,
+		// Filter out ControlPlaneEndpoint, which is written by the
+		// cluster controller
+		conversion.HashedFilterField[*OpenStackCluster](
+			func(s *OpenStackClusterSpec) *OpenStackClusterSpec {
+				if s.ControlPlaneEndpoint != (clusterv1.APIEndpoint{}) {
+					f := *s
+					f.ControlPlaneEndpoint = clusterv1.APIEndpoint{}
+					return &f
+				}
+				return s
+			},
+		),
 	),
 	"status": conversion.HashedFieldRestorer(
 		func(c *OpenStackCluster) *OpenStackClusterStatus {

api/v1alpha7/openstackcluster_conversion.go

@@ -78,12 +78,14 @@ var v1alpha7OpenStackClusterRestorer = conversion.RestorerFor[*OpenStackCluster]
 			return &c.Spec
 		},
 		restorev1alpha7ClusterSpec,
-		// Filter out Bastion, which is restored separately
-		conversion.HashedFilterField[*OpenStackCluster, OpenStackClusterSpec](
+		// Filter out Bastion, which is restored separately, and
+		// ControlPlaneEndpoint, which is written by the cluster controller
+		conversion.HashedFilterField[*OpenStackCluster](
 			func(s *OpenStackClusterSpec) *OpenStackClusterSpec {
-				if s.Bastion != nil {
+				if s.Bastion != nil || s.ControlPlaneEndpoint != (clusterv1.APIEndpoint{}) {
 					f := *s
 					f.Bastion = nil
+					f.ControlPlaneEndpoint = clusterv1.APIEndpoint{}
 					return &f
 				}
 				return s

test/e2e/suites/apivalidations/filters_test.go

@@ -47,7 +47,7 @@ var _ = Describe("Filter API validations", func() {
 		// Initialise a basic cluster object in the correct namespace
 		cluster = &infrav1.OpenStackCluster{}
 		cluster.Namespace = namespace.Name
-		cluster.GenerateName = "cluster-"
+		cluster.GenerateName = clusterNamePrefix
 	})
 
 	DescribeTable("Allow valid neutron filter tags", func(tags []infrav1.FilterByNeutronTags) {

test/e2e/suites/apivalidations/openstackcluster_test.go

@@ -25,6 +25,8 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"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"
 )
 
@@ -52,7 +54,7 @@ var _ = Describe("OpenStackCluster API validations", func() {
 			// Initialise a basic cluster object in the correct namespace
 			cluster = &infrav1.OpenStackCluster{}
 			cluster.Namespace = namespace.Name
-			cluster.GenerateName = "cluster-"
+			cluster.GenerateName = clusterNamePrefix
 		})
 
 		It("should allow the smallest permissible cluster spec", func() {
@@ -177,4 +179,78 @@ var _ = Describe("OpenStackCluster API validations", func() {
 			Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed")
 		})
 	})
+
+	Context("v1alpha7", func() {
+		var cluster *infrav1alpha7.OpenStackCluster
+
+		BeforeEach(func() {
+			// Initialise a basic cluster object in the correct namespace
+			cluster = &infrav1alpha7.OpenStackCluster{}
+			cluster.Namespace = namespace.Name
+			cluster.GenerateName = clusterNamePrefix
+		})
+
+		It("should restore cluster spec idempotently after controller writes to controlPlaneEndpoint", func() {
+			// Set identityRef.Kind, as it will be lost if the restorer does not execute
+			cluster.Spec.IdentityRef = &infrav1alpha7.OpenStackIdentityReference{
+				Kind: "FakeKind",
+				Name: "identity-ref",
+			}
+			Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
+
+			// Fetch the infrav1 version of the cluster
+			infrav1Cluster := &infrav1.OpenStackCluster{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, infrav1Cluster)).To(Succeed(), "OpenStackCluster fetch should succeed")
+
+			// Update the infrav1 cluster to set the control plane endpoint
+			infrav1Cluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{
+				Host: "foo",
+				Port: 1234,
+			}
+			Expect(k8sClient.Update(ctx, infrav1Cluster)).To(Succeed(), "Setting control plane endpoint should succeed")
+
+			// Fetch the v1alpha7 version of the cluster and ensure that both the new control plane endpoint and the identityRef.Kind are present
+			cluster = &infrav1alpha7.OpenStackCluster{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed")
+			Expect(cluster.Spec.ControlPlaneEndpoint).To(Equal(*infrav1Cluster.Spec.ControlPlaneEndpoint), "Control plane endpoint should be restored")
+			Expect(cluster.Spec.IdentityRef.Kind).To(Equal("FakeKind"), "IdentityRef.Kind should be restored")
+		})
+	})
+
+	Context("v1alpha6", func() {
+		var cluster *infrav1alpha6.OpenStackCluster //nolint:staticcheck
+
+		BeforeEach(func() {
+			// Initialise a basic cluster object in the correct namespace
+			cluster = &infrav1alpha6.OpenStackCluster{} //nolint:staticcheck
+			cluster.Namespace = namespace.Name
+			cluster.GenerateName = clusterNamePrefix
+		})
+
+		It("should restore cluster spec idempotently after controller writes to controlPlaneEndpoint", func() {
+			// Set identityRef.Kind, as it will be lost if the restorer does not execute
+			cluster.Spec.IdentityRef = &infrav1alpha6.OpenStackIdentityReference{
+				Kind: "FakeKind",
+				Name: "identity-ref",
+			}
+			Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed")
+
+			// Fetch the infrav1 version of the cluster
+			infrav1Cluster := &infrav1.OpenStackCluster{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, infrav1Cluster)).To(Succeed(), "OpenStackCluster fetch should succeed")
+
+			// Update the infrav1 cluster to set the control plane endpoint
+			infrav1Cluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{
+				Host: "foo",
+				Port: 1234,
+			}
+			Expect(k8sClient.Update(ctx, infrav1Cluster)).To(Succeed(), "Setting control plane endpoint should succeed")
+
+			// Fetch the v1alpha6 version of the cluster and ensure that both the new control plane endpoint and the identityRef.Kind are present
+			cluster = &infrav1alpha6.OpenStackCluster{} //nolint:staticcheck
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed")
+			Expect(cluster.Spec.ControlPlaneEndpoint).To(Equal(*infrav1Cluster.Spec.ControlPlaneEndpoint), "Control plane endpoint should be restored")
+			Expect(cluster.Spec.IdentityRef.Kind).To(Equal("FakeKind"), "IdentityRef.Kind should be restored")
+		})
+	})
 })

test/e2e/suites/apivalidations/suite_test.go

@@ -22,6 +22,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"testing"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -56,6 +57,10 @@ var (
 	mgrDone    chan struct{}
 )
 
+const (
+	clusterNamePrefix = "cluster-"
+)
+
 func TestAPIs(t *testing.T) {
 	RegisterFailHandler(Fail)
 
@@ -144,7 +149,7 @@ var _ = BeforeSuite(func() {
 	DeferCleanup(func() {
 		By("Tearing down manager")
 		mgrCancel()
-		Eventually(mgrDone).Should(BeClosed(), "Manager should stop")
+		Eventually(mgrDone).WithTimeout(time.Second*5).Should(BeClosed(), "Manager should stop")
 	})
 })