From d57201ccb7c316a797fa1c0776f02c5c2aefff66 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 6 Sep 2024 10:46:10 +0200 Subject: [PATCH 01/12] Update gardener infrastructure egress cidrs for ACL extension. --- controllers/set/infrastructure_status.go | 95 +++++++++ controllers/set/infrastructure_status_test.go | 190 ++++++++++++++++++ controllers/set/reconcile.go | 5 + 3 files changed, 290 insertions(+) create mode 100644 controllers/set/infrastructure_status.go create mode 100644 controllers/set/infrastructure_status_test.go diff --git a/controllers/set/infrastructure_status.go b/controllers/set/infrastructure_status.go new file mode 100644 index 0000000..ef848a6 --- /dev/null +++ b/controllers/set/infrastructure_status.go @@ -0,0 +1,95 @@ +package set + +import ( + "encoding/json" + "fmt" + "net/netip" + "strings" + + v2 "github.com/metal-stack/firewall-controller-manager/api/v2" + "github.com/metal-stack/firewall-controller-manager/controllers" + "github.com/metal-stack/metal-lib/pkg/pointer" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallSet], ownedFirewalls []*v2.Firewall) error { + infrastructureName, ok := extractInfrastructureNameFromSeedNamespace(c.c.GetSeedNamespace()) + if !ok { + return nil + } + + infraObj := &unstructured.Unstructured{} + + infraObj.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "extensions.gardener.cloud", + Kind: "Infrastructure", + Version: "v1alpha1", + }) + + err := c.c.GetSeedClient().Get(r.Ctx, client.ObjectKey{ + Namespace: c.c.GetSeedNamespace(), + Name: infrastructureName, + }, infraObj) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + + var egressCIDRs []string + + for _, fw := range ownedFirewalls { + for _, network := range fw.Status.FirewallNetworks { + if pointer.SafeDeref(network.NetworkType) != "external" { + continue + } + + for _, ip := range network.IPs { + parsed, err := netip.ParseAddr(ip) + if err != nil { + continue + } + + egressCIDRs = append(egressCIDRs, fmt.Sprintf("%s/%d", ip, parsed.BitLen())) + } + } + } + + infraStatusPatch := map[string]any{ + "status": map[string]any{ + "egressCIDRs": egressCIDRs, + }, + } + + jsonPatch, err := json.Marshal(infraStatusPatch) + if err != nil { + return fmt.Errorf("unable to marshal infrastructure status patch: %w", err) + } + + err = c.c.GetSeedClient().Status().Patch(r.Ctx, infraObj, client.RawPatch(types.MergePatchType, jsonPatch)) + if err != nil { + return fmt.Errorf("error patching infrastructure status egress cidrs field: %w", err) + } + + c.log.Info("found gardener infrastructure resource and patched egress cidrs for acl extension", "infrastructure-name", infraObj.GetName(), "egress-cidrs", strings.Join(egressCIDRs, ",")) + + return nil +} + +func extractInfrastructureNameFromSeedNamespace(namespace string) (string, bool) { + if !strings.HasPrefix(namespace, "shoot--") { + return "", false + } + + parts := strings.Split(namespace, "--") + if len(parts) < 3 { + return "", false + } + + return strings.Join(parts[2:], "--"), true +} diff --git a/controllers/set/infrastructure_status_test.go b/controllers/set/infrastructure_status_test.go new file mode 100644 index 0000000..6e489cd --- /dev/null +++ b/controllers/set/infrastructure_status_test.go @@ -0,0 +1,190 @@ +package set + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" + v2 "github.com/metal-stack/firewall-controller-manager/api/v2" + "github.com/metal-stack/firewall-controller-manager/api/v2/config" + "github.com/metal-stack/firewall-controller-manager/controllers" + "github.com/metal-stack/metal-lib/pkg/pointer" + "github.com/metal-stack/metal-lib/pkg/testcommon" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/yaml" +) + +func Test_controller_updateInfrastructureStatus(t *testing.T) { + scheme := runtime.NewScheme() + ctx := context.Background() + log := logr.Logger{} + + testNamespace := "shoot--abcdef--mycluster1" + rawInfra := ` +apiVersion: extensions.gardener.cloud/v1alpha1 +kind: Infrastructure +metadata: + name: mycluster1 + namespace: shoot--abcdef--mycluster1 +spec: + providerConfig: + apiVersion: metal.provider.extensions.gardener.cloud/v1alpha1 + firewall: + controllerVersion: auto +status: + phase: "foo" +` + + var testInfraMapObj map[string]any + err := yaml.Unmarshal([]byte(rawInfra), &testInfraMapObj) + require.NoError(t, err) + + tests := []struct { + name string + objs []client.Object + ownedFirewalls []*v2.Firewall + want client.Object + wantErr error + }{ + { + name: "no infrastructure present", + objs: nil, + wantErr: nil, + }, + { + name: "infrastructure is present", + objs: []client.Object{ + &unstructured.Unstructured{ + Object: testInfraMapObj, + }, + }, + ownedFirewalls: []*v2.Firewall{ + { + Status: v2.FirewallStatus{ + FirewallNetworks: []v2.FirewallNetwork{ + { + NetworkType: pointer.Pointer("external"), + IPs: []string{"1.1.1.1"}, + }, + { + NetworkType: pointer.Pointer("underlay"), + IPs: []string{"10.8.0.4"}, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "1000", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, + }, + }, + wantErr: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objs...).WithStatusSubresource(tt.objs...).Build() + + cc, err := config.New(&config.NewControllerConfig{ + SeedClient: c, + SeedNamespace: testNamespace, + SkipValidation: true, + }) + require.NoError(t, err) + + ctrl := &controller{ + log: log, + c: cc, + } + + err = ctrl.updateInfrastructureStatus(&controllers.Ctx[*v2.FirewallSet]{ + Ctx: ctx, + Log: log, + }, tt.ownedFirewalls) + if diff := cmp.Diff(tt.wantErr, err, testcommon.ErrorStringComparer()); diff != "" { + t.Errorf("error diff (+got -want):\n %s", diff) + } + + if tt.want != nil { + u := unstructured.Unstructured{} + u.SetGroupVersionKind(tt.want.GetObjectKind().GroupVersionKind()) + + err = c.Get(ctx, client.ObjectKeyFromObject(tt.want), &u) + require.NoError(t, err) + + if diff := cmp.Diff(tt.want, &u); diff != "" { + t.Errorf("diff (+got -want):\n %s", diff) + } + } + }) + } +} + +func Test_extractInfrastructureNameFromSeedNamespace(t *testing.T) { + tests := []struct { + name string + namespace string + want string + wantBool bool + }{ + { + name: "default namespace not considered", + namespace: "default", + want: "", + wantBool: false, + }, + { + name: "correctly extract from gardener namespace scheme", + namespace: "shoot--abcdef--mycluster1", + want: "mycluster1", + wantBool: true, + }, + { + name: "incorrect namespace scheme #1", + namespace: "shoot--abcdef", + want: "", + wantBool: false, + }, + { + name: "another double-hyphen in cluster name", + namespace: "shoot--abcdef--myclust--er1", + want: "myclust--er1", + wantBool: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, gotBool := extractInfrastructureNameFromSeedNamespace(tt.namespace) + if diff := cmp.Diff(got, tt.want); diff != "" { + t.Errorf("diff (+got -want):\n %s", diff) + } + if diff := cmp.Diff(gotBool, tt.wantBool); diff != "" { + t.Errorf("diff (+got -want):\n %s", diff) + } + }) + } +} diff --git a/controllers/set/reconcile.go b/controllers/set/reconcile.go index 6591ea3..00e359a 100644 --- a/controllers/set/reconcile.go +++ b/controllers/set/reconcile.go @@ -121,6 +121,11 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error { return err } + err = c.updateInfrastructureStatus(r, ownedFirewalls) + if err != nil { + return err + } + return nil } From 63f5e2479540f581c8f610ece96cf75f963ba460 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 6 Sep 2024 10:53:17 +0200 Subject: [PATCH 02/12] Test with CIDRs already set. --- controllers/set/infrastructure_status_test.go | 129 ++++++++++++++---- 1 file changed, 103 insertions(+), 26 deletions(-) diff --git a/controllers/set/infrastructure_status_test.go b/controllers/set/infrastructure_status_test.go index 6e489cd..38aef99 100644 --- a/controllers/set/infrastructure_status_test.go +++ b/controllers/set/infrastructure_status_test.go @@ -25,44 +25,121 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { log := logr.Logger{} testNamespace := "shoot--abcdef--mycluster1" - rawInfra := ` -apiVersion: extensions.gardener.cloud/v1alpha1 -kind: Infrastructure -metadata: - name: mycluster1 - namespace: shoot--abcdef--mycluster1 -spec: - providerConfig: - apiVersion: metal.provider.extensions.gardener.cloud/v1alpha1 - firewall: - controllerVersion: auto -status: - phase: "foo" -` - - var testInfraMapObj map[string]any - err := yaml.Unmarshal([]byte(rawInfra), &testInfraMapObj) - require.NoError(t, err) tests := []struct { name string - objs []client.Object + objs func() []client.Object ownedFirewalls []*v2.Firewall want client.Object wantErr error }{ { - name: "no infrastructure present", - objs: nil, + name: "no infrastructure present", + objs: func() []client.Object { + return nil + }, wantErr: nil, }, { - name: "infrastructure is present", - objs: []client.Object{ - &unstructured.Unstructured{ - Object: testInfraMapObj, + name: "infrastructure is present, egress cidrs were not yet set", + objs: func() []client.Object { + rawInfra := ` + apiVersion: extensions.gardener.cloud/v1alpha1 + kind: Infrastructure + metadata: + name: mycluster1 + namespace: shoot--abcdef--mycluster1 + spec: + providerConfig: + apiVersion: metal.provider.extensions.gardener.cloud/v1alpha1 + firewall: + controllerVersion: auto + status: + phase: "foo" + ` + + var testInfraMapObj map[string]any + err := yaml.Unmarshal([]byte(rawInfra), &testInfraMapObj) + require.NoError(t, err) + + return []client.Object{ + &unstructured.Unstructured{ + Object: testInfraMapObj, + }, + } + }, + ownedFirewalls: []*v2.Firewall{ + { + Status: v2.FirewallStatus{ + FirewallNetworks: []v2.FirewallNetwork{ + { + NetworkType: pointer.Pointer("external"), + IPs: []string{"1.1.1.1"}, + }, + { + NetworkType: pointer.Pointer("underlay"), + IPs: []string{"10.8.0.4"}, + }, + }, + }, }, }, + want: &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "1000", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, + }, + }, + wantErr: nil, + }, + { + name: "infrastructure is present, egress cidrs have already been set", + objs: func() []client.Object { + rawInfra := ` + apiVersion: extensions.gardener.cloud/v1alpha1 + kind: Infrastructure + metadata: + name: mycluster1 + namespace: shoot--abcdef--mycluster1 + spec: + providerConfig: + apiVersion: metal.provider.extensions.gardener.cloud/v1alpha1 + firewall: + controllerVersion: auto + status: + phase: "foo" + egressCIDRs: + - 5.6.7.8/32 + - 1.2.3.4/32 + ` + + var testInfraMapObj map[string]any + err := yaml.Unmarshal([]byte(rawInfra), &testInfraMapObj) + require.NoError(t, err) + + return []client.Object{ + &unstructured.Unstructured{ + Object: testInfraMapObj, + }, + } + }, ownedFirewalls: []*v2.Firewall{ { Status: v2.FirewallStatus{ @@ -107,7 +184,7 @@ status: } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objs...).WithStatusSubresource(tt.objs...).Build() + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objs()...).WithStatusSubresource(tt.objs()...).Build() cc, err := config.New(&config.NewControllerConfig{ SeedClient: c, From fb333fff00fe035c1ee08dbe55c03bb06da1e5b9 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 6 Sep 2024 10:59:36 +0200 Subject: [PATCH 03/12] Improve test a bit. --- controllers/set/infrastructure_status_test.go | 85 +++++++++---------- 1 file changed, 41 insertions(+), 44 deletions(-) diff --git a/controllers/set/infrastructure_status_test.go b/controllers/set/infrastructure_status_test.go index 38aef99..260cdc6 100644 --- a/controllers/set/infrastructure_status_test.go +++ b/controllers/set/infrastructure_status_test.go @@ -16,7 +16,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/yaml" ) func Test_controller_updateInfrastructureStatus(t *testing.T) { @@ -43,28 +42,28 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { { name: "infrastructure is present, egress cidrs were not yet set", objs: func() []client.Object { - rawInfra := ` - apiVersion: extensions.gardener.cloud/v1alpha1 - kind: Infrastructure - metadata: - name: mycluster1 - namespace: shoot--abcdef--mycluster1 - spec: - providerConfig: - apiVersion: metal.provider.extensions.gardener.cloud/v1alpha1 - firewall: - controllerVersion: auto - status: - phase: "foo" - ` - - var testInfraMapObj map[string]any - err := yaml.Unmarshal([]byte(rawInfra), &testInfraMapObj) - require.NoError(t, err) - return []client.Object{ &unstructured.Unstructured{ - Object: testInfraMapObj, + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + }, + }, }, } }, @@ -112,31 +111,29 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { { name: "infrastructure is present, egress cidrs have already been set", objs: func() []client.Object { - rawInfra := ` - apiVersion: extensions.gardener.cloud/v1alpha1 - kind: Infrastructure - metadata: - name: mycluster1 - namespace: shoot--abcdef--mycluster1 - spec: - providerConfig: - apiVersion: metal.provider.extensions.gardener.cloud/v1alpha1 - firewall: - controllerVersion: auto - status: - phase: "foo" - egressCIDRs: - - 5.6.7.8/32 - - 1.2.3.4/32 - ` - - var testInfraMapObj map[string]any - err := yaml.Unmarshal([]byte(rawInfra), &testInfraMapObj) - require.NoError(t, err) - return []client.Object{ &unstructured.Unstructured{ - Object: testInfraMapObj, + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.2.3.4/32", "2.3.4.5/32"}, + }, + }, }, } }, From c62590034041cd398d0ebee3be3626f23ff2d67d Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 6 Sep 2024 11:20:54 +0200 Subject: [PATCH 04/12] Skip update if already up-to-date. --- controllers/set/infrastructure_status.go | 15 +++- controllers/set/infrastructure_status_test.go | 76 ++++++++++++++++++- 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/controllers/set/infrastructure_status.go b/controllers/set/infrastructure_status.go index ef848a6..1e493ec 100644 --- a/controllers/set/infrastructure_status.go +++ b/controllers/set/infrastructure_status.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/netip" + "slices" "strings" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" @@ -41,7 +42,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallS return err } - var egressCIDRs []string + var egressCIDRs []any for _, fw := range ownedFirewalls { for _, network := range fw.Status.FirewallNetworks { @@ -60,6 +61,16 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallS } } + // check if an update is required or not + if currentStatus, ok := infraObj.Object["status"].(map[string]any); ok { + if currentCIDRs, ok := currentStatus["egressCIDRs"].([]any); ok { + if slices.Equal(egressCIDRs, currentCIDRs) { + c.log.Info("found gardener infrastructure resource, egress cidrs already up-to-date", "infrastructure-name", infraObj.GetName(), "egress-cidrs", egressCIDRs) + return nil + } + } + } + infraStatusPatch := map[string]any{ "status": map[string]any{ "egressCIDRs": egressCIDRs, @@ -76,7 +87,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallS return fmt.Errorf("error patching infrastructure status egress cidrs field: %w", err) } - c.log.Info("found gardener infrastructure resource and patched egress cidrs for acl extension", "infrastructure-name", infraObj.GetName(), "egress-cidrs", strings.Join(egressCIDRs, ",")) + c.log.Info("found gardener infrastructure resource and patched egress cidrs for acl extension", "infrastructure-name", infraObj.GetName(), "egress-cidrs", egressCIDRs) return nil } diff --git a/controllers/set/infrastructure_status_test.go b/controllers/set/infrastructure_status_test.go index 260cdc6..081acbc 100644 --- a/controllers/set/infrastructure_status_test.go +++ b/controllers/set/infrastructure_status_test.go @@ -4,7 +4,7 @@ import ( "context" "testing" - "github.com/go-logr/logr" + "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/config" @@ -21,7 +21,7 @@ import ( func Test_controller_updateInfrastructureStatus(t *testing.T) { scheme := runtime.NewScheme() ctx := context.Background() - log := logr.Logger{} + log := testr.New(t) testNamespace := "shoot--abcdef--mycluster1" @@ -109,7 +109,7 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { wantErr: nil, }, { - name: "infrastructure is present, egress cidrs have already been set", + name: "infrastructure is present, egress cidrs have already been set but not up-to-date", objs: func() []client.Object { return []client.Object{ &unstructured.Unstructured{ @@ -178,6 +178,76 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, wantErr: nil, }, + { + name: "infrastructure is present, egress cidrs have already been set and are up-to-date", + objs: func() []client.Object { + return []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, + }, + }, + } + }, + ownedFirewalls: []*v2.Firewall{ + { + Status: v2.FirewallStatus{ + FirewallNetworks: []v2.FirewallNetwork{ + { + NetworkType: pointer.Pointer("external"), + IPs: []string{"1.1.1.1"}, + }, + { + NetworkType: pointer.Pointer("underlay"), + IPs: []string{"10.8.0.4"}, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, + }, + }, + wantErr: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From ef22b3574d3efb70509e5394fb8d23477c955f10 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Fri, 6 Sep 2024 11:55:14 +0200 Subject: [PATCH 05/12] Include egress rules. --- controllers/set/infrastructure_status.go | 46 +++++++++-- controllers/set/infrastructure_status_test.go | 80 +++++++++++++++++++ 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/controllers/set/infrastructure_status.go b/controllers/set/infrastructure_status.go index 1e493ec..d32e35d 100644 --- a/controllers/set/infrastructure_status.go +++ b/controllers/set/infrastructure_status.go @@ -61,16 +61,50 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallS } } - // check if an update is required or not - if currentStatus, ok := infraObj.Object["status"].(map[string]any); ok { - if currentCIDRs, ok := currentStatus["egressCIDRs"].([]any); ok { - if slices.Equal(egressCIDRs, currentCIDRs) { - c.log.Info("found gardener infrastructure resource, egress cidrs already up-to-date", "infrastructure-name", infraObj.GetName(), "egress-cidrs", egressCIDRs) - return nil + type infrastructure struct { + Spec struct { + ProviderConfig struct { + Firewall struct { + EgressRules []struct { + IPs []string `json:"ips"` + } `json:"egressRules"` + } `json:"firewall"` + } `json:"providerConfig"` + } `json:"spec"` + Status struct { + EgressCIDRs []any `json:"egressCIDRs"` + } `json:"status"` + } + + infraRaw, err := json.Marshal(infraObj) + if err != nil { + return fmt.Errorf("unable to convert gardener infrastructure object: %w", err) + } + + var typedInfra infrastructure + err = json.Unmarshal(infraRaw, &typedInfra) + if err != nil { + return fmt.Errorf("unable to convert gardener infrastructure object: %w", err) + } + + // add egress rules cidrs + for _, rule := range typedInfra.Spec.ProviderConfig.Firewall.EgressRules { + for _, ip := range rule.IPs { + parsed, err := netip.ParseAddr(ip) + if err != nil { + continue } + + egressCIDRs = append(egressCIDRs, fmt.Sprintf("%s/%d", ip, parsed.BitLen())) } } + // check if an update is required or not + if slices.Equal(egressCIDRs, typedInfra.Status.EgressCIDRs) { + c.log.Info("found gardener infrastructure resource, egress cidrs already up-to-date", "infrastructure-name", infraObj.GetName(), "egress-cidrs", egressCIDRs) + return nil + } + infraStatusPatch := map[string]any{ "status": map[string]any{ "egressCIDRs": egressCIDRs, diff --git a/controllers/set/infrastructure_status_test.go b/controllers/set/infrastructure_status_test.go index 081acbc..d1b4c3e 100644 --- a/controllers/set/infrastructure_status_test.go +++ b/controllers/set/infrastructure_status_test.go @@ -248,6 +248,86 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, wantErr: nil, }, + { + name: "infrastructure has egress rules", + objs: func() []client.Object { + return []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + "egressRules": []any{ + map[string]any{ + "ips": []any{"3.4.5.6"}, + }, + }, + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, + }, + }, + } + }, + ownedFirewalls: []*v2.Firewall{ + { + Status: v2.FirewallStatus{ + FirewallNetworks: []v2.FirewallNetwork{ + { + NetworkType: pointer.Pointer("external"), + IPs: []string{"1.1.1.1"}, + }, + { + NetworkType: pointer.Pointer("underlay"), + IPs: []string{"10.8.0.4"}, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "1000", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + "egressRules": []any{ + map[string]any{ + "ips": []any{"3.4.5.6"}, + }, + }, + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32", "3.4.5.6/32"}, + }, + }, + }, + wantErr: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 920033ec1590617f28abbb24bdc1d7bb48b78ae6 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 10:53:16 +0200 Subject: [PATCH 06/12] Sort CIDRs. --- controllers/set/infrastructure_status.go | 15 ++++ controllers/set/infrastructure_status_test.go | 84 +++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/controllers/set/infrastructure_status.go b/controllers/set/infrastructure_status.go index d32e35d..8cda677 100644 --- a/controllers/set/infrastructure_status.go +++ b/controllers/set/infrastructure_status.go @@ -5,6 +5,7 @@ import ( "fmt" "net/netip" "slices" + "sort" "strings" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" @@ -99,6 +100,9 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallS } } + sortUntypedStringSlice(egressCIDRs) + sortUntypedStringSlice(typedInfra.Status.EgressCIDRs) + // check if an update is required or not if slices.Equal(egressCIDRs, typedInfra.Status.EgressCIDRs) { c.log.Info("found gardener infrastructure resource, egress cidrs already up-to-date", "infrastructure-name", infraObj.GetName(), "egress-cidrs", egressCIDRs) @@ -138,3 +142,14 @@ func extractInfrastructureNameFromSeedNamespace(namespace string) (string, bool) return strings.Join(parts[2:], "--"), true } + +func sortUntypedStringSlice(s []any) { + sort.Slice(s, func(i, j int) bool { + a, aok := s[i].(string) + b, bok := s[j].(string) + if aok && bok { + return a < b + } + return false + }) +} diff --git a/controllers/set/infrastructure_status_test.go b/controllers/set/infrastructure_status_test.go index d1b4c3e..492eb1f 100644 --- a/controllers/set/infrastructure_status_test.go +++ b/controllers/set/infrastructure_status_test.go @@ -328,6 +328,90 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, wantErr: nil, }, + { + name: "skip update on different order of ip elements in slice", + objs: func() []client.Object { + return []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.2/32", "1.1.1.1/32"}, + }, + }, + }, + } + }, + ownedFirewalls: []*v2.Firewall{ + { + Status: v2.FirewallStatus{ + FirewallNetworks: []v2.FirewallNetwork{ + { + NetworkType: pointer.Pointer("external"), + IPs: []string{"1.1.1.1"}, + }, + { + NetworkType: pointer.Pointer("underlay"), + IPs: []string{"10.8.0.4"}, + }, + }, + }, + }, + { + Status: v2.FirewallStatus{ + FirewallNetworks: []v2.FirewallNetwork{ + { + NetworkType: pointer.Pointer("external"), + IPs: []string{"1.1.1.2"}, + }, + { + NetworkType: pointer.Pointer("underlay"), + IPs: []string{"10.8.0.5"}, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, + }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.2/32", "1.1.1.1/32"}, + }, + }, + }, + wantErr: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 8eca522fdef2a6f822a5fff1dcd67212dc10b821 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 13:26:19 +0200 Subject: [PATCH 07/12] Move logic to firewall deployment controller. Cannot be done on set-level because the sets will toggle each other's status. --- .../infrastructure_status.go | 9 +--- .../infrastructure_status_test.go | 6 +-- controllers/deployment/reconcile.go | 42 ++++++++++++++++--- controllers/deployment/status.go | 11 +---- controllers/set/reconcile.go | 5 --- 5 files changed, 42 insertions(+), 31 deletions(-) rename controllers/{set => deployment}/infrastructure_status.go (95%) rename controllers/{set => deployment}/infrastructure_status_test.go (99%) diff --git a/controllers/set/infrastructure_status.go b/controllers/deployment/infrastructure_status.go similarity index 95% rename from controllers/set/infrastructure_status.go rename to controllers/deployment/infrastructure_status.go index 8cda677..32a553c 100644 --- a/controllers/set/infrastructure_status.go +++ b/controllers/deployment/infrastructure_status.go @@ -1,4 +1,4 @@ -package set +package deployment import ( "encoding/json" @@ -18,12 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallSet], ownedFirewalls []*v2.Firewall) error { - infrastructureName, ok := extractInfrastructureNameFromSeedNamespace(c.c.GetSeedNamespace()) - if !ok { - return nil - } - +func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallDeployment], infrastructureName string, ownedFirewalls []*v2.Firewall) error { infraObj := &unstructured.Unstructured{} infraObj.SetGroupVersionKind(schema.GroupVersionKind{ diff --git a/controllers/set/infrastructure_status_test.go b/controllers/deployment/infrastructure_status_test.go similarity index 99% rename from controllers/set/infrastructure_status_test.go rename to controllers/deployment/infrastructure_status_test.go index 492eb1f..578eb6d 100644 --- a/controllers/set/infrastructure_status_test.go +++ b/controllers/deployment/infrastructure_status_test.go @@ -1,4 +1,4 @@ -package set +package deployment import ( "context" @@ -429,10 +429,10 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { c: cc, } - err = ctrl.updateInfrastructureStatus(&controllers.Ctx[*v2.FirewallSet]{ + err = ctrl.updateInfrastructureStatus(&controllers.Ctx[*v2.FirewallDeployment]{ Ctx: ctx, Log: log, - }, tt.ownedFirewalls) + }, "mycluster1", tt.ownedFirewalls) if diff := cmp.Diff(tt.wantErr, err, testcommon.ErrorStringComparer()); diff != "" { t.Errorf("error diff (+got -want):\n %s", diff) } diff --git a/controllers/deployment/reconcile.go b/controllers/deployment/reconcile.go index c427791..2b5625f 100644 --- a/controllers/deployment/reconcile.go +++ b/controllers/deployment/reconcile.go @@ -43,22 +43,32 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error return nil } + var reconcileErr error switch s := r.Target.Spec.Strategy; s { case v2.StrategyRecreate: - err = c.recreateStrategy(r, ownedSets, latestSet) + reconcileErr = c.recreateStrategy(r, ownedSets, latestSet) case v2.StrategyRollingUpdate: - err = c.rollingUpdateStrategy(r, ownedSets, latestSet) + reconcileErr = c.rollingUpdateStrategy(r, ownedSets, latestSet) default: - err = fmt.Errorf("unknown deployment strategy: %s", s) + reconcileErr = fmt.Errorf("unknown deployment strategy: %s", s) } - statusErr := c.setStatus(r) + // refetch sets after possible creation + // we want to update the set status before a possible return due to reconciliation having failed + ownedSets, _, err = controllers.GetOwnedResources(r.Ctx, c.c.GetSeedClient(), nil, r.Target, &v2.FirewallSetList{}, func(fsl *v2.FirewallSetList) []*v2.FirewallSet { + return fsl.GetItems() + }) + if err != nil { + return fmt.Errorf("unable to get owned sets: %w", err) + } + err = c.setStatus(r, ownedSets) if err != nil { return err } - if statusErr != nil { - return err + + if reconcileErr != nil { + return reconcileErr } // we are done with the update, give the set the shortest distance if this is not already the case @@ -73,6 +83,26 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallDeployment]) error r.Log.Info("swapped latest set to shortest distance", "distance", v2.FirewallShortestDistance) } + infrastructureName, ok := extractInfrastructureNameFromSeedNamespace(c.c.GetSeedNamespace()) + if ok { + var ownedFirewalls []*v2.Firewall + for _, set := range ownedSets { + fws, _, err := controllers.GetOwnedResources(r.Ctx, c.c.GetSeedClient(), nil, set, &v2.FirewallList{}, func(fl *v2.FirewallList) []*v2.Firewall { + return fl.GetItems() + }) + if err != nil { + return fmt.Errorf("unable to get owned firewalls: %w", err) + } + + ownedFirewalls = append(ownedFirewalls, fws...) + } + + err = c.updateInfrastructureStatus(r, infrastructureName, ownedFirewalls) + if err != nil { + return err + } + } + return nil } diff --git a/controllers/deployment/status.go b/controllers/deployment/status.go index 02b78eb..abe1999 100644 --- a/controllers/deployment/status.go +++ b/controllers/deployment/status.go @@ -1,20 +1,11 @@ package deployment import ( - "fmt" - v2 "github.com/metal-stack/firewall-controller-manager/api/v2" "github.com/metal-stack/firewall-controller-manager/controllers" ) -func (c *controller) setStatus(r *controllers.Ctx[*v2.FirewallDeployment]) error { - ownedSets, _, err := controllers.GetOwnedResources(r.Ctx, c.c.GetSeedClient(), nil, r.Target, &v2.FirewallSetList{}, func(fsl *v2.FirewallSetList) []*v2.FirewallSet { - return fsl.GetItems() - }) - if err != nil { - return fmt.Errorf("unable to get owned sets: %w", err) - } - +func (c *controller) setStatus(r *controllers.Ctx[*v2.FirewallDeployment], ownedSets []*v2.FirewallSet) error { latestSet, err := controllers.MaxRevisionOf(ownedSets) if err != nil { return err diff --git a/controllers/set/reconcile.go b/controllers/set/reconcile.go index 00e359a..6591ea3 100644 --- a/controllers/set/reconcile.go +++ b/controllers/set/reconcile.go @@ -121,11 +121,6 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error { return err } - err = c.updateInfrastructureStatus(r, ownedFirewalls) - if err != nil { - return err - } - return nil } From e7f2708538e554892dfb6f326011a8090247d714 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 13:28:21 +0200 Subject: [PATCH 08/12] Small refactor. --- .../deployment/infrastructure_status.go | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/controllers/deployment/infrastructure_status.go b/controllers/deployment/infrastructure_status.go index 32a553c..7500aa4 100644 --- a/controllers/deployment/infrastructure_status.go +++ b/controllers/deployment/infrastructure_status.go @@ -38,25 +38,6 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD return err } - var egressCIDRs []any - - for _, fw := range ownedFirewalls { - for _, network := range fw.Status.FirewallNetworks { - if pointer.SafeDeref(network.NetworkType) != "external" { - continue - } - - for _, ip := range network.IPs { - parsed, err := netip.ParseAddr(ip) - if err != nil { - continue - } - - egressCIDRs = append(egressCIDRs, fmt.Sprintf("%s/%d", ip, parsed.BitLen())) - } - } - } - type infrastructure struct { Spec struct { ProviderConfig struct { @@ -83,7 +64,25 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD return fmt.Errorf("unable to convert gardener infrastructure object: %w", err) } - // add egress rules cidrs + var egressCIDRs []any + + for _, fw := range ownedFirewalls { + for _, network := range fw.Status.FirewallNetworks { + if pointer.SafeDeref(network.NetworkType) != "external" { + continue + } + + for _, ip := range network.IPs { + parsed, err := netip.ParseAddr(ip) + if err != nil { + continue + } + + egressCIDRs = append(egressCIDRs, fmt.Sprintf("%s/%d", ip, parsed.BitLen())) + } + } + } + for _, rule := range typedInfra.Spec.ProviderConfig.Firewall.EgressRules { for _, ip := range rule.IPs { parsed, err := netip.ParseAddr(ip) From 57b7c0868244e6b5956c50f7706eb6923cb64151 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 14:07:13 +0200 Subject: [PATCH 09/12] Also trigger ACL extension reconcile. --- .../deployment/infrastructure_status.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/controllers/deployment/infrastructure_status.go b/controllers/deployment/infrastructure_status.go index 7500aa4..c71662c 100644 --- a/controllers/deployment/infrastructure_status.go +++ b/controllers/deployment/infrastructure_status.go @@ -121,6 +121,30 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD c.log.Info("found gardener infrastructure resource and patched egress cidrs for acl extension", "infrastructure-name", infraObj.GetName(), "egress-cidrs", egressCIDRs) + aclObj := &unstructured.Unstructured{} + + aclObj.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "extensions.gardener.cloud", + Kind: "Extension", + Version: "v1alpha1", + }) + + err = c.c.GetSeedClient().Get(r.Ctx, client.ObjectKey{ + Namespace: c.c.GetSeedNamespace(), + Name: "acl", + }, infraObj) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + + err = v2.AddAnnotation(r.Ctx, c.c.GetSeedClient(), aclObj, "gardener.cloud/operation", "reconcile") + if err != nil { + return fmt.Errorf("error annotating acl extension with reconcile operation: %w", err) + } + return nil } From d14d7b24a32b13437c692085a973cc9d48d620ae Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 14:22:27 +0200 Subject: [PATCH 10/12] Fix. --- .../deployment/infrastructure_status.go | 4 +- .../deployment/infrastructure_status_test.go | 232 ++++++++++-------- 2 files changed, 137 insertions(+), 99 deletions(-) diff --git a/controllers/deployment/infrastructure_status.go b/controllers/deployment/infrastructure_status.go index c71662c..14bc713 100644 --- a/controllers/deployment/infrastructure_status.go +++ b/controllers/deployment/infrastructure_status.go @@ -132,7 +132,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD err = c.c.GetSeedClient().Get(r.Ctx, client.ObjectKey{ Namespace: c.c.GetSeedNamespace(), Name: "acl", - }, infraObj) + }, aclObj) if err != nil { if apierrors.IsNotFound(err) { return nil @@ -145,6 +145,8 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD return fmt.Errorf("error annotating acl extension with reconcile operation: %w", err) } + c.log.Info("added reconcile annotation to gardener acl extension object") + return nil } diff --git a/controllers/deployment/infrastructure_status_test.go b/controllers/deployment/infrastructure_status_test.go index 578eb6d..3cb8cea 100644 --- a/controllers/deployment/infrastructure_status_test.go +++ b/controllers/deployment/infrastructure_status_test.go @@ -29,7 +29,7 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { name string objs func() []client.Object ownedFirewalls []*v2.Firewall - want client.Object + want []client.Object wantErr error }{ { @@ -40,7 +40,7 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { wantErr: nil, }, { - name: "infrastructure is present, egress cidrs were not yet set", + name: "infrastructure is present, egress cidrs were not yet set (acl extension gets annotated)", objs: func() []client.Object { return []client.Object{ &unstructured.Unstructured{ @@ -65,6 +65,17 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, }, }, + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Extension", + "metadata": map[string]any{ + "name": "acl", + "namespace": testNamespace, + "resourceVersion": "999", + }, + }, + }, } }, ownedFirewalls: []*v2.Firewall{ @@ -83,26 +94,43 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, }, }, - want: &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "1000", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", + want: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "1000", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, }, }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.1/32"}, + }, + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Extension", + "metadata": map[string]any{ + "name": "acl", + "namespace": testNamespace, + "resourceVersion": "1000", + "annotations": map[string]any{ + "gardener.cloud/operation": "reconcile", + }, + }, + "status": nil, }, }, }, @@ -153,26 +181,28 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, }, }, - want: &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "1000", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", + want: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "1000", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, }, }, - }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.1/32"}, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, }, }, }, @@ -223,26 +253,28 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, }, }, - want: &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", + want: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, }, }, - }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.1/32"}, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, + }, }, }, }, @@ -298,31 +330,33 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, }, }, - want: &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "1000", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", - "egressRules": []any{ - map[string]any{ - "ips": []any{"3.4.5.6"}, + want: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "1000", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + "egressRules": []any{ + map[string]any{ + "ips": []any{"3.4.5.6"}, + }, }, }, }, }, - }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.1/32", "3.4.5.6/32"}, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32", "3.4.5.6/32"}, + }, }, }, }, @@ -387,26 +421,28 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, }, }, - want: &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", + want: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + }, }, }, - }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.2/32", "1.1.1.1/32"}, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.2/32", "1.1.1.1/32"}, + }, }, }, }, @@ -437,14 +473,14 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { t.Errorf("error diff (+got -want):\n %s", diff) } - if tt.want != nil { + for _, want := range tt.want { u := unstructured.Unstructured{} - u.SetGroupVersionKind(tt.want.GetObjectKind().GroupVersionKind()) + u.SetGroupVersionKind(want.GetObjectKind().GroupVersionKind()) - err = c.Get(ctx, client.ObjectKeyFromObject(tt.want), &u) + err = c.Get(ctx, client.ObjectKeyFromObject(want), &u) require.NoError(t, err) - if diff := cmp.Diff(tt.want, &u); diff != "" { + if diff := cmp.Diff(want, &u); diff != "" { t.Errorf("diff (+got -want):\n %s", diff) } } From d57116341b330069aeaff6d4f82d0cc75c585594 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 15:07:05 +0200 Subject: [PATCH 11/12] Simplify. --- .../deployment/infrastructure_status.go | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/controllers/deployment/infrastructure_status.go b/controllers/deployment/infrastructure_status.go index 14bc713..1caa62b 100644 --- a/controllers/deployment/infrastructure_status.go +++ b/controllers/deployment/infrastructure_status.go @@ -5,7 +5,6 @@ import ( "fmt" "net/netip" "slices" - "sort" "strings" v2 "github.com/metal-stack/firewall-controller-manager/api/v2" @@ -49,7 +48,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD } `json:"providerConfig"` } `json:"spec"` Status struct { - EgressCIDRs []any `json:"egressCIDRs"` + EgressCIDRs []string `json:"egressCIDRs"` } `json:"status"` } @@ -64,7 +63,7 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD return fmt.Errorf("unable to convert gardener infrastructure object: %w", err) } - var egressCIDRs []any + var egressCIDRs []string for _, fw := range ownedFirewalls { for _, network := range fw.Status.FirewallNetworks { @@ -94,8 +93,8 @@ func (c *controller) updateInfrastructureStatus(r *controllers.Ctx[*v2.FirewallD } } - sortUntypedStringSlice(egressCIDRs) - sortUntypedStringSlice(typedInfra.Status.EgressCIDRs) + slices.Sort(egressCIDRs) + slices.Sort(typedInfra.Status.EgressCIDRs) // check if an update is required or not if slices.Equal(egressCIDRs, typedInfra.Status.EgressCIDRs) { @@ -162,14 +161,3 @@ func extractInfrastructureNameFromSeedNamespace(namespace string) (string, bool) return strings.Join(parts[2:], "--"), true } - -func sortUntypedStringSlice(s []any) { - sort.Slice(s, func(i, j int) bool { - a, aok := s[i].(string) - b, bok := s[j].(string) - if aok && bok { - return a < b - } - return false - }) -} From 19fc65b7ddf288f6afc66b771eda994ee611e8ff Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 9 Sep 2024 15:22:51 +0200 Subject: [PATCH 12/12] Simplify. --- .../deployment/infrastructure_status_test.go | 242 +++++++++--------- 1 file changed, 115 insertions(+), 127 deletions(-) diff --git a/controllers/deployment/infrastructure_status_test.go b/controllers/deployment/infrastructure_status_test.go index 3cb8cea..96fa1cb 100644 --- a/controllers/deployment/infrastructure_status_test.go +++ b/controllers/deployment/infrastructure_status_test.go @@ -27,56 +27,52 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { tests := []struct { name string - objs func() []client.Object + objs []client.Object ownedFirewalls []*v2.Firewall want []client.Object wantErr error }{ { - name: "no infrastructure present", - objs: func() []client.Object { - return nil - }, + name: "no infrastructure present", + objs: nil, wantErr: nil, }, { name: "infrastructure is present, egress cidrs were not yet set (acl extension gets annotated)", - objs: func() []client.Object { - return []client.Object{ - &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", - }, + objs: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", }, }, - "status": map[string]any{ - "phase": "foo", - }, + }, + "status": map[string]any{ + "phase": "foo", }, }, - &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Extension", - "metadata": map[string]any{ - "name": "acl", - "namespace": testNamespace, - "resourceVersion": "999", - }, + }, + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Extension", + "metadata": map[string]any{ + "name": "acl", + "namespace": testNamespace, + "resourceVersion": "999", }, }, - } + }, }, ownedFirewalls: []*v2.Firewall{ { @@ -138,32 +134,30 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, { name: "infrastructure is present, egress cidrs have already been set but not up-to-date", - objs: func() []client.Object { - return []client.Object{ - &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", - }, + objs: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", }, }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.2.3.4/32", "2.3.4.5/32"}, - }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.2.3.4/32", "2.3.4.5/32"}, }, }, - } + }, }, ownedFirewalls: []*v2.Firewall{ { @@ -210,32 +204,30 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, { name: "infrastructure is present, egress cidrs have already been set and are up-to-date", - objs: func() []client.Object { - return []client.Object{ - &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", - }, + objs: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", }, }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.1/32"}, - }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, }, }, - } + }, }, ownedFirewalls: []*v2.Firewall{ { @@ -282,37 +274,35 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, { name: "infrastructure has egress rules", - objs: func() []client.Object { - return []client.Object{ - &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", - "egressRules": []any{ - map[string]any{ - "ips": []any{"3.4.5.6"}, - }, + objs: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", + "egressRules": []any{ + map[string]any{ + "ips": []any{"3.4.5.6"}, }, }, }, }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.1/32"}, - }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.1/32"}, }, }, - } + }, }, ownedFirewalls: []*v2.Firewall{ { @@ -364,32 +354,30 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { }, { name: "skip update on different order of ip elements in slice", - objs: func() []client.Object { - return []client.Object{ - &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": "extensions.gardener.cloud/v1alpha1", - "kind": "Infrastructure", - "metadata": map[string]any{ - "name": "mycluster1", - "namespace": testNamespace, - "resourceVersion": "999", - }, - "spec": map[string]any{ - "providerConfig": map[string]any{ - "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", - "firewall": map[string]any{ - "controllerVersion": "auto", - }, + objs: []client.Object{ + &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "extensions.gardener.cloud/v1alpha1", + "kind": "Infrastructure", + "metadata": map[string]any{ + "name": "mycluster1", + "namespace": testNamespace, + "resourceVersion": "999", + }, + "spec": map[string]any{ + "providerConfig": map[string]any{ + "apiVersion": "metal.provider.extensions.gardener.cloud/v1alpha1", + "firewall": map[string]any{ + "controllerVersion": "auto", }, }, - "status": map[string]any{ - "phase": "foo", - "egressCIDRs": []any{"1.1.1.2/32", "1.1.1.1/32"}, - }, + }, + "status": map[string]any{ + "phase": "foo", + "egressCIDRs": []any{"1.1.1.2/32", "1.1.1.1/32"}, }, }, - } + }, }, ownedFirewalls: []*v2.Firewall{ { @@ -451,7 +439,7 @@ func Test_controller_updateInfrastructureStatus(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objs()...).WithStatusSubresource(tt.objs()...).Build() + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objs...).WithStatusSubresource(tt.objs...).Build() cc, err := config.New(&config.NewControllerConfig{ SeedClient: c,