Multigateway support for Egress Gateway#39304
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
github-actions
bot
added
the
kind/community-contribution
carlos-abad
force-pushed
the
multigateway
branch
2 times, most recently
from
f3cffa3 to
72f2914
Compare
tommyp1ckles
added
the
release-note/major
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
tommyp1ckles
added
dont-merge/needs-release-note-label
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Some nits on the docs.
Regarding the UX, I agree with Tom: do we really need this requirement on the first item of the list being in egressGateway as well?
carlos-abad
force-pushed
the
multigateway
branch
2 times, most recently
from
38feeee to
ece11c1
Compare
Not really. I removed that requirement following Tom's guidance. |
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Looks OK from my side, thank you!
derailed
left a comment
derailed
left a comment
There was a problem hiding this comment.
@carlos-abad Nice work!
|
/test |
Head branch was pushed to by a user without write access
yeah that ones a known flake |
|
/test |
giorio94
left a comment
giorio94
left a comment
There was a problem hiding this comment.
Thanks! A couple of comments inline for my codeowners.
Signed-off-by: Carlos Abad <[email protected]>
Since the boolean v6needed is determined from the CEGP destinationCIDRs field, we can move it to the PolicyConfig object rather than the PolicyGatewayConfig. This change will be very useful for the multigateway Egress Policy since we plan to have multiple policyGatewayConfig objects per PolicyConfig. This change will safe from storing the exact same boolean value in all policyGatewayConfig objects. Signed-off-by: Carlos Abad <[email protected]>
This change adds support for parsing the multiple gateways from a multigateway CEGP and storing them in a PolicyConfig object. This change doesn't modify how the BPF tables are written. Signed-off-by: Carlos Abad <[email protected]>
As discussed in the design the assingment follows a round-robin algorithm. To create repeatable and stable assignments the lists of gaterways and endpoints are ordered before the rounds robin-assignment. Design: CFP-38341-multigateway-egress-gateway-policy Signed-off-by: Carlos Abad <[email protected]>
Signed-off-by: Carlos Abad <[email protected]>
Head branch was pushed to by a user without write access
To run this test manually please set the kind cluster using the egressgw preset: - make kind-egressgw - make kind-image - make kind-egressgw-install-cilium And run it in unsafe mode. cilium connectivity test --include-unsafe-tests --test seq-egress-gateway-multigateway Signed-off-by: Carlos Abad <[email protected]>
|
/test |
|
The Maybe it's a flake caused by a pod randomly restarting? If so, can someone rerun the tests? |
| - egressGateway: | ||
| nodeSelector: |
There was a problem hiding this comment.
Heads up that these changes were broken on arrival. Evidently we don't have CI coverage for this file, otherwise we would have noticed this earlier. But also in this case the developer didn't test this file and the reviewers didn't test this file, so we merged it in a broken state. This was fixed in #40112.
cc @cilium/egress-gateway as reviewers should we be doing anything differently to avoid introducing breakage like this into the tree?
There was a problem hiding this comment.
I recommend we introduce automatic testing for the examples. Maybe with a separate Github flow?
By the way, just mentioning that this file already had a bug in it. When I added the new field I replicated the field above, duplicating the existing bug.
There was a problem hiding this comment.
I agree automated testing of example YAML files would be better than expect reviewers catch every indentation and whatnot issues.
There was a problem hiding this comment.
Yeah improving the automated testing seems reasonable. In this case, having a short test to load the YAML using the Cilium libraries to verify that it matches the right syntax would have helped; it doesn't have to be an end-to-end test.
By the way, just mentioning that this file already had a bug in it. When I added the new field I replicated the field above, duplicating the existing bug.
If you know about a bug, I would encourage you to submit a fix for it.
In this case the bug I was referring to is introduced by this specific change, because EgressGateways is defined as []EgressGateway and EgressGateway does not have a egressGateway field inside of it.
12 participants
Add Multigateway support to egress gateway.
Link to the CFP
Fixes: #38341