Fix for traffic accounting (#96)

metal-stack · Jul 26, 2021 · f70d32b · f70d32b
1 parent 686a7e0
commit f70d32b
Show file tree

Hide file tree

Showing 7 changed files with 93 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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