Merge pull request kubernetes-sigs#1303 from mercedes-benz/tobiasgiese/bastion-recreation

✨ Add re-creation of bastion host on change

Author: k8s-ci-robot

api/v1alpha6/openstackcluster_webhook.go

@@ -105,17 +105,9 @@ func (r *OpenStackCluster) ValidateUpdate(oldRaw runtime.Object) error {
 		r.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{}
 	}
 
-	// Allow changes to the bastion spec only if no bastion host is deployed (i.e. Spec.Bastion.Enabled=false).
-	if old.Status.Bastion == nil {
-		old.Spec.Bastion = &Bastion{}
-		r.Spec.Bastion = &Bastion{}
-	}
-
-	// Allow toggling the bastion enabled flag.
-	if old.Spec.Bastion != nil && r.Spec.Bastion != nil {
-		old.Spec.Bastion.Enabled = true
-		r.Spec.Bastion.Enabled = true
-	}
+	// Allow changes to the bastion spec.
+	old.Spec.Bastion = &Bastion{}
+	r.Spec.Bastion = &Bastion{}
 
 	// Allow changes on AllowedCIDRs
 	if r.Spec.APIServerLoadBalancer.Enabled {

api/v1alpha6/openstackcluster_webhook_test.go

@@ -112,90 +112,7 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "Toggle OpenStackCluster.Spec.Bastion.Enabled flag is allowed",
-			oldTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobar",
-							Image:     "foobar",
-							Flavor:    "minimal",
-						},
-						Enabled: false,
-					},
-				},
-			},
-			newTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobarbaz",
-							Image:     "foobarbaz",
-							Flavor:    "medium",
-						},
-						Enabled: true,
-					},
-				},
-			},
-			wantErr: false,
-		},
-		{
-			name: "Changing empty OpenStackCluster.Spec.Bastion is allowed",
-			oldTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-				},
-			},
-			newTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobar",
-							Image:     "foobar",
-							Flavor:    "medium",
-						},
-						Enabled: true,
-					},
-				},
-			},
-			wantErr: false,
-		},
-		{
-			name: "Changing OpenStackCluster.Spec.Bastion with no deployed bastion host is allowed",
-			oldTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobar",
-							Image:     "foobar",
-							Flavor:    "minimal",
-						},
-						Enabled: false,
-					},
-				},
-				Status: OpenStackClusterStatus{},
-			},
-			newTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobarbaz",
-							Image:     "foobarbaz",
-							Flavor:    "medium",
-						},
-						Enabled: true,
-					},
-				},
-			},
-			wantErr: false,
-		},
-		{
-			name: "Changing OpenStackCluster.Spec.Bastion with deployed bastion host is not allowed",
+			name: "Changing OpenStackCluster.Spec.Bastion is allowed",
 			oldTemplate: &OpenStackCluster{
 				Spec: OpenStackClusterSpec{
 					CloudName: "foobar",
@@ -227,46 +144,6 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) {
 					},
 				},
 			},
-			wantErr: true,
-		},
-		{
-			name: "Disabling the OpenStackCluster.Spec.Bastion while it's running is allowed",
-			oldTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobar",
-							Image:     "foobar",
-							Flavor:    "minimal",
-						},
-						Enabled: true,
-					},
-				},
-				Status: OpenStackClusterStatus{
-					Bastion: &Instance{
-						Name: "foobar",
-					},
-				},
-			},
-			newTemplate: &OpenStackCluster{
-				Spec: OpenStackClusterSpec{
-					CloudName: "foobar",
-					Bastion: &Bastion{
-						Instance: OpenStackMachineSpec{
-							CloudName: "foobar",
-							Image:     "foobar",
-							Flavor:    "minimal",
-						},
-						Enabled: false,
-					},
-				},
-				Status: OpenStackClusterStatus{
-					Bastion: &Instance{
-						Name: "foobar",
-					},
-				},
-			},
 			wantErr: false,
 		},
 		{

controllers/openstackcluster_controller.go

@@ -51,6 +51,10 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 )
 
+const (
+	BastionInstanceHashAnnotation = "infrastructure.cluster.x-k8s.io/bastion-hash"
+)
+
 // OpenStackClusterReconciler reconciles a OpenStackCluster object.
 type OpenStackClusterReconciler struct {
 	Client           client.Client
@@ -242,6 +246,8 @@ func deleteBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackClus
 	}
 	openStackCluster.Status.BastionSecurityGroup = nil
 
+	delete(openStackCluster.ObjectMeta.Annotations, BastionInstanceHashAnnotation)
+
 	return nil
 }
 
@@ -313,20 +319,35 @@ func reconcileBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackC
 		return err
 	}
 
+	instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+	bastionHash, err := compute.HashInstanceSpec(instanceSpec)
+	if err != nil {
+		return errors.Wrap(err, "failed computing bastion hash from instance spec")
+	}
+
 	instanceStatus, err := computeService.GetInstanceStatusByName(openStackCluster, fmt.Sprintf("%s-bastion", cluster.Name))
 	if err != nil {
 		return err
 	}
 	if instanceStatus != nil {
-		bastion, err := instanceStatus.APIInstance(openStackCluster)
-		if err != nil {
+		if !bastionHashHasChanged(bastionHash, openStackCluster.ObjectMeta.Annotations) {
+			bastion, err := instanceStatus.APIInstance(openStackCluster)
+			if err != nil {
+				return err
+			}
+			// Add the current hash if no annotation is set.
+			if _, ok := openStackCluster.ObjectMeta.Annotations[BastionInstanceHashAnnotation]; !ok {
+				annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
+			}
+			openStackCluster.Status.Bastion = bastion
+			return nil
+		}
+
+		if err := deleteBastion(scope, cluster, openStackCluster); err != nil {
 			return err
 		}
-		openStackCluster.Status.Bastion = bastion
-		return nil
 	}
 
-	instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
 	instanceStatus, err = computeService.CreateInstance(openStackCluster, openStackCluster, instanceSpec, cluster.Name)
 	if err != nil {
 		return errors.Errorf("failed to reconcile bastion: %v", err)
@@ -360,6 +381,7 @@ func reconcileBastion(scope *scope.Scope, cluster *clusterv1.Cluster, openStackC
 	}
 	bastion.FloatingIP = fp.FloatingIP
 	openStackCluster.Status.Bastion = bastion
+	annotations.AddAnnotations(openStackCluster, map[string]string{BastionInstanceHashAnnotation: bastionHash})
 	return nil
 }
 
@@ -390,6 +412,15 @@ func bastionToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, clusterNa
 	return instanceSpec
 }
 
+// bastionHashHasChanged returns a boolean whether if the latest bastion hash, built from the instance spec, has changed or not.
+func bastionHashHasChanged(computeHash string, clusterAnnotations map[string]string) bool {
+	latestHash, ok := clusterAnnotations[BastionInstanceHashAnnotation]
+	if !ok {
+		return false
+	}
+	return latestHash != computeHash
+}
+
 func reconcileNetworkComponents(scope *scope.Scope, cluster *clusterv1.Cluster, openStackCluster *infrav1.OpenStackCluster) error {
 	clusterName := fmt.Sprintf("%s-%s", cluster.Namespace, cluster.Name)
 

docs/book/src/clusteropenstack/configuration.md

@@ -475,10 +475,9 @@ spec:
       sshKeyName: <Key pair name>
 ```
 
-The `enabled` flag is toggleable. Thus, you're able to save resources while the bastion host is not needed.  
-All other parameters can be changed via an `OpenStackCluster` update while the bastion host is not running.
-
-> Note: as a rolling update is not ideal during a bastion host session, we prevent changes to a running bastion configuration.
+All parameters are mutable during the runtime of the bastion host.
+The bastion host will be re-created if it's enabled and the instance spec has been changed.
+This is done by a simple checksum validation of the instance spec which is stored in the `OpenStackCluster` annotation `infrastructure.cluster.x-k8s.io/bastion-hash`.
 
 A floating IP is created and associated to the bastion host automatically, but you can add the IP address explicitly:
 

go.mod

@@ -4,6 +4,7 @@ go 1.17
 
 require (
 	github.com/blang/semver v3.5.1+incompatible
+	github.com/davecgh/go-spew v1.1.1
 	github.com/go-logr/logr v1.2.0
 	github.com/golang/mock v1.6.0
 	github.com/google/gofuzz v1.2.0
@@ -47,7 +48,6 @@ require (
 	github.com/cespare/xxhash/v2 v2.1.2 // indirect
 	github.com/coredns/caddy v1.1.0 // indirect
 	github.com/coredns/corefile-migration v1.0.17 // indirect
-	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/docker/distribution v2.8.1+incompatible // indirect
 	github.com/docker/docker v20.10.17+incompatible // indirect
 	github.com/docker/go-connections v0.4.0 // indirect

pkg/cloud/services/compute/instance.go

@@ -35,6 +35,7 @@ import (
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha6"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/record"
 	capoerrors "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/errors"
+	"sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/hash"
 )
 
 const (
@@ -722,3 +723,11 @@ func (s *Service) isTrunkExtSupported() (trunknSupported bool, err error) {
 	}
 	return true, nil
 }
+
+func HashInstanceSpec(computeInstance *InstanceSpec) (string, error) {
+	instanceHash, err := hash.ComputeSpewHash(computeInstance)
+	if err != nil {
+		return "", err
+	}
+	return strconv.Itoa(int(instanceHash)), nil
+}

pkg/utils/hash/hash.go

@@ -0,0 +1,52 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package hash
+
+import (
+	"fmt"
+	"hash"
+	"hash/fnv"
+
+	"github.com/davecgh/go-spew/spew"
+)
+
+// SpewHashObject writes specified object to hash using the spew library
+// which follows pointers and prints actual values of the nested objects
+// ensuring the hash does not change when a pointer changes.
+func SpewHashObject(hasher hash.Hash, objectToWrite interface{}) error {
+	hasher.Reset()
+	printer := spew.ConfigState{
+		Indent:         " ",
+		SortKeys:       true,
+		DisableMethods: true,
+		SpewKeys:       true,
+	}
+
+	if _, err := printer.Fprintf(hasher, "%#v", objectToWrite); err != nil {
+		return fmt.Errorf("failed to write object to hasher")
+	}
+	return nil
+}
+
+// ComputeSpewHash computes the hash of a InstanceSpec using the spew library.
+func ComputeSpewHash(objectToWrite interface{}) (uint32, error) {
+	instanceSpecHasher := fnv.New32a()
+	if err := SpewHashObject(instanceSpecHasher, objectToWrite); err != nil {
+		return 0, err
+	}
+	return instanceSpecHasher.Sum32(), nil
+}