fix: Use PortMappingPolicySetting #689
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #689 +/- ##
==========================================
- Coverage 41.90% 38.94% -2.97%
==========================================
Files 72 83 +11
Lines 10151 10697 +546
==========================================
- Hits 4254 4166 -88
- Misses 5426 6033 +607
- Partials 471 498 +27 |
ashvindeodhar
left a comment
ashvindeodhar
left a comment
There was a problem hiding this comment.
Adelina,
To confirm: You are converting the policy to hnsv1 to maintain the compatibility. You are saying a unified way is needed to do similar thing for all policy types in general in subsequent PRs?
cni/network/network_windows.go
Outdated
| Type: "NAT", | ||
| switch mapping.Protocol { | ||
| case "TCP": | ||
| protocol = 6 |
There was a problem hiding this comment.
We have consts defined for these as protocolTCP, protocolUDP
network/policy/policy_windows.go
Outdated
| if err := json.Unmarshal(policy.Data, &endpointPolicy); err != nil { | ||
| return false | ||
| } | ||
| if endpointPolicy.Type == "NAT" { |
There was a problem hiding this comment.
I think line 126 is an issue - you changed the Type from NAT to hnsv2.PortMapping in func getPoliciesFromRuntimeCfg.
Here you need to check if type is hnsv2.PortMapping and not NAT
There was a problem hiding this comment.
Correct! I seem to have missed this :/
network/policy/policy_windows.go
Outdated
| jsonPolicies = append(jsonPolicies, serializedOutboundNatPolicy) | ||
| } | ||
| } | ||
| } else if isPolicyTypeOutBoundNAT := IsPolicyTypeNAT(policy); isPolicyTypeOutBoundNAT { |
There was a problem hiding this comment.
please don't reuse the bool , check is for NAT policy and variable is outboundNAT
There was a problem hiding this comment.
Yeah, sorry, this was a editor autocomplete mistake. Fixing :)
| "github.com/Azure/azure-container-networking/cns" | ||
| "github.com/Azure/azure-container-networking/log" | ||
| "github.com/Azure/azure-container-networking/network" | ||
| "github.com/Azure/azure-container-networking/network/policy" |
There was a problem hiding this comment.
Also share what all testing has been performed? Just containerd or docker has been tested to make HnsV1 is working as earlier too.
There was a problem hiding this comment.
Specifically the portmapping scenario
Yes, that is the ideea. Of course, if there are other plans to phase out hnsv1 completely, that would of course simplify things. But with state right now, yes, I believe there should be subsequent PRs to address the other policy types. |
adelina-t
force-pushed
the
usePortMappingPolicySettings
branch
from
5cd9886 to
730e23d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/lgtm @ninzavivek please approve if you are ok and merge once all the checks are completed. Thanks @adelina-t |
ninzavivek
left a comment
ninzavivek
left a comment
There was a problem hiding this comment.
@adelina-t , please mention what all testing has been done - esp docker container with port mappings.
| return nil, err | ||
| } | ||
| natPolicy := hcsshim.NatPolicy{ | ||
| Type: "NAT", |
There was a problem hiding this comment.
please use constant here.
| "github.com/Azure/azure-container-networking/cns" | ||
| "github.com/Azure/azure-container-networking/log" | ||
| "github.com/Azure/azure-container-networking/network" | ||
| "github.com/Azure/azure-container-networking/network/policy" |
There was a problem hiding this comment.
Specifically the portmapping scenario
In order to support VIPs for container Port Mappings, we should use PortMappingPolicySetting type from HNSv2 instead of the old NatPolicy from HNSv1.
adelina-t
force-pushed
the
usePortMappingPolicySettings
branch
from
730e23d to
c203eb1
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5 participants
In order to support VIPs for container Port Mappings, we should
use PortMappingPolicySetting type from HNSv2 instead of the old
NatPolicy from HNSv1.
Reason for Change:
Kubernetes e2e test: [sig-scheduling] SchedulerPredicates [Serial] validates that there is no conflict between pods with same hostPort but different hostIP and protocol [Conformance] fails on Windows Server 2004 ( as shown here: https://testgrid.k8s.io/sig-windows-containerd#aks-engine-azure-windows-master-2004-containerd-serial-slow ). The reason for this failure is that setting hostIP for port mappings are not supported with Azure-CNI. This PR uses allows setting of VIP by using the PortMappingPolicySetting type instead of NatPolicy.
Notes:
As suggested by @erfrimod , instead of converting From HNSv1 NatPolicy to HNSv2 PortMappingPolicySetting, we should directly unmarshal the policies recieved from the Runtime config into HNSv2 PortMappingPolicySetting. However, if we are to preserve compatibility with HNSv1, a conversion from one to the other is still needed. This is less than ideal and a unified way of approaching this should be introduced for all policy setting types in a subsequent PR.