feat: [NPM] iptables comments for v2

npm/pkg/dataplane/ipsets/ipset.go

@@ -42,6 +42,7 @@ func (setMetadata *IPSetMetadata) GetHashedName() string {
 	return util.GetHashedName(prefixedName)
 }
 
+// TODO join with colon instead of dash for easier readability?
 func (setMetadata *IPSetMetadata) GetPrefixName() string {
 	switch setMetadata.Type {
 	case CIDRBlocks:

npm/pkg/dataplane/ipsets/testutils.go

@@ -2,6 +2,8 @@ package ipsets
 
 import "github.com/Azure/azure-container-networking/npm/util"
 
+// TODO deprecate the TestSet type and replace TestNSSet etc. with just their metadata
+// since you can get prefix name and hashed name with metadata methods
 type TestSet struct {
 	Metadata   *IPSetMetadata
 	PrefixName string

npm/pkg/dataplane/policies/policy.go

@@ -2,7 +2,6 @@ package policies
 
 import (
 	"fmt"
-	"strconv"
 	"strings"
 
 	"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
@@ -69,13 +68,15 @@ type ACLPolicy struct {
 
 const policyIDPrefix = "azure-acl"
 
+// FIXME this impacts windows DP if it isn't equivalent to netPol.PolicyKey
 // aclPolicyID returns azure-acl-<network policy namespace>-<network policy name> format
 // to differentiate ACLs among different network policies,
 // but aclPolicy in the same network policy has the same aclPolicyID.
 func aclPolicyID(policyNS, policyName string) string {
 	return fmt.Sprintf("%s-%s-%s", policyIDPrefix, policyNS, policyName)
 }
 
+// TODO make this a method of NPMNetworkPolicy, and just use netPol.PolicyKey as the PolicyID
 func NewACLPolicy(policyNS, policyName string, target Verdict, direction Direction) *ACLPolicy {
 	acl := &ACLPolicy{
 		PolicyID:  aclPolicyID(policyNS, policyName),
@@ -132,7 +133,19 @@ func (aclPolicy *ACLPolicy) hasKnownTarget() bool {
 }
 
 func (aclPolicy *ACLPolicy) satisifiesPortAndProtocolConstraints() bool {
-	return (aclPolicy.Protocol != UnspecifiedProtocol) || (aclPolicy.DstPorts.Port == 0 && aclPolicy.DstPorts.EndPort == 0)
+	// namedports handle protocol constraints
+	return (aclPolicy.hasNamedPort() && aclPolicy.Protocol == UnspecifiedProtocol) ||
+		aclPolicy.Protocol != UnspecifiedProtocol ||
+		aclPolicy.DstPorts.isUnspecified()
+}
+
+func (aclPolicy *ACLPolicy) hasNamedPort() bool {
+	for _, peer := range aclPolicy.DstList {
+		if peer.IPSet.Type == ipsets.NamedPorts {
+			return true
+		}
+	}
+	return false
 }
 
 func (netPol *NPMNetworkPolicy) String() string {
@@ -221,13 +234,8 @@ func (portRange *Ports) isValidRange() bool {
 	return portRange.Port <= portRange.EndPort
 }
 
-func (portRange *Ports) toIPTablesString() string {
-	start := strconv.Itoa(int(portRange.Port))
-	if portRange.Port == portRange.EndPort {
-		return start
-	}
-	end := strconv.Itoa(int(portRange.EndPort))
-	return start + ":" + end
+func (portRange *Ports) isUnspecified() bool {
+	return portRange.Port == 0
 }
 
 type Direction string

npm/pkg/dataplane/policies/policy_linux.go

@@ -1,6 +1,14 @@
 package policies
 
-import "github.com/Azure/azure-container-networking/npm/util"
+import (
+	"fmt"
+	"strconv"
+	"strings"
+
+	"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
+	"github.com/Azure/azure-container-networking/npm/util"
+	"k8s.io/klog"
+)
 
 // returns two booleans indicating whether the network policy has ingress and egress respectively
 func (networkPolicy *NPMNetworkPolicy) hasIngressAndEgress() (hasIngress, hasEgress bool) {
@@ -25,3 +33,212 @@ func (networkPolicy *NPMNetworkPolicy) chainName(prefix string) string {
 	policyHash := util.Hash(networkPolicy.PolicyKey)
 	return joinWithDash(prefix, policyHash)
 }
+
+type UniqueDirection bool
+
+const (
+	forIngress UniqueDirection = true
+	forEgress  UniqueDirection = false
+)
+
+func (networkPolicy *NPMNetworkPolicy) commentForJumpToIngress() string {
+	return networkPolicy.commentForJump(forIngress)
+}
+
+func (networkPolicy *NPMNetworkPolicy) commentForJumpToEgress() string {
+	return networkPolicy.commentForJump(forEgress)
+}
+
+func (networkPolicy *NPMNetworkPolicy) commentForJump(direction UniqueDirection) string {
+	prefix := "EGRESS"
+	if direction == forIngress {
+		prefix = "INGRESS"
+	}
+
+	toFrom := "FROM"
+	if direction == forIngress {
+		toFrom = "TO"
+	}
+
+	podSelectorComment := "all"
+	if len(networkPolicy.PodSelectorList) > 0 {
+		podSelectorComment = commentForInfos(networkPolicy.PodSelectorList)
+	}
+	return fmt.Sprintf("%s-POLICY-%s-%s-%s-IN-ns-%s", prefix, networkPolicy.PolicyKey, toFrom, podSelectorComment, networkPolicy.NameSpace)
+}
+
+func commentForInfos(infos []SetInfo) string {
+	infoComments := make([]string, 0, len(infos))
+	for _, info := range infos {
+		infoComments = append(infoComments, info.comment())
+	}
+	return strings.Join(infoComments, "-AND-")
+}
+
+func (info SetInfo) comment() string {
+	name := info.IPSet.GetPrefixName()
+	if info.Included {
+		return name
+	}
+	return "!" + name
+}
+
+func (aclPolicy *ACLPolicy) comment() string {
+	// cleanPeerList contains peers that aren't namedports
+	var cleanPeerList []SetInfo
+	// TODO remove the if check and initialize the list like in egress once we replace SrcList and DstList with PeerList
+	if aclPolicy.Direction == Ingress {
+		cleanPeerList = aclPolicy.SrcList
+	} else {
+		cleanPeerList = make([]SetInfo, 0, len(aclPolicy.DstList))
+	}
+
+	var namedPortPeer SetInfo
+	foundNamedPortPeer := false
+	for _, info := range aclPolicy.DstList {
+		if info.IPSet.Type == ipsets.NamedPorts {
+			if foundNamedPortPeer {
+				klog.Errorf("while creating ACL comment, unexpectedly found more than one namedPort peer for ACL:\n%s", aclPolicy.String())
+			}
+			namedPortPeer = info
+			foundNamedPortPeer = true
+		} else if aclPolicy.Direction == Egress {
+			// TODO remove the if check once we replace SrcList and DstList with PeerList
+			cleanPeerList = append(cleanPeerList, info)
+		}
+	}
+
+	builder := strings.Builder{}
+	if aclPolicy.Target == Allowed {
+		builder.WriteString("ALLOW")
+	} else {
+		builder.WriteString("DROP")
+	}
+
+	if len(cleanPeerList) == 0 {
+		builder.WriteString("-ALL")
+	} else {
+		if aclPolicy.Direction == Ingress {
+			builder.WriteString("-FROM-")
+		} else {
+			builder.WriteString("-TO-")
+		}
+		builder.WriteString(commentForInfos(cleanPeerList))
+	}
+
+	builder.WriteString(aclPolicy.Protocol.comment())
+	builder.WriteString(aclPolicy.DstPorts.comment())
+	if foundNamedPortPeer {
+		builder.WriteString("-TO-" + namedPortPeer.comment())
+	}
+	return builder.String()
+}
+
+func (proto Protocol) comment() string {
+	if proto == UnspecifiedProtocol {
+		return ""
+	}
+	return fmt.Sprintf("-ON-%s", string(proto))
+}
+
+func (portRange *Ports) comment() string {
+	if portRange.Port == 0 {
+		return ""
+	}
+	if portRange.Port >= portRange.EndPort {
+		return fmt.Sprintf("-TO-PORT-%d", portRange.Port)
+	}
+	return fmt.Sprintf("-TO-PORT-%d:%d", portRange.Port, portRange.EndPort)
+}
+
+func (portRange *Ports) toIPTablesString() string {
+	start := strconv.Itoa(int(portRange.Port))
+	if portRange.Port == portRange.EndPort {
+		return start
+	}
+	end := strconv.Itoa(int(portRange.EndPort))
+	return start + ":" + end
+}
+
+/*
+	Notes on commenting
+
+	v1 overall:
+		"[value]" means include if needed e.g. if a port is specified
+		- prefix:
+			- no to/from rules and not allowing external:
+				- allowed: "ALLOW-ALL"
+				- denied: "DROP-ALL"
+			- otherwise: drop the "ALL"
+		- suffix:
+			- no to/from rules or port rules and not allowing external:
+				- ingress: "-FROM-all-namespaces"
+				- egress: "-TO-all-namespaces"
+			- otherwise: "" (no suffix)
+		- append these to each other to form the whole comment:
+			prefix
+			[-cidrIPSetName]
+			[-AND-nsSelectorComment]
+			[-AND]
+			[-podSelectorComment]
+			[-protocolComment]
+			[-portComment]
+			-TO  (or "-FROM" if egress)
+			-targetSelectorComment
+			suffix
+
+	v2 overall for ACLs:
+		- prefix:
+			- allowed:
+				- no IPSets in SrcList/DstList (i.e. allow external): "ALLOW-ALL"
+				- otherwise:
+					- ingress: "ALLOW-FROM"
+					- egress: "ALLOW-TO"
+			- denied: replace "ALLOW" with "DROP"
+		- similar idea (think there are at most two non-namedPort ipsets e.g. ns selector and pod selector):
+			prefix
+			[-ipset1Name]
+			[-AND]
+			[-ipset2Name]
+			[-ON-protocolComment]
+			[-TO-namedPortIPSetName]
+			[-TO-portComment]
+		NOTE: can have none or only one of namedPort and port (range)
+
+	v2 for jumps to ingress/egress chains:
+		- prefix:
+			- ingress: "INGRESS"
+			- egress: "EGRESS"
+		- form:
+			INGRESS     (or "EGRESS-" if egress)
+			-POLICY
+			-policyKey
+			-TO         (or "-FROM" if egress)
+			[-podSelectorComment]   (or "all" if there are no pod selectors)
+			-IN-ns
+			-namespaceName
+
+	strings for protocol, ports, selectors:
+		protocol: just "name"
+
+		port (range):
+			- v1:
+				- for single port: PORT-x
+				- for namedport: PORT-name
+			- v2:
+				- for single port: PORT-x
+				- with endport: PORT-x:y
+				- in v2, namedports are specified as IPSets
+
+		selector:
+			"[!]" means optionally include "!" if the label is not included
+			- namespace selectors (only for v1):
+				- if there are no match expressions or labels:
+					all-namespaces
+				- otherwise:
+					ns-[!]label1-AND-ns-[!]label2...
+			- other w/out ns:
+				[!]label1-AND-[!]label2...
+			- other w/ ns:
+				[!]label1-AND-[!]label2...-AND-ns-[!]labelM-IN-ns-name
+*/

npm/pkg/dataplane/policies/policymanager_linux.go

@@ -111,11 +111,11 @@ func (pMgr *PolicyManager) deleteOldJumpRulesOnRemove(policy *NPMNetworkPolicy)
 	return nil
 }
 
-func (pMgr *PolicyManager) deleteJumpRule(policy *NPMNetworkPolicy, isIngress bool) error {
+func (pMgr *PolicyManager) deleteJumpRule(policy *NPMNetworkPolicy, direction UniqueDirection) error {
 	var specs []string
 	var baseChainName string
 	var chainName string
-	if isIngress {
+	if direction == forIngress {
 		specs = ingressJumpSpecs(policy)
 		baseChainName = util.IptablesAzureIngressChain
 		chainName = policy.ingressChainName()
@@ -139,13 +139,17 @@ func (pMgr *PolicyManager) deleteJumpRule(policy *NPMNetworkPolicy, isIngress bo
 func ingressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string {
 	chainName := networkPolicy.ingressChainName()
 	specs := []string{util.IptablesJumpFlag, chainName}
-	return append(specs, matchSetSpecsForNetworkPolicy(networkPolicy, DstMatch)...)
+	specs = append(specs, matchSetSpecsForNetworkPolicy(networkPolicy, DstMatch)...)
+	specs = append(specs, commentSpecs(networkPolicy.commentForJumpToIngress())...)
+	return specs
 }
 
 func egressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string {
 	chainName := networkPolicy.egressChainName()
 	specs := []string{util.IptablesJumpFlag, chainName}
-	return append(specs, matchSetSpecsForNetworkPolicy(networkPolicy, SrcMatch)...)
+	specs = append(specs, matchSetSpecsForNetworkPolicy(networkPolicy, SrcMatch)...)
+	specs = append(specs, commentSpecs(networkPolicy.commentForJumpToEgress())...)
+	return specs
 }
 
 // noflush add to chains impacted
@@ -210,9 +214,7 @@ func iptablesRuleSpecs(aclPolicy *ACLPolicy) []string {
 	specs = append(specs, dstPortSpecs(aclPolicy.DstPorts)...)
 	specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.SrcList)...)
 	specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.DstList)...)
-	if aclPolicy.Comment != "" {
-		specs = append(specs, commentSpecs(aclPolicy.Comment)...)
-	}
+	specs = append(specs, commentSpecs(aclPolicy.comment())...)
 	return specs
 }