Tests for AdminNetworkPolicy

[tanyaveksler]

Added some ANP tests from policy-assistant.
Fixed a small bug in handling named ports in ANP

[shireenf-ibm]

hi @tanyaveksler ,

• why the tests where added in connlist pkg and not eval ?
• also we prefer to avoid hard-coded expected output since it will be more difficult to maintain

• in the base branch of admin-netpols, I have already added multiple tests to the connlist_test (with test directories (of yaml resources) and expected-output files)
• thanks for fixing the bug with named-port (fixed it in the base branch too)

[tanyaveksler]

Hi @shireenf-ibm,

• the tests were added in connlist pkg and not eval, because they test all connections for the given configuration, rather then a connection between two specific nodes.
• regarding the hardcoded tests and expected results, this is the first draft of adding tests from policy-assistant. Since in policy-assistant the tests are hardcoded, rather than in yamls (unlike the tests that you added in the base branch of admin-netpols), we decided with @adisos to add them hardcoded as well. Also, it is in sync with what we have in the eval pkg.

[adisos]

Hi @shireenf-ibm,

* the tests were added in `connlist` pkg and not `eval`, because they test all connections for the given configuration, rather then a  connection between two specific nodes.

* regarding the hardcoded tests and expected results, this is the first draft of adding tests from policy-assistant. Since in policy-assistant the tests are hardcoded, rather than in yamls (unlike the tests that you added in the base branch of admin-netpols), we decided with @adisos to add them hardcoded as well. Also, it is in sync with what we have in the `eval` pkg.

it's okay to have the hard-coded input instead of YAMLs, but the expected output should better be in a file and not hard-coded, as this is going to be more difficult to update following changes in output..
unless the expected result is in terms of vars/structs instead of output string. in that case you can encode the expected connectivity in certain test vars, and have some code that compares expected connectivity with the one returned from the analysis function..

[tanyaveksler]

@shireenf-ibm , please look at TestANPConnectivityFromParsedResources, the last test "multiple ANPs (priority order #2)".
It has two ANPs, the first one having priority 101, that allows TCP 80-81 and UDP 80-81 for all peers.
and the second one having priority 100, that denies the same ports/protocols for all peers.
According to my understanding, the result should be that all connections are allowed (and this is what I put in expected results), but in the actual run the result is denying TCP 80-81 and UDP 80-81 connections for all peers (as if the deny policy were of a higher priority).
Do I miss something, or is it a bug?

[shireenf-ibm]

@shireenf-ibm , please look at TestANPConnectivityFromParsedResources, the last test "multiple ANPs (priority order #2)". It has two ANPs, the first one having priority 101, that allows TCP 80-81 and UDP 80-81 for all peers. and the second one having priority 100, that denies the same ports/protocols for all peers. According to my understanding, the result should be that all connections are allowed (and this is what I put in expected results), but in the actual run the result is denying TCP 80-81 and UDP 80-81 connections for all peers (as if the deny policy were of a higher priority). Do I miss something, or is it a bug?

from the description I think the actual results are correct , for ANPs the low priority takes precedence;
so if the ANP with priority 100 denies the connections that where allowed in an ANP with priority 101, I expect a deny.
you can find more here:

Priority is a value from 0 to 1000. Policies with lower priority values have higher precedence, and are checked before policies with higher priority values.

[shireenf-ibm]

Hi @shireenf-ibm,

* the tests were added in `connlist` pkg and not `eval`, because they test all connections for the given configuration, rather then a  connection between two specific nodes.

* regarding the hardcoded tests and expected results, this is the first draft of adding tests from policy-assistant. Since in policy-assistant the tests are hardcoded, rather than in yamls (unlike the tests that you added in the base branch of admin-netpols), we decided with @adisos to add them hardcoded as well. Also, it is in sync with what we have in the `eval` pkg.

I see, however, since multiple connlist tests are already added, and our goal is to test the new functionality in eval;
I still suggest to move these tests to the eval pkg (eval_test.go or a new test file under eval);

• in eval_test.go we have examples with hard-coded input, including funcs for adding these hard-coded inputs to the policy-engine (for example: netpolFromYaml and podFromYaml , there are also funcs which return the hard-coded policy itself);
  these funcs can be helpful and eliminate the need to add new funcs to parser.go
• and in order to get all conns results: you can loop the pods (see example: TestGeneralPerformance and get all conns by using pe.allAllowedConnections which also uses all new anp functions.(as if it was called from connlist)).

you may choose to write the results and compare with expected files
or instead you can add entries of "interesting" pods and expected "allowed_conns" between them.
in my opinion it is not critic to get all conns (like in connlist) for unit tests
@adisos , what do you think?

[tanyaveksler]

@shireenf-ibm , please look at TestANPConnectivityFromParsedResources, the last test "multiple ANPs (priority order #2)". It has two ANPs, the first one having priority 101, that allows TCP 80-81 and UDP 80-81 for all peers. and the second one having priority 100, that denies the same ports/protocols for all peers. According to my understanding, the result should be that all connections are allowed (and this is what I put in expected results), but in the actual run the result is denying TCP 80-81 and UDP 80-81 connections for all peers (as if the deny policy were of a higher priority). Do I miss something, or is it a bug?

from the description I think the actual results are correct , for ANPs the low priority takes precedence; so if the ANP with priority 100 denies the connections that where allowed in an ANP with priority 101, I expect a deny. you can find more here:

Priority is a value from 0 to 1000. Policies with lower priority values have higher precedence, and are checked before policies with higher priority values.

OK, I see that lower priority has higher precedence. Then, another test results look incorrect. Look at the test "multiple ANPs (priority order #1)" - it is the opposite to the one described above: ANP with priority 99 allows TCP 80-81 and UDP 80-81, while ANP with priority 100 denies them. Yet, in the actual output those connections are denied.

[shireenf-ibm]

@shireenf-ibm , please look at TestANPConnectivityFromParsedResources, the last test "multiple ANPs (priority order #2)". It has two ANPs, the first one having priority 101, that allows TCP 80-81 and UDP 80-81 for all peers. and the second one having priority 100, that denies the same ports/protocols for all peers. According to my understanding, the result should be that all connections are allowed (and this is what I put in expected results), but in the actual run the result is denying TCP 80-81 and UDP 80-81 connections for all peers (as if the deny policy were of a higher priority). Do I miss something, or is it a bug?

from the description I think the actual results are correct , for ANPs the low priority takes precedence; so if the ANP with priority 100 denies the connections that where allowed in an ANP with priority 101, I expect a deny. you can find more here:

Priority is a value from 0 to 1000. Policies with lower priority values have higher precedence, and are checked before policies with higher priority values.

OK, I see that lower priority has higher precedence. Then, another test results look incorrect. Look at the test "multiple ANPs (priority order #1)" - it is the opposite to the one described above: ANP with priority 99 allows TCP 80-81 and UDP 80-81, while ANP with priority 100 denies them. Yet, in the actual output those connections are denied.

by debugging I see that the problem is that the ANPs has no names, but when saving them in the policy-engine, I have a map from anp-name to its object; since both have the same empty ("") name string; so only the last ANP is saved in policy-engine as if it is the only ANP that was provided.
do you have the option to add names to the ANPs? (i.e. to add also metadata not just spec )

[shireenf-ibm]

@tanyaveksler updated base branch to return error for anp objects without names

[tanyaveksler]

@adisos and @shireenf-ibm: I addressed your comments, the PR is ready for your review.

[shireenf-ibm]

should these funcs be exported?

[tanyaveksler]

They shouldn't :)

[adisos]

small typos..