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

named-port bug fix and tests #412

Merged
merged 6 commits into from
Oct 9, 2024
Merged

Conversation

shireenf-ibm
Copy link
Contributor

@shireenf-ibm shireenf-ibm commented Sep 18, 2024

#405

bug(s) description:
1. the analyzer ignored netpol rules with named ports that didn't have a match in the dst pod's configuration
(for example: if an ingress netpol captures pod-a with ingress rule that contains a named-port port-a (TCP protocol) and the pod's specification does not has a containerPort/ Port with this name -- this rule was ignored, has no mention in the output)

- fix: now we will see an ingress conn to pod-a on: TCP port-a in the output

  1. when converting named-port to number, the analyzer didn't compare netpol rule's protocol (which is accompanied with the port) to the protocol of the pod's container port which has same port-name.
    for example: if a pod has containerPort with name: newport and protocol: UDP port:90
    and a netpol capturing that pod with ingress rule: port: newport protocol:TCP
    we saw on the output an ingress connection TCP 90 which is not true since this is not the same port.
    • fix: : the port-name and protocol are considered when converting a named port; if protocols don't match we will see the port-name in the output (in the example: conn is TCP newport)

3. Peer2PeerConnection (Connection) assumed the ports will always contain a numbered interval only
- fix: : PortRange interface contains also a NamedPort option; (the connection on a protocol may either be numbered or named; depends if we succeeded to convert it by from the pod's configuration or not)

tests with examples of the above (with matched and unmatched named-ports) were added

@shireenf-ibm shireenf-ibm changed the title bug fix and tests named-port bug fix and tests Sep 18, 2024
@adisos
Copy link
Collaborator

adisos commented Oct 6, 2024

#405

bug(s) description:

1. the analyzer ignored netpol rules with `named ports` that didn't have a match in the dst pod's configuration
   (for example: if an ingress netpol captures `pod-a`  with ingress rule that contains a named-port `port-a`  (TCP protocol) and the pod's specification does not has a containerPort/ Port with this name -- this rule was ignored, has no mention in the output)
   
   * **fix:** now we will see an  ingress conn to `pod-a` on: `TCP port-a` in the output

2. when converting named-port to number, the analyzer didn't compare netpol rule's protocol (which is accompanied with the port) to the protocol of the pod's container port which has same port-name.
   for example: if a pod has containerPort with `name: newport` and `protocol: UDP port:90`
   and a netpol capturing that pod with ingress rule: `port: newport protocol:TCP`
   we saw on the output an ingress connection `TCP 90` which is not true since this is not the same port.
   
   * **fix:** : the port-name and protocol are considered when converting a named port; if protocols don't match we will see the port-name in the output (in the example: conn is `TCP newport`)

3. `Peer2PeerConnection`  (`Connection`) assumed the ports will always contain a numbered interval only
   
   * **fix:** : `PortRange` interface contains also a `NamedPort` option; (the connection on a protocol may either be numbered or named; depends if we succeeded to convert it by from the pod's configuration or not)

tests with examples of the above (with matched and unmatched named-ports) were added

I agree with 2 , but not sure about 1 , whether it is an issue. (Does 3 follow from 1 ?)
As for 1 -- if a concrete Pod has no matching named port on the yaml config, can it accept traffic on such named port that the policy specifies?
(For exposure analysis it makes sense to specify named ports in the output, since we do not have the real pod configuration).

@@ -349,8 +350,12 @@ func GetConnectionSetFromP2PConnection(c Peer2PeerConnection) *common.Connection
protocolsToPortSetMap := make(map[v1.Protocol]*common.PortSet, len(c.ProtocolsAndPorts()))
for protocol, portRageArr := range c.ProtocolsAndPorts() {
protocolsToPortSetMap[protocol] = common.MakePortSet(false)
for _, p := range portRageArr {
protocolsToPortSetMap[protocol].AddPortRange(p.Start(), p.End())
for _, p := range portRageArr { // each single port range may contain either named port or an interval of start and end numbers
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename portRageArr to portRangeArr

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 and others added 3 commits October 7, 2024 11:07
@shireenf-ibm
Copy link
Contributor Author

#405
bug(s) description:

1. the analyzer ignored netpol rules with `named ports` that didn't have a match in the dst pod's configuration
   (for example: if an ingress netpol captures `pod-a`  with ingress rule that contains a named-port `port-a`  (TCP protocol) and the pod's specification does not has a containerPort/ Port with this name -- this rule was ignored, has no mention in the output)
   
   * **fix:** now we will see an  ingress conn to `pod-a` on: `TCP port-a` in the output

2. when converting named-port to number, the analyzer didn't compare netpol rule's protocol (which is accompanied with the port) to the protocol of the pod's container port which has same port-name.
   for example: if a pod has containerPort with `name: newport` and `protocol: UDP port:90`
   and a netpol capturing that pod with ingress rule: `port: newport protocol:TCP`
   we saw on the output an ingress connection `TCP 90` which is not true since this is not the same port.
   
   * **fix:** : the port-name and protocol are considered when converting a named port; if protocols don't match we will see the port-name in the output (in the example: conn is `TCP newport`)

3. `Peer2PeerConnection`  (`Connection`) assumed the ports will always contain a numbered interval only
   
   * **fix:** : `PortRange` interface contains also a `NamedPort` option; (the connection on a protocol may either be numbered or named; depends if we succeeded to convert it by from the pod's configuration or not)

tests with examples of the above (with matched and unmatched named-ports) were added

I agree with 2 , but not sure about 1 , whether it is an issue. (Does 3 follow from 1 ?) As for 1 -- if a concrete Pod has no matching named port on the yaml config, can it accept traffic on such named port that the policy specifies? (For exposure analysis it makes sense to specify named ports in the output, since we do not have the real pod configuration).

reverted 1 & 3

@pull-request-size pull-request-size bot added size/L and removed size/XL labels Oct 7, 2024
@shireenf-ibm
Copy link
Contributor Author

  • deleted tests that does not have fixed results (comparing to main).

remaining tests results fix :

  • test: netpol_named_port_test

output with bug included following line: (UDP 8956 is a wrong convert of pod's namedPort: - name: newport containerPort: 8956 protocol: SCTP
helloworld/new-pod[Deployment] => helloworld/pod-a[Deployment] : UDP 8956
after fixing the bug this line will not be included sincepod-a configuration does not contain a newport: 8956 with UDP

  • netpol_named_port_test_2:

output with bug would have following line:
helloworld/pod-a[Deployment] => helloworld/new-pod[Deployment] : SCTP 8956,9090,**UDP 9090**
after fixing bug the correct connection is:
helloworld/pod-a[Deployment] => helloworld/new-pod[Deployment] : SCTP 8956,9090
(since new-pod does not have a port named: newport: 9090 with protocol UDP`)

Copy link
Collaborator

@adisos adisos Oct 9, 2024

Choose a reason for hiding this comment

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

please add comment near each named port in the tests, if it is expected to be matched by network policy rule with named port or not.

also, do we need both tests? what is the difference between what they cover?
a comment with details about each test may help in the unit tests file.

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 comments to netpols files + connlist_test file.
although the tests cover same "issue"
(both tests contain a named-port with protocol that does not match the protocol in the pod's configuration (with same named-port)
and one test's netpol includes also a named-port that has no matching port name in the pod.)

the output of the tests differs,

  • in one test since there is no matching port - we don't see any connection between the 2 pods in the connlist
  • in the second tests, we see connection with the two ports that were successfully converted

@shireenf-ibm shireenf-ibm merged commit eb62282 into main Oct 9, 2024
4 checks passed
@shireenf-ibm shireenf-ibm deleted the issue_405_bug_with_named_ports branch October 9, 2024 09:02
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.

fix a bug in getting netpol connections from rule with named port and protocol
2 participants