From f70d32b38b9fd5757068a3cddd2c5bd529f9ded8 Mon Sep 17 00:00:00 2001 From: Stefan Majer Date: Mon, 26 Jul 2021 11:32:50 +0200 Subject: [PATCH] Fix for traffic accounting (#96) --- ...l-stack.io_clusterwidenetworkpolicies.yaml | 4 +- .../crd/bases/metal-stack.io_firewalls.yaml | 2 +- controllers/firewall_controller.go | 86 ++++++++++++++++--- pkg/nftables/nftables.tpl | 8 +- pkg/nftables/test_data/more-rules.nftable.v4 | 8 +- pkg/nftables/test_data/simple.nftable.v4 | 8 +- pkg/nftables/test_data/validated.nftable.v4 | 8 +- 7 files changed, 93 insertions(+), 31 deletions(-) diff --git a/config/crd/bases/metal-stack.io_clusterwidenetworkpolicies.yaml b/config/crd/bases/metal-stack.io_clusterwidenetworkpolicies.yaml index 6f79a46e..cfdcf141 100644 --- a/config/crd/bases/metal-stack.io_clusterwidenetworkpolicies.yaml +++ b/config/crd/bases/metal-stack.io_clusterwidenetworkpolicies.yaml @@ -4,7 +4,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.4.0 + controller-gen.kubebuilder.io/version: v0.6.1 creationTimestamp: null name: clusterwidenetworkpolicies.metal-stack.io spec: @@ -74,6 +74,7 @@ spec: numbers. x-kubernetes-int-or-string: true protocol: + default: TCP description: The protocol (TCP, UDP, or SCTP) which traffic must match. If not specified, this field defaults to TCP. @@ -174,6 +175,7 @@ spec: numbers. x-kubernetes-int-or-string: true protocol: + default: TCP description: The protocol (TCP, UDP, or SCTP) which traffic must match. If not specified, this field defaults to TCP. diff --git a/config/crd/bases/metal-stack.io_firewalls.yaml b/config/crd/bases/metal-stack.io_firewalls.yaml index e3935011..b8844dd5 100644 --- a/config/crd/bases/metal-stack.io_firewalls.yaml +++ b/config/crd/bases/metal-stack.io_firewalls.yaml @@ -4,7 +4,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.4.0 + controller-gen.kubebuilder.io/version: v0.6.1 creationTimestamp: null name: firewalls.metal-stack.io spec: diff --git a/controllers/firewall_controller.go b/controllers/firewall_controller.go index 76c829a8..7ed22424 100644 --- a/controllers/firewall_controller.go +++ b/controllers/firewall_controller.go @@ -18,7 +18,9 @@ package controllers import ( "context" + "crypto/md5" //nolint:gosec "crypto/rsa" + "encoding/json" "fmt" "reflect" "time" @@ -40,14 +42,15 @@ import ( "github.com/hashicorp/go-multierror" + mn "github.com/metal-stack/metal-lib/pkg/net" + networking "k8s.io/api/networking/v1" + firewallv1 "github.com/metal-stack/firewall-controller/api/v1" "github.com/metal-stack/firewall-controller/pkg/collector" "github.com/metal-stack/firewall-controller/pkg/network" "github.com/metal-stack/firewall-controller/pkg/nftables" "github.com/metal-stack/firewall-controller/pkg/suricata" "github.com/metal-stack/firewall-controller/pkg/updater" - mn "github.com/metal-stack/metal-lib/pkg/net" - networking "k8s.io/api/networking/v1" ) // FirewallReconciler reconciles a Firewall object @@ -59,6 +62,7 @@ type FirewallReconciler struct { EnableIDS bool EnableSignatureCheck bool CAPubKey *rsa.PublicKey + policySpecsChecksums map[string][16]byte } const ( @@ -123,14 +127,25 @@ func (r *FirewallReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { requeue.RequeueAfter = i } + var cwnps firewallv1.ClusterwideNetworkPolicyList + if err := r.List(ctx, &cwnps, client.InNamespace(f.Namespace)); err != nil { + return done, err + } + changed, err := r.isSpecsChanged(cwnps) + if err != nil { + return done, err + } + var errors *multierror.Error - log.Info("reconciling nftables rules") - if err = r.reconcileRules(ctx, f, log); err != nil { - errors = multierror.Append(errors, err) + if changed { + log.Info("reconciling nftables rules") + if err = r.reconcileRules(ctx, f, cwnps, log); err != nil { + errors = multierror.Append(errors, err) + } } log.Info("reconciling network settings") - changed, err := network.ReconcileNetwork(f, log) + changed, err = network.ReconcileNetwork(f, log) if changed && err == nil { r.recorder.Event(&f, corev1.EventTypeNormal, "Network settings", "reconcilation succeeded (frr.conf)") } else if changed && err != nil { @@ -231,22 +246,24 @@ func convert(np networking.NetworkPolicy) (*firewallv1.ClusterwideNetworkPolicy, } // reconcileRules reconciles the nftable rules for this firewall -func (r *FirewallReconciler) reconcileRules(ctx context.Context, f firewallv1.Firewall, log logr.Logger) error { - var clusterNPs firewallv1.ClusterwideNetworkPolicyList - if err := r.List(ctx, &clusterNPs, client.InNamespace(f.Namespace)); err != nil { - return err - } - +func (r *FirewallReconciler) reconcileRules(ctx context.Context, f firewallv1.Firewall, cwnps firewallv1.ClusterwideNetworkPolicyList, log logr.Logger) error { var services corev1.ServiceList if err := r.List(ctx, &services); err != nil { return err } - nftablesFirewall := nftables.NewFirewall(&clusterNPs, &services, f.Spec, log) + nftablesFirewall := nftables.NewFirewall(&cwnps, &services, f.Spec, log) if err := nftablesFirewall.Reconcile(); err != nil { return err } + // It's assumed that nftablesFirewall.Reconcile can modify CWNPs + for _, cwnp := range cwnps.Items { + if err := r.updateCWNPChecksum(cwnp); err != nil { + return err + } + } + return nil } @@ -445,6 +462,49 @@ func (r *FirewallReconciler) updateStatus(ctx context.Context, f firewallv1.Fire return nil } +func (r *FirewallReconciler) isSpecsChanged(cwnps firewallv1.ClusterwideNetworkPolicyList) (bool, error) { + if r.policySpecsChecksums == nil { + r.policySpecsChecksums = make(map[string][16]byte) + } + + for _, cwnp := range cwnps.Items { + nn := types.NamespacedName{ + Name: cwnp.Name, + Namespace: cwnp.Namespace, + } + oldSum, exists := r.policySpecsChecksums[nn.String()] + if !exists { + return true, nil + } + + j, err := json.Marshal(cwnp.Spec) + if err != nil { + return false, fmt.Errorf("failed to parse '%s' CWNP spec: %w", cwnp.Name, err) + } + + currentSum := md5.Sum(j) //nolint:gosec + if exists && !reflect.DeepEqual(oldSum, currentSum) { + return true, nil + } + } + + return false, nil +} + +func (r *FirewallReconciler) updateCWNPChecksum(cwnp firewallv1.ClusterwideNetworkPolicy) error { + j, err := json.Marshal(cwnp.Spec) + if err != nil { + return fmt.Errorf("failed to parse updated '%s' CWNP spec: %w", cwnp.Name, err) + } + + nn := types.NamespacedName{ + Name: cwnp.Name, + Namespace: cwnp.Namespace, + } + r.policySpecsChecksums[nn.String()] = md5.Sum(j) //nolint:gosec + return nil +} + // SetupWithManager configures this controller to watch for the CRDs in a specific namespace func (r *FirewallReconciler) SetupWithManager(mgr ctrl.Manager) error { r.recorder = mgr.GetEventRecorderFor("FirewallController") diff --git a/pkg/nftables/nftables.tpl b/pkg/nftables/nftables.tpl index 2118b4a4..d393e1cf 100644 --- a/pkg/nftables/nftables.tpl +++ b/pkg/nftables/nftables.tpl @@ -30,12 +30,12 @@ table ip firewall { type filter hook forward priority 1; policy drop; # network traffic accounting for external traffic - ip saddr != @internal_prefixes oifname "vlan{{ .PrivateVrfID }}" counter name external_in comment "count external traffic incomming" - ip daddr != @internal_prefixes iifname "vrf{{ .PrivateVrfID }}" counter name external_out comment "count external traffic outgoing" + ip saddr != @internal_prefixes oifname {"vlan{{ .PrivateVrfID }}", "vrf{{ .PrivateVrfID }}"} counter name external_in comment "count external traffic incomming" + ip daddr != @internal_prefixes iifname {"vlan{{ .PrivateVrfID }}", "vrf{{ .PrivateVrfID }}"} counter name external_out comment "count external traffic outgoing" # network traffic accounting for internal traffic - ip saddr @internal_prefixes oifname "vlan{{ .PrivateVrfID }}" counter name internal_in comment "count internal traffic incomming" - ip daddr @internal_prefixes iifname "vrf{{ .PrivateVrfID }}" counter name internal_out comment "count internal traffic outgoing" + ip saddr @internal_prefixes oifname {"vlan{{ .PrivateVrfID }}", "vrf{{ .PrivateVrfID }}"} counter name internal_in comment "count internal traffic incomming" + ip daddr @internal_prefixes iifname {"vlan{{ .PrivateVrfID }}", "vrf{{ .PrivateVrfID }}"} counter name internal_out comment "count internal traffic outgoing" # rate limits {{- range .RateLimitRules }} diff --git a/pkg/nftables/test_data/more-rules.nftable.v4 b/pkg/nftables/test_data/more-rules.nftable.v4 index 08eb1ab8..d4726849 100644 --- a/pkg/nftables/test_data/more-rules.nftable.v4 +++ b/pkg/nftables/test_data/more-rules.nftable.v4 @@ -30,12 +30,12 @@ table ip firewall { type filter hook forward priority 1; policy drop; # network traffic accounting for external traffic - ip saddr != @internal_prefixes oifname "vlan42" counter name external_in comment "count external traffic incomming" - ip daddr != @internal_prefixes iifname "vrf42" counter name external_out comment "count external traffic outgoing" + ip saddr != @internal_prefixes oifname {"vlan42", "vrf42"} counter name external_in comment "count external traffic incomming" + ip daddr != @internal_prefixes iifname {"vlan42", "vrf42"} counter name external_out comment "count external traffic outgoing" # network traffic accounting for internal traffic - ip saddr @internal_prefixes oifname "vlan42" counter name internal_in comment "count internal traffic incomming" - ip daddr @internal_prefixes iifname "vrf42" counter name internal_out comment "count internal traffic outgoing" + ip saddr @internal_prefixes oifname {"vlan42", "vrf42"} counter name internal_in comment "count internal traffic incomming" + ip daddr @internal_prefixes iifname {"vlan42", "vrf42"} counter name internal_out comment "count internal traffic outgoing" # rate limits meta iifname "eth0" limit rate over 10 mbytes/second counter name drop_ratelimit drop diff --git a/pkg/nftables/test_data/simple.nftable.v4 b/pkg/nftables/test_data/simple.nftable.v4 index 01ee060e..7a732a5f 100644 --- a/pkg/nftables/test_data/simple.nftable.v4 +++ b/pkg/nftables/test_data/simple.nftable.v4 @@ -30,12 +30,12 @@ table ip firewall { type filter hook forward priority 1; policy drop; # network traffic accounting for external traffic - ip saddr != @internal_prefixes oifname "vlan42" counter name external_in comment "count external traffic incomming" - ip daddr != @internal_prefixes iifname "vrf42" counter name external_out comment "count external traffic outgoing" + ip saddr != @internal_prefixes oifname {"vlan42", "vrf42"} counter name external_in comment "count external traffic incomming" + ip daddr != @internal_prefixes iifname {"vlan42", "vrf42"} counter name external_out comment "count external traffic outgoing" # network traffic accounting for internal traffic - ip saddr @internal_prefixes oifname "vlan42" counter name internal_in comment "count internal traffic incomming" - ip daddr @internal_prefixes iifname "vrf42" counter name internal_out comment "count internal traffic outgoing" + ip saddr @internal_prefixes oifname {"vlan42", "vrf42"} counter name internal_in comment "count internal traffic incomming" + ip daddr @internal_prefixes iifname {"vlan42", "vrf42"} counter name internal_out comment "count internal traffic outgoing" # rate limits meta iifname "eth0" limit rate over 10 mbytes/second counter name drop_ratelimit drop diff --git a/pkg/nftables/test_data/validated.nftable.v4 b/pkg/nftables/test_data/validated.nftable.v4 index 6e16e46c..d7558519 100644 --- a/pkg/nftables/test_data/validated.nftable.v4 +++ b/pkg/nftables/test_data/validated.nftable.v4 @@ -30,12 +30,12 @@ table ip firewall { type filter hook forward priority 1; policy drop; # network traffic accounting for external traffic - ip saddr != @internal_prefixes oifname "vlan42" counter name external_in comment "count external traffic incomming" - ip daddr != @internal_prefixes iifname "vrf42" counter name external_out comment "count external traffic outgoing" + ip saddr != @internal_prefixes oifname {"vlan42", "vrf42"} counter name external_in comment "count external traffic incomming" + ip daddr != @internal_prefixes iifname {"vlan42", "vrf42"} counter name external_out comment "count external traffic outgoing" # network traffic accounting for internal traffic - ip saddr @internal_prefixes oifname "vlan42" counter name internal_in comment "count internal traffic incomming" - ip daddr @internal_prefixes iifname "vrf42" counter name internal_out comment "count internal traffic outgoing" + ip saddr @internal_prefixes oifname {"vlan42", "vrf42"} counter name internal_in comment "count internal traffic incomming" + ip daddr @internal_prefixes iifname {"vlan42", "vrf42"} counter name internal_out comment "count internal traffic outgoing" # rate limits