Merge pull request kubernetes-sigs#2053 from shiftstack/issue2050_backport

🐛 [release-0.10] Fix enabling of disabled bastion on upgrade to v1beta1

Author: k8s-ci-robot

api/v1alpha5/zz_generated.conversion.go

@@ -478,7 +478,10 @@ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
 
 	optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
 	optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
-	optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
+
+	if (*dst).Enabled != nil && !*(*dst).Enabled {
+		(*dst).Enabled = (*previous).Enabled
+	}
 }
 
 func Convert_v1alpha6_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error {

api/v1alpha6/openstackcluster_conversion.go

@@ -416,7 +416,10 @@ func restorev1beta1Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
 	restorev1beta1MachineSpec((*previous).Spec, (*dst).Spec)
 	optional.RestoreString(&(*previous).FloatingIP, &(*dst).FloatingIP)
 	optional.RestoreString(&(*previous).AvailabilityZone, &(*dst).AvailabilityZone)
-	optional.RestoreBool(&(*previous).Enabled, &(*dst).Enabled)
+
+	if (*dst).Enabled != nil && !*(*dst).Enabled {
+		(*dst).Enabled = (*previous).Enabled
+	}
 }
 
 func Convert_v1alpha7_Bastion_To_v1beta1_Bastion(in *Bastion, out *infrav1.Bastion, s apiconversion.Scope) error {

api/v1alpha6/zz_generated.conversion.go

@@ -795,7 +795,7 @@ type Bastion struct {
 	// waiting until the bastion has been deleted.
 	// +kubebuilder:default:=true
 	// +optional
-	Enabled optional.Bool `json:"enabled,omitempty"`
+	Enabled *bool `json:"enabled,omitempty"`
 
 	// Spec for the bastion itself
 	Spec *OpenStackMachineSpec `json:"spec,omitempty"`

api/v1alpha7/openstackcluster_conversion.go

@@ -215,6 +215,24 @@ var _ = Describe("OpenStackCluster API validations", func() {
 			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")
 		})
+
+		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")
+
+			// 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")
+
+			infrav1Bastion := infrav1Cluster.Spec.Bastion
+
+			// NOTE(mdbooth): It may be reasonable to remove the
+			// bastion if it is disabled with no other properties.
+			// It would be reasonable to update the assertions
+			// accordingly if we did that.
+			Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed")
+			Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled")
+		})
 	})
 
 	Context("v1alpha6", func() {
@@ -252,5 +270,23 @@ var _ = Describe("OpenStackCluster API validations", func() {
 			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")
 		})
+
+		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")
+
+			// 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")
+
+			infrav1Bastion := infrav1Cluster.Spec.Bastion
+
+			// NOTE(mdbooth): It may be reasonable to remove the
+			// bastion if it is disabled with no other properties.
+			// It would be reasonable to update the assertions
+			// accordingly if we did that.
+			Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed")
+			Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled")
+		})
 	})
 })