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

Fix return path of NodePort traffic when using Calico network policy. #263

Merged
merged 1 commit into from
Jan 9, 2019

Conversation

ikatson
Copy link
Contributor

@ikatson ikatson commented Dec 8, 2018

The issue #75 was previously fixed in #130.

However when using aws-vpc-cni together with Calico network policy, that fix does not work, as Calico stops traversing the mangle table early and the CONNMARK rules put by the code in #130 are never reached.

This PR configures Felix (part of calico) to RETURN inside iptables mangle table instead of ACCEPT, so that the rules put by AWS VPC CNI get actually executed.

This bug was referenced in #231 (comment), although the issue was created for smth different.

iptables mangle table before this fix (note ACCEPT in cali-PREROUTING):

Chain PREROUTING (policy ACCEPT 416 packets, 99482 bytes)
 pkts bytes target     prot opt in     out     source               destination
11966 2652K cali-PREROUTING  all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* cali:6gwbT8clXdHdC1b1 */
  0 0 CONNMARK   all  --  eth0   *       0.0.0.0/0            0.0.0.0/0            /* AWS, primary ENI */ ADDRTYPE match dst-type LOCAL limit-in CONNMARK or 0x80
    0     0 CONNMARK   all  --  eni+   *       0.0.0.0/0            0.0.0.0/0            /* AWS, primary ENI */ CONNMARK restore mask 0x80

...

Chain cali-PREROUTING (1 references)
 pkts bytes target     prot opt in     out     source               destination
11421 2612K ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* cali:Wr3YEtmil9N6TPC4 */ ctstate RELATED,ESTABLISHED
    0     0 ACCEPT     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* cali:KrbSWCu2Ux7fTOrL */ mark match 0x10000/0x10000
....

iptables mangle table after this fix (note RETURN in cali-PREROUTING):

Chain PREROUTING (policy ACCEPT 416 packets, 99482 bytes)
 pkts bytes target     prot opt in     out     source               destination
11966 2652K cali-PREROUTING  all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* cali:6gwbT8clXdHdC1b1 */
  351 92631 CONNMARK   all  --  eth0   *       0.0.0.0/0            0.0.0.0/0            /* AWS, primary ENI */ ADDRTYPE match dst-type LOCAL limit-in CONNMARK or 0x80
    0     0 CONNMARK   all  --  eni+   *       0.0.0.0/0            0.0.0.0/0            /* AWS, primary ENI */ CONNMARK restore mask 0x80

...

Chain cali-PREROUTING (1 references)
 pkts bytes target     prot opt in     out     source               destination
11421 2612K RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* cali:Wr3YEtmil9N6TPC4 */ ctstate RELATED,ESTABLISHED
    0     0 RETURN     all  --  *      *       0.0.0.0/0            0.0.0.0/0            /* cali:KrbSWCu2Ux7fTOrL */ mark match 0x10000/0x10000
....

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Previously, commit 2cce7de fixed the return path
of NodePort traffic when pods were on secondary ENIs.

However when using aws-vpc-cni together with Calico network policy, the fix that was introduced
in 2cce7de does not work, as Calico terminates the mangle table
rule traversal early and the CONNMARK rules put by AWS VPC CNI are never reached.

This PR configures Felix (part of calico) to RETURN inside iptables mangle table instead of ACCEPT, so that
the rules that existed in the mangle table after the calico ones get a chance to be executed.
@caseydavenport
Copy link
Contributor

I think @fasaxc should review from the Calico side since he was involved in the original work in #130

Copy link
Contributor

@fasaxc fasaxc left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

I had to read up a lot on #130 and all the references to get this, but it looks sane to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants