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

IPv4 Randomize SNAT support for IPv6 pods #1903

Merged
merged 3 commits into from
Mar 8, 2022
Merged

Conversation

achevuru
Copy link
Contributor

@achevuru achevuru commented Mar 3, 2022

What type of PR is this?
Enhancement + Addresses few inconsistencies/bugs with IPv6 mode.

Which issue does this PR fix:
NA

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

  • For Egress IPv4 flows from an IPv6 pod, we SNAT the node-local IPv4 address assigned to an IPv6 Pod to Node's Primary IPv4 address. We enable random-fully by default if the underlying iptables version supports it. PR extends support for AWS_VPC_K8S_CNI_RANDOMIZESNAT to the egress-v4-cni-plugin which will allow user to configure this.
  • Addresses duplicate IPv4 route issue across CNI restarts in IPv6 mode.

Testing done on this change:

  • Validated that random-fully option is only set if AWS_VPC_K8S_CNI_RANDOMIZESNAT is set to prng for egress IPv4 flows.
  • Validated that the node local IPv4 route isn't replicated across CNI restarts.

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:

  • Extends AWS_VPC_K8S_CNI_RANDOMIZESNAT support to egress-v4-cni-plugin.

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

@achevuru achevuru requested a review from a team as a code owner March 3, 2022 20:27
ipt, err := iptables.NewWithProtocol(iptables.ProtocolIPv4)
if err != nil {
return fmt.Errorf("failed to locate iptables: %v", err)
}

rules := iptRules4(target, src, chain, comment, ipt.HasRandomFully())
useRandomFully := true
if randomizeSNAT == "none" {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. shall we support for "--random" as well? i.e. randomizeSNAT == "hashrandom"
  2. we should check ipt.HasRandomFully() as well, and if it's not supported, fallback to either "--random" or none. (depends on question 1)

Copy link
Contributor

@M00nF1sh M00nF1sh 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

@jayanthvn jayanthvn left a comment

Choose a reason for hiding this comment

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

lgtm, but I think we should add some documentation around this, I mean explaining how this works.

@jayanthvn jayanthvn added this to the v1.11.1 milestone Mar 7, 2022
@achevuru
Copy link
Contributor Author

achevuru commented Mar 8, 2022

lgtm, but I think we should add some documentation around this, I mean explaining how this works.

Behavior currently documented under AWS_VPC_K8S_CNI_RANDOMIZESNAT will still be correct. We're extending the env variable out to the secondary CNI plugin as well. We can add a note if required under that. Will track it for README updates prior to 1.11 release.

@achevuru achevuru merged commit 8855418 into aws:master Mar 8, 2022
@jayanthvn
Copy link
Contributor

lgtm, but I think we should add some documentation around this, I mean explaining how this works.

Behavior currently documented under AWS_VPC_K8S_CNI_RANDOMIZESNAT will still be correct. We're extending the env variable out to the secondary CNI plugin as well. We can add a note if required under that. Will track it for README updates prior to 1.11 release.

Sounds good.

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.

None yet

3 participants