[Feature] Route the per-VPC NAT lifecycle through the agent#215
Open
HiranAdikari wants to merge 2 commits into
Open
[Feature] Route the per-VPC NAT lifecycle through the agent#215HiranAdikari wants to merge 2 commits into
HiranAdikari wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds NAT-slice agent capabilities and RBAC coverage, routes KubeOVN NAT and bootstrap operations through ChangesNAT seam migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The NAT slice (F15) of the credential-locality work: the external network bootstrap (provider network, VLAN, external subnet, NAD), the per-VPC NAT gateway lifecycle (gateway, EIP, SNAT rule, default route), the readiness/teardown waiters, and the presence check now run through the cluster-access seam, so a VPC's NAT works in a remote (agent-only) zone. nat.go no longer touches the dynamic client at all. - clusteraccess: six new capability families — provider-networks and vlans (create-only), vpc-nat-gateways, iptables-eips and iptables-snat-rules (get/create/delete), and pods (read-only get/list for the gateway-pod waiters). StatusFallbackOK is explicitly false on every Get-routable family per the documented rule (only the VM family degrades on an old agent). - kubeovn: every direct dynamic call in nat.go swapped to the seam; creates keep their already-exists tolerance and all created objects use fixed, deterministic names (SSA-safe). The dead patchVPCSpec helper and its raw patch are removed. - The remote client's local-only boundary shrinks to the per-VPC DNS ops (ConfigMap-backed) and best-effort local cleanups; the pin test asserts exactly that. - Agent RBAC regenerated: get/create/patch/delete on the three NAT CRDs, create/patch on provider-networks and vlans, get/list on pods (a subset of the generator's existing base inventory rule). Security note: the new grants land in the agent's single cluster-wide ClusterRole like every existing capability. The pods get/list grant duplicates what the base inventory rule already gave the agent, so this slice does not widen the SA's effective read surface; the holistic least-privilege pass over the whole role is tracked separately. Unit-tested (seam tests for the lifecycle, waiters, teardown, remote nil-dynamic coverage, and the capability allow-sets), race-clean, and the RBAC drift check is byte-stable. A live KubeOVN integration run (NAT egress on a real cluster, toggles off and on) remains before merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RC3BDHCFmJBdxvGVW69FAr
HiranAdikari
force-pushed
the
feat/agent-nat-routing
branch
from
be41ed6 to
65b55df
Compare
HiranAdikari
changed the base branch from
feat/agent-network-plumbing
to
controlplane
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flux/platform/dc-agent/base/rbac.yaml`:
- Around line 26-31: The RBAC change is ineffective because the existing
combined nodes/pods ClusterRole rule is still granting the broader permissions.
Update the rules in base/rbac.yaml so the ClusterRole no longer includes the old
nodes+pods get/list/watch entry, and keep only the minimal pods get/list access
in the relevant rule set. If nodes access is still needed, split it into a
separate nodes-only rule with just the required verbs; use the existing
ClusterRole rules block to locate and adjust the permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: beb4d4ae-f044-4b63-a8be-c2d177995b03
📒 Files selected for processing (8)
dc-api/internal/providers/clusteraccess/capabilities.godc-api/internal/providers/clusteraccess/capabilities_test.godc-api/internal/providers/kubeovn/client.godc-api/internal/providers/kubeovn/nat.godc-api/internal/providers/kubeovn/nat_seam_test.godc-api/internal/providers/kubeovn/remote_client_test.godc-api/internal/providers/registry.goflux/platform/dc-agent/base/rbac.yaml
The generated pods get/list rule sat next to the fixed base inventory rule (nodes/pods get/list/watch) that already grants a superset. RBAC rules are additive, so the duplicate granted nothing — and a subset rule beside its superset reads like a failed attempt to narrow the grant, which review flagged. The generator now skips a capability rule whose verbs the base rules fully cover, computed at render time: if a base rule is ever removed (the tracked least-privilege pass), regeneration re-emits the capability rule that then carries the grant. Partially covered rules are still emitted whole. Regenerated rbac.yaml drops the pods line; a test pins both the subsumed and partially-covered cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RC3BDHCFmJBdxvGVW69FAr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The NAT slice (F15) of the credential-locality work: the external-network bootstrap (ProviderNetwork, Vlan, external Subnet, NAD), the per-VPC NAT gateway lifecycle (VpcNatGateway, IptablesEIP, IptablesSnatRule, the VPC default route), the readiness/teardown waiters, and the presence check now run through the cluster-access seam — so per-VPC NAT works in a remote, agent-only zone.
nat.gono longer references the direct dynamic client at all.Builds on the now-merged #214 (the NSG/route/peering slice): the default-route add/remove reuses that PR's seam-routed
patchVPCStaticRouteswith the dedicated static-routes fieldManager.Changes
provider-networks+vlans(create-only bootstrap objects),vpc-nat-gateways+iptables-eips+iptables-snat-rules(get/create/delete), andpods(read-only get/list, for the gateway-pod waiters).StatusFallbackOKis explicitlyfalseon every Get-routable family per the documented rule — only the VM family degrades to a status-only read on an old agent; these error clearly instead.nat.goswapped to the seam. Creates keep their already-exists tolerance (local POST path; the agent path is SSA and inherently idempotent), and all created objects already used fixed deterministic names, which SSA requires. The deadpatchVPCSpechelper (and its raw patch) is removed — the package's pre-existing lint-issue count drops by one.RBAC (grows this time — please review)
Regenerated agent ClusterRole gains:
get/create/patch/deleteon the three NAT CRDs andcreate/patchonprovider-networks+vlans. The pods capability routes get/list but emits no RBAC rule at all: the generator now skips rules fully subsumed by its fixed base inventory rule (nodes/pods get/list/watch), so this slice does not widen the agent's effective read surface — the capability entry exists for GVK mapping and the routing allow-set. (Subsumption is computed at render time, so if the base rule is ever narrowed in the tracked least-privilege pass, regeneration re-emits the pods rule that then carries the grant.) All grants land in the single cluster-wide ClusterRole like every existing capability; the holistic least-privilege pass over the whole role is tracked separately.Verification
Both build + vet clean (including the integration tag); full unit suite passes — 0 fail / 0 skip, race-clean on the touched packages; the RBAC drift check is byte-stable after regeneration; golangci-lint adds 0 new issues (and drops 1 pre-existing). Seam tests cover the lifecycle creates (GVRs, fixed names), the delete path's NotFound tolerance, the EIP/SNAT/pod waiters reading through the seam, remote (nil-dynamic) coverage, and the new families' exact allow-sets.
Remaining pre-merge checks
Per repo policy for KubeOVN driver changes: a live integration run against a real KubeOVN cluster (NAT egress end-to-end with routing off, then on with a connected agent), and the structural diff of generated NAT CRs against known-good live objects. Needs cluster access.
Not included
Per-VPC DNS (F20 — ConfigMap + Deployment families) is the next and final slice; the DNS ops remain behind the local-only error. Best-effort local cleanups stay deliberately remote-skipped.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes