W.I.P Extend anp egress peer #447

shireenf-ibm · 2024-11-18T09:34:33Z

base PR: #380
issue: #442

support Networks field in AdminNetworkPolicyEgressPeer
add logger to k8s objects and raise some warnings (on IPv6 cidr / nodes field / named ports without match ..)
more tests (good paths w/wo warnings)
unit tests on logger warnings
some "tbd" in the committed code

…rt_admin_netpolicy

…ll but: udp 5353 instead of SCTP 1-65535,TCP 1-65535,UDP 1-5352,5354-65535)

…t) for connectionSet

…licies in the input resources

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

…tend_anp_egress_peer

adisos

initial comments

adisos · 2024-12-01T11:18:25Z

pkg/netpol/eval/resources.go

@@ -59,8 +61,9 @@ type (
 )

 // NewPolicyEngine returns a new PolicyEngine with an empty initial state
-func NewPolicyEngine() *PolicyEngine {
+func NewPolicyEngine(l logger.Logger) *PolicyEngine {


can avoid breaking current exported functions API ?

reverted this change

adisos · 2024-12-01T11:18:36Z

pkg/netpol/eval/resources.go

@@ -71,16 +74,16 @@ func NewPolicyEngine() *PolicyEngine {
 	}
 }

-func NewPolicyEngineWithObjects(objects []parser.K8sObject) (*PolicyEngine, error) {
-	pe := NewPolicyEngine()
+func NewPolicyEngineWithObjects(objects []parser.K8sObject, l logger.Logger) (*PolicyEngine, error) {


here as well?

reverted this change

adisos · 2024-12-01T11:18:58Z

pkg/netpol/eval/resources.go

 	err := pe.addObjectsByKind(objects)
 	return pe, err
 }

 // NewPolicyEngineWithOptions returns a new policy engine with an empty state but updating the exposure analysis flag
 // TBD: currently exposure-analysis is the only option supported by policy-engine, so no need for options list param
-func NewPolicyEngineWithOptions(exposureFlag bool) *PolicyEngine {
-	pe := NewPolicyEngine()
+func NewPolicyEngineWithOptions(exposureFlag bool, l logger.Logger) *PolicyEngine {


there is an option WithLogger in connlist, can it be used here as well, as an option for NewPolicyEngineWithOptions ?
Instead of breaking the exported functions API?
better using same pattern as in connlist, have dedicated function "With.." for each option, and a dedicated type such as type ConnlistAnalyzerOption func(*ConnlistAnalyzer).

is it ok to change the input parameter of NewPolicyEngineWithOptions to be a list of options ...PolicyEngineOption?
(the options will include both the exposure flag and logger)
or this will be considered as API breaking?

this is also API breaking..
can add new func NewPolicyEngineWithOptionsList to be used with the new type of options, and add a comment for the NewPolicyEngineWithOptions that it is deprecated.

adisos · 2024-12-01T11:54:58Z

pkg/netpol/eval/internal/k8s/adminnetpol.go

 ////////////////////////////////////////////////////////////////////////////////////////////

 // AdminNetworkPolicy is an alias for k8s adminNetworkPolicy object
-type AdminNetworkPolicy apisv1a.AdminNetworkPolicy
+type AdminNetworkPolicy struct {


please move this type to be at the top of this file (also for banp)

done; (the banp struct is on top of its file)

adisos · 2024-12-01T12:15:28Z

test_outputs/connlist/anp_and_banp_using_networks_test_connlist_output.csv

why is the ingress connections cidrs split, if not impacted by netpols?
if this is due to computation of disjoint cidrs, should consider separate the computation to ingress and egress, so that refinement is not "global", but per each direction separately.

adisos

warning formats: be consistent with current format, for example:

$ ./bin/k8snetpolicy list --dirpath tests/bad_netpols/subdir5/
2024/12/01 15:12:11 Network policy "default/shippingservice-netpol": port name:  "abc" has no match in the configuration of the destination peer "default/shippingservice-1"; it will be ignored, and will not appear in the connectivity results.
0.0.0.0-255.255.255.255 => default/adservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/cartservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/checkoutservice[Deployment] : All Connections

and example from current implementation:

r$ ./bin/k8snetpolicy list --dirpath tests/anp_and_banp_using_networks_with_ipv6_test/
2024/12/01 15:14:13 admin network policy "network-as-egress-peer": Warning  in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported; it will be ignored, and will not appear in the connectivity results.

can be changed to:

admin network policy "network-as-egress-peer": in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported

also, better explain the impact of the "IPV6 not supported" warning in the README documentation.

adisos · 2024-12-01T13:18:41Z

warning formats: be consistent with current format, for example:

$ ./bin/k8snetpolicy list --dirpath tests/bad_netpols/subdir5/
2024/12/01 15:12:11 Network policy "default/shippingservice-netpol": port name:  "abc" has no match in the configuration of the destination peer "default/shippingservice-1"; it will be ignored, and will not appear in the connectivity results.
0.0.0.0-255.255.255.255 => default/adservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/cartservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/checkoutservice[Deployment] : All Connections

and example from current implementation:

r$ ./bin/k8snetpolicy list --dirpath tests/anp_and_banp_using_networks_with_ipv6_test/
2024/12/01 15:14:13 admin network policy "network-as-egress-peer": Warning  in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported; it will be ignored, and will not appear in the connectivity results.

can be changed to:

admin network policy "network-as-egress-peer": in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported

also, better explain the impact of the "IPV6 not supported" warning in the README documentation.

Also, in this example (from anp_and_banp_using_networks_with_ipv6_test) there are 2 actual warnings, repeated multiple times in the log.. can this be fixed?

adisos · 2024-12-01T13:27:06Z

tests/anp_and_banp_using_networks_test/policies.yaml

can enhance this example with some cidr range contained in "pass" to be allowed by regular network policy, and so that there is another cidr range in "pass" reaching the "base" ANP as in the current example. (so that it demonstrates effect of both levels of NP and BANP for pass of IP-block).

adisos · 2024-12-01T13:32:33Z

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

+	minimumPort = 1
+	maximumPort = 65535


ports are already defined in pkg/netpol/internal/common/portset.go (can be exported and used from there)

adisos · 2024-12-01T13:36:58Z

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

+					// raising a warning that the "named port" of the rule is ignored, since it has no match in the pod config.
+					np.Logger.Warnf(np.netpolWarning(alerts.WarnUnmatchedNamedPort(portName, dst.String())))
+				}
+			} // @tbd should raise a warning/error if the portRange is empty (when also namedPort is empty)


as for "tbd should raise a warning/error if the portRange is empty" :
please add an example test covering this case.. what is the output currently? can a warning by sufficient? (rule becomes ineffective?)

added a test.
currently an empty port range is ignored (skipped);
so on one hand I can add a warning (since no problems are caused); but on the other hand according to this
end port:

This field cannot be defined if the port field is not defined or if the port field is defined as a named (string) port. The endPort must be equal or greater than port

.
so it is illegal; does this mean we should raise an error?

ok, so let's change to an error (to be consistent, if for other invalid values in policy resource an error is issued).

adisos · 2024-12-01T13:41:04Z

pkg/netpol/eval/internal/k8s/adminnetpol.go

@@ -185,9 +412,15 @@ func ruleConnections(ports *[]apisv1a.AdminNetworkPolicyPort, dst Peer) (*common
 			}
 			portSet.AddPort(intstr.FromInt32(anpPort.PortNumber.Port))
 		case anpPort.NamedPort != nil:
+			if dst.PeerType() == IPBlockType {
+				// IPblock does not have named-ports defined
+				// @tbd should return error? (a rule that combines networks and pods may have such port?)


as for "tbd" comment: can add an example test for this case as well?
if a consistent connectivity output is possible, a warning is preferred.

done; added test anp_test_named_ports_multiple_peers with a warning that the named-port is ignored for an IP-Block

shireenf-ibm · 2024-12-02T08:48:55Z

warning formats: be consistent with current format, for example:

$ ./bin/k8snetpolicy list --dirpath tests/bad_netpols/subdir5/
2024/12/01 15:12:11 Network policy "default/shippingservice-netpol": port name:  "abc" has no match in the configuration of the destination peer "default/shippingservice-1"; it will be ignored, and will not appear in the connectivity results.
0.0.0.0-255.255.255.255 => default/adservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/cartservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/checkoutservice[Deployment] : All Connections

and example from current implementation:

r$ ./bin/k8snetpolicy list --dirpath tests/anp_and_banp_using_networks_with_ipv6_test/
2024/12/01 15:14:13 admin network policy "network-as-egress-peer": Warning  in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported; it will be ignored, and will not appear in the connectivity results.

can be changed to:

admin network policy "network-as-egress-peer": in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported

also, better explain the impact of the "IPV6 not supported" warning in the README documentation.

made tiny fixes so warnings from netpols and ANPs be consistent (actually, both warnings from netpols and anps were added in this PR).
added a note under ### Possible warnings in the docs\connlist_output.md regarding unsupported IPv6

shireenf-ibm · 2024-12-02T09:07:02Z

warning formats: be consistent with current format, for example:

$ ./bin/k8snetpolicy list --dirpath tests/bad_netpols/subdir5/
2024/12/01 15:12:11 Network policy "default/shippingservice-netpol": port name:  "abc" has no match in the configuration of the destination peer "default/shippingservice-1"; it will be ignored, and will not appear in the connectivity results.
0.0.0.0-255.255.255.255 => default/adservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/cartservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/checkoutservice[Deployment] : All Connections

and example from current implementation:

r$ ./bin/k8snetpolicy list --dirpath tests/anp_and_banp_using_networks_with_ipv6_test/
2024/12/01 15:14:13 admin network policy "network-as-egress-peer": Warning  in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported; it will be ignored, and will not appear in the connectivity results.

can be changed to:

admin network policy "network-as-egress-peer": in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported

also, better explain the impact of the "IPV6 not supported" warning in the README documentation.

Also, in this example (from anp_and_banp_using_networks_with_ipv6_test) there are 2 actual warnings, repeated multiple times in the log.. can this be fixed?

the warning is logged each time the rule is scanned - for finding allowed conns between two peers (different peers each time);
we can "mark the policy as visited" (adding an attribute to its struct) to avoid repeating the warnings log;
should add it now or in another issue?

adisos · 2024-12-02T10:04:45Z

warning formats: be consistent with current format, for example:
$ ./bin/k8snetpolicy list --dirpath tests/bad_netpols/subdir5/
2024/12/01 15:12:11 Network policy "default/shippingservice-netpol": port name:  "abc" has no match in the configuration of the destination peer "default/shippingservice-1"; it will be ignored, and will not appear in the connectivity results.
0.0.0.0-255.255.255.255 => default/adservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/cartservice[Deployment] : All Connections
0.0.0.0-255.255.255.255 => default/checkoutservice[Deployment] : All Connections
and example from current implementation:
r$ ./bin/k8snetpolicy list --dirpath tests/anp_and_banp_using_networks_with_ipv6_test/
2024/12/01 15:14:13 admin network policy "network-as-egress-peer": Warning  in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported; it will be ignored, and will not appear in the connectivity results.
can be changed to:
admin network policy "network-as-egress-peer": in rule "allow-all-egress-to-intranet": IPv6 addresses are not supported
also, better explain the impact of the "IPV6 not supported" warning in the README documentation.
Also, in this example (from anp_and_banp_using_networks_with_ipv6_test) there are 2 actual warnings, repeated multiple times in the log.. can this be fixed?
the warning is logged each time the rule is scanned - for finding allowed conns between two peers (different peers each time); we can "mark the policy as visited" (adding an attribute to its struct) to avoid repeating the warnings log; should add it now or in another issue?

ok, let's fix this in this branch

shireenf-ibm added 30 commits April 30, 2024 13:01

adding ANP to parser.k8sobj

7286094

fixing gocritic rangeValCopy by indexing

cc14430

Merge github.com:np-guard/netpol-analyzer into support_admin_netpolicy

bb14609

Merge branch 'main' of github.com:np-guard/netpol-analyzer into suppo…

03af344

…rt_admin_netpolicy

Merge github.com:np-guard/netpol-analyzer into support_admin_netpolicy

47b8718

w.i.p. anp support - first commit

150f698

more examples (2 ANPs/ ANP+NP)

87243c2

fixing references

4adf709

new_test that ensures rule ordering in ANP is respected

d25107d

update the conn representation as complement in case it is shorter (a…

d72b159

…ll but: udp 5353 instead of SCTP 1-65535,TCP 1-65535,UDP 1-5352,5354-65535)

test with swapped rules from another test + diff test

4dfeb6d

more-tests

de0ff91

fixing conns computations and a test with multiple ANPs

3eacc3c

extending output formats of existing tests

0a85d43

tiny fix

3e9db6e

fixing a tinu bug in ruleConnections func

836ed4f

tiny doc updte

6641bfd

tiny doc update

619cb88

a @todo tbd while review

b082158

return error if ANPs are without name or not unique names

484d447

remove redundant lines

71031e8

reverting the changes adding complement string representation (all bu…

f672a0b

…t) for connectionSet

Merge github.com:np-guard/netpol-analyzer into support_admin_netpolicy

eb7a685

Merge remote-tracking branch 'origin/HEAD' into support_admin_netpolicy

634706f

minor updates to netpol_errors

2e2ca27

currently disabling exposure-analysis when there are admin-network-po…

bc29f3f

…licies in the input resources

some organizations (mainly comments updates)

56e9f5d

updating some todo messages

6391bbe

updating some todo messages/questions

9b93505

todo question

fb5a582

shireenf-ibm and others added 4 commits November 27, 2024 10:10

test update

5ad1aed

Merge branch 'main' into support_admin_netpolicy

b8b98c6

test update

8a0144b

add test details

7b0c7b8

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

shireenf-ibm mentioned this pull request Nov 28, 2024

W.I.P Support admin netpolicy #380

Merged

Base automatically changed from support_admin_netpolicy to main November 28, 2024 11:42

Merge remote-tracking branch 'origin/support_admin_netpolicy' into ex…

f11aeaa

…tend_anp_egress_peer

shireenf-ibm marked this pull request as ready for review November 28, 2024 15:16

shireenf-ibm added 2 commits November 28, 2024 17:24

Merge remote-tracking branch 'origin/main' into extend_anp_egress_peer

3199bb5

Merge remote-tracking branch 'origin/main' into extend_anp_egress_peer

78b6cd7

shireenf-ibm mentioned this pull request Dec 1, 2024

adding bad path tests #437

Merged

Merge remote-tracking branch 'origin/main' into extend_anp_egress_peer

df63e52

adisos reviewed Dec 1, 2024

View reviewed changes

switching sections of adminnetpol.go file

40e94d0

adisos reviewed Dec 1, 2024

View reviewed changes

shireenf-ibm added 2 commits December 1, 2024 17:59

adding options to policy-engine without breaking the API

cdb8b84

min&max port

1a7939e

warning consistency + doc update on IPv6

f2fb131

enhance test

606c64f

shireenf-ibm added 2 commits December 2, 2024 13:27

test : netpol with empty port range

a86298f

test anp with named-port-with-ips

8c86f15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

W.I.P Extend anp egress peer #447

W.I.P Extend anp egress peer #447

shireenf-ibm commented Nov 18, 2024 •

edited

Loading

adisos left a comment

adisos Dec 1, 2024

shireenf-ibm Dec 1, 2024

adisos Dec 1, 2024

shireenf-ibm Dec 1, 2024

adisos Dec 1, 2024

adisos Dec 1, 2024

shireenf-ibm Dec 1, 2024 •

edited

Loading

adisos Dec 1, 2024

shireenf-ibm Dec 1, 2024

adisos Dec 1, 2024

shireenf-ibm Dec 1, 2024

adisos Dec 1, 2024 •

edited

Loading

adisos left a comment

adisos commented Dec 1, 2024

adisos Dec 1, 2024

shireenf-ibm Dec 2, 2024

adisos Dec 1, 2024

shireenf-ibm Dec 1, 2024

adisos Dec 1, 2024 •

edited

Loading

shireenf-ibm Dec 2, 2024

adisos Dec 2, 2024 •

edited

Loading

adisos Dec 1, 2024

shireenf-ibm Dec 2, 2024

shireenf-ibm commented Dec 2, 2024

shireenf-ibm commented Dec 2, 2024 •

edited

Loading

adisos commented Dec 2, 2024

W.I.P Extend anp egress peer #447

Are you sure you want to change the base?

W.I.P Extend anp egress peer #447

Conversation

shireenf-ibm commented Nov 18, 2024 • edited Loading

adisos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shireenf-ibm Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisos Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

adisos left a comment

Choose a reason for hiding this comment

adisos commented Dec 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisos Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisos Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shireenf-ibm commented Dec 2, 2024

shireenf-ibm commented Dec 2, 2024 • edited Loading

adisos commented Dec 2, 2024

shireenf-ibm commented Nov 18, 2024 •

edited

Loading

shireenf-ibm Dec 1, 2024 •

edited

Loading

adisos Dec 1, 2024 •

edited

Loading

adisos Dec 1, 2024 •

edited

Loading

adisos Dec 2, 2024 •

edited

Loading

shireenf-ibm commented Dec 2, 2024 •

edited

Loading