Skip to content

Commit

Permalink
Add more logging information for netpol
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@manuelbuil @rbrtbnfgl
manuelbuil authored and rbrtbnfgl committed Jan 8, 2024
1 parent 75eb570 commit ffc965d
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 17 deletions.
1 change: 1 addition & 0 deletions pkg/controllers/netpol/network_policy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 32 additions & 16 deletions pkg/controllers/netpol/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -406,15 +406,15 @@ 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
}
}
for _, portProtocol := range egressRule.namedPorts {
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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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
}
}
Expand All @@ -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)
Expand Down Expand Up @@ -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
}

Expand All @@ -514,6 +529,7 @@ func (npc *NetworkPolicyController) buildNetworkPoliciesInfo() ([]networkPolicyI
namespace: policy.Namespace,
podSelector: podSelector,
policyType: kubeIngressPolicyType,
annotations: policy.Annotations,
}

ingressType, egressType := false, false
Expand Down
88 changes: 87 additions & 1 deletion pkg/controllers/netpol/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}

0 comments on commit ffc965d

Please sign in to comment.