projectcalico
add info to ipampool message in felix
Description
In the context of passing through typha to directly watch calico resources and avoid scale issues in calico/Vpp, this is introduced in order to use felix messages instead of an ipam watcher in calico/Vpp agent. It adds some missing information to the ipamPool message in Felix.
@hedibouattour curious how this is used in VPP - for the other data planes, we use a global option passed in via the configuration to the dataplane.
@caseydavenport It is needed to know the type of every added or updated ippool, is there another way of knowing that? Not sure what global option you are referring to. FYI, this is the issue I am addressing https://github.com/projectcalico/vpp-dataplane/issues/293
Ah yes, I was thinking of this: https://github.com/projectcalico/calico/blob/8b362e2565637599a9a4dd73458f4bf321543c3c/felix/dataplane/driver.go#L223-L229
That's only available to the internal dataplane driver, however I believe ext data plane drivers will still receive a proto.ConfigUpdate message.
However however, I think the field you need - the Encapsulation sub-struct of the ConfigParams structure might not be properly serialized into that message :sweat_smile:
So, perhaps passing this per-pool would be OK. Maybe @coutinhop could look into whether or not the encapsulation field is properly serialized into that message for external data plane implementations - if you used that, it would mean you could take advantage of the existing logic to determine enabled encaps in the calc graph.
That said, this PR seems reasonable enough regardless so might make sense to merge anyway.
Hello @caseydavenport, is there anything that needs to be done in order for this to be merged?
Sorry @caseydavenport and @hedibouattour for missing the notifications before.
@caseydavenport I think the Encapsulation struct is serialized by/for the external dataplane driver in its own message, but not in ConfigUpdate: https://github.com/projectcalico/calico/blob/cd9d293774aa7231aa8959d69760e59cdb055bc1/felix/dataplane/external/ext_dataplane.go#L244
Though I'm not sure it would be better to use it instead of watching the IP pools (@hedibouattour would know better how VPP would use the information). For a little bit of context, this Encapsulation struct is the result of a calculation performed by EncapsulationCalculator, which keeps track of IPIP and VXLAN in pools to determine whether they should be enabled/disabled by Felix: https://github.com/projectcalico/calico/blob/cd9d293774aa7231aa8959d69760e59cdb055bc1/felix/calc/encapsulation_resolver.go#L110
Would it make sense for your use case to look at this instead of watching every IP pool?
Indeed using this message will help us avoid redoing calculations in calico vpp to know enabled encapsulations. However if we use this we need to make some changes in order to receive the message mapping pools to enabled encapsulations: the current encapsulation message in Felix is not enough. So, shall we make this change to that message?
However if we use this we need to make some changes in order to receive the message mapping pools to enabled encapsulations:
If what you really need is a mapping of pool->encap, then I think what you have in this PR is correct.
If you need a global "Is encap X enabled on any pool?" then the existing message is correct.
Sounds like you need the former, so I would suggest we stick with this.
@caseydavenport, a test that seems unrelated to the patch is not passing. Could you please rerun the tests.
[batch=4 pid=10321] [Fail] _BPF_ _BPF-SAFE_ BPF tests (udp, ct=false, log=debug, tunnel=ipip, dsr=false) (kubernetes backend) with a 3 node cluster with a policy allowing ingress to w[0][0] from all regular workloads with icmp blocked from workloads, external client with wrong target port [It] should get port unreachable via node1->node0 fwd