From 6e7e1f67b723caaf636bc456ea7afbbae328aae6 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 11:34:58 +0200 Subject: [PATCH 01/12] Implement orchestrated firewall distance configuration. --- Makefile | 54 +++---- api/v2/defaults/defaults.go | 6 +- api/v2/types_firewall.go | 9 ++ api/v2/types_firewallmonitor.go | 11 +- api/v2/types_firewallset.go | 19 +++ api/v2/types_utils.go | 10 ++ api/v2/validation/firewall.go | 8 ++ api/v2/validation/firewall_test.go | 10 +- api/v2/validation/firewalldeployment_test.go | 5 +- api/v2/validation/firewallset_test.go | 7 +- ...ll.metal-stack.io_firewalldeployments.yaml | 2 +- ...ewall.metal-stack.io_firewallmonitors.yaml | 16 ++- .../firewall.metal-stack.io_firewalls.yaml | 11 +- .../firewall.metal-stack.io_firewallsets.yaml | 7 +- controllers/deployment/reconcile.go | 37 +++-- controllers/deployment/recreate.go | 2 +- controllers/deployment/rolling.go | 4 +- controllers/deployment/status.go | 12 +- controllers/firewall/reconcile.go | 11 +- controllers/firewall/status.go | 135 +++++++++++++----- controllers/monitor/reconcile.go | 29 +--- controllers/set/delete.go | 2 +- controllers/set/reconcile.go | 2 + controllers/set/status.go | 3 +- integration/integration_test.go | 63 ++++++-- 25 files changed, 312 insertions(+), 163 deletions(-) diff --git a/Makefile b/Makefile index db78a3a..a78e74d 100644 --- a/Makefile +++ b/Makefile @@ -8,17 +8,18 @@ GITVERSION := $(shell git describe --long --all) BUILDDATE := $(shell date -Iseconds) VERSION := $(or ${VERSION},$(shell git describe --tags --exact-match 2> /dev/null || git symbolic-ref -q --short HEAD || git rev-parse --short HEAD)) -# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set) -ifeq (,$(shell go env GOBIN)) -GOBIN=$(shell go env GOPATH)/bin -else -GOBIN=$(shell go env GOBIN) -endif +CONTROLLER_TOOLS_VERSION ?= v0.11.3 +LOCALBIN ?= $(shell pwd)/bin +CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen +ENVTEST ?= $(LOCALBIN)/setup-envtest include .env all: manager +$(LOCALBIN): + mkdir -p $(LOCALBIN) + # clean generated code .PHONY: clean clean: @@ -26,8 +27,7 @@ clean: # Run tests test: - @if ! which $(SETUP_ENVTEST) > /dev/null; then echo "setup-envtest needs to be installed. you can use setup-envtest target to achieve this."; exit 1; fi - KUBEBUILDER_ASSETS="$(shell $(SETUP_ENVTEST) use --arch=amd64 --bin-dir $(PWD)/bin -p path)" go test ./... -coverprofile cover.out + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test ./... -coverprofile cover.out # Build manager binary manager: generate fmt vet @@ -72,35 +72,13 @@ vet: generate: controller-gen $(CONTROLLER_GEN) object paths="./..." -# find or download controller-gen -# download controller-gen if necessary -controller-gen: -ifeq (, $(shell which controller-gen)) - @{ \ - set -e ;\ - CONTROLLER_GEN_TMP_DIR=$$(mktemp -d) ;\ - cd $$CONTROLLER_GEN_TMP_DIR ;\ - go mod init tmp ;\ - go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.10.0 ;\ - rm -rf $$CONTROLLER_GEN_TMP_DIR ;\ - } -CONTROLLER_GEN=$(GOBIN)/controller-gen -else -CONTROLLER_GEN=$(shell which controller-gen) -endif +.PHONY: controller-gen +controller-gen: $(CONTROLLER_GEN) +$(CONTROLLER_GEN): $(LOCALBIN) + test -s $(LOCALBIN)/controller-gen && $(LOCALBIN)/controller-gen --version | grep -q $(CONTROLLER_TOOLS_VERSION) || \ + GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_TOOLS_VERSION) .PHONY: setup-envtest -setup-envtest: -ifeq (, $(shell which setup-envtest)) - @{ \ - set -e ;\ - TMP_DIR=$$(mktemp -d) ;\ - cd $$TMP_DIR ;\ - go mod init tmp ;\ - go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest ;\ - rm -rf $$TMP_DIR ;\ - } -SETUP_ENVTEST=$(GOBIN)/setup-envtest -else -SETUP_ENVTEST=$(shell which setup-envtest) -endif +setup-envtest: $(ENVTEST) +$(ENVTEST): $(LOCALBIN) + test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest diff --git a/api/v2/defaults/defaults.go b/api/v2/defaults/defaults.go index 2a25814..cdfa3af 100644 --- a/api/v2/defaults/defaults.go +++ b/api/v2/defaults/defaults.go @@ -15,6 +15,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) +const ( + DefaultFirewallReconcileInterval = "10s" +) + type ( firewallDefaulter struct { c *config.ControllerConfig @@ -163,7 +167,7 @@ func (r *firewallDeploymentDefaulter) Default(ctx context.Context, obj runtime.O func defaultFirewallSpec(f *v2.FirewallSpec) { if f.Interval == "" { - f.Interval = "10s" + f.Interval = DefaultFirewallReconcileInterval } } diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index bee2476..4514f04 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -47,6 +47,10 @@ type Firewall struct { Spec FirewallSpec `json:"spec"` // Status contains current status information on the firewall. Status FirewallStatus `json:"status,omitempty"` + + // Distance defines the as-path length of a firewall. + // This field is typically orchestrated by the deployment controller. + Distance FirewallDistance `json:"distance"` } // FirewallSpec defines parameters for the firewall creation along with configuration for the firewall-controller. @@ -100,6 +104,7 @@ type FirewallSpec struct { // LogAcceptedConnections if set to true, also log accepted connections in the droptailer log. LogAcceptedConnections bool `json:"logAcceptedConnections,omitempty"` + // DNSServerAddress specifies DNS server address used by DNS proxy DNSServerAddress string `json:"dnsServerAddress,omitempty"` // DNSPort specifies port to which DNS proxy should be bound @@ -172,6 +177,8 @@ const ( FirewallControllerConnected ConditionType = "Connected" // FirewallMonitorDeployed indicates that the firewall monitor is deployed into the shoot cluster FirewallMonitorDeployed ConditionType = "MonitorDeployed" + // FirewallDistanceConfigured indicates that the firewall-controller has configured the given firewall distance. + FirewallDistanceConfigured ConditionType = "Distance" ) // ShootAccess contains secret references to construct a shoot client in the firewall-controller to update its firewall monitor. @@ -222,6 +229,8 @@ type ControllerConnection struct { ActualVersion string `json:"actualVersion,omitempty"` // Updated is a timestamp when the controller has last reconciled the firewall resource. Updated metav1.Time `json:"lastRun,omitempty"` + // ActualDistance is the actual distance as reflected by the firewall-controller. + ActualDistance FirewallDistance `json:"actualDistance,omitempty"` } // FirewallNetwork holds refined information about a network that the firewall is connected to. diff --git a/api/v2/types_firewallmonitor.go b/api/v2/types_firewallmonitor.go index e96e129..56eb138 100644 --- a/api/v2/types_firewallmonitor.go +++ b/api/v2/types_firewallmonitor.go @@ -53,11 +53,12 @@ type FirewallMonitor struct { } type ControllerStatus struct { - Message string `json:"message,omitempty"` - FirewallStats *FirewallStats `json:"stats,omitempty"` - ControllerVersion string `json:"controllerVersion,omitempty"` - NftablesExporterVersion string `json:"nftablesExporterVersion,omitempty"` - Updated metav1.Time `json:"lastRun,omitempty"` + Message string `json:"message,omitempty"` + FirewallStats *FirewallStats `json:"stats,omitempty"` + ControllerVersion string `json:"controllerVersion,omitempty"` + NftablesExporterVersion string `json:"nftablesExporterVersion,omitempty"` + Updated metav1.Time `json:"lastRun,omitempty"` + Distance FirewallDistance `json:"distance,omitempty"` } // FirewallStats contains firewall statistics diff --git a/api/v2/types_firewallset.go b/api/v2/types_firewallset.go index db3a92b..f1e9d93 100644 --- a/api/v2/types_firewallset.go +++ b/api/v2/types_firewallset.go @@ -12,6 +12,9 @@ import ( const ( // FirewallControllerSetAnnotation is a tag added to the firewall entity indicating to which set a firewall belongs to. FirewallControllerSetAnnotation = "firewall.metal.stack.io/set" + + FirewallShortestDistance = FirewallDistance(0) + FirewallRollingUpdateSetDistance = FirewallDistance(2) ) func FirewallSetTag(setName string) string { @@ -53,8 +56,24 @@ type FirewallSetSpec struct { Selector map[string]string `json:"selector,omitempty"` // Template is the firewall spec used for creating the firewalls. Template FirewallTemplateSpec `json:"template"` + // Distance defines the as-path length of the firewalls. + // This field is typically orchestrated by the deployment controller. + Distance FirewallDistance `json:"distance"` } +// FirewallDistance defines the as-path length of firewalls, influencing how strong they attract +// network traffic for routing traffic in and out of the cluster. +// This is of particular interest during rolling firewall updates, i.e. when there is +// more than a single firewall running in front of the cluster. +// During a rolling update, new firewalls start with a longer distance such that +// traffic is only attracted by the existing firewalls ("firewall staging"). +// When the new firewall has connected successfully to the firewall monitor, the deployment +// controller throws away the old firewalls and the new firewall takes over the routing. +// The deployment controller will then shorten the distance of the new firewall. +// This approach reduces service interruption of the external user traffic of the cluster +// (for firewall-controller versions that support this feature). +type FirewallDistance int + type FirewallSetStatus struct { // TargetReplicas is the amount of firewall replicas targeted to be running. TargetReplicas int `json:"targetReplicas"` diff --git a/api/v2/types_utils.go b/api/v2/types_utils.go index 52f109b..957a380 100644 --- a/api/v2/types_utils.go +++ b/api/v2/types_utils.go @@ -1,8 +1,11 @@ package v2 import ( + "strconv" + "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -134,3 +137,10 @@ func annotationWasRemoved(update event.UpdateEvent, annotation string) bool { return o && !n } + +// IsAnnotationTrue returns true if the given object has an annotation with a given +// key and the value of this annotation is a true boolean. +func IsAnnotationTrue(o client.Object, key string) bool { + enabled, err := strconv.ParseBool(o.GetAnnotations()[key]) + return err == nil && enabled +} diff --git a/api/v2/validation/firewall.go b/api/v2/validation/firewall.go index 00dd6e0..1f783e8 100644 --- a/api/v2/validation/firewall.go +++ b/api/v2/validation/firewall.go @@ -13,6 +13,10 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) +const ( + FirewallMaxDistance = 8 +) + type firewallValidator struct{} func NewFirewallValidator(log logr.Logger) *genericValidator[*v2.Firewall, *firewallValidator] { @@ -22,6 +26,10 @@ func NewFirewallValidator(log logr.Logger) *genericValidator[*v2.Firewall, *fire func (v *firewallValidator) ValidateCreate(log logr.Logger, f *v2.Firewall) field.ErrorList { var allErrs field.ErrorList + if f.Distance < 0 || f.Distance > FirewallMaxDistance { + allErrs = append(allErrs, field.Invalid(field.NewPath("distance"), f.Distance, fmt.Sprintf("distance must be between 0 and %d", f.Distance))) + } + allErrs = append(allErrs, validateFirewallAnnotations(f)...) allErrs = append(allErrs, v.validateSpec(&f.Spec, field.NewPath("spec"))...) diff --git a/api/v2/validation/firewall_test.go b/api/v2/validation/firewall_test.go index 75833c4..22ce425 100644 --- a/api/v2/validation/firewall_test.go +++ b/api/v2/validation/firewall_test.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr/testr" "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/defaults" "github.com/metal-stack/metal-lib/pkg/testcommon" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -23,7 +24,7 @@ func Test_firewallValidator_ValidateCreate(t *testing.T) { }, }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", @@ -46,6 +47,7 @@ func Test_firewallValidator_ValidateCreate(t *testing.T) { }, }, }, + Distance: 0, } tests := []struct { @@ -117,7 +119,7 @@ func Test_firewallValidator_ValidateUpdate(t *testing.T) { }, }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", @@ -140,6 +142,7 @@ func Test_firewallValidator_ValidateUpdate(t *testing.T) { }, }, }, + Distance: 0, }, oldF: &v2.Firewall{ ObjectMeta: metav1.ObjectMeta{ @@ -147,7 +150,7 @@ func Test_firewallValidator_ValidateUpdate(t *testing.T) { Namespace: "default", }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", @@ -170,6 +173,7 @@ func Test_firewallValidator_ValidateUpdate(t *testing.T) { }, }, }, + Distance: 0, }, wantErr: nil, }, diff --git a/api/v2/validation/firewalldeployment_test.go b/api/v2/validation/firewalldeployment_test.go index 8e6045e..785219e 100644 --- a/api/v2/validation/firewalldeployment_test.go +++ b/api/v2/validation/firewalldeployment_test.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr/testr" "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/defaults" "github.com/metal-stack/metal-lib/pkg/testcommon" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,7 +31,7 @@ func Test_firewallDeploymentValidator_ValidateCreate(t *testing.T) { }, }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", @@ -116,7 +117,7 @@ func Test_firewallDeploymentValidator_ValidateUpdate(t *testing.T) { }, }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", diff --git a/api/v2/validation/firewallset_test.go b/api/v2/validation/firewallset_test.go index fb2a8ae..fccf002 100644 --- a/api/v2/validation/firewallset_test.go +++ b/api/v2/validation/firewallset_test.go @@ -7,6 +7,7 @@ import ( "github.com/go-logr/logr/testr" "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/defaults" "github.com/metal-stack/metal-lib/pkg/testcommon" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,6 +23,7 @@ func Test_firewalSetValidator_ValidateCreate(t *testing.T) { Selector: map[string]string{ "purpose": "shoot-firewall", }, + Distance: 0, Template: v2.FirewallTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -29,7 +31,7 @@ func Test_firewalSetValidator_ValidateCreate(t *testing.T) { }, }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", @@ -133,6 +135,7 @@ func Test_firewallSetValidator_ValidateUpdate(t *testing.T) { "purpose": "shoot-firewall", "a": "b", }, + Distance: 0, Template: v2.FirewallTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -141,7 +144,7 @@ func Test_firewallSetValidator_ValidateUpdate(t *testing.T) { }, }, Spec: v2.FirewallSpec{ - Interval: "10s", + Interval: defaults.DefaultFirewallReconcileInterval, ControllerURL: "https://metal-stack.io/controller.img", ControllerVersion: "v", NftablesExporterURL: "http://exporter.tar.gz", diff --git a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml index 47a1430..4a7c38b 100644 --- a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml +++ b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.10.0 + controller-gen.kubebuilder.io/version: v0.11.3 creationTimestamp: null name: firewalldeployments.firewall.metal-stack.io spec: diff --git a/config/crds/firewall.metal-stack.io_firewallmonitors.yaml b/config/crds/firewall.metal-stack.io_firewallmonitors.yaml index 9742e68..e6b82af 100644 --- a/config/crds/firewall.metal-stack.io_firewallmonitors.yaml +++ b/config/crds/firewall.metal-stack.io_firewallmonitors.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.10.0 + controller-gen.kubebuilder.io/version: v0.11.3 creationTimestamp: null name: firewallmonitors.firewall.metal-stack.io spec: @@ -88,6 +88,20 @@ spec: properties: controllerVersion: type: string + distance: + description: FirewallDistance defines the as-path length of firewalls, + influencing how strong they attract network traffic for routing + traffic in and out of the cluster. This is of particular interest + during rolling firewall updates, i.e. when there is more than a + single firewall running in front of the cluster. During a rolling + update, new firewalls will start with a longer distance such that + traffic is only attracted by the existing firewalls ("firewall staging"). + When the new firewall has connected successfully to the firewall + monitor, the deployment controller throws away the old firewalls + and the new will firewall takes over the routing. The deployment + controller will then shorten the distance of the new firewall. This + approach reduces downtime of the effective user traffic of the cluster. + type: integer lastRun: format: date-time type: string diff --git a/config/crds/firewall.metal-stack.io_firewalls.yaml b/config/crds/firewall.metal-stack.io_firewalls.yaml index 4db6f50..3e91354 100644 --- a/config/crds/firewall.metal-stack.io_firewalls.yaml +++ b/config/crds/firewall.metal-stack.io_firewalls.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.10.0 + controller-gen.kubebuilder.io/version: v0.11.3 creationTimestamp: null name: firewalls.firewall.metal-stack.io spec: @@ -49,6 +49,10 @@ spec: of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' type: string + distance: + description: Distance defines the as-path length of a firewall. This field + is typically orchestrated by the deployment controller. + type: integer kind: description: 'Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client @@ -238,6 +242,10 @@ spec: inside the firewall resource. This will be updated by the firewall monitor controller. properties: + actualDistance: + description: ActualDistance is the actual distance as reflected + by the firewall-controller. + type: integer actualVersion: description: ActualVersion is the actual version running at the firewall-controller. @@ -389,6 +397,7 @@ spec: - phase type: object required: + - distance - spec type: object served: true diff --git a/config/crds/firewall.metal-stack.io_firewallsets.yaml b/config/crds/firewall.metal-stack.io_firewallsets.yaml index 1be2004..de307d0 100644 --- a/config/crds/firewall.metal-stack.io_firewallsets.yaml +++ b/config/crds/firewall.metal-stack.io_firewallsets.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.10.0 + controller-gen.kubebuilder.io/version: v0.11.3 creationTimestamp: null name: firewallsets.firewall.metal-stack.io spec: @@ -55,6 +55,10 @@ spec: spec: description: Spec contains the firewall set specification. properties: + distance: + description: Distance defines the as-path length of the firewalls. + This field is typically orchestrated by the deployment controller. + type: integer replicas: description: Replicas is the amount of firewall replicas targeted to be running. @@ -235,6 +239,7 @@ spec: type: object type: object required: + - distance - replicas - template type: object diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 058e46e..ed0b533 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -37,7 +37,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error if current == nil { r.Log.Info("no firewall set is present, creating a new one") - _, err := c.createFirewallSet(r, 0) + _, err := c.createFirewallSet(r, v2.FirewallShortestDistance, 0) if err != nil { return err } @@ -63,19 +63,31 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error return err } + // we are done with the update, give the set the shortest distance if not already set + if current.Status.ReadyReplicas == current.Spec.Replicas && current.Spec.Distance != v2.FirewallShortestDistance { + current.Spec.Distance = v2.FirewallShortestDistance + + err := c.c.GetSeedClient().Update(r.Ctx, current) + if err != nil { + return fmt.Errorf("unable to swap latest set distance to 0: %w", err) + } + + r.Log.Info("swapped latest set to distance to 0") + } + return nil } -func (c *controller) createNextFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], current *v2.FirewallSet) (*v2.FirewallSet, error) { +func (c *controller) createNextFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], current *v2.FirewallSet, distance v2.FirewallDistance) (*v2.FirewallSet, error) { revision, err := controllers.NextRevision(current) if err != nil { return nil, err } - return c.createFirewallSet(r, revision) + return c.createFirewallSet(r, distance, revision) } -func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], revision int) (*v2.FirewallSet, error) { +func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], distance v2.FirewallDistance, revision int) (*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()) @@ -104,6 +116,7 @@ func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment Spec: v2.FirewallSetSpec{ Replicas: r.Target.Spec.Replicas, Template: r.Target.Spec.Template, + Distance: distance, }, } @@ -144,22 +157,18 @@ func (c *controller) syncFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], return nil } -func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment], lastSet *v2.FirewallSet) (bool, error) { - v, ok := lastSet.Annotations[v2.RollSetAnnotation] - if ok { - rollSet, err := strconv.ParseBool(v) - if err == nil && rollSet { - r.Log.Info("set roll initiated by annotation") - return true, nil - } +func (c *controller) isNewSetRequired(r *controllers.Ctx[*v2.FirewallDeployment], latestSet *v2.FirewallSet) (bool, error) { + if v2.IsAnnotationTrue(latestSet, v2.RollSetAnnotation) { + r.Log.Info("set roll initiated by annotation") + return true, nil } var ( newS = &r.Target.Spec.Template.Spec - oldS = &lastSet.Spec.Template.Spec + oldS = &latestSet.Spec.Template.Spec ) - ok = sizeHasChanged(newS, oldS) + ok := sizeHasChanged(newS, oldS) if ok { r.Log.Info("firewall size has changed", "size", newS.Size) return ok, nil diff --git a/controllers/deployment/recreate.go b/controllers/deployment/recreate.go index 921fe64..c03c743 100644 --- a/controllers/deployment/recreate.go +++ b/controllers/deployment/recreate.go @@ -22,7 +22,7 @@ func (c *controller) recreateStrategy(r *controllers.Ctx[*v2.FirewallDeployment] return err } - newSet, err := c.createNextFirewallSet(r, current) + newSet, err := c.createNextFirewallSet(r, current, v2.FirewallShortestDistance) if err != nil { return err } diff --git a/controllers/deployment/rolling.go b/controllers/deployment/rolling.go index dcdcf23..49174c0 100644 --- a/controllers/deployment/rolling.go +++ b/controllers/deployment/rolling.go @@ -16,9 +16,9 @@ func (c *controller) rollingUpdateStrategy(r *controllers.Ctx[*v2.FirewallDeploy } if newSetRequired { - r.Log.Info("significant changes detected in the spec, creating new firewall set") + r.Log.Info("significant changes detected in the spec, creating new firewall set", "distance", v2.FirewallRollingUpdateSetDistance) - newSet, err := c.createNextFirewallSet(r, current) + newSet, err := c.createNextFirewallSet(r, current, v2.FirewallRollingUpdateSetDistance) if err != nil { return err } diff --git a/controllers/deployment/status.go b/controllers/deployment/status.go index 57d2ccf..02b78eb 100644 --- a/controllers/deployment/status.go +++ b/controllers/deployment/status.go @@ -15,22 +15,22 @@ func (c *controller) setStatus(r *controllers.Ctx[*v2.FirewallDeployment]) error return fmt.Errorf("unable to get owned sets: %w", err) } - lastSet, err := controllers.MaxRevisionOf(ownedSets) + latestSet, err := controllers.MaxRevisionOf(ownedSets) if err != nil { return err } r.Target.Status.TargetReplicas = r.Target.Spec.Replicas - if lastSet != nil { - revision, err := controllers.Revision(lastSet) + if latestSet != nil { + revision, err := controllers.Revision(latestSet) if err != nil { return err } r.Target.Status.ObservedRevision = revision - r.Target.Status.ProgressingReplicas = lastSet.Status.ProgressingReplicas - r.Target.Status.UnhealthyReplicas = lastSet.Status.UnhealthyReplicas - r.Target.Status.ReadyReplicas = lastSet.Status.ReadyReplicas + r.Target.Status.ProgressingReplicas = latestSet.Status.ProgressingReplicas + r.Target.Status.UnhealthyReplicas = latestSet.Status.UnhealthyReplicas + r.Target.Status.ReadyReplicas = latestSet.Status.ReadyReplicas } if r.Target.Status.ReadyReplicas >= r.Target.Spec.Replicas { diff --git a/controllers/firewall/reconcile.go b/controllers/firewall/reconcile.go index e3eba97..a0da458 100644 --- a/controllers/firewall/reconcile.go +++ b/controllers/firewall/reconcile.go @@ -15,22 +15,17 @@ import ( "k8s.io/apimachinery/pkg/util/sets" ) -// Reconciler must always return either a error or requeue to ensure that it detects if a firewall get lost etc. +// Reconciler must always return either an error or requeue to ensure that it detects if a firewall get lost etc. func (c *controller) Reconcile(r *controllers.Ctx[*v2.Firewall]) error { var f *models.V1FirewallResponse defer func() { - _, err := c.ensureFirewallMonitor(r) + 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 f == nil { - r.Log.Error(fmt.Errorf("not owning a firewall"), "controller is not owning a firewall") - return - } - - if err := c.setStatus(r, f); err != nil { + if err := c.setStatus(r, f, mon); err != nil { r.Log.Error(err, "unable to set firewall status") } }() diff --git a/controllers/firewall/status.go b/controllers/firewall/status.go index f6df579..8158640 100644 --- a/controllers/firewall/status.go +++ b/controllers/firewall/status.go @@ -1,65 +1,66 @@ package firewall import ( - "context" "errors" "fmt" - "strconv" "time" 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/cache" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func (c *controller) setStatus(r *controllers.Ctx[*v2.Firewall], m *models.V1FirewallResponse) error { +func (c *controller) setStatus(r *controllers.Ctx[*v2.Firewall], f *models.V1FirewallResponse, mon *v2.FirewallMonitor) error { var errs []error - machineStatus, err := getMachineStatus(m) - if err == nil { - r.Target.Status.MachineStatus = machineStatus - } else { + err := setMachineStatus(r.Target, f) + if err != nil { errs = append(errs, err) } - firewallNetworks, err := getFirewallNetworks(r.Ctx, c.networkCache, m) - if err == nil { - r.Target.Status.FirewallNetworks = firewallNetworks - } else { + err = c.setFirewallNetworks(r, f) + if err != nil { errs = append(errs, err) } - if enabled, err := strconv.ParseBool(r.Target.Annotations[v2.FirewallNoControllerConnectionAnnotation]); err == nil && enabled { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionTrue, "NotChecking", "Not checking controller connection due to firewall annotation.") - r.Target.Status.Conditions.Set(cond) - } else if r.Target.Status.ControllerStatus == nil { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "NotConnected", "Controller has not yet connected.") - r.Target.Status.Conditions.Set(cond) - } + SetFirewallStatus(r.Target, mon) r.Target.Status.ShootAccess = c.c.GetShootAccess() return errors.Join(errs...) } -func getMachineStatus(m *models.V1FirewallResponse) (*v2.MachineStatus, error) { - result := &v2.MachineStatus{} +func setMachineStatus(fw *v2.Firewall, f *models.V1FirewallResponse) error { + if f == nil { + return nil + } - if m.ID == nil || m.Allocation == nil || m.Allocation.Created == nil || m.Liveliness == nil { - return nil, fmt.Errorf("firewall entity is missing essential fields") + result, err := getMachineStatus(f) + if err != nil { + return err } - result.MachineID = *m.ID - result.AllocationTimestamp = metav1.NewTime(time.Time(*m.Allocation.Created)) - result.Liveliness = *m.Liveliness + fw.Status.MachineStatus = result + + return nil +} - if m.Events != nil && m.Events.CrashLoop != nil { - result.CrashLoop = *m.Events.CrashLoop +func getMachineStatus(f *models.V1FirewallResponse) (*v2.MachineStatus, error) { + if f.ID == nil || f.Allocation == nil || f.Allocation.Created == nil || f.Liveliness == nil { + return nil, fmt.Errorf("firewall entity from metal-api is missing essential fields") } - if m.Events != nil && len(m.Events.Log) > 0 && m.Events.Log[0].Event != nil { - log := m.Events.Log[0] + + result := &v2.MachineStatus{} + result.MachineID = *f.ID + result.AllocationTimestamp = metav1.NewTime(time.Time(*f.Allocation.Created)) + result.Liveliness = *f.Liveliness + + if f.Events != nil && f.Events.CrashLoop != nil { + result.CrashLoop = *f.Events.CrashLoop + } + if f.Events != nil && len(f.Events.Log) > 0 && f.Events.Log[0].Event != nil { + log := f.Events.Log[0] result.LastEvent = &v2.MachineLastEvent{ Event: *log.Event, @@ -71,7 +72,7 @@ func getMachineStatus(m *models.V1FirewallResponse) (*v2.MachineStatus, error) { return result, nil } -func getFirewallNetworks(ctx context.Context, cache *cache.Cache[string, *models.V1NetworkResponse], m *models.V1FirewallResponse) ([]v2.FirewallNetwork, error) { +func (c *controller) setFirewallNetworks(r *controllers.Ctx[*v2.Firewall], f *models.V1FirewallResponse) error { // check whether network prefixes were updated in metal-api // prefixes in the firewall machine allocation are just a snapshot when the firewall was created. // -> when changing prefixes in the referenced network the firewall does not know about any prefix changes. @@ -79,20 +80,25 @@ func getFirewallNetworks(ctx context.Context, cache *cache.Cache[string, *models // we replace the prefixes from the snapshot with the actual prefixes that are currently attached to the network. // this allows dynamic prefix reconfiguration of the firewall. - if m.Allocation == nil { - return nil, fmt.Errorf("firewall entity is missing essential fields") + if f == nil { + return nil + } + + if f.Allocation == nil { + return fmt.Errorf("firewall entity is missing essential fields") } var result []v2.FirewallNetwork - for _, n := range m.Allocation.Networks { + + for _, n := range f.Allocation.Networks { n := n if n.Networkid == nil { continue } - nw, err := cache.Get(ctx, *n.Networkid) + nw, err := c.networkCache.Get(r.Ctx, *n.Networkid) if err != nil { - return nil, err + return err } result = append(result, v2.FirewallNetwork{ @@ -107,5 +113,60 @@ func getFirewallNetworks(ctx context.Context, cache *cache.Cache[string, *models }) } - return result, nil + r.Target.Status.FirewallNetworks = result + + return nil +} + +func SetFirewallStatus(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) + + cond = v2.NewCondition(v2.FirewallDistanceConfigured, v2.ConditionTrue, "NotChecking", "Not checking distance due to firewall annotation.") + fw.Status.Conditions.Set(cond) + + return + } + + if mon == nil { + return + } + + if mon.ControllerStatus == nil { + cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "NotConnected", "Controller has not yet connected.") + fw.Status.Conditions.Set(cond) + + cond = v2.NewCondition(v2.FirewallDistanceConfigured, v2.ConditionFalse, "NotConnected", "Controller has not yet connected.") + fw.Status.Conditions.Set(cond) + + return + } + + connection := &v2.ControllerConnection{ + ActualVersion: mon.ControllerStatus.ControllerVersion, + Updated: mon.ControllerStatus.Updated, + ActualDistance: mon.ControllerStatus.Distance, + } + + fw.Status.ControllerStatus = connection + + if connection.Updated.Time.IsZero() { + cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "NotConnected", "Controller has not yet connected.") + fw.Status.Conditions.Set(cond) + } else if time.Since(connection.Updated.Time) > 5*time.Minute { + cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "StoppedReconciling", fmt.Sprintf("Controller has stopped reconciling since %s.", connection.Updated.Time.String())) + fw.Status.Conditions.Set(cond) + } else { + cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionTrue, "Connected", fmt.Sprintf("Controller reconciled firewall at %s.", connection.Updated.Time.String())) + fw.Status.Conditions.Set(cond) + } + + if fw.Distance == connection.ActualDistance { + cond := v2.NewCondition(v2.FirewallDistanceConfigured, v2.ConditionTrue, "Configured", fmt.Sprintf("Controller has configured the specified distance %d.", fw.Distance)) + fw.Status.Conditions.Set(cond) + } else { + cond := v2.NewCondition(v2.FirewallDistanceConfigured, v2.ConditionFalse, "NotConfigured", fmt.Sprintf("Controller has configured distance %d, but %d is specified.", connection.ActualDistance, fw.Distance)) + fw.Status.Conditions.Set(cond) + } } diff --git a/controllers/monitor/reconcile.go b/controllers/monitor/reconcile.go index d162860..2a81f63 100644 --- a/controllers/monitor/reconcile.go +++ b/controllers/monitor/reconcile.go @@ -9,6 +9,7 @@ import ( v2 "github.com/metal-stack/firewall-controller-manager/api/v2" "github.com/metal-stack/firewall-controller-manager/api/v2/helper" "github.com/metal-stack/firewall-controller-manager/controllers" + "github.com/metal-stack/firewall-controller-manager/controllers/firewall" "github.com/metal-stack/metal-lib/pkg/pointer" corev1 "k8s.io/api/core/v1" @@ -52,33 +53,7 @@ func (c *controller) updateFirewallStatus(r *controllers.Ctx[*v2.FirewallMonitor return nil, fmt.Errorf("associated firewall of monitor not found: %w", err) } - if enabled, err := strconv.ParseBool(fw.Annotations[v2.FirewallNoControllerConnectionAnnotation]); err == nil && enabled { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionTrue, "NotChecking", "Not checking controller connection due to firewall annotation.") - fw.Status.Conditions.Set(cond) - } else { - if r.Target.ControllerStatus == nil { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "NotConnected", "Controller has not yet connected.") - fw.Status.Conditions.Set(cond) - } else { - connection := &v2.ControllerConnection{ - ActualVersion: r.Target.ControllerStatus.ControllerVersion, - Updated: r.Target.ControllerStatus.Updated, - } - - fw.Status.ControllerStatus = connection - - if connection.Updated.Time.IsZero() { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "NotConnected", "Controller has not yet connected.") - fw.Status.Conditions.Set(cond) - } else if time.Since(connection.Updated.Time) > 5*time.Minute { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionFalse, "StoppedReconciling", fmt.Sprintf("Controller has stopped reconciling since %s.", connection.Updated.Time.String())) - fw.Status.Conditions.Set(cond) - } else { - cond := v2.NewCondition(v2.FirewallControllerConnected, v2.ConditionTrue, "Connected", fmt.Sprintf("Controller reconciled firewall at %s.", connection.Updated.Time.String())) - fw.Status.Conditions.Set(cond) - } - } - } + firewall.SetFirewallStatus(fw, r.Target) err = c.c.GetSeedClient().Status().Update(r.Ctx, fw) if err != nil { diff --git a/controllers/set/delete.go b/controllers/set/delete.go index cb21f9f..b6281fe 100644 --- a/controllers/set/delete.go +++ b/controllers/set/delete.go @@ -78,7 +78,7 @@ func (c *controller) deleteAfterTimeout(r *controllers.Ctx[*v2.FirewallSet], fws } // deletePhysicalOrphans checks in the backend if there are firewall entities that belong to the controller -// event though there is no corresponding firewall resource managed by this controller. +// even though there is no corresponding firewall resource managed by this controller. // // such firewalls will be deleted in the backend. func (c *controller) deletePhysicalOrphans(r *controllers.Ctx[*v2.FirewallSet]) error { diff --git a/controllers/set/reconcile.go b/controllers/set/reconcile.go index 94b51b8..54d99e7 100644 --- a/controllers/set/reconcile.go +++ b/controllers/set/reconcile.go @@ -28,6 +28,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error { for _, fw := range ownedFirewalls { fw.Spec = r.Target.Spec.Template.Spec + fw.Distance = r.Target.Spec.Distance err := c.c.GetSeedClient().Update(r.Ctx, fw, &client.UpdateOptions{}) if err != nil { @@ -122,6 +123,7 @@ func (c *controller) createFirewall(r *controllers.Ctx[*v2.FirewallSet]) (*v2.Fi fw := &v2.Firewall{ ObjectMeta: *meta, Spec: r.Target.Spec.Template.Spec, + Distance: r.Target.Spec.Distance, } err = c.c.GetSeedClient().Create(r.Ctx, fw, &client.CreateOptions{}) diff --git a/controllers/set/status.go b/controllers/set/status.go index faa87b1..5efdf53 100644 --- a/controllers/set/status.go +++ b/controllers/set/status.go @@ -22,9 +22,10 @@ func (c *controller) setStatus(r *controllers.Ctx[*v2.FirewallSet], ownedFirewal created = pointer.SafeDeref(fw.Status.Conditions.Get(v2.FirewallCreated)).Status == v2.ConditionTrue ready = pointer.SafeDeref(fw.Status.Conditions.Get(v2.FirewallReady)).Status == v2.ConditionTrue connected = pointer.SafeDeref(fw.Status.Conditions.Get(v2.FirewallControllerConnected)).Status == v2.ConditionTrue + distance = pointer.SafeDeref(fw.Status.Conditions.Get(v2.FirewallDistanceConfigured)).Status == v2.ConditionTrue ) - if created && ready && connected { + if created && ready && connected && distance { r.Target.Status.ReadyReplicas++ continue } diff --git a/integration/integration_test.go b/integration/integration_test.go index 2cee2e5..1eebf03 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -5,6 +5,7 @@ import ( "time" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" + "github.com/metal-stack/firewall-controller-manager/api/v2/defaults" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/stretchr/testify/mock" @@ -140,6 +141,7 @@ var _ = Context("integration test", Ordered, func() { ControllerVersion: "v2.0.0", NftablesExporterURL: "http://exporter.tar.gz", NftablesExporterVersion: "v1.0.0", + Interval: defaults.DefaultFirewallReconcileInterval, }, }, }, @@ -254,7 +256,8 @@ var _ = Context("integration test", Ordered, func() { It("should allow an update of the firewall monitor", func() { // simulating a firewall-controller updating the resource mon.ControllerStatus = &v2.ControllerStatus{ - Updated: metav1.NewTime(time.Now()), + Updated: metav1.NewTime(time.Now()), + Distance: v2.FirewallShortestDistance, } Expect(k8sClient.Update(ctx, mon)).To(Succeed()) }) @@ -271,7 +274,6 @@ var _ = Context("integration test", Ordered, func() { It("should inherit the spec from the set", func() { wantSpec := set.Spec.Template.Spec.DeepCopy() - wantSpec.Interval = "10s" Expect(&fw.Spec).To(BeComparableTo(wantSpec)) }) @@ -340,6 +342,17 @@ var _ = Context("integration test", Ordered, func() { Expect(cond.Reason).To(Equal("Connected")) Expect(cond.Message).To(Equal(fmt.Sprintf("Controller reconciled firewall at %s.", mon.ControllerStatus.Updated.Time.String()))) }) + + It("should have configured the distance", func() { + cond := testcommon.WaitForCondition(k8sClient, ctx, fw.DeepCopy(), func(fd *v2.Firewall) v2.Conditions { + return fd.Status.Conditions + }, v2.FirewallDistanceConfigured, v2.ConditionTrue, 15*time.Second) + + Expect(cond.LastTransitionTime).NotTo(BeZero()) + Expect(cond.LastUpdateTime).NotTo(BeZero()) + Expect(cond.Reason).To(Equal("Configured")) + Expect(cond.Message).To(Equal(fmt.Sprintf("Controller has configured the specified distance %d.", v2.FirewallShortestDistance))) + }) }) Context("the firewall set resource", func() { @@ -357,7 +370,6 @@ var _ = Context("integration test", Ordered, func() { It("should inherit the spec from the deployement", func() { wantSpec := deployment.Spec.Template.Spec.DeepCopy() - wantSpec.Interval = "10s" Expect(&set.Spec.Template.Spec).To(BeComparableTo(wantSpec)) }) @@ -502,7 +514,7 @@ var _ = Context("integration test", Ordered, func() { }) }) - Context("the firewall resource", func() { + Context("the new firewall resource", func() { It("should be named after the namespace (it's the shoot name in the end)", func() { Expect(fw.Name).To(HavePrefix(namespaceName + "-firewall-")) }) @@ -513,7 +525,6 @@ var _ = Context("integration test", Ordered, func() { It("should inherit the spec from the set", func() { wantSpec := set.Spec.Template.Spec.DeepCopy() - wantSpec.Interval = "10s" Expect(&fw.Spec).To(BeComparableTo(wantSpec)) }) @@ -561,6 +572,17 @@ var _ = Context("integration test", Ordered, func() { Expect(cond.Message).To(Equal(fmt.Sprintf("Firewall %q is not ready.", *installingFirewall.Allocation.Name))) }) + It("should not yet have a distance configured", func() { + cond := testcommon.WaitForCondition(k8sClient, ctx, fw.DeepCopy(), func(fd *v2.Firewall) v2.Conditions { + return fd.Status.Conditions + }, v2.FirewallDistanceConfigured, v2.ConditionFalse, 15*time.Second) + + Expect(cond.LastTransitionTime).NotTo(BeZero()) + Expect(cond.LastUpdateTime).NotTo(BeZero()) + Expect(cond.Reason).To(Equal("NotConnected")) + Expect(cond.Message).To(Equal("Controller has not yet connected.")) + }) + It("should have the monitor condition true", func() { cond := testcommon.WaitForCondition(k8sClient, ctx, fw.DeepCopy(), func(fd *v2.Firewall) v2.Conditions { return fd.Status.Conditions @@ -622,7 +644,7 @@ var _ = Context("integration test", Ordered, func() { }) }) - Context("the firewall set resource", func() { + Context("the new firewall set resource", func() { It("should be named after the deployment", func() { Expect(set.Name).To(HavePrefix(deployment.Name + "-")) }) @@ -635,13 +657,16 @@ var _ = Context("integration test", Ordered, func() { Expect(set.Spec.Replicas).To(Equal(1)) }) - It("should inherit the spec from the deployement", func() { + It("should inherit the spec from the deployment", func() { wantSpec := deployment.Spec.Template.Spec.DeepCopy() - wantSpec.Size = "n2-medium-x86" - wantSpec.Interval = "10s" + wantSpec.Size = "n2-medium-x86" // this is the change that triggered the rolling update Expect(&set.Spec.Template.Spec).To(BeComparableTo(wantSpec)) }) + It("should start with a higher distance", func() { + Expect(set.Spec.Distance).To(Equal(v2.FirewallRollingUpdateSetDistance)) + }) + It("should have the deployment as an owner", func() { Expect(set.ObjectMeta.OwnerReferences).To(HaveLen(1)) Expect(set.ObjectMeta.OwnerReferences[0].Name).To(Equal(deployment.Name)) @@ -722,7 +747,7 @@ var _ = Context("integration test", Ordered, func() { readyFirewall = firewall2("Phoned Home", "is phoning home") ) - When("the firewall gets ready and the firewall-controller connects", func() { + When("the firewall gets ready and the firewall-controller connects", Ordered, func() { It("should allow an update of the firewall monitor", func() { swapMetalClient(&metalclient.MetalMockFns{ Machine: func(m *mock.Mock) { @@ -752,10 +777,22 @@ var _ = Context("integration test", Ordered, func() { Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) // refetch // simulating a firewall-controller updating the resource mon.ControllerStatus = &v2.ControllerStatus{ - Updated: metav1.NewTime(time.Now()), + Updated: metav1.NewTime(time.Now()), + Distance: v2.FirewallRollingUpdateSetDistance, } Expect(k8sClient.Update(ctx, mon)).To(Succeed()) }) + + It("the firewall should reflect the distance during a rolling update", func() { + cond := testcommon.WaitForCondition(k8sClient, ctx, fw.DeepCopy(), func(fd *v2.Firewall) v2.Conditions { + return fd.Status.Conditions + }, v2.FirewallDistanceConfigured, v2.ConditionTrue, 15*time.Second) + + Expect(cond.LastTransitionTime).NotTo(BeZero()) + Expect(cond.LastUpdateTime).NotTo(BeZero()) + Expect(cond.Reason).To(Equal("Configured")) + Expect(cond.Message).To(Equal(fmt.Sprintf("Controller has configured the specified distance %d.", v2.FirewallRollingUpdateSetDistance))) + }) }) Context("the old generation disappears", func() { @@ -787,6 +824,10 @@ var _ = Context("integration test", Ordered, func() { return fw.Status.ControllerStatus }, 15*time.Second, interval).Should(Not(BeNil()), "controller connection was not synced to firewall resource") }) + + It("the firewall set should be updated to shortest distance as the update has succeeded", func() { + Expect(set.Spec.Distance).To(Equal(v2.FirewallShortestDistance)) + }) }) }) From 2cd24c9efe44d3b63eb861f06236ed780ed3ae87 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 11:45:24 +0200 Subject: [PATCH 02/12] Add missing validation on set resource. --- api/v2/validation/firewallset.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/v2/validation/firewallset.go b/api/v2/validation/firewallset.go index 785e7ba..defdd43 100644 --- a/api/v2/validation/firewallset.go +++ b/api/v2/validation/firewallset.go @@ -1,6 +1,8 @@ package validation import ( + "fmt" + "github.com/go-logr/logr" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" apivalidation "k8s.io/apimachinery/pkg/api/validation" @@ -30,6 +32,10 @@ func (v *firewallSetValidator) validateSpec(log logr.Logger, f *v2.FirewallSetSp allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "replicas cannot be a negative number")) } + if f.Distance < 0 || f.Distance > FirewallMaxDistance { + allErrs = append(allErrs, field.Invalid(fldPath.Child("distance"), f.Distance, fmt.Sprintf("distance must be between 0 and %d", f.Distance))) + } + if f.Selector == nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), f.Selector, "selector should not be nil")) } else { From f2db592038c85e2a531009106e82a1d5b365bdda Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 12:02:24 +0200 Subject: [PATCH 03/12] Return error on status update error to enqueue another reconcile. --- controllers/generic_controller.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/controllers/generic_controller.go b/controllers/generic_controller.go index 7e5aae8..a0811e2 100644 --- a/controllers/generic_controller.go +++ b/controllers/generic_controller.go @@ -129,22 +129,24 @@ func (g GenericController[O]) Reconcile(ctx context.Context, req ctrl.Request) ( } } + var statusErr error + if g.hasStatus { defer func() { log.Info("updating status") refetched := g.reconciler.New() - err := g.c.Get(ctx, req.NamespacedName, refetched, &client.GetOptions{}) - if err != nil { - log.Error(err, "unable to fetch resource before status update") + statusErr := g.c.Get(ctx, req.NamespacedName, refetched, &client.GetOptions{}) + if statusErr != nil { + log.Error(statusErr, "unable to fetch resource before status update") return } g.reconciler.SetStatus(o, refetched) - err = g.c.Status().Update(ctx, refetched) - if err != nil { - log.Error(err, "status could not be updated") + statusErr = g.c.Status().Update(ctx, refetched) + if statusErr != nil { + log.Error(statusErr, "status could not be updated") } return }() @@ -163,7 +165,7 @@ func (g GenericController[O]) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - return ctrl.Result{}, nil + return ctrl.Result{}, statusErr } type ItemGetter[O client.ObjectList, E metav1.Object] func(O) []E From 12d0cbdfa0c3b567f495d62d7d86490c785d5bce Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 12:45:12 +0200 Subject: [PATCH 04/12] Refetch. --- integration/integration_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/integration/integration_test.go b/integration/integration_test.go index 1eebf03..bd61d62 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -255,6 +255,8 @@ var _ = Context("integration test", Ordered, func() { It("should allow an update of the firewall monitor", func() { // simulating a firewall-controller updating the resource + mon := mon.DeepCopy() + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) mon.ControllerStatus = &v2.ControllerStatus{ Updated: metav1.NewTime(time.Now()), Distance: v2.FirewallShortestDistance, From f0070050203dbd6f89df9cc713b33456661c5970 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 12:58:54 +0200 Subject: [PATCH 05/12] Put firewall cache in there. --- controllers/firewall/controller.go | 34 +++++++++++++++--------------- controllers/firewall/delete.go | 2 +- controllers/firewall/reconcile.go | 2 +- integration/integration_test.go | 7 ++++-- 4 files changed, 24 insertions(+), 21 deletions(-) diff --git a/controllers/firewall/controller.go b/controllers/firewall/controller.go index 890e3b1..8b3449e 100644 --- a/controllers/firewall/controller.go +++ b/controllers/firewall/controller.go @@ -23,10 +23,11 @@ import ( ) type controller struct { - c *config.ControllerConfig - log logr.Logger - recorder record.EventRecorder - networkCache *cache.Cache[string, *models.V1NetworkResponse] + c *config.ControllerConfig + log logr.Logger + recorder record.EventRecorder + networkCache *cache.Cache[string, *models.V1NetworkResponse] + firewallCache *cache.Cache[*v2.Firewall, []*models.V1FirewallResponse] } func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.Manager, c *config.ControllerConfig) error { @@ -41,6 +42,18 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M } return resp.Payload, nil }), + firewallCache: cache.New(3*time.Second, func(ctx context.Context, fw *v2.Firewall) ([]*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 find error: %w", err) + } + + return resp.Payload, nil + }), }) return ctrl.NewControllerManagedBy(mgr). @@ -79,16 +92,3 @@ func (c *controller) New() *v2.Firewall { func (c *controller) SetStatus(reconciled *v2.Firewall, refetched *v2.Firewall) { refetched.Status = reconciled.Status } - -func (c *controller) findAssociatedFirewalls(ctx context.Context, fw *v2.Firewall) ([]*models.V1FirewallResponse, error) { - resp, err := c.c.GetMetal().Firewall().FindFirewalls(firewall.NewFindFirewallsParams().WithBody(&models.V1FirewallFindRequest{ - AllocationName: fw.Name, - AllocationProject: fw.Spec.Project, - Tags: []string{c.c.GetClusterTag()}, - }).WithContext(ctx), nil) - if err != nil { - return nil, fmt.Errorf("firewall find error: %w", err) - } - - return resp.Payload, nil -} diff --git a/controllers/firewall/delete.go b/controllers/firewall/delete.go index a878a5b..bbba348 100644 --- a/controllers/firewall/delete.go +++ b/controllers/firewall/delete.go @@ -18,7 +18,7 @@ func (c *controller) Delete(r *controllers.Ctx[*v2.Firewall]) error { return fmt.Errorf("unable to delete firewall monitor: %w", err) } - fws, err := c.findAssociatedFirewalls(r.Ctx, r.Target) + fws, err := c.firewallCache.Get(r.Ctx, r.Target) if err != nil { return controllers.RequeueAfter(10*time.Second, err.Error()) } diff --git a/controllers/firewall/reconcile.go b/controllers/firewall/reconcile.go index a0da458..69e5571 100644 --- a/controllers/firewall/reconcile.go +++ b/controllers/firewall/reconcile.go @@ -30,7 +30,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.Firewall]) error { } }() - fws, err := c.findAssociatedFirewalls(r.Ctx, r.Target) + fws, err := c.firewallCache.Get(r.Ctx, r.Target) if err != nil { return controllers.RequeueAfter(10*time.Second, err.Error()) } diff --git a/integration/integration_test.go b/integration/integration_test.go index bd61d62..442a7a3 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -255,8 +255,7 @@ var _ = Context("integration test", Ordered, func() { It("should allow an update of the firewall monitor", func() { // simulating a firewall-controller updating the resource - mon := mon.DeepCopy() - Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) // refetch mon.ControllerStatus = &v2.ControllerStatus{ Updated: metav1.NewTime(time.Now()), Distance: v2.FirewallShortestDistance, @@ -828,6 +827,10 @@ var _ = Context("integration test", Ordered, func() { }) It("the firewall set should be updated to shortest distance as the update has succeeded", func() { + Eventually(func() v2.FirewallDistance { + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(set), set)).To(Succeed()) + return set.Spec.Distance + }).Within(5 * time.Second).ProbeEvery(200 * time.Millisecond).Should(Equal(v2.FirewallShortestDistance)) Expect(set.Spec.Distance).To(Equal(v2.FirewallShortestDistance)) }) }) From 9d870c7fd6d3ee9b1cebced87150454e8d3c8822 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 15:36:50 +0200 Subject: [PATCH 06/12] Add field to indicate whether distance is supported or not. --- api/v2/types_firewallmonitor.go | 1 + .../firewall.metal-stack.io_firewallmonitors.yaml | 13 ++++++++----- controllers/firewall/status.go | 5 ++++- integration/integration_test.go | 10 ++++++---- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/api/v2/types_firewallmonitor.go b/api/v2/types_firewallmonitor.go index 56eb138..88d6f91 100644 --- a/api/v2/types_firewallmonitor.go +++ b/api/v2/types_firewallmonitor.go @@ -59,6 +59,7 @@ type ControllerStatus struct { NftablesExporterVersion string `json:"nftablesExporterVersion,omitempty"` Updated metav1.Time `json:"lastRun,omitempty"` Distance FirewallDistance `json:"distance,omitempty"` + DistanceSupported bool `json:"distanceSupported,omitempty"` } // FirewallStats contains firewall statistics diff --git a/config/crds/firewall.metal-stack.io_firewallmonitors.yaml b/config/crds/firewall.metal-stack.io_firewallmonitors.yaml index e6b82af..f0d94e6 100644 --- a/config/crds/firewall.metal-stack.io_firewallmonitors.yaml +++ b/config/crds/firewall.metal-stack.io_firewallmonitors.yaml @@ -94,14 +94,17 @@ spec: traffic in and out of the cluster. This is of particular interest during rolling firewall updates, i.e. when there is more than a single firewall running in front of the cluster. During a rolling - update, new firewalls will start with a longer distance such that - traffic is only attracted by the existing firewalls ("firewall staging"). + update, new firewalls start with a longer distance such that traffic + is only attracted by the existing firewalls ("firewall staging"). When the new firewall has connected successfully to the firewall monitor, the deployment controller throws away the old firewalls - and the new will firewall takes over the routing. The deployment - controller will then shorten the distance of the new firewall. This - approach reduces downtime of the effective user traffic of the cluster. + and the new firewall takes over the routing. The deployment controller + will then shorten the distance of the new firewall. This approach + reduces service interruption of the external user traffic of the + cluster (for firewall-controller versions that support this feature). type: integer + distanceSupported: + type: boolean lastRun: format: date-time type: string diff --git a/controllers/firewall/status.go b/controllers/firewall/status.go index 8158640..72871bc 100644 --- a/controllers/firewall/status.go +++ b/controllers/firewall/status.go @@ -162,7 +162,10 @@ func SetFirewallStatus(fw *v2.Firewall, mon *v2.FirewallMonitor) { fw.Status.Conditions.Set(cond) } - if fw.Distance == connection.ActualDistance { + if !mon.ControllerStatus.DistanceSupported { + cond := v2.NewCondition(v2.FirewallDistanceConfigured, v2.ConditionTrue, "NotChecking", "Controller does not support distance reconciliation.") + fw.Status.Conditions.Set(cond) + } else if fw.Distance == connection.ActualDistance { cond := v2.NewCondition(v2.FirewallDistanceConfigured, v2.ConditionTrue, "Configured", fmt.Sprintf("Controller has configured the specified distance %d.", fw.Distance)) fw.Status.Conditions.Set(cond) } else { diff --git a/integration/integration_test.go b/integration/integration_test.go index 3a08ed6..899de46 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -257,8 +257,9 @@ var _ = Context("integration test", Ordered, func() { // simulating a firewall-controller updating the resource Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) // refetch mon.ControllerStatus = &v2.ControllerStatus{ - Updated: metav1.NewTime(time.Now()), - Distance: v2.FirewallShortestDistance, + Updated: metav1.NewTime(time.Now()), + Distance: v2.FirewallShortestDistance, + DistanceSupported: true, } Expect(k8sClient.Update(ctx, mon)).To(Succeed()) }) @@ -781,8 +782,9 @@ var _ = Context("integration test", Ordered, func() { Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mon), mon)).To(Succeed()) // refetch // simulating a firewall-controller updating the resource mon.ControllerStatus = &v2.ControllerStatus{ - Updated: metav1.NewTime(time.Now()), - Distance: v2.FirewallRollingUpdateSetDistance, + Updated: metav1.NewTime(time.Now()), + Distance: v2.FirewallRollingUpdateSetDistance, + DistanceSupported: true, } Expect(k8sClient.Update(ctx, mon)).To(Succeed()) }) From aa40e2a8b7f3ac71bbaa84665b7a50da027032b1 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 21:19:41 +0200 Subject: [PATCH 07/12] Improve validations. --- api/v2/types_firewallset.go | 3 ++- api/v2/validation/firewall.go | 12 ++++++------ api/v2/validation/firewallset.go | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/api/v2/types_firewallset.go b/api/v2/types_firewallset.go index f1e9d93..49c3160 100644 --- a/api/v2/types_firewallset.go +++ b/api/v2/types_firewallset.go @@ -14,7 +14,8 @@ const ( FirewallControllerSetAnnotation = "firewall.metal.stack.io/set" FirewallShortestDistance = FirewallDistance(0) - FirewallRollingUpdateSetDistance = FirewallDistance(2) + FirewallRollingUpdateSetDistance = FirewallDistance(3) + FirewallLongestDistance = FirewallDistance(8) ) func FirewallSetTag(setName string) string { diff --git a/api/v2/validation/firewall.go b/api/v2/validation/firewall.go index 1f783e8..8ccad8e 100644 --- a/api/v2/validation/firewall.go +++ b/api/v2/validation/firewall.go @@ -13,10 +13,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -const ( - FirewallMaxDistance = 8 -) - type firewallValidator struct{} func NewFirewallValidator(log logr.Logger) *genericValidator[*v2.Firewall, *firewallValidator] { @@ -26,8 +22,8 @@ func NewFirewallValidator(log logr.Logger) *genericValidator[*v2.Firewall, *fire func (v *firewallValidator) ValidateCreate(log logr.Logger, f *v2.Firewall) field.ErrorList { var allErrs field.ErrorList - if f.Distance < 0 || f.Distance > FirewallMaxDistance { - allErrs = append(allErrs, field.Invalid(field.NewPath("distance"), f.Distance, fmt.Sprintf("distance must be between 0 and %d", f.Distance))) + if f.Distance < v2.FirewallShortestDistance || f.Distance > v2.FirewallLongestDistance { + allErrs = append(allErrs, field.Invalid(field.NewPath("distance"), f.Distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) } allErrs = append(allErrs, validateFirewallAnnotations(f)...) @@ -108,6 +104,10 @@ func (*firewallValidator) validateSpec(f *v2.FirewallSpec, fldPath *field.Path) func (v *firewallValidator) ValidateUpdate(log logr.Logger, fOld, fNew *v2.Firewall) field.ErrorList { var allErrs field.ErrorList + if fNew.Distance < v2.FirewallShortestDistance || fNew.Distance > v2.FirewallLongestDistance { + allErrs = append(allErrs, field.Invalid(field.NewPath("distance"), fNew.Distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) + } + allErrs = append(allErrs, validateFirewallAnnotations(fNew)...) allErrs = append(allErrs, v.validateSpecUpdate(&fOld.Spec, &fNew.Spec, field.NewPath("spec"))...) diff --git a/api/v2/validation/firewallset.go b/api/v2/validation/firewallset.go index defdd43..eb2aa0f 100644 --- a/api/v2/validation/firewallset.go +++ b/api/v2/validation/firewallset.go @@ -32,8 +32,8 @@ func (v *firewallSetValidator) validateSpec(log logr.Logger, f *v2.FirewallSetSp allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "replicas cannot be a negative number")) } - if f.Distance < 0 || f.Distance > FirewallMaxDistance { - allErrs = append(allErrs, field.Invalid(fldPath.Child("distance"), f.Distance, fmt.Sprintf("distance must be between 0 and %d", f.Distance))) + if f.Distance < v2.FirewallShortestDistance || f.Distance > v2.FirewallLongestDistance { + allErrs = append(allErrs, field.Invalid(fldPath.Child("distance"), f.Distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) } if f.Selector == nil { From 1907ef348099e9dc137a6c7d722f507084e887f2 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 31 Mar 2023 21:29:31 +0200 Subject: [PATCH 08/12] Small refactor. --- api/v2/validation/firewall.go | 20 ++++++++++++-------- api/v2/validation/firewallset.go | 6 +----- controllers/deployment/reconcile.go | 6 +++--- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/api/v2/validation/firewall.go b/api/v2/validation/firewall.go index 8ccad8e..eb43789 100644 --- a/api/v2/validation/firewall.go +++ b/api/v2/validation/firewall.go @@ -22,12 +22,9 @@ func NewFirewallValidator(log logr.Logger) *genericValidator[*v2.Firewall, *fire func (v *firewallValidator) ValidateCreate(log logr.Logger, f *v2.Firewall) field.ErrorList { var allErrs field.ErrorList - if f.Distance < v2.FirewallShortestDistance || f.Distance > v2.FirewallLongestDistance { - allErrs = append(allErrs, field.Invalid(field.NewPath("distance"), f.Distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) - } - allErrs = append(allErrs, validateFirewallAnnotations(f)...) allErrs = append(allErrs, v.validateSpec(&f.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, validateDistance(f.Distance, field.NewPath("distance"))...) return allErrs } @@ -104,12 +101,9 @@ func (*firewallValidator) validateSpec(f *v2.FirewallSpec, fldPath *field.Path) func (v *firewallValidator) ValidateUpdate(log logr.Logger, fOld, fNew *v2.Firewall) field.ErrorList { var allErrs field.ErrorList - if fNew.Distance < v2.FirewallShortestDistance || fNew.Distance > v2.FirewallLongestDistance { - allErrs = append(allErrs, field.Invalid(field.NewPath("distance"), fNew.Distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) - } - allErrs = append(allErrs, validateFirewallAnnotations(fNew)...) allErrs = append(allErrs, v.validateSpecUpdate(&fOld.Spec, &fNew.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, validateDistance(fNew.Distance, field.NewPath("distance"))...) return allErrs } @@ -144,3 +138,13 @@ func validateFirewallAnnotations(f *v2.Firewall) field.ErrorList { return allErrs } + +func validateDistance(distance v2.FirewallDistance, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + if distance < v2.FirewallShortestDistance || distance > v2.FirewallLongestDistance { + allErrs = append(allErrs, field.Invalid(fldPath, distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) + } + + return allErrs +} diff --git a/api/v2/validation/firewallset.go b/api/v2/validation/firewallset.go index eb2aa0f..e04dd41 100644 --- a/api/v2/validation/firewallset.go +++ b/api/v2/validation/firewallset.go @@ -1,8 +1,6 @@ package validation import ( - "fmt" - "github.com/go-logr/logr" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" apivalidation "k8s.io/apimachinery/pkg/api/validation" @@ -32,9 +30,7 @@ func (v *firewallSetValidator) validateSpec(log logr.Logger, f *v2.FirewallSetSp allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "replicas cannot be a negative number")) } - if f.Distance < v2.FirewallShortestDistance || f.Distance > v2.FirewallLongestDistance { - allErrs = append(allErrs, field.Invalid(fldPath.Child("distance"), f.Distance, fmt.Sprintf("distance must be between %d and %d", v2.FirewallShortestDistance, v2.FirewallLongestDistance))) - } + allErrs = append(allErrs, validateDistance(f.Distance, fldPath.Child("distance"))...) if f.Selector == nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), f.Selector, "selector should not be nil")) diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index ed0b533..2581c1a 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -63,16 +63,16 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error return err } - // we are done with the update, give the set the shortest distance if not already set + // 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 err := c.c.GetSeedClient().Update(r.Ctx, current) if err != nil { - return fmt.Errorf("unable to swap latest set distance to 0: %w", err) + return fmt.Errorf("unable to swap latest set distance to %d: %w", v2.FirewallShortestDistance, err) } - r.Log.Info("swapped latest set to distance to 0") + r.Log.Info("swapped latest set to shortest distance", "distance", v2.FirewallShortestDistance) } return nil From 84b8ed56854af8e6b6ea1f382ad4a4ada87acae6 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Thu, 6 Apr 2023 07:22:30 +0200 Subject: [PATCH 09/12] Stagger firewall replicas. --- api/v2/types_firewall.go | 53 +++++++++++++++++++++-------------- api/v2/types_firewall_test.go | 10 +++++-- controllers/set/reconcile.go | 41 +++++++++++++++++++++++---- 3 files changed, 76 insertions(+), 28 deletions(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index 4514f04..e2b7267 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -273,37 +273,39 @@ func (f *FirewallList) GetItems() []*Firewall { return result } -// SortFirewallsDeletion sorts the given firewall slice for deletion. +// SortFirewallsByImportance sorts the given firewall slice by important, +// e.g. for scale down. +// // It considers certain criteria which firewalls should be kept longest and // which one's can be deleted first. The precedence is: // -// - Weight annotation (defaults to 0 if no annotation is present) -// - Connected firewalls -// - Ready firewalls -// - Created Firealls -// - Younger Firewalls +// - Weight annotation (prefer higher weight, defaults to 0 if no annotation is present) +// - Firewall lifecycle phase (connected > ready > created, prefer shorter distance when equal) +// - Firewall age (prefer younger firewalls) // // The firewalls at the beginning of the slice should be kept as long as possible. // The firewalls at the end of the slice should be removed first. // // The firewalls can be popped off from the slice in a deletion loop. -func SortFirewallsDeletion(fws []*Firewall) { - weight := func(fw *Firewall) (weight int) { - a, ok := fw.Annotations[FirewallWeightAnnotation] - if !ok { - return - } +func SortFirewallsByImportance(fws []*Firewall) { + var ( + conditionTypes = []ConditionType{FirewallControllerConnected, FirewallReady, FirewallCreated} + + weight = func(fw *Firewall) (weight int) { + a, ok := fw.Annotations[FirewallWeightAnnotation] + if !ok { + return + } + + parsed, err := strconv.ParseInt(a, 10, 32) + if err != nil { + return + } - parsed, err := strconv.ParseInt(a, 10, 32) - if err != nil { + weight = int(parsed) return } - - weight = int(parsed) - return - } - - conditionTypes := []ConditionType{FirewallControllerConnected, FirewallReady, FirewallCreated} + ) sort.Slice(fws, func(i, j int) bool { a := fws[i] @@ -328,9 +330,18 @@ func SortFirewallsDeletion(fws []*Firewall) { if !aTrue && bTrue { return false } + if aTrue && bTrue { + // prefer shorter distances because these are potentially "active" + if a.Distance < b.Distance { + return true + } + if a.Distance > b.Distance { + return false + } + } } - // prefer younger firewalls + // prefer younger firewalls (these potentially run on a more up-to-date operating system image) return !a.CreationTimestamp.Before(&b.CreationTimestamp) }) } diff --git a/api/v2/types_firewall_test.go b/api/v2/types_firewall_test.go index b96c5e6..38e3843 100644 --- a/api/v2/types_firewall_test.go +++ b/api/v2/types_firewall_test.go @@ -9,7 +9,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestSortForScaleDown(t *testing.T) { +func Test_SortFirewallsByImportance(t *testing.T) { now := time.Now() tests := []struct { @@ -32,6 +32,9 @@ func TestSortForScaleDown(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{Name: "lowest weight", Annotations: map[string]string{FirewallWeightAnnotation: "-100"}}, }, + { + ObjectMeta: metav1.ObjectMeta{Name: "connected shortest distance", CreationTimestamp: metav1.NewTime(now.Add(-20 * time.Minute))}, Distance: -1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + }, { ObjectMeta: metav1.ObjectMeta{Name: "middle", CreationTimestamp: metav1.NewTime(now)}, }, @@ -58,6 +61,9 @@ func TestSortForScaleDown(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{Name: "highest weight", Annotations: map[string]string{FirewallWeightAnnotation: "100"}}, }, + { + ObjectMeta: metav1.ObjectMeta{Name: "connected shortest distance", CreationTimestamp: metav1.NewTime(now.Add(-20 * time.Minute))}, Distance: -1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + }, { ObjectMeta: metav1.ObjectMeta{Name: "connected younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, @@ -94,7 +100,7 @@ func TestSortForScaleDown(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - SortFirewallsDeletion(tt.fws) + SortFirewallsByImportance(tt.fws) if diff := cmp.Diff(tt.want, tt.fws, cmpopts.IgnoreFields(Condition{}, "LastTransitionTime", "LastUpdateTime")); diff != "" { t.Errorf("diff (+got -want):\n %s", diff) diff --git a/controllers/set/reconcile.go b/controllers/set/reconcile.go index 54d99e7..1949274 100644 --- a/controllers/set/reconcile.go +++ b/controllers/set/reconcile.go @@ -26,14 +26,48 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error { ownedFirewalls = append(ownedFirewalls, adoptions...) - for _, fw := range ownedFirewalls { + // we sort for distance orchestration and for scale down deletion + // - the most important firewall will get the shortest distance to attract all the traffic + // - 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 { fw.Spec = r.Target.Spec.Template.Spec - fw.Distance = r.Target.Spec.Distance + + // stagger firewall replicas to achieve active/standby behavior + // + // we give the most important firewall the shortest distance within a set + // this firewall attracts the traffic within the replica set + // + // the second most important firewall gets a longer distance + // such that it takes over the traffic in case the first replica dies + // (first-level backup) + // + // the rest of the firewalls get an even higher distance + // (third-level backup) + // one of them moves up on next set sync after deletion of the "active" replica + var distance v2.FirewallDistance + switch i { + case 0: + distance = r.Target.Spec.Distance + 0 + case 1: + distance = r.Target.Spec.Distance + 1 + default: + distance = r.Target.Spec.Distance + 2 + } + + if distance > v2.FirewallLongestDistance { + distance = v2.FirewallLongestDistance + } + + fw.Distance = distance err := c.c.GetSeedClient().Update(r.Ctx, fw, &client.UpdateOptions{}) if err != nil { return fmt.Errorf("error updating firewall spec: %w", err) } + + r.Log.Info("updated/synced firewall", "name", fw.Name, "distance", distance) } currentAmount := len(ownedFirewalls) @@ -58,9 +92,6 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error { if currentAmount > r.Target.Spec.Replicas { r.Log.Info("scale down", "current", currentAmount, "want", r.Target.Spec.Replicas) - // puts the least important at the end of the slice, we will then pop them off for deletion - v2.SortFirewallsDeletion(ownedFirewalls) - for i := r.Target.Spec.Replicas; i < currentAmount; i++ { var fw *v2.Firewall fw, ownedFirewalls = pop(ownedFirewalls) From 14b6aad19a46d28dd8b46ad7395b9e3b5a53ea07 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Thu, 13 Apr 2023 04:12:13 +0200 Subject: [PATCH 10/12] Allow scaling. --- api/v2/types_firewall.go | 1 + api/v2/types_firewalldeployment.go | 1 + api/v2/types_firewallset.go | 2 ++ api/v2/validation/firewalldeployment.go | 3 --- .../crds/firewall.metal-stack.io_firewalldeployments.yaml | 3 +++ config/crds/firewall.metal-stack.io_firewalls.yaml | 4 ++++ config/crds/firewall.metal-stack.io_firewallsets.yaml | 7 +++++++ 7 files changed, 18 insertions(+), 3 deletions(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index e2b7267..37e1e32 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -36,6 +36,7 @@ const ( // +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase" // +kubebuilder:printcolumn:name="Machine ID",type="string",JSONPath=".status.machineStatus.machineID" // +kubebuilder:printcolumn:name="Last Event",type="string",JSONPath=".status.machineStatus.lastEvent.event" +// +kubebuilder:printcolumn:name="Distance",type="string",priority=1,JSONPath=".distance" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".status.machineStatus.allocationTimestamp" // +kubebuilder:printcolumn:name="Spec Version",type="string",priority=1,JSONPath=".spec.controllerVersion" // +kubebuilder:printcolumn:name="Actual Version",type="string",priority=1,JSONPath=".status.controllerStatus.actualVersion" diff --git a/api/v2/types_firewalldeployment.go b/api/v2/types_firewalldeployment.go index cab9e61..d7b8268 100644 --- a/api/v2/types_firewalldeployment.go +++ b/api/v2/types_firewalldeployment.go @@ -12,6 +12,7 @@ import ( // +kubebuilder:object:root=true // +kubebuilder:resource:shortName=fwdeploy // +kubebuilder:subresource:status +// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.readyReplicas // +kubebuilder:printcolumn:name="Replicas",type=integer,JSONPath=`.spec.replicas` // +kubebuilder:printcolumn:name="Ready",type=integer,JSONPath=`.status.readyReplicas` // +kubebuilder:printcolumn:name="Progressing",type=integer,JSONPath=`.status.progressingReplicas` diff --git a/api/v2/types_firewallset.go b/api/v2/types_firewallset.go index 49c3160..b70555c 100644 --- a/api/v2/types_firewallset.go +++ b/api/v2/types_firewallset.go @@ -31,10 +31,12 @@ func FirewallManagedByTag() string { // +kubebuilder:object:root=true // +kubebuilder:resource:shortName=fwset // +kubebuilder:subresource:status +// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.readyReplicas // +kubebuilder:printcolumn:name="Replicas",type=integer,JSONPath=`.spec.replicas` // +kubebuilder:printcolumn:name="Ready",type=integer,JSONPath=`.status.readyReplicas` // +kubebuilder:printcolumn:name="Progressing",type=integer,JSONPath=`.status.progressingReplicas` // +kubebuilder:printcolumn:name="Unhealthy",type=integer,JSONPath=`.status.unhealthyReplicas` +// +kubebuilder:printcolumn:name="Distance",type="string",priority=1,JSONPath=".spec.distance" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" type FirewallSet struct { metav1.TypeMeta `json:",inline"` diff --git a/api/v2/validation/firewalldeployment.go b/api/v2/validation/firewalldeployment.go index 00f0c2a..6b28354 100644 --- a/api/v2/validation/firewalldeployment.go +++ b/api/v2/validation/firewalldeployment.go @@ -37,9 +37,6 @@ func (*firewallDeploymentValidator) validateSpec(log logr.Logger, f *v2.Firewall if f.Replicas < 0 { allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "replicas cannot be a negative number")) } - if f.Replicas > 1 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "for now, no more than a single firewall replica is allowed")) - } if f.Selector == nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), f.Selector, "selector should not be nil")) diff --git a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml index 4a7c38b..584cf0f 100644 --- a/config/crds/firewall.metal-stack.io_firewalldeployments.yaml +++ b/config/crds/firewall.metal-stack.io_firewalldeployments.yaml @@ -316,4 +316,7 @@ spec: served: true storage: true subresources: + scale: + specReplicasPath: .spec.replicas + statusReplicasPath: .status.readyReplicas status: {} diff --git a/config/crds/firewall.metal-stack.io_firewalls.yaml b/config/crds/firewall.metal-stack.io_firewalls.yaml index 3e91354..05d1fd2 100644 --- a/config/crds/firewall.metal-stack.io_firewalls.yaml +++ b/config/crds/firewall.metal-stack.io_firewalls.yaml @@ -27,6 +27,10 @@ spec: - jsonPath: .status.machineStatus.lastEvent.event name: Last Event type: string + - jsonPath: .distance + name: Distance + priority: 1 + type: string - jsonPath: .status.machineStatus.allocationTimestamp name: Age type: date diff --git a/config/crds/firewall.metal-stack.io_firewallsets.yaml b/config/crds/firewall.metal-stack.io_firewallsets.yaml index de307d0..bb817d4 100644 --- a/config/crds/firewall.metal-stack.io_firewallsets.yaml +++ b/config/crds/firewall.metal-stack.io_firewallsets.yaml @@ -30,6 +30,10 @@ spec: - jsonPath: .status.unhealthyReplicas name: Unhealthy type: integer + - jsonPath: .spec.distance + name: Distance + priority: 1 + type: string - jsonPath: .metadata.creationTimestamp name: Age type: date @@ -278,4 +282,7 @@ spec: served: true storage: true subresources: + scale: + specReplicasPath: .spec.replicas + statusReplicasPath: .status.readyReplicas status: {} From 3042844f86827f26a7c3581adac6602ec34c1b5a Mon Sep 17 00:00:00 2001 From: Gerrit Date: Thu, 13 Apr 2023 10:15:23 +0200 Subject: [PATCH 11/12] Review findings. --- api/v2/types_firewall_test.go | 12 ++++++------ api/v2/types_firewallset.go | 6 +++++- api/v2/validation/firewalldeployment.go | 3 +++ api/v2/validation/firewallset.go | 5 +++++ api/v2/validation/firewallset_test.go | 12 ++++++++++++ 5 files changed, 31 insertions(+), 7 deletions(-) diff --git a/api/v2/types_firewall_test.go b/api/v2/types_firewall_test.go index 38e3843..3823240 100644 --- a/api/v2/types_firewall_test.go +++ b/api/v2/types_firewall_test.go @@ -33,7 +33,7 @@ func Test_SortFirewallsByImportance(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "lowest weight", Annotations: map[string]string{FirewallWeightAnnotation: "-100"}}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "connected shortest distance", CreationTimestamp: metav1.NewTime(now.Add(-20 * time.Minute))}, Distance: -1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + ObjectMeta: metav1.ObjectMeta{Name: "connected shortest distance", CreationTimestamp: metav1.NewTime(now.Add(-20 * time.Minute))}, Distance: FirewallShortestDistance, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, { ObjectMeta: metav1.ObjectMeta{Name: "middle", CreationTimestamp: metav1.NewTime(now)}, @@ -42,7 +42,7 @@ func Test_SortFirewallsByImportance(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "created"}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallCreated, ConditionTrue, "Created", "")}}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "connected younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + ObjectMeta: metav1.ObjectMeta{Name: "connected younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Distance: FirewallShortestDistance + 1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, { ObjectMeta: metav1.ObjectMeta{Name: "ready younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, @@ -54,7 +54,7 @@ func Test_SortFirewallsByImportance(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "no information"}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "connected older", CreationTimestamp: metav1.NewTime(now.Add(-5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + ObjectMeta: metav1.ObjectMeta{Name: "connected older", CreationTimestamp: metav1.NewTime(now.Add(-5 * time.Minute))}, Distance: FirewallShortestDistance + 1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, }, want: []*Firewall{ @@ -62,13 +62,13 @@ func Test_SortFirewallsByImportance(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "highest weight", Annotations: map[string]string{FirewallWeightAnnotation: "100"}}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "connected shortest distance", CreationTimestamp: metav1.NewTime(now.Add(-20 * time.Minute))}, Distance: -1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + ObjectMeta: metav1.ObjectMeta{Name: "connected shortest distance", CreationTimestamp: metav1.NewTime(now.Add(-20 * time.Minute))}, Distance: FirewallShortestDistance, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "connected younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + ObjectMeta: metav1.ObjectMeta{Name: "connected younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Distance: FirewallShortestDistance + 1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, { - ObjectMeta: metav1.ObjectMeta{Name: "connected older", CreationTimestamp: metav1.NewTime(now.Add(-5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, + ObjectMeta: metav1.ObjectMeta{Name: "connected older", CreationTimestamp: metav1.NewTime(now.Add(-5 * time.Minute))}, Distance: FirewallShortestDistance + 1, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallControllerConnected, ConditionTrue, "Connected", ""), NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, }, { ObjectMeta: metav1.ObjectMeta{Name: "ready younger", CreationTimestamp: metav1.NewTime(now.Add(5 * time.Minute))}, Status: FirewallStatus{Conditions: Conditions{NewCondition(FirewallReady, ConditionTrue, "Ready", "")}}, diff --git a/api/v2/types_firewallset.go b/api/v2/types_firewallset.go index b70555c..b19a4a8 100644 --- a/api/v2/types_firewallset.go +++ b/api/v2/types_firewallset.go @@ -16,6 +16,10 @@ const ( FirewallShortestDistance = FirewallDistance(0) FirewallRollingUpdateSetDistance = FirewallDistance(3) FirewallLongestDistance = FirewallDistance(8) + + // FirewallMaxReplicas defines the maximum amount of firewall replicas to be defined. + // It does not make sense to allow large values here as it wastes a lot of machines. + FirewallMaxReplicas = 4 ) func FirewallSetTag(setName string) string { @@ -75,7 +79,7 @@ type FirewallSetSpec struct { // The deployment controller will then shorten the distance of the new firewall. // This approach reduces service interruption of the external user traffic of the cluster // (for firewall-controller versions that support this feature). -type FirewallDistance int +type FirewallDistance uint8 type FirewallSetStatus struct { // TargetReplicas is the amount of firewall replicas targeted to be running. diff --git a/api/v2/validation/firewalldeployment.go b/api/v2/validation/firewalldeployment.go index 6b28354..ab19882 100644 --- a/api/v2/validation/firewalldeployment.go +++ b/api/v2/validation/firewalldeployment.go @@ -37,6 +37,9 @@ func (*firewallDeploymentValidator) validateSpec(log logr.Logger, f *v2.Firewall if f.Replicas < 0 { allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "replicas cannot be a negative number")) } + if f.Replicas > v2.FirewallMaxReplicas { + allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, fmt.Sprintf("no more than %d firewall replicas are allowed", v2.FirewallMaxReplicas))) + } if f.Selector == nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("selector"), f.Selector, "selector should not be nil")) diff --git a/api/v2/validation/firewallset.go b/api/v2/validation/firewallset.go index e04dd41..821f10b 100644 --- a/api/v2/validation/firewallset.go +++ b/api/v2/validation/firewallset.go @@ -1,6 +1,8 @@ package validation import ( + "fmt" + "github.com/go-logr/logr" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" apivalidation "k8s.io/apimachinery/pkg/api/validation" @@ -29,6 +31,9 @@ func (v *firewallSetValidator) validateSpec(log logr.Logger, f *v2.FirewallSetSp if f.Replicas < 0 { allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, "replicas cannot be a negative number")) } + if f.Replicas > v2.FirewallMaxReplicas { + allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), f.Replicas, fmt.Sprintf("no more than %d firewall replicas are allowed", v2.FirewallMaxReplicas))) + } allErrs = append(allErrs, validateDistance(f.Distance, fldPath.Child("distance"))...) diff --git a/api/v2/validation/firewallset_test.go b/api/v2/validation/firewallset_test.go index fccf002..14c904e 100644 --- a/api/v2/validation/firewallset_test.go +++ b/api/v2/validation/firewallset_test.go @@ -110,6 +110,18 @@ func Test_firewalSetValidator_ValidateCreate(t *testing.T) { }, }, }, + { + name: "no more than max allowed replicas", + mutateFn: func(f *v2.FirewallSet) *v2.FirewallSet { + f.Spec.Replicas = 200 + return f + }, + wantErr: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Message: ` "firewall" is invalid: spec.replicas: Invalid value: 200: no more than 4 firewall replicas are allowed`, + }, + }, + }, } for _, tt := range tests { tt := tt From 90994699990dfd6425e3f9bde461a5e666ca9f3c Mon Sep 17 00:00:00 2001 From: Gerrit Date: Thu, 13 Apr 2023 10:20:43 +0200 Subject: [PATCH 12/12] Typo in comment. --- api/v2/types_firewall.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index 37e1e32..db324aa 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -274,7 +274,7 @@ func (f *FirewallList) GetItems() []*Firewall { return result } -// SortFirewallsByImportance sorts the given firewall slice by important, +// SortFirewallsByImportance sorts the given firewall slice by importance, // e.g. for scale down. // // It considers certain criteria which firewalls should be kept longest and