From 5e900229d54e7b756f657e7def0de61d7dd148b5 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Sun, 20 Oct 2024 16:49:10 +0300 Subject: [PATCH 01/17] eval command support + unit tests --- pkg/netpol/eval/check.go | 258 ++++++------------ pkg/netpol/eval/check_eval.go | 257 +++++++++++++++++ pkg/netpol/eval/eval_test.go | 14 +- pkg/netpol/eval/internal/k8s/adminnetpol.go | 157 +++++++++++ .../internal/k8s/baseline_admin_netpol.go | 34 +++ pkg/netpol/eval/internal/k8s/netpol.go | 2 +- 6 files changed, 543 insertions(+), 179 deletions(-) create mode 100644 pkg/netpol/eval/check_eval.go diff --git a/pkg/netpol/eval/check.go b/pkg/netpol/eval/check.go index df1a58c7..fb15a94f 100644 --- a/pkg/netpol/eval/check.go +++ b/pkg/netpol/eval/check.go @@ -22,42 +22,12 @@ import ( "github.com/np-guard/netpol-analyzer/pkg/netpol/internal/common" ) -// CheckIfAllowed returns true if the given input connection is allowed by network policies -func (pe *PolicyEngine) CheckIfAllowed(src, dst, protocol, port string) (bool, error) { - srcPeer, err := pe.getPeer(src) - if err != nil { - return false, err - } - dstPeer, err := pe.getPeer(dst) - if err != nil { - return false, err - } - // cases where any connection is always allowed - if isPodToItself(srcPeer, dstPeer) || isPeerNodeIP(srcPeer, dstPeer) || isPeerNodeIP(dstPeer, srcPeer) { - return true, nil - } +// this file contains methods that return all allowed connections between two peers; +// those funcs are related to the `list` & `diff` commands. - hasResult, res := pe.cache.hasConnectionResult(srcPeer, dstPeer, protocol, port) - if hasResult { - return res, nil - } - - egressRes, err := pe.allowedXgressConnection(srcPeer, dstPeer, false, protocol, port) - if err != nil { - return false, err - } - if !egressRes { - pe.cache.addConnectionResult(srcPeer, dstPeer, protocol, port, false) - return false, nil - } - ingressRes, err := pe.allowedXgressConnection(srcPeer, dstPeer, true, protocol, port) - if err != nil { - return false, err - } - pe.cache.addConnectionResult(srcPeer, dstPeer, protocol, port, ingressRes) - return ingressRes, nil -} +// it also contains inner funcs in `eval` package; used in this file and `check_eval.go` +// convertPeerToPodPeer converts a given workload peer to a PodPeer object. func (pe *PolicyEngine) convertPeerToPodPeer(peer Peer) (*k8s.PodPeer, error) { var podObj *k8s.Pod var podNamespace *k8s.Namespace @@ -76,6 +46,9 @@ func (pe *PolicyEngine) convertPeerToPodPeer(peer Peer) (*k8s.PodPeer, error) { return podPeer, nil } +// getPeerNamespaceObject returns the namespace object for the given pod. +// If the pod is a representative peer, it returns nil, +// Otherwise, it returns the namespace object from the policy engine. func (pe *PolicyEngine) getPeerNamespaceObject(podObj *k8s.Pod) (*k8s.Namespace, error) { // if this is a representative peer which is not in a real namespace return nil; // PolicyEngine does not contain representative namespaces. @@ -102,79 +75,6 @@ func (pe *PolicyEngine) changePodPeerToAnotherPodObject(peer *k8s.PodPeer) { } } -// AllAllowedConnectionsBetweenWorkloadPeers returns the allowed connections from srcPeer to dstPeer, -// expecting that srcPeer and dstPeer are in level of workloads (WorkloadPeer) -func (pe *PolicyEngine) AllAllowedConnectionsBetweenWorkloadPeers(srcPeer, dstPeer Peer) (*common.ConnectionSet, error) { - if srcPeer.IsPeerIPType() && !dstPeer.IsPeerIPType() { - // assuming dstPeer is WorkloadPeer/RepresentativePeer, should be converted to k8s.Peer - dstPodPeer, err := pe.convertPeerToPodPeer(dstPeer) - if err != nil { - return nil, err - } - return pe.allAllowedConnectionsBetweenPeers(srcPeer, dstPodPeer) - } - if dstPeer.IsPeerIPType() && !srcPeer.IsPeerIPType() { - // assuming srcPeer is WorkloadPeer/RepresentativePeer, should be converted to k8s.Peer - srcPodPeer, err := pe.convertPeerToPodPeer(srcPeer) - if err != nil { - return nil, err - } - return pe.allAllowedConnectionsBetweenPeers(srcPodPeer, dstPeer) - } - if !dstPeer.IsPeerIPType() && !srcPeer.IsPeerIPType() { - // assuming srcPeer and dstPeer are WorkloadPeer/RepresentativePeer, should be converted to k8s.Peer - srcPodPeer, err := pe.convertPeerToPodPeer(srcPeer) - if err != nil { - return nil, err - } - dstPodPeer, err := pe.convertPeerToPodPeer(dstPeer) - if err != nil { - return nil, err - } - // if src and dst are the same workload peer, their conversion to pods should be of different pods - // (if owner has more than 1 instances) - if srcPeer.String() == dstPeer.String() { - pe.changePodPeerToAnotherPodObject(dstPodPeer) - } - return pe.allAllowedConnectionsBetweenPeers(srcPodPeer, dstPodPeer) - } - return nil, errors.New(netpolerrors.BothSrcAndDstIPsErrStr(srcPeer.String(), dstPeer.String())) -} - -// allAllowedConnectionsBetweenPeers: returns the allowed connections from srcPeer to dstPeer -// expecting that srcPeer and dstPeer are in level of pods (PodPeer) -// allowed conns are computed considering all the available resources of k8s network policy api: -// admin-network-policies, network-policies and baseline-admin-network-policies -func (pe *PolicyEngine) allAllowedConnectionsBetweenPeers(srcPeer, dstPeer Peer) (*common.ConnectionSet, error) { - srcK8sPeer := srcPeer.(k8s.Peer) - dstK8sPeer := dstPeer.(k8s.Peer) - var res *common.ConnectionSet - 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 - } - // egress: get egress allowed connections between the src and dst by - // walking through all k8s egress policies capturing the src; - // evaluating first ANPs then NPs and finally the BANP - res, err = pe.allAllowedXgressConnections(srcK8sPeer, dstK8sPeer, false) - if err != nil { - return nil, err - } - if res.IsEmpty() { - return res, nil - } - // ingress: get ingress allowed connections between the src and dst by - // walking through all k8s ingress policies capturing the dst; - // evaluating first ANPs then NPs and finally the BANP - ingressRes, err := pe.allAllowedXgressConnections(srcK8sPeer, dstK8sPeer, true) - if err != nil { - return nil, err - } - res.Intersection(ingressRes) - return res, nil -} - // getPod: returns a Pod object corresponding to the input pod name func (pe *PolicyEngine) getPod(p string) *k8s.Pod { if pod, ok := pe.podsMap[p]; ok { @@ -207,50 +107,6 @@ func (pe *PolicyEngine) getPoliciesSelectingPod(peer k8s.Peer, direction netv1.P return res, nil } -// allowedXgressConnections returns true if the given connection from src to dst on given direction(ingress/egress) -// is allowed by network policies rules -func (pe *PolicyEngine) allowedXgressConnection(src, dst k8s.Peer, isIngress bool, protocol, port string) (bool, error) { - // relevant policies: policies that capture dst if isIngress, else policies that capture src - var err error - var netpols []*k8s.NetworkPolicy - if isIngress { - netpols, err = pe.getPoliciesSelectingPod(dst, netv1.PolicyTypeIngress) - } else { - netpols, err = pe.getPoliciesSelectingPod(src, netv1.PolicyTypeEgress) - } - if err != nil { - return false, err - } - - if len(netpols) == 0 { // no networkPolicy captures the relevant pod on the required direction - return true, nil // all connections allowed - } - - // iterate relevant network policies (that capture the required pod) - for _, policy := range netpols { - // if isIngress: check for ingress rules that capture src within 'from' - // if not isIngress: check for egress rules that capture dst within 'to' - if isIngress { - res, err := policy.IngressAllowedConn(src, protocol, port, dst) - if err != nil { - return false, err - } - if res { - return true, nil - } - } else { - res, err := policy.EgressAllowedConn(dst, protocol, port) - if err != nil { - return false, err - } - if res { - return true, nil - } - } - } - return false, nil -} - // isPeerNodeIP returns true if peer1 is an IP address of a node and peer2 is a pod on that node func isPeerNodeIP(peer1, peer2 k8s.Peer) bool { if peer2.PeerType() == k8s.PodType && peer1.PeerType() == k8s.IPBlockType { @@ -312,15 +168,18 @@ func (pe *PolicyEngine) getPeer(p string) (k8s.Peer, error) { return nil, errors.New(netpolerrors.InvalidPeerErrStr(p)) } -// checkIfAllowedNew: (connection-set based computation) returns true if the given input connection is -// allowed by network policies -// currently used only for testing -func (pe *PolicyEngine) checkIfAllowedNew(src, dst, protocol, port string) (bool, error) { - allowedConns, err := pe.allAllowedConnections(src, dst) - if err != nil { - return false, err +// GetPeerExposedTCPConnections returns the tcp connection (ports) exposed by a workload/pod peer +func GetPeerExposedTCPConnections(peer Peer) *common.ConnectionSet { + switch currentPeer := peer.(type) { + case *k8s.IPBlockPeer: + return nil + case *k8s.WorkloadPeer: + return currentPeer.Pod.PodExposedTCPConnections() + case *k8s.PodPeer: + return currentPeer.Pod.PodExposedTCPConnections() + default: + return nil } - return allowedConns.Contains(port, protocol), nil } // allAllowedConnections: returns allowed connection between input strings of src and dst @@ -338,18 +197,77 @@ func (pe *PolicyEngine) allAllowedConnections(src, dst string) (*common.Connecti return allowedConns, err } -// GetPeerExposedTCPConnections returns the tcp connection (ports) exposed by a workload/pod peer -func GetPeerExposedTCPConnections(peer Peer) *common.ConnectionSet { - switch currentPeer := peer.(type) { - case *k8s.IPBlockPeer: - return nil - case *k8s.WorkloadPeer: - return currentPeer.Pod.PodExposedTCPConnections() - case *k8s.PodPeer: - return currentPeer.Pod.PodExposedTCPConnections() - default: - return nil +// AllAllowedConnectionsBetweenWorkloadPeers returns the allowed connections from srcPeer to dstPeer, +// expecting that srcPeer and dstPeer are in level of workloads (WorkloadPeer) +func (pe *PolicyEngine) AllAllowedConnectionsBetweenWorkloadPeers(srcPeer, dstPeer Peer) (*common.ConnectionSet, error) { + if srcPeer.IsPeerIPType() && !dstPeer.IsPeerIPType() { + // assuming dstPeer is WorkloadPeer/RepresentativePeer, should be converted to k8s.Peer + dstPodPeer, err := pe.convertPeerToPodPeer(dstPeer) + if err != nil { + return nil, err + } + return pe.allAllowedConnectionsBetweenPeers(srcPeer, dstPodPeer) + } + if dstPeer.IsPeerIPType() && !srcPeer.IsPeerIPType() { + // assuming srcPeer is WorkloadPeer/RepresentativePeer, should be converted to k8s.Peer + srcPodPeer, err := pe.convertPeerToPodPeer(srcPeer) + if err != nil { + return nil, err + } + return pe.allAllowedConnectionsBetweenPeers(srcPodPeer, dstPeer) } + if !dstPeer.IsPeerIPType() && !srcPeer.IsPeerIPType() { + // assuming srcPeer and dstPeer are WorkloadPeer/RepresentativePeer, should be converted to k8s.Peer + srcPodPeer, err := pe.convertPeerToPodPeer(srcPeer) + if err != nil { + return nil, err + } + dstPodPeer, err := pe.convertPeerToPodPeer(dstPeer) + if err != nil { + return nil, err + } + // if src and dst are the same workload peer, their conversion to pods should be of different pods + // (if owner has more than 1 instances) + if srcPeer.String() == dstPeer.String() { + pe.changePodPeerToAnotherPodObject(dstPodPeer) + } + return pe.allAllowedConnectionsBetweenPeers(srcPodPeer, dstPodPeer) + } + return nil, errors.New(netpolerrors.BothSrcAndDstIPsErrStr(srcPeer.String(), dstPeer.String())) +} + +// allAllowedConnectionsBetweenPeers: returns the allowed connections from srcPeer to dstPeer +// expecting that srcPeer and dstPeer are in level of pods (PodPeer) +// allowed conns are computed considering all the available resources of k8s network policy api: +// admin-network-policies, network-policies and baseline-admin-network-policies +func (pe *PolicyEngine) allAllowedConnectionsBetweenPeers(srcPeer, dstPeer Peer) (*common.ConnectionSet, error) { + srcK8sPeer := srcPeer.(k8s.Peer) + dstK8sPeer := dstPeer.(k8s.Peer) + var res *common.ConnectionSet + 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 + } + // egress: get egress allowed connections between the src and dst by + // walking through all k8s egress policies capturing the src; + // evaluating first ANPs then NPs and finally the BANP + res, err = pe.allAllowedXgressConnections(srcK8sPeer, dstK8sPeer, false) + if err != nil { + return nil, err + } + if res.IsEmpty() { + return res, nil + } + // ingress: get ingress allowed connections between the src and dst by + // walking through all k8s ingress policies capturing the dst; + // evaluating first ANPs then NPs and finally the BANP + ingressRes, err := pe.allAllowedXgressConnections(srcK8sPeer, dstK8sPeer, true) + if err != nil { + return nil, err + } + res.Intersection(ingressRes) + return res, nil } // allAllowedXgressConnections: returns the allowed connections from srcPeer to dstPeer on the diff --git a/pkg/netpol/eval/check_eval.go b/pkg/netpol/eval/check_eval.go new file mode 100644 index 00000000..b012feb4 --- /dev/null +++ b/pkg/netpol/eval/check_eval.go @@ -0,0 +1,257 @@ +/* +Copyright 2023- IBM Inc. All Rights Reserved. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package eval + +import ( + "errors" + + netv1 "k8s.io/api/networking/v1" + + "github.com/np-guard/netpol-analyzer/pkg/internal/netpolerrors" + "github.com/np-guard/netpol-analyzer/pkg/netpol/eval/internal/k8s" +) + +// this file contains methods for checking wether specific connection between two peers is allowed or not; +// those funcs are related to the `eval` command + +// checkIfAllowedNew: (connection-set based computation) returns true if the given input connection is +// allowed by network policies +// currently used only for testing +func (pe *PolicyEngine) checkIfAllowedNew(src, dst, protocol, port string) (bool, error) { + allowedConns, err := pe.allAllowedConnections(src, dst) + if err != nil { + return false, err + } + return allowedConns.Contains(port, protocol), nil +} + +// CheckIfAllowed returns true if the given input connection is allowed by k8s: admin-network-policies, +// network-policies and the baseline-admin-network-policy +func (pe *PolicyEngine) CheckIfAllowed(src, dst, protocol, port string) (bool, error) { + srcPeer, err := pe.getPeer(src) + if err != nil { + return false, err + } + dstPeer, err := pe.getPeer(dst) + if err != nil { + return false, err + } + // cases where any connection is always allowed + if isPodToItself(srcPeer, dstPeer) || isPeerNodeIP(srcPeer, dstPeer) || isPeerNodeIP(dstPeer, srcPeer) { + return true, nil + } + + hasResult, res := pe.cache.hasConnectionResult(srcPeer, dstPeer, protocol, port) + if hasResult { + return res, nil + } + + egressRes, err := pe.allowedXgressConnection(srcPeer, dstPeer, false, protocol, port) + if err != nil { + return false, err + } + if !egressRes { + pe.cache.addConnectionResult(srcPeer, dstPeer, protocol, port, false) + return false, nil + } + ingressRes, err := pe.allowedXgressConnection(srcPeer, dstPeer, true, protocol, port) + if err != nil { + return false, err + } + pe.cache.addConnectionResult(srcPeer, dstPeer, protocol, port, ingressRes) + return ingressRes, nil +} + +// allowedXgressConnection returns if the given input connection is allowed on the given ingress/egress direction +// by k8s policies api +func (pe *PolicyEngine) allowedXgressConnection(src, dst k8s.Peer, isIngress bool, protocol, port string) (bool, error) { + // first checks if the connection is allowed or denied by the admin-network-policies (return anpRes). + // if the connection is passed by the ANPs or not captured by them, then will continue to NPs (pass = true) + anpRes, pass, err := pe.allowedXgressConnectionByAdminNetpols(src, dst, isIngress, protocol, port) + if err != nil { + return false, err + } + if !pass { // i.e the connection is captured by the adminNetworkPolicies and definitely is either allowed or denied + pe.cache.addConnectionResult(src, dst, protocol, port, anpRes) + return anpRes, nil + } + // else pass == true : means that: + // - the admin-network-policies did not capture the connection (if the ANPs rules did not capture the connection explicitly, + // then it is not captured.) + // - or that the rules captured the connection with action: pass; so it will be determined with netpols/ banp. + netpolRes, captured, err := pe.allowedXgressConnectionByNetpols(src, dst, isIngress, protocol, port) + if err != nil { + return false, err + } + // if the src/dst was captured by the relevant xgress policies, then the connection is + // definitely allowed or denied by the policy rules (either explicitly or implicitly) + if captured { + pe.cache.addConnectionResult(src, dst, protocol, port, netpolRes) + return netpolRes, nil + } + // else !captured : means that the xgress connection will be determined by the baseline-admin-network-policy, + // or the system-default for connection that was not captured by any policy which is allowed. + defaultRes, err := pe.allowedXgressByBaselineAdminNetpolOrByDefault(src, dst, isIngress, protocol, port) + if err != nil { + return false, err + } + pe.cache.addConnectionResult(src, dst, protocol, port, defaultRes) + return defaultRes, nil +} + +// allowedXgressConnectionByAdminNetpols returns if the given input connection is allowed on the given ingress/egress direction +// by k8s admin-network-policies +func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, isIngress bool, protocol, port string) (res, pass bool, + err error) { + // iterate sorted by priority admin netpols + for _, anp := range pe.sortedAdminNetpols { + if isIngress { + selectsDst, err := anp.Selects(dst, true) + if err != nil { + return false, false, err + } + if selectsDst { + res, captured, err := anp.CheckIngressConnAllowed(src, dst, protocol, port) + if err != nil { + return false, false, err + } + if !captured { + continue // continue to next ANP + } + return analyzeANPCapturedRes(res) + } + } else { // egress + selectsSrc, err := anp.Selects(src, false) + if err != nil { + return false, false, err + } + if selectsSrc { + res, captured, err := anp.CheckEgressConnAllowed(dst, protocol, port) + if err != nil { + return false, false, err + } + if !captured { + continue // continue to next ANP + } + return analyzeANPCapturedRes(res) + } + } + } + // getting here means the connection was not captured by any ANP - pass to netpols + return false, true, nil +} + +// analyzeANPCapturedRes when an admin-network-policy captures a connection , its result may be Allow (final- allowed conn), +// or Deny (final - denied conn) or Pass (to be determined by netpol/ banp) +func analyzeANPCapturedRes(anpRes int) (allowedOrDenied, pass bool, err error) { + switch anpRes { + case k8s.Pass: + return false, true, nil + case k8s.Allow: + return true, false, nil + case k8s.Deny: + return false, false, nil + } + return false, false, errors.New(netpolerrors.UnknownRuleActionErr) // will not get here +} + +// allowedXgressConnectionByNetpols returns true if the given connection from src to dst on given direction(ingress/egress) +// is allowed by network policies rules +func (pe *PolicyEngine) allowedXgressConnectionByNetpols(src, dst k8s.Peer, isIngress bool, protocol, port string) (res, captured bool, + err error) { + // relevant policies: policies that capture dst if isIngress, else policies that capture src + var netpols []*k8s.NetworkPolicy + if isIngress { + netpols, err = pe.getPoliciesSelectingPod(dst, netv1.PolicyTypeIngress) + } else { + netpols, err = pe.getPoliciesSelectingPod(src, netv1.PolicyTypeEgress) + } + if err != nil { + return false, false, err + } + + if len(netpols) == 0 { // no networkPolicy captures the relevant pod on the required direction + return false, false, nil // result will be determined later by banp / system-default + } + + // iterate relevant network policies (that capture the required pod) + for _, policy := range netpols { + // if isIngress: check for ingress rules that capture src within 'from' + // if not isIngress: check for egress rules that capture dst within 'to' + if isIngress { + res, err := policy.IngressAllowedConn(src, protocol, port, dst) + if err != nil { + return false, false, err + } + if res { + return true, true, nil + } + } else { + res, err := policy.EgressAllowedConn(dst, protocol, port) + if err != nil { + return false, false, err + } + if res { + return true, true, nil + } + } + } + // the src/dst was captured by policies but the given connection is not allowed (so it is implicitly denied) + return false, true, nil +} + +// allowedXgressByBaselineAdminNetpolOrByDefault returns if the given input connection is allowed on the given ingress/egress direction +// by k8s baseline-admin-network-policy; if not captured by the BANP, then returns true as system-default +func (pe *PolicyEngine) allowedXgressByBaselineAdminNetpolOrByDefault(src, dst k8s.Peer, isIngress bool, protocol, + port string) (bool, error) { + if pe.baselineAdminNetpol == nil { + return true, nil // system-default : any non-captured conn is allowed + } + if isIngress { + selectsDst, err := pe.baselineAdminNetpol.Selects(dst, true) + if err != nil { + return false, err + } + if selectsDst { + res, captured, err := pe.baselineAdminNetpol.CheckIngressConnAllowed(src, dst, protocol, port) + if err != nil { + return false, err + } + if !captured { + return true, nil // system-default + } + return analyzeBANPCapturedRes(res) + } + } else { + selectsSrc, err := pe.baselineAdminNetpol.Selects(src, false) + if err != nil { + return false, err + } + if selectsSrc { + res, captured, err := pe.baselineAdminNetpol.CheckEgressConnAllowed(dst, protocol, port) + if err != nil { + return false, err + } + if !captured { + return true, nil // system-default + } + return analyzeBANPCapturedRes(res) + } + } + return true, nil // default +} + +// analyzeBANPCapturedRes when a baseline-admin-network-policy captures a connection , its result may be Allow or Deny +func analyzeBANPCapturedRes(res int) (allowedOrDenied bool, err error) { + switch res { + case k8s.Allow: + return true, nil + case k8s.Deny: + return false, nil + } + return false, errors.New(netpolerrors.UnknownRuleActionErr) // will not get here +} diff --git a/pkg/netpol/eval/eval_test.go b/pkg/netpol/eval/eval_test.go index 05ca2430..c7c4a09a 100644 --- a/pkg/netpol/eval/eval_test.go +++ b/pkg/netpol/eval/eval_test.go @@ -1840,10 +1840,9 @@ func runParsedResourcesEvalTests(t *testing.T, testList []testutils.ParsedResour contProtocol, contPort := pickContainedConn(allowedConns) if contPort != "" { var res bool - // Tanya: uncomment whenever CheckIfAllowed supports ANPs - // res, err := pe.CheckIfAllowed(srcForEval, dstForEval, contProtocol, contPort) - // require.Nil(t, err, test.TestInfo) - // require.Equal(t, true, res, test.TestInfo) + res, err := pe.CheckIfAllowed(src, dst, contProtocol, contPort) + require.Nil(t, err, test.TestInfo) + require.Equal(t, true, res, test.TestInfo) res, err = pe.checkIfAllowedNew(src, dst, contProtocol, contPort) require.Nil(t, err, test.TestInfo) require.Equal(t, true, res, test.TestInfo) @@ -1851,10 +1850,9 @@ func runParsedResourcesEvalTests(t *testing.T, testList []testutils.ParsedResour uncontProtocol, uncontPort := pickUncontainedConn(allowedConns) if uncontPort != "" { var res bool - // Tanya: uncomment whenever CheckIfAllowed supports ANPs - // res, err := pe.CheckIfAllowed(srcForEval, dstForEval, uncontProtocol, uncontPort) - // require.Nil(t, err, test.TestInfo) - // require.Equal(t, false, res, test.TestInfo) + res, err := pe.CheckIfAllowed(src, dst, uncontProtocol, uncontPort) + require.Nil(t, err, test.TestInfo) + require.Equal(t, false, res, test.TestInfo) res, err = pe.checkIfAllowedNew(src, dst, uncontProtocol, uncontPort) require.Nil(t, err, test.TestInfo) require.Equal(t, false, res, test.TestInfo) diff --git a/pkg/netpol/eval/internal/k8s/adminnetpol.go b/pkg/netpol/eval/internal/k8s/adminnetpol.go index e77ac01d..f068c2c7 100644 --- a/pkg/netpol/eval/internal/k8s/adminnetpol.go +++ b/pkg/netpol/eval/internal/k8s/adminnetpol.go @@ -9,6 +9,8 @@ package k8s import ( "errors" "fmt" + "strconv" + "strings" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -261,6 +263,125 @@ func subjectSelectsPeer(anpSubject apisv1a.AdminNetworkPolicySubject, p Peer) (b return doesPodsFieldMatchPeer(anpSubject.Pods, p) } +// anpPortContains returns if the given AdminNetworkPolicyPort selects the input connection +// +//nolint:gosec // memory aliasing in for loop is to fit an old func +//gocyclo:ignore +func anpPortContains(rulePorts *[]apisv1a.AdminNetworkPolicyPort, protocol, port string, dst Peer) (bool, error) { + if rulePorts == nil { + return true, nil // If this field is empty or missing, this rule matches all ports (traffic not restricted by port) + } + intPort, err := strconv.ParseInt(port, portBase, portBits) + if err != nil { + return false, err + } + for _, anpPort := range *rulePorts { + if !onlyOnePortFieldsSet(anpPort) { + return false, errors.New(fmt.Sprintf("Error in Ports : %v", anpPort) + netpolerrors.ANPPortsError) + } + switch { // only one case fits + case anpPort.PortNumber != nil: + if strings.ToUpper(protocol) != getProtocolStr(&anpPort.PortNumber.Protocol) { + continue + } + if int64(anpPort.PortNumber.Port) == intPort { + return true, nil + } + case anpPort.NamedPort != nil: + podProtocol, podPort := dst.GetPeerPod().ConvertPodNamedPort(*anpPort.NamedPort) + if !strings.EqualFold(protocol, podProtocol) { + continue + } + if int64(podPort) == intPort { + return true, nil + } + case anpPort.PortRange != nil: + ruleProtocol := &anpPort.PortRange.Protocol + if strings.ToUpper(protocol) != getProtocolStr(ruleProtocol) { + continue + } + if isEmptyPortRange(anpPort.PortRange.Start, anpPort.PortRange.End) { + return false, nil + } + if intPort >= int64(anpPort.PortRange.Start) && intPort <= int64(anpPort.PortRange.End) { + return true, nil + } + } + } + return false, nil +} + +// checkIfEgressRuleContainsConn check if the egress rule captures the given connection, if yes returns if it is passed/allowed/denied +func checkIfEgressRuleContainsConn(rulePeers []apisv1a.AdminNetworkPolicyEgressPeer, rulePorts *[]apisv1a.AdminNetworkPolicyPort, dst Peer, + action, protocol, port string, isBANPrule bool) (res int, captured bool, err error) { + if len(rulePeers) == 0 { + return 0, false, errors.New(netpolerrors.ANPEgressRulePeersErr) + } + peerSelected, err := egressRuleSelectsPeer(rulePeers, dst) + if err != nil { + return 0, false, err + } + if !peerSelected { + return 0, false, nil + } + connSelected, err := anpPortContains(rulePorts, protocol, port, dst) + if err != nil { + return 0, false, err + } + if !connSelected { + return 0, false, nil + } + // if the protocol and port are in the rulePorts, then action determines what to return + return determineConnResByAction(action, isBANPrule) +} + +// checkIfIngressRuleContainsConn check if the ingress rule captures the given connection, if yes returns if it is passed/allowed/denied +func checkIfIngressRuleContainsConn(rulePeers []apisv1a.AdminNetworkPolicyIngressPeer, rulePorts *[]apisv1a.AdminNetworkPolicyPort, + src, dst Peer, action, protocol, port string, isBANPrule bool) (res int, captured bool, err error) { + if len(rulePeers) == 0 { + return 0, false, errors.New(netpolerrors.ANPIngressRulePeersErr) + } + peerSelected, err := ingressRuleSelectsPeer(rulePeers, src) + if err != nil { + return 0, false, err + } + if !peerSelected { + return 0, false, nil + } + connSelected, err := anpPortContains(rulePorts, protocol, port, dst) + if err != nil { + return 0, false, err + } + if !connSelected { + return 0, false, nil + } + // if the protocol and port are in the rulePorts, then action determines what to return + return determineConnResByAction(action, isBANPrule) +} + +const ( + Pass = iota + Allow + Deny +) + +// determineConnResByAction gets rule action that captured a connection and returns the rule res (allow, pass, deny) +func determineConnResByAction(action string, isBANPrule bool) (res int, captured bool, err error) { + switch action { + case string(apisv1a.AdminNetworkPolicyRuleActionPass): + if isBANPrule { + return -1, false, errors.New(netpolerrors.UnknownRuleActionErr) + } + return Pass, true, nil + case string(apisv1a.AdminNetworkPolicyRuleActionAllow): + return Allow, true, nil + case string(apisv1a.AdminNetworkPolicyRuleActionDeny): + return Deny, true, nil + default: + return -1, false, errors.New(netpolerrors.UnknownRuleActionErr) + } +} + //////////////////////////////////////////////////////////////////////////////////////////// // AdminNetworkPolicy is an alias for k8s adminNetworkPolicy object @@ -346,3 +467,39 @@ func (anp *AdminNetworkPolicy) HasValidPriority() bool { // current implementation satisfies k8s requirement return anp.Spec.Priority >= minPriority && anp.Spec.Priority <= maxPriority } + +// CheckEgressConnAllowed checks if the input conn is allowed/passed/denied or not captured on egress +func (anp *AdminNetworkPolicy) CheckEgressConnAllowed(dst Peer, protocol, port string) (res int, captured bool, err error) { + for _, rule := range anp.Spec.Egress { + rulePeers := rule.To + rulePorts := rule.Ports + ruleRes, captured, err := checkIfEgressRuleContainsConn(rulePeers, rulePorts, dst, string(rule.Action), protocol, port, false) + if err != nil { + return 0, false, anp.anpRuleErr(rule.Name, err.Error()) + } + if !captured { + continue + } + return ruleRes, captured, nil + } + // getting here means the protocol+port was not captured + return Pass, false, nil +} + +// CheckIngressConnAllowed checks if the input conn is allowed/passed/denied or not captured on ingress +func (anp *AdminNetworkPolicy) CheckIngressConnAllowed(src, dst Peer, protocol, port string) (res int, captured bool, err error) { + for _, rule := range anp.Spec.Ingress { + rulePeers := rule.From + rulePorts := rule.Ports + ruleRes, captured, err := checkIfIngressRuleContainsConn(rulePeers, rulePorts, src, dst, string(rule.Action), protocol, port, false) + if err != nil { + return 0, false, anp.anpRuleErr(rule.Name, err.Error()) + } + if !captured { + continue + } + return ruleRes, captured, nil + } + // getting here means the protocol+port was not captured + return Pass, false, nil +} diff --git a/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go b/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go index b4bac4a6..625d3ce7 100644 --- a/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go +++ b/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go @@ -71,3 +71,37 @@ func (banp *BaselineAdminNetworkPolicy) GetIngressPolicyConns(src, dst Peer) (*P } return res, nil } + +func (banp *BaselineAdminNetworkPolicy) CheckEgressConnAllowed(dst Peer, protocol, port string) (res int, captured bool, err error) { + for _, rule := range banp.Spec.Egress { + rulePeers := rule.To + rulePorts := rule.Ports + res, captured, err := checkIfEgressRuleContainsConn(rulePeers, rulePorts, dst, string(rule.Action), protocol, port, true) + if err != nil { + return 0, false, err + } + if !captured { + continue + } + return res, captured, nil + } + // getting here means the protocol+port was not captured + return Allow, false, nil +} + +func (banp *BaselineAdminNetworkPolicy) CheckIngressConnAllowed(src, dst Peer, protocol, port string) (res int, captured bool, err error) { + for _, rule := range banp.Spec.Ingress { + rulePeers := rule.From + rulePorts := rule.Ports + res, captured, err := checkIfIngressRuleContainsConn(rulePeers, rulePorts, src, dst, string(rule.Action), protocol, port, true) + if err != nil { + return 0, false, err + } + if !captured { + continue + } + return res, captured, nil + } + // getting here means the protocol+port was not captured + return Allow, false, nil +} diff --git a/pkg/netpol/eval/internal/k8s/netpol.go b/pkg/netpol/eval/internal/k8s/netpol.go index 331a67bd..475087cb 100644 --- a/pkg/netpol/eval/internal/k8s/netpol.go +++ b/pkg/netpol/eval/internal/k8s/netpol.go @@ -66,7 +66,7 @@ const ( ) func getProtocolStr(p *v1.Protocol) string { - if p == nil { // If not specified, this field defaults to TCP. + if p == nil || string(*p) == "" { // If not specified, this field defaults to TCP. return "TCP" } return string(*p) From 1e8ccceece3606ff7c3fd0eaed6b5c7899550eed Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Mon, 21 Oct 2024 16:00:37 +0300 Subject: [PATCH 02/17] command line tests with anps + updating support for workloads resources --- pkg/cli/command_test.go | 63 +++++++++++++++++++++++++++++++++- pkg/cli/evaluate.go | 33 ++++++++++++++++-- pkg/manifests/parser/k8sobj.go | 40 +++++++++++++++++++++ 3 files changed, 132 insertions(+), 4 deletions(-) diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index 34852b70..6566a748 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -350,7 +350,10 @@ func TestEvalCommandOutput(t *testing.T) { cases := []struct { dir string sourcePod string + sourceNs string + destNs string destPod string + protocol string port string evalResult bool }{ @@ -368,13 +371,71 @@ func TestEvalCommandOutput(t *testing.T) { port: "80", evalResult: false, }, + { + dir: "anp_demo", + sourceNs: "gryffindor", + sourcePod: "harry-potter-1", + destPod: "luna-lovegood-1", + destNs: "ravenclaw", + protocol: "udp", + port: "52", + evalResult: true, + }, + { + dir: "anp_test_6", + sourceNs: "network-policy-conformance-slytherin", + sourcePod: "draco-malfoy-1", + destPod: "cedric-diggory-1", + destNs: "network-policy-conformance-hufflepuff", + protocol: "udp", + port: "5353", + evalResult: false, + }, + { + dir: "anp_test_multiple_anps", + sourceNs: "network-policy-conformance-ravenclaw", + sourcePod: "luna-lovegood-1", + destPod: "draco-malfoy-1", + destNs: "network-policy-conformance-slytherin", + protocol: "sctp", + port: "9003", + evalResult: false, + }, + { + dir: "anp_with_np_and_banp_pass_test", + sourceNs: "ns2", + sourcePod: "pod1-1", + destPod: "pod1-1", + destNs: "ns1", + port: "80", + evalResult: true, + }, + { + dir: "anp_with_np_pass_test", + sourceNs: "ns2", + sourcePod: "pod1-1", + destPod: "pod1-1", + destNs: "ns1", + port: "8080", + evalResult: false, + }, } for _, tt := range cases { tt := tt testName := "eval_" + tt.dir + "_from_" + tt.sourcePod + "_to_" + tt.destPod t.Run(testName, func(t *testing.T) { + if tt.protocol == "" { + tt.protocol = defaultProtocol + } + if tt.sourceNs == "" { + tt.sourceNs = defaultNs + } + if tt.destNs == "" { + tt.destNs = defaultNs + } args := []string{"eval", "--dirpath", testutils.GetTestDirPath(tt.dir), - "-s", tt.sourcePod, "-d", tt.destPod, "-p", tt.port} + "-s", tt.sourcePod, "-d", tt.destPod, "-p", tt.port, "--protocol", tt.protocol, + "-n", tt.sourceNs, "--destination-namespace", tt.destNs} actualOut, err := buildAndExecuteCommand(args) require.Nil(t, err, "test: %q", testName) require.Contains(t, actualOut, fmt.Sprintf("%v", tt.evalResult), diff --git a/pkg/cli/evaluate.go b/pkg/cli/evaluate.go index f99aec3c..70ba6e31 100644 --- a/pkg/cli/evaluate.go +++ b/pkg/cli/evaluate.go @@ -29,11 +29,16 @@ import ( // Currently adds many options flags, so wait until cobra supports something // like NamedFlagSet's. +const ( + defaultNs = "default" + defaultProtocol = "tcp" +) + var ( // evaluated connection information - protocol = "tcp" - sourcePod = types.NamespacedName{Namespace: "default"} - destinationPod = types.NamespacedName{Namespace: "default"} + protocol = defaultProtocol + sourcePod = types.NamespacedName{Namespace: defaultNs} + destinationPod = types.NamespacedName{Namespace: defaultNs} srcExternalIP string dstExternalIP string port string @@ -63,6 +68,7 @@ func validateEvalFlags() error { return nil } +//gocyclo:ignore func updatePolicyEngineObjectsFromDirPath(pe *eval.PolicyEngine, podNames []types.NamespacedName) error { // get relevant resources from dir path eLogger := logger.NewDefaultLoggerWithVerbosity(determineLogVerbosity()) @@ -92,12 +98,33 @@ func updatePolicyEngineObjectsFromDirPath(pe *eval.PolicyEngine, podNames []type for i := range objectsList { obj := objectsList[i] switch obj.Kind { + // workloads kinds case parser.Pod: err = pe.InsertObject(obj.Pod) + case parser.ReplicaSet: + err = pe.InsertObject(obj.ReplicaSet) + case parser.Deployment: + err = pe.InsertObject(obj.Deployment) + case parser.DaemonSet: + err = pe.InsertObject(obj.DaemonSet) + case parser.StatefulSet: + err = pe.InsertObject(obj.StatefulSet) + case parser.ReplicationController: + err = pe.InsertObject(obj.ReplicationController) + case parser.Job: + err = pe.InsertObject(obj.Job) + case parser.CronJob: + err = pe.InsertObject(obj.CronJob) + // ns kind case parser.Namespace: err = pe.InsertObject(obj.Namespace) + // netpols kinds case parser.NetworkPolicy: err = pe.InsertObject(obj.NetworkPolicy) + case parser.AdminNetworkPolicy: + err = pe.InsertObject(obj.AdminNetworkPolicy) + case parser.BaselineAdminNetworkPolicy: + err = pe.InsertObject(obj.BaselineAdminNetworkPolicy) default: continue } diff --git a/pkg/manifests/parser/k8sobj.go b/pkg/manifests/parser/k8sobj.go index a8fe697a..1477c3e8 100644 --- a/pkg/manifests/parser/k8sobj.go +++ b/pkg/manifests/parser/k8sobj.go @@ -211,6 +211,8 @@ var policyKinds = map[string]bool{ BaselineAdminNetworkPolicy: true, } +//nolint:funlen // cases may not be shorten +//gocyclo:ignore func FilterObjectsList(allObjects []K8sObject, podNames []types.NamespacedName) []K8sObject { podNamesMap := make(map[string]bool, 0) nsMap := make(map[string]bool, 0) @@ -234,6 +236,40 @@ func FilterObjectsList(allObjects []K8sObject, podNames []types.NamespacedName) if _, ok := podNamesMap[types.NamespacedName{Name: obj.Pod.Name, Namespace: obj.Pod.Namespace}.String()]; ok { res = append(res, obj) } + // the input pod-names is the name of the pod (replica); + // so if the manifest resources contain a "workload" type (other than Pod); + // the pod name is actually a name of a replica of the workload, and not the name of the workload (obj.Name); + // so here we will compare only namespaces; + // and podName will be compared later after creating the pods (replicas) from the + // workload in the eval package + case StatefulSet: + if _, ok := nsMap[obj.StatefulSet.Namespace]; ok { + res = append(res, obj) + } + case DaemonSet: + if _, ok := nsMap[obj.DaemonSet.Namespace]; ok { + res = append(res, obj) + } + case Deployment: + if _, ok := nsMap[obj.Deployment.Namespace]; ok { + res = append(res, obj) + } + case ReplicaSet: + if _, ok := nsMap[obj.ReplicaSet.Namespace]; ok { + res = append(res, obj) + } + case Job: + if _, ok := nsMap[obj.Job.Namespace]; ok { + res = append(res, obj) + } + case CronJob: + if _, ok := nsMap[obj.CronJob.Namespace]; ok { + res = append(res, obj) + } + case ReplicationController: + if _, ok := nsMap[obj.ReplicationController.Namespace]; ok { + res = append(res, obj) + } case Service: if _, ok := nsMap[obj.Service.Namespace]; ok { res = append(res, obj) @@ -246,6 +282,10 @@ func FilterObjectsList(allObjects []K8sObject, podNames []types.NamespacedName) if _, ok := nsMap[obj.Ingress.Namespace]; ok { res = append(res, obj) } + case AdminNetworkPolicy: + res = append(res, obj) + case BaselineAdminNetworkPolicy: + res = append(res, obj) default: continue } From 3eac6cf972b26648712cf0e399100409ca89a809 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Wed, 30 Oct 2024 14:40:10 +0200 Subject: [PATCH 03/17] adding one more test so all god paths are covered --- pkg/cli/command_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index 6566a748..3e3c3d72 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -419,6 +419,15 @@ func TestEvalCommandOutput(t *testing.T) { port: "8080", evalResult: false, }, + { + dir: "anp_banp_core_test", + sourceNs: "network-policy-conformance-gryffindor", + sourcePod: "harry-potter-1", + destPod: "cedric-diggory-1", + destNs: "network-policy-conformance-hufflepuff", + port: "8080", + evalResult: true, + }, } for _, tt := range cases { tt := tt From ccb1ea7ca672b48ea66f7d5a51cbe0b47a77ce12 Mon Sep 17 00:00:00 2001 From: shireenf-ibm <82180114+shireenf-ibm@users.noreply.github.com> Date: Mon, 4 Nov 2024 14:16:58 +0200 Subject: [PATCH 04/17] Update pkg/netpol/eval/internal/k8s/netpol.go Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com> --- pkg/netpol/eval/internal/k8s/netpol.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/netpol/eval/internal/k8s/netpol.go b/pkg/netpol/eval/internal/k8s/netpol.go index fc460eb5..78645f6d 100644 --- a/pkg/netpol/eval/internal/k8s/netpol.go +++ b/pkg/netpol/eval/internal/k8s/netpol.go @@ -67,7 +67,7 @@ const ( func getProtocolStr(p *v1.Protocol) string { if p == nil || string(*p) == "" { // If not specified, this field defaults to TCP. - return "TCP" + return string(v1.ProtocolTCP) } return string(*p) } From 3d6431ad51a667ddcb7463b009d54e6d39033c04 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Wed, 6 Nov 2024 11:38:35 +0200 Subject: [PATCH 05/17] first fixes --- pkg/cli/evaluate.go | 18 +++-- pkg/netpol/eval/check_eval.go | 54 ++++++-------- pkg/netpol/eval/internal/k8s/adminnetpol.go | 73 ++++++++++--------- .../internal/k8s/baseline_admin_netpol.go | 46 ++++++++---- 4 files changed, 102 insertions(+), 89 deletions(-) diff --git a/pkg/cli/evaluate.go b/pkg/cli/evaluate.go index 70ba6e31..9a1f34ef 100644 --- a/pkg/cli/evaluate.go +++ b/pkg/cli/evaluate.go @@ -10,9 +10,11 @@ import ( "context" "errors" "fmt" + "strings" "time" "github.com/spf13/cobra" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -30,18 +32,18 @@ import ( // like NamedFlagSet's. const ( - defaultNs = "default" - defaultProtocol = "tcp" + defaultNs = metav1.NamespaceDefault ) var ( // evaluated connection information - protocol = defaultProtocol - sourcePod = types.NamespacedName{Namespace: defaultNs} - destinationPod = types.NamespacedName{Namespace: defaultNs} - srcExternalIP string - dstExternalIP string - port string + defaultProtocol = strings.ToLower(string(v1.ProtocolTCP)) + protocol = defaultProtocol + sourcePod = types.NamespacedName{Namespace: defaultNs} + destinationPod = types.NamespacedName{Namespace: defaultNs} + srcExternalIP string + dstExternalIP string + port string ) func validateEvalFlags() error { diff --git a/pkg/netpol/eval/check_eval.go b/pkg/netpol/eval/check_eval.go index b012feb4..195783c0 100644 --- a/pkg/netpol/eval/check_eval.go +++ b/pkg/netpol/eval/check_eval.go @@ -71,11 +71,11 @@ func (pe *PolicyEngine) CheckIfAllowed(src, dst, protocol, port string) (bool, e func (pe *PolicyEngine) allowedXgressConnection(src, dst k8s.Peer, isIngress bool, protocol, port string) (bool, error) { // first checks if the connection is allowed or denied by the admin-network-policies (return anpRes). // if the connection is passed by the ANPs or not captured by them, then will continue to NPs (pass = true) - anpRes, pass, err := pe.allowedXgressConnectionByAdminNetpols(src, dst, isIngress, protocol, port) + anpRes, passOrNonCaptured, err := pe.allowedXgressConnectionByAdminNetpols(src, dst, isIngress, protocol, port) if err != nil { return false, err } - if !pass { // i.e the connection is captured by the adminNetworkPolicies and definitely is either allowed or denied + if !passOrNonCaptured { // i.e the connection is captured by the adminNetworkPolicies and definitely is either allowed or denied pe.cache.addConnectionResult(src, dst, protocol, port, anpRes) return anpRes, nil } @@ -105,8 +105,8 @@ func (pe *PolicyEngine) allowedXgressConnection(src, dst k8s.Peer, isIngress boo // allowedXgressConnectionByAdminNetpols returns if the given input connection is allowed on the given ingress/egress direction // by k8s admin-network-policies -func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, isIngress bool, protocol, port string) (res, pass bool, - err error) { +func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, isIngress bool, protocol, port string) (res, + passOrNonCaptured bool, err error) { // iterate sorted by priority admin netpols for _, anp := range pe.sortedAdminNetpols { if isIngress { @@ -115,14 +115,14 @@ func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, return false, false, err } if selectsDst { - res, captured, err := anp.CheckIngressConnAllowed(src, dst, protocol, port) + res, err := anp.CheckIngressConnAllowed(src, dst, protocol, port) if err != nil { return false, false, err } - if !captured { + if res == k8s.NotCaptured { continue // continue to next ANP } - return analyzeANPCapturedRes(res) + return isAllowedByANPCapturedRes(res) } } else { // egress selectsSrc, err := anp.Selects(src, false) @@ -130,14 +130,14 @@ func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, return false, false, err } if selectsSrc { - res, captured, err := anp.CheckEgressConnAllowed(dst, protocol, port) + res, err := anp.CheckEgressConnAllowed(dst, protocol, port) if err != nil { return false, false, err } - if !captured { + if res == k8s.NotCaptured { continue // continue to next ANP } - return analyzeANPCapturedRes(res) + return isAllowedByANPCapturedRes(res) } } } @@ -145,9 +145,14 @@ func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, return false, true, nil } -// analyzeANPCapturedRes when an admin-network-policy captures a connection , its result may be Allow (final- allowed conn), +// isAllowedByANPCapturedRes when an admin-network-policy captures a connection , its result may be Allow (final- allowed conn), // or Deny (final - denied conn) or Pass (to be determined by netpol/ banp) -func analyzeANPCapturedRes(anpRes int) (allowedOrDenied, pass bool, err error) { +// return value (allowedOrDenied, pass bool, err error) +// * if the given ANP result is Allow or Deny : returns true for allow and false for deny as the value of allowedOrDenied. +// in this case returns pass = false (not important) +// * if the given ANP result is Pass : returns true for pass and false for allow/deny +// as the value of pass is the only important in this case. +func isAllowedByANPCapturedRes(anpRes k8s.ANPRulesResult) (allowedOrDenied, pass bool, err error) { switch anpRes { case k8s.Pass: return false, true, nil @@ -217,14 +222,11 @@ func (pe *PolicyEngine) allowedXgressByBaselineAdminNetpolOrByDefault(src, dst k return false, err } if selectsDst { - res, captured, err := pe.baselineAdminNetpol.CheckIngressConnAllowed(src, dst, protocol, port) + res, err := pe.baselineAdminNetpol.CheckIngressConnAllowed(src, dst, protocol, port) if err != nil { return false, err } - if !captured { - return true, nil // system-default - } - return analyzeBANPCapturedRes(res) + return res, nil } } else { selectsSrc, err := pe.baselineAdminNetpol.Selects(src, false) @@ -232,26 +234,12 @@ func (pe *PolicyEngine) allowedXgressByBaselineAdminNetpolOrByDefault(src, dst k return false, err } if selectsSrc { - res, captured, err := pe.baselineAdminNetpol.CheckEgressConnAllowed(dst, protocol, port) + res, err := pe.baselineAdminNetpol.CheckEgressConnAllowed(dst, protocol, port) if err != nil { return false, err } - if !captured { - return true, nil // system-default - } - return analyzeBANPCapturedRes(res) + return res, nil } } return true, nil // default } - -// analyzeBANPCapturedRes when a baseline-admin-network-policy captures a connection , its result may be Allow or Deny -func analyzeBANPCapturedRes(res int) (allowedOrDenied bool, err error) { - switch res { - case k8s.Allow: - return true, nil - case k8s.Deny: - return false, nil - } - return false, errors.New(netpolerrors.UnknownRuleActionErr) // will not get here -} diff --git a/pkg/netpol/eval/internal/k8s/adminnetpol.go b/pkg/netpol/eval/internal/k8s/adminnetpol.go index 98c38456..1025f693 100644 --- a/pkg/netpol/eval/internal/k8s/adminnetpol.go +++ b/pkg/netpol/eval/internal/k8s/adminnetpol.go @@ -253,7 +253,7 @@ func anpPortContains(rulePorts *[]apisv1a.AdminNetworkPolicyPort, protocol, port } switch { // only one case fits case anpPort.PortNumber != nil: - if strings.ToUpper(protocol) != getProtocolStr(&anpPort.PortNumber.Protocol) { + if !strings.EqualFold(protocol, getProtocolStr(&anpPort.PortNumber.Protocol)) { continue } if int64(anpPort.PortNumber.Port) == intPort { @@ -285,23 +285,23 @@ func anpPortContains(rulePorts *[]apisv1a.AdminNetworkPolicyPort, protocol, port // checkIfEgressRuleContainsConn check if the egress rule captures the given connection, if yes returns if it is passed/allowed/denied func checkIfEgressRuleContainsConn(rulePeers []apisv1a.AdminNetworkPolicyEgressPeer, rulePorts *[]apisv1a.AdminNetworkPolicyPort, dst Peer, - action, protocol, port string, isBANPrule bool) (res int, captured bool, err error) { + action, protocol, port string, isBANPrule bool) (res ANPRulesResult, err error) { if len(rulePeers) == 0 { - return 0, false, errors.New(netpolerrors.ANPEgressRulePeersErr) + return NotCaptured, errors.New(netpolerrors.ANPEgressRulePeersErr) } peerSelected, err := egressRuleSelectsPeer(rulePeers, dst) if err != nil { - return 0, false, err + return NotCaptured, err } if !peerSelected { - return 0, false, nil + return NotCaptured, nil } connSelected, err := anpPortContains(rulePorts, protocol, port, dst) if err != nil { - return 0, false, err + return NotCaptured, err } if !connSelected { - return 0, false, nil + return NotCaptured, nil } // if the protocol and port are in the rulePorts, then action determines what to return return determineConnResByAction(action, isBANPrule) @@ -309,48 +309,53 @@ func checkIfEgressRuleContainsConn(rulePeers []apisv1a.AdminNetworkPolicyEgressP // checkIfIngressRuleContainsConn check if the ingress rule captures the given connection, if yes returns if it is passed/allowed/denied func checkIfIngressRuleContainsConn(rulePeers []apisv1a.AdminNetworkPolicyIngressPeer, rulePorts *[]apisv1a.AdminNetworkPolicyPort, - src, dst Peer, action, protocol, port string, isBANPrule bool) (res int, captured bool, err error) { + src, dst Peer, action, protocol, port string, isBANPrule bool) (res ANPRulesResult, err error) { if len(rulePeers) == 0 { - return 0, false, errors.New(netpolerrors.ANPIngressRulePeersErr) + return NotCaptured, errors.New(netpolerrors.ANPIngressRulePeersErr) } peerSelected, err := ingressRuleSelectsPeer(rulePeers, src) if err != nil { - return 0, false, err + return NotCaptured, err } if !peerSelected { - return 0, false, nil + return NotCaptured, nil } connSelected, err := anpPortContains(rulePorts, protocol, port, dst) if err != nil { - return 0, false, err + return NotCaptured, err } if !connSelected { - return 0, false, nil + return NotCaptured, nil } // if the protocol and port are in the rulePorts, then action determines what to return return determineConnResByAction(action, isBANPrule) } +// ANPRulesResult represents the result of the anp/banp rules to a given connection +// it may be : not-captured, pass (anp only), allow or deny +type ANPRulesResult int + const ( - Pass = iota + NotCaptured ANPRulesResult = iota + Pass Allow Deny ) // determineConnResByAction gets rule action that captured a connection and returns the rule res (allow, pass, deny) -func determineConnResByAction(action string, isBANPrule bool) (res int, captured bool, err error) { +func determineConnResByAction(action string, isBANPrule bool) (res ANPRulesResult, err error) { switch action { case string(apisv1a.AdminNetworkPolicyRuleActionPass): if isBANPrule { - return -1, false, errors.New(netpolerrors.UnknownRuleActionErr) + return NotCaptured, errors.New(netpolerrors.UnknownRuleActionErr) } - return Pass, true, nil + return Pass, nil case string(apisv1a.AdminNetworkPolicyRuleActionAllow): - return Allow, true, nil + return Allow, nil case string(apisv1a.AdminNetworkPolicyRuleActionDeny): - return Deny, true, nil + return Deny, nil default: - return -1, false, errors.New(netpolerrors.UnknownRuleActionErr) + return NotCaptured, errors.New(netpolerrors.UnknownRuleActionErr) } } @@ -440,38 +445,38 @@ func (anp *AdminNetworkPolicy) HasValidPriority() bool { return anp.Spec.Priority >= minANPPriority && anp.Spec.Priority <= maxANPPriority } -// CheckEgressConnAllowed checks if the input conn is allowed/passed/denied or not captured on egress -func (anp *AdminNetworkPolicy) CheckEgressConnAllowed(dst Peer, protocol, port string) (res int, captured bool, err error) { +// CheckEgressConnAllowed checks if the input conn is allowed/passed/denied or not captured on egress by current admin-network-policy +func (anp *AdminNetworkPolicy) CheckEgressConnAllowed(dst Peer, protocol, port string) (res ANPRulesResult, err error) { for _, rule := range anp.Spec.Egress { rulePeers := rule.To rulePorts := rule.Ports - ruleRes, captured, err := checkIfEgressRuleContainsConn(rulePeers, rulePorts, dst, string(rule.Action), protocol, port, false) + ruleRes, err := checkIfEgressRuleContainsConn(rulePeers, rulePorts, dst, string(rule.Action), protocol, port, false) if err != nil { - return 0, false, anp.anpRuleErr(rule.Name, err.Error()) + return NotCaptured, anp.anpRuleErr(rule.Name, err.Error()) } - if !captured { + if ruleRes == NotCaptured { // next rule continue } - return ruleRes, captured, nil + return ruleRes, nil } // getting here means the protocol+port was not captured - return Pass, false, nil + return NotCaptured, nil } -// CheckIngressConnAllowed checks if the input conn is allowed/passed/denied or not captured on ingress -func (anp *AdminNetworkPolicy) CheckIngressConnAllowed(src, dst Peer, protocol, port string) (res int, captured bool, err error) { +// CheckIngressConnAllowed checks if the input conn is allowed/passed/denied or not captured on ingress by current admin-network-policy +func (anp *AdminNetworkPolicy) CheckIngressConnAllowed(src, dst Peer, protocol, port string) (res ANPRulesResult, err error) { for _, rule := range anp.Spec.Ingress { rulePeers := rule.From rulePorts := rule.Ports - ruleRes, captured, err := checkIfIngressRuleContainsConn(rulePeers, rulePorts, src, dst, string(rule.Action), protocol, port, false) + ruleRes, err := checkIfIngressRuleContainsConn(rulePeers, rulePorts, src, dst, string(rule.Action), protocol, port, false) if err != nil { - return 0, false, anp.anpRuleErr(rule.Name, err.Error()) + return NotCaptured, anp.anpRuleErr(rule.Name, err.Error()) } - if !captured { + if ruleRes == NotCaptured { // next rule continue } - return ruleRes, captured, nil + return ruleRes, nil } // getting here means the protocol+port was not captured - return Pass, false, nil + return NotCaptured, nil } diff --git a/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go b/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go index 4baa3167..f8bb577e 100644 --- a/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go +++ b/pkg/netpol/eval/internal/k8s/baseline_admin_netpol.go @@ -7,9 +7,12 @@ SPDX-License-Identifier: Apache-2.0 package k8s import ( + "errors" "fmt" apisv1a "sigs.k8s.io/network-policy-api/apis/v1alpha1" + + "github.com/np-guard/netpol-analyzer/pkg/internal/netpolerrors" ) // BaselineAdminNetworkPolicy is an alias for k8s BaselineAdminNetworkPolicy object @@ -72,36 +75,51 @@ func (banp *BaselineAdminNetworkPolicy) GetIngressPolicyConns(src, dst Peer) (*P return res, nil } -func (banp *BaselineAdminNetworkPolicy) CheckEgressConnAllowed(dst Peer, protocol, port string) (res int, captured bool, err error) { +// CheckEgressConnAllowed checks if the input conn is allowed/denied on egress by the baseline-admin-network-policy; +// note that if the baseline-admin-network-policy does not capture the given connection thus it is allowed by default. +func (banp *BaselineAdminNetworkPolicy) CheckEgressConnAllowed(dst Peer, protocol, port string) (res bool, err error) { for _, rule := range banp.Spec.Egress { rulePeers := rule.To rulePorts := rule.Ports - res, captured, err := checkIfEgressRuleContainsConn(rulePeers, rulePorts, dst, string(rule.Action), protocol, port, true) + res, err := checkIfEgressRuleContainsConn(rulePeers, rulePorts, dst, string(rule.Action), protocol, port, true) if err != nil { - return 0, false, err + return false, err } - if !captured { + if res == NotCaptured { // next rule continue } - return res, captured, nil + return allowedByBANPRules(res) } - // getting here means the protocol+port was not captured - return Allow, false, nil + // getting here means the protocol+port was not captured thus allowed as system-default + return true, nil } -func (banp *BaselineAdminNetworkPolicy) CheckIngressConnAllowed(src, dst Peer, protocol, port string) (res int, captured bool, err error) { +// CheckIngressConnAllowed checks if the input conn is allowed/denied on ingress by the baseline-admin-network-policy; +// note that if the baseline-admin-network-policy does not capture the given connection thus it is allowed by default. +func (banp *BaselineAdminNetworkPolicy) CheckIngressConnAllowed(src, dst Peer, protocol, port string) (res bool, err error) { for _, rule := range banp.Spec.Ingress { rulePeers := rule.From rulePorts := rule.Ports - res, captured, err := checkIfIngressRuleContainsConn(rulePeers, rulePorts, src, dst, string(rule.Action), protocol, port, true) + res, err := checkIfIngressRuleContainsConn(rulePeers, rulePorts, src, dst, string(rule.Action), protocol, port, true) if err != nil { - return 0, false, err + return false, err } - if !captured { + if res == NotCaptured { // next rule continue } - return res, captured, nil + return allowedByBANPRules(res) + } + // getting here means the protocol+port was not captured thus allowed as system-default + return true, nil +} + +// analyzeBANPCapturedRes when a baseline-admin-network-policy captures a connection , its result may be Allow or Deny +func allowedByBANPRules(res ANPRulesResult) (allowedOrDenied bool, err error) { + switch res { + case Allow: + return true, nil + case Deny: + return false, nil } - // getting here means the protocol+port was not captured - return Allow, false, nil + return false, errors.New(netpolerrors.UnknownRuleActionErr) // will not get here } From b171aa692f29d6fa48482b722f44453c635c3fe8 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Wed, 6 Nov 2024 14:44:14 +0200 Subject: [PATCH 06/17] common code --- pkg/netpol/eval/internal/k8s/adminnetpol.go | 26 ++++------- pkg/netpol/eval/internal/k8s/netpol.go | 49 +++++++++++++-------- 2 files changed, 40 insertions(+), 35 deletions(-) diff --git a/pkg/netpol/eval/internal/k8s/adminnetpol.go b/pkg/netpol/eval/internal/k8s/adminnetpol.go index 1025f693..b519a56f 100644 --- a/pkg/netpol/eval/internal/k8s/adminnetpol.go +++ b/pkg/netpol/eval/internal/k8s/adminnetpol.go @@ -10,7 +10,6 @@ import ( "errors" "fmt" "strconv" - "strings" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -197,7 +196,7 @@ func ruleConnections(ports *[]apisv1a.AdminNetworkPolicyPort, dst Peer) (*common if anpPort.PortRange.Protocol != "" { protocol = anpPort.PortRange.Protocol } - if isEmptyPortRange(anpPort.PortRange.Start, anpPort.PortRange.End) { + if isEmptyPortRange(int64(anpPort.PortRange.Start), int64(anpPort.PortRange.End)) { continue // @todo should raise a warning } portSet.AddPortRange(int64(anpPort.PortRange.Start), int64(anpPort.PortRange.End)) @@ -243,6 +242,9 @@ func anpPortContains(rulePorts *[]apisv1a.AdminNetworkPolicyPort, protocol, port if rulePorts == nil { return true, nil // If this field is empty or missing, this rule matches all ports (traffic not restricted by port) } + if protocol == "" && port == "" { + return false, nil // nothing to do + } intPort, err := strconv.ParseInt(port, portBase, portBits) if err != nil { return false, err @@ -253,29 +255,19 @@ func anpPortContains(rulePorts *[]apisv1a.AdminNetworkPolicyPort, protocol, port } switch { // only one case fits case anpPort.PortNumber != nil: - if !strings.EqualFold(protocol, getProtocolStr(&anpPort.PortNumber.Protocol)) { - continue - } - if int64(anpPort.PortNumber.Port) == intPort { + if doesRulePortContain(getProtocolStr(&anpPort.PortNumber.Protocol), protocol, + int64(anpPort.PortNumber.Port), int64(anpPort.PortNumber.Port), intPort) { return true, nil } case anpPort.NamedPort != nil: podProtocol, podPort := dst.GetPeerPod().ConvertPodNamedPort(*anpPort.NamedPort) - if !strings.EqualFold(protocol, podProtocol) { - continue - } - if int64(podPort) == intPort { + if doesRulePortContain(podProtocol, protocol, int64(podPort), int64(podPort), intPort) { return true, nil } case anpPort.PortRange != nil: ruleProtocol := &anpPort.PortRange.Protocol - if strings.ToUpper(protocol) != getProtocolStr(ruleProtocol) { - continue - } - if isEmptyPortRange(anpPort.PortRange.Start, anpPort.PortRange.End) { - return false, nil - } - if intPort >= int64(anpPort.PortRange.Start) && intPort <= int64(anpPort.PortRange.End) { + if doesRulePortContain(getProtocolStr(ruleProtocol), protocol, int64(anpPort.PortRange.Start), + int64(anpPort.PortRange.End), intPort) { return true, nil } } diff --git a/pkg/netpol/eval/internal/k8s/netpol.go b/pkg/netpol/eval/internal/k8s/netpol.go index 78645f6d..c2784fcd 100644 --- a/pkg/netpol/eval/internal/k8s/netpol.go +++ b/pkg/netpol/eval/internal/k8s/netpol.go @@ -76,7 +76,7 @@ func getProtocolStr(p *v1.Protocol) string { // or the port name if it is a named port // if input port is a named port, and the dst peer is nil or does not have a matching named port defined, returns // an empty range represented by (-1,-1) with the named port string -func (np *NetworkPolicy) getPortsRange(rulePort netv1.NetworkPolicyPort, dst Peer) (start, end int32, +func (np *NetworkPolicy) getPortsRange(rulePort netv1.NetworkPolicyPort, dst Peer) (start, end int64, portName string, err error) { if rulePort.Port.Type == intstr.String { // rule.Port is namedPort ruleProtocol := getProtocolStr(rulePort.Protocol) @@ -100,22 +100,37 @@ func (np *NetworkPolicy) getPortsRange(rulePort netv1.NetworkPolicyPort, dst Pee return common.NoPort, common.NoPort, portName, nil } // else, found match for the rule's named-port in the pod's ports, so it may be converted to port number - start = podPortNum - end = podPortNum + start = int64(podPortNum) + end = int64(podPortNum) } else { // rule.Port is number - start = rulePort.Port.IntVal + start = int64(rulePort.Port.IntVal) end = start if rulePort.EndPort != nil { - end = *rulePort.EndPort + end = int64(*rulePort.EndPort) } } return start, end, portName, nil } -func isEmptyPortRange(start, end int32) bool { +func isEmptyPortRange(start, end int64) bool { return start == common.NoPort && end == common.NoPort } +// doesRulePortContain gets protocol and port numbers of a rule and other protocol and port; +// returns if other is contained in the rule's port +func doesRulePortContain(ruleProtocol, otherProtocol string, ruleStartPort, ruleEndPort, otherPort int64) bool { + if !strings.EqualFold(ruleProtocol, otherProtocol) { + return false + } + if isEmptyPortRange(ruleStartPort, ruleEndPort) { + return false + } + if otherPort >= ruleStartPort && otherPort <= ruleEndPort { + return true + } + return false +} + func (np *NetworkPolicy) ruleConnections(rulePorts []netv1.NetworkPolicyPort, dst Peer) (*common.ConnectionSet, error) { if len(rulePorts) == 0 { return common.MakeConnectionSet(true), nil // If this field is empty or missing, this rule matches all ports @@ -154,7 +169,7 @@ func (np *NetworkPolicy) ruleConnections(rulePorts []netv1.NetworkPolicyPort, ds ports.AddPort(intstr.FromString(portName)) } if !isEmptyPortRange(startPort, endPort) { - ports.AddPortRange(int64(startPort), int64(endPort)) + ports.AddPortRange(startPort, endPort) } } res.AddConnection(protocol, ports) @@ -175,10 +190,14 @@ func (np *NetworkPolicy) ruleConnsContain(rulePorts []netv1.NetworkPolicyPort, p if len(rulePorts) == 0 { return true, nil // If this field is empty or missing, this rule matches all ports (traffic not restricted by port) } + if protocol == "" && port == "" { + return false, nil // nothing to do + } + intPort, err := strconv.ParseInt(port, portBase, portBits) + if err != nil { + return false, err + } for i := range rulePorts { - if strings.ToUpper(protocol) != getProtocolStr(rulePorts[i].Protocol) { - continue - } if rulePorts[i].Port == nil { // If this field is not provided, this matches all port names and numbers. return true, nil } @@ -186,14 +205,8 @@ func (np *NetworkPolicy) ruleConnsContain(rulePorts []netv1.NetworkPolicyPort, p if err != nil { return false, err } - if isEmptyPortRange(startPort, endPort) { - return false, nil - } - intPort, err := strconv.ParseInt(port, portBase, portBits) - if err != nil { - return false, err - } - if intPort >= int64(startPort) && intPort <= int64(endPort) { + if doesRulePortContain(getProtocolStr(rulePorts[i].Protocol), protocol, + startPort, endPort, intPort) { return true, nil } } From 31ef6cf78f6a1163cffbae0afa3003e532eb5bf1 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Wed, 6 Nov 2024 14:53:38 +0200 Subject: [PATCH 07/17] revert accepting workloads as input for evaluate cmd-line --- pkg/cli/evaluate.go | 16 ---------------- pkg/manifests/parser/k8sobj.go | 35 ---------------------------------- 2 files changed, 51 deletions(-) diff --git a/pkg/cli/evaluate.go b/pkg/cli/evaluate.go index 9a1f34ef..b0e292fa 100644 --- a/pkg/cli/evaluate.go +++ b/pkg/cli/evaluate.go @@ -100,24 +100,8 @@ func updatePolicyEngineObjectsFromDirPath(pe *eval.PolicyEngine, podNames []type for i := range objectsList { obj := objectsList[i] switch obj.Kind { - // workloads kinds case parser.Pod: err = pe.InsertObject(obj.Pod) - case parser.ReplicaSet: - err = pe.InsertObject(obj.ReplicaSet) - case parser.Deployment: - err = pe.InsertObject(obj.Deployment) - case parser.DaemonSet: - err = pe.InsertObject(obj.DaemonSet) - case parser.StatefulSet: - err = pe.InsertObject(obj.StatefulSet) - case parser.ReplicationController: - err = pe.InsertObject(obj.ReplicationController) - case parser.Job: - err = pe.InsertObject(obj.Job) - case parser.CronJob: - err = pe.InsertObject(obj.CronJob) - // ns kind case parser.Namespace: err = pe.InsertObject(obj.Namespace) // netpols kinds diff --git a/pkg/manifests/parser/k8sobj.go b/pkg/manifests/parser/k8sobj.go index 1477c3e8..5c520fdc 100644 --- a/pkg/manifests/parser/k8sobj.go +++ b/pkg/manifests/parser/k8sobj.go @@ -211,7 +211,6 @@ var policyKinds = map[string]bool{ BaselineAdminNetworkPolicy: true, } -//nolint:funlen // cases may not be shorten //gocyclo:ignore func FilterObjectsList(allObjects []K8sObject, podNames []types.NamespacedName) []K8sObject { podNamesMap := make(map[string]bool, 0) @@ -236,40 +235,6 @@ func FilterObjectsList(allObjects []K8sObject, podNames []types.NamespacedName) if _, ok := podNamesMap[types.NamespacedName{Name: obj.Pod.Name, Namespace: obj.Pod.Namespace}.String()]; ok { res = append(res, obj) } - // the input pod-names is the name of the pod (replica); - // so if the manifest resources contain a "workload" type (other than Pod); - // the pod name is actually a name of a replica of the workload, and not the name of the workload (obj.Name); - // so here we will compare only namespaces; - // and podName will be compared later after creating the pods (replicas) from the - // workload in the eval package - case StatefulSet: - if _, ok := nsMap[obj.StatefulSet.Namespace]; ok { - res = append(res, obj) - } - case DaemonSet: - if _, ok := nsMap[obj.DaemonSet.Namespace]; ok { - res = append(res, obj) - } - case Deployment: - if _, ok := nsMap[obj.Deployment.Namespace]; ok { - res = append(res, obj) - } - case ReplicaSet: - if _, ok := nsMap[obj.ReplicaSet.Namespace]; ok { - res = append(res, obj) - } - case Job: - if _, ok := nsMap[obj.Job.Namespace]; ok { - res = append(res, obj) - } - case CronJob: - if _, ok := nsMap[obj.CronJob.Namespace]; ok { - res = append(res, obj) - } - case ReplicationController: - if _, ok := nsMap[obj.ReplicationController.Namespace]; ok { - res = append(res, obj) - } case Service: if _, ok := nsMap[obj.Service.Namespace]; ok { res = append(res, obj) From 20aae297658087f1b04b0fc99e4fe2eae5a3d906 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Sun, 10 Nov 2024 20:24:41 +0200 Subject: [PATCH 08/17] adding tests from dir to command and eval tests; command-line needs generating pod files --- go.mod | 2 +- pkg/cli/command_test.go | 146 +++++++------- pkg/internal/testutils/generate_pod_yamls.go | 188 +++++++++++++++++++ pkg/netpol/eval/eval_test.go | 102 ++++++++++ 4 files changed, 373 insertions(+), 65 deletions(-) create mode 100644 pkg/internal/testutils/generate_pod_yamls.go diff --git a/go.mod b/go.mod index 4d23cb61..c81a00be 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/openshift/api v0.0.0-20230502160752-c71432710382 github.com/spf13/cobra v1.8.1 github.com/stretchr/testify v1.9.0 + gopkg.in/yaml.v2 v2.4.0 k8s.io/api v0.29.2 k8s.io/apimachinery v0.29.2 k8s.io/cli-runtime v0.29.2 @@ -56,7 +57,6 @@ require ( google.golang.org/appengine v1.6.7 // indirect google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect - gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/klog/v2 v2.110.1 // indirect k8s.io/kube-openapi v0.0.0-20231010175941-2dd684a91f00 // indirect diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index 3e3c3d72..9182d7c7 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -348,14 +348,15 @@ func TestDiffCommandOutput(t *testing.T) { // TestEvalCommandOutput tests the output of legal eval command func TestEvalCommandOutput(t *testing.T) { cases := []struct { - dir string - sourcePod string - sourceNs string - destNs string - destPod string - protocol string - port string - evalResult bool + dir string + sourcePod string + sourceNs string + destNs string + destPod string + protocol string + port string + evalResult bool + generateManifests bool // indicates if the test dir does not contain pods - to be generated }{ { dir: "onlineboutique", @@ -372,61 +373,67 @@ func TestEvalCommandOutput(t *testing.T) { evalResult: false, }, { - dir: "anp_demo", - sourceNs: "gryffindor", - sourcePod: "harry-potter-1", - destPod: "luna-lovegood-1", - destNs: "ravenclaw", - protocol: "udp", - port: "52", - evalResult: true, - }, - { - dir: "anp_test_6", - sourceNs: "network-policy-conformance-slytherin", - sourcePod: "draco-malfoy-1", - destPod: "cedric-diggory-1", - destNs: "network-policy-conformance-hufflepuff", - protocol: "udp", - port: "5353", - evalResult: false, - }, - { - dir: "anp_test_multiple_anps", - sourceNs: "network-policy-conformance-ravenclaw", - sourcePod: "luna-lovegood-1", - destPod: "draco-malfoy-1", - destNs: "network-policy-conformance-slytherin", - protocol: "sctp", - port: "9003", - evalResult: false, - }, - { - dir: "anp_with_np_and_banp_pass_test", - sourceNs: "ns2", - sourcePod: "pod1-1", - destPod: "pod1-1", - destNs: "ns1", - port: "80", - evalResult: true, - }, - { - dir: "anp_with_np_pass_test", - sourceNs: "ns2", - sourcePod: "pod1-1", - destPod: "pod1-1", - destNs: "ns1", - port: "8080", - evalResult: false, - }, - { - dir: "anp_banp_core_test", - sourceNs: "network-policy-conformance-gryffindor", - sourcePod: "harry-potter-1", - destPod: "cedric-diggory-1", - destNs: "network-policy-conformance-hufflepuff", - port: "8080", - evalResult: true, + dir: "anp_demo", + sourceNs: "gryffindor", + sourcePod: "harry-potter", + destPod: "luna-lovegood", + destNs: "ravenclaw", + protocol: "udp", + port: "52", + evalResult: true, + generateManifests: true, + }, + { + dir: "anp_test_6", + sourceNs: "network-policy-conformance-slytherin", + sourcePod: "draco-malfoy", + destPod: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + protocol: "udp", + port: "5353", + evalResult: false, + generateManifests: true, + }, + { + dir: "anp_test_multiple_anps", + sourceNs: "network-policy-conformance-ravenclaw", + sourcePod: "luna-lovegood", + destPod: "draco-malfoy", + destNs: "network-policy-conformance-slytherin", + protocol: "sctp", + port: "9003", + evalResult: false, + generateManifests: true, + }, + { + dir: "anp_with_np_and_banp_pass_test", + sourceNs: "ns2", + sourcePod: "pod1", + destPod: "pod1", + destNs: "ns1", + port: "80", + evalResult: true, + generateManifests: true, + }, + { + dir: "anp_with_np_pass_test", + sourceNs: "ns2", + sourcePod: "pod1", + destPod: "pod1", + destNs: "ns1", + port: "8080", + evalResult: false, + generateManifests: true, + }, + { + dir: "anp_banp_core_test", + sourceNs: "network-policy-conformance-gryffindor", + sourcePod: "harry-potter", + destPod: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + port: "8080", + evalResult: true, + generateManifests: true, }, } for _, tt := range cases { @@ -442,7 +449,18 @@ func TestEvalCommandOutput(t *testing.T) { if tt.destNs == "" { tt.destNs = defaultNs } - args := []string{"eval", "--dirpath", testutils.GetTestDirPath(tt.dir), + dirPath := testutils.GetTestDirPath(tt.dir) + var err error + if tt.generateManifests { + // getting here means the test dir contains workloads in the manifests (not pods) + // but since eval command only supports pods, we will generate a copy of the dirs with + // pods yaml files from the matching workload resource of the tt's source and dst. + // so the command may be executed with the given args + dirPath, err = testutils.GenerateTempDirWithPods(dirPath, tt.sourcePod, tt.sourceNs, tt.destPod, tt.destNs) + require.Nil(t, err, "test: %q", testName) + defer os.RemoveAll(dirPath) // clean up after finishing the test + } + args := []string{"eval", "--dirpath", dirPath, "-s", tt.sourcePod, "-d", tt.destPod, "-p", tt.port, "--protocol", tt.protocol, "-n", tt.sourceNs, "--destination-namespace", tt.destNs} actualOut, err := buildAndExecuteCommand(args) diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go new file mode 100644 index 00000000..2249914c --- /dev/null +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -0,0 +1,188 @@ +/* +Copyright 2023- IBM Inc. All Rights Reserved. + +SPDX-License-Identifier: Apache-2.0 +*/ +package testutils + +import ( + "bytes" + "os" + "path/filepath" + "text/template" + + "gopkg.in/yaml.v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// a test util for eval command-line; generating a new temporary directory with pod files to be used for testing. +// eval command supports only test directories with Pod objects. +// some of our testing directories are containing other Workload types in the manifests; in order to be able to test +// eval command on those dirs (in unit-testing); this file funcs are used. +// we copy the test-die into a new temporary dir, and generate into the tempDir : +// Pod yaml files for given src and dst peers from their workload resources + +// GenerateTempDirWithPods generates new temporary dir that copies origDir and adds yaml files of Pod kind +// matching the input workload resources of the src and dst +// the function returns the path of the generated temp dir. +func GenerateTempDirWithPods(origDir, srcName, srcNs, dstName, dstNs string) (string, error) { + // making temp directory in the temp path + tmpDir, err := os.MkdirTemp("", "temp-*") + if err != nil { + return "", err + } + // copy orig dir into the temp dir + err = copyDir(origDir, tmpDir, srcName, srcNs, dstName, dstNs) + return tmpDir, err +} + +// copyDir copies files of network-policies from origDir into tempDir +// and generates into the tempDir : Pod yaml files for given src and dst peers from their workload resources +// in the origDir +func copyDir(origDir, tempDir, srcName, srcNs, dstName, dstNs string) error { + return filepath.Walk(origDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { // nothing to do + return nil + } + origFile := filepath.Join(origDir, info.Name()) + tempFile := filepath.Join(tempDir, info.Name()) + // if the file contains workload with given srcName or dstName : get the workload object and + // generate matching pod yaml file in the temp dir + // this is needed since we need also a copy of the workload labels which may be used in the netpols rules + + // find src and get its labels + srcLabels, err := checkFileContainsInputWorkloadAndGetItsLabels(origFile, srcName, srcNs) + if err != nil { + return err + } + if srcLabels != nil { // nil means the src was not found in that file + err = generatePodYaml(tempDir, srcName, srcNs, srcLabels) + if err != nil { + return err + } + } + + // find dst and get its labels + dstLabels, err := checkFileContainsInputWorkloadAndGetItsLabels(origFile, dstName, dstNs) + if err != nil { + return err + } + if dstLabels != nil { // nil means the dst was not found in that file + err = generatePodYaml(tempDir, dstName, dstNs, dstLabels) + if err != nil { + return err + } + } + // copy the orig file (having objects with kinds other than ns, pod, netpols will not affect the result + // since it will not be parsed with the eval command) + return copyFile(origFile, tempFile) + }) +} + +// PodInfo contains metadata of the pod so we can : +// 1. extract relevant workload from input resources +// 2. generate relevant pod template +type PodInfo struct { + Name string `yaml:"name"` + Namespace string `yaml:"namespace,omitempty"` + Labels map[string]string `yaml:"labels,omitempty"` +} + +// workloadMetadata the yaml is unmarshal to workload metadata struct which is the only interesting part for our goals +type WorkloadMetadata struct { + Metadata PodInfo `yaml:"metadata"` +} + +// checkFileContainsInputWorkloadAndGetItsLabels gets yaml contents and checks if it contains a workload object with the +// given name and namespace +// note that this assumes that if the object name and namespace matches the input, it is the workload object +// since our tests-dirs contain unique names for workloads (different than policies names) +// @todo : verify the kind is of a workload type too +func checkFileContainsInputWorkloadAndGetItsLabels(origFile, podName, podNs string) (map[string]string, error) { + fileContents, err := os.ReadFile(origFile) + if err != nil { + return nil, err + } + // Splitting the YAML into multiple documents + docs := splitYamlDocs(fileContents) + + // we are interested in Metadata of the workload only. + // Iterate through objects to find the matching one + for _, doc := range docs { + var obj WorkloadMetadata + if err := yaml.Unmarshal(doc, &obj); err != nil { + return nil, err + } + if obj.Metadata.Name == podName && (obj.Metadata.Namespace == podNs || + (obj.Metadata.Namespace == "" && podNs == metav1.NamespaceDefault)) { + if obj.Metadata.Labels == nil { + return map[string]string{}, nil + } + return obj.Metadata.Labels, nil + } + } + return nil, nil +} + +const yamlSep = "---" + +// splitYamlDocs splits a YAML file into separate documents. +// It returns a slice of byte slices, where each byte slice represents a YAML document. +func splitYamlDocs(data []byte) (docs [][]byte) { + // Split on YAML document separator + for _, docYAML := range bytes.Split(data, []byte(yamlSep)) { + if len(bytes.TrimSpace(docYAML)) == 0 { + continue + } + docs = append(docs, docYAML) + } + return docs +} + +// copyFile copies origFile to tempFile +func copyFile(origFile, tempFile string) error { + contents, err := os.ReadFile(origFile) + if err != nil { + return err + } + err = os.WriteFile(tempFile, contents, os.ModeAppend) + return err +} + +const podYamlTemplate = `apiVersion: v1 +kind: Pod +metadata: + name: {{ .Name }} + namespace: {{ .Namespace }} + labels: + {{- range $key, $value := .Labels }} + {{ $key }}: {{ $value }} + {{- end }} +spec: + containers: + - name: container-1 + image: nginx:latest +` + +const yamlSuffix = ".yaml" + +// generatePodYaml generates a YAML file for a given pod data. +func generatePodYaml(dirName, podName, podNs string, labels map[string]string) error { + pod := PodInfo{Name: podName, Namespace: podNs, Labels: labels} + fileName := podNs + "_" + podName + yamlSuffix + podFile := filepath.Join(dirName, fileName) + // write the pod template using the pod data + podTmpl, err := template.New("pod").Parse(podYamlTemplate) + if err != nil { + return err + } + var buf bytes.Buffer + if err := podTmpl.Execute(&buf, pod); err != nil { + return err + } + // write to file + return os.WriteFile(podFile, buf.Bytes(), os.ModeAppend) +} diff --git a/pkg/netpol/eval/eval_test.go b/pkg/netpol/eval/eval_test.go index 19ddfb1b..b7ee2eff 100644 --- a/pkg/netpol/eval/eval_test.go +++ b/pkg/netpol/eval/eval_test.go @@ -1870,3 +1870,105 @@ func TestAllParsedResourcesEvalTests(t *testing.T) { runParsedResourcesEvalTests(t, examples.BANPWithNetPolV1FromParsedResourcesTest) runParsedResourcesEvalTests(t, examples.ANPWithBANPFromParsedResourcesTest) } + +func TestDirPathEvalResults(t *testing.T) { + cases := []struct { + dir string + sourcePod string + sourceNs string + destNs string + destPod string + protocol string + port string + evalResult bool + }{ + { + dir: "anp_demo", + sourceNs: "gryffindor", + sourcePod: "harry-potter", + destPod: "luna-lovegood", + destNs: "ravenclaw", + protocol: "udp", + port: "52", + evalResult: true, + }, + { + dir: "anp_test_6", + sourceNs: "network-policy-conformance-slytherin", + sourcePod: "draco-malfoy", + destPod: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + protocol: "udp", + port: "5353", + evalResult: false, + }, + { + dir: "anp_test_multiple_anps", + sourceNs: "network-policy-conformance-ravenclaw", + sourcePod: "luna-lovegood", + destPod: "draco-malfoy", + destNs: "network-policy-conformance-slytherin", + protocol: "sctp", + port: "9003", + evalResult: false, + }, + { + dir: "anp_with_np_and_banp_pass_test", + sourceNs: "ns2", + sourcePod: "pod1", + destPod: "pod1", + destNs: "ns1", + port: "80", + evalResult: true, + }, + { + dir: "anp_with_np_pass_test", + sourceNs: "ns2", + sourcePod: "pod1", + destPod: "pod1", + destNs: "ns1", + port: "8080", + evalResult: false, + }, + { + dir: "anp_banp_core_test", + sourceNs: "network-policy-conformance-gryffindor", + sourcePod: "harry-potter", + destPod: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + port: "8080", + evalResult: true, + }, + } + for _, tt := range cases { + tt := tt + testName := "eval_" + tt.dir + "_from_" + tt.sourcePod + "_to_" + tt.destPod + t.Run(testName, func(t *testing.T) { + t.Parallel() + if tt.protocol == "" { + tt.protocol = strings.ToLower(string(v1.ProtocolTCP)) + } + path := testutils.GetTestDirPath(tt.dir) + rList, errs := fsscanner.GetResourceInfosFromDirPath([]string{path}, true, false) + require.Empty(t, errs, "test: %q", testName) + objectsList, processingErrs := parser.ResourceInfoListToK8sObjectsList(rList, logger.NewDefaultLogger(), false) + require.Empty(t, processingErrs, "test: %q", testName) + pe, err := NewPolicyEngineWithObjects(objectsList) + require.Nil(t, err, "test: %q", testName) + var src, dst string + for podStr, podObj := range pe.podsMap { + if podObj.Owner.Name == tt.sourcePod && podObj.Namespace == tt.sourceNs { + src = podStr + } + if podObj.Owner.Name == tt.destPod && podObj.Namespace == tt.destNs { + dst = podStr + } + } + require.NotEmpty(t, src, "test %q, could not find pod for %s", testName, tt.sourcePod) + require.NotEmpty(t, dst, "test %q, could not find pod for %s", testName, tt.destPod) + res, err := pe.CheckIfAllowed(src, dst, tt.protocol, tt.port) + require.Nil(t, err, "test: %q", testName) + require.Equal(t, tt.evalResult, res, "unexpected result for test %q, should be %v", testName, tt.evalResult) + }) + } +} From fedb42215e40caff6a17d1a4e31d1bc800526a15 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Sun, 10 Nov 2024 21:25:26 +0200 Subject: [PATCH 09/17] generating the tmp dir in the project path, since permissions are denied on github to the "general" tmp dir --- pkg/cli/command_test.go | 2 +- pkg/internal/testutils/generate_pod_yamls.go | 22 +++++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index 9182d7c7..e5d124d6 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -458,7 +458,7 @@ func TestEvalCommandOutput(t *testing.T) { // so the command may be executed with the given args dirPath, err = testutils.GenerateTempDirWithPods(dirPath, tt.sourcePod, tt.sourceNs, tt.destPod, tt.destNs) require.Nil(t, err, "test: %q", testName) - defer os.RemoveAll(dirPath) // clean up after finishing the test + defer os.RemoveAll(testutils.TmpDir) // clean up after finishing the test } args := []string{"eval", "--dirpath", dirPath, "-s", tt.sourcePod, "-d", tt.destPod, "-p", tt.port, "--protocol", tt.protocol, diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go index 2249914c..69287eb8 100644 --- a/pkg/internal/testutils/generate_pod_yamls.go +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -13,6 +13,8 @@ import ( "gopkg.in/yaml.v2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/np-guard/netpol-analyzer/pkg/internal/projectpath" ) // a test util for eval command-line; generating a new temporary directory with pod files to be used for testing. @@ -22,18 +24,32 @@ import ( // we copy the test-die into a new temporary dir, and generate into the tempDir : // Pod yaml files for given src and dst peers from their workload resources +const ( + tmpPattern = "temp-*" + exeMode = 0o777 +) + +var TmpDir = filepath.Join(projectpath.Root, "temp") // cleaned up after the test is done + // GenerateTempDirWithPods generates new temporary dir that copies origDir and adds yaml files of Pod kind // matching the input workload resources of the src and dst // the function returns the path of the generated temp dir. func GenerateTempDirWithPods(origDir, srcName, srcNs, dstName, dstNs string) (string, error) { + // create the TmpDir path if does not exist + if _, err := os.Stat(TmpDir); os.IsNotExist(err) { + osErr := os.Mkdir(TmpDir, exeMode) + if osErr != nil { + return "", osErr + } + } // making temp directory in the temp path - tmpDir, err := os.MkdirTemp("", "temp-*") + testTmpDir, err := os.MkdirTemp(TmpDir, tmpPattern) if err != nil { return "", err } // copy orig dir into the temp dir - err = copyDir(origDir, tmpDir, srcName, srcNs, dstName, dstNs) - return tmpDir, err + err = copyDir(origDir, testTmpDir, srcName, srcNs, dstName, dstNs) + return testTmpDir, err } // copyDir copies files of network-policies from origDir into tempDir From 279b75963ee98300949e32e384bcb3f9844d2d04 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Sun, 10 Nov 2024 21:31:33 +0200 Subject: [PATCH 10/17] fixes to fit github permissions --- pkg/internal/testutils/generate_pod_yamls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go index 69287eb8..50d63334 100644 --- a/pkg/internal/testutils/generate_pod_yamls.go +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -164,7 +164,7 @@ func copyFile(origFile, tempFile string) error { if err != nil { return err } - err = os.WriteFile(tempFile, contents, os.ModeAppend) + err = os.WriteFile(tempFile, contents, exeMode) return err } @@ -200,5 +200,5 @@ func generatePodYaml(dirName, podName, podNs string, labels map[string]string) e return err } // write to file - return os.WriteFile(podFile, buf.Bytes(), os.ModeAppend) + return os.WriteFile(podFile, buf.Bytes(), exeMode) } From 480676e82d7302b8c7b8af07e22d217f6e225f49 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 11:07:40 +0200 Subject: [PATCH 11/17] comments + changing mode --- pkg/cli/command_test.go | 2 ++ pkg/internal/testutils/generate_pod_yamls.go | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index e5d124d6..f69412a3 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -357,6 +357,7 @@ func TestEvalCommandOutput(t *testing.T) { port string evalResult bool generateManifests bool // indicates if the test dir does not contain pods - to be generated + // this field will be used till the eval command supports workload inputs too (not just pods) }{ { dir: "onlineboutique", @@ -451,6 +452,7 @@ func TestEvalCommandOutput(t *testing.T) { } dirPath := testutils.GetTestDirPath(tt.dir) var err error + // TODO: following "if" will be deprecated when eval supports input workloads, not just pods if tt.generateManifests { // getting here means the test dir contains workloads in the manifests (not pods) // but since eval command only supports pods, we will generate a copy of the dirs with diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go index 50d63334..e3495772 100644 --- a/pkg/internal/testutils/generate_pod_yamls.go +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -23,10 +23,11 @@ import ( // eval command on those dirs (in unit-testing); this file funcs are used. // we copy the test-die into a new temporary dir, and generate into the tempDir : // Pod yaml files for given src and dst peers from their workload resources +// note that : this file will not be used when the eval command supports workload inputs (and not just pods) const ( tmpPattern = "temp-*" - exeMode = 0o777 + exeMode = 0o600 ) var TmpDir = filepath.Join(projectpath.Root, "temp") // cleaned up after the test is done From d5406d1244979f048692980c245c9ab60373bad3 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 11:32:04 +0200 Subject: [PATCH 12/17] renaming struct field --- pkg/cli/command_test.go | 142 ++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 71 deletions(-) diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index f69412a3..1255c54a 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -348,15 +348,15 @@ func TestDiffCommandOutput(t *testing.T) { // TestEvalCommandOutput tests the output of legal eval command func TestEvalCommandOutput(t *testing.T) { cases := []struct { - dir string - sourcePod string - sourceNs string - destNs string - destPod string - protocol string - port string - evalResult bool - generateManifests bool // indicates if the test dir does not contain pods - to be generated + dir string + sourcePod string + sourceNs string + destNs string + destPod string + protocol string + port string + evalResult bool + generatePodManifests bool // indicates if the test dir does not contain pods - to be generated // this field will be used till the eval command supports workload inputs too (not just pods) }{ { @@ -374,67 +374,67 @@ func TestEvalCommandOutput(t *testing.T) { evalResult: false, }, { - dir: "anp_demo", - sourceNs: "gryffindor", - sourcePod: "harry-potter", - destPod: "luna-lovegood", - destNs: "ravenclaw", - protocol: "udp", - port: "52", - evalResult: true, - generateManifests: true, - }, - { - dir: "anp_test_6", - sourceNs: "network-policy-conformance-slytherin", - sourcePod: "draco-malfoy", - destPod: "cedric-diggory", - destNs: "network-policy-conformance-hufflepuff", - protocol: "udp", - port: "5353", - evalResult: false, - generateManifests: true, - }, - { - dir: "anp_test_multiple_anps", - sourceNs: "network-policy-conformance-ravenclaw", - sourcePod: "luna-lovegood", - destPod: "draco-malfoy", - destNs: "network-policy-conformance-slytherin", - protocol: "sctp", - port: "9003", - evalResult: false, - generateManifests: true, - }, - { - dir: "anp_with_np_and_banp_pass_test", - sourceNs: "ns2", - sourcePod: "pod1", - destPod: "pod1", - destNs: "ns1", - port: "80", - evalResult: true, - generateManifests: true, - }, - { - dir: "anp_with_np_pass_test", - sourceNs: "ns2", - sourcePod: "pod1", - destPod: "pod1", - destNs: "ns1", - port: "8080", - evalResult: false, - generateManifests: true, - }, - { - dir: "anp_banp_core_test", - sourceNs: "network-policy-conformance-gryffindor", - sourcePod: "harry-potter", - destPod: "cedric-diggory", - destNs: "network-policy-conformance-hufflepuff", - port: "8080", - evalResult: true, - generateManifests: true, + dir: "anp_demo", + sourceNs: "gryffindor", + sourcePod: "harry-potter", + destPod: "luna-lovegood", + destNs: "ravenclaw", + protocol: "udp", + port: "52", + evalResult: true, + generatePodManifests: true, + }, + { + dir: "anp_test_6", + sourceNs: "network-policy-conformance-slytherin", + sourcePod: "draco-malfoy", + destPod: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + protocol: "udp", + port: "5353", + evalResult: false, + generatePodManifests: true, + }, + { + dir: "anp_test_multiple_anps", + sourceNs: "network-policy-conformance-ravenclaw", + sourcePod: "luna-lovegood", + destPod: "draco-malfoy", + destNs: "network-policy-conformance-slytherin", + protocol: "sctp", + port: "9003", + evalResult: false, + generatePodManifests: true, + }, + { + dir: "anp_with_np_and_banp_pass_test", + sourceNs: "ns2", + sourcePod: "pod1", + destPod: "pod1", + destNs: "ns1", + port: "80", + evalResult: true, + generatePodManifests: true, + }, + { + dir: "anp_with_np_pass_test", + sourceNs: "ns2", + sourcePod: "pod1", + destPod: "pod1", + destNs: "ns1", + port: "8080", + evalResult: false, + generatePodManifests: true, + }, + { + dir: "anp_banp_core_test", + sourceNs: "network-policy-conformance-gryffindor", + sourcePod: "harry-potter", + destPod: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + port: "8080", + evalResult: true, + generatePodManifests: true, }, } for _, tt := range cases { @@ -453,7 +453,7 @@ func TestEvalCommandOutput(t *testing.T) { dirPath := testutils.GetTestDirPath(tt.dir) var err error // TODO: following "if" will be deprecated when eval supports input workloads, not just pods - if tt.generateManifests { + if tt.generatePodManifests { // getting here means the test dir contains workloads in the manifests (not pods) // but since eval command only supports pods, we will generate a copy of the dirs with // pods yaml files from the matching workload resource of the tt's source and dst. From 957e4f0b7fd8575430efb9a45803c1a629816352 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 11:36:07 +0200 Subject: [PATCH 13/17] updating mode fields --- pkg/internal/testutils/generate_pod_yamls.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go index e3495772..9bebdb29 100644 --- a/pkg/internal/testutils/generate_pod_yamls.go +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -27,7 +27,8 @@ import ( const ( tmpPattern = "temp-*" - exeMode = 0o600 + fileMode = 0o600 + dirMode = 0o700 ) var TmpDir = filepath.Join(projectpath.Root, "temp") // cleaned up after the test is done @@ -38,7 +39,7 @@ var TmpDir = filepath.Join(projectpath.Root, "temp") // cleaned up after the tes func GenerateTempDirWithPods(origDir, srcName, srcNs, dstName, dstNs string) (string, error) { // create the TmpDir path if does not exist if _, err := os.Stat(TmpDir); os.IsNotExist(err) { - osErr := os.Mkdir(TmpDir, exeMode) + osErr := os.Mkdir(TmpDir, dirMode) if osErr != nil { return "", osErr } @@ -165,7 +166,7 @@ func copyFile(origFile, tempFile string) error { if err != nil { return err } - err = os.WriteFile(tempFile, contents, exeMode) + err = os.WriteFile(tempFile, contents, fileMode) return err } @@ -201,5 +202,5 @@ func generatePodYaml(dirName, podName, podNs string, labels map[string]string) e return err } // write to file - return os.WriteFile(podFile, buf.Bytes(), exeMode) + return os.WriteFile(podFile, buf.Bytes(), fileMode) } From 0f4520ae56efeb6b577b0e0700f14ff8c4c46d9a Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 11:51:49 +0200 Subject: [PATCH 14/17] rename func --- pkg/internal/testutils/generate_pod_yamls.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go index 9bebdb29..b1feb23a 100644 --- a/pkg/internal/testutils/generate_pod_yamls.go +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -49,15 +49,15 @@ func GenerateTempDirWithPods(origDir, srcName, srcNs, dstName, dstNs string) (st if err != nil { return "", err } - // copy orig dir into the temp dir - err = copyDir(origDir, testTmpDir, srcName, srcNs, dstName, dstNs) + // copy orig dir into the temp dir and add to temp dir generated pods + err = copyDirAndAddPods(origDir, testTmpDir, srcName, srcNs, dstName, dstNs) return testTmpDir, err } -// copyDir copies files of network-policies from origDir into tempDir +// copyDirAndAddPods copies files of network-policies from origDir into tempDir // and generates into the tempDir : Pod yaml files for given src and dst peers from their workload resources // in the origDir -func copyDir(origDir, tempDir, srcName, srcNs, dstName, dstNs string) error { +func copyDirAndAddPods(origDir, tempDir, srcName, srcNs, dstName, dstNs string) error { return filepath.Walk(origDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err From fa6755513d29f896cbf3f4937e0fda4fa3cc7692 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 15:50:45 +0200 Subject: [PATCH 15/17] tmp dir --- pkg/cli/command_test.go | 3 ++- pkg/internal/testutils/generate_pod_yamls.go | 26 ++++++++------------ 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/pkg/cli/command_test.go b/pkg/cli/command_test.go index 1255c54a..26d4e27b 100644 --- a/pkg/cli/command_test.go +++ b/pkg/cli/command_test.go @@ -458,8 +458,9 @@ func TestEvalCommandOutput(t *testing.T) { // but since eval command only supports pods, we will generate a copy of the dirs with // pods yaml files from the matching workload resource of the tt's source and dst. // so the command may be executed with the given args - dirPath, err = testutils.GenerateTempDirWithPods(dirPath, tt.sourcePod, tt.sourceNs, tt.destPod, tt.destNs) + err = testutils.GenerateTempDirWithPods(dirPath, tt.sourcePod, tt.sourceNs, tt.destPod, tt.destNs) require.Nil(t, err, "test: %q", testName) + dirPath = testutils.TmpDir defer os.RemoveAll(testutils.TmpDir) // clean up after finishing the test } args := []string{"eval", "--dirpath", dirPath, diff --git a/pkg/internal/testutils/generate_pod_yamls.go b/pkg/internal/testutils/generate_pod_yamls.go index b1feb23a..5a9aac72 100644 --- a/pkg/internal/testutils/generate_pod_yamls.go +++ b/pkg/internal/testutils/generate_pod_yamls.go @@ -36,28 +36,22 @@ var TmpDir = filepath.Join(projectpath.Root, "temp") // cleaned up after the tes // GenerateTempDirWithPods generates new temporary dir that copies origDir and adds yaml files of Pod kind // matching the input workload resources of the src and dst // the function returns the path of the generated temp dir. -func GenerateTempDirWithPods(origDir, srcName, srcNs, dstName, dstNs string) (string, error) { - // create the TmpDir path if does not exist +func GenerateTempDirWithPods(origDir, srcName, srcNs, dstName, dstNs string) error { + // create the TmpDir if _, err := os.Stat(TmpDir); os.IsNotExist(err) { osErr := os.Mkdir(TmpDir, dirMode) if osErr != nil { - return "", osErr + return osErr } } - // making temp directory in the temp path - testTmpDir, err := os.MkdirTemp(TmpDir, tmpPattern) - if err != nil { - return "", err - } // copy orig dir into the temp dir and add to temp dir generated pods - err = copyDirAndAddPods(origDir, testTmpDir, srcName, srcNs, dstName, dstNs) - return testTmpDir, err + return copyDirAndAddPods(origDir, srcName, srcNs, dstName, dstNs) } // copyDirAndAddPods copies files of network-policies from origDir into tempDir // and generates into the tempDir : Pod yaml files for given src and dst peers from their workload resources // in the origDir -func copyDirAndAddPods(origDir, tempDir, srcName, srcNs, dstName, dstNs string) error { +func copyDirAndAddPods(origDir, srcName, srcNs, dstName, dstNs string) error { return filepath.Walk(origDir, func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -66,7 +60,7 @@ func copyDirAndAddPods(origDir, tempDir, srcName, srcNs, dstName, dstNs string) return nil } origFile := filepath.Join(origDir, info.Name()) - tempFile := filepath.Join(tempDir, info.Name()) + tempFile := filepath.Join(TmpDir, info.Name()) // if the file contains workload with given srcName or dstName : get the workload object and // generate matching pod yaml file in the temp dir // this is needed since we need also a copy of the workload labels which may be used in the netpols rules @@ -77,7 +71,7 @@ func copyDirAndAddPods(origDir, tempDir, srcName, srcNs, dstName, dstNs string) return err } if srcLabels != nil { // nil means the src was not found in that file - err = generatePodYaml(tempDir, srcName, srcNs, srcLabels) + err = generatePodYaml(srcName, srcNs, srcLabels) if err != nil { return err } @@ -89,7 +83,7 @@ func copyDirAndAddPods(origDir, tempDir, srcName, srcNs, dstName, dstNs string) return err } if dstLabels != nil { // nil means the dst was not found in that file - err = generatePodYaml(tempDir, dstName, dstNs, dstLabels) + err = generatePodYaml(dstName, dstNs, dstLabels) if err != nil { return err } @@ -188,10 +182,10 @@ spec: const yamlSuffix = ".yaml" // generatePodYaml generates a YAML file for a given pod data. -func generatePodYaml(dirName, podName, podNs string, labels map[string]string) error { +func generatePodYaml(podName, podNs string, labels map[string]string) error { pod := PodInfo{Name: podName, Namespace: podNs, Labels: labels} fileName := podNs + "_" + podName + yamlSuffix - podFile := filepath.Join(dirName, fileName) + podFile := filepath.Join(TmpDir, fileName) // write the pod template using the pod data podTmpl, err := template.New("pod").Parse(podYamlTemplate) if err != nil { From ffdcefc2a7bb349a4370ffede14b5b4536ba40dc Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 16:08:08 +0200 Subject: [PATCH 16/17] renaming attributes --- pkg/netpol/eval/eval_test.go | 119 ++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 58 deletions(-) diff --git a/pkg/netpol/eval/eval_test.go b/pkg/netpol/eval/eval_test.go index b7ee2eff..6a5ccb45 100644 --- a/pkg/netpol/eval/eval_test.go +++ b/pkg/netpol/eval/eval_test.go @@ -1871,78 +1871,81 @@ func TestAllParsedResourcesEvalTests(t *testing.T) { runParsedResourcesEvalTests(t, examples.ANPWithBANPFromParsedResourcesTest) } +// TestDirPathEvalResults tests eval results of an allowed connection between two peers in the given dir. +// note that: that for some tests, the directory may contain workload resources (not pod resources), then eval result will be +// between src pod and dst pod with pod names owned by these workloads (pods which are added by policy engine ). func TestDirPathEvalResults(t *testing.T) { cases := []struct { - dir string - sourcePod string - sourceNs string - destNs string - destPod string - protocol string - port string - evalResult bool + dir string + sourceWorkload string + sourceNs string + destNs string + destWorkload string + protocol string + port string + evalResult bool }{ { - dir: "anp_demo", - sourceNs: "gryffindor", - sourcePod: "harry-potter", - destPod: "luna-lovegood", - destNs: "ravenclaw", - protocol: "udp", - port: "52", - evalResult: true, + dir: "anp_demo", + sourceNs: "gryffindor", + sourceWorkload: "harry-potter", + destWorkload: "luna-lovegood", + destNs: "ravenclaw", + protocol: "udp", + port: "52", + evalResult: true, }, { - dir: "anp_test_6", - sourceNs: "network-policy-conformance-slytherin", - sourcePod: "draco-malfoy", - destPod: "cedric-diggory", - destNs: "network-policy-conformance-hufflepuff", - protocol: "udp", - port: "5353", - evalResult: false, + dir: "anp_test_6", + sourceNs: "network-policy-conformance-slytherin", + sourceWorkload: "draco-malfoy", + destWorkload: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + protocol: "udp", + port: "5353", + evalResult: false, }, { - dir: "anp_test_multiple_anps", - sourceNs: "network-policy-conformance-ravenclaw", - sourcePod: "luna-lovegood", - destPod: "draco-malfoy", - destNs: "network-policy-conformance-slytherin", - protocol: "sctp", - port: "9003", - evalResult: false, + dir: "anp_test_multiple_anps", + sourceNs: "network-policy-conformance-ravenclaw", + sourceWorkload: "luna-lovegood", + destWorkload: "draco-malfoy", + destNs: "network-policy-conformance-slytherin", + protocol: "sctp", + port: "9003", + evalResult: false, }, { - dir: "anp_with_np_and_banp_pass_test", - sourceNs: "ns2", - sourcePod: "pod1", - destPod: "pod1", - destNs: "ns1", - port: "80", - evalResult: true, + dir: "anp_with_np_and_banp_pass_test", + sourceNs: "ns2", + sourceWorkload: "pod1", + destWorkload: "pod1", + destNs: "ns1", + port: "80", + evalResult: true, }, { - dir: "anp_with_np_pass_test", - sourceNs: "ns2", - sourcePod: "pod1", - destPod: "pod1", - destNs: "ns1", - port: "8080", - evalResult: false, + dir: "anp_with_np_pass_test", + sourceNs: "ns2", + sourceWorkload: "pod1", + destWorkload: "pod1", + destNs: "ns1", + port: "8080", + evalResult: false, }, { - dir: "anp_banp_core_test", - sourceNs: "network-policy-conformance-gryffindor", - sourcePod: "harry-potter", - destPod: "cedric-diggory", - destNs: "network-policy-conformance-hufflepuff", - port: "8080", - evalResult: true, + dir: "anp_banp_core_test", + sourceNs: "network-policy-conformance-gryffindor", + sourceWorkload: "harry-potter", + destWorkload: "cedric-diggory", + destNs: "network-policy-conformance-hufflepuff", + port: "8080", + evalResult: true, }, } for _, tt := range cases { tt := tt - testName := "eval_" + tt.dir + "_from_" + tt.sourcePod + "_to_" + tt.destPod + testName := "eval_" + tt.dir + "_from_" + tt.sourceWorkload + "_to_" + tt.destWorkload t.Run(testName, func(t *testing.T) { t.Parallel() if tt.protocol == "" { @@ -1957,15 +1960,15 @@ func TestDirPathEvalResults(t *testing.T) { require.Nil(t, err, "test: %q", testName) var src, dst string for podStr, podObj := range pe.podsMap { - if podObj.Owner.Name == tt.sourcePod && podObj.Namespace == tt.sourceNs { + if podObj.Owner.Name == tt.sourceWorkload && podObj.Namespace == tt.sourceNs { src = podStr } - if podObj.Owner.Name == tt.destPod && podObj.Namespace == tt.destNs { + if podObj.Owner.Name == tt.destWorkload && podObj.Namespace == tt.destNs { dst = podStr } } - require.NotEmpty(t, src, "test %q, could not find pod for %s", testName, tt.sourcePod) - require.NotEmpty(t, dst, "test %q, could not find pod for %s", testName, tt.destPod) + require.NotEmpty(t, src, "test %q, could not find pod for %s", testName, tt.sourceWorkload) + require.NotEmpty(t, dst, "test %q, could not find pod for %s", testName, tt.destWorkload) res, err := pe.CheckIfAllowed(src, dst, tt.protocol, tt.port) require.Nil(t, err, "test: %q", testName) require.Equal(t, tt.evalResult, res, "unexpected result for test %q, should be %v", testName, tt.evalResult) From 088cb77a1c710d23ecdd8ef7632dc7a639af8732 Mon Sep 17 00:00:00 2001 From: shireenf-ibm Date: Tue, 12 Nov 2024 17:32:36 +0200 Subject: [PATCH 17/17] modifying func --- pkg/netpol/eval/check_eval.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/netpol/eval/check_eval.go b/pkg/netpol/eval/check_eval.go index 195783c0..21f4d2a9 100644 --- a/pkg/netpol/eval/check_eval.go +++ b/pkg/netpol/eval/check_eval.go @@ -148,17 +148,15 @@ func (pe *PolicyEngine) allowedXgressConnectionByAdminNetpols(src, dst k8s.Peer, // isAllowedByANPCapturedRes when an admin-network-policy captures a connection , its result may be Allow (final- allowed conn), // or Deny (final - denied conn) or Pass (to be determined by netpol/ banp) // return value (allowedOrDenied, pass bool, err error) -// * if the given ANP result is Allow or Deny : returns true for allow and false for deny as the value of allowedOrDenied. -// in this case returns pass = false (not important) -// * if the given ANP result is Pass : returns true for pass and false for allow/deny -// as the value of pass is the only important in this case. -func isAllowedByANPCapturedRes(anpRes k8s.ANPRulesResult) (allowedOrDenied, pass bool, err error) { +// * if the given ANP result is Allow or Deny : returns true for allow and false for deny as the value of res. +// * if the given ANP result is Pass : returns true for passOrNonCaptured +func isAllowedByANPCapturedRes(anpRes k8s.ANPRulesResult) (res, passOrNonCaptured bool, err error) { switch anpRes { - case k8s.Pass: + case k8s.Pass: // we can not determine yet, pass to next policy layer return false, true, nil - case k8s.Allow: + case k8s.Allow: // result is true (conn is allowed), no need to pass to next policy layer return true, false, nil - case k8s.Deny: + case k8s.Deny: // result is false (conn is not allowed), no need to pass to next policy layer return false, false, nil } return false, false, errors.New(netpolerrors.UnknownRuleActionErr) // will not get here