From 010ac87b34a46852c4c42d3e0738fda6231f187e Mon Sep 17 00:00:00 2001 From: Tanya Veksler Date: Tue, 12 Nov 2024 14:00:07 +0200 Subject: [PATCH] Improving explainability print format. --- pkg/netpol/connlist/connlist.go | 4 +- pkg/netpol/connlist/conns_formatter.go | 2 +- pkg/netpol/eval/check.go | 13 +++---- pkg/netpol/eval/internal/k8s/adminnetpol.go | 8 ++-- .../internal/k8s/baseline_admin_netpol.go | 6 +-- pkg/netpol/eval/internal/k8s/netpol.go | 30 ++++++++++---- .../internal/common/augmented_intervalset.go | 39 +++++++++++++++---- pkg/netpol/internal/common/connectionset.go | 21 ++++++++-- 8 files changed, 87 insertions(+), 36 deletions(-) diff --git a/pkg/netpol/connlist/connlist.go b/pkg/netpol/connlist/connlist.go index f3926627..abfe868e 100644 --- a/pkg/netpol/connlist/connlist.go +++ b/pkg/netpol/connlist/connlist.go @@ -550,8 +550,8 @@ func (ca *ConnlistAnalyzer) getConnectionsBetweenPeers(pe *eval.PolicyEngine, pe return nil, nil, err } } - // skip empty connections - if allowedConnections.IsEmpty() { + // skip empty connections when running without explainability + if allowedConnections.IsEmpty() && !ca.explain { continue } p2pConnection, err := ca.getP2PConnOrUpdateExposureConn(pe, allowedConnections, srcPeer, dstPeer, exposureMaps) diff --git a/pkg/netpol/connlist/conns_formatter.go b/pkg/netpol/connlist/conns_formatter.go index c7e4153c..72cb2203 100644 --- a/pkg/netpol/connlist/conns_formatter.go +++ b/pkg/netpol/connlist/conns_formatter.go @@ -74,7 +74,7 @@ func (c singleConnFields) string() string { } func (c singleConnFields) stringWithExplanation() string { - return fmt.Sprintf("%s => %s :\n%s", c.Src, c.Dst, c.Explanation) + return fmt.Sprintf("CONNECTIONS BETWEEN %s => %s:\n\n%s", c.Src, c.Dst, c.Explanation) } // formSingleP2PConn returns a string representation of single connection fields as singleConnFields object diff --git a/pkg/netpol/eval/check.go b/pkg/netpol/eval/check.go index 5c138f31..5d1c4062 100644 --- a/pkg/netpol/eval/check.go +++ b/pkg/netpol/eval/check.go @@ -151,7 +151,10 @@ func (pe *PolicyEngine) allAllowedConnectionsBetweenPeers(srcPeer, dstPeer Peer) var err error // cases where any connection is always allowed if isPodToItself(srcK8sPeer, dstK8sPeer) || isPeerNodeIP(srcK8sPeer, dstK8sPeer) || isPeerNodeIP(dstK8sPeer, srcK8sPeer) { - return common.MakeConnectionSet(true), nil + res = common.MakeConnectionSet(true) + res.AddCommonImplyingRule(common.PodToItselfRule, true) + res.AddCommonImplyingRule(common.PodToItselfRule, false) + return res, nil } // egress: get egress allowed connections between the src and dst by // walking through all k8s egress policies capturing the src; @@ -576,8 +579,6 @@ func (pe *PolicyEngine) getAllAllowedXgressConnectionsFromANPs(src, dst k8s.Peer return policiesConns, true, nil } -const systemDefaultRule = "system default: allow all" - // analyzing baseline-admin-network-policy for conns between peers (object kind == BaselineAdminNetworkPolicy): // getXgressDefaultConns returns the default connections between src and dst on the given direction (ingress/egress); @@ -589,8 +590,7 @@ const systemDefaultRule = "system default: allow all" func (pe *PolicyEngine) getXgressDefaultConns(src, dst k8s.Peer, isIngress bool) (*k8s.PolicyConnections, error) { res := k8s.NewPolicyConnections() if pe.baselineAdminNetpol == nil { - res.AllowedConns = common.MakeConnectionSet(true) - res.AllowedConns.AddCommonImplyingRule(systemDefaultRule, isIngress) + res.AllowedConns = common.MakeAllConnectionSetWithRule(common.SystemDefaultRule, isIngress) return res, nil } if isIngress { // ingress @@ -621,7 +621,6 @@ func (pe *PolicyEngine) getXgressDefaultConns(src, dst k8s.Peer, isIngress bool) // if banp rules didn't capture xgress conn between src and dst, return system-default: allow-all; // if banp rule captured xgress conn, only DeniedConns should be impacted by banp rule, // whenever AllowedConns should anyway be system-default: allow-all - res.AllowedConns = common.MakeConnectionSet(true) - res.AllowedConns.AddCommonImplyingRule(systemDefaultRule, isIngress) + res.AllowedConns = common.MakeAllConnectionSetWithRule(common.SystemDefaultRule, isIngress) return res, nil } diff --git a/pkg/netpol/eval/internal/k8s/adminnetpol.go b/pkg/netpol/eval/internal/k8s/adminnetpol.go index ad4a0246..7b945bb5 100644 --- a/pkg/netpol/eval/internal/k8s/adminnetpol.go +++ b/pkg/netpol/eval/internal/k8s/adminnetpol.go @@ -169,7 +169,7 @@ func updatePolicyConns(rulePorts *[]apisv1a.AdminNetworkPolicyPort, ruleName str // ruleConnections returns the connectionSet from the current rule.Ports func ruleConnections(ports *[]apisv1a.AdminNetworkPolicyPort, ruleName string, dst Peer, isIngress bool) (*common.ConnectionSet, error) { if ports == nil { - return common.MakeConnectionSet(true), nil // If Ports is not set then the rule does not filter traffic via port. + return common.MakeAllConnectionSetWithRule(ruleName, isIngress), nil // If Ports is not set then the rule does not filter traffic via port. } res := common.MakeConnectionSet(false) for _, anpPort := range *ports { @@ -282,7 +282,7 @@ func (anp *AdminNetworkPolicy) anpRuleErr(ruleName, description string) error { } func (anp *AdminNetworkPolicy) fullName() string { - return anp.Name + return "[ANP] " + anp.Name } func ruleFullName(policyName, ruleName, action string, isIngress bool) string { @@ -300,7 +300,7 @@ func (anp *AdminNetworkPolicy) GetIngressPolicyConns(src, dst Peer) (*PolicyConn rulePeers := rule.From rulePorts := rule.Ports if err := updateConnsIfIngressRuleSelectsPeer(rulePeers, rulePorts, - ruleFullName("ANP "+anp.fullName(), rule.Name, string(rule.Action), true), + ruleFullName(anp.fullName(), rule.Name, string(rule.Action), true), src, dst, res, string(rule.Action), false); err != nil { return nil, anp.anpRuleErr(rule.Name, err.Error()) } @@ -315,7 +315,7 @@ func (anp *AdminNetworkPolicy) GetEgressPolicyConns(dst Peer) (*PolicyConnection rulePeers := rule.To rulePorts := rule.Ports if err := updateConnsIfEgressRuleSelectsPeer(rulePeers, rulePorts, - ruleFullName("ANP "+anp.fullName(), rule.Name, string(rule.Action), false), + ruleFullName(anp.fullName(), rule.Name, string(rule.Action), false), dst, res, string(rule.Action), false); err != nil { return nil, anp.anpRuleErr(rule.Name, err.Error()) } diff --git a/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go b/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go index 831149ce..d058f90d 100644 --- a/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go +++ b/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go @@ -47,7 +47,7 @@ func banpRuleErr(ruleName, description string) error { } func (banp *BaselineAdminNetworkPolicy) fullName() string { - return banp.Name + return "[BANP] " + banp.Name } // GetEgressPolicyConns returns the connections from the egress rules selecting the dst in spec of the baselineAdminNetworkPolicy @@ -57,7 +57,7 @@ func (banp *BaselineAdminNetworkPolicy) GetEgressPolicyConns(dst Peer) (*PolicyC rulePeers := rule.To rulePorts := rule.Ports if err := updateConnsIfEgressRuleSelectsPeer(rulePeers, rulePorts, - ruleFullName("BANP "+banp.fullName(), rule.Name, string(rule.Action), false), + ruleFullName(banp.fullName(), rule.Name, string(rule.Action), false), dst, res, string(rule.Action), true); err != nil { return nil, banpRuleErr(rule.Name, err.Error()) } @@ -72,7 +72,7 @@ func (banp *BaselineAdminNetworkPolicy) GetIngressPolicyConns(src, dst Peer) (*P rulePeers := rule.From rulePorts := rule.Ports if err := updateConnsIfIngressRuleSelectsPeer(rulePeers, rulePorts, - ruleFullName("BANP "+banp.fullName(), rule.Name, string(rule.Action), true), + ruleFullName(banp.fullName(), rule.Name, string(rule.Action), true), src, dst, res, string(rule.Action), true); err != nil { return nil, banpRuleErr(rule.Name, err.Error()) } diff --git a/pkg/netpol/eval/internal/k8s/netpol.go b/pkg/netpol/eval/internal/k8s/netpol.go index cb05df93..260b66ff 100644 --- a/pkg/netpol/eval/internal/k8s/netpol.go +++ b/pkg/netpol/eval/internal/k8s/netpol.go @@ -121,10 +121,9 @@ func isEmptyPortRange(start, end int32) bool { func (np *NetworkPolicy) ruleConnections(rulePorts []netv1.NetworkPolicyPort, dst Peer, ruleIdx int, isIngress bool) (*common.ConnectionSet, error) { if len(rulePorts) == 0 { - res := common.MakeConnectionSet(true) // If this field is empty or missing, this rule matches all ports - res.AddCommonImplyingRule(np.ruleName(ruleIdx, isIngress), isIngress) - return res, nil + // If this field is empty or missing, this rule matches all ports // (traffic not restricted by port) + return common.MakeAllConnectionSetWithRule(np.ruleName(ruleIdx, isIngress), isIngress), nil } res := common.MakeConnectionSet(false) ruleName := np.ruleName(ruleIdx, isIngress) @@ -358,7 +357,11 @@ func (np *NetworkPolicy) nameWithDirection(isIngress bool) string { // GetEgressAllowedConns returns the set of allowed connections from any captured pod to the destination peer func (np *NetworkPolicy) GetEgressAllowedConns(dst Peer) (*common.ConnectionSet, error) { res := common.MakeConnectionSet(false) - res.AddCommonImplyingRule(np.nameWithDirection(false), false) + if len(np.Spec.Egress) == 0 { + res.AddCommonImplyingRule(np.nameWithDirection(false), false) + return res, nil + } + dstSelectedByAnyRule := false for idx, rule := range np.Spec.Egress { rulePeers := rule.To rulePorts := rule.Ports @@ -369,6 +372,7 @@ func (np *NetworkPolicy) GetEgressAllowedConns(dst Peer) (*common.ConnectionSet, if !peerSelected { continue } + dstSelectedByAnyRule = true ruleConns, err := np.ruleConnections(rulePorts, dst, idx, false) if err != nil { return res, err @@ -378,13 +382,20 @@ func (np *NetworkPolicy) GetEgressAllowedConns(dst Peer) (*common.ConnectionSet, return res, nil } } + if !dstSelectedByAnyRule { + res.AddCommonImplyingRule(np.nameWithDirection(false), false) + } return res, nil } // GetIngressAllowedConns returns the set of allowed connections to a captured dst pod from the src peer func (np *NetworkPolicy) GetIngressAllowedConns(src, dst Peer) (*common.ConnectionSet, error) { res := common.MakeConnectionSet(false) - res.AddCommonImplyingRule(np.nameWithDirection(true), true) + if len(np.Spec.Ingress) == 0 { + res.AddCommonImplyingRule(np.nameWithDirection(true), true) + return res, nil + } + srcSelectedByAnyRule := false for idx, rule := range np.Spec.Ingress { rulePeers := rule.From rulePorts := rule.Ports @@ -395,7 +406,7 @@ func (np *NetworkPolicy) GetIngressAllowedConns(src, dst Peer) (*common.Connecti if !peerSelected { continue } - + srcSelectedByAnyRule = true ruleConns, err := np.ruleConnections(rulePorts, dst, idx, true) if err != nil { return res, err @@ -405,6 +416,9 @@ func (np *NetworkPolicy) GetIngressAllowedConns(src, dst Peer) (*common.Connecti return res, nil } } + if !srcSelectedByAnyRule { + res.AddCommonImplyingRule(np.nameWithDirection(true), true) + } return res, nil } @@ -512,7 +526,7 @@ func (np *NetworkPolicy) Selects(p *Pod, direction netv1.PolicyType) (bool, erro } func (np *NetworkPolicy) fullName() string { - return types.NamespacedName{Name: np.Name, Namespace: np.Namespace}.String() + return "[NP] " + types.NamespacedName{Name: np.Name, Namespace: np.Namespace}.String() } func (np *NetworkPolicy) ruleName(ruleIdx int, isIngress bool) string { @@ -520,7 +534,7 @@ func (np *NetworkPolicy) ruleName(ruleIdx int, isIngress bool) string { if isIngress { xgress = ingressName } - return fmt.Sprintf("NP %s//%s rule #%d", np.fullName(), xgress, ruleIdx+1) + return fmt.Sprintf("%s//%s rule #%d", np.fullName(), xgress, ruleIdx+1) } // ///////////////////////////////////////////////////////////////////////////////////////////// diff --git a/pkg/netpol/internal/common/augmented_intervalset.go b/pkg/netpol/internal/common/augmented_intervalset.go index 5d1ce714..66179b98 100644 --- a/pkg/netpol/internal/common/augmented_intervalset.go +++ b/pkg/netpol/internal/common/augmented_intervalset.go @@ -62,10 +62,22 @@ func (rules ImplyingRulesType) Copy() ImplyingRulesType { } const ( - explTitle = "due to the following policices//rules:" - newLine = "\n" + ExplWithRulesTitle = "due to the following policices//rules:" + IngressDirectionTitle = "INGRESS DIRECTION:" + EgressDirectionTitle = "EGRESS DIRECTION:" + NewLine = "\n" + SystemDefaultRule = "the system default: allow all" + PodToItselfRule = "pod to itself: allow all" + ExplSystemDefault = "due to " + SystemDefaultRule ) +func (rules *ImplyingXgressRulesType) onlySystemDefaultRule() bool { + if _, ok := (*rules)[SystemDefaultRule]; ok { + return len(*rules) == 1 + } + return false +} + func (rules *ImplyingXgressRulesType) String() string { if rules.Empty() { return "" @@ -76,21 +88,34 @@ func (rules *ImplyingXgressRulesType) String() string { formattedRules = append(formattedRules, fmt.Sprintf("%d) %s", order+1, name)) } sort.Strings(formattedRules) // the rule index begins the string, like "2)" - return strings.Join(formattedRules, newLine) + return strings.Join(formattedRules, NewLine) } func (rules ImplyingRulesType) String() string { + if rules.Ingress.onlySystemDefaultRule() && rules.Egress.onlySystemDefaultRule() { + return " " + ExplSystemDefault + NewLine + } res := "" if !rules.Ingress.Empty() { - res += fmt.Sprintf("INGRESS DIRECTION:\n%s\n", rules.Ingress.String()) + res += IngressDirectionTitle + if rules.Ingress.onlySystemDefaultRule() { + res += " " + ExplSystemDefault + NewLine + } else { + res += NewLine + rules.Ingress.String() + NewLine + } } if !rules.Egress.Empty() { - res += fmt.Sprintf("EGRESS DIRECTION:\n%s\n", rules.Egress.String()) + res += EgressDirectionTitle + if rules.Egress.onlySystemDefaultRule() { + res += " " + ExplSystemDefault + NewLine + } else { + res += NewLine + rules.Egress.String() + NewLine + } } if res == "" { - return "" + return NewLine } - return " " + explTitle + newLine + res + return " " + ExplWithRulesTitle + NewLine + res } func (rules *ImplyingXgressRulesType) Empty() bool { diff --git a/pkg/netpol/internal/common/connectionset.go b/pkg/netpol/internal/common/connectionset.go index 84070fd9..5faa9b3c 100644 --- a/pkg/netpol/internal/common/connectionset.go +++ b/pkg/netpol/internal/common/connectionset.go @@ -40,6 +40,11 @@ func MakeConnectionSet(all bool) *ConnectionSet { return &ConnectionSet{AllowedProtocols: map[v1.Protocol]*PortSet{}, CommonImplyingRules: MakeImplyingRules()} } +func MakeAllConnectionSetWithRule(rule string, isIngress bool) *ConnectionSet { + return &ConnectionSet{AllowAll: true, AllowedProtocols: map[v1.Protocol]*PortSet{}, + CommonImplyingRules: MakeImplyingRulesWithRule(rule, isIngress)} +} + // Add common implying rule, i.e., a rule that is relevant for the whole ConnectionSet func (conn *ConnectionSet) AddCommonImplyingRule(implyingRule string, isIngress bool) { conn.CommonImplyingRules.AddRule(implyingRule, isIngress) @@ -158,9 +163,10 @@ func (conn *ConnectionSet) rebuildAllowAllExplicitly() { // Union updates ConnectionSet object to be the union result with other ConnectionSet func (conn *ConnectionSet) Union(other *ConnectionSet) { - if conn.IsEmpty() && other.IsEmpty() && len(conn.AllowedProtocols) == 0 && len(other.AllowedProtocols) == 0 { + if conn.IsEmpty() && (other.IsEmpty() || other.AllowAll) && len(conn.AllowedProtocols) == 0 && len(other.AllowedProtocols) == 0 { // a special case when we should union implying rules conn.CommonImplyingRules.Union(other.CommonImplyingRules) + conn.AllowAll = other.AllowAll return } if conn.AllowAll || other.IsEmpty() { @@ -189,6 +195,13 @@ func (conn *ConnectionSet) Subtract(other *ConnectionSet) { if other.IsEmpty() { // nothing to subtract return } + if other.AllowAll && len(other.AllowedProtocols) == 0 { + // a special case when we should replace current common implying rules by others' + conn.CommonImplyingRules = other.CommonImplyingRules.Copy() + conn.AllowAll = false + conn.AllowedProtocols = map[v1.Protocol]*PortSet{} + return + } if conn.AllowAll { conn.rebuildAllowAllExplicitly() conn.AllowAll = false @@ -451,7 +464,7 @@ func portsStringWithExplanation(ports []PortRange, protocolString string) (strin if len(portsStr) == 0 { return noConnsStr + noPortsExplanation.String(), false } - return strings.Join(portsStr, newLine), true + return strings.Join(portsStr, NewLine), true } func protocolAndPortsStr(protocol v1.Protocol, ports string) string { @@ -460,7 +473,7 @@ func protocolAndPortsStr(protocol v1.Protocol, ports string) string { func ExplanationFromConnProperties(allProtocolsAndPorts bool, commonImplyingRules ImplyingRulesType, protocolsAndPorts map[v1.Protocol][]PortRange) string { - if allProtocolsAndPorts || len(protocolsAndPorts) == 0 { + if len(protocolsAndPorts) == 0 { connStr := noConnsStr if allProtocolsAndPorts { connStr = allConnsStr @@ -477,6 +490,6 @@ func ExplanationFromConnProperties(allProtocolsAndPorts bool, commonImplyingRule } } sort.Strings(connStrings) - connStr = strings.Join(connStrings, connsAndPortRangeSeparator) + connStr = strings.Join(connStrings, NewLine) return connStr }