[cni]: dualstack overlay hostIP issue hotfix #2290
Conversation
paulyufan2
force-pushed
the
hostIPIssue
branch
2 times, most recently
from
09996d5 to
e0d830f
Compare
cni/network/network_windows.go
Outdated
| @@ -264,59 +263,55 @@ func getPoliciesFromRuntimeCfg(nwCfg *cni.NetworkConfig, isIPv6Enabled bool) []p | |||
| protocol = policy.ProtocolUdp | |||
| } | |||
|
|
|||
| hostIP, err := netip.ParseAddr(mapping.HostIp) | |||
| if err != nil { | |||
| return nil, errors.Wrap(err, "failed to parse hostIP") | |||
There was a problem hiding this comment.
Can we pass the incorrect hostIP as well in the error ?
cni/network/network_windows.go
Outdated
| rawPolicy, _ := json.Marshal(&hnsv2.PortMappingPolicySetting{ | ||
| // To support hostport policy mapping for ipv6 in dualstack overlay mode | ||
| // uint32 NatFlagsIPv6 = 2 | ||
| var flag hnsv2.NatFlags |
There was a problem hiding this comment.
So earlier we were adding ipv6 policies based on cniTypesCurr.Result (which is from IPAM i suppose), but now we are directly reading the hostIP address type(v4 or v6) and based on that adding the flags ?
What if the hostip is not provided ?(is that a valid case ?)
cni/network/network.go
Outdated
| epPolicies, err := getPoliciesFromRuntimeCfg(opt.nwCfg) | ||
| if err != nil { | ||
| logger.Error("failed to get policies from runtime configurations", zap.Error(err)) | ||
| return epInfo, err |
There was a problem hiding this comment.
im surprised the linter doesn't complain about not wrapping the error
There was a problem hiding this comment.
This is using zap logger here
There was a problem hiding this comment.
it's not about logging, it's about the error returned, unless that linter is not enabled for checking that errors are wrapped? @rbtr
There was a problem hiding this comment.
@paulyufan2 can you wrap this error before returning?
paulyufan2
force-pushed
the
hostIPIssue
branch
from
2e4965b to
27cf355
Compare
paulyufan2
force-pushed
the
hostIPIssue
branch
2 times, most recently
from
dca20ff to
475486e
Compare
paulyufan2
force-pushed
the
hostIPIssue
branch
3 times, most recently
from
e1ebd65 to
1258288
Compare
paulyufan2
force-pushed
the
hostIPIssue
branch
from
2155a14 to
602c3bd
Compare
paulyufan2
force-pushed
the
hostIPIssue
branch
from
602c3bd to
ce61393
Compare
There was a problem hiding this comment.
Let's add more test cases involving ipv6 hostip as well.
There was a problem hiding this comment.
added an ipv6 UT test
Reason for Change:
This PR is to fix this issue: https://dev.azure.com/msazure/One/_workitems/edit/25403093/
If no hostIP is provided, then we assume to use ipv4 flag, if hostIP is provided and it's ipv6 address, then we set flag to ipv6 HNS flag.
Issue Fixed:
Requirements:
Notes: