cni: Only require ipam.Cidrs when masquerade is enabled#11978
cni: Only require ipam.Cidrs when masquerade is enabled#11978aanm merged 1 commit intocilium:masterfrom
Conversation
|
Please set the appropriate release note label. |
christarazi
left a comment
There was a problem hiding this comment.
Changes look good, just a small comment.
If I understand you correctly, this is a temporary Azure fix until we support masquerading on Azure? If that's the case, happy to merge this in as a 1.8 backport.
There was a problem hiding this comment.
Could you add test cases for masquerade as it's now an extra variable?
|
Please set the appropriate release note label. |
2 similar comments
|
Please set the appropriate release note label. |
|
Please set the appropriate release note label. |
|
Please set the appropriate release note label. |
4e5d562 to
f1f68a6
Compare
|
@christarazi thanks for the review, PR updated. You understood correctly. I don't know if anyone is actively working on masquerade support for Azure at the moment (I'd like to give a stab at it, but can't promise). Yes it would be great to backport to 1.8, so Azure CNI would work when released (albeit not supporting all features AWS does). |
christarazi
left a comment
There was a problem hiding this comment.
Looks great, thanks for the PR!
|
test-me-please |
Since recently, allocations results returned without CIDRs became fatal for CNI (CNI cmdAdd() would fail due to interfaceAdd() failing, due to RoutingInfo.parse() erroring out on `len(cidrs) == 0`). That (now mandatory) `IPv4CIDRs` is used when masquerading is enabled, to source-route traffic from the pods to its attached VPC CIDRs through a dedicated local table (that I assume is exempt from masquerade). But on Azure we don't collect and dont't propagate subnets CIDRs, and that bit of information is missing from ciliumnodes azure crd/ipam. Adding masquerade support to Azure CNI (therefore, collecting and bubbling down per-address CIDRs) would be a nice feature to have in the longer term. But we might be late in the feature freeze to implement that on time to unbreak Azure CNI before 1.8 release; and there's no reason to insist on having CIDRs when masquerade is disabled anyway. Signed-off-by: Benjamin Pineau <benjamin.pineau@datadoghq.com>
f1f68a6 to
cddd622
Compare
|
test-me-please |
Since recently, allocations results returned without CIDRs became fatal
for CNI (CNI cmdAdd() would fail due to interfaceAdd() failing, due to
RoutingInfo.parse() bailing out on
len(cidrs) == 0).That (now mandatory)
IPv4CIDRsis used when masquerading is enabled,to source-route traffic from the pods to its attached VPC CIDRs through
a dedicated local table (that I assume is exempt from masquerade).
But on Azure we don't collect and dont't propagate subnets CIDRs, and
that bit of information is missing from ciliumnodes azure crd/ipam.
Adding masquerade support to Azure CNI (therefore, collecting and bubbling
down per-address CIDRs) would be a nice feature to have in the longer term.
But we might be late in the feature freeze to implement that on time to
unbreak Azure CNI before 1.8 release; and there's no reason to insist on
having CIDRs when masquerade is disabled anyway.
Please ensure your pull request adheres to the following guidelines: