feat(gateway): Add NetworkGateway/NetworkRule controllers and webhook - #351
Conversation
f810d87 to
f0f9d98
Compare
9a8c9b2 to
d34659a
Compare
d34659a to
623af9e
Compare
…on webhook Adds the CRD-driven layer that feeds the gateway engine from the previous branch, and the NetworkRule admission webhook: - internal/controller/networkgateway_controller.go: NetworkGateway reconciler; watches NetworkGateway/NetworkRule, resolves backend SRv6 uSIDs (usidresolver.go), builds DesiredRule/DesiredBackend, and drives internal/gateway's Engine. - internal/controller/networkrule_controller.go: NetworkRule reconciler; finalizer, Status.PrimaryNode assignment, BGP-withdrawal- before-NAT-teardown delete ordering. - internal/controller/status.go: setGatewayCondition/setRuleCondition helpers. - internal/webhook: NetworkRuleValidator admission webhook (vpcRef/vpcAttachmentRef authorization) and a pluggable Authorizer interface (AllowAllAuthorizer placeholder pending the companion operator integration). - cmd/galactic-router/root.go: wires the webhook server into the manager, opt-in behind --webhook-enabled (internal/config/router.go). - go.mod: bump go.datum.net/network to the commit that adds the NetworkGateway/NetworkRule types (datum-cloud/network#14), now that they've landed upstream. This replaces an earlier, since-abandoned 'replace go.datum.net/network => ../network' stopgap that pointed at a sibling checkout while those types were still only local. - scripts/ci.sh: NETWORK_SHA extraction matches the require line's module path exactly, not just a substring. - containers/galactic-router/Dockerfile: also regenerates internal/plumbing/ebpf/edgeprog's bpf2go output now, alongside internal/plumbing/ebpf/prog's -- the reconcilers added here wire internal/gateway.Engine into cmd/galactic-router for the first time, which transitively needs edgeprog's generated types. Known gap, not addressed by this branch: config/webhook/ (the ValidatingWebhookConfiguration + Service + cert-manager Certificate this webhook needs to actually run) doesn't exist yet. The webhook defaults to disabled (WebhookEnabled=false), so this doesn't affect any existing deployment, but the feature is incomplete without those manifests -- follow-up work, not in scope here. Third branch in the edge-gateway stack; builds on feat/edge-gateway-02-engine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
623af9e to
b9c9b57
Compare
ecv
left a comment
ecv
left a comment
There was a problem hiding this comment.
Holding off on approval for one line, not for the controllers.
go.mod:117 on this branch adds replace go.datum.net/network => ../network, and that takes the whole module down anywhere the sibling checkout does not exist. Lint, Build, Unit Tests and Unit Tests (root) are all failing here on internal/cnibgp/bgp.go, which has nothing to do with this change. Every branch below inherits it, which is why #350 is the only green PR in the stack. Filed as #358 with the full trail.
The good news is it looks unblocked: datum-cloud/network's default branch already carries api/v1alpha1/gateway_types.go and rule_types.go, so a re-pin and deleting the replace should be the whole fix, with a compile to confirm the shape has not drifted since your local copy.
On the controllers themselves, no objection. The finalizer-guarded teardown ordering in NetworkRuleReconciler is the part I looked hardest at and it holds up: the BGP route is withdrawn before the datapath state goes, which is the order that matters.
One thing I want to flag as intentional rather than as a finding, since a reader landing here cold will wonder: NetworkGatewayReconciler and NetworkRuleReconciler are not registered with any manager on this branch, and the RBAC for the new resources is not here either. Both arrive later in the stack, in #352 and #354. That is the stack working as designed, not dead code.
Happy to approve as soon as the replace is gone.
|
Went back through the controllers properly, rather than stopping at the build failure. Four things, filed rather than left inline. The one I would fix before this merges#364. Every gateway node's process watches every gateway object in the namespace, and the node check that scopes work to "my own gateway" reads the target off the object. On a delete there is no object to read, so the not-found branch stops the engine unconditionally. Delete one node's gateway and every other gateway node in that namespace empties its rule table. That is the opposite of what active-active is for, and it is quiet: each node believes it did the right thing. Follow-ups#365. Advertisement wiring, self-address publication and the orphan sweep all log their errors and continue, and the Ready condition is computed from the engine result alone. A node that programmed every rule and advertised none reports #367. Two timing gaps. A rule created before any gateway node registers waits for the informer resync, not for the nodes, because this reconciler watches only rules. The comment on that path says a gateway event re-triggers it and then correctly says the opposite in the same sentence. Separately, teardown rebuilds advertisement names from currently-registered nodes, so an advertisement created for a node that has since left is never withdrawn while the finalizer comes off anyway. #366. The authorization path is complete in code and inert in practice: the placeholder authorizer is what the production caller passes, and no webhook configuration is deployed anywhere in the stack. Worth separating because the Accepted condition is granted on "gateway nodes exist" and is what gates programming, so the name promises a check that never ran. All of it is documented in comments, so nothing here was hidden. It is the next reader I care about. What reads wellThe teardown ordering is right: route withdrawal precedes NAT release, and the finalizer's doc comment is honest about what it does not coordinate across nodes.
Deliberately not reporting: the reconcilers not being registered with any manager here, and the RBAC for the new resources being absent. Both arrive in #352 and #354. That is the stack working. Still holding approval on #358 rather than on any of the above. |
…eway Every gateway node's process reconciles every NetworkGateway in the namespace (SetupWithManager has no predicate), and the node-targeting check only runs after a successful Get. On NotFound the reconciler had no way to tell whose object had just been deleted, so it assumed every deletion was its own and called Engine.Stop unconditionally -- tearing down every forwarding rule this node holds. Since Engine.Stop walks e.active and removes every rule, deleting one gateway node's NetworkGateway silently dropped the data plane on every other gateway node in the namespace too, inverting the active-active design's blast radius: draining one node took out all of them, and nothing reprogrammed the survivors until their own gateway object reconciled again (next rule change or cache resync). On NotFound, ask whether some NetworkGateway still targets this node (isGatewayNode, already used by NetworkRuleReconciler for the same purpose) instead of assuming the deletion was this node's own. If one still does, this node's own object is untouched and its engine keeps running; only stop when this node no longer has a NetworkGateway of its own. Adds TestNetworkGatewayReconciler_IgnoresDeletionOfOtherNodesGateway: two gateway nodes, delete the other node's object, assert this node's engine is not stopped. Fixes #364 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
fixes #364 |
2 participants
Summary
The gateway engine from the previous PR needs something to feed it desired state from the cluster. This adds the NetworkGateway and NetworkRule reconcilers that watch those CRDs, resolve each backend's SRv6 uSID, and drive the engine, plus a NetworkRule admission webhook that authorizes VPC ownership before a rule is created. The webhook is wired into galactic-router behind an opt-in flag, disabled by default. Third branch in the edge-gateway stack; builds on the control-plane engine PR.
Test plan
task test:unit)task lintandtask buildare cleanNote
The webhook's ValidatingWebhookConfiguration, Service, and cert-manager wiring don't exist yet. It stays disabled by default, so nothing here is affected, but the feature isn't deployable end-to-end until that follow-up lands.
Related to #17