NPM adhering to both ingress and egress rules

[vakalapa]

Currently NPM accepts a packet if it hits at least one rule in either ingress chain or egress chain. This behavior had an obvious flaw where if ingress allowed and egress blocked it, NPM would send the packets through.

This change will help evaluate both INGRESS and EGRESS rules before accepting/taking a decision on a packet. NPM will now MARK a packet for ingress/egress and RETURN the MARK'ed packet. Then packet will be accepted in the main chain after all the ingress and egress rules are processed.

Below is an example

Chain AZURE-NPM (1 references)
 pkts bytes target     prot opt in     out     source               destination
  367 18860 AZURE-NPM-INGRESS-PORT  all  --  *      *       0.0.0.0/0            0.0.0.0/0
  367 18860 AZURE-NPM-EGRESS-PORT  all  --  *      *       0.0.0.0/0            0.0.0.0/0
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x3000 /* ACCEPT-on-INGRESS-and-EGRESS-mark-0x3000 */
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x2000 /* ACCEPT-on-INGRESS-mark-0x2000 */
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x1000 /* ACCEPT-on-EGRESS-mark-0x1000 */
  346 17600 AZURE-NPM-TARGET-SETS  all  --  *      *       0.0.0.0/0            0.0.0.0/0
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            state RELATED,ESTABLISHED

Chain AZURE-NPM-EGRESS-PORT (1 references)
 pkts bytes target     prot opt in     out     source               destination
    0     0 MARK       tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            match-set azure-npm-1078940524 dst tcp dpt:80 match-set azure-npm-2071429381 src /* ALLOW-ns-ns-name:network-policy-b-AND-TCP-PORT-80-FROM-ns-network-policy-2312 */ MARK or 0x1000
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x3000 /* RETURN-on-EGRESS-and-INGRESS-mark-0x3000 */
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x1000 /* RETURN-on-EGRESS-mark-0x1000 */
   21  1260 AZURE-NPM-TARGET-SETS  all  --  *      *       0.0.0.0/0            0.0.0.0/0            match-set azure-npm-2071429381 src /* ALLOW-ALL-FROM-ns-network-policy-2312-TO-JUMP-TO-AZURE-NPM-TARGET-SETS */

Chain AZURE-NPM-EGRESS-TO (0 references)
 pkts bytes target     prot opt in     out     source               destination
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x3000 /* RETURN-on-EGRESS-and-INGRESS-mark-0x3000 */
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x1000 /* RETURN-on-EGRESS-mark-0x1000 */

Chain AZURE-NPM-INGRESS-FROM (0 references)
 pkts bytes target     prot opt in     out     source               destination
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x2000 /* RETURN-on-INGRESS-mark-0x2000 */

Chain AZURE-NPM-INGRESS-PORT (1 references)
 pkts bytes target     prot opt in     out     source               destination
    0     0 MARK       tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            match-set azure-npm-2071429381 dst match-set azure-npm-1078940524 src tcp dpt:80 /* ALLOW-ns-ns-name:network-policy-b-AND-TCP-PORT-80-TO-ns-network-policy-2312 */ MARK set 0x2000
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0x2000 /* RETURN-on-INGRESS-mark-0x2000 */
    0     0 AZURE-NPM-TARGET-SETS  all  --  *      *       0.0.0.0/0            0.0.0.0/0            match-set azure-npm-2071429381 dst /* ALLOW-ALL-TO-ns-network-policy-2312-TO-JUMP-TO-AZURE-NPM-TARGET-SETS */

Chain AZURE-NPM-TARGET-SETS (3 references)
 pkts bytes target     prot opt in     out     source               destination
   21  1260 DROP       all  --  *      *       0.0.0.0/0            0.0.0.0/0            match-set azure-npm-2071429381 src /* DROP-ALL-FROM-ns-network-policy-2312 */
    0     0 DROP       all  --  *      *       0.0.0.0/0            0.0.0.0/0            match-set azure-npm-2071429381 dst /* DROP-ALL-TO-ns-network-policy-2312 */

[codecov]

Codecov Report

Merging #765 (b52c0ec) into master (c3aa60f) will increase coverage by 0.35%.
The diff coverage is 68.23%.

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
+ Coverage   41.43%   41.79%   +0.35%     
==========================================
  Files         141      142       +1     
  Lines       13461    13763     +302     
==========================================
+ Hits         5578     5752     +174     
- Misses       7191     7293     +102     
- Partials      692      718      +26     

[csfmomo]

Do we want to add a test case to hit this scenario? For example, ingress allow the certain packet however egress deny it, then the final result should be packet be denied. I think the test can be added in translatePolicy_test.go. Not sure whether our current test already covered it since I saw you also updated translatePolicy_test.go

[csfmomo]

What does this ctmark mean?

[vakalapa]

Correcting this, it is not a Connection Tracker mark, instead a mark field in skb.

[neaggarwMS]

What is the difference between EgressX vs Egress. Can you update the comments and explain the significance of these margers

[neaggarwMS]

I guess you explained the difference in the Brownbag meeting, can you add comment here as well explaining what all these marks represent

[vakalapa]

@csfmomo Looks like the 2 policies are individually covered in our unit tests already. And with k8s e2e we will be covering them together, so I am skipping adding any new testcases. Let me know if you think otherwise

[neaggarwMS]

I guess you explained the difference in the Brownbag meeting, can you add comment here as well explaining what all these marks represent

[tamilmani1989]

i didn't do detailed review as i don't have much context on this code. Looks like one big function. should break this function. May be separate PR.
Should add more comments wherever you are changing it. I see you added in few places. can you add in places wherever you could find

[vakalapa]

i didn't do detailed review as i don't have much context on this code. Looks like one big function. should break this function. May be separate PR.
Should add more comments wherever you are changing it. I see you added in few places. can you add in places wherever you could find

@tamilmani1989 totally agree with cutting down the large functions translateIngressPolicy and translateEgressPolicy. I will log it as a separate WI.