Integration test and improvements on recreate strategy. (#35)

api/v2/defaults/defaults.go

@@ -79,9 +79,6 @@ func (r *firewallSetDefaulter) Default(ctx context.Context, obj runtime.Object)
 
 	r.log.Info("defaulting firewallset resource", "name", f.GetName(), "namespace", f.GetNamespace())
 
-	if f.Spec.Replicas == 0 {
-		f.Spec.Replicas = 1
-	}
 	if f.Spec.Selector == nil {
 		f.Spec.Selector = f.Spec.Template.Labels
 	}
@@ -99,9 +96,6 @@ func (r *firewallDeploymentDefaulter) Default(ctx context.Context, obj runtime.O
 
 	r.log.Info("defaulting firewalldeployment resource", "name", f.GetName(), "namespace", f.GetNamespace())
 
-	if f.Spec.Replicas == 0 {
-		f.Spec.Replicas = 1
-	}
 	if f.Spec.Strategy == "" {
 		f.Spec.Strategy = v2.StrategyRollingUpdate
 	}

api/v2/types_firewallset.go

@@ -30,6 +30,10 @@ func FirewallManagedByTag() string {
 	return fmt.Sprintf("%s=%s", FirewallControllerManagedByAnnotation, FirewallControllerManager)
 }
 
+func (f FirewallDistance) Pointer() *FirewallDistance {
+	return &f
+}
+
 // FirewallSet contains the spec template of a firewall resource similar to a Kubernetes ReplicaSet and takes care that the desired amount of firewall replicas is running.
 //
 // +kubebuilder:object:root=true

controllers/deployment/reconcile.go

@@ -29,15 +29,15 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error
 		return fmt.Errorf("unable to get owned sets: %w", err)
 	}
 
-	current, err := controllers.MaxRevisionOf(ownedSets)
+	latestSet, err := controllers.MaxRevisionOf(ownedSets)
 	if err != nil {
 		return err
 	}
 
-	if current == nil {
+	if latestSet == nil {
 		r.Log.Info("no firewall set is present, creating a new one")
 
-		_, err := c.createFirewallSet(r, v2.FirewallShortestDistance, 0)
+		_, err := c.createFirewallSet(r, 0, nil)
 		if err != nil {
 			return err
 		}
@@ -47,9 +47,9 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error
 
 	switch s := r.Target.Spec.Strategy; s {
 	case v2.StrategyRecreate:
-		err = c.recreateStrategy(r, ownedSets, current)
+		err = c.recreateStrategy(r, ownedSets, latestSet)
 	case v2.StrategyRollingUpdate:
-		err = c.rollingUpdateStrategy(r, ownedSets, current)
+		err = c.rollingUpdateStrategy(r, ownedSets, latestSet)
 	default:
 		err = fmt.Errorf("unknown deployment strategy: %s", s)
 	}
@@ -64,10 +64,10 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error
 	}
 
 	// we are done with the update, give the set the shortest distance if this is not already the case
-	if current.Status.ReadyReplicas == current.Spec.Replicas && current.Spec.Distance != v2.FirewallShortestDistance {
-		current.Spec.Distance = v2.FirewallShortestDistance
+	if latestSet.Status.ReadyReplicas == latestSet.Spec.Replicas && latestSet.Spec.Distance != v2.FirewallShortestDistance {
+		latestSet.Spec.Distance = v2.FirewallShortestDistance
 
-		err := c.c.GetSeedClient().Update(r.Ctx, current)
+		err := c.c.GetSeedClient().Update(r.Ctx, latestSet)
 		if err != nil {
 			return fmt.Errorf("unable to swap latest set distance to %d: %w", v2.FirewallShortestDistance, err)
 		}
@@ -78,16 +78,23 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error
 	return nil
 }
 
-func (c *controller) createNextFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], current *v2.FirewallSet, distance v2.FirewallDistance) (*v2.FirewallSet, error) {
-	revision, err := controllers.NextRevision(current)
+func (c *controller) createNextFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], set *v2.FirewallSet, ows *setOverrides) (*v2.FirewallSet, error) {
+	revision, err := controllers.NextRevision(set)
 	if err != nil {
 		return nil, err
 	}
 
-	return c.createFirewallSet(r, distance, revision)
+	return c.createFirewallSet(r, revision, ows)
 }
 
-func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], distance v2.FirewallDistance, revision int) (*v2.FirewallSet, error) {
+type setOverrides struct {
+	// override default distance (shortest distance)
+	distance *v2.FirewallDistance
+	// override default replicas (inherited from set spec)
+	replicas *int
+}
+
+func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], revision int, ows *setOverrides) (*v2.FirewallSet, error) {
 	if lastCreation, ok := c.lastSetCreation[r.Target.Name]; ok && time.Since(lastCreation) < c.c.GetSafetyBackoff() {
 		// this is just for safety reasons to prevent mass-allocations
 		r.Log.Info("backing off from firewall set creation as last creation is only seconds ago", "ago", time.Since(lastCreation).String())
@@ -99,7 +106,18 @@ func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment
 		return nil, err
 	}
 
-	name := fmt.Sprintf("%s-%s", r.Target.Name, uuid.String()[:5])
+	var (
+		name     = fmt.Sprintf("%s-%s", r.Target.Name, uuid.String()[:5])
+		distance = v2.FirewallShortestDistance
+		replicas = r.Target.Spec.Replicas
+	)
+
+	if ows != nil && ows.distance != nil {
+		distance = *ows.distance
+	}
+	if ows != nil && ows.replicas != nil {
+		replicas = *ows.replicas
+	}
 
 	set := &v2.FirewallSet{
 		ObjectMeta: metav1.ObjectMeta{
@@ -114,7 +132,7 @@ func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment
 			Labels: r.Target.Labels,
 		},
 		Spec: v2.FirewallSetSpec{
-			Replicas: r.Target.Spec.Replicas,
+			Replicas: replicas,
 			Template: r.Target.Spec.Template,
 			Distance: distance,
 		},

controllers/deployment/recreate.go

@@ -2,45 +2,58 @@ package deployment
 
 import (
 	"fmt"
+	"time"
 
 	v2 "github.com/metal-stack/firewall-controller-manager/api/v2"
 	"github.com/metal-stack/firewall-controller-manager/controllers"
+	"github.com/metal-stack/metal-lib/pkg/pointer"
 )
 
 // recreateStrategy first deletes the existing firewall sets and then creates a new one
-func (c *controller) recreateStrategy(r *controllers.Ctx[*v2.FirewallDeployment], ownedSets []*v2.FirewallSet, current *v2.FirewallSet) error {
-	newSetRequired, err := c.isNewSetRequired(r, current)
+func (c *controller) recreateStrategy(r *controllers.Ctx[*v2.FirewallDeployment], ownedSets []*v2.FirewallSet, latestSet *v2.FirewallSet) error {
+	newSetRequired, err := c.isNewSetRequired(r, latestSet)
 	if err != nil {
 		return err
 	}
 
 	if newSetRequired {
-		r.Log.Info("significant changes detected in the spec, cleaning up old sets then create new firewall set")
+		r.Log.Info("significant changes detected in the spec, create new scaled down firewall set, then cleaning up old sets")
 
-		err = c.deleteFirewallSets(r, ownedSets...)
+		set, err := c.createNextFirewallSet(r, latestSet, &setOverrides{
+			replicas: pointer.Pointer(0),
+		})
 		if err != nil {
 			return err
 		}
 
-		newSet, err := c.createNextFirewallSet(r, current, v2.FirewallShortestDistance)
-		if err != nil {
-			return err
-		}
+		c.recorder.Eventf(set, "Normal", "Recreate", "recreated firewallset old: %s new: %s", latestSet.Name, set.Name)
 
-		c.recorder.Eventf(newSet, "Normal", "Recreate", "recreated firewallset old: %s new: %s", current.Name, newSet.Name)
+		latestSet = set
+	}
 
-		return nil
+	err = c.deleteFirewallSets(r, controllers.Except(ownedSets, latestSet)...)
+	if err != nil {
+		return err
 	}
 
-	err = c.syncFirewallSet(r, current)
+	err = c.syncFirewallSet(r, latestSet)
 	if err != nil {
 		return fmt.Errorf("unable to update firewall set: %w", err)
 	}
 
-	if current.Status.ReadyReplicas == current.Spec.Replicas {
-		cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionTrue, "NewFirewallSetAvailable", fmt.Sprintf("FirewallSet %q has successfully progressed.", current.Name))
-		r.Target.Status.Conditions.Set(cond)
+	if latestSet.Status.ReadyReplicas != latestSet.Spec.Replicas {
+		r.Log.Info("set replicas are not yet ready")
+
+		if time.Since(latestSet.CreationTimestamp.Time) > c.c.GetProgressDeadline() {
+			cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionFalse, "ProgressDeadlineExceeded", fmt.Sprintf("FirewallSet %q has timed out progressing.", latestSet.Name))
+			r.Target.Status.Conditions.Set(cond)
+		}
+
+		return nil
 	}
 
+	cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionTrue, "NewFirewallSetAvailable", fmt.Sprintf("FirewallSet %q has successfully progressed.", latestSet.Name))
+	r.Target.Status.Conditions.Set(cond)
+
 	return nil
 }

controllers/deployment/rolling.go

@@ -9,16 +9,18 @@ import (
 )
 
 // rollingUpdateStrategy first creates a new set and deletes the old one's when the new one becomes ready
-func (c *controller) rollingUpdateStrategy(r *controllers.Ctx[*v2.FirewallDeployment], ownedSets []*v2.FirewallSet, current *v2.FirewallSet) error {
-	newSetRequired, err := c.isNewSetRequired(r, current)
+func (c *controller) rollingUpdateStrategy(r *controllers.Ctx[*v2.FirewallDeployment], ownedSets []*v2.FirewallSet, latestSet *v2.FirewallSet) error {
+	newSetRequired, err := c.isNewSetRequired(r, latestSet)
 	if err != nil {
 		return err
 	}
 
 	if newSetRequired {
 		r.Log.Info("significant changes detected in the spec, creating new firewall set", "distance", v2.FirewallRollingUpdateSetDistance)
 
-		newSet, err := c.createNextFirewallSet(r, current, v2.FirewallRollingUpdateSetDistance)
+		newSet, err := c.createNextFirewallSet(r, latestSet, &setOverrides{
+			distance: v2.FirewallRollingUpdateSetDistance.Pointer(),
+		})
 		if err != nil {
 			return err
 		}
@@ -30,28 +32,28 @@ func (c *controller) rollingUpdateStrategy(r *controllers.Ctx[*v2.FirewallDeploy
 		return c.cleanupIntermediateSets(r, ownedSets)
 	}
 
-	err = c.syncFirewallSet(r, current)
+	err = c.syncFirewallSet(r, latestSet)
 	if err != nil {
 		return fmt.Errorf("unable to update firewall set: %w", err)
 	}
 
-	if current.Status.ReadyReplicas != current.Spec.Replicas {
+	if latestSet.Status.ReadyReplicas != latestSet.Spec.Replicas {
 		r.Log.Info("set replicas are not yet ready")
 
-		if time.Since(current.CreationTimestamp.Time) > c.c.GetProgressDeadline() {
-			cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionFalse, "ProgressDeadlineExceeded", fmt.Sprintf("FirewallSet %q has timed out progressing.", current.Name))
+		if time.Since(latestSet.CreationTimestamp.Time) > c.c.GetProgressDeadline() {
+			cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionFalse, "ProgressDeadlineExceeded", fmt.Sprintf("FirewallSet %q has timed out progressing.", latestSet.Name))
 			r.Target.Status.Conditions.Set(cond)
 		}
 
 		return c.cleanupIntermediateSets(r, ownedSets)
 	}
 
-	cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionTrue, "NewFirewallSetAvailable", fmt.Sprintf("FirewallSet %q has successfully progressed.", current.Name))
+	cond := v2.NewCondition(v2.FirewallDeplomentProgressing, v2.ConditionTrue, "NewFirewallSetAvailable", fmt.Sprintf("FirewallSet %q has successfully progressed.", latestSet.Name))
 	r.Target.Status.Conditions.Set(cond)
 
 	r.Log.Info("ensuring old sets are cleaned up")
 
-	oldSets := controllers.Except(ownedSets, current)
+	oldSets := controllers.Except(ownedSets, latestSet)
 
 	return c.deleteFirewallSets(r, oldSets...)
 }

controllers/firewall/delete.go

@@ -8,8 +8,8 @@ import (
 	v2 "github.com/metal-stack/firewall-controller-manager/api/v2"
 	"github.com/metal-stack/firewall-controller-manager/controllers"
 	"github.com/metal-stack/metal-go/api/client/machine"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func (c *controller) Delete(r *controllers.Ctx[*v2.Firewall]) error {
@@ -41,7 +41,9 @@ func (c *controller) Delete(r *controllers.Ctx[*v2.Firewall]) error {
 
 		resp, err := c.c.GetMetal().Machine().FreeMachine(machine.NewFreeMachineParams().WithID(*f.ID).WithContext(r.Ctx), nil)
 		if err != nil {
-			return fmt.Errorf("firewall delete error: %w", err)
+			r.Log.Error(err, "firewall deletion failed")
+
+			return controllers.RequeueAfter(5*time.Second, "firewall deletion failed, retrying")
 		}
 
 		r.Log.Info("deleted firewall", "firewall-name", f.Name, "id", *resp.Payload.ID)
@@ -62,8 +64,11 @@ func (c *controller) deleteFirewallMonitor(ctx context.Context, fw *v2.Firewall)
 
 	err := c.c.GetShootClient().Delete(ctx, mon)
 	if err != nil {
-		return client.IgnoreNotFound(err)
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return err
 	}
 
-	return nil
+	return controllers.RequeueAfter(1*time.Second, "waiting for firewall monitor to be deleted, requeuing")
 }

controllers/firewall/reconcile.go

@@ -19,15 +19,16 @@ import (
 func (c *controller) Reconcile(r *controllers.Ctx[*v2.Firewall]) error {
 	var f *models.V1FirewallResponse
 	defer func() {
+		if err := c.setStatus(r, f); err != nil {
+			r.Log.Error(err, "unable to set firewall status")
+		}
+
 		mon, err := c.ensureFirewallMonitor(r)
 		if err != nil {
 			r.Log.Error(err, "unable to deploy firewall monitor")
-			// not returning, we can still try to update the status
 		}
 
-		if err := c.setStatus(r, f, mon); err != nil {
-			r.Log.Error(err, "unable to set firewall status")
-		}
+		SetFirewallStatusFromMonitor(r.Target, mon)
 	}()
 
 	fws, err := c.firewallCache.Get(r.Ctx, r.Target)

controllers/firewall/status.go

@@ -11,7 +11,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func (c *controller) setStatus(r *controllers.Ctx[*v2.Firewall], f *models.V1FirewallResponse, mon *v2.FirewallMonitor) error {
+func (c *controller) setStatus(r *controllers.Ctx[*v2.Firewall], f *models.V1FirewallResponse) error {
 	var errs []error
 
 	err := setMachineStatus(r.Target, f)
@@ -24,8 +24,6 @@ func (c *controller) setStatus(r *controllers.Ctx[*v2.Firewall], f *models.V1Fir
 		errs = append(errs, err)
 	}
 
-	SetFirewallStatus(r.Target, mon)
-
 	r.Target.Status.ShootAccess = c.c.GetShootAccess()
 
 	return errors.Join(errs...)
@@ -118,7 +116,7 @@ func (c *controller) setFirewallNetworks(r *controllers.Ctx[*v2.Firewall], f *mo
 	return nil
 }
 
-func SetFirewallStatus(fw *v2.Firewall, mon *v2.FirewallMonitor) {
+func SetFirewallStatusFromMonitor(fw *v2.Firewall, mon *v2.FirewallMonitor) {
 	if v2.IsAnnotationTrue(fw, v2.FirewallNoControllerConnectionAnnotation) {
 		cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionTrue, "NotChecking", "Not checking controller connection due to firewall annotation.")
 		fw.Status.Conditions.Set(cond)

controllers/monitor/reconcile.go

@@ -23,7 +23,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallMonitor]) error {
 	fw, err := c.updateFirewallStatus(r)
 	if err != nil {
 		r.Log.Error(err, "unable to update firewall status")
-		return controllers.RequeueAfter(3*time.Second, "unable to update firewall status, retrying: %w")
+		return controllers.RequeueAfter(3*time.Second, "unable to update firewall status, retrying")
 	}
 
 	err = c.offerFirewallControllerMigrationSecret(r, fw)
@@ -53,7 +53,7 @@ func (c *controller) updateFirewallStatus(r *controllers.Ctx[*v2.FirewallMonitor
 		return nil, fmt.Errorf("associated firewall of monitor not found: %w", err)
 	}
 
-	firewall.SetFirewallStatus(fw, r.Target)
+	firewall.SetFirewallStatusFromMonitor(fw, r.Target)
 
 	err = c.c.GetSeedClient().Status().Update(r.Ctx, fw)
 	if err != nil {

controllers/set/reconcile.go

@@ -31,7 +31,13 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error {
 	// - the least important at the end of the slice can be popped off for deletion on scale down
 	v2.SortFirewallsByImportance(ownedFirewalls)
 
-	for i, fw := range ownedFirewalls {
+	for i, ownedFw := range ownedFirewalls {
+		fw := &v2.Firewall{}
+		err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKeyFromObject(ownedFw), fw)
+		if err != nil {
+			return fmt.Errorf("error fetching firewall: %w", err)
+		}
+
 		fw.Spec = r.Target.Spec.Template.Spec
 
 		// stagger firewall replicas to achieve active/standby behavior
@@ -62,7 +68,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error {
 
 		fw.Distance = distance
 
-		err := c.c.GetSeedClient().Update(r.Ctx, fw, &client.UpdateOptions{})
+		err = c.c.GetSeedClient().Update(r.Ctx, fw, &client.UpdateOptions{})
 		if err != nil {
 			return fmt.Errorf("error updating firewall spec: %w", err)
 		}