Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't remove representative peers in any-namespace #352

Merged
merged 13 commits into from
May 21, 2024
Merged

don't remove representative peers in any-namespace #352

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
84ddee3
always keep representative peers which match all-namespaces
shireenf-ibm May 15, 2024
fb29b62
Merge branch 'new_exposure_analysis_first_branch' of github.com:np-gu…
shireenf-ibm May 15, 2024
8ef848a
update test output after merge
shireenf-ibm May 15, 2024
3c9e430
examples with rules exposing pod to an existing ns
shireenf-ibm May 16, 2024
e134fa1
Update pkg/netpol/eval/exposure.go
shireenf-ibm May 16, 2024
722964f
don't refine rep. peers matching any pod in a namespace + changing so…
shireenf-ibm May 16, 2024
e4276e4
adding same test with nil podSelector instead of empty one
shireenf-ibm May 16, 2024
b7c498a
adding test with inaccurate output
shireenf-ibm May 16, 2024
124a4f6
fixing comment syntax
shireenf-ibm May 20, 2024
256ed88
Update pkg/netpol/connlist/connlist_test.go
shireenf-ibm May 20, 2024
58c5df1
gofmt
shireenf-ibm May 21, 2024
025ee8f
updating comments in yaml
shireenf-ibm May 21, 2024
7e0595f
tiny fix
shireenf-ibm May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions pkg/netpol/connlist/connlist_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,4 +918,21 @@ var goodPathTests = []struct {
exposureAnalysis: true,
outputFormats: []string{output.DOTFormat},
},
{
// test that when the rule enable any-namespace with podSelector, a representative peer is created even
// if there is a matching pod in a specific namespace
testDirName: "test_exposure_to_any_namespace_with_podSelector",
exposureAnalysis: true,
outputFormats: ExposureValidFormats,
},
{
testDirName: "test_conn_to_all_pods_in_an_existing_ns",
exposureAnalysis: true,
outputFormats: ExposureValidFormats,
},
{
testDirName: "test_conn_to_new_pod_in_an_existing_ns",
exposureAnalysis: true,
outputFormats: ExposureValidFormats,
},
}
7 changes: 7 additions & 0 deletions pkg/netpol/eval/exposure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,19 @@ func (pe *PolicyEngine) extractLabelsAndRefineRepresentativePeers(podObj *k8s.Po

// refineRepresentativePeersMatchingLabels removes from the policy engine all representative peers
// with labels matching the given labels of a real pod
// representative peers matching any-namespace will not be removed.
func (pe *PolicyEngine) refineRepresentativePeersMatchingLabels(realPodLabels, realNsLabels map[string]string) {
keysToDelete := make([]string, 0)
// look for representative peers with labels matching the given real pod's (and its namespace) labels
for key, peer := range pe.representativePeersMap {
potentialPodSelector := labels.SelectorFromSet(labels.Set(peer.Pod.Labels))
potentialNsSelector := labels.SelectorFromSet(labels.Set(peer.PotentialNamespaceLabels))
if potentialNsSelector.Empty() { // representative peer that matches any-namespace, will not be removed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it differentiate between

namespaceSelector: {}
podSelector: {k: v}

(which is any namespace)

to

podSelector: {k: v}

(which is the network policy namespace)?

Copy link
Collaborator

@adisos adisos May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and what about

namespaceSelector: {k: v}
podSelector: {}

this creates exposure to all pods in a certain namespace..
does this require indication as exposure?
it's not as matching pod with certain labels..

do we have any example test capturing this case with current implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it differentiate between

namespaceSelector: {}
podSelector: {k: v}

(which is any namespace)

to

podSelector: {k: v}

(which is the network policy namespace)?

yes, it does
examples:

  1. tests/test_conn_with_pod_selector_in_any_ns with :
  • from:
    - namespaceSelector: {}
    podSelector:
    matchLabels:
    role: monitoring

(output: test_outputs\connlist\exposure_test_conn_with_pod_selector_in_any_ns_connlist_output.dot.png)

  1. tests/test_conn_with_only_pod_selector
  • from:
    - podSelector:
    matchLabels:
    role: monitoring
    (output:test_outputs\connlist\exposure_test_conn_with_only_pod_selector_connlist_output.dot.png)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and what about

namespaceSelector: {k: v}
podSelector: {}

this creates exposure to all pods in a certain namespace.. does this require indication as exposure? it's not as matching pod with certain labels..

do we have any example test capturing this case with current implementation?

we have following example:
test: tests\test_conn_to_all_pods_in_a_new_ns
with:

  • from:
    - namespaceSelector:
    matchLabels:
    kubernetes.io/metadata.name: backend
    podSelector: {}

output: test_outputs\connlist\exposure_test_conn_to_all_pods_in_a_new_ns_connlist_output.dot.png

in this example the backend ns is not in the manifests and we have no real pods in this ns so we see an exposure to representative peers backend\[all pods].

if we had a real pod in the namespace backend : it would be a match and we won't see the exposure line.(like all cases where the podSelector was not used, if there is a pod in the namespace the it is a match)

Copy link
Collaborator

@adisos adisos May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this example the backend ns is not in the manifests and we have no real pods in this ns

I would add another example where there is a real pod in the namespace backend

if we had a real pod in the namespace backend : it would be a match and we won't see the exposure line.

ok, the question is - should we consider changing this behavior (when the policy permits all pods in a real namespace)? if all pods in a namespace are permitted, it might be relevant as exposure info, even if there is a certain real pod in the namespace. (BTW there could be a real pod in that namespace on which there is a certain policy that blocks such a connection for this pod.. in such a case the connection to this pod is not in the report, nor the exposure to this namespace). [consider adding such an example as well]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to a policy that selects all pods in its namespace.. (let it be foo namespace) let's assume we do not have a pod in that namespace but another pod (bar/pod1) has exposure to this namespace (e.g. policy that permits egress to foo) the question is - do we consider such policy on a representative peer (namespace)? (for example, a policy that is a default deny for the foo namespace)

I will add an example to see the behavior and update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added an example locally, the exposure analysis of bar still shows the line:
bar/workload-a[Deployment] => foo/[all pods] : All Connections
even that we have default-deny-in-foo for ingress

should fix the bug in this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create new item and implement in a separate PR .
analysis should generate representative peers by also considering policies that capture them, and separate into sub-sections (sub-peers) that are having different connectivity by the policies (for example pods in ns with certain labels, and pods in ns without these labels).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we had a real pod in the namespace backend : it would be a match and we won't see the exposure line.

ok, the question is - should we consider changing this behavior (when the policy permits all pods in a real namespace)? if all pods in a namespace are permitted, it might be relevant as exposure info, even if there is a certain real pod in the namespace.

I see, yes we can change this. our assumption was that if there is a rule selecting a namespace and there was a real pod in that namespace so the "developer" knows that connections are allowed to that namespace and didn't add it to the exposure lines. so by changing this we want to emphasize that conns with all pods in that namespace are allowed . ( however this will affect also tests with only namespaceSelector, where the namespace is real and there is a pod in it in the manifests )

(BTW there could be a real pod in that namespace on which there is a certain policy that blocks such a connection for this pod.. in such a case the connection to this pod is not in the report, nor the exposure to this namespace). [consider adding such an example as well]

I prefer to add such examples within the adding more complicated tests PR

ok, let's have this change in this PR , including adding the relevant examples , and mark by the example which has inaccurate output, a TODO comment, to make sure it is fixed in the next PR .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

shireenf-ibm marked this conversation as resolved.
Show resolved Hide resolved
continue
}
// remove representative peer matching both realPodLabels and realNsLabels.
// a representative peer that matches any-pod in a specific namespace, and real Labels are for a specific pod in same namespace,
// the representative peer will be removed (we have at least one real pod in that namespace)
if potentialPodSelector.Matches(labels.Set(realPodLabels)) && potentialNsSelector.Matches(labels.Set(realNsLabels)) {
keysToDelete = append(keysToDelete, key)
}
Expand Down
23 changes: 23 additions & 0 deletions test_outputs/connlist/exposure_test_conn_to_all_pods_in_an_existing_ns_connlist_output.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
digraph {
subgraph "cluster_backend" {
color="black"
fontcolor="black"
"backend/backend-app[Deployment]" [label="backend-app[Deployment]" color="blue" fontcolor="blue"]
label="backend"
}
subgraph "cluster_hello_world" {
color="black"
fontcolor="black"
"hello-world/workload-a[Deployment]" [label="workload-a[Deployment]" color="blue" fontcolor="blue"]
label="hello-world"
}
"0.0.0.0-255.255.255.255" [label="0.0.0.0-255.255.255.255" color="red2" fontcolor="red2"]
"entire-cluster" [label="entire-cluster" color="red2" fontcolor="red2" shape=diamond]
"0.0.0.0-255.255.255.255" -> "backend/backend-app[Deployment]" [label="All Connections" color="gold2" fontcolor="darkgreen"]
"backend/backend-app[Deployment]" -> "0.0.0.0-255.255.255.255" [label="All Connections" color="gold2" fontcolor="darkgreen"]
"backend/backend-app[Deployment]" -> "entire-cluster" [label="All Connections" color="gold2" fontcolor="darkgreen" weight=0.5]
"backend/backend-app[Deployment]" -> "hello-world/workload-a[Deployment]" [label="TCP 8050" color="gold2" fontcolor="darkgreen"]
"entire-cluster" -> "backend/backend-app[Deployment]" [label="All Connections" color="gold2" fontcolor="darkgreen" weight=1]
"hello-world/workload-a[Deployment]" -> "backend/backend-app[Deployment]" [label="All Connections" color="gold2" fontcolor="darkgreen"]
"hello-world/workload-a[Deployment]" -> "entire-cluster" [label="All Connections" color="gold2" fontcolor="darkgreen" weight=0.5]
}
Binary file added ...nnlist/exposure_test_conn_to_all_pods_in_an_existing_ns_connlist_output.dot.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
95 changes: 95 additions & 0 deletions ...nnlist/exposure_test_conn_to_all_pods_in_an_existing_ns_connlist_output.dot.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 18 additions & 0 deletions test_outputs/connlist/exposure_test_conn_to_all_pods_in_an_existing_ns_connlist_output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
0.0.0.0-255.255.255.255 => backend/backend-app[Deployment] : All Connections
backend/backend-app[Deployment] => 0.0.0.0-255.255.255.255 : All Connections
backend/backend-app[Deployment] => hello-world/workload-a[Deployment] : TCP 8050
hello-world/workload-a[Deployment] => backend/backend-app[Deployment] : All Connections

Exposure Analysis Result:
Egress Exposure:
backend/backend-app[Deployment] => 0.0.0.0-255.255.255.255 : All Connections
backend/backend-app[Deployment] => entire-cluster : All Connections
hello-world/workload-a[Deployment] => entire-cluster : All Connections

Ingress Exposure:
backend/backend-app[Deployment] <= 0.0.0.0-255.255.255.255 : All Connections
backend/backend-app[Deployment] <= entire-cluster : All Connections

Workloads not protected by network policies:
backend/backend-app[Deployment] is not protected on Egress
backend/backend-app[Deployment] is not protected on Ingress
24 changes: 24 additions & 0 deletions test_outputs/connlist/exposure_test_conn_to_new_pod_in_an_existing_ns_connlist_output.dot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
digraph {
subgraph "cluster_backend" {
color="black"
fontcolor="black"
"backend/backend-app[Deployment]" [label="backend-app[Deployment]" color="blue" fontcolor="blue"]
"pod with {app=backend-new}_in_backend" [label="pod with {app=backend-new}" color="red2" fontcolor="red2"]
label="backend"
}
subgraph "cluster_hello_world" {
color="black"
fontcolor="black"
"hello-world/workload-a[Deployment]" [label="workload-a[Deployment]" color="blue" fontcolor="blue"]
label="hello-world"
}
"0.0.0.0-255.255.255.255" [label="0.0.0.0-255.255.255.255" color="red2" fontcolor="red2"]
"entire-cluster" [label="entire-cluster" color="red2" fontcolor="red2" shape=diamond]
"0.0.0.0-255.255.255.255" -> "backend/backend-app[Deployment]" [label="All Connections" color="gold2" fontcolor="darkgreen"]
"backend/backend-app[Deployment]" -> "0.0.0.0-255.255.255.255" [label="All Connections" color="gold2" fontcolor="darkgreen"]
"backend/backend-app[Deployment]" -> "entire-cluster" [label="All Connections" color="gold2" fontcolor="darkgreen" weight=0.5]
"entire-cluster" -> "backend/backend-app[Deployment]" [label="All Connections" color="gold2" fontcolor="darkgreen" weight=1]
"hello-world/workload-a[Deployment]" -> "backend/backend-app[Deployment]" [label="All Connections" color="gold2" fontcolor="darkgreen"]
"hello-world/workload-a[Deployment]" -> "entire-cluster" [label="All Connections" color="gold2" fontcolor="darkgreen" weight=0.5]
"pod with {app=backend-new}_in_backend" -> "hello-world/workload-a[Deployment]" [label="TCP 8050" color="gold2" fontcolor="darkgreen" weight=1]
}
Binary file added ...onnlist/exposure_test_conn_to_new_pod_in_an_existing_ns_connlist_output.dot.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading