Update firewalls in maintenance reconciliation

api/v2/config/controller.go

@@ -59,6 +59,9 @@ type NewControllerConfig struct {
 	FirewallHealthTimeout time.Duration
 	// CreateTimeout is used in the firewall creation phase to recreate a firewall when it does not become ready.
 	CreateTimeout time.Duration
+
+	// SkipValidation skips configuration validation, use this only for testing purposes
+	SkipValidation bool
 }
 
 type ControllerConfig struct {
@@ -90,7 +93,7 @@ type ControllerConfig struct {
 }
 
 func New(c *NewControllerConfig) (*ControllerConfig, error) {
-	if err := c.validate(); err != nil {
+	if err := c.validate(); !c.SkipValidation && err != nil {
 		return nil, err
 	}
 

api/v2/types_firewall.go

@@ -213,6 +213,8 @@ type MachineStatus struct {
 	CrashLoop bool `json:"crashLoop,omitempty"`
 	// LastEvent contains the last provisioning event of the machine.
 	LastEvent *MachineLastEvent `json:"lastEvent,omitempty"`
+	// ImageID contains the used os image id of the firewall (the fully qualified version, no shorthand version).
+	ImageID string `json:"imageID,omitempty"`
 }
 
 // MachineLastEvent contains the last provisioning event of the machine.

config/crds/firewall.metal-stack.io_firewallmonitors.yaml

@@ -232,6 +232,10 @@ spec:
                 description: CrashLoop can occur during provisioning of the firewall
                   causing the firewall not to get ready.
                 type: boolean
+              imageID:
+                description: ImageID contains the used os image id of the firewall
+                  (the fully qualified version, no shorthand version).
+                type: string
               lastEvent:
                 description: LastEvent contains the last provisioning event of the
                   machine.

config/crds/firewall.metal-stack.io_firewalls.yaml

@@ -350,6 +350,10 @@ spec:
                     description: CrashLoop can occur during provisioning of the firewall
                       causing the firewall not to get ready.
                     type: boolean
+                  imageID:
+                    description: ImageID contains the used os image id of the firewall
+                      (the fully qualified version, no shorthand version).
+                    type: string
                   lastEvent:
                     description: LastEvent contains the last provisioning event of
                       the machine.

controllers/deployment/controller.go

@@ -1,6 +1,8 @@
 package deployment
 
 import (
+	"context"
+	"fmt"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -14,13 +16,17 @@ import (
 	"github.com/metal-stack/firewall-controller-manager/api/v2/defaults"
 	"github.com/metal-stack/firewall-controller-manager/api/v2/validation"
 	"github.com/metal-stack/firewall-controller-manager/controllers"
+	"github.com/metal-stack/metal-go/api/client/image"
+	"github.com/metal-stack/metal-go/api/models"
+	"github.com/metal-stack/metal-lib/pkg/cache"
 )
 
 type controller struct {
 	c               *config.ControllerConfig
 	log             logr.Logger
 	lastSetCreation map[string]time.Time
 	recorder        record.EventRecorder
+	imageCache      *cache.Cache[string, *models.V1ImageResponse]
 }
 
 func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.Manager, c *config.ControllerConfig) error {
@@ -29,6 +35,14 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M
 		log:             log,
 		recorder:        recorder,
 		lastSetCreation: map[string]time.Time{},
+		imageCache: cache.New(5*time.Minute, func(ctx context.Context, id string) (*models.V1ImageResponse, error) {
+			resp, err := c.GetMetal().Image().FindLatestImage(image.NewFindLatestImageParams().WithID(id).WithContext(ctx), nil)
+			if err != nil {
+				return nil, fmt.Errorf("latest firewall image %q not found: %w", id, err)
+			}
+
+			return resp.Payload, nil
+		}),
 	})
 
 	return ctrl.NewControllerManagedBy(mgr).

controllers/deployment/reconcile.go

@@ -1,16 +1,14 @@
 package deployment
 
 import (
-	"context"
 	"fmt"
 	"strconv"
 	"time"
 
 	"github.com/google/uuid"
 	v2 "github.com/metal-stack/firewall-controller-manager/api/v2"
 	"github.com/metal-stack/firewall-controller-manager/controllers"
-	metalgo "github.com/metal-stack/metal-go"
-	"github.com/metal-stack/metal-go/api/client/image"
+	"github.com/metal-stack/metal-lib/pkg/pointer"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/util/retry"
@@ -206,7 +204,7 @@ func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment]
 		return ok, nil
 	}
 
-	ok, err := osImageHasChanged(r.Ctx, c.c.GetMetal(), newS, oldS)
+	ok, err := c.osImageHasChanged(r, latestSet, newS, oldS)
 	if err != nil {
 		return false, err
 	}
@@ -228,19 +226,48 @@ func sizeHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool {
 	return newS.Size != oldS.Size
 }
 
-func osImageHasChanged(ctx context.Context, m metalgo.Client, newS *v2.FirewallSpec, oldS *v2.FirewallSpec) (bool, error) {
+func (c *controller) osImageHasChanged(r *controllers.Ctx[*v2.FirewallDeployment], latestSet *v2.FirewallSet, newS *v2.FirewallSpec, oldS *v2.FirewallSpec) (bool, error) {
 	if newS.Image != oldS.Image {
-		image, err := m.Image().FindLatestImage(image.NewFindLatestImageParams().WithID(newS.Image).WithContext(ctx), nil)
-		if err != nil {
-			return false, fmt.Errorf("latest firewall image not found:%s %w", newS.Image, err)
-		}
+		return true, nil
+	}
 
-		if image.Payload != nil && image.Payload.ID != nil && *image.Payload.ID != oldS.Image {
-			return true, nil
-		}
+	if !r.WithinMaintenance {
+		return false, nil
 	}
 
-	return false, nil
+	// let's resolve the latest image from the api in case a shorthand image flag is being used
+	image, err := c.imageCache.Get(r.Ctx, newS.Image)
+	if err != nil {
+		return false, err
+	}
+
+	if pointer.SafeDeref(image.ID) == newS.Image {
+		// early return because no shorthand image used
+		return false, err
+	}
+
+	// now compare to the actual image deployed on the firewalls in this set
+
+	ownedFirewalls, _, err := controllers.GetOwnedResources(r.Ctx, c.c.GetSeedClient(), nil, latestSet, &v2.FirewallList{}, func(fl *v2.FirewallList) []*v2.Firewall {
+		return fl.GetItems()
+	})
+	if err != nil {
+		return false, fmt.Errorf("unable to get owned firewalls: %w", err)
+	}
+
+	if len(ownedFirewalls) == 0 {
+		return false, err
+	}
+
+	v2.SortFirewallsByImportance(ownedFirewalls)
+
+	fw := ownedFirewalls[0] // this is the currently active one
+
+	if fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" {
+		return false, err
+	}
+
+	return pointer.SafeDeref(image.ID) != fw.Status.MachineStatus.ImageID, nil
 }
 
 func networksHaveChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool {

controllers/deployment/reconcile_test.go

@@ -0,0 +1,219 @@
+package deployment
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/go-logr/logr"
+	"github.com/google/go-cmp/cmp"
+	v2 "github.com/metal-stack/firewall-controller-manager/api/v2"
+	"github.com/metal-stack/firewall-controller-manager/api/v2/config"
+	"github.com/metal-stack/firewall-controller-manager/controllers"
+	"github.com/metal-stack/metal-go/api/client/image"
+	"github.com/metal-stack/metal-go/api/models"
+	metaltestclient "github.com/metal-stack/metal-go/test/client"
+	"github.com/metal-stack/metal-lib/pkg/cache"
+	"github.com/metal-stack/metal-lib/pkg/pointer"
+	"github.com/metal-stack/metal-lib/pkg/testcommon"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func Test_controller_osImageHasChanged(t *testing.T) {
+	ctx := context.Background()
+	scheme := runtime.NewScheme()
+	err := v2.AddToScheme(scheme)
+	require.NoError(t, err)
+
+	latestSet := &v2.FirewallSet{
+		ObjectMeta: v1.ObjectMeta{
+			Namespace: "firewall",
+		},
+	}
+
+	tests := []struct {
+		name              string
+		newS              *v2.FirewallSpec
+		oldS              *v2.FirewallSpec
+		mocks             *metaltestclient.MetalMockFns
+		existingFws       []v2.Firewall
+		want              bool
+		withinMaintenance bool
+		wantErr           error
+	}{
+		{
+			name: "image was updated",
+			oldS: &v2.FirewallSpec{
+				Image: "a",
+			},
+			newS: &v2.FirewallSpec{
+				Image: "b",
+			},
+			withinMaintenance: false,
+			want:              true,
+			wantErr:           nil,
+		},
+		{
+			name: "image was updated in maintenance",
+			oldS: &v2.FirewallSpec{
+				Image: "a",
+			},
+			newS: &v2.FirewallSpec{
+				Image: "b",
+			},
+			withinMaintenance: true,
+			want:              true,
+			wantErr:           nil,
+		},
+		{
+			name: "image might auto-update but not in maintenance mode",
+			oldS: &v2.FirewallSpec{
+				Image: "a",
+			},
+			newS: &v2.FirewallSpec{
+				Image: "a",
+			},
+			withinMaintenance: false,
+			want:              false,
+			wantErr:           nil,
+		},
+		{
+			name: "no auto-update because no shorthand image used",
+			oldS: &v2.FirewallSpec{
+				Image: "firewall-ubuntu-3.0.20240503",
+			},
+			newS: &v2.FirewallSpec{
+				Image: "firewall-ubuntu-3.0.20240503",
+			},
+			mocks: &metaltestclient.MetalMockFns{
+				Image: func(mock *mock.Mock) {
+					mock.On("FindLatestImage", image.NewFindLatestImageParams().WithID("firewall-ubuntu-3.0.20240503").WithContext(ctx), nil).Return(&image.FindLatestImageOK{
+						Payload: &models.V1ImageResponse{
+							ID: pointer.Pointer("firewall-ubuntu-3.0.20240503"),
+						},
+					}, nil)
+				},
+			},
+			withinMaintenance: true,
+			want:              false,
+			wantErr:           nil,
+		},
+		{
+			name: "no auto-update because firewall already runs latest image",
+			oldS: &v2.FirewallSpec{
+				Image: "firewall-ubuntu-3.0",
+			},
+			newS: &v2.FirewallSpec{
+				Image: "firewall-ubuntu-3.0",
+			},
+			mocks: &metaltestclient.MetalMockFns{
+				Image: func(mock *mock.Mock) {
+					mock.On("FindLatestImage", image.NewFindLatestImageParams().WithID("firewall-ubuntu-3.0").WithContext(ctx), nil).Return(&image.FindLatestImageOK{
+						Payload: &models.V1ImageResponse{
+							ID: pointer.Pointer("firewall-ubuntu-3.0.20240503"),
+						},
+					}, nil)
+				},
+			},
+			existingFws: []v2.Firewall{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name:      "a",
+						Namespace: "firewall",
+						OwnerReferences: []v1.OwnerReference{
+							*v1.NewControllerRef(latestSet, v2.GroupVersion.WithKind("FirewallSet")),
+						},
+					},
+					Status: v2.FirewallStatus{
+						MachineStatus: &v2.MachineStatus{
+							ImageID: "firewall-ubuntu-3.0.20240503",
+						},
+					},
+				},
+			},
+			withinMaintenance: true,
+			want:              false,
+			wantErr:           nil,
+		},
+		{
+			name: "auto-update because firewall not running latest image",
+			oldS: &v2.FirewallSpec{
+				Image: "firewall-ubuntu-3.0",
+			},
+			newS: &v2.FirewallSpec{
+				Image: "firewall-ubuntu-3.0",
+			},
+			mocks: &metaltestclient.MetalMockFns{
+				Image: func(mock *mock.Mock) {
+					mock.On("FindLatestImage", image.NewFindLatestImageParams().WithID("firewall-ubuntu-3.0").WithContext(ctx), nil).Return(&image.FindLatestImageOK{
+						Payload: &models.V1ImageResponse{
+							ID: pointer.Pointer("firewall-ubuntu-3.0.20240503"),
+						},
+					}, nil)
+				},
+			},
+			existingFws: []v2.Firewall{
+				{
+					ObjectMeta: v1.ObjectMeta{
+						Name:      "a",
+						Namespace: "firewall",
+						OwnerReferences: []v1.OwnerReference{
+							*v1.NewControllerRef(latestSet, v2.GroupVersion.WithKind("FirewallSet")),
+						},
+					},
+					Status: v2.FirewallStatus{
+						MachineStatus: &v2.MachineStatus{
+							ImageID: "firewall-ubuntu-3.0.20240501",
+						},
+					},
+				},
+			},
+			withinMaintenance: true,
+			want:              true,
+			wantErr:           nil,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, mc := metaltestclient.NewMetalMockClient(t, tt.mocks)
+
+			cc, err := config.New(&config.NewControllerConfig{
+				SeedClient:     fake.NewClientBuilder().WithScheme(scheme).WithLists(&v2.FirewallList{Items: tt.existingFws}).Build(),
+				SkipValidation: true,
+			})
+			require.NoError(t, err)
+
+			c := &controller{
+				c: cc,
+				imageCache: cache.New(5*time.Minute, func(ctx context.Context, id string) (*models.V1ImageResponse, error) {
+					resp, err := mc.Image().FindLatestImage(image.NewFindLatestImageParams().WithID(id).WithContext(ctx), nil)
+					if err != nil {
+						return nil, fmt.Errorf("latest firewall image %q not found: %w", id, err)
+					}
+
+					return resp.Payload, nil
+				}),
+			}
+
+			r := &controllers.Ctx[*v2.FirewallDeployment]{
+				Ctx:               ctx,
+				Log:               logr.Logger{},
+				Target:            &v2.FirewallDeployment{},
+				WithinMaintenance: tt.withinMaintenance,
+			}
+
+			got, err := c.osImageHasChanged(r, latestSet, tt.newS, tt.oldS)
+			if diff := cmp.Diff(tt.wantErr, err, testcommon.ErrorStringComparer()); diff != "" {
+				t.Errorf("error diff (+got -want):\n %s", diff)
+			}
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("diff (+got -want):\n %s", diff)
+			}
+		})
+	}
+}