Add NatFlags flag to OutboundNatPolicySetting #1013
Conversation
| @@ -135,6 +135,7 @@ type OutboundNatPolicySetting struct { | |||
| VirtualIP string `json:",omitempty"` | |||
| Exceptions []string `json:",omitempty"` | |||
| Destinations []string `json:",omitempty"` | |||
| Flags NatFlags `json:",omitempty"` | |||
There was a problem hiding this comment.
It appears that the version check for ipv6 support already exists in the form of IPv6DualStackSupported. If more checks need to be added please let me know.
| @@ -135,6 +135,7 @@ type OutboundNatPolicySetting struct { | |||
| VirtualIP string `json:",omitempty"` | |||
| Exceptions []string `json:",omitempty"` | |||
| Destinations []string `json:",omitempty"` | |||
| Flags NatFlags `json:",omitempty"` | |||
There was a problem hiding this comment.
Should an enum be defined as well for each value of NatFlags? Right now its just declared as an int:
// NatFlags are flags for portmappings.
type NatFlags uint32
so I did not add one just for ipv6.
There was a problem hiding this comment.
Yes, I think it's a good idea to define the enum. Something like this
|
@vikas-bh Thanks Vikas. Could you sign off on the commit with |
Signed-off-by: Vikas Bhardwaj <vikasb@microsoft.com>
Signed-off-by: Vikas Bhardwaj <vikasb@microsoft.com>
| @@ -36,6 +36,7 @@ var ( | |||
| LoadBalancerFlagsNone LoadBalancerFlags = 0 | |||
| // LoadBalancerFlagsDSR enables Direct Server Return (DSR) | |||
| LoadBalancerFlagsDSR LoadBalancerFlags = 1 | |||
| LoadBalancerFlagsIPv6 LoadBalancerFlags = 2 | |||
| ) | |||
There was a problem hiding this comment.
Ideally these would be constants also, is there any reason they aren't?
There was a problem hiding this comment.
There was a problem hiding this comment.
I have no idea why they were not made constants in the first place over here :). I would prefer we not change this as these are being used from kubeproxy.
There was a problem hiding this comment.
Just checking in case anyone knew 😆. Them being constants wouldn't affect anything on them pulling in a new release though, so it should be harmless. They'll be the same type (and value), just not reassignable now.
|
@vikas-bh I can probably push a change right after to fix the things that should be constants, unless you want to. Do we want a new release of the shim with these changes? Is this for k8s? |
This is for ipv6 support being added to the Windows CNI (it is under development). I say we hold on for a bit before creating a new release in case there are more things we find which need to be fixed in hcsshim. |
Signed-off-by: Vikas Bhardwaj <vikasb@microsoft.com>
|
@vikas-bh Sounds good! |
|
/lgtm |
Related work items: microsoft#388, microsoft#389, microsoft#393, microsoft#394, microsoft#395, microsoft#396, microsoft#397, microsoft#398, microsoft#399, microsoft#400, microsoft#401, microsoft#402, microsoft#403, microsoft#404, microsoft#405, microsoft#931, microsoft#973, microsoft#1001, microsoft#1003, microsoft#1004, microsoft#1005, microsoft#1006, microsoft#1007, microsoft#1009, microsoft#1010, microsoft#1012, microsoft#1013, microsoft#1014, microsoft#1015, microsoft#1016, microsoft#1017, microsoft#1019, microsoft#1021, microsoft#1022, microsoft#1024, microsoft#1025, microsoft#1027, microsoft#1028, microsoft#1029, microsoft#1030, microsoft#1031, microsoft#1033
This is to allow setting the ipv6 flag when creating outbound nat policy.