Issue 210 bug fix (#211)

* space after dot in warning msg * refine ingress-controller conns correctly when focusworkload is used * doc update * support focus workload ns/name format * warn when the focus workload does not exist * fix * Update README.md Co-authored-by: Adi Sosnovich <[email protected]> * fixes * Update pkg/netpol/connlist/connlist.go Co-authored-by: Adi Sosnovich <[email protected]> * fixes * fixes * Update pkg/netpol/connlist/connlist.go Co-authored-by: Adi Sosnovich <[email protected]> * lint fix --------- Co-authored-by: Adi Sosnovich <[email protected]>
np-guard · Aug 13, 2023 · b13e18d · b13e18d
1 parent 0aa1e90
commit b13e18d
Show file tree

Hide file tree

Showing 7 changed files with 203 additions and 57 deletions.
diff --git a/README.md b/README.md
@@ -61,7 +61,8 @@ Examples:
 
 Flags:
  -f, --file string Write output to specified file
- --focusworkload Focus connections of specified workload name in the output
+ --focusworkload Focus connections of specified workload in the output (supported formats: <workload-name>, <workload-namespace>/<workload-name>)
+ (to focus connections from Ingress/Route only, use `ingress-controller` as <workload-name>)
  -o, --output string Required output format (txt, json, dot, csv, md) (default "txt")
  -h, --help help for list
 

diff --git a/cmd/netpolicy/cmd/command_test.go b/cmd/netpolicy/cmd/command_test.go
@@ -241,7 +241,33 @@ func TestCommands(t *testing.T) {
  exact: true,
  isErr: false,
  },
-
+ {
+ name: "test_legal_list_with_focus_workload_format_of_ns_and_name",
+ args: []string{
+ "list",
+ "--dirpath",
+ filepath.Join(getTestsDir(), "onlineboutique_workloads"),
+ "--focusworkload",
+ "default/emailservice",
+ },
+ expectedOutput: "default/checkoutservice[Deployment] => default/emailservice[Deployment] : TCP 8080",
+ exact: true,
+ isErr: false,
+ },
+ {
+ name: "test_legal_list_with_focus_workload_of_ingress_controller",
+ args: []string{
+ "list",
+ "--dirpath",
+ filepath.Join(getTestsDir(), "acs-security-demos"),
+ "--focusworkload",
+ "ingress-controller",
+ },
+ expectedOutput: "{ingress-controller} => frontend/asset-cache[Deployment] : TCP 8080\n" +
+ "{ingress-controller} => frontend/webapp[Deployment] : TCP 8080",
+ exact: true,
+ isErr: false,
+ },
  {
  name: "test_legal_list_with_focus_workload_json_output",
  args: []string{

diff --git a/cmd/netpolicy/cmd/list.go b/cmd/netpolicy/cmd/list.go
@@ -124,7 +124,8 @@ defined`,
 
  // define any flags and configuration settings.
  // Use PersistentFlags() for flags inherited by subcommands or Flags() for local flags.
- c.Flags().StringVarP(&focusWorkload, "focusworkload", "", "", "Focus connections of specified workload name in the output")
+ c.Flags().StringVarP(&focusWorkload, "focusworkload", "", "",
+ "Focus connections of specified workload in the output (<workload-name> or <workload-namespace/workload-name>)")
  // output format - default txt
  supportedFormats := strings.Join(connlist.ValidFormats, ",")
  c.Flags().StringVarP(&output, "output", "o", common.DefaultFormat, "Required output format ("+supportedFormats+")")

diff --git a/pkg/netpol/connlist/connlist.go b/pkg/netpol/connlist/connlist.go
@@ -17,6 +17,7 @@ import (
  "strings"
 
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/apimachinery/pkg/types"
  "k8s.io/client-go/kubernetes"
 
  v1 "k8s.io/api/core/v1"
@@ -354,13 +355,15 @@ func (ca *ConnlistAnalyzer) includePairOfWorkloads(src, dst eval.Peer) bool {
  return true
  }
  // at least one of src/dst should be the focus workload
- if !src.IsPeerIPType() && src.Name() == ca.focusWorkload {
- return true
- }
- if !dst.IsPeerIPType() && dst.Name() == ca.focusWorkload {
- return true
- }
- return false
+ return ca.isPeerFocusWorkload(src) || ca.isPeerFocusWorkload(dst)
+}
+
+func getPeerNsNameFormat(peer eval.Peer) string {
+ return types.NamespacedName{Namespace: peer.Namespace(), Name: peer.Name()}.String()
+}
+
+func (ca *ConnlistAnalyzer) isPeerFocusWorkload(peer eval.Peer) bool {
+ return !peer.IsPeerIPType() && (peer.Name() == ca.focusWorkload || getPeerNsNameFormat(peer) == ca.focusWorkload)
 }
 
 // getConnectionsList returns connections list from PolicyEngine and ingressAnalyzer objects
@@ -371,16 +374,38 @@ func (ca *ConnlistAnalyzer) getConnectionsList(pe *eval.PolicyEngine, ia *ingres
  return connsRes, []Peer{}, nil
  }
 
+ // get workload peers and ip blocks
+ peerList, err := pe.GetPeersList()
+ if err != nil {
+ ca.errors = append(ca.errors, newResourceEvaluationError(err))
+ return nil, nil, err
+ }
+ // represent peerList as []connlist.Peer list to be returned
+ peers := make([]Peer, len(peerList))
+ for i, p := range peerList {
+ peers[i] = p
+ }
+
+ excludeIngressAnalysis := (ia == nil || ia.IsEmpty())
+
+ // if ca.focusWorkload is not empty, check if it exists in the peers before proceeding
+ existFocusWorkload, warningMsg := ca.existsFocusWorkload(peers, excludeIngressAnalysis)
+ if ca.focusWorkload != "" && !existFocusWorkload {
+ ca.errors = append(ca.errors, newConnlistAnalyzerWarning(errors.New(warningMsg)))
+ ca.logger.Warnf(warningMsg)
+ return nil, nil, nil
+ }
+
  // compute connections between peers based on pe analysis of network policies
- peersAllowedConns, peersRes, err := ca.getConnectionsBetweenPeers(pe)
+ peersAllowedConns, err := ca.getConnectionsBetweenPeers(pe, peers)
  if err != nil {
  ca.errors = append(ca.errors, newResourceEvaluationError(err))
  return nil, nil, err
  }
  connsRes = peersAllowedConns
 
- if ia == nil || ia.IsEmpty() {
- return connsRes, peersRes, nil
+ if excludeIngressAnalysis {
+ return connsRes, peers, nil
  }
 
  // analyze ingress connections - create connection objects for relevant ingress analyzer connections
@@ -391,38 +416,48 @@ func (ca *ConnlistAnalyzer) getConnectionsList(pe *eval.PolicyEngine, ia *ingres
  }
  connsRes = append(connsRes, ingressAllowedConns...)
 
- if len(peersAllowedConns) == 0 {
- ca.logger.Warnf("connectivity analysis found no allowed connectivity between pairs from the configured workloads or external IP-blocks")
+ if ca.focusWorkload == "" && len(peersAllowedConns) == 0 {
+ ca.logger.Warnf("Connectivity analysis found no allowed connectivity between pairs from the configured workloads or external IP-blocks")
  }
 
- return connsRes, peersRes, nil
+ return connsRes, peers, nil
 }
 
-// getConnectionsBetweenPeers returns connections list from PolicyEngine object
-func (ca *ConnlistAnalyzer) getConnectionsBetweenPeers(pe *eval.PolicyEngine) ([]Peer2PeerConnection, []Peer, error) {
- // get workload peers and ip blocks
- peerList, err := pe.GetPeersList()
- if err != nil {
- ca.errors = append(ca.errors, newResourceEvaluationError(err))
- return nil, nil, err
+// existsFocusWorkload checks if the provided focus workload is ingress-controller
+// or if it exists in the peers list from the parsed resources
+// if not returns a suitable warning message
+func (ca *ConnlistAnalyzer) existsFocusWorkload(peers []Peer, excludeIngressAnalysis bool) (existFocusWorkload bool, warning string) {
+ if ca.focusWorkload == ingressanalyzer.IngressPodName {
+ if excludeIngressAnalysis { // if the ingress-analyzer is empty,
+ // then no routes/k8s-ingress objects -> ingrss-controller pod will not be added
+ return false, "The ingress-controller workload was not added to the analysis, since Ingress/Route resources were not found." +
+ " Connectivity map report will be empty."
+ }
+ return true, ""
  }
 
- peers := make([]Peer, len(peerList))
- for i, p := range peerList {
- peers[i] = p
+ // check if the focusworkload is in the peers
+ for _, peer := range peers {
+ if ca.focusWorkload == peer.Name() || ca.focusWorkload == getPeerNsNameFormat(peer) {
+ return true, ""
+ }
  }
+ return false, "Workload " + ca.focusWorkload + " does not exist in the input resources. Connectivity map report will be empty."
+}
 
+// getConnectionsBetweenPeers returns connections list from PolicyEngine object
+func (ca *ConnlistAnalyzer) getConnectionsBetweenPeers(pe *eval.PolicyEngine, peers []Peer) ([]Peer2PeerConnection, error) {
  connsRes := make([]Peer2PeerConnection, 0)
- for i := range peerList {
- srcPeer := peerList[i]
- for j := range peerList {
- dstPeer := peerList[j]
+ for i := range peers {
+ srcPeer := peers[i]
+ for j := range peers {
+ dstPeer := peers[j]
  if !ca.includePairOfWorkloads(srcPeer, dstPeer) {
  continue
  }
  allowedConnections, err := pe.AllAllowedConnectionsBetweenWorkloadPeers(srcPeer, dstPeer)
  if err != nil {
- return nil, nil, err
+ return nil, err
  }
  // skip empty connections
  if allowedConnections.IsEmpty() {
@@ -438,7 +473,7 @@ func (ca *ConnlistAnalyzer) getConnectionsBetweenPeers(pe *eval.PolicyEngine) ([
  }
  }
 
- return connsRes, peers, nil
+ return connsRes, nil
 }
 
 // getIngressAllowedConnections returns connections list from IngressAnalyzer intersected with PolicyEngine's connections
@@ -455,6 +490,10 @@ func (ca *ConnlistAnalyzer) getIngressAllowedConnections(ia *ingressanalyzer.Ing
  return nil, err
  }
  for peerStr, peerAndConn := range ingressConns {
+ // refines to only relevant connections if ca.focusWorkload is not empty
+ if !ca.includePairOfWorkloads(ingressControllerPod, peerAndConn.Peer) {
+ continue
+ }
  // compute allowed connections based on pe.policies to the peer, then intersect the conns with
  // ingress connections to the peer -> the intersection will be appended to the result
  peConn, err := pe.AllAllowedConnectionsBetweenWorkloadPeers(ingressControllerPod, peerAndConn.Peer)
@@ -494,8 +533,8 @@ func (ca *ConnlistAnalyzer) warnBlockedIngress(peerStr string, ingressObjs map[s
  warningMsg = "Route resource " + ingressObjs[scan.Route][0]
  }
  warningMsg += " specified workload " + peerStr + " as a backend, but network policies are blocking " +
- "ingress connections from an arbitrary in-cluster source to this workload." +
+ "ingress connections from an arbitrary in-cluster source to this workload. " +
  "Connectivity map will not include a possibly allowed connection between the ingress controller and this workload."
- ca.errors = append(ca.errors, newIngressAnalyzerConnsBlockedWarning(errors.New(warningMsg)))
+ ca.errors = append(ca.errors, newConnlistAnalyzerWarning(errors.New(warningMsg)))
  ca.logger.Warnf(warningMsg)
 }
diff --git a/pkg/netpol/connlist/connlist_errors.go b/pkg/netpol/connlist/connlist_errors.go
@@ -24,7 +24,7 @@ type resourceEvaluationError struct {
  origErr error
 }
 
-type ingressAnalyzerConnsBlockedWarning struct {
+type connlistAnalyzerWarning struct {
  origErr error
 }
 
@@ -36,7 +36,7 @@ func (e *resourceEvaluationError) Error() string {
  return e.origErr.Error()
 }
 
-func (e *ingressAnalyzerConnsBlockedWarning) Error() string {
+func (e *connlistAnalyzerWarning) Error() string {
  return e.origErr.Error()
 }
 
@@ -70,6 +70,6 @@ func newResourceEvaluationError(err error) *connlistGeneratingError {
  return &connlistGeneratingError{err: &resourceEvaluationError{err}, fatal: true, severe: false}
 }
 
-func newIngressAnalyzerConnsBlockedWarning(err error) *connlistGeneratingError {
- return &connlistGeneratingError{err: &ingressAnalyzerConnsBlockedWarning{err}, fatal: false, severe: false}
+func newConnlistAnalyzerWarning(err error) *connlistGeneratingError {
+ return &connlistGeneratingError{err: &connlistAnalyzerWarning{err}, fatal: false, severe: false}
 }
diff --git a/pkg/netpol/connlist/connlist_test.go b/pkg/netpol/connlist/connlist_test.go
@@ -128,12 +128,102 @@ func TestConnList(t *testing.T) {
  }
 }
 
-func TestWithFocusWorkload(t *testing.T) {
- analyzer1 := NewConnlistAnalyzer(WithFocusWorkload("emailservice"))
- dirPath := filepath.Join(testutils.GetTestsDir(), "onlineboutique_workloads")
- res, _, err := analyzer1.ConnlistFromDirPath(dirPath)
- require.Len(t, res, 1)
- require.Nil(t, err)
+func TestConnListWithFocusWorkload(t *testing.T) {
+ cases := []struct {
+ name string
+ focusWorkload string
+ testDirName string
+ expectedConnsOutput string
+ notExistingWorkload bool
+ }{
+ {
+ name: "focus workload from netpols",
+ focusWorkload: "emailservice",
+ testDirName: "onlineboutique_workloads",
+ expectedConnsOutput: "default/checkoutservice[Deployment] => default/emailservice[Deployment] : TCP 8080",
+ },
+ {
+ name: "test with external ingress conns enabled to a single workload in addition to its p2p conns",
+ focusWorkload: "details-v1-79f774bdb9",
+ testDirName: "k8s_ingress_test",
+ expectedConnsOutput: "0.0.0.0-255.255.255.255 => default/details-v1-79f774bdb9[ReplicaSet] : All Connections\n" +
+ "default/details-v1-79f774bdb9[ReplicaSet] => 0.0.0.0-255.255.255.255 : All Connections\n" +
+ "default/details-v1-79f774bdb9[ReplicaSet] => default/productpage-v1-6b746f74dc[ReplicaSet] : All Connections\n" +
+ "default/details-v1-79f774bdb9[ReplicaSet] => default/ratings-v1-b6994bb9[ReplicaSet] : All Connections\n" +
+ "default/details-v1-79f774bdb9[ReplicaSet] => default/reviews-v1-545db77b95[ReplicaSet] : All Connections\n" +
+ "default/details-v1-79f774bdb9[ReplicaSet] => default/reviews-v2-7bf8c9648f[ReplicaSet] : All Connections\n" +
+ "default/details-v1-79f774bdb9[ReplicaSet] => default/reviews-v3-84779c7bbc[ReplicaSet] : All Connections\n" +
+ "default/productpage-v1-6b746f74dc[ReplicaSet] => default/details-v1-79f774bdb9[ReplicaSet] : All Connections\n" +
+ "default/ratings-v1-b6994bb9[ReplicaSet] => default/details-v1-79f774bdb9[ReplicaSet] : All Connections\n" +
+ "default/reviews-v1-545db77b95[ReplicaSet] => default/details-v1-79f774bdb9[ReplicaSet] : All Connections\n" +
+ "default/reviews-v2-7bf8c9648f[ReplicaSet] => default/details-v1-79f774bdb9[ReplicaSet] : All Connections\n" +
+ "default/reviews-v3-84779c7bbc[ReplicaSet] => default/details-v1-79f774bdb9[ReplicaSet] : All Connections\n" +
+ "{ingress-controller} => default/details-v1-79f774bdb9[ReplicaSet] : TCP 9080",
+ },
+ {
+ name: "test focus workload with <workload-namespace>/<workload-name> format",
+ testDirName: "acs-security-demos-added-workloads",
+ focusWorkload: "backend/recommendation",
+ expectedConnsOutput: "backend/checkout[Deployment] => backend/recommendation[Deployment] : TCP 8080\n" +
+ "backend/recommendation[Deployment] => backend/catalog[Deployment] : TCP 8080\n" +
+ "backend/reports[Deployment] => backend/recommendation[Deployment] : TCP 8080\n" +
+ "frontend/webapp[Deployment] => backend/recommendation[Deployment] : TCP 8080",
+ },
+ {
+ name: "test with external ingress conns enabled to multiple workloads, refined by one workload name",
+ testDirName: "acs-security-demos-added-workloads",
+ focusWorkload: "asset-cache",
+ expectedConnsOutput: "{ingress-controller} => frontend/asset-cache[Deployment] : TCP 8080",
+ },
+ {
+ name: "test with external ingress conns enabled to multiple workloads, refined by one workload with ns/name format",
+ testDirName: "acs-security-demos-added-workloads",
+ focusWorkload: "frontend/asset-cache",
+ expectedConnsOutput: "{ingress-controller} => frontend/asset-cache[Deployment] : TCP 8080",
+ },
+ {
+ name: "test with external ingress conns enabled to multiple workloads, refined by ingress-controller",
+ testDirName: "acs-security-demos-added-workloads",
+ focusWorkload: "ingress-controller",
+ expectedConnsOutput: "{ingress-controller} => frontend/asset-cache[Deployment] : TCP 8080\n" +
+ "{ingress-controller} => frontend/blog[Deployment] : TCP 8080\n" +
+ "{ingress-controller} => frontend/webapp[Deployment] : TCP 8080\n" +
+ "{ingress-controller} => zeroday/zeroday[Deployment] : TCP 8080",
+ },
+ {
+ name: "focus workload-name does not exist",
+ focusWorkload: "abcd",
+ testDirName: "onlineboutique_workloads", // this dir contains only warning of the non existing workload
+ expectedConnsOutput: "",
+ notExistingWorkload: true,
+ },
+ {
+ name: "focus ns/workload-name does not exist",
+ focusWorkload: "default/abcd",
+ testDirName: "onlineboutique_workloads", // this dir contains only warning of the non existing workload
+ expectedConnsOutput: "",
+ notExistingWorkload: true,
+ },
+ }
+
+ for _, entry := range cases {
+ analyzerWithFocusWorkload := NewConnlistAnalyzer(WithFocusWorkload(entry.focusWorkload))
+ dirPath := filepath.Join(testutils.GetTestsDir(), entry.testDirName)
+ res, _, err := analyzerWithFocusWorkload.ConnlistFromDirPath(dirPath)
+ require.Nil(t, err, "test: %s", entry.name)
+ out, err := analyzerWithFocusWorkload.ConnectionsListToString(res)
+ require.Nil(t, err, "test: %s", entry.name)
+ require.Equal(t, entry.expectedConnsOutput, out, "test: %s", entry.name)
+ if entry.notExistingWorkload {
+ // at least one error, one is the warning error on not existing workload
+ caErrors := analyzerWithFocusWorkload.Errors()
+ require.GreaterOrEqual(t, len(caErrors), 1, "test: %s", entry.name)
+ warnType := &connlistAnalyzerWarning{}
+ if len(caErrors) == 1 {
+ require.True(t, errors.As(caErrors[0].Error(), &warnType), "test: %s", entry.name)
+ }
+ }
+ }
 }
 
 func TestErrNetpolBadCIDR(t *testing.T) {
@@ -260,14 +350,3 @@ func TestWithFocusWorkloadWithReplicasConnections(t *testing.T) {
  require.Nil(t, err)
  require.NotContains(t, out, "kube-system/calico-node[DaemonSet] => kube-system/calico-node[DaemonSet] : All Connections")
 }
-
-func TestWithFocusWorkloadWithIngressObjects(t *testing.T) {
- analyzer := NewConnlistAnalyzer(WithFocusWorkload("details-v1-79f774bdb9"))
- dirPath := filepath.Join(testutils.GetTestsDir(), "k8s_ingress_test")
- res, _, err := analyzer.ConnlistFromDirPath(dirPath)
- require.Len(t, res, 13)
- require.Nil(t, err)
- out, err := analyzer.ConnectionsListToString(res)
- require.Nil(t, err)
- require.Contains(t, out, "{ingress-controller} => default/details-v1-79f774bdb9[ReplicaSet] : TCP 9080")
-}
diff --git a/pkg/netpol/connlist/internal/ingressanalyzer/ingress_analyzer.go b/pkg/netpol/connlist/internal/ingressanalyzer/ingress_analyzer.go
@@ -126,7 +126,7 @@ func (ia *IngressAnalyzer) mapServiceToPeers(svc *corev1.Service) error {
 func (ia *IngressAnalyzer) getServiceSelectedPeers(svc *corev1.Service) ([]eval.Peer, error) {
  svcStr := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}.String()
  if svc.Spec.Selector == nil {
- ia.logger.Warnf("ignoring " + scan.Service + whiteSpace + svcStr + colon + missingSelectorWarning)
+ ia.logger.Warnf("Ignoring " + scan.Service + whiteSpace + svcStr + colon + missingSelectorWarning)
  return nil, nil
  }
  svcLabelsSelector, err := convertServiceSelectorToLabelSelector(svc.Spec.Selector)
@@ -337,7 +337,7 @@ func (ia *IngressAnalyzer) getIngressObjectTargetedPeersAndPorts(ns string,
  for _, svc := range svcList {
  peersAndPorts, ok := ia.servicesToPortsAndPeersMap[ns][svc.serviceName]
  if !ok {
- ia.logger.Warnf("ignoring target service " + svc.serviceName + " : service not found")
+ ia.logger.Warnf("Ignoring target service " + svc.serviceName + " : service not found")
  }
  for _, peer := range peersAndPorts.peers {
  currIngressPeerConn, err := ia.getIngressPeerConnection(peer, peersAndPorts.ports, svc.servicePort)