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.

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

Conversation

shireenf-ibm
Copy link
Contributor

@shireenf-ibm shireenf-ibm commented May 15, 2024

issue #236

sub task:

    • handling cases of empty nsSelector (any-namespace) with non-empty podSelector : don't refine its representative peer if there is a matching pod in a specific namespace

also: keep representative peer for the case of all pods within a certain namespace

@shireenf-ibm shireenf-ibm mentioned this pull request May 15, 2024
@shireenf-ibm shireenf-ibm marked this pull request as draft May 15, 2024 10:07
@shireenf-ibm shireenf-ibm requested a review from adisos May 15, 2024 10:07
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

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

added one question

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

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

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
Copy link
Contributor Author

in this commit
added two examples of exposure to an existing namespace (having real pod in the ns)
with current approach:

  • in first test test_conn_to_all_pods_in_an_existing_ns since we have a real pod in backend we don't see "all pods" exposure line
  • in second test test_conn_to_new_pod_in_an_existing_ns I used specific podSelector and so we see it in the exposure report

pkg/netpol/eval/exposure.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm requested a review from adisos May 16, 2024 11:25
@shireenf-ibm shireenf-ibm requested a review from adisos May 21, 2024 05:54
@shireenf-ibm shireenf-ibm marked this pull request as ready for review May 21, 2024 06:07
@shireenf-ibm shireenf-ibm merged commit f930f3c into new_exposure_analysis_first_branch May 21, 2024
2 checks passed
@shireenf-ibm shireenf-ibm deleted the sub_task_special_refinement_case branch May 21, 2024 06:07
shireenf-ibm added a commit that referenced this pull request Aug 8, 2024
* exposure-analysis flag

* initial support of exposure from only namespaceSelectors

* lint gofmt

* wip - defining an interface for exposure-analysis results (#295)

* wip - define an interface for the new returned value (new API)

* Update pkg/netpol/connlist/exposed_pods.go

Co-authored-by: Adi Sosnovich <[email protected]>

* wip- some updates to the interfaces (only)

* Update pkg/netpol/connlist/exposed_pods.go

Co-authored-by: Adi Sosnovich <[email protected]>

* interface doc update

* fixes

* Update pkg/netpol/connlist/exposed_peer.go

Co-authored-by: Adi Sosnovich <[email protected]>

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* connlist implementing exposure analysis (#296)

* connlist implementing exposure analysis

* Update pkg/netpol/connlist/connlist.go

Co-authored-by: Adi Sosnovich <[email protected]>

* fix rep. pod name

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposed_peer.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposed_peer.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis.go

Co-authored-by: Adi Sosnovich <[email protected]>

* add func that updates the protected flag of a pod

* return error values

* avoid fields dups among types

* update func doc

* getConnectionsBetweenPeers update doc + returns the exposureMap

* move connection interface, avoid code dup, and compare conns using ConnectionSet

* make the func an exposureMap func

* fixing issue of same string in podsOwnerMap

* Update pkg/netpol/connlist/exposure_analysis.go

Co-authored-by: Adi Sosnovich <[email protected]>

* renaming Connection interface + move PortRange

* struct embedding

* using connectionSet internally + move the refinement to one iter at the end

* Update pkg/netpol/connection/connection.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis.go

Co-authored-by: Adi Sosnovich <[email protected]>

* rename AllConnections

* verify conversion

* storing the maximum entire cluster connection

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* Revert api changes (#298)

* revert Connection, diff AllowedConnectivity, PortRange , ConnectionSet

* revert connlist API changes

* revert eval API changes

* Update pkg/netpol/connlist/connlist.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/connlist.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* renaming funcs

* exposing common.Connection as connlist.AllowedSet

* revert exposing common.Connection as connlist.AllowedSet

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* Exposure analysis unit tests (#299)

* unit tests for the functionality of connlist/exposure_analysis.go

* Update pkg/netpol/connlist/exposure_analysis_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* adding getallTCPconnections

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* Entire cluster exposure opt (#304)

* optimizing entire cluster exposure

* optimizing performance - compute entire cluster exposure only once

* Update pkg/netpol/eval/internal/k8s/pod.go

Co-authored-by: Adi Sosnovich <[email protected]>

* multiple fixes

* Update pkg/netpol/eval/internal/k8s/pod.go

Co-authored-by: Adi Sosnovich <[email protected]>

* using empty connswt instead of nil

* add pod exposure data struct

* exporting podExposureInfo

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* pre proccessing policy for general conns (#306)

* first commit: pre proccessing policy for general conns

* func doc

* fixing to PortSet; instead of PortSet{}, call MakePortSet(false) to ensure initializing empty maps for named ports

* fixing handling connections with namedPort

* fixing lint issued by github - not relevant to PR

* tiny fix

* fixes

* Policy engine with new api func for exposure analysis  (#307)

* task1 add new api func to policy-engine; so pre-process runs only for exposure-analysis

* task2 on exposure analysis benefit from the stored data

* eliminate isRuleGeneral; skip general rules in ruleSelectsPeer

* missing func doc

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* todo comment

* revert initiating conns between two peers

* avoid iterating policy if its general conns are all conns

* todo comment

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* adding representativePeer struct (#309)

* adding representativePeer struct

* Update pkg/netpol/eval/internal/k8s/peer.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/resources.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/resources.go

Co-authored-by: Adi Sosnovich <[email protected]>

* gofmt

* connlist if-else fix

* fixes to exposure_map.go

* fixing connlist includePairOfWorkloads

* fix GetPeerList() - separate GetRepresentativePeersList

* comment how Pod of representative peer is originated

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* Optimize representative peers generation  (#314)

* add representative peers for all non-empty rules while policies upsert

* refine pods that has a match in the resources - first commit

* handling returned err from convertPeerToPodPeer

* handle case of namespaceSelector containing name key

* generate unizue rep peers and refine while upsering objects

* Update pkg/netpol/eval/resources.go

Co-authored-by: Adi Sosnovich <[email protected]>

* fixes

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* go fmt

* lint fix

* exposure analysis test (#316)

* exposure analysis test

* exposure data comparison and new test

* using ca calls to compute exposed peers

* Update pkg/netpol/connlist/exposure_analysis_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/connlist/exposure_analysis_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* lint fix

* flattening tests dir

* exposure map fixes

* 3 values of protected data

* splitting exposure map into two maps

* fixes

* required changes

* code fixes

* typo fix

* a new test for increasing coverage

* new test

* comment update

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* delete unused file

* textual output with exposure analysis (#331)

* textual output with exposure analysis

* tiny code enhancement

* readme update

* output change + code modify

* fixes

* dot output with exposure results (#333)

* dot output with exposure results

* tiny fix

* new tests

* fixing golangci-lint 1.58.0 errors

* exposure analysis with pod selectors (#343)

* code changes + new tests with pod selectors

* fixes and new test

* running onlineboutique_workloads with exposure

* running k8s_ingress_test_new with exposure

* linter fix - headers

* exposure analysis with focus-workload (#349)

* exposure analysis with focus-workload

* focus-workload fixes

* textual output enhancement (adding [] to strings with multiple words)

* fix

* enhancing dot view (#353)

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

* always keep representative peers which match all-namespaces

* update test output after merge

* examples with rules exposing pod to an existing ns

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* don't refine rep. peers matching any pod in a namespace + changing some tests to keep initial purpose+updating results of existing tests

* adding same test with nil podSelector instead of empty one

* adding test with inaccurate output

* fixing comment syntax

* Update pkg/netpol/connlist/connlist_test.go

Co-authored-by: Adi Sosnovich <[email protected]>

* gofmt

* updating comments in yaml

* tiny fix

---------

Co-authored-by: Adi Sosnovich <[email protected]>

* supporting csv, md and json formats (#360)

* supporting csv, md and json formats

* fixing after merge with base branch

* consider real exposure flag

* csv, md, json are consistent with txt - two sections

* dot graphs with exposure edges dashed and with different colors

* merging with master branch

* empty_commit

* handling selectors with matchexpressions (fixed) (#377)

* support match expression operators for generating and selecting representative peers + first examples

* more tests

* more tests

* updating code with label selectors

* merge fixes

* duplicated tests with matching pods

* fixing code + tests with multiple policies

* update comments in exposure.go

* renaming function and updating comments and doc of representative_selectors.go

* move `RepresentativeNsLabelSelector` field from namespace.go to pod.go

* 1. reverting changes to AddPodByNameAndNamespace and resolveSingleMissingNamespace (to original version from main branch)
2. creating a new func for adding representative pods to the policy-engine, without representative namespaces. a representative pod which should not be in a real namespace, will have no namespace

* avoid duplicating code of generating the default namespace name map; and some updates to netpol.go

* eliminate representativePeer.PotentialNamespaceLabelSelector as it duplicates Pod.RepresentativeNsLabelSelector

* renaming the func in representative_selectors.go again

* a new test with handling a special case of equiv rules written in a different way

* unit test for representative_selectors.go

* removing redundant code

* updating documentation of new fields in pod.go

* fixes in resources.go

* fix in check.go

* update few comments

Signed-off-by: adisos <[email protected]>

* renaming AddObjects + updating its documentation

* renaming netpol funcs

* renaming connPeers

* fixing representative pods naming and updating relevant funcs

* renaming "GeneralConns" to "ExposedGeneralConns"

* removing PolicyNsFlag

* no need to split namespaces with policies at first

* Revert "no need to split namespaces with policies at first"

This reverts commit 03e384e.

* rename  extractLabelsAndRefineRepresentativePeers and refineRepresentativePeersMatchingLabels

* renaming checkIfP2PConnOrExposureConn

* lint fix

* func allAllowedConnectionsBetweenPeers: remove ingressSet, egressSet

* using new terms for general conns : ClusterWideExposure and ExternalExposure

* an example why should split namespaces at the beginning with the policies

* eliminate RepresentativePeer struct

* fixing some typos and adding some very used words to a cspell file

* more typos fixes

* updating some comments

* updating readme (all formats supported)

* getting netpols before pods for live cluster - so it works well for both exposure-analysis on/off

* Update pkg/netpol/eval/check.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/check.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/exposure.go

Co-authored-by: Adi Sosnovich <[email protected]>

* rename getSelectorsAndUpdateExposedGeneralConns

* rename ScanPolicyRulesAndUpdateExposedWideConns

* rename updateNetworkPolicyWideExposureConns

* Update pkg/netpol/eval/resources.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/peer.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/peer.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/pod.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/pod.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/resources.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/resources.go

Co-authored-by: Adi Sosnovich <[email protected]>

* fixing lint

* Update pkg/netpol/eval/internal/k8s/representative_selectors.go

Co-authored-by: Adi Sosnovich <[email protected]>

* lint fix

* Update pkg/netpol/eval/internal/k8s/representative_selectors.go

Co-authored-by: Adi Sosnovich <[email protected]>

* fixing the last commit

* fixing the SelectorsFullMatch doc

* removing unnecessaryDeepCopy calls

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* Update pkg/netpol/eval/internal/k8s/netpol.go

Co-authored-by: Adi Sosnovich <[email protected]>

* lint fix

* some renamings in representative_selectors + document why returning full match for rep selector in case of empty rule

* adding line to comment

* split funcs in check.go for readability

* rename hasRepresentativePod

* updating comment

* updating comment of storing the named port

* updating String() func of workloadpeer

* comment update

* updating comment

* new func of selectors match in `netpol.go` to avoid duplicates

* updating comment in pod.go (what do the combinations of rep selectors imply for)

* renaming str vars

* eliminating addIfMissingNamespace func

* new tests - rep peers when there is real ns but no real pods matching

* add comment on String() func

* rename handleRequirementWithInOpAndSingleValue

* renaming test dirs and expected output of exposure-analysis tests

* new fixes

---------

Signed-off-by: adisos <[email protected]>
Co-authored-by: adisos <[email protected]>
Co-authored-by: Adi Sosnovich <[email protected]>

---------

Signed-off-by: adisos <[email protected]>
Co-authored-by: Adi Sosnovich <[email protected]>
Co-authored-by: Tanya <[email protected]>
Co-authored-by: adisos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants