From ffc965d0ddade7446a3af4dbf1e1ecc43cbd7d0e Mon Sep 17 00:00:00 2001 From: Manuel Buil Date: Thu, 20 Oct 2022 15:25:06 +0200 Subject: [PATCH] Add more logging information for netpol * Add one new NFLOG rule per network policy * Be able to control the limit and limit-burst parameters via annotation Signed-off-by: Manuel Buil --- .../netpol/network_policy_controller.go | 1 + pkg/controllers/netpol/policy.go | 48 ++++++---- pkg/controllers/netpol/utils.go | 88 ++++++++++++++++++- 3 files changed, 120 insertions(+), 17 deletions(-) diff --git a/pkg/controllers/netpol/network_policy_controller.go b/pkg/controllers/netpol/network_policy_controller.go index efbcc88ce1..863936a7dc 100644 --- a/pkg/controllers/netpol/network_policy_controller.go +++ b/pkg/controllers/netpol/network_policy_controller.go @@ -95,6 +95,7 @@ type networkPolicyInfo struct { name string namespace string podSelector labels.Selector + annotations map[string]string // set of pods matching network policy spec podselector label selector targetPods map[string]podInfo diff --git a/pkg/controllers/netpol/policy.go b/pkg/controllers/netpol/policy.go index e8333ac3d5..e109d87897 100644 --- a/pkg/controllers/netpol/policy.go +++ b/pkg/controllers/netpol/policy.go @@ -225,7 +225,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, srcPodIPSetName, namedPortIPSetName, - endPoints.protocol, endPoints.port, endPoints.endport, ipFamily); err != nil { + endPoints.protocol, endPoints.port, endPoints.endport, ipFamily, policy); err != nil { return err } } @@ -238,7 +238,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, - comment, srcPodIPSetName, targetDestPodIPSetName, "", "", "", ipFamily); err != nil { + comment, srcPodIPSetName, targetDestPodIPSetName, "", "", "", ipFamily, policy); err != nil { return err } } @@ -251,7 +251,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from all sources to dest pods selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, "", targetDestPodIPSetName, - portProtocol.protocol, portProtocol.port, portProtocol.endport, ipFamily); err != nil { + portProtocol.protocol, portProtocol.port, portProtocol.endport, ipFamily, policy); err != nil { return err } } @@ -269,7 +269,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from all sources to dest pods selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, - comment, "", namedPortIPSetName, endPoints.protocol, endPoints.port, endPoints.endport, ipFamily); err != nil { + comment, "", namedPortIPSetName, endPoints.protocol, endPoints.port, endPoints.endport, ipFamily, policy); err != nil { return err } } @@ -281,7 +281,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from all sources to dest pods selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, "", targetDestPodIPSetName, - "", "", "", ipFamily); err != nil { + "", "", "", ipFamily, policy); err != nil { return err } } @@ -297,7 +297,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, srcIPBlockIPSetName, targetDestPodIPSetName, portProtocol.protocol, portProtocol.port, - portProtocol.endport, ipFamily); err != nil { + portProtocol.endport, ipFamily, policy); err != nil { return err } } @@ -314,7 +314,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from specified ipBlocks to dest pods selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, srcIPBlockIPSetName, namedPortIPSetName, - endPoints.protocol, endPoints.port, endPoints.endport, ipFamily); err != nil { + endPoints.protocol, endPoints.port, endPoints.endport, ipFamily, policy); err != nil { return err } } @@ -323,7 +323,7 @@ func (npc *NetworkPolicyController) processIngressRules(policy networkPolicyInfo comment := "rule to ACCEPT traffic from specified ipBlocks to dest pods selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, srcIPBlockIPSetName, - targetDestPodIPSetName, "", "", "", ipFamily); err != nil { + targetDestPodIPSetName, "", "", "", ipFamily, policy); err != nil { return err } } @@ -380,7 +380,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, - namedPortIPSetName, endPoints.protocol, endPoints.port, endPoints.endport, ipFamily); err != nil { + namedPortIPSetName, endPoints.protocol, endPoints.port, endPoints.endport, ipFamily, policy); err != nil { return err } } @@ -393,7 +393,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, - dstPodIPSetName, "", "", "", ipFamily); err != nil { + dstPodIPSetName, "", "", "", ipFamily, policy); err != nil { return err } } @@ -406,7 +406,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, comment := "rule to ACCEPT traffic from source pods to all destinations selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, - "", portProtocol.protocol, portProtocol.port, portProtocol.endport, ipFamily); err != nil { + "", portProtocol.protocol, portProtocol.port, portProtocol.endport, ipFamily, policy); err != nil { return err } } @@ -414,7 +414,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, comment := "rule to ACCEPT traffic from source pods to all destinations selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, - "", portProtocol.protocol, portProtocol.port, portProtocol.endport, ipFamily); err != nil { + "", portProtocol.protocol, portProtocol.port, portProtocol.endport, ipFamily, policy); err != nil { return err } } @@ -426,7 +426,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, comment := "rule to ACCEPT traffic from source pods to all destinations selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, - "", "", "", "", ipFamily); err != nil { + "", "", "", "", ipFamily, policy); err != nil { return err } } @@ -441,7 +441,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, dstIPBlockIPSetName, portProtocol.protocol, portProtocol.port, - portProtocol.endport, ipFamily); err != nil { + portProtocol.endport, ipFamily, policy); err != nil { return err } } @@ -450,7 +450,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, comment := "rule to ACCEPT traffic from source pods to specified ipBlocks selected by policy name: " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyChainName, comment, targetSourcePodIPSetName, - dstIPBlockIPSetName, "", "", "", ipFamily); err != nil { + dstIPBlockIPSetName, "", "", "", ipFamily, policy); err != nil { return err } } @@ -460,7 +460,7 @@ func (npc *NetworkPolicyController) processEgressRules(policy networkPolicyInfo, } func (npc *NetworkPolicyController) appendRuleToPolicyChain(policyChainName, comment, srcIPSetName, dstIPSetName, - protocol, dPort, endDport string, ipFamily api.IPFamily) error { + protocol, dPort, endDport string, ipFamily api.IPFamily, policy networkPolicyInfo) error { args := make([]string, 0) args = append(args, "-A", policyChainName) @@ -493,6 +493,21 @@ func (npc *NetworkPolicyController) appendRuleToPolicyChain(policyChainName, com args = append(args, "-m", "mark", "--mark", "0x10000/0x10000", "-j", "RETURN", "\n") npc.filterTableRules[ipFamily].WriteString(strings.Join(args, " ")) + // Extra logs to get more information about the policy dropping the packet via ulog2 + logRuleComment := "\"rule to log dropped traffic\"" + + // The network policy annotation can include a log config + limit, limitBurst := getIptablesNFlogLimit(policy.annotations) + + // LogComment is capped at 64 characters and we are using 16, hence policyNamespace and policyName + // must fit in 48 characters otherwise we can only log the first 24 characters + policyNamespaceAndName := safeJoin(policy.namespace, policy.name) + logComment := "\"DROP by policy " + policyNamespaceAndName + "\"" + logArgs := []string{"-A", policyChainName, "-m", "comment", "--comment", logRuleComment, "-m", "limit", + "--limit", limit, "--limit-burst", limitBurst, "-m", "mark", "!", "--mark", "0x10000/0x10000", + "-j", "NFLOG", "--nflog-group", "100", "--nflog-prefix", logComment, "\n"} + npc.filterTableRules[ipFamily].WriteString(strings.Join(logArgs, " ")) + return nil } @@ -514,6 +529,7 @@ func (npc *NetworkPolicyController) buildNetworkPoliciesInfo() ([]networkPolicyI namespace: policy.Namespace, podSelector: podSelector, policyType: kubeIngressPolicyType, + annotations: policy.Annotations, } ingressType, egressType := false, false diff --git a/pkg/controllers/netpol/utils.go b/pkg/controllers/netpol/utils.go index 1508c26f36..5cb5d705fc 100644 --- a/pkg/controllers/netpol/utils.go +++ b/pkg/controllers/netpol/utils.go @@ -5,17 +5,23 @@ import ( "reflect" "regexp" "strconv" + "strings" "github.com/cloudnativelabs/kube-router/v2/pkg/utils" api "k8s.io/api/core/v1" klog "k8s.io/klog/v2" netutils "k8s.io/utils/net" + "k8s.io/apimachinery/pkg/util/sets" ) const ( PodCompleted api.PodPhase = "Completed" ) +var ( + stringSuffixes = sets.NewString("second", "minute", "hour", "day", "s", "m", "h", "d") +) + // isPodUpdateNetPolRelevant checks the attributes that we care about for building NetworkPolicies on the host and if it // finds a relevant change, it returns true otherwise it returns false. The things we care about for NetworkPolicies: // 1. Is the phase of the pod changing? (matters for catching completed, succeeded, or failed jobs) @@ -118,7 +124,7 @@ func (npc *NetworkPolicyController) createPodWithPortPolicyRule(ports []protocol comment := "rule to ACCEPT traffic from source pods to dest pods selected by policy name " + policy.name + " namespace " + policy.namespace if err := npc.appendRuleToPolicyChain(policyName, comment, srcSetName, dstSetName, portProtocol.protocol, - portProtocol.port, portProtocol.endport, ipFamily); err != nil { + portProtocol.port, portProtocol.endport, ipFamily, policy); err != nil { return err } } @@ -164,3 +170,83 @@ func getPodIPForFamily(pod podInfo, ipFamily api.IPFamily) (string, error) { return "", fmt.Errorf("did not recognize IP Family for pod: %s:%s family: %s", pod.namespace, pod.name, ipFamily) } + +// safeJoin joins the namespace and name, ensuring that the result is less than or equal to 48 characters +func safeJoin(namespace string, name string) string { + if (len(namespace) + len(name)) < 48 { + return namespace + "/" + name + } + + // We must create at least one substring + if len(namespace) < 24 { + lengthSubString := (48 - len(namespace) - 1) + return namespace + "/" + name[0:lengthSubString] + } + + if len(name) < 24 { + lengthSubString := (48 - len(name) - 1) + return namespace[0:lengthSubString] + "/" + name + } + + //If we arrive here, both are over 24 characters + return namespace[0:23] + "/" + name[0:23] +} + +// getIptablesNFlogLimit reads the annotations setting the nflog limit and limit-burst config +// "kube-router.io/netpol-nflog-limit" and "kube-router.io/netpol-nflog-limit-burst" +func getIptablesNFlogLimit(annotations map[string]string) (string, string) { + defaultLimit := "10/minute" + defaultLimitBurst := "10" + + limit, ok := annotations["kube-router.io/netpol-nflog-limit"] + if !ok { + limit = defaultLimit + } + + limitBurst, ok := annotations["kube-router.io/netpol-nflog-limit-burst"] + if !ok { + limitBurst = defaultLimitBurst + } + + if !areNFlogParamsCorrect(limit, limitBurst) { + klog.Warning("Network Policy annotations are wrong, default values will be used. Check the docs for more information") + return defaultLimit, defaultLimitBurst + } + + return limit, limitBurst +} + + +// areNFlogParamsCorrect verifies that the nflog parameters are correct +// * kube-router.io/netpol-nflog-limit must be an integer, with an optional "/second", "/minute", "/hour", "/day" or the first character of each time unit +// * kube-router.io/netpol-nflog-limit-burst must be an integer +func areNFlogParamsCorrect(limit string, limitBurst string) bool { + + // If limit does not set a time unit, check if limit and limitBurst are integers + param := strings.Split(limit, "/") + if len(param) == 1 { + if isInteger(param[0]) && isInteger(limitBurst) { + return true + } + } + + // If limit sets a time unit, check if it is among the supported ones + if len(param) == 2 { + if stringSuffixes.Has(param[1]) { + // If the time unit is supported, check that limit and limitBurst are integers + if isInteger(param[0]) && isInteger(limitBurst) { + return true + } + } + } + + return false +} + +// isInteger returns true if the passed value is an integer +func isInteger(x string) bool { + if _, err := strconv.Atoi(x); err == nil { + return true + } + return false +}