From c4a5c43b529a21ee2e75e48d5ea14e46fb16a696 Mon Sep 17 00:00:00 2001
From: Gerrit <info@gerritschwerthelm.de>
Date: Tue, 9 Jul 2024 09:25:58 +0200
Subject: [PATCH 1/2] Fix firewall not recreated when manually deleted.

I think we broke this functionality when we started to optimize machine queries to reduce load on the metal-api.
---
 controllers/firewall/controller.go | 33 +++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/controllers/firewall/controller.go b/controllers/firewall/controller.go
index d8cec5c..3dd7f04 100644
--- a/controllers/firewall/controller.go
+++ b/controllers/firewall/controller.go
@@ -2,7 +2,9 @@ package firewall
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -44,30 +46,41 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M
 		}),
 		// the cache is only very short but on quickly repeated status updates, this should prevent the metal-api from being flooded
 		firewallCache: cache.New(5*time.Second, func(ctx context.Context, fw *v2.Firewall) ([]*models.V1FirewallResponse, error) {
+			searchFirewalls := func() ([]*models.V1FirewallResponse, error) {
+				resp, err := c.GetMetal().Firewall().FindFirewalls(firewall.NewFindFirewallsParams().WithBody(&models.V1FirewallFindRequest{
+					AllocationName:    fw.Name,
+					AllocationProject: fw.Spec.Project,
+					Tags:              []string{c.GetClusterTag()},
+				}).WithContext(ctx), nil)
+				if err != nil {
+					return nil, fmt.Errorf("firewall search error: %w", err)
+				}
+
+				return resp.Payload, nil
+			}
+
 			// First try to find the firewall by machineID but check that allocation, project and hostname still matches
 			// this prevent erroneous situations where a metal admin just deleted the allocated firewall by hand
 			if fw.Status.MachineStatus != nil && fw.Status.MachineStatus.MachineID != "" {
 				resp, err := c.GetMetal().Firewall().FindFirewall(firewall.NewFindFirewallParams().WithContext(ctx).WithID(fw.Status.MachineStatus.MachineID), nil)
 				if err != nil {
+					var defaultErr *firewall.FindFirewallDefault
+					if errors.As(err, &defaultErr) && defaultErr.Code() == http.StatusNotFound {
+						return searchFirewalls()
+					}
+
 					return nil, fmt.Errorf("firewall find error: %w", err)
 				}
+
 				if resp.Payload.Allocation != nil &&
 					*resp.Payload.Allocation.Project == fw.Spec.Project &&
 					*resp.Payload.Allocation.Hostname == fw.Name {
 					return []*models.V1FirewallResponse{resp.Payload}, nil
 				}
 			}
-			// in any other situations make a expensive find firewalls call
-			resp, err := c.GetMetal().Firewall().FindFirewalls(firewall.NewFindFirewallsParams().WithBody(&models.V1FirewallFindRequest{
-				AllocationName:    fw.Name,
-				AllocationProject: fw.Spec.Project,
-				Tags:              []string{c.GetClusterTag()},
-			}).WithContext(ctx), nil)
-			if err != nil {
-				return nil, fmt.Errorf("firewall find error: %w", err)
-			}
 
-			return resp.Payload, nil
+			// in any other situations make a expensive find firewalls call
+			return searchFirewalls()
 		}),
 	})
 

From 33a33ca986a65b7479cb4551efadf866a495067f Mon Sep 17 00:00:00 2001
From: Gerrit <info@gerritschwerthelm.de>
Date: Tue, 9 Jul 2024 09:45:42 +0200
Subject: [PATCH 2/2] Comment.

---
 controllers/firewall/controller.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/controllers/firewall/controller.go b/controllers/firewall/controller.go
index 3dd7f04..6c18b9a 100644
--- a/controllers/firewall/controller.go
+++ b/controllers/firewall/controller.go
@@ -61,6 +61,10 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M
 
 			// First try to find the firewall by machineID but check that allocation, project and hostname still matches
 			// this prevent erroneous situations where a metal admin just deleted the allocated firewall by hand
+			//
+			// This is kind of an anti-pattern because we depend on our own status, but performance benefit of this approach is
+			// big enough that we agreed to do it. We still need to run the expensive lookup in the metal-api in case deriving
+			// the machine from the status field does not work.
 			if fw.Status.MachineStatus != nil && fw.Status.MachineStatus.MachineID != "" {
 				resp, err := c.GetMetal().Firewall().FindFirewall(firewall.NewFindFirewallParams().WithContext(ctx).WithID(fw.Status.MachineStatus.MachineID), nil)
 				if err != nil {