Merge pull request #1866 from shiftstack/issue_1855

k8s-ci-robot · web-flow · commit 8ca470acf2cf · 2024-02-14T05:47:05.000-08:00
🐛 Prevent the bastion to be removed before it's been disabled
diff --git a/api/v1alpha8/openstackcluster_types.go b/api/v1alpha8/openstackcluster_types.go
@@ -155,8 +155,8 @@ type OpenStackClusterSpec struct {
 	// Bastion is the OpenStack instance to login the nodes
 	//
 	// As a rolling update is not ideal during a bastion host session, we
-	// prevent changes to a running bastion configuration. Set `enabled: false` to
-	// make changes.
+	// prevent changes to a running bastion configuration. To make changes, it's required
+	// to first set `enabled: false` which will remove the bastion and then changes can be made.
 	//+optional
 	Bastion *Bastion `json:"bastion,omitempty"`
 
diff --git a/api/v1alpha8/openstackcluster_webhook.go b/api/v1alpha8/openstackcluster_webhook.go
@@ -116,6 +116,13 @@ func (r *OpenStackCluster) ValidateUpdate(oldRaw runtime.Object) (admission.Warn
 		r.Spec.APIServerPort = 0
 	}
 
+	// Allow to remove the bastion spec only if it was disabled before.
+	if r.Spec.Bastion == nil {
+		if old.Spec.Bastion != nil && old.Spec.Bastion.Enabled {
+			allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "bastion"), "cannot be removed before disabling it"))
+		}
+	}
+
 	// Allow changes to the bastion spec.
 	old.Spec.Bastion = &Bastion{}
 	r.Spec.Bastion = &Bastion{}
diff --git a/api/v1alpha8/openstackcluster_webhook_test.go b/api/v1alpha8/openstackcluster_webhook_test.go
@@ -358,6 +358,42 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "Removing OpenStackCluster.Spec.Bastion when it is enabled is not allowed",
+			oldTemplate: &OpenStackCluster{
+				Spec: OpenStackClusterSpec{
+					Bastion: &Bastion{
+						Enabled: true,
+						Instance: OpenStackMachineSpec{
+							Flavor: "m1.small",
+							Image:  ImageFilter{Name: "ubuntu"},
+						},
+					},
+				},
+			},
+			newTemplate: &OpenStackCluster{
+				Spec: OpenStackClusterSpec{},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Removing OpenStackCluster.Spec.Bastion when it is disabled is allowed",
+			oldTemplate: &OpenStackCluster{
+				Spec: OpenStackClusterSpec{
+					Bastion: &Bastion{
+						Enabled: false,
+						Instance: OpenStackMachineSpec{
+							Flavor: "m1.small",
+							Image:  ImageFilter{Name: "ubuntu"},
+						},
+					},
+				},
+			},
+			newTemplate: &OpenStackCluster{
+				Spec: OpenStackClusterSpec{},
+			},
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml
@@ -4899,8 +4899,8 @@ spec:
 
 
                   As a rolling update is not ideal during a bastion host session, we
-                  prevent changes to a running bastion configuration. Set `enabled: false` to
-                  make changes.
+                  prevent changes to a running bastion configuration. To make changes, it's required
+                  to first set `enabled: false` which will remove the bastion and then changes can be made.
                 properties:
                   availabilityZone:
                     type: string
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml
@@ -2324,8 +2324,8 @@ spec:
 
 
                           As a rolling update is not ideal during a bastion host session, we
-                          prevent changes to a running bastion configuration. Set `enabled: false` to
-                          make changes.
+                          prevent changes to a running bastion configuration. To make changes, it's required
+                          to first set `enabled: false` which will remove the bastion and then changes can be made.
                         properties:
                           availabilityZone:
                             type: string
diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go
@@ -263,7 +263,10 @@ func deleteBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackClust
 			}
 		}
 
-		instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+		instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster.Name)
+		if err != nil {
+			return err
+		}
 		if err = computeService.DeleteInstance(openStackCluster, openStackCluster, instanceStatus, instanceSpec); err != nil {
 			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete bastion: %w", err))
 			return fmt.Errorf("failed to delete bastion: %w", err)
@@ -346,7 +349,10 @@ func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCl
 		return reconcile.Result{}, err
 	}
 
-	instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+	instanceSpec, err := bastionToInstanceSpec(openStackCluster, cluster.Name)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
 	bastionHash, err := compute.HashInstanceSpec(instanceSpec)
 	if err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed computing bastion hash from instance spec: %w", err)
@@ -437,7 +443,15 @@ func reconcileBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackCl
 	return ctrl.Result{}, nil
 }
 
-func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterName string) *compute.InstanceSpec {
+func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterName string) (*compute.InstanceSpec, error) {
+	if openStackCluster.Spec.Bastion == nil {
+		return nil, fmt.Errorf("bastion spec is nil")
+	}
+
+	if openStackCluster.Status.Bastion == nil {
+		return nil, fmt.Errorf("bastion status is nil")
+	}
+
 	instanceSpec := &compute.InstanceSpec{
 		Name:          bastionName(clusterName),
 		Flavor:        openStackCluster.Spec.Bastion.Instance.Flavor,
@@ -458,7 +472,7 @@ func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterNa
 
 	instanceSpec.Ports = openStackCluster.Spec.Bastion.Instance.Ports
 
-	return instanceSpec
+	return instanceSpec, nil
 }
 
 func bastionName(clusterName string) string {
diff --git a/docs/book/src/clusteropenstack/configuration.md b/docs/book/src/clusteropenstack/configuration.md
@@ -35,6 +35,8 @@
   - [Custom pod network CIDR](#custom-pod-network-cidr)
   - [Accessing nodes through the bastion host via SSH](#accessing-nodes-through-the-bastion-host-via-ssh)
     - [Enabling the bastion host](#enabling-the-bastion-host)
+    - [Making changes to the bastion host](#making-changes-to-the-bastion-host)
+    - [Disabling the bastion](#disabling-the-bastion)
     - [Obtain floating IP address of the bastion node](#obtain-floating-ip-address-of-the-bastion-node)
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
@@ -654,6 +656,19 @@ spec:
 
 If `managedSecurityGroups: true`, security group rule opening 22/tcp is added to security groups for bastion, controller, and worker nodes respectively. Otherwise, you have to add `securityGroups` to the `bastion` in `OpenStackCluster` spec and `OpenStackMachineTemplate` spec template respectively.
 
+### Making changes to the bastion host
+
+Changes can be made to the bastion instance, like for example changing the flavor.
+First, you have to disable the bastion host by setting `enabled: false` in the `OpenStackCluster.Spec.Bastion` field.
+The bastion will be deleted, you can check the status of the bastion host by running `kubectl get openstackcluster` and looking at the `Bastion` field in status.
+Once it's gone, you can re-enable the bastion host by setting `enabled: true` and then making changes to the bastion instance spec by modifying the `OpenStackCluster.Spec.Bastion.Instance` field.
+The bastion host will be re-created with the new instance spec.
+
+### Disabling the bastion
+
+To disable the bastion host, set `enabled: false` in the `OpenStackCluster.Spec.Bastion` field. The bastion host will be deleted, you can check the status of the bastion host by running `kubectl get openstackcluster` and looking at the `Bastion` field in status.
+Once it's gone, you can now remove the `OpenStackCluster.Spec.Bastion` field from the `OpenStackCluster` spec.
+
 ### Obtain floating IP address of the bastion node
 
 Once the workload cluster is up and running after being configured for an SSH bastion host, you can use the kubectl get openstackcluster command to look up the floating IP address of the bastion host (make sure the kubectl context is set to the management cluster). The output will look something like this:
