From 731288030e6e860e9dab41a81645425243aad397 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Thu, 31 Aug 2023 13:42:04 +0200 Subject: [PATCH 01/16] Fix OS image update not detected for versions without suffix.. --- controllers/deployment/reconcile.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 52289fb..a529c98 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -215,15 +215,13 @@ func sizeHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { } func osImageHasChanged(ctx context.Context, m metalgo.Client, 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) - } + 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) + } - if image.Payload != nil && image.Payload.ID != nil && *image.Payload.ID != oldS.Image { - return true, nil - } + if image.Payload != nil && image.Payload.ID != nil && *image.Payload.ID != oldS.Image { + return true, nil } return false, nil From 742783da9f5255552db375f6b82b2446df4e089e Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 1 Sep 2023 10:22:00 +0200 Subject: [PATCH 02/16] Adding cache. --- controllers/deployment/controller.go | 14 ++++++++++++++ controllers/deployment/reconcile.go | 13 ++++++------- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index 0af5a13..282db5d 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -1,6 +1,8 @@ package deployment import ( + "context" + "fmt" "time" "github.com/go-logr/logr" @@ -14,6 +16,9 @@ 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 { @@ -21,6 +26,7 @@ type controller struct { 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). diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index a529c98..525efb4 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -9,8 +9,7 @@ import ( "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" "sigs.k8s.io/controller-runtime/pkg/client" @@ -192,7 +191,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.Ctx, newS, oldS) if err != nil { return false, err } @@ -214,13 +213,13 @@ 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) { - image, err := m.Image().FindLatestImage(image.NewFindLatestImageParams().WithID(newS.Image).WithContext(ctx), nil) +func (c *controller) osImageHasChanged(ctx context.Context, newS *v2.FirewallSpec, oldS *v2.FirewallSpec) (bool, error) { + image, err := c.imageCache.Get(ctx, newS.Image) if err != nil { - return false, fmt.Errorf("latest firewall image not found:%s %w", newS.Image, err) + return false, err } - if image.Payload != nil && image.Payload.ID != nil && *image.Payload.ID != oldS.Image { + if pointer.SafeDeref(image.ID) != oldS.Image { return true, nil } From 9aff591eddd0ac6aaf3670899f22dae7948755d2 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 1 Sep 2023 10:38:34 +0200 Subject: [PATCH 03/16] Fix tests. --- integration/integration_test.go | 34 +++++++++++++++++++++++++++++ integration/metal_resources_test.go | 11 +++++----- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 4e3ded5..638de11 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -19,6 +19,7 @@ import ( testcommon "github.com/metal-stack/firewall-controller-manager/integration/common" metalfirewall "github.com/metal-stack/metal-go/api/client/firewall" + "github.com/metal-stack/metal-go/api/client/image" "github.com/metal-stack/metal-go/api/client/machine" "github.com/metal-stack/metal-go/api/client/network" "github.com/metal-stack/metal-go/api/models" @@ -176,6 +177,9 @@ var _ = Context("integration test", Ordered, func() { Machine: func(m *mock.Mock) { m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Create(ctx, deployment())).To(Succeed()) @@ -437,6 +441,9 @@ var _ = Context("integration test", Ordered, func() { Machine: func(m *mock.Mock) { m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) deploy := deployment() @@ -729,6 +736,9 @@ var _ = Context("integration test", Ordered, func() { Network: func(m *mock.Mock) { m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) // refetch @@ -816,6 +826,9 @@ var _ = Context("integration test", Ordered, func() { m.On("FreeMachine", mock.Anything, nil).Return(&machine.FreeMachineOK{Payload: &models.V1MachineResponse{ID: firewall1.ID}}, nil).Maybe() m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Delete(ctx, deployment())).To(Succeed()) @@ -859,6 +872,9 @@ var _ = Context("integration test", Ordered, func() { Machine: func(m *mock.Mock) { m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) deploy := deployment() @@ -1150,6 +1166,9 @@ var _ = Context("integration test", Ordered, func() { m.On("FreeMachine", mock.Anything, nil).Return(nil, &machine.FreeMachineDefault{Payload: httperrors.Conflict(fmt.Errorf("deletion blocked"))}).Maybe() m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) deploy.Spec.Template.Spec.Networks = []string{"internet", "mpls"} @@ -1186,6 +1205,9 @@ var _ = Context("integration test", Ordered, func() { m.On("FreeMachine", mock.Anything, nil).Return(&machine.FreeMachineOK{Payload: &models.V1MachineResponse{ID: firewall1.ID}}, nil).Maybe() m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Eventually(func() bool { @@ -1251,6 +1273,9 @@ var _ = Context("integration test", Ordered, func() { Network: func(m *mock.Mock) { m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(newMon), newMon)).To(Succeed()) // refetch @@ -1501,6 +1526,9 @@ var _ = Context("integration test", Ordered, func() { m.On("FreeMachine", mock.Anything, nil).Return(&machine.FreeMachineOK{Payload: &models.V1MachineResponse{ID: firewall1.ID}}, nil).Maybe() m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Delete(ctx, deployment())).To(Succeed()) @@ -1599,6 +1627,9 @@ var _ = Context("integration test", Ordered, func() { Machine: func(m *mock.Mock) { m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Create(ctx, fw)).To(Succeed()) @@ -1805,6 +1836,9 @@ var _ = Context("integration test", Ordered, func() { m.On("FreeMachine", mock.Anything, nil).Return(&machine.FreeMachineOK{Payload: &models.V1MachineResponse{ID: firewall1.ID}}, nil).Maybe() m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe() }, + Image: func(m *mock.Mock) { + m.On("FindLatestImage", mock.Anything, nil).Return(&image.FindLatestImageOK{Payload: image1}, nil).Maybe() + }, }) Expect(k8sClient.Delete(ctx, deployment())).To(Succeed()) diff --git a/integration/metal_resources_test.go b/integration/metal_resources_test.go index 6431952..411dd6b 100644 --- a/integration/metal_resources_test.go +++ b/integration/metal_resources_test.go @@ -247,13 +247,12 @@ var ( imageExpiration = pointer.Pointer(strfmt.DateTime(testTime.Add(3 * 24 * time.Hour))) image1 = &models.V1ImageResponse{ Classification: "supported", - Description: "debian-description", + Description: "firewall-image-description", ExpirationDate: imageExpiration, - Features: []string{"machine"}, - ID: pointer.Pointer("debian"), - Name: "debian-name", - URL: "debian-url", - Usedby: []string{"456"}, + Features: []string{"firewall"}, + ID: pointer.Pointer("firewall-ubuntu-2.0"), + Name: "firewall-image-name", + URL: "firewall-image-url", } partition1 = &models.V1PartitionResponse{ Bootconfig: &models.V1PartitionBootConfiguration{ From 1b1523ae527d37ae70f3f3490b42a869932656bd Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Wed, 24 Apr 2024 08:35:13 +0200 Subject: [PATCH 04/16] Update. --- api/v2/types_firewall.go | 2 ++ ...ewall.metal-stack.io_firewallmonitors.yaml | 4 +++ .../firewall.metal-stack.io_firewalls.yaml | 4 +++ controllers/deployment/reconcile.go | 36 +++++++++++++++---- controllers/firewall/status.go | 13 ++++--- 5 files changed, 48 insertions(+), 11 deletions(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index e768905..34b18e8 100644 --- a/api/v2/types_firewall.go +++ b/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. diff --git a/config/crds/firewall.metal-stack.io_firewallmonitors.yaml b/config/crds/firewall.metal-stack.io_firewallmonitors.yaml index c73f36a..dc859c3 100644 --- a/config/crds/firewall.metal-stack.io_firewallmonitors.yaml +++ b/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. diff --git a/config/crds/firewall.metal-stack.io_firewalls.yaml b/config/crds/firewall.metal-stack.io_firewalls.yaml index f0f56f9..c82118b 100644 --- a/config/crds/firewall.metal-stack.io_firewalls.yaml +++ b/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. diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 8d9472a..4147034 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -1,7 +1,6 @@ package deployment import ( - "context" "fmt" "strconv" "time" @@ -205,7 +204,7 @@ func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment] return ok, nil } - ok, err := c.osImageHasChanged(r.Ctx, newS, oldS) + ok, err := c.osImageHasChanged(r, latestSet, newS, oldS) if err != nil { return false, err } @@ -227,14 +226,39 @@ func sizeHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { return newS.Size != oldS.Size } -func (c *controller) osImageHasChanged(ctx context.Context, newS *v2.FirewallSpec, oldS *v2.FirewallSpec) (bool, error) { - image, err := c.imageCache.Get(ctx, newS.Image) +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 { + return true, nil + } + + if r.WithinMaintenance { + return false, nil + } + + // let's resolve the latest image from the api in case a shorthand image flag is being used + // then compare to the actual image deployed on the firewalls in this set + + image, err := c.imageCache.Get(r.Ctx, newS.Image) if err != nil { return false, err } - if pointer.SafeDeref(image.ID) != oldS.Image { - return true, nil + 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) + } + + for _, fw := range ownedFirewalls { + if fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" { + continue + } + + if pointer.SafeDeref(image.ID) != fw.Status.MachineStatus.ImageID { + // when there is one firewall where the image does not match, let's roll the set + return true, nil + } } return false, nil diff --git a/controllers/firewall/status.go b/controllers/firewall/status.go index 29c501d..693fbe8 100644 --- a/controllers/firewall/status.go +++ b/controllers/firewall/status.go @@ -8,6 +8,7 @@ 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/models" + "github.com/metal-stack/metal-lib/pkg/pointer" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -45,14 +46,16 @@ func setMachineStatus(fw *v2.Firewall, f *models.V1FirewallResponse) error { } func getMachineStatus(f *models.V1FirewallResponse) (*v2.MachineStatus, error) { - if f.ID == nil || f.Allocation == nil || f.Allocation.Created == nil || f.Liveliness == nil { + if f.ID == nil || f.Allocation == nil || f.Allocation.Created == nil || f.Liveliness == nil || f.Allocation.Image == nil { return nil, fmt.Errorf("firewall entity from metal-api is missing essential fields") } - result := &v2.MachineStatus{} - result.MachineID = *f.ID - result.AllocationTimestamp = metav1.NewTime(time.Time(*f.Allocation.Created)) - result.Liveliness = *f.Liveliness + result := &v2.MachineStatus{ + MachineID: *f.ID, + AllocationTimestamp: metav1.NewTime(time.Time(*f.Allocation.Created)), + Liveliness: *f.Liveliness, + ImageID: pointer.SafeDeref(f.Allocation.Image.ID), + } if f.Events != nil && f.Events.CrashLoop != nil { result.CrashLoop = *f.Events.CrashLoop From b980e3660ab872ea4cc61cd7a8a8d10ab03ef833 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Thu, 2 May 2024 15:20:24 +0200 Subject: [PATCH 05/16] Better condition. --- controllers/deployment/reconcile.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 4147034..9f54111 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -250,18 +250,19 @@ func (c *controller) osImageHasChanged(r *controllers.Ctx[*v2.FirewallDeployment return false, fmt.Errorf("unable to get owned firewalls: %w", err) } - for _, fw := range ownedFirewalls { - if fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" { - continue - } + if len(ownedFirewalls) == 0 { + return false, err + } - if pointer.SafeDeref(image.ID) != fw.Status.MachineStatus.ImageID { - // when there is one firewall where the image does not match, let's roll the set - return true, nil - } + v2.SortFirewallsByImportance(ownedFirewalls) + + fw := ownedFirewalls[0] // this is the currently active one + + if fw.Status.Phase != v2.FirewallPhaseRunning || fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" { + return false, err } - return false, nil + return pointer.SafeDeref(image.ID) != fw.Status.MachineStatus.ImageID, nil } func networksHaveChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { From 176db9e3e8e9645c64a822fbc363b4dfed56502a Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Thu, 2 May 2024 15:22:48 +0200 Subject: [PATCH 06/16] Early return. --- controllers/deployment/reconcile.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 9f54111..4d0bf05 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -236,13 +236,18 @@ func (c *controller) osImageHasChanged(r *controllers.Ctx[*v2.FirewallDeployment } // let's resolve the latest image from the api in case a shorthand image flag is being used - // then compare to the actual image deployed on the firewalls in this set - 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() }) From fa2c3a669c5c3b76c7623468843a202218a81c52 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Fri, 3 May 2024 09:57:44 +0200 Subject: [PATCH 07/16] Provide test. --- api/v2/config/controller.go | 5 +- controllers/deployment/reconcile.go | 4 +- controllers/deployment/reconcile_test.go | 219 +++++++++++++++++++++++ 3 files changed, 225 insertions(+), 3 deletions(-) create mode 100644 controllers/deployment/reconcile_test.go diff --git a/api/v2/config/controller.go b/api/v2/config/controller.go index 74094ba..f12a185 100644 --- a/api/v2/config/controller.go +++ b/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 } diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 4d0bf05..dadea28 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -231,7 +231,7 @@ func (c *controller) osImageHasChanged(r *controllers.Ctx[*v2.FirewallDeployment return true, nil } - if r.WithinMaintenance { + if !r.WithinMaintenance { return false, nil } @@ -263,7 +263,7 @@ func (c *controller) osImageHasChanged(r *controllers.Ctx[*v2.FirewallDeployment fw := ownedFirewalls[0] // this is the currently active one - if fw.Status.Phase != v2.FirewallPhaseRunning || fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" { + if fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" { return false, err } diff --git a/controllers/deployment/reconcile_test.go b/controllers/deployment/reconcile_test.go new file mode 100644 index 0000000..acce903 --- /dev/null +++ b/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) + } + }) + } +} From 3107b07aefc89a067382eb2007adad8318ae4369 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Tue, 7 May 2024 12:03:48 +0200 Subject: [PATCH 08/16] Turn into own update controller. --- api/v2/types_firewalldeployment.go | 8 + api/v2/zz_generated.deepcopy.go | 16 ++ controllers/deployment/controller.go | 1 + controllers/deployment/reconcile.go | 50 +---- controllers/deployment/reconcile_test.go | 219 ------------------- controllers/update/controller.go | 68 ++++++ controllers/update/reconcile.go | 146 +++++++++++++ controllers/update/reconcile_test.go | 262 +++++++++++++++++++++++ main.go | 16 +- 9 files changed, 514 insertions(+), 272 deletions(-) delete mode 100644 controllers/deployment/reconcile_test.go create mode 100644 controllers/update/controller.go create mode 100644 controllers/update/reconcile.go create mode 100644 controllers/update/reconcile_test.go diff --git a/api/v2/types_firewalldeployment.go b/api/v2/types_firewalldeployment.go index d7b8268..69e01fa 100644 --- a/api/v2/types_firewalldeployment.go +++ b/api/v2/types_firewalldeployment.go @@ -46,6 +46,8 @@ type FirewallDeploymentSpec struct { // Replicas is the amount of firewall replicas targeted to be running. // Defaults to 1. Replicas int `json:"replicas,omitempty"` + // AutoUpdate defines the behavior for automatic updates. + AutoUpdate FirewallAutoUpdate `json:"autoUpdate"` // Selector is a label query over firewalls that should match the replicas count. // If selector is empty, it is defaulted to the labels present on the firewall template. // Label keys and values that must match in order to be controlled by this replication @@ -55,6 +57,12 @@ type FirewallDeploymentSpec struct { Template FirewallTemplateSpec `json:"template"` } +type FirewallAutoUpdate struct { + // MachineImage auto updates the os image of the firewall within the maintenance time window + // in case a newer version of the os is available. + MachineImage bool `json:"machineImage"` +} + // FirewallDeploymentStatus contains current status information on the firewall deployment. type FirewallDeploymentStatus struct { // TargetReplicas is the amount of firewall replicas targeted to be running. diff --git a/api/v2/zz_generated.deepcopy.go b/api/v2/zz_generated.deepcopy.go index b3cd8bc..b9d9399 100644 --- a/api/v2/zz_generated.deepcopy.go +++ b/api/v2/zz_generated.deepcopy.go @@ -208,6 +208,21 @@ func (in *Firewall) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FirewallAutoUpdate) DeepCopyInto(out *FirewallAutoUpdate) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FirewallAutoUpdate. +func (in *FirewallAutoUpdate) DeepCopy() *FirewallAutoUpdate { + if in == nil { + return nil + } + out := new(FirewallAutoUpdate) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *FirewallDeployment) DeepCopyInto(out *FirewallDeployment) { *out = *in @@ -270,6 +285,7 @@ func (in *FirewallDeploymentList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *FirewallDeploymentSpec) DeepCopyInto(out *FirewallDeploymentSpec) { *out = *in + out.AutoUpdate = in.AutoUpdate if in.Selector != nil { in, out := &in.Selector, &out.Selector *out = make(map[string]string, len(*in)) diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index b8ebe59..d987380 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -50,6 +50,7 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M &v2.FirewallDeployment{}, builder.WithPredicates( predicate.And( + predicate.Not(v2.AnnotationAddedPredicate(v2.MaintenanceAnnotation)), predicate.Not(v2.AnnotationRemovedPredicate(v2.MaintenanceAnnotation)), predicate.Or( predicate.GenerationChangedPredicate{}, // prevents reconcile on status sub resource update diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index dadea28..300758c 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -8,7 +8,6 @@ import ( "github.com/google/uuid" 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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/retry" @@ -204,10 +203,7 @@ func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment] return ok, nil } - ok, err := c.osImageHasChanged(r, latestSet, newS, oldS) - if err != nil { - return false, err - } + ok = osImageHasChanged(newS, oldS) if ok { r.Log.Info("firewall image has changed", "image", newS.Image) return ok, nil @@ -226,48 +222,8 @@ func sizeHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { return newS.Size != oldS.Size } -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 { - return true, nil - } - - if !r.WithinMaintenance { - 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 osImageHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { + return newS.Image != oldS.Image } func networksHaveChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { diff --git a/controllers/deployment/reconcile_test.go b/controllers/deployment/reconcile_test.go deleted file mode 100644 index acce903..0000000 --- a/controllers/deployment/reconcile_test.go +++ /dev/null @@ -1,219 +0,0 @@ -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) - } - }) - } -} diff --git a/controllers/update/controller.go b/controllers/update/controller.go new file mode 100644 index 0000000..991adde --- /dev/null +++ b/controllers/update/controller.go @@ -0,0 +1,68 @@ +package update + +import ( + "context" + "time" + + "github.com/go-logr/logr" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + 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" + metalgo "github.com/metal-stack/metal-go" + "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 + recorder record.EventRecorder + imageCache *cache.Cache[string, *models.V1ImageResponse] +} + +func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.Manager, c *config.ControllerConfig) error { + g := controllers.NewGenericController(log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ + c: c, + log: log, + recorder: recorder, + imageCache: newImageCache(c.GetMetal()), + }).WithoutStatus() + + return ctrl.NewControllerManagedBy(mgr). + For( + &v2.FirewallDeployment{}, + builder.WithPredicates( + v2.AnnotationAddedPredicate(v2.MaintenanceAnnotation), + ), + ). + Named("FirewallDeployment"). + WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). + Complete(g) +} + +func (c *controller) New() *v2.FirewallDeployment { + return &v2.FirewallDeployment{} +} + +func (c *controller) SetStatus(_ *v2.FirewallDeployment, _ *v2.FirewallDeployment) {} + +func (c *controller) Delete(_ *controllers.Ctx[*v2.FirewallDeployment]) error { + return nil +} + +func newImageCache(m metalgo.Client) *cache.Cache[string, *models.V1ImageResponse] { + return cache.New(5*time.Minute, func(ctx context.Context, id string) (*models.V1ImageResponse, error) { + resp, err := m.Image().FindLatestImage(image.NewFindLatestImageParams().WithID(id).WithContext(ctx), nil) + if err != nil { + return nil, err + } + + return resp.Payload, nil + }) +} diff --git a/controllers/update/reconcile.go b/controllers/update/reconcile.go new file mode 100644 index 0000000..c3e57d7 --- /dev/null +++ b/controllers/update/reconcile.go @@ -0,0 +1,146 @@ +package update + +import ( + "errors" + "fmt" + "strconv" + "strings" + + "github.com/Masterminds/semver/v3" + v2 "github.com/metal-stack/firewall-controller-manager/api/v2" + "github.com/metal-stack/firewall-controller-manager/controllers" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error { + return c.autoUpdateOS(r) +} + +func (c *controller) autoUpdateOS(r *controllers.Ctx[*v2.FirewallDeployment]) error { + if !r.Target.Spec.AutoUpdate.MachineImage { + return nil + } + + if !r.WithinMaintenance { + c.log.Info("not checking for newer os image, not in maintenance time window") + return nil + } + + c.log.Info("checking for newer os image") + + // first, let's resolve the latest image from the api + + os, version, err := getOsAndSemverFromImage(r.Target.Spec.Template.Spec.Image) + if err != nil { + return fmt.Errorf("image version cannot be parsed: %w", err) + } + + var ( + imageToResolve = r.Target.Spec.Template.Spec.Image + isFullyQualifiedImageNotation = version.Patch() != 0 + ) + + if isFullyQualifiedImageNotation { + imageToResolve = fmt.Sprintf("%s-%d.%d", os, version.Major(), version.Minor()) + } + + image, err := c.imageCache.Get(r.Ctx, imageToResolve) + if err != nil { + return fmt.Errorf("unable to retrieve latest os image from metal-api: %w", err) + } + + if image.ID == nil { + return fmt.Errorf("returned image from metal-api contains no id") + } + + // now figure out the actual running image of the current firewall + // this can be different from specification in case a shorthand image notation is being used + // so we need the exact version running + + ownedSets, _, err := controllers.GetOwnedResources(r.Ctx, c.c.GetSeedClient(), nil, r.Target, &v2.FirewallSetList{}, func(fsl *v2.FirewallSetList) []*v2.FirewallSet { + return fsl.GetItems() + }) + if err != nil { + return fmt.Errorf("unable to get owned sets: %w", err) + } + + latestSet, err := controllers.MaxRevisionOf(ownedSets) + if err != nil { + return err + } + + 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 fmt.Errorf("unable to get owned firewalls: %w", err) + } + + v2.SortFirewallsByImportance(ownedFirewalls) + + if len(ownedFirewalls) == 0 { + return nil + } + + fw := ownedFirewalls[0] // this is the currently active one + + if fw.Status.MachineStatus == nil || fw.Status.MachineStatus.ImageID == "" { + return nil + } + + // finally, we can do the image comparison + + if *image.ID == fw.Status.MachineStatus.ImageID { + r.Log.Info("no new os version available, not triggering auto-update") + return nil + } + + r.Log.Info("newer os version is available, triggering auto-update") + + err = retry.RetryOnConflict(retry.DefaultBackoff, func() error { + refetched := &v2.FirewallDeployment{} + err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKeyFromObject(r.Target), refetched) + if err != nil { + return fmt.Errorf("unable re-fetch firewall deployment: %w", err) + } + + if refetched.Annotations == nil { + refetched.Annotations = map[string]string{} + } + + refetched.Annotations[v2.RollSetAnnotation] = strconv.FormatBool(true) + if !isFullyQualifiedImageNotation { + refetched.Spec.Template.Spec.Image = *image.ID + } + + err = c.c.GetSeedClient().Update(r.Ctx, refetched) + if err != nil { + return fmt.Errorf("unable to update firewall deployment: %w", err) + } + + return nil + }) + if err != nil { + return err + } + + return nil +} + +// copied over from metal-api because this is only available in internal package +func getOsAndSemverFromImage(id string) (string, *semver.Version, error) { + imageParts := strings.Split(id, "-") + if len(imageParts) < 2 { + return "", nil, errors.New("image does not contain a version") + } + + parts := len(imageParts) - 1 + os := strings.Join(imageParts[:parts], "-") + version := strings.Join(imageParts[parts:], "") + v, err := semver.NewVersion(version) + if err != nil { + return "", nil, err + } + return os, v, nil +} diff --git a/controllers/update/reconcile_test.go b/controllers/update/reconcile_test.go new file mode 100644 index 0000000..fcbfb05 --- /dev/null +++ b/controllers/update/reconcile_test.go @@ -0,0 +1,262 @@ +package update + +import ( + "context" + "strconv" + "testing" + + "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/pointer" + "github.com/metal-stack/metal-lib/pkg/testcommon" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "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) + + tests := []struct { + name string + metalMocks *metaltestclient.MetalMockFns + fwDeploy *v2.FirewallDeployment + existingFws []v2.Firewall + postTestFn func(t *testing.T, c client.Client) + want bool + withinMaintenance bool + wantErr error + }{ + { + name: "auto-update disabled", + fwDeploy: &v2.FirewallDeployment{ + Spec: v2.FirewallDeploymentSpec{ + Template: v2.FirewallTemplateSpec{ + Spec: v2.FirewallSpec{ + Image: "a", + }, + }, + AutoUpdate: v2.FirewallAutoUpdate{ + MachineImage: false, + }, + }, + }, + withinMaintenance: true, + wantErr: nil, + }, + { + name: "not in maintenance time window", + fwDeploy: &v2.FirewallDeployment{ + Spec: v2.FirewallDeploymentSpec{ + Template: v2.FirewallTemplateSpec{ + Spec: v2.FirewallSpec{ + Image: "a", + }, + }, + AutoUpdate: v2.FirewallAutoUpdate{ + MachineImage: true, + }, + }, + }, + withinMaintenance: false, + wantErr: nil, + }, + { + name: "not in maintenance time window", + fwDeploy: &v2.FirewallDeployment{ + Spec: v2.FirewallDeploymentSpec{ + Template: v2.FirewallTemplateSpec{ + Spec: v2.FirewallSpec{ + Image: "a", + }, + }, + AutoUpdate: v2.FirewallAutoUpdate{ + MachineImage: true, + }, + }, + }, + withinMaintenance: false, + wantErr: nil, + }, + { + name: "auto-update when using shorthand image notation", + fwDeploy: &v2.FirewallDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment", + Namespace: "firewall", + }, + Spec: v2.FirewallDeploymentSpec{ + Template: v2.FirewallTemplateSpec{ + Spec: v2.FirewallSpec{ + Image: "firewall-ubuntu-3.0", + }, + }, + AutoUpdate: v2.FirewallAutoUpdate{ + MachineImage: true, + }, + }, + }, + withinMaintenance: true, + metalMocks: &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: metav1.ObjectMeta{ + Name: "a", + Namespace: "firewall", + }, + Status: v2.FirewallStatus{ + MachineStatus: &v2.MachineStatus{ + ImageID: "firewall-ubuntu-3.0.20240101", + }, + }, + }, + }, + postTestFn: func(t *testing.T, c client.Client) { + fwdeploy := &v2.FirewallDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment", + Namespace: "firewall", + }, + } + err := c.Get(context.Background(), client.ObjectKeyFromObject(fwdeploy), fwdeploy) + require.NoError(t, err) + + assert.Equal(t, "firewall-ubuntu-3.0", fwdeploy.Spec.Template.Spec.Image) + assert.Equal(t, fwdeploy.Annotations[v2.RollSetAnnotation], strconv.FormatBool(true)) + }, + wantErr: nil, + }, + { + name: "auto-update when using fully-qualified image notation", + fwDeploy: &v2.FirewallDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment", + Namespace: "firewall", + }, + Spec: v2.FirewallDeploymentSpec{ + Template: v2.FirewallTemplateSpec{ + Spec: v2.FirewallSpec{ + Image: "firewall-ubuntu-3.0.20230101", + }, + }, + AutoUpdate: v2.FirewallAutoUpdate{ + MachineImage: true, + }, + }, + }, + withinMaintenance: true, + metalMocks: &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: metav1.ObjectMeta{ + Name: "a", + Namespace: "firewall", + }, + Status: v2.FirewallStatus{ + MachineStatus: &v2.MachineStatus{ + ImageID: "firewall-ubuntu-3.0.20230101", + }, + }, + }, + }, + postTestFn: func(t *testing.T, c client.Client) { + fwdeploy := &v2.FirewallDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment", + Namespace: "firewall", + }, + } + err := c.Get(context.Background(), client.ObjectKeyFromObject(fwdeploy), fwdeploy) + require.NoError(t, err) + + assert.Equal(t, "firewall-ubuntu-3.0.20240503", fwdeploy.Spec.Template.Spec.Image) + assert.Equal(t, fwdeploy.Annotations[v2.RollSetAnnotation], strconv.FormatBool(true)) + }, + wantErr: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, mc := metaltestclient.NewMetalMockClient(t, tt.metalMocks) + + latestSet := v2.FirewallSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "firewall", + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(tt.fwDeploy, v2.GroupVersion.WithKind("FirewallDeployment")), + }, + }, + } + + var ownedFirewalls []v2.Firewall + for _, existingFw := range tt.existingFws { + ownedFirewall := existingFw.DeepCopy() + ownedFirewall.ObjectMeta.OwnerReferences = []metav1.OwnerReference{ + *metav1.NewControllerRef(&latestSet, v2.GroupVersion.WithKind("FirewallSet")), + } + ownedFirewalls = append(ownedFirewalls, *ownedFirewall) + } + + client := fake.NewClientBuilder().WithScheme(scheme).WithLists( + &v2.FirewallSetList{Items: []v2.FirewallSet{latestSet}}, + &v2.FirewallList{Items: ownedFirewalls}, + ).WithObjects(tt.fwDeploy).Build() + + cc, err := config.New(&config.NewControllerConfig{ + SeedClient: client, + SkipValidation: true, + }) + require.NoError(t, err) + + c := &controller{ + c: cc, + imageCache: newImageCache(mc), + } + + r := &controllers.Ctx[*v2.FirewallDeployment]{ + Ctx: ctx, + Log: logr.Logger{}, + Target: tt.fwDeploy, + WithinMaintenance: tt.withinMaintenance, + } + + err = c.autoUpdateOS(r) + if diff := cmp.Diff(tt.wantErr, err, testcommon.ErrorStringComparer()); diff != "" { + t.Errorf("error diff (+got -want):\n %s", diff) + } + + if tt.postTestFn != nil { + tt.postTestFn(t, client) + } + }) + } +} diff --git a/main.go b/main.go index e6a2893..db9ccce 100644 --- a/main.go +++ b/main.go @@ -33,6 +33,7 @@ import ( "github.com/metal-stack/firewall-controller-manager/controllers/firewall" "github.com/metal-stack/firewall-controller-manager/controllers/monitor" "github.com/metal-stack/firewall-controller-manager/controllers/set" + "github.com/metal-stack/firewall-controller-manager/controllers/update" ) const ( @@ -135,7 +136,7 @@ func main() { Cache: cache.Options{ SyncPeriod: &reconcileInterval, DefaultNamespaces: map[string]cache.Config{ - namespace: cache.Config{}, + namespace: {}, }, }, HealthProbeBindAddress: healthAddr, @@ -238,7 +239,7 @@ func main() { LeaderElection: false, Cache: cache.Options{ DefaultNamespaces: map[string]cache.Config{ - v2.FirewallShootNamespace: cache.Config{}, + v2.FirewallShootNamespace: {}, }, }, GracefulShutdownTimeout: pointer.Pointer(time.Duration(0)), @@ -272,16 +273,19 @@ func main() { } if err := deployment.SetupWithManager(ctrl.Log.WithName("controllers").WithName("deployment"), seedMgr.GetEventRecorderFor("firewall-deployment-controller"), seedMgr, cc); err != nil { - log.Fatalf("unable to setup controller deployment %v", err) + log.Fatalf("unable to setup deployment controller: %v", err) } if err := set.SetupWithManager(ctrl.Log.WithName("controllers").WithName("set"), seedMgr.GetEventRecorderFor("firewall-set-controller"), seedMgr, cc); err != nil { - log.Fatalf("unable to setup controller set %v", err) + log.Fatalf("unable to setup set controller: %v", err) } if err := firewall.SetupWithManager(ctrl.Log.WithName("controllers").WithName("firewall"), seedMgr.GetEventRecorderFor("firewall-controller"), seedMgr, cc); err != nil { - log.Fatalf("unable to setup controller firewall %v", err) + log.Fatalf("unable to setup firewall controller: %v", err) } if err := monitor.SetupWithManager(ctrl.Log.WithName("controllers").WithName("firewall-monitor"), shootMgr, cc); err != nil { - log.Fatalf("unable to setup controller monitor %v", err) + log.Fatalf("unable to setup monitor controller: %v", err) + } + if err := update.SetupWithManager(ctrl.Log.WithName("controllers").WithName("update"), seedMgr.GetEventRecorderFor("update-controller"), seedMgr, cc); err != nil { + log.Fatalf("unable to setup update controller: %v", err) } if err := deployment.SetupWebhookWithManager(ctrl.Log.WithName("defaulting-webhook"), seedMgr, cc); err != nil { From c340e817328fbf9d958e5317b2439817b6dd86de Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Tue, 7 May 2024 12:06:48 +0200 Subject: [PATCH 09/16] Cleanup deployment-controller. --- controllers/deployment/controller.go | 14 -------------- controllers/deployment/reconcile.go | 12 ++++++------ controllers/deployment/recreate.go | 9 ++------- controllers/deployment/rolling.go | 9 ++------- 4 files changed, 10 insertions(+), 34 deletions(-) diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index d987380..6366b87 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -1,8 +1,6 @@ package deployment import ( - "context" - "fmt" "time" "github.com/go-logr/logr" @@ -16,9 +14,6 @@ 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 { @@ -26,7 +21,6 @@ type controller struct { 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 { @@ -35,14 +29,6 @@ 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). diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 300758c..ecc82ae 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -186,10 +186,10 @@ func (c *controller) syncFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], return nil } -func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment], latestSet *v2.FirewallSet) (bool, error) { +func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment], latestSet *v2.FirewallSet) bool { if v2.IsAnnotationTrue(latestSet, v2.RollSetAnnotation) { r.Log.Info("set roll initiated by annotation") - return true, nil + return true } var ( @@ -200,22 +200,22 @@ func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment] ok := sizeHasChanged(newS, oldS) if ok { r.Log.Info("firewall size has changed", "size", newS.Size) - return ok, nil + return ok } ok = osImageHasChanged(newS, oldS) if ok { r.Log.Info("firewall image has changed", "image", newS.Image) - return ok, nil + return ok } ok = networksHaveChanged(newS, oldS) if ok { r.Log.Info("firewall networks have changed", "networks", newS.Networks) - return ok, nil + return ok } - return false, nil + return false } func sizeHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { diff --git a/controllers/deployment/recreate.go b/controllers/deployment/recreate.go index 819a43a..b7b5322 100644 --- a/controllers/deployment/recreate.go +++ b/controllers/deployment/recreate.go @@ -11,12 +11,7 @@ import ( // 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, latestSet *v2.FirewallSet) error { - newSetRequired, err := c.isNewSetRequired(r, latestSet) - if err != nil { - return err - } - - if newSetRequired { + if c.isNewSetRequired(r, latestSet) { r.Log.Info("significant changes detected in the spec, create new scaled down firewall set, then cleaning up old sets") set, err := c.createNextFirewallSet(r, latestSet, &setOverrides{ @@ -31,7 +26,7 @@ func (c *controller) recreateStrategy(r *controllers.Ctx[*v2.FirewallDeployment] latestSet = set } - err = c.deleteFirewallSets(r, controllers.Except(ownedSets, latestSet)...) + err := c.deleteFirewallSets(r, controllers.Except(ownedSets, latestSet)...) if err != nil { return err } diff --git a/controllers/deployment/rolling.go b/controllers/deployment/rolling.go index 1875002..5a5e08a 100644 --- a/controllers/deployment/rolling.go +++ b/controllers/deployment/rolling.go @@ -10,12 +10,7 @@ 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, latestSet *v2.FirewallSet) error { - newSetRequired, err := c.isNewSetRequired(r, latestSet) - if err != nil { - return err - } - - if newSetRequired { + if c.isNewSetRequired(r, latestSet) { r.Log.Info("significant changes detected in the spec, creating new firewall set", "distance", v2.FirewallRollingUpdateSetDistance) newSet, err := c.createNextFirewallSet(r, latestSet, &setOverrides{ @@ -32,7 +27,7 @@ func (c *controller) rollingUpdateStrategy(r *controllers.Ctx[*v2.FirewallDeploy return c.cleanupIntermediateSets(r, ownedSets) } - err = c.syncFirewallSet(r, latestSet) + err := c.syncFirewallSet(r, latestSet) if err != nil { return fmt.Errorf("unable to update firewall set: %w", err) } From cf227b1260d011b28167c49cfc7961e9cea22408 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Tue, 7 May 2024 12:08:27 +0200 Subject: [PATCH 10/16] Do not change condition. --- controllers/deployment/controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index 6366b87..a604540 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -36,7 +36,6 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M &v2.FirewallDeployment{}, builder.WithPredicates( predicate.And( - predicate.Not(v2.AnnotationAddedPredicate(v2.MaintenanceAnnotation)), predicate.Not(v2.AnnotationRemovedPredicate(v2.MaintenanceAnnotation)), predicate.Or( predicate.GenerationChangedPredicate{}, // prevents reconcile on status sub resource update From 5d848ecfa1d8d85bdedb232cafd3c1637f82a316 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Tue, 7 May 2024 14:29:39 +0200 Subject: [PATCH 11/16] Forgot var renaming. --- controllers/update/reconcile.go | 2 +- controllers/update/reconcile_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/update/reconcile.go b/controllers/update/reconcile.go index c3e57d7..fa6f0b6 100644 --- a/controllers/update/reconcile.go +++ b/controllers/update/reconcile.go @@ -110,7 +110,7 @@ func (c *controller) autoUpdateOS(r *controllers.Ctx[*v2.FirewallDeployment]) er } refetched.Annotations[v2.RollSetAnnotation] = strconv.FormatBool(true) - if !isFullyQualifiedImageNotation { + if isFullyQualifiedImageNotation { refetched.Spec.Template.Spec.Image = *image.ID } diff --git a/controllers/update/reconcile_test.go b/controllers/update/reconcile_test.go index fcbfb05..6ba56f8 100644 --- a/controllers/update/reconcile_test.go +++ b/controllers/update/reconcile_test.go @@ -24,7 +24,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -func Test_controller_osImageHasChanged(t *testing.T) { +func Test_controller_autoUpdateOS(t *testing.T) { ctx := context.Background() scheme := runtime.NewScheme() err := v2.AddToScheme(scheme) From a129aba21946694ae47f983c7355985dba16a715 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Tue, 7 May 2024 14:39:48 +0200 Subject: [PATCH 12/16] Make manifests. --- .../firewall.metal-stack.io_firewalldeployments.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml index af0ea84..3100ea4 100644 --- a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml +++ b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml @@ -59,6 +59,17 @@ spec: spec: description: Spec contains the firewall deployment specification. properties: + autoUpdate: + description: AutoUpdate defines the behavior for automatic updates. + properties: + machineImage: + description: |- + MachineImage auto updates the os image of the firewall within the maintenance time window + in case a newer version of the os is available. + type: boolean + required: + - machineImage + type: object replicas: description: |- Replicas is the amount of firewall replicas targeted to be running. @@ -258,6 +269,7 @@ spec: type: object type: object required: + - autoUpdate - template type: object status: From 92b9101cc556660c9eda8249ba5813bb82faa653 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Wed, 8 May 2024 15:47:00 +0200 Subject: [PATCH 13/16] Do not react on maintenance annotation. --- controllers/deployment/controller.go | 1 + integration/suite_test.go | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index a604540..6366b87 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -36,6 +36,7 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M &v2.FirewallDeployment{}, builder.WithPredicates( predicate.And( + predicate.Not(v2.AnnotationAddedPredicate(v2.MaintenanceAnnotation)), predicate.Not(v2.AnnotationRemovedPredicate(v2.MaintenanceAnnotation)), predicate.Or( predicate.GenerationChangedPredicate{}, // prevents reconcile on status sub resource update diff --git a/integration/suite_test.go b/integration/suite_test.go index 89389c0..608e49f 100644 --- a/integration/suite_test.go +++ b/integration/suite_test.go @@ -16,6 +16,7 @@ import ( "github.com/metal-stack/firewall-controller-manager/controllers/firewall" "github.com/metal-stack/firewall-controller-manager/controllers/monitor" "github.com/metal-stack/firewall-controller-manager/controllers/set" + "github.com/metal-stack/firewall-controller-manager/controllers/update" metalclient "github.com/metal-stack/metal-go/test/client" "github.com/metal-stack/metal-lib/pkg/tag" . "github.com/onsi/ginkgo/v2" @@ -158,6 +159,14 @@ var _ = BeforeSuite(func() { ) Expect(err).ToNot(HaveOccurred()) + err = update.SetupWithManager( + ctrl.Log.WithName("controllers").WithName("update"), + mgr.GetEventRecorderFor("update-controller"), + mgr, + cc, + ) + Expect(err).ToNot(HaveOccurred()) + err = deployment.SetupWebhookWithManager(ctrl.Log.WithName("defaulting-webhook"), mgr, cc) Expect(err).ToNot(HaveOccurred()) err = set.SetupWebhookWithManager(ctrl.Log.WithName("defaulting-webhook"), mgr, cc) From 9e219053438005cbea26fe74e42c202fb381a775 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Fri, 10 May 2024 08:47:40 +0200 Subject: [PATCH 14/16] Log healthcheck with debug level. --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index db9ccce..2c6bad6 100644 --- a/main.go +++ b/main.go @@ -42,7 +42,7 @@ const ( func healthCheckFunc(log *slog.Logger, seedClient controllerclient.Client, namespace string) func(req *http.Request) error { return func(req *http.Request) error { - log.Info("health check called") + log.Debug("health check called") fws := &v2.FirewallList{} err := seedClient.List(req.Context(), fws, controllerclient.InNamespace(namespace)) From 1552595f97f2149f508d2b64bf37eb225ce40787 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Mon, 27 May 2024 14:57:20 +0200 Subject: [PATCH 15/16] Simplify --- controllers/deployment/reconcile.go | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index ecc82ae..c427791 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -197,35 +197,20 @@ func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment] oldS = &latestSet.Spec.Template.Spec ) - ok := sizeHasChanged(newS, oldS) - if ok { + if newS.Size != oldS.Size { r.Log.Info("firewall size has changed", "size", newS.Size) - return ok + return true } - ok = osImageHasChanged(newS, oldS) - if ok { + if newS.Image != oldS.Image { r.Log.Info("firewall image has changed", "image", newS.Image) - return ok + return true } - ok = networksHaveChanged(newS, oldS) - if ok { + if !sets.NewString(oldS.Networks...).Equal(sets.NewString(newS.Networks...)) { r.Log.Info("firewall networks have changed", "networks", newS.Networks) - return ok + return true } return false } - -func sizeHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { - return newS.Size != oldS.Size -} - -func osImageHasChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { - return newS.Image != oldS.Image -} - -func networksHaveChanged(newS *v2.FirewallSpec, oldS *v2.FirewallSpec) bool { - return !sets.NewString(oldS.Networks...).Equal(sets.NewString(newS.Networks...)) -} From cab45bd9b1daeccfb56eeab459dc02d1aee90473 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Mon, 27 May 2024 15:08:17 +0200 Subject: [PATCH 16/16] Longer timeout. --- integration/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index b011771..daf0296 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -1707,7 +1707,7 @@ var _ = Context("integration test", Ordered, func() { It("should adopt the existing firewall", func() { fw = testcommon.WaitForResourceAmount(k8sClient, ctx, namespaceName, 1, &v2.FirewallList{}, func(l *v2.FirewallList) []*v2.Firewall { return l.GetItems() - }, 3*time.Second) + }, 10*time.Second) Expect(fw.Name).To(Equal("test")) Expect(fw.OwnerReferences).To(HaveLen(1))