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

Adding an ipv6 policy for port mapping policies when dual stack is en… #104

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

kestratt
Copy link
Contributor

@kestratt kestratt commented Mar 4, 2024

…abled

Currently only ipv4 port mapping policies are added to the host for dual stack scenarios. Adding an extra policy without the VIP specified to handle ipv6 traffic. HNS will use the network's management ipv6 as the vip for nat-ing

Validation:

  1. Local validation with crictl and port mapping set in the pod config file
  2. Validation in a SF cluster

cni/cni.go Outdated
@@ -355,6 +355,16 @@ func (config *NetworkConfig) GetEndpointInfo(
logrus.Debugf("Created raw policy from mapping: %+v --- %+v", mapping, policy)
epInfo.Policies = append(epInfo.Policies, policy)

if config.OptionalFlags.EnableDualStack {
v6flags := flags | 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does 3 here represent Ipv6 support? Isn't there a constant defined in hcsshim for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. just updated the PR with the hcsshim flags

@kestratt kestratt force-pushed the kestratt/dualstacknatpolicy branch from 98984a7 to 9220588 Compare March 4, 2024 23:23
NatFlagsNone NatFlags = iota
NatFlagsLocalRoutedVip
NatFlagsIPv6
var (
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be making changes in the vendor repo. We should be making the required changes in https://github.com/microsoft/hcsshim and creating a tag when the changes are merged, then revendor hcsshim in this directory.
This will ensure the changes aren't wiped out in the future due to any version upgrade.

github.com/Microsoft/hcsshim v0.8.25

Ref: https://git-scm.com/book/en/v2/Git-Basics-Tagging

Copy link
Contributor Author

@kestratt kestratt Mar 6, 2024

Choose a reason for hiding this comment

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

Yes, the vendor changes were temporarily made and pushed for validation. Local tests were failing due to infra issues. the hcsshim changes weren't needed. Thanks for the comment!

@kestratt kestratt force-pushed the kestratt/dualstacknatpolicy branch from 04e45fd to b598c1a Compare March 6, 2024 22:23
@kestratt kestratt force-pushed the kestratt/dualstacknatpolicy branch from b598c1a to 1d17fb7 Compare March 6, 2024 23:07
@kestratt kestratt merged commit f02d417 into microsoft:master Mar 8, 2024
3 checks passed
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