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

Add a high priority rule to handle link-local traffic via local route table in SGPP Mode for both IPV4 and IPV6. #3148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

orsenthil
Copy link
Member

@orsenthil orsenthil commented Dec 11, 2024

What type of PR is this?
bug-fix

Which issue does this PR fix?:

#2797

What does this PR do / Why do we need it?:

When POD_SECURITY_GROUP_ENFORCING_MODE is set to strict, pods with SGPP configured have their traffic routed over a branch ENI and thus entirely bypass the primary interface on the node.
Thus pods with SGPP strict mode, will fail to reach link-local addresses. This change ensures that when SGPP is configured in the strict mode, the link-local traffic goes through local route table. This is similar to how ICMPv6 packets from the gateway is handled currently for strict mode.

Testing Information

before this change - the nodes in EKS cluster with SGPP=strict will have the route table.

ip rule list
20:	from all lookup local
1024:	from all fwmark 0x80/0x80 lookup main
32766:	from all lookup main
32767:	from all lookup default
  • Note the missing rule 0

after this this change.

# ip rule list
0:	from 169.254.0.0/16 to 169.254.0.0/16 lookup local
20:	from all lookup local
512:	from all to 192.168.2.200 lookup main
512:	from all to 192.168.13.200 lookup main
1024:	from all fwmark 0x80/0x80 lookup main
32766:	from all lookup main
32767:	from all lookup default
  • rule 0 for 169.254.0.0/16 to 169.254.0.0/16 is added by this change.
  • The 512 priority rule is added during running of tests.

@orsenthil orsenthil requested a review from a team as a code owner December 11, 2024 04:31
@orsenthil orsenthil changed the title Add a high priority rule to handle link-Local traffic via local route table in SGPP Mode for both IPV4 and IPV6. Add a high priority rule to handle link-local traffic via local route table in SGPP Mode for both IPV4 and IPV6. Dec 11, 2024
@orsenthil
Copy link
Member Author

orsenthil commented Dec 16, 2024

One comment on adding high priority rule is, this changes changes the definition strict mode, and makes communication possible which wasn't previously, by definition.

We introduced POD_SECURITY_GROUP_ENFORCING_MODE knob for these specific user cases as communication of pods on the same host / another service on the same host. The outbound traffic for pods using SGPP when using the standard mode will have the Security Group enforced by branch ENI - in support new SGPP standard mode #1907

We need to evaluate this if this change is required at all.

@bleggett
Copy link

bleggett commented Dec 17, 2024

One comment on adding high priority rule is, this changes changes the definition strict mode, and makes communication possible which wasn't previously, by definition.

We introduced POD_SECURITY_GROUP_ENFORCING_MODE knob for these specific user cases as communication of pods on the same host / another service on the same host. The outbound traffic for pods using SGPP when using the standard mode will have the Security Group enforced by branch ENI - in support new SGPP standard mode #1907

We need to evaluate this if this change is required at all.

This statement specifically I'm not sure is true:

makes communication possible which wasn't previously, by definition.

Or rather, it's moot ATM because vpc-cni simply doesn't route link-local traffic correctly at all in SGPP strict mode, no matter the contents of the SG.

It was suggested that instead of doing what this PR does, explicitly allowlisting link-local CIDRs in SGs would work - but this does not appear to work, the LL packets don't even make it all the way to the ENI for proper enforcement with strict mode (presumably because they're link-local and it's a different link).

So this is still a basic vpc-cni traffic routing bug that needs a variation of the above fix - strict doesn't properly route link-local packets at all, no matter the contents of the SG.

@orsenthil
Copy link
Member Author

Allowing link-local traffic will require more changes than this PR. That brought in the changes that is almost close the introduction of standard mode that was introduced - #1907 - So, the standard mode seems to be only way right now for the link-local traffic.

@bleggett
Copy link

bleggett commented Dec 17, 2024

Allowing link-local traffic will require more changes than this PR. That brought in the changes that is almost close the introduction of standard mode that was introduced - #1907 - So, the standard mode seems to be only way right now for the link-local traffic.

Yes, using standard is the only way at this time. I think that must be fixed in some manner tho. It's not really acceptable for a CNI to mishandle an entire IETF defined CIDR range by default, that at least should be feature-flaggable behavior.

It's not a security concern either - the fundamental problem (I believe) is that link-local traffic cannot make it far enough for SGPP enforcement to apply to it in the first place, so there is nothing to assert policy against either way.

@cb-troydai
Copy link

Today, under the strict mode, we can apply security group rules to allow host process access to pod health check pod. Drop link local prevent us from doing that.

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.

3 participants