diff --git a/api/v2/types_annotations.go b/api/v2/types_annotations.go new file mode 100644 index 0000000..9d10fea --- /dev/null +++ b/api/v2/types_annotations.go @@ -0,0 +1,183 @@ +package v2 + +import ( + "context" + "strconv" + + "github.com/google/go-cmp/cmp" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +const ( + // ReconcileAnnotation can be used to trigger a reconciliation of a resource managed by a controller. + // The value of the annotation does not matter, the controller will cleanup the annotation automatically and trigger a reconciliation of the resource. + ReconcileAnnotation = "firewall.metal-stack.io/reconcile" + // MaintenanceAnnotation can be used to trigger a maintenance reconciliation for which a controller might have special behavior. + // The value of the annotation does not matter, the controller will cleanup the annotation automatically. + MaintenanceAnnotation = "firewall.metal-stack.io/maintain" + // RollSetAnnotation can be used to trigger a rolling update of a firewall deployment. + // The value of the annotation needs to be true otherwise the controller will ignore it. + RollSetAnnotation = "firewall.metal-stack.io/roll-set" + // RevisionAnnotation stores the revision number of a resource. + RevisionAnnotation = "firewall.metal-stack.io/revision" + + // FirewallNoControllerConnectionAnnotation can be used as an annotation to the firewall resource in order + // to indicate that the firewall-controller does not connect to the firewall monitor. this way, the replica + // set will become healthy without a controller connection. + // + // this can be useful to silence a problem temporarily and was used in the past for migration of firewall-controller v1. + FirewallNoControllerConnectionAnnotation = "firewall.metal-stack.io/no-controller-connection" + // FirewallControllerManagedByAnnotation is used as tag for creating a firewall to indicate who is managing the firewall. + FirewallControllerManagedByAnnotation = "firewall.metal-stack.io/managed-by" + // FirewallWeightAnnotation is considered when deciding which firewall is thrown away on scale down. + // Value must be parsable as an integer. Firewalls with higher weight are kept longer. + // Defaults to 0 if no annotation is present. Negative values are allowed. + FirewallWeightAnnotation = "firewall.metal-stack.io/weight" + + // FirewallControllerSetAnnotation is a tag added to the firewall entity indicating to which set a firewall belongs to. + FirewallControllerSetAnnotation = "firewall.metal.stack.io/set" +) + +// IsAnnotationPresent returns true if the given object has an annotation with a given +// key, the value of this annotation does not matter. +func IsAnnotationPresent(o client.Object, key string) bool { + _, ok := o.GetAnnotations()[key] + return ok +} + +// 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 +} + +// IsAnnotationFalse returns true if the given object has an annotation with a given +// key and the value of this annotation is a false boolean. +func IsAnnotationFalse(o client.Object, key string) bool { + enabled, err := strconv.ParseBool(o.GetAnnotations()[key]) + return err == nil && !enabled +} + +// AddAnnotation adds an annotation by a given key to an object if not present by updating it with the given client. +func AddAnnotation(ctx context.Context, c client.Client, o client.Object, key, value string) error { + annotations := o.GetAnnotations() + + if annotations == nil { + annotations = map[string]string{} + } + + if existingValue, ok := annotations[key]; ok && existingValue == value { + return nil + } + + annotations[key] = value + + o.SetAnnotations(annotations) + + err := c.Update(ctx, o) + if err != nil { + return err + } + + return nil +} + +// RemoveAnnotation removes an annotation by a given key from an object if present by updating it with the given client. +func RemoveAnnotation(ctx context.Context, c client.Client, o client.Object, key string) error { + annotations := o.GetAnnotations() + + if annotations == nil { + return nil + } + + _, ok := annotations[key] + if !ok { + return nil + } + + delete(annotations, key) + + o.SetAnnotations(annotations) + + err := c.Update(ctx, o) + if err != nil { + return err + } + + return nil +} + +// AnnotationAddedPredicate returns a predicate when the given annotation key was added. +func AnnotationAddedPredicate(annotation string) predicate.Funcs { + return predicate.Funcs{ + CreateFunc: func(ce event.CreateEvent) bool { + return false + }, + UpdateFunc: func(update event.UpdateEvent) bool { + return annotationWasAdded(update, annotation) + }, + DeleteFunc: func(de event.DeleteEvent) bool { + return false + }, + } +} + +// AnnotationRemovedPredicate returns a predicate when the given annotation key was removed. +func AnnotationRemovedPredicate(annotation string) predicate.Funcs { + return predicate.Funcs{ + CreateFunc: func(ce event.CreateEvent) bool { + return false + }, + UpdateFunc: func(update event.UpdateEvent) bool { + return annotationWasRemoved(update, annotation) + }, + DeleteFunc: func(de event.DeleteEvent) bool { + return false + }, + } +} + +func annotationWasRemoved(update event.UpdateEvent, annotation string) bool { + if cmp.Equal(update.ObjectOld.GetAnnotations(), update.ObjectNew.GetAnnotations()) { + return false + } + + var ( + _, o = update.ObjectOld.GetAnnotations()[annotation] + _, n = update.ObjectNew.GetAnnotations()[annotation] + ) + + if n { + return false + } + + if !o { + return false + } + + return o && !n +} + +func annotationWasAdded(update event.UpdateEvent, annotation string) bool { + if cmp.Equal(update.ObjectOld.GetAnnotations(), update.ObjectNew.GetAnnotations()) { + return false + } + + var ( + _, o = update.ObjectOld.GetAnnotations()[annotation] + _, n = update.ObjectNew.GetAnnotations()[annotation] + ) + + if o { + return false + } + + if !n { + return false + } + + return !o && n +} diff --git a/api/v2/types_annotations_test.go b/api/v2/types_annotations_test.go new file mode 100644 index 0000000..ce8060c --- /dev/null +++ b/api/v2/types_annotations_test.go @@ -0,0 +1,427 @@ +package v2 + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/metal-stack/metal-lib/pkg/testcommon" + "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" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestIsAnnotationPresent(t *testing.T) { + tests := []struct { + name string + o client.Object + key string + want bool + }{ + { + name: "not present", + o: &Firewall{}, + key: "a", + want: false, + }, + { + name: "present", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "", + }, + }, + }, + key: "a", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsAnnotationPresent(tt.o, tt.key); got != tt.want { + t.Errorf("IsAnnotationPresent() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsAnnotationIsTrue(t *testing.T) { + tests := []struct { + name string + o client.Object + key string + want bool + }{ + { + name: "not present", + o: &Firewall{}, + key: "a", + want: false, + }, + { + name: "not true", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "foo", + }, + }, + }, + key: "a", + want: false, + }, + { + name: "true", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "true", + }, + }, + }, + key: "a", + want: true, + }, + { + name: "different variant of true is also allowed", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "1", + }, + }, + }, + key: "a", + want: true, + }, + { + name: "false", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "false", + }, + }, + }, + key: "a", + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsAnnotationTrue(tt.o, tt.key); got != tt.want { + t.Errorf("IsAnnotationTrue() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsAnnotationIsFalse(t *testing.T) { + tests := []struct { + name string + o client.Object + key string + want bool + }{ + { + name: "not present", + o: &Firewall{}, + key: "a", + want: false, + }, + { + name: "not true", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "foo", + }, + }, + }, + key: "a", + want: false, + }, + { + name: "true", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "true", + }, + }, + }, + key: "a", + want: false, + }, + { + name: "false", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "false", + }, + }, + }, + key: "a", + want: true, + }, + { + name: "different variant of false is also allowed", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: map[string]string{ + "a": "0", + }, + }, + }, + key: "a", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsAnnotationFalse(tt.o, tt.key); got != tt.want { + t.Errorf("IsAnnotationFalse() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestAddAnnotation(t *testing.T) { + scheme := runtime.NewScheme() + err := AddToScheme(scheme) + require.NoError(t, err) + ctx := context.Background() + + tests := []struct { + name string + o *Firewall + key string + value string + wantErr error + want *Firewall + }{ + { + name: "add", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Name: "test", + ResourceVersion: "0", + }, + }, + key: "test", + value: "true", + wantErr: nil, + want: &Firewall{ + TypeMeta: v1.TypeMeta{ + Kind: "Firewall", + APIVersion: "firewall.metal-stack.io/v2", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "test", + ResourceVersion: "1", + Annotations: map[string]string{ + "test": "true", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.o).Build() + + err := AddAnnotation(ctx, c, tt.o, tt.key, tt.value) + if diff := cmp.Diff(tt.wantErr, err, testcommon.ErrorStringComparer()); diff != "" { + t.Errorf("error diff (+got -want):\n %s", diff) + } + + updated := &Firewall{} + err = c.Get(ctx, client.ObjectKeyFromObject(tt.o), updated) + require.NoError(t, err) + + if diff := cmp.Diff(updated, tt.want); diff != "" { + t.Errorf("diff (+got -want):\n %s", diff) + } + }) + } +} + +func TestRemoveAnnotation(t *testing.T) { + scheme := runtime.NewScheme() + err := AddToScheme(scheme) + require.NoError(t, err) + ctx := context.Background() + + tests := []struct { + name string + o *Firewall + key string + value string + wantErr error + want *Firewall + }{ + { + name: "remove", + o: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Name: "test", + ResourceVersion: "0", + Annotations: map[string]string{ + "test": "true", + }, + }, + }, + key: "test", + wantErr: nil, + want: &Firewall{ + TypeMeta: v1.TypeMeta{ + Kind: "Firewall", + APIVersion: "firewall.metal-stack.io/v2", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "test", + ResourceVersion: "1", + Annotations: nil, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.o).Build() + + err := RemoveAnnotation(ctx, c, tt.o, tt.key) + if diff := cmp.Diff(tt.wantErr, err, testcommon.ErrorStringComparer()); diff != "" { + t.Errorf("error diff (+got -want):\n %s", diff) + } + + updated := &Firewall{} + err = c.Get(ctx, client.ObjectKeyFromObject(tt.o), updated) + require.NoError(t, err) + + if diff := cmp.Diff(updated, tt.want); diff != "" { + t.Errorf("diff (+got -want):\n %s", diff) + } + }) + } +} +func Test_annotationWasRemoved(t *testing.T) { + tests := []struct { + name string + o map[string]string + n map[string]string + annotation string + want bool + }{ + { + name: "annotation is not present", + annotation: "c", + o: map[string]string{"a": ""}, + n: map[string]string{"b": ""}, + want: false, + }, + { + name: "annotation is present", + annotation: "c", + o: map[string]string{"c": ""}, + n: map[string]string{"c": ""}, + want: false, + }, + { + name: "annotation was added", + annotation: "c", + o: nil, + n: map[string]string{"c": ""}, + want: false, + }, + { + name: "annotation was removed", + annotation: "c", + o: map[string]string{"c": ""}, + n: nil, + want: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + if got := annotationWasRemoved(event.UpdateEvent{ + ObjectOld: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: tt.o, + }, + }, + ObjectNew: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: tt.n, + }, + }, + }, tt.annotation); got != tt.want { + t.Errorf("annotationWasRemoved() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_annotationWasAdded(t *testing.T) { + tests := []struct { + name string + o map[string]string + n map[string]string + annotation string + want bool + }{ + { + name: "annotation is not present", + annotation: "c", + o: map[string]string{"a": ""}, + n: map[string]string{"b": ""}, + want: false, + }, + { + name: "annotation is present", + annotation: "c", + o: map[string]string{"c": ""}, + n: map[string]string{"c": ""}, + want: false, + }, + { + name: "annotation was added", + annotation: "c", + o: nil, + n: map[string]string{"c": ""}, + want: true, + }, + { + name: "annotation was removed", + annotation: "c", + o: map[string]string{"c": ""}, + n: nil, + want: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + if got := annotationWasAdded(event.UpdateEvent{ + ObjectOld: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: tt.o, + }, + }, + ObjectNew: &Firewall{ + ObjectMeta: v1.ObjectMeta{ + Annotations: tt.n, + }, + }, + }, tt.annotation); got != tt.want { + t.Errorf("annotationWasAdded() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/api/v2/types_firewall.go b/api/v2/types_firewall.go index 322ce25..e768905 100644 --- a/api/v2/types_firewall.go +++ b/api/v2/types_firewall.go @@ -9,18 +9,6 @@ import ( ) const ( - // FirewallNoControllerConnectionAnnotation can be used as an annotation to the firewall resource in order - // to indicate that the firewall-controller does not connect to the firewall monitor. this way, the replica - // set will become healthy without a controller connection. - // - // this can be useful to silence a problem temporarily and was used in the past for migration of firewall-controller v1. - FirewallNoControllerConnectionAnnotation = "firewall.metal-stack.io/no-controller-connection" - // FirewallControllerManagedByAnnotation is used as tag for creating a firewall to indicate who is managing the firewall. - FirewallControllerManagedByAnnotation = "firewall.metal-stack.io/managed-by" - // FirewallWeightAnnotation is considered when deciding which firewall is thrown away on scale down. - // Value must be parsable as an integer. Firewalls with higher weight are kept longer. - // Defaults to 0 if no annotation is present. Negative values are allowed. - FirewallWeightAnnotation = "firewall.metal-stack.io/weight" // FirewallControllerManager is a name of the firewall-controller-manager managing the firewall. FirewallControllerManager = "firewall-controller-manager" ) @@ -327,7 +315,7 @@ func SortFirewallsByImportance(fws []*Firewall) { a := fws[i] b := fws[j] - // prefer heigher weight + // prefer higher weight if weight(a) > weight(b) { return true } diff --git a/api/v2/types_firewallset.go b/api/v2/types_firewallset.go index edc8409..cfd58fa 100644 --- a/api/v2/types_firewallset.go +++ b/api/v2/types_firewallset.go @@ -10,9 +10,6 @@ import ( // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. 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(3) FirewallLongestDistance = FirewallDistance(8) diff --git a/api/v2/types_utils.go b/api/v2/types_utils.go index 42130e7..644be46 100644 --- a/api/v2/types_utils.go +++ b/api/v2/types_utils.go @@ -1,19 +1,12 @@ 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" ) const ( - FinalizerName = "firewall.metal-stack.io/firewall-controller-manager" - RollSetAnnotation = "firewall.metal-stack.io/roll-set" - RevisionAnnotation = "firewall.metal-stack.io/revision" + // FinalizerName is the finalizer name used by this controller. + FinalizerName = "firewall.metal-stack.io/firewall-controller-manager" ) // ConditionStatus is the status of a condition. @@ -104,41 +97,3 @@ func (cs Conditions) filterOutCondition(t ConditionType) Conditions { } return newConditions } - -// SkipReconcileAnnotationRemoval returns a predicate when the firewall controller reconcile annotation -// was cleaned up. -func SkipRollSetAnnotationRemoval() predicate.Funcs { - return predicate.Funcs{ - UpdateFunc: func(update event.UpdateEvent) bool { - return !annotationWasRemoved(update, RollSetAnnotation) - }, - } -} - -func annotationWasRemoved(update event.UpdateEvent, annotation string) bool { - if cmp.Equal(update.ObjectOld.GetAnnotations(), update.ObjectNew.GetAnnotations()) { - return false - } - - var ( - _, o = update.ObjectOld.GetAnnotations()[annotation] - _, n = update.ObjectNew.GetAnnotations()[annotation] - ) - - if n { - return false - } - - if !o { - return false - } - - 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/types_utils_test.go b/api/v2/types_utils_test.go index 488e3e7..5ecad28 100644 --- a/api/v2/types_utils_test.go +++ b/api/v2/types_utils_test.go @@ -5,8 +5,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/event" ) func TestConditions(t *testing.T) { @@ -75,61 +73,3 @@ func TestConditions(t *testing.T) { t.Errorf("diff (+got -want):\n %s", diff) } } - -func Test_annotationWasRemoved(t *testing.T) { - tests := []struct { - name string - o map[string]string - n map[string]string - annotation string - want bool - }{ - { - name: "annotation is not present", - annotation: "c", - o: map[string]string{"a": ""}, - n: map[string]string{"b": ""}, - want: false, - }, - { - name: "annotation is present", - annotation: "c", - o: map[string]string{"c": ""}, - n: map[string]string{"c": ""}, - want: false, - }, - { - name: "annotation was added", - annotation: "c", - o: nil, - n: map[string]string{"c": ""}, - want: false, - }, - { - name: "annotation was removed", - annotation: "c", - o: map[string]string{"c": ""}, - n: nil, - want: true, - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - if got := annotationWasRemoved(event.UpdateEvent{ - ObjectOld: &Firewall{ - ObjectMeta: v1.ObjectMeta{ - Annotations: tt.o, - }, - }, - ObjectNew: &Firewall{ - ObjectMeta: v1.ObjectMeta{ - Annotations: tt.n, - }, - }, - }, tt.annotation); got != tt.want { - t.Errorf("annotationWasRemoved() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/controllers/deployment/controller.go b/controllers/deployment/controller.go index 0af5a13..a604540 100644 --- a/controllers/deployment/controller.go +++ b/controllers/deployment/controller.go @@ -24,7 +24,7 @@ type controller struct { } func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.Manager, c *config.ControllerConfig) error { - g := controllers.NewGenericController[*v2.FirewallDeployment](log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ + g := controllers.NewGenericController(log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ c: c, log: log, recorder: recorder, @@ -35,15 +35,28 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M For( &v2.FirewallDeployment{}, builder.WithPredicates( - predicate.Or( - predicate.GenerationChangedPredicate{}, // prevents reconcile on status sub resource update - predicate.AnnotationChangedPredicate{}, - predicate.LabelChangedPredicate{}, + predicate.And( + predicate.Not(v2.AnnotationRemovedPredicate(v2.MaintenanceAnnotation)), + predicate.Or( + predicate.GenerationChangedPredicate{}, // prevents reconcile on status sub resource update + predicate.AnnotationChangedPredicate{}, + predicate.LabelChangedPredicate{}, + ), ), ), ). Named("FirewallDeployment"). - Owns(&v2.FirewallSet{}). + Owns( + &v2.FirewallSet{}, + builder.WithPredicates( + predicate.Not( + predicate.Or( + v2.AnnotationAddedPredicate(v2.ReconcileAnnotation), + v2.AnnotationRemovedPredicate(v2.ReconcileAnnotation), + ), + ), + ), + ). WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). Complete(g) } diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index 52289fb..0b19889 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -13,6 +13,7 @@ import ( "github.com/metal-stack/metal-go/api/client/image" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -157,12 +158,25 @@ func (c *controller) createFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment } func (c *controller) syncFirewallSet(r *controllers.Ctx[*v2.FirewallDeployment], set *v2.FirewallSet) error { - set.Spec.Replicas = r.Target.Spec.Replicas - set.Spec.Template = r.Target.Spec.Template + err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + refetched := &v2.FirewallSet{} + err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKeyFromObject(set), refetched) + if err != nil { + return fmt.Errorf("unable re-fetch firewall set: %w", err) + } + + refetched.Spec.Replicas = r.Target.Spec.Replicas + refetched.Spec.Template = r.Target.Spec.Template - err := c.c.GetSeedClient().Update(r.Ctx, set) + err = c.c.GetSeedClient().Update(r.Ctx, refetched) + if err != nil { + return fmt.Errorf("unable to update/sync firewall set: %w", err) + } + + return nil + }) if err != nil { - return fmt.Errorf("unable to update/sync firewall set: %w", err) + return err } r.Log.Info("updated firewall set", "set-name", set.Name) diff --git a/controllers/firewall/controller.go b/controllers/firewall/controller.go index 65c6eed..d8cec5c 100644 --- a/controllers/firewall/controller.go +++ b/controllers/firewall/controller.go @@ -31,7 +31,7 @@ type controller struct { } func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.Manager, c *config.ControllerConfig) error { - g := controllers.NewGenericController[*v2.Firewall](log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ + g := controllers.NewGenericController(log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ log: log, recorder: recorder, c: c, diff --git a/controllers/generic_controller.go b/controllers/generic_controller.go index 17772fc..0d60c16 100644 --- a/controllers/generic_controller.go +++ b/controllers/generic_controller.go @@ -18,9 +18,10 @@ import ( type ( Ctx[O client.Object] struct { - Ctx context.Context - Log logr.Logger - Target O + Ctx context.Context + Log logr.Logger + Target O + WithinMaintenance bool } Reconciler[O client.Object] interface { // New returns a new object of O. @@ -81,9 +82,10 @@ func (g GenericController[O]) Reconcile(ctx context.Context, req ctrl.Request) ( o = g.reconciler.New() log = g.logger(req) rctx = &Ctx[O]{ - Ctx: ctx, - Log: log, - Target: o, + Ctx: ctx, + Log: log, + Target: o, + WithinMaintenance: false, } ) @@ -129,40 +131,79 @@ func (g GenericController[O]) Reconcile(ctx context.Context, req ctrl.Request) ( } } + if v2.IsAnnotationPresent(o, v2.ReconcileAnnotation) { + err := v2.RemoveAnnotation(ctx, g.c, o, v2.ReconcileAnnotation) + if err != nil { + return ctrl.Result{}, fmt.Errorf("unable to remove reconcile annotation: %w", err) + } + + // the update of the annotation removal triggers the next reconciliation + log.Info("removed reconcile annotation from resource") + return ctrl.Result{}, nil + } + var statusErr error if g.hasStatus { defer func() { log.Info("updating status") - refetched := g.reconciler.New() + obj := g.reconciler.New() - statusErr = g.c.Get(ctx, req.NamespacedName, refetched, &client.GetOptions{}) + statusErr = g.c.Get(ctx, req.NamespacedName, obj, &client.GetOptions{}) if statusErr != nil { log.Error(statusErr, "unable to fetch resource before status update") return } - g.reconciler.SetStatus(o, refetched) + g.reconciler.SetStatus(o, obj) - statusErr = g.c.Status().Update(ctx, refetched) + statusErr = g.c.Status().Update(ctx, obj) if statusErr != nil { log.Error(statusErr, "status could not be updated") } + return }() } + if v2.IsAnnotationPresent(o, v2.MaintenanceAnnotation) { + log.Info("reconciling in maintenance mode") + + rctx.WithinMaintenance = true + + defer func() { + obj := g.reconciler.New() + + err := g.c.Get(ctx, req.NamespacedName, obj, &client.GetOptions{}) + if err != nil { + log.Error(err, "unable to fetch resource before maintenance annotation removal") + return + } + + err = v2.RemoveAnnotation(ctx, g.c, obj, v2.MaintenanceAnnotation) + if err != nil { + log.Error(err, "unable to cleanup maintenance annotation") + return + } + + log.Info("cleaned up maintenance annotation") + }() + } + log.Info("reconciling resource") + err := g.reconciler.Reconcile(rctx) if err != nil { var requeueErr *requeueError - if errors.As(err, &requeueErr) { + + switch { + case errors.As(err, &requeueErr): log.Info(requeueErr.Error()) return ctrl.Result{RequeueAfter: requeueErr.after}, nil //nolint:nilerr we need to return nil such that the requeue works + default: + log.Error(err, "error during reconcile") + return ctrl.Result{}, err } - - log.Error(err, "error during reconcile") - return ctrl.Result{}, err } return ctrl.Result{}, statusErr diff --git a/controllers/monitor/controller.go b/controllers/monitor/controller.go index ed37217..cc5d5ee 100644 --- a/controllers/monitor/controller.go +++ b/controllers/monitor/controller.go @@ -3,6 +3,7 @@ package monitor import ( "github.com/go-logr/logr" 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" @@ -16,16 +17,21 @@ type controller struct { } func SetupWithManager(log logr.Logger, mgr ctrl.Manager, c *config.ControllerConfig) error { - g := controllers.NewGenericController[*v2.FirewallMonitor](log, c.GetShootClient(), c.GetShootNamespace(), &controller{ + g := controllers.NewGenericController(log, c.GetShootClient(), c.GetShootNamespace(), &controller{ log: log, c: c, }).WithoutStatus() return ctrl.NewControllerManagedBy(mgr). - For(&v2.FirewallMonitor{}). + For(&v2.FirewallMonitor{}, + builder.WithPredicates( + predicate.Not( + v2.AnnotationRemovedPredicate(v2.RollSetAnnotation), + ), + ), + ). Named("FirewallMonitor"). WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetShootNamespace()))). - WithEventFilter(v2.SkipRollSetAnnotationRemoval()). Complete(g) } diff --git a/controllers/monitor/reconcile.go b/controllers/monitor/reconcile.go index 3583881..d85e445 100644 --- a/controllers/monitor/reconcile.go +++ b/controllers/monitor/reconcile.go @@ -54,51 +54,33 @@ func (c *controller) updateFirewallStatus(r *controllers.Ctx[*v2.FirewallMonitor } func (c *controller) rollSetAnnotation(r *controllers.Ctx[*v2.FirewallMonitor]) error { - v, ok := r.Target.Annotations[v2.RollSetAnnotation] - if !ok { + rollSet := v2.IsAnnotationTrue(r.Target, v2.RollSetAnnotation) + if !rollSet { return nil } - r.Log.Info("resource was annotated", "annotation", v2.RollSetAnnotation, "value", v) + r.Log.Info("initiating firewall set roll as requested by user annotation") - delete(r.Target.Annotations, v2.RollSetAnnotation) + fw := &v2.Firewall{ + ObjectMeta: metav1.ObjectMeta{ + Name: r.Target.Name, + Namespace: c.c.GetSeedNamespace(), + }, + } - err := c.c.GetShootClient().Update(r.Ctx, r.Target) + set, err := findCorrespondingSet(r.Ctx, c.c.GetSeedClient(), fw) if err != nil { - return err + return client.IgnoreNotFound(err) } - r.Log.Info("cleaned up annotation") - - rollSet, err := strconv.ParseBool(v) + err = v2.AddAnnotation(r.Ctx, c.c.GetSeedClient(), set, v2.RollSetAnnotation, strconv.FormatBool(true)) if err != nil { - r.Log.Error(err, "unable to parse annotation value, ignoring") - return nil + return fmt.Errorf("unable to annotate firewall set: %w", err) } - if rollSet { - r.Log.Info("initiating firewall set roll as requested by user annotation") - - fw := &v2.Firewall{ - ObjectMeta: metav1.ObjectMeta{ - Name: r.Target.Name, - Namespace: c.c.GetSeedNamespace(), - }, - } - - set, err := findCorrespondingSet(r.Ctx, c.c.GetSeedClient(), fw) - if err != nil { - return client.IgnoreNotFound(err) - } - - set.Annotations[v2.RollSetAnnotation] = strconv.FormatBool(true) - - err = c.c.GetSeedClient().Update(r.Ctx, set) - if err != nil { - return fmt.Errorf("unable to annotate firewall set: %w", err) - } - - r.Log.Info("firewall set annotated") + err = v2.RemoveAnnotation(r.Ctx, c.c.GetShootClient(), r.Target, v2.RollSetAnnotation) + if err != nil { + return fmt.Errorf("unable to cleanup firewall monitor roll-set annotation: %w", err) } return nil diff --git a/controllers/set/controller.go b/controllers/set/controller.go index 6fc09fc..06909c5 100644 --- a/controllers/set/controller.go +++ b/controllers/set/controller.go @@ -20,7 +20,7 @@ type controller struct { } func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.Manager, c *config.ControllerConfig) error { - g := controllers.NewGenericController[*v2.FirewallSet](log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ + g := controllers.NewGenericController(log, c.GetSeedClient(), c.GetSeedNamespace(), &controller{ log: log, recorder: recorder, c: c, @@ -37,7 +37,17 @@ func SetupWithManager(log logr.Logger, recorder record.EventRecorder, mgr ctrl.M ), ). Named("FirewallSet"). - Owns(&v2.Firewall{}). + Owns( + &v2.Firewall{}, + builder.WithPredicates( + predicate.Not( + predicate.Or( + v2.AnnotationAddedPredicate(v2.ReconcileAnnotation), + v2.AnnotationRemovedPredicate(v2.ReconcileAnnotation), + ), + ), + ), + ). WithEventFilter(predicate.NewPredicateFuncs(controllers.SkipOtherNamespace(c.GetSeedNamespace()))). Complete(g) } diff --git a/controllers/set/controller_test.go b/controllers/set/controller_test.go index 13a3bce..77ba538 100644 --- a/controllers/set/controller_test.go +++ b/controllers/set/controller_test.go @@ -2,6 +2,7 @@ package set_test import ( "fmt" + "strconv" "time" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" @@ -119,7 +120,7 @@ var _ = Context("firewall set controller", Ordered, func() { firewall := testcommon.WaitForResourceAmount(k8sClient, ctx, namespaceName, 1, &v2.FirewallList{}, func(l *v2.FirewallList) []*v2.Firewall { return l.GetItems() - }, 3*time.Second) + }, 5*time.Second) Expect(firewall.Name).To(Equal(newest.Name), "older firewalls were kept") }) @@ -146,4 +147,23 @@ var _ = Context("firewall set controller", Ordered, func() { Expect(k8sClient.Update(ctx, set)).NotTo(Succeed()) }) }) + + Describe("reconcile annotation", Ordered, func() { + It("the annotation can be added", func() { + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(set), set)).To(Succeed()) + if set.Annotations == nil { + set.Annotations = map[string]string{} + } + set.Annotations[v2.ReconcileAnnotation] = strconv.FormatBool(true) + Expect(k8sClient.Update(ctx, set)).To(Succeed()) + }) + + It("the annotation was cleaned up again", func() { + Eventually(func() bool { + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(set), set)).To(Succeed()) + _, present := set.Annotations[v2.ReconcileAnnotation] + return present + }, "50ms").Should(BeFalse()) + }) + }) }) diff --git a/integration/integration_test.go b/integration/integration_test.go index 8117e32..1a6637a 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -830,6 +830,10 @@ var _ = Context("integration test", Ordered, func() { }) Expect(k8sClient.Delete(ctx, deployment())).To(Succeed()) + + _ = testcommon.WaitForResourceAmount(k8sClient, ctx, namespaceName, 0, &v2.FirewallDeploymentList{}, func(l *v2.FirewallDeploymentList) []*v2.FirewallDeployment { + return l.GetItems() + }, 10*time.Second) }) }) @@ -1537,6 +1541,10 @@ var _ = Context("integration test", Ordered, func() { }) Expect(k8sClient.Delete(ctx, deployment())).To(Succeed()) + + _ = testcommon.WaitForResourceAmount(k8sClient, ctx, namespaceName, 0, &v2.FirewallDeploymentList{}, func(l *v2.FirewallDeploymentList) []*v2.FirewallDeployment { + return l.GetItems() + }, 10*time.Second) }) }) @@ -1797,7 +1805,7 @@ var _ = Context("integration test", Ordered, func() { It("should have the progress condition true", func() { cond := testcommon.WaitForCondition(k8sClient, ctx, deployment(), func(fd *v2.FirewallDeployment) v2.Conditions { return fd.Status.Conditions - }, v2.FirewallDeplomentProgressing, v2.ConditionTrue, 15*time.Second) + }, v2.FirewallDeplomentProgressing, v2.ConditionTrue, 30*time.Second) Expect(cond.LastTransitionTime).NotTo(BeZero()) Expect(cond.LastUpdateTime).NotTo(BeZero()) @@ -1841,6 +1849,10 @@ var _ = Context("integration test", Ordered, func() { }) Expect(k8sClient.Delete(ctx, deployment())).To(Succeed()) + + _ = testcommon.WaitForResourceAmount(k8sClient, ctx, namespaceName, 0, &v2.FirewallDeploymentList{}, func(l *v2.FirewallDeploymentList) []*v2.FirewallDeployment { + return l.GetItems() + }, 10*time.Second) }) })