From 65b55dfbf74ef6a74859078dfaa755a656bf66ce Mon Sep 17 00:00:00 2001 From: HiranAdikari Date: Fri, 3 Jul 2026 11:56:21 +0530 Subject: [PATCH 1/2] Route the per-VPC NAT lifecycle via the agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01RC3BDHCFmJBdxvGVW69FAr --- .../providers/clusteraccess/capabilities.go | 129 +++- .../clusteraccess/capabilities_test.go | 102 ++- dc-api/internal/providers/kubeovn/client.go | 66 +- dc-api/internal/providers/kubeovn/nat.go | 92 ++- .../providers/kubeovn/nat_seam_test.go | 703 ++++++++++++++++++ .../providers/kubeovn/remote_client_test.go | 21 +- dc-api/internal/providers/registry.go | 5 +- flux/platform/dc-agent/base/rbac.yaml | 18 + 8 files changed, 1053 insertions(+), 83 deletions(-) create mode 100644 dc-api/internal/providers/kubeovn/nat_seam_test.go diff --git a/dc-api/internal/providers/clusteraccess/capabilities.go b/dc-api/internal/providers/clusteraccess/capabilities.go index cc6514d8..94714b7d 100644 --- a/dc-api/internal/providers/clusteraccess/capabilities.go +++ b/dc-api/internal/providers/clusteraccess/capabilities.go @@ -227,19 +227,127 @@ var ( // Secret (create for the direct POST, patch because the agent create is SSA). AgentVerbs: []Verb{VerbGet, VerbCreate, VerbApply}, } + // providerNetworkCapability onboards the ProviderNetwork create that + // EnsureExternalNetworkBootstrap routes through the kubeovn seam (the F15 NAT + // slice). Cluster-scoped: binds a host bridge to the KubeOVN SDN. The GVR + // matches kubeovn.providerNetworkGVR exactly. + providerNetworkCapability = AgentCapability{ + GVR: schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "provider-networks"}, + APIVersion: "kubeovn.io/v1", + Kind: "ProviderNetwork", + Namespaced: false, + // dc-api routes only the idempotent bootstrap create — nothing reads, + // lists, or deletes a ProviderNetwork through the seam. + RouteVerbs: []Verb{VerbCreate}, + // SA grant: create + patch, because the AgentBacked create is a server-side + // apply (VerbApply→patch). Mirrors serviceAccountCapability's write grant. + AgentVerbs: []Verb{VerbCreate, VerbApply}, + } + // vlanCapability onboards the Vlan create that EnsureExternalNetworkBootstrap + // routes through the kubeovn seam (the F15 NAT slice). Cluster-scoped: + // references the ProviderNetwork and carries the external VLAN id. + vlanCapability = AgentCapability{ + GVR: schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "vlans"}, + APIVersion: "kubeovn.io/v1", + Kind: "Vlan", + Namespaced: false, + // dc-api routes only the idempotent bootstrap create (same shape as + // providerNetworkCapability). + RouteVerbs: []Verb{VerbCreate}, + // SA grant: create + patch (SSA is the agent's create mechanism). + AgentVerbs: []Verb{VerbCreate, VerbApply}, + } + // vpcNatGatewayCapability onboards the VpcNatGateway lifecycle that + // EnsureVpcNAT/DeleteVpcNAT/IsVpcNATPresent route through the kubeovn seam + // (the F15 NAT slice). Cluster-scoped in KubeOVN (the gateway CRD carries no + // namespace; only the StatefulSet pod it spawns lives in kube-system). + vpcNatGatewayCapability = AgentCapability{ + GVR: schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "vpc-nat-gateways"}, + APIVersion: "kubeovn.io/v1", + Kind: "VpcNatGateway", + Namespaced: false, + // dc-api routes the create (EnsureVpcNAT), the presence read + // (IsVpcNATPresent), and the delete (DeleteVpcNAT). No list/apply — no NAT + // op lists gateways or applies a spec field, and RouteVerbs never widen + // ahead of the verb actually being routed. + RouteVerbs: []Verb{VerbGet, VerbCreate, VerbDelete}, + // SA grant: get for the presence read, create + patch for the create (the + // agent create is SSA), delete for the teardown. + AgentVerbs: []Verb{VerbGet, VerbCreate, VerbApply, VerbDelete}, + // Explicitly false (the documented rule: only the VM family degrades to a + // status-only read on an old agent; every other family errors clearly). + StatusFallbackOK: false, + } + // iptablesEIPCapability onboards the IptablesEIP lifecycle that EnsureVpcNAT/ + // DeleteVpcNAT/IsVpcNATPresent route through the kubeovn seam. Cluster-scoped. + // The seam Get here is a FULL-object consumer in spirit (waitForEIPReady and + // readEIPAssignedIP read .status.ready/.status.ip), but StatusFallbackOK stays + // false per the documented rule: only the VM family sets it. + iptablesEIPCapability = AgentCapability{ + GVR: schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "iptables-eips"}, + APIVersion: "kubeovn.io/v1", + Kind: "IptablesEIP", + Namespaced: false, + // Same routable set as vpcNatGatewayCapability: Get (presence check + + // readiness poll + assigned-IP read), Create, Delete. + RouteVerbs: []Verb{VerbGet, VerbCreate, VerbDelete}, + AgentVerbs: []Verb{VerbGet, VerbCreate, VerbApply, VerbDelete}, + // Explicitly false — see the capability doc comment above: a status-only + // degrade WOULD serve these .status readers, but only the VM family opts + // in; an old agent errors clearly instead. + StatusFallbackOK: false, + } + // iptablesSnatRuleCapability onboards the IptablesSnatRule lifecycle that + // EnsureVpcNAT/DeleteVpcNAT route through the kubeovn seam. Cluster-scoped. + iptablesSnatRuleCapability = AgentCapability{ + GVR: schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "iptables-snat-rules"}, + APIVersion: "kubeovn.io/v1", + Kind: "IptablesSnatRule", + Namespaced: false, + // Same routable set as the other two NAT families: Get (readiness poll), + // Create, Delete. + RouteVerbs: []Verb{VerbGet, VerbCreate, VerbDelete}, + AgentVerbs: []Verb{VerbGet, VerbCreate, VerbApply, VerbDelete}, + // Explicitly false (the documented rule: only the VM family opts in). + StatusFallbackOK: false, + } + // podCapability onboards the READ-ONLY pod ops the NAT lifecycle needs: + // waitForPodRunning polls the gateway pod's status.phase (Get) and + // WaitVpcNATPodsGone polls the gateway StatefulSet's pods by label selector + // (List). Deliberately NO write verbs — dc-api never mutates a pod through the + // seam, and declaring only reads keeps the routable surface (and the agent's + // RBAC) least-privilege. Note the emitted RBAC rule (get,list) is a subset of + // the generator's fixed base inventory rule (nodes/pods get,list,watch), so + // this capability grants the agent's SA nothing it did not already have — it + // exists for GVK mapping and the routing allow-set. + podCapability = AgentCapability{ + GVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + APIVersion: "v1", + Kind: "Pod", + Namespaced: true, + RouteVerbs: []Verb{VerbGet, VerbList}, + AgentVerbs: []Verb{VerbGet, VerbList}, + // Explicitly false (the documented rule: only the VM family opts in — + // waitForPodRunning reads .status.phase but an old agent errors clearly). + StatusFallbackOK: false, + } ) // AgentCapabilities is THE registry: one declaration per resource family. // // Onboarded (RouteVerbs + AgentVerbs set): vmCapability, the three network // families nadCapability/vpcCapability/subnetCapability (the kubeovn CRD CRUD -// slice), vmImageCapability (read/list path + image create), and the three +// slice), vmImageCapability (read/list path + image create), the three // cloud-provider SA bootstrap families serviceAccountCapability/ // roleBindingCapability/secretCapability (F32 — EnsureCloudProviderSA routed -// through the harvester seam). vmiCapability stays a GVK-only pre-seeded entry -// that keeps the wire mapper a superset without granting any routing or RBAC. -// New families are added here (one struct each) in later phases — never by -// editing the mapper, the allow-set switch, or the RBAC YAML by hand. +// through the harvester seam), and the six NAT-slice families +// providerNetworkCapability/vlanCapability/vpcNatGatewayCapability/ +// iptablesEIPCapability/iptablesSnatRuleCapability/podCapability (F15 — the +// per-VPC NAT lifecycle routed through the kubeovn seam; pods read-only). +// vmiCapability stays a GVK-only pre-seeded entry that keeps the wire mapper a +// superset without granting any routing or RBAC. New families are added here +// (one struct each) in later phases — never by editing the mapper, the +// allow-set switch, or the RBAC YAML by hand. var AgentCapabilities = []AgentCapability{ vmCapability, vmiCapability, @@ -250,6 +358,12 @@ var AgentCapabilities = []AgentCapability{ serviceAccountCapability, roleBindingCapability, secretCapability, + providerNetworkCapability, + vlanCapability, + vpcNatGatewayCapability, + iptablesEIPCapability, + iptablesSnatRuleCapability, + podCapability, } // Registry returns the declared capabilities. @@ -297,8 +411,9 @@ func buildDerived() { // routed through the Vpc/Subnet families; the image family also routes // VerbCreate for CreateImage, but VM/NAD already contribute it so the union is // unchanged). The SA/RoleBinding/Secret families (F32) route only Get/Create, -// both already in the union, so the union is unchanged by them too. -// agentDecision consults this for +// both already in the union, so the union is unchanged by them too — as are the +// six NAT-slice families (F15), whose Get/List/Create/Delete are all already +// members. agentDecision consults this for // membership, then gates reads vs writes by the per-family env toggles // (VerbList falls on the read side — see IsReadVerb). // diff --git a/dc-api/internal/providers/clusteraccess/capabilities_test.go b/dc-api/internal/providers/clusteraccess/capabilities_test.go index fffe4b1d..d36e8a34 100644 --- a/dc-api/internal/providers/clusteraccess/capabilities_test.go +++ b/dc-api/internal/providers/clusteraccess/capabilities_test.go @@ -221,10 +221,102 @@ func TestRoutableVerbs_CloudProviderSAFamilies(t *testing.T) { } } +// TestRoutableVerbs_NATFamilies pins the routable sets for the five kubeovn +// families the F15 NAT slice onboards: ProviderNetwork and Vlan route exactly +// {Create} (the idempotent external-network bootstrap creates); VpcNatGateway, +// IptablesEIP, and IptablesSnatRule route exactly {Get, Create, Delete} (the +// per-VPC NAT lifecycle: presence/readiness reads, creates, teardown deletes). +// No NAT family routes Apply/Update/List/Watch — no NAT op applies a spec field +// or lists these CRDs, and RouteVerbs never widen ahead of the routed verb. All +// keep StatusFallbackOK=false (only the VM family sets it): the EIP readiness/ +// assigned-IP reads DO consume status, but the documented rule stands and an +// old agent yields a clear error instead of a silent partial. +func TestRoutableVerbs_NATFamilies(t *testing.T) { + bootstrapOnly := []schema.GroupVersionResource{ + {Group: "kubeovn.io", Version: "v1", Resource: "provider-networks"}, + {Group: "kubeovn.io", Version: "v1", Resource: "vlans"}, + } + for _, gvr := range bootstrapOnly { + verbs, ok := RoutableVerbs(gvr) + if !ok { + t.Errorf("%s must be an onboarded routable family", gvr.Resource) + continue + } + if !verbs[VerbCreate] { + t.Errorf("%s must route VerbCreate, got %v", gvr.Resource, verbs) + } + if len(verbs) != 1 { + t.Errorf("%s routable set = %v, want exactly {Create}", gvr.Resource, verbs) + } + if StatusFallbackOK(gvr) { + t.Errorf("%s must keep StatusFallbackOK=false", gvr.Resource) + } + } + + lifecycle := []schema.GroupVersionResource{ + {Group: "kubeovn.io", Version: "v1", Resource: "vpc-nat-gateways"}, + {Group: "kubeovn.io", Version: "v1", Resource: "iptables-eips"}, + {Group: "kubeovn.io", Version: "v1", Resource: "iptables-snat-rules"}, + } + for _, gvr := range lifecycle { + verbs, ok := RoutableVerbs(gvr) + if !ok { + t.Errorf("%s must be an onboarded routable family", gvr.Resource) + continue + } + for _, v := range []Verb{VerbGet, VerbCreate, VerbDelete} { + if !verbs[v] { + t.Errorf("%s must route %v, got %v", gvr.Resource, v, verbs) + } + } + for _, v := range []Verb{VerbList, VerbApply, VerbUpdate, VerbWatch} { + if verbs[v] { + t.Errorf("%s must NOT route %v", gvr.Resource, v) + } + } + if len(verbs) != 3 { + t.Errorf("%s routable set = %v, want exactly {Get, Create, Delete}", gvr.Resource, verbs) + } + if StatusFallbackOK(gvr) { + t.Errorf("%s must keep StatusFallbackOK=false", gvr.Resource) + } + } +} + +// TestRoutableVerbs_PodsReadOnly pins the pod family (F15 NAT slice) to exactly +// {Get, List} — the gateway-pod status poll and the pods-gone label-selector +// poll. NO write verb may ever be routable for pods: dc-api never mutates a pod +// through the seam, and this pin is the regression guard against a future slice +// accidentally widening a core-group workload family into the write path. +func TestRoutableVerbs_PodsReadOnly(t *testing.T) { + podGVR := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} + verbs, ok := RoutableVerbs(podGVR) + if !ok { + t.Fatal("pods must be an onboarded routable family") + } + for _, v := range []Verb{VerbGet, VerbList} { + if !verbs[v] { + t.Errorf("pods must route %v, got %v", v, verbs) + } + } + for _, v := range []Verb{VerbCreate, VerbApply, VerbUpdate, VerbDelete, VerbWatch} { + if verbs[v] { + t.Errorf("pods must NOT route %v — the family is READ-ONLY", v) + } + } + if len(verbs) != 2 { + t.Errorf("pods routable set = %v, want exactly {Get, List}", verbs) + } + if StatusFallbackOK(podGVR) { + t.Error("pods must keep StatusFallbackOK=false (only the VM family sets it)") + } +} + // TestDefaultGVKMapperResolvesKnownGVRs pins the derived GVK table to the -// onboarded entries (and same APIVersion/Kind). The six original entries plus the +// onboarded entries (and same APIVersion/Kind). The six original entries, the // three cloud-provider-SA bootstrap families (serviceaccounts, rolebindings, -// secrets — F32). +// secrets — F32), and the six NAT-slice families (provider-networks, vlans, +// vpc-nat-gateways, iptables-eips, iptables-snat-rules, pods — F15). func TestDefaultGVKMapperResolvesKnownGVRs(t *testing.T) { m := DefaultGVKMapper() cases := []struct { @@ -241,6 +333,12 @@ func TestDefaultGVKMapperResolvesKnownGVRs(t *testing.T) { {schema.GroupVersionResource{Group: "", Version: "v1", Resource: "serviceaccounts"}, "v1", "ServiceAccount"}, {schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "rolebindings"}, "rbac.authorization.k8s.io/v1", "RoleBinding"}, {schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, "v1", "Secret"}, + {schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "provider-networks"}, "kubeovn.io/v1", "ProviderNetwork"}, + {schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "vlans"}, "kubeovn.io/v1", "Vlan"}, + {schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "vpc-nat-gateways"}, "kubeovn.io/v1", "VpcNatGateway"}, + {schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "iptables-eips"}, "kubeovn.io/v1", "IptablesEIP"}, + {schema.GroupVersionResource{Group: "kubeovn.io", Version: "v1", Resource: "iptables-snat-rules"}, "kubeovn.io/v1", "IptablesSnatRule"}, + {schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, "v1", "Pod"}, } for _, c := range cases { av, k, ok := m.GVK(c.gvr) diff --git a/dc-api/internal/providers/kubeovn/client.go b/dc-api/internal/providers/kubeovn/client.go index 865e2666..e98feb94 100644 --- a/dc-api/internal/providers/kubeovn/client.go +++ b/dc-api/internal/providers/kubeovn/client.go @@ -182,12 +182,16 @@ type Client struct { // pre-seam behaviour) unless WithRoutedAccessor injects a Routed accessor. // Routed through it: the CRD CRUD lifecycle of the three onboarded network // families — CreateVNet/GetVNet/DeleteVNet (Vpc), the Subnet+NAD - // create/get/delete in CreateSubnet/GetSubnet/DeleteSubnet — AND the - // VPC/Subnet spec-write plumbing (peering spec.vpcPeerings, route-table + // create/get/delete in CreateSubnet/GetSubnet/DeleteSubnet — the VPC/Subnet + // spec-write plumbing (peering spec.vpcPeerings, route-table // spec.staticRoutes, NSG spec.acls), whose read-modify-write ops read via the // seam Get and write via the seam Apply (SSA of a minimal single-field - // object). Still local-only on c.dynamic: the stuck-finalizer recovery, DNS - // zone/record ConfigMaps, NAT/per-VPC-CoreDNS plumbing, and namespace/quota + // object) — AND the F15 NAT lifecycle (nat.go: the external-network bootstrap + // plus EnsureVpcNAT/DeleteVpcNAT/IsVpcNATPresent/WaitVpcNATPodsGone on the + // provider-network/vlan/vpc-nat-gateway/iptables-eip/iptables-snat-rule + // families, with read-only pod polls). Still local-only on c.dynamic: the + // stuck-finalizer recovery and other best-effort local cleanups, DNS + // zone/record ConfigMaps, per-VPC-CoreDNS plumbing, and namespace/quota // provisioning — those GVRs/verbs are not in the agent's mapper/RBAC and are // deferred to a later slice. The seam never leaks an agent concept past the // NetworkProvider interface. @@ -209,44 +213,48 @@ type Client struct { // remoteRegion/remoteZone are non-empty ONLY for a credential-free REMOTE // client built by NewRemoteClient. For such a client c.dynamic is nil but the // access seam is an agent-only Routed accessor, so the ONBOARDED CRD lifecycle - // (VPC/Subnet/NAD create/get/delete) AND the VPC/Subnet spec-write plumbing - // (peering, static routes, ACLs) DO route through the zone's agent — every - // such method runs its k8s I/O through c.access, never c.dynamic. Only the - // remaining plumbing (NAT, per-VPC DNS/CoreDNS, DNS zone/record ConfigMaps, - // namespace/quota provisioning) is local-only for a remote zone and returns a - // clear localOnlyErr instead of dereferencing the nil dynamic client; the - // best-effort local cleanups (stuck-finalizer recovery, the pre-delete ACL - // clear) are silently skipped, with the agent-side delete owning finalizer - // convergence. Empty for the LOCAL client → today's behaviour. This is the - // documented boundary of the remote-zone build: the CRD lifecycle and the - // spec-write plumbing reach the agent; the rest is deferred behind an explicit - // error rather than silently misrouting to the local cluster. + // (VPC/Subnet/NAD create/get/delete), the VPC/Subnet spec-write plumbing + // (peering, static routes, ACLs), AND the F15 NAT lifecycle (external-network + // bootstrap + per-VPC gateway/EIP/SNAT + pod polls) DO route through the + // zone's agent — every such method runs its k8s I/O through c.access, never + // c.dynamic. Only the remaining plumbing (per-VPC DNS/CoreDNS, DNS zone/record + // ConfigMaps, namespace/quota provisioning) is local-only for a remote zone + // and returns a clear localOnlyErr instead of dereferencing the nil dynamic + // client; the best-effort local cleanups (stuck-finalizer recovery, the + // pre-delete ACL clear) are silently skipped, with the agent-side delete + // owning finalizer convergence. Empty for the LOCAL client → today's + // behaviour. This is the documented boundary of the remote-zone build: the + // CRD lifecycle, the spec-write plumbing, and NAT reach the agent; the rest + // is deferred behind an explicit error rather than silently misrouting to the + // local cluster. remoteRegion, remoteZone string } // localOnlyErr is returned by a REMOTE client's remaining local-only PLUMBING -// methods (NAT, per-VPC DNS/CoreDNS, DNS zone/record ConfigMaps, namespace/ -// quota provisioning). dc-api holds no kubeconfig for a remote zone and those +// methods (per-VPC DNS/CoreDNS, DNS zone/record ConfigMaps, namespace/quota +// provisioning). dc-api holds no kubeconfig for a remote zone and those // operations lean on c.dynamic for GVRs that the agent's mapper/RBAC do not -// onboard. The onboarded CRD lifecycle (VPC/Subnet/NAD create/get/delete) and -// the VPC/Subnet spec-write plumbing (peering, static routes, ACLs) do NOT use -// this — they route through c.access to the zone's agent. +// onboard. The onboarded CRD lifecycle (VPC/Subnet/NAD create/get/delete), the +// VPC/Subnet spec-write plumbing (peering, static routes, ACLs), and the F15 +// NAT lifecycle do NOT use this — they route through c.access to the zone's +// agent. func (c *Client) localOnlyErr(op string) error { return fmt.Errorf( - "%s is not supported for remote zone %s/%s yet: dc-api holds no direct KubeOVN credentials there and this plumbing (NAT/per-VPC DNS) is local-only for now (the VPC/Subnet/NAD CRD lifecycle and the peering/route/ACL spec writes do route through the zone's agent)", + "%s is not supported for remote zone %s/%s yet: dc-api holds no direct KubeOVN credentials there and this plumbing (per-VPC DNS) is local-only for now (the VPC/Subnet/NAD CRD lifecycle, the peering/route/ACL spec writes, and the NAT lifecycle do route through the zone's agent)", op, c.remoteRegion, c.remoteZone) } // NewRemoteClient builds a credential-free KubeOVN client for a REMOTE zone. It // has NO kubeconfig and NO dynamic client; the onboarded CRD lifecycle // (CreateVNet/GetVNet/DeleteVNet, CreateSubnet/GetSubnet/DeleteSubnet and the -// NAD create/get/delete inside them) AND the VPC/Subnet spec-write plumbing -// (peering, route tables, NSG ACLs) run through the supplied agent-only -// Accessor (a clusteraccess.Routed whose Direct fallback is a clusteraccess. -// NoCreds, so a missing agent fails clearly rather than hitting the local -// cluster). The remaining plumbing methods (NAT, per-VPC DNS) return -// localOnlyErr. region/zone are carried only for clear error messages. Mirrors -// harvester.NewRemoteClient. +// NAD create/get/delete inside them), the VPC/Subnet spec-write plumbing +// (peering, route tables, NSG ACLs), AND the F15 NAT lifecycle (external- +// network bootstrap, EnsureVpcNAT/DeleteVpcNAT/IsVpcNATPresent/ +// WaitVpcNATPodsGone) run through the supplied agent-only Accessor (a +// clusteraccess.Routed whose Direct fallback is a clusteraccess.NoCreds, so a +// missing agent fails clearly rather than hitting the local cluster). The +// remaining plumbing methods (per-VPC DNS) return localOnlyErr. region/zone are +// carried only for clear error messages. Mirrors harvester.NewRemoteClient. func NewRemoteClient(access clusteraccess.Accessor, region, zone string) *Client { return &Client{ // dynamic + restConfig deliberately nil — guarded on plumbing methods. diff --git a/dc-api/internal/providers/kubeovn/nat.go b/dc-api/internal/providers/kubeovn/nat.go index 70189a41..bd995362 100644 --- a/dc-api/internal/providers/kubeovn/nat.go +++ b/dc-api/internal/providers/kubeovn/nat.go @@ -17,6 +17,12 @@ // 1. External subnet name is hardcoded "ovn-vpc-external-network". // 2. Subnet provider must be 3-dot-segment: ..ovn. // 3. VPC 0.0.0.0/0 route is YOUR responsibility; merge, don't overwrite. +// +// Every Kubernetes op in this file runs through the cluster-access seam +// (c.access) on onboarded capability families — NAT is agent-routable, so the +// whole lifecycle works in a remote (agent-only) zone. All created objects use +// FIXED, deterministic metadata.name values (never generateName), which is what +// makes the agent path's SSA create idempotent. package kubeovn import ( @@ -34,7 +40,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" ) // ── GVRs for F15 NAT resources ──────────────────────────────────────────────── @@ -173,7 +178,10 @@ func (c *Client) EnsureVpcNAT( }, }, } - if _, err := c.dynamic.Resource(vpcNatGatewayGVR).Create(ctx, gw, metav1.CreateOptions{}); err != nil { + // Cluster-scoped → ns="". Seam Create: local Direct keeps the plain POST with + // its IsAlreadyExists tolerance; the agent path is an SSA, which is idempotent + // on the fixed name so a retry converges instead of erroring. + if _, err := c.access.Create(ctx, vpcNatGatewayGVR, "", gw, metav1.CreateOptions{}); err != nil { if !k8serrors.IsAlreadyExists(err) { return nil, fmt.Errorf("ensure vpc nat: create VpcNatGateway %q: %w", gwName, err) } @@ -206,7 +214,7 @@ func (c *Client) EnsureVpcNAT( }, }, } - if _, err := c.dynamic.Resource(iptablesEIPGVR).Create(ctx, eipObj, metav1.CreateOptions{}); err != nil { + if _, err := c.access.Create(ctx, iptablesEIPGVR, "", eipObj, metav1.CreateOptions{}); err != nil { if !k8serrors.IsAlreadyExists(err) { return nil, fmt.Errorf("ensure vpc nat: create IptablesEIP %q: %w", eipName, err) } @@ -240,7 +248,7 @@ func (c *Client) EnsureVpcNAT( }, }, } - if _, err := c.dynamic.Resource(iptablesSnatRuleGVR).Create(ctx, snat, metav1.CreateOptions{}); err != nil { + if _, err := c.access.Create(ctx, iptablesSnatRuleGVR, "", snat, metav1.CreateOptions{}); err != nil { if !k8serrors.IsAlreadyExists(err) { return nil, fmt.Errorf("ensure vpc nat: create IptablesSnatRule %q: %w", snatName, err) } @@ -284,17 +292,20 @@ func (c *Client) DeleteVpcNAT(ctx context.Context, vpcName string) error { } // ── 2. Delete IptablesSnatRule ──────────────────────────────────────────── - if err := c.dynamic.Resource(iptablesSnatRuleGVR).Delete(ctx, snatName, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { + // Cluster-scoped → ns="". Seam Deletes: the agent path already treats a + // missing object as a successful idempotent delete; the IsNotFound tolerance + // below covers the local Direct path. + if err := c.access.Delete(ctx, iptablesSnatRuleGVR, "", snatName, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { return fmt.Errorf("delete vpc nat: delete IptablesSnatRule %q: %w", snatName, err) } // ── 3. Delete IptablesEIP ───────────────────────────────────────────────── - if err := c.dynamic.Resource(iptablesEIPGVR).Delete(ctx, eipName, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { + if err := c.access.Delete(ctx, iptablesEIPGVR, "", eipName, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { return fmt.Errorf("delete vpc nat: delete IptablesEIP %q: %w", eipName, err) } // ── 4. Delete VpcNatGateway ─────────────────────────────────────────────── - if err := c.dynamic.Resource(vpcNatGatewayGVR).Delete(ctx, gwName, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { + if err := c.access.Delete(ctx, vpcNatGatewayGVR, "", gwName, metav1.DeleteOptions{}); err != nil && !k8serrors.IsNotFound(err) { return fmt.Errorf("delete vpc nat: delete VpcNatGateway %q: %w", gwName, err) } @@ -320,7 +331,9 @@ func (c *Client) WaitVpcNATPodsGone(ctx context.Context, vpcUID string, timeout selector := fmt.Sprintf("app=vpc-nat-gw-%s", gwName) deadline := time.Now().Add(timeout) for time.Now().Before(deadline) { - list, err := c.dynamic.Resource(podGVR).Namespace("kube-system").List(ctx, metav1.ListOptions{ + // Pods are namespaced → pass the namespace. Seam List so the poll works in + // a remote (agent-only) zone; the pod family routes reads only. + list, err := c.access.List(ctx, podGVR, "kube-system", metav1.ListOptions{ LabelSelector: selector, }) if err == nil && len(list.Items) == 0 { @@ -340,7 +353,9 @@ func (c *Client) WaitVpcNATPodsGone(ctx context.Context, vpcUID string, timeout // loop to skip VPCs that are already fully provisioned. func (c *Client) IsVpcNATPresent(ctx context.Context, vpcName string) (bool, error) { gwName := natGWName(vpcName) - _, err := c.dynamic.Resource(vpcNatGatewayGVR).Get(ctx, gwName, metav1.GetOptions{}) + // Cluster-scoped → ns="". Seam Gets: the agent path shapes a missing object + // as a k8s NotFound, so the IsNotFound checks fire identically on both seams. + _, err := c.access.Get(ctx, vpcNatGatewayGVR, "", gwName, metav1.GetOptions{}) if k8serrors.IsNotFound(err) { return false, nil } @@ -349,7 +364,7 @@ func (c *Client) IsVpcNATPresent(ctx context.Context, vpcName string) (bool, err } eipName := natEIPName(vpcName) - _, err = c.dynamic.Resource(iptablesEIPGVR).Get(ctx, eipName, metav1.GetOptions{}) + _, err = c.access.Get(ctx, iptablesEIPGVR, "", eipName, metav1.GetOptions{}) if k8serrors.IsNotFound(err) { return false, nil } @@ -378,7 +393,10 @@ func (c *Client) ensureProviderNetwork(ctx context.Context, name, bridge string) }, }, } - _, err := c.dynamic.Resource(providerNetworkGVR).Create(ctx, obj, metav1.CreateOptions{}) + // Cluster-scoped → ns="". Seam Create: local Direct keeps the plain POST with + // its IsAlreadyExists tolerance; the agent path is an idempotent SSA on the + // fixed (bridge-derived) name. + _, err := c.access.Create(ctx, providerNetworkGVR, "", obj, metav1.CreateOptions{}) if err != nil && !k8serrors.IsAlreadyExists(err) { return fmt.Errorf("create ProviderNetwork %q: %w", name, err) } @@ -397,12 +415,17 @@ func (c *Client) ensureVlan(ctx context.Context, name, providerName string, vlan }, }, "spec": map[string]interface{}{ - "id": vlanID, + // int64, not int — unstructured content must be JSON-typed + // (runtime.DeepCopyJSON panics on plain int; the wire bytes are + // identical either way). + "id": int64(vlanID), "provider": providerName, }, }, } - _, err := c.dynamic.Resource(vlanGVR).Create(ctx, obj, metav1.CreateOptions{}) + // Cluster-scoped → ns="". Seam Create (same tolerance rationale as + // ensureProviderNetwork); the name is fixed ("ext-vlan-"). + _, err := c.access.Create(ctx, vlanGVR, "", obj, metav1.CreateOptions{}) if err != nil && !k8serrors.IsAlreadyExists(err) { return fmt.Errorf("create Vlan %q: %w", name, err) } @@ -451,10 +474,12 @@ func (c *Client) ensureExternalSubnet(ctx context.Context, providerName, cidr, g // Check existence by Get first — Harvester's network webhook returns a // non-AlreadyExists error ("subnet is using the provider already") when the // Subnet exists, which our IsAlreadyExists check below cannot catch. - if _, getErr := c.dynamic.Resource(subnetGVR).Get(ctx, subnetName, metav1.GetOptions{}); getErr == nil { + // Cluster-scoped → ns="". subnetGVR is an onboarded family, so both the Get + // and the Create route through the seam for a remote zone. + if _, getErr := c.access.Get(ctx, subnetGVR, "", subnetName, metav1.GetOptions{}); getErr == nil { return nil // already exists, idempotent } - _, err := c.dynamic.Resource(subnetGVR).Create(ctx, obj, metav1.CreateOptions{}) + _, err := c.access.Create(ctx, subnetGVR, "", obj, metav1.CreateOptions{}) if err != nil && !k8serrors.IsAlreadyExists(err) { return fmt.Errorf("create external Subnet %q: %w", subnetName, err) } @@ -503,7 +528,9 @@ func (c *Client) ensureExternalNAD(ctx context.Context, provider string) error { }, }, } - _, err := c.dynamic.Resource(nadGVR).Namespace(ns).Create(ctx, obj, metav1.CreateOptions{}) + // NAD is namespaced → pass ns. nadGVR is an onboarded family, so this create + // routes through the seam for a remote zone (fixed, hardcoded name). + _, err := c.access.Create(ctx, nadGVR, ns, obj, metav1.CreateOptions{}) if err != nil && !k8serrors.IsAlreadyExists(err) { return fmt.Errorf("create external NAD %s/%s: %w", ns, nadName, err) } @@ -517,7 +544,10 @@ func (c *Client) ensureExternalNAD(ctx context.Context, provider string) error { // preserved). Idempotent: if a default route with the same nextHopIP already // exists it is not duplicated. func (c *Client) ensureVPCDefaultRoute(ctx context.Context, vpcName, lanIP string) error { - vpc, err := c.dynamic.Resource(vpcGVR).Get(ctx, vpcName, metav1.GetOptions{}) + // Seam Get on the onboarded vpcGVR (cluster-scoped → ns="") feeding the + // read-modify-write; the write below (patchVPCStaticRoutes) is already an SSA + // through the seam, so the whole op is agent-routable. + vpc, err := c.access.Get(ctx, vpcGVR, "", vpcName, metav1.GetOptions{}) if err != nil { return fmt.Errorf("get vpc %q: %w", vpcName, err) } @@ -573,7 +603,9 @@ func (c *Client) ensureVPCDefaultRoute(ctx context.Context, vpcName, lanIP strin // fully managed by dc-api in F15 v1, so we treat any 0.0.0.0/0 as ours. // Idempotent — safe to call when no such route exists. func (c *Client) removeVPCDefaultRoute(ctx context.Context, vpcName string) error { - vpc, err := c.dynamic.Resource(vpcGVR).Get(ctx, vpcName, metav1.GetOptions{}) + // Seam Get (see ensureVPCDefaultRoute); the agent path shapes a missing VPC + // as a k8s NotFound so the already-gone tolerance fires on both seams. + vpc, err := c.access.Get(ctx, vpcGVR, "", vpcName, metav1.GetOptions{}) if k8serrors.IsNotFound(err) { return nil // VPC already gone } @@ -606,7 +638,9 @@ func (c *Client) waitForPodRunning(ctx context.Context, ns, podName string, time waitCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() for { - obj, err := c.dynamic.Resource(podGVR).Namespace(ns).Get(ctx, podName, metav1.GetOptions{}) + // Seam Get (pods are namespaced → pass ns): the pod family routes reads + // only, so this status poll works in a remote (agent-only) zone. + obj, err := c.access.Get(ctx, podGVR, ns, podName, metav1.GetOptions{}) if err == nil { phase, _, _ := unstructured.NestedString(obj.Object, "status", "phase") if phase == "Running" { @@ -626,7 +660,8 @@ func (c *Client) waitForEIPReady(ctx context.Context, eipName string, timeout ti waitCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() for { - obj, err := c.dynamic.Resource(iptablesEIPGVR).Get(ctx, eipName, metav1.GetOptions{}) + // Seam Get on the onboarded iptables-eips family (cluster-scoped → ns=""). + obj, err := c.access.Get(ctx, iptablesEIPGVR, "", eipName, metav1.GetOptions{}) if err == nil { if ready, _, _ := unstructured.NestedBool(obj.Object, "status", "ready"); ready { return nil @@ -645,7 +680,7 @@ func (c *Client) waitForEIPReady(ctx context.Context, eipName string, timeout ti // difference between spec and status is a KubeOVN quirk verified live on // v1.15.4). func (c *Client) readEIPAssignedIP(ctx context.Context, eipName string) (net.IP, error) { - obj, err := c.dynamic.Resource(iptablesEIPGVR).Get(ctx, eipName, metav1.GetOptions{}) + obj, err := c.access.Get(ctx, iptablesEIPGVR, "", eipName, metav1.GetOptions{}) if err != nil { return nil, fmt.Errorf("get IptablesEIP %q: %w", eipName, err) } @@ -665,7 +700,8 @@ func (c *Client) waitForSnatReady(ctx context.Context, snatName string, timeout waitCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() for { - obj, err := c.dynamic.Resource(iptablesSnatRuleGVR).Get(ctx, snatName, metav1.GetOptions{}) + // Seam Get on the onboarded iptables-snat-rules family (cluster-scoped → ns=""). + obj, err := c.access.Get(ctx, iptablesSnatRuleGVR, "", snatName, metav1.GetOptions{}) if err == nil { if ready, _, _ := unstructured.NestedBool(obj.Object, "status", "ready"); ready { return nil @@ -840,15 +876,3 @@ func ipLessOrEqual(a, b net.IP) bool { func marshalPatch(v interface{}) ([]byte, error) { return json.Marshal(v) } - -// patchVPCSpec is a helper that applies a JSON MergePatch to a VPC spec field. -// Not exported — callers use the specific helpers above. -func (c *Client) patchVPCSpec(ctx context.Context, vpcName string, specPatch map[string]interface{}) error { - patch := map[string]interface{}{"spec": specPatch} - patchBytes, err := json.Marshal(patch) - if err != nil { - return fmt.Errorf("marshal vpc spec patch: %w", err) - } - _, err = c.dynamic.Resource(vpcGVR).Patch(ctx, vpcName, types.MergePatchType, patchBytes, metav1.PatchOptions{}) - return err -} diff --git a/dc-api/internal/providers/kubeovn/nat_seam_test.go b/dc-api/internal/providers/kubeovn/nat_seam_test.go new file mode 100644 index 00000000..85813c92 --- /dev/null +++ b/dc-api/internal/providers/kubeovn/nat_seam_test.go @@ -0,0 +1,703 @@ +package kubeovn + +// nat_seam_test.go proves the F15 NAT slice of the credential-locality work: +// the whole per-VPC NAT lifecycle in nat.go — the external-network bootstrap +// (ProviderNetwork/Vlan/external Subnet/NAD), EnsureVpcNAT's gateway/EIP/SNAT +// chain, DeleteVpcNAT's teardown, IsVpcNATPresent, and the pod/EIP/SNAT status +// waiters — runs through the cluster-access seam (c.access.Create/Get/Delete/ +// List) instead of c.dynamic, so it works for a REMOTE (agent-only) zone. +// +// Coverage: +// - EnsureVpcNAT creates the gateway, EIP, and SNAT rule via seam Creates with +// the correct GVRs and FIXED, deterministic names (SSA on the agent path +// requires fixed names — no generateName anywhere), keeps the IsAlreadyExists +// tolerance, and writes the VPC default route through the seam Apply the +// parent slice routed (minimal single-field object, dedicated field manager). +// - DeleteVpcNAT issues the three seam Deletes in reverse order, strips ONLY +// the 0.0.0.0/0 route (preserving peering routes), and tolerates NotFound. +// - IsVpcNATPresent reads both presence Gets through the seam. +// - The pod-running/EIP-ready/SNAT-ready waiters and the pods-gone poll read +// status via seam Get/List (pods namespaced, NAT CRDs cluster-scoped). +// - Remote client (NewRemoteClient, c.dynamic == nil): the full lifecycle works +// purely through the seam — the regression guard against re-introducing a +// c.dynamic dereference (which would panic on the nil client) or a +// localOnlyErr guard (which would re-open the remote gap). + +import ( + "context" + "net" + "strings" + "sync" + "testing" + "time" + + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/wso2/dc-api/internal/providers/clusteraccess" +) + +// ── A recording accessor for the NAT lifecycle ──────────────────────────────── + +type recordedNATGet struct { + gvr schema.GroupVersionResource + ns string + name string +} + +type recordedNATCreate struct { + gvr schema.GroupVersionResource + ns string + obj *unstructured.Unstructured +} + +type recordedNATDelete struct { + gvr schema.GroupVersionResource + ns string + name string +} + +type recordedNATList struct { + gvr schema.GroupVersionResource + ns string + selector string +} + +// natRecordingAccessor serves programmable objects from Get (keyed by name — all +// NAT fixture names are distinct), records every Get/Create/Delete/List/Apply +// with its full argument set, and can inject per-resource create/delete errors +// (the AlreadyExists / NotFound tolerance probes). Objects are pre-seeded in +// their CONVERGED state (pod Running, EIP/SNAT ready) so the waiters return on +// their first seam read. +type natRecordingAccessor struct { + mu sync.Mutex + + objects map[string]*unstructured.Unstructured + + // createErrs/deleteErrs inject an error per GVR.Resource. + createErrs map[string]error + deleteErrs map[string]error + + // podList is what List returns (nil → empty list). + podList *unstructured.UnstructuredList + + gets []recordedNATGet + creates []recordedNATCreate + deletes []recordedNATDelete + lists []recordedNATList + applies []recordedApply // shared shape with network_plumbing_seam_test.go +} + +var _ clusteraccess.Accessor = (*natRecordingAccessor)(nil) + +func (a *natRecordingAccessor) Get(_ context.Context, gvr schema.GroupVersionResource, ns, name string, _ metav1.GetOptions) (*unstructured.Unstructured, error) { + a.mu.Lock() + defer a.mu.Unlock() + a.gets = append(a.gets, recordedNATGet{gvr: gvr, ns: ns, name: name}) + obj, ok := a.objects[name] + if !ok { + return nil, k8serrors.NewNotFound(gvr.GroupResource(), name) + } + return obj.DeepCopy(), nil +} + +func (a *natRecordingAccessor) List(_ context.Context, gvr schema.GroupVersionResource, ns string, opts metav1.ListOptions) (*unstructured.UnstructuredList, error) { + a.mu.Lock() + defer a.mu.Unlock() + a.lists = append(a.lists, recordedNATList{gvr: gvr, ns: ns, selector: opts.LabelSelector}) + if a.podList != nil { + return a.podList, nil + } + return &unstructured.UnstructuredList{}, nil +} + +func (a *natRecordingAccessor) Create(_ context.Context, gvr schema.GroupVersionResource, ns string, obj *unstructured.Unstructured, _ metav1.CreateOptions) (*unstructured.Unstructured, error) { + a.mu.Lock() + defer a.mu.Unlock() + a.creates = append(a.creates, recordedNATCreate{gvr: gvr, ns: ns, obj: obj.DeepCopy()}) + if err := a.createErrs[gvr.Resource]; err != nil { + return nil, err + } + return obj, nil +} + +func (a *natRecordingAccessor) Apply(_ context.Context, gvr schema.GroupVersionResource, ns string, obj *unstructured.Unstructured, fieldManager string, force bool) (*unstructured.Unstructured, error) { + a.mu.Lock() + defer a.mu.Unlock() + a.applies = append(a.applies, recordedApply{gvr: gvr, ns: ns, obj: obj, fieldManager: fieldManager, force: force}) + return obj, nil +} + +func (a *natRecordingAccessor) Update(_ context.Context, _ schema.GroupVersionResource, _ string, obj *unstructured.Unstructured, _ metav1.UpdateOptions) (*unstructured.Unstructured, error) { + return obj, nil +} + +func (a *natRecordingAccessor) Delete(_ context.Context, gvr schema.GroupVersionResource, ns, name string, _ metav1.DeleteOptions) error { + a.mu.Lock() + defer a.mu.Unlock() + a.deletes = append(a.deletes, recordedNATDelete{gvr: gvr, ns: ns, name: name}) + if err := a.deleteErrs[gvr.Resource]; err != nil { + return err + } + return nil +} + +func (a *natRecordingAccessor) sawGetGVR(gvr schema.GroupVersionResource) bool { + a.mu.Lock() + defer a.mu.Unlock() + for _, g := range a.gets { + if g.gvr == gvr { + return true + } + } + return false +} + +// ── Converged NAT fixture objects ───────────────────────────────────────────── + +func runningPod(name string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", "kind": "Pod", + "metadata": map[string]interface{}{"name": name, "namespace": "kube-system"}, + "status": map[string]interface{}{"phase": "Running"}, + }} +} + +func readyEIP(name, ip string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "kubeovn.io/v1", "kind": "IptablesEIP", + "metadata": map[string]interface{}{"name": name}, + "status": map[string]interface{}{"ready": true, "ip": ip}, + }} +} + +func readySnat(name string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "kubeovn.io/v1", "kind": "IptablesSnatRule", + "metadata": map[string]interface{}{"name": name}, + "status": map[string]interface{}{"ready": true}, + }} +} + +// convergedNATObjects seeds every object EnsureVpcNAT's waiters poll, in its +// converged state, for the VPC "vnet-a". +func convergedNATObjects(eipIP string) map[string]*unstructured.Unstructured { + return map[string]*unstructured.Unstructured{ + "vpc-nat-gw-natgw-vnet-a-0": runningPod("vpc-nat-gw-natgw-vnet-a-0"), + "eip-vnet-a": readyEIP("eip-vnet-a", eipIP), + "snat-vnet-a": readySnat("snat-vnet-a"), + "vnet-a": seededVpc("vnet-a", []interface{}{}, []interface{}{}), + } +} + +// assertFixedName asserts a created object carries the expected deterministic +// metadata.name and NO generateName — the invariant that keeps the agent path's +// SSA create idempotent (SSA cannot apply a generateName object). +func assertFixedName(t *testing.T, obj *unstructured.Unstructured, wantName string) { + t.Helper() + if got := obj.GetName(); got != wantName { + t.Errorf("created object name = %q, want the fixed name %q", got, wantName) + } + if gn := obj.GetGenerateName(); gn != "" { + t.Errorf("created object %q uses generateName %q — SSA requires fixed names", wantName, gn) + } +} + +// ── EnsureVpcNAT: seam creates with fixed names + seam route write ──────────── + +func TestEnsureVpcNAT_SeamLifecycle_CreatesWithFixedNames(t *testing.T) { + acc := &natRecordingAccessor{objects: convergedNATObjects("192.168.10.100")} + c := &Client{access: acc} + + ip, err := c.EnsureVpcNAT(context.Background(), "vnet-a", "10.0.1.0/24", "subnet-1", net.ParseIP("10.0.1.254")) + if err != nil { + t.Fatalf("EnsureVpcNAT: %v", err) + } + if ip.String() != "192.168.10.100" { + t.Errorf("assigned EIP = %s, want 192.168.10.100 (from .status.ip)", ip) + } + + // Three seam creates, in order, all cluster-scoped (ns=""). + if len(acc.creates) != 3 { + t.Fatalf("seam Create called %d times, want 3 (gateway, EIP, SNAT)", len(acc.creates)) + } + wantCreates := []struct { + gvr schema.GroupVersionResource + name string + }{ + {vpcNatGatewayGVR, natGWName("vnet-a")}, + {iptablesEIPGVR, natEIPName("vnet-a")}, + {iptablesSnatRuleGVR, natSnatName("vnet-a")}, + } + for i, want := range wantCreates { + rec := acc.creates[i] + if rec.gvr != want.gvr { + t.Errorf("create[%d] GVR = %v, want %v", i, rec.gvr, want.gvr) + } + if rec.ns != "" { + t.Errorf("create[%d] namespace = %q, want \"\" (cluster-scoped)", i, rec.ns) + } + assertFixedName(t, rec.obj, want.name) + } + + // Gateway spec pins vpc/subnet/lanIp. + gwSpec, _, _ := unstructured.NestedMap(acc.creates[0].obj.Object, "spec") + if gwSpec["vpc"] != "vnet-a" || gwSpec["subnet"] != "subnet-1" || gwSpec["lanIp"] != "10.0.1.254" { + t.Errorf("gateway spec = %v, want vpc=vnet-a subnet=subnet-1 lanIp=10.0.1.254", gwSpec) + } + + // EIP spec: natGwDp set, v4ip OMITTED (KubeOVN's IPAM picks the address). + eipSpec, _, _ := unstructured.NestedMap(acc.creates[1].obj.Object, "spec") + if eipSpec["natGwDp"] != natGWName("vnet-a") { + t.Errorf("EIP spec.natGwDp = %v, want %q", eipSpec["natGwDp"], natGWName("vnet-a")) + } + if _, hasV4IP := eipSpec["v4ip"]; hasV4IP { + t.Error("EIP spec carries v4ip — it must be omitted so KubeOVN's IPAM allocates") + } + + // SNAT spec pins eip/internalCIDR. + snatSpec, _, _ := unstructured.NestedMap(acc.creates[2].obj.Object, "spec") + if snatSpec["eip"] != natEIPName("vnet-a") || snatSpec["internalCIDR"] != "10.0.1.0/24" { + t.Errorf("SNAT spec = %v, want eip=%q internalCIDR=10.0.1.0/24", snatSpec, natEIPName("vnet-a")) + } + + // The default-route write went through the seam Apply the parent slice + // routed: minimal single-field staticRoutes object, dedicated field manager. + if len(acc.applies) != 1 { + t.Fatalf("seam Apply called %d times, want 1 (VPC default route)", len(acc.applies)) + } + list := assertMinimalApply(t, acc.applies[0], vpcGVR, "Vpc", "vnet-a", "staticRoutes", fieldManagerStaticRoutes) + want := []interface{}{map[string]interface{}{ + "cidr": "0.0.0.0/0", "nextHopIP": "10.0.1.254", "policy": "policyDst", "routeTable": "", + }} + assertJSONEqual(t, list, want, "spec.staticRoutes (default route)") + + // The waiters and the route read all went through the seam Get. + for _, gvr := range []schema.GroupVersionResource{podGVR, iptablesEIPGVR, iptablesSnatRuleGVR, vpcGVR} { + if !acc.sawGetGVR(gvr) { + t.Errorf("no seam Get recorded for %v — a read bypassed the seam", gvr) + } + } + // The gateway-pod poll is the one namespaced read (kube-system). + for _, g := range acc.gets { + if g.gvr == podGVR { + if g.ns != "kube-system" || g.name != "vpc-nat-gw-natgw-vnet-a-0" { + t.Errorf("pod Get = ns %q name %q, want kube-system/vpc-nat-gw-natgw-vnet-a-0", g.ns, g.name) + } + } else if g.ns != "" { + t.Errorf("%v Get carries namespace %q, want \"\" (cluster-scoped)", g.gvr, g.ns) + } + } +} + +func TestEnsureVpcNAT_AlreadyExistsTolerated(t *testing.T) { + acc := &natRecordingAccessor{ + objects: convergedNATObjects("192.168.10.101"), + createErrs: map[string]error{ + vpcNatGatewayGVR.Resource: k8serrors.NewAlreadyExists(vpcNatGatewayGVR.GroupResource(), natGWName("vnet-a")), + iptablesEIPGVR.Resource: k8serrors.NewAlreadyExists(iptablesEIPGVR.GroupResource(), natEIPName("vnet-a")), + iptablesSnatRuleGVR.Resource: k8serrors.NewAlreadyExists(iptablesSnatRuleGVR.GroupResource(), natSnatName("vnet-a")), + }, + } + c := &Client{access: acc} + + // A re-run against existing resources must converge, not error: the local + // POST path tolerates AlreadyExists (and the agent path's SSA never raises it). + ip, err := c.EnsureVpcNAT(context.Background(), "vnet-a", "10.0.1.0/24", "subnet-1", net.ParseIP("10.0.1.254")) + if err != nil { + t.Fatalf("EnsureVpcNAT on pre-existing resources must succeed, got %v", err) + } + if ip.String() != "192.168.10.101" { + t.Errorf("assigned EIP = %s, want 192.168.10.101", ip) + } + if len(acc.applies) != 1 { + t.Errorf("seam Apply called %d times, want 1 (default route still ensured)", len(acc.applies)) + } +} + +// ── DeleteVpcNAT: seam deletes + route strip + NotFound tolerance ───────────── + +func TestDeleteVpcNAT_SeamDeletesAndRouteStrip(t *testing.T) { + defaultRoute := map[string]interface{}{"cidr": "0.0.0.0/0", "nextHopIP": "10.0.1.254", "policy": "policyDst", "routeTable": ""} + peeringRoute := map[string]interface{}{"cidr": "10.2.0.0/16", "nextHopIP": "100.64.10.2", "policy": "policyDst", "routeTable": ""} + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{ + "vnet-a": seededVpc("vnet-a", []interface{}{}, []interface{}{defaultRoute, peeringRoute}), + }} + c := &Client{access: acc} + + if err := c.DeleteVpcNAT(context.Background(), "vnet-a"); err != nil { + t.Fatalf("DeleteVpcNAT: %v", err) + } + + // The route strip removed ONLY the 0.0.0.0/0 entry (peering preserved), + // through the same minimal seam Apply as the ensure path. + if len(acc.applies) != 1 { + t.Fatalf("seam Apply called %d times, want 1 (route strip)", len(acc.applies)) + } + list := assertMinimalApply(t, acc.applies[0], vpcGVR, "Vpc", "vnet-a", "staticRoutes", fieldManagerStaticRoutes) + assertJSONEqual(t, list, []interface{}{peeringRoute}, "spec.staticRoutes after NAT delete") + + // Three seam deletes in reverse creation order, all cluster-scoped. + if len(acc.deletes) != 3 { + t.Fatalf("seam Delete called %d times, want 3 (SNAT, EIP, gateway)", len(acc.deletes)) + } + wantDeletes := []struct { + gvr schema.GroupVersionResource + name string + }{ + {iptablesSnatRuleGVR, natSnatName("vnet-a")}, + {iptablesEIPGVR, natEIPName("vnet-a")}, + {vpcNatGatewayGVR, natGWName("vnet-a")}, + } + for i, want := range wantDeletes { + rec := acc.deletes[i] + if rec.gvr != want.gvr || rec.name != want.name { + t.Errorf("delete[%d] = %v %q, want %v %q", i, rec.gvr, rec.name, want.gvr, want.name) + } + if rec.ns != "" { + t.Errorf("delete[%d] namespace = %q, want \"\" (cluster-scoped)", i, rec.ns) + } + } +} + +func TestDeleteVpcNAT_NotFoundTolerated(t *testing.T) { + acc := &natRecordingAccessor{ + // No VPC object: the route-strip Get is NotFound → silent no-op. + objects: map[string]*unstructured.Unstructured{}, + deleteErrs: map[string]error{ + iptablesSnatRuleGVR.Resource: k8serrors.NewNotFound(iptablesSnatRuleGVR.GroupResource(), natSnatName("vnet-a")), + iptablesEIPGVR.Resource: k8serrors.NewNotFound(iptablesEIPGVR.GroupResource(), natEIPName("vnet-a")), + vpcNatGatewayGVR.Resource: k8serrors.NewNotFound(vpcNatGatewayGVR.GroupResource(), natGWName("vnet-a")), + }, + } + c := &Client{access: acc} + + if err := c.DeleteVpcNAT(context.Background(), "vnet-a"); err != nil { + t.Fatalf("DeleteVpcNAT on already-deleted resources must be nil, got %v", err) + } + if len(acc.applies) != 0 { + t.Errorf("seam Apply called %d times for a missing VPC, want 0", len(acc.applies)) + } + if len(acc.deletes) != 3 { + t.Errorf("seam Delete called %d times, want 3 (all attempted despite NotFound)", len(acc.deletes)) + } +} + +// ── IsVpcNATPresent: presence reads through the seam ────────────────────────── + +func TestIsVpcNATPresent_SeamGets(t *testing.T) { + t.Run("both present", func(t *testing.T) { + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{ + "natgw-vnet-a": {Object: map[string]interface{}{"apiVersion": "kubeovn.io/v1", "kind": "VpcNatGateway", "metadata": map[string]interface{}{"name": "natgw-vnet-a"}}}, + "eip-vnet-a": readyEIP("eip-vnet-a", "192.168.10.100"), + }} + c := &Client{access: acc} + present, err := c.IsVpcNATPresent(context.Background(), "vnet-a") + if err != nil { + t.Fatalf("IsVpcNATPresent: %v", err) + } + if !present { + t.Error("present = false, want true (gateway + EIP both exist)") + } + if !acc.sawGetGVR(vpcNatGatewayGVR) || !acc.sawGetGVR(iptablesEIPGVR) { + t.Errorf("presence Gets did not go through the seam: %v", acc.gets) + } + }) + + t.Run("gateway missing", func(t *testing.T) { + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{ + "eip-vnet-a": readyEIP("eip-vnet-a", "192.168.10.100"), + }} + c := &Client{access: acc} + present, err := c.IsVpcNATPresent(context.Background(), "vnet-a") + if err != nil || present { + t.Errorf("IsVpcNATPresent = (%v, %v), want (false, nil) when the gateway is missing", present, err) + } + }) + + t.Run("eip missing", func(t *testing.T) { + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{ + "natgw-vnet-a": {Object: map[string]interface{}{"apiVersion": "kubeovn.io/v1", "kind": "VpcNatGateway", "metadata": map[string]interface{}{"name": "natgw-vnet-a"}}}, + }} + c := &Client{access: acc} + present, err := c.IsVpcNATPresent(context.Background(), "vnet-a") + if err != nil || present { + t.Errorf("IsVpcNATPresent = (%v, %v), want (false, nil) when the EIP is missing", present, err) + } + }) +} + +// ── The waiters: status reads via seam Get/List ─────────────────────────────── + +func TestWaitVpcNATPodsGone_SeamList(t *testing.T) { + t.Run("no pods returns immediately", func(t *testing.T) { + acc := &natRecordingAccessor{} + c := &Client{access: acc} + + if err := c.WaitVpcNATPodsGone(context.Background(), "vnet-a", 5*time.Second); err != nil { + t.Fatalf("WaitVpcNATPodsGone with no pods: %v", err) + } + if len(acc.lists) != 1 { + t.Fatalf("seam List called %d times, want 1", len(acc.lists)) + } + rec := acc.lists[0] + if rec.gvr != podGVR { + t.Errorf("List GVR = %v, want pods", rec.gvr) + } + if rec.ns != "kube-system" { + t.Errorf("List namespace = %q, want kube-system", rec.ns) + } + wantSelector := "app=vpc-nat-gw-" + natGWName("vnet-a") + if rec.selector != wantSelector { + t.Errorf("List label selector = %q, want %q", rec.selector, wantSelector) + } + }) + + t.Run("lingering pod blocks until ctx deadline", func(t *testing.T) { + lingering := &unstructured.UnstructuredList{Items: []unstructured.Unstructured{*runningPod("vpc-nat-gw-natgw-vnet-a-0")}} + acc := &natRecordingAccessor{podList: lingering} + c := &Client{access: acc} + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + if err := c.WaitVpcNATPodsGone(ctx, "vnet-a", 5*time.Second); err == nil { + t.Fatal("WaitVpcNATPodsGone must not return nil while pods linger") + } + if len(acc.lists) == 0 { + t.Error("seam List was never called") + } + }) +} + +func TestNATWaiters_ReadStatusViaSeam(t *testing.T) { + acc := &natRecordingAccessor{objects: convergedNATObjects("192.168.10.100")} + c := &Client{access: acc} + ctx := context.Background() + + if err := c.waitForPodRunning(ctx, "kube-system", "vpc-nat-gw-natgw-vnet-a-0", time.Second); err != nil { + t.Errorf("waitForPodRunning on a Running pod: %v", err) + } + if err := c.waitForEIPReady(ctx, "eip-vnet-a", time.Second); err != nil { + t.Errorf("waitForEIPReady on a ready EIP: %v", err) + } + if err := c.waitForSnatReady(ctx, "snat-vnet-a", time.Second); err != nil { + t.Errorf("waitForSnatReady on a ready SNAT rule: %v", err) + } + ip, err := c.readEIPAssignedIP(ctx, "eip-vnet-a") + if err != nil { + t.Errorf("readEIPAssignedIP: %v", err) + } else if ip.String() != "192.168.10.100" { + t.Errorf("readEIPAssignedIP = %s, want 192.168.10.100", ip) + } + + // Every read above went through the seam. + for _, gvr := range []schema.GroupVersionResource{podGVR, iptablesEIPGVR, iptablesSnatRuleGVR} { + if !acc.sawGetGVR(gvr) { + t.Errorf("no seam Get recorded for %v", gvr) + } + } +} + +// TestNATWaiters_TimeoutErrors pins the timeout branch of each private waiter: +// an object that never converges makes the waiter return a clear timeout error +// (not nil, not a hang). The pod is Pending, the EIP not-ready, and the SNAT +// rule not-ready; a ~20ms budget forces waitCtx.Done() on the first select. +func TestNATWaiters_TimeoutErrors(t *testing.T) { + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{ + "vpc-nat-gw-natgw-vnet-a-0": {Object: map[string]interface{}{ + "apiVersion": "v1", "kind": "Pod", + "metadata": map[string]interface{}{"name": "vpc-nat-gw-natgw-vnet-a-0", "namespace": "kube-system"}, + "status": map[string]interface{}{"phase": "Pending"}, + }}, + "eip-vnet-a": {Object: map[string]interface{}{ + "apiVersion": "kubeovn.io/v1", "kind": "IptablesEIP", + "metadata": map[string]interface{}{"name": "eip-vnet-a"}, + "status": map[string]interface{}{"ready": false}, + }}, + "snat-vnet-a": {Object: map[string]interface{}{ + "apiVersion": "kubeovn.io/v1", "kind": "IptablesSnatRule", + "metadata": map[string]interface{}{"name": "snat-vnet-a"}, + "status": map[string]interface{}{"ready": false}, + }}, + }} + c := &Client{access: acc} + ctx := context.Background() + + if err := c.waitForPodRunning(ctx, "kube-system", "vpc-nat-gw-natgw-vnet-a-0", 20*time.Millisecond); err == nil { + t.Error("waitForPodRunning on a Pending pod must time out, got nil") + } else if !strings.Contains(err.Error(), "did not reach Running") { + t.Errorf("waitForPodRunning timeout error = %q, want the did-not-reach-Running message", err) + } + if err := c.waitForEIPReady(ctx, "eip-vnet-a", 20*time.Millisecond); err == nil { + t.Error("waitForEIPReady on a not-ready EIP must time out, got nil") + } else if !strings.Contains(err.Error(), "did not become ready") { + t.Errorf("waitForEIPReady timeout error = %q, want the did-not-become-ready message", err) + } + if err := c.waitForSnatReady(ctx, "snat-vnet-a", 20*time.Millisecond); err == nil { + t.Error("waitForSnatReady on a not-ready SNAT rule must time out, got nil") + } else if !strings.Contains(err.Error(), "did not become ready") { + t.Errorf("waitForSnatReady timeout error = %q, want the did-not-become-ready message", err) + } +} + +func TestReadEIPAssignedIP_NoIPYetErrors(t *testing.T) { + // EIP exists but the controller hasn't allocated yet (no .status.ip). + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{ + "eip-vnet-a": {Object: map[string]interface{}{ + "apiVersion": "kubeovn.io/v1", "kind": "IptablesEIP", + "metadata": map[string]interface{}{"name": "eip-vnet-a"}, + "status": map[string]interface{}{"ready": false}, + }}, + }} + c := &Client{access: acc} + + _, err := c.readEIPAssignedIP(context.Background(), "eip-vnet-a") + if err == nil || !strings.Contains(err.Error(), "no .status.ip") { + t.Errorf("readEIPAssignedIP without an allocation = %v, want a clear no-.status.ip error", err) + } +} + +// ── External-network bootstrap: seam creates with fixed names ───────────────── + +func TestEnsureExternalNetworkBootstrap_SeamCreates(t *testing.T) { + // Run the bootstrap on a REMOTE client (nil dynamic): every one of its four + // object creations (ProviderNetwork, Vlan, NAD, external Subnet) plus the + // Subnet existence pre-check must route through the seam. + acc := &natRecordingAccessor{objects: map[string]*unstructured.Unstructured{}} + c := NewRemoteClient(acc, "lk", "zone-2") + c.WithExternalNetwork(ExternalNetworkConfig{ + Bridge: "mgmt-br", + CIDR: "192.168.10.0/24", + Gateway: "192.168.10.254", + ReservedIPs: []string{"192.168.10.15"}, + VLANID: 100, + }) + + if err := c.EnsureExternalNetworkBootstrap(context.Background()); err != nil { + t.Fatalf("EnsureExternalNetworkBootstrap (remote): %v", err) + } + + if len(acc.creates) != 4 { + t.Fatalf("seam Create called %d times, want 4 (ProviderNetwork, Vlan, NAD, Subnet)", len(acc.creates)) + } + wantCreates := []struct { + gvr schema.GroupVersionResource + ns string + name string + }{ + {providerNetworkGVR, "", "mgmt-br"}, + {vlanGVR, "", "ext-vlan-100"}, + {nadGVR, "kube-system", "ovn-vpc-external-network"}, + {subnetGVR, "", "ovn-vpc-external-network"}, + } + for i, want := range wantCreates { + rec := acc.creates[i] + if rec.gvr != want.gvr { + t.Errorf("create[%d] GVR = %v, want %v", i, rec.gvr, want.gvr) + } + if rec.ns != want.ns { + t.Errorf("create[%d] namespace = %q, want %q", i, rec.ns, want.ns) + } + assertFixedName(t, rec.obj, want.name) + } + + // The Vlan's spec.id must be int64-typed (unstructured content is + // JSON-typed; a plain int would panic in DeepCopyJSON at runtime) and carry + // the configured VLAN id. The explicit .(int64) assertion pins the cast in + // ensureVlan — removing it would fail here, not at runtime. + vlanSpec, _, _ := unstructured.NestedMap(acc.creates[1].obj.Object, "spec") + if id, ok := vlanSpec["id"].(int64); !ok || id != 100 { + t.Errorf("Vlan spec.id = %v (%T), want int64(100)", vlanSpec["id"], vlanSpec["id"]) + } + + // The external Subnet excludes the gateway + reserved IPs and carries the + // 3-dot-segment provider (gotcha 2). + subnetSpec, _, _ := unstructured.NestedMap(acc.creates[3].obj.Object, "spec") + assertJSONEqual(t, subnetSpec["excludeIps"], []interface{}{"192.168.10.254", "192.168.10.15"}, "external subnet excludeIps") + if subnetSpec["provider"] != "ovn-vpc-external-network.kube-system.ovn" { + t.Errorf("external subnet provider = %v, want ovn-vpc-external-network.kube-system.ovn", subnetSpec["provider"]) + } + + // The existence pre-check Get routed through the seam too. + if !acc.sawGetGVR(subnetGVR) { + t.Error("external Subnet existence pre-check did not go through the seam") + } +} + +func TestEnsureExternalNetworkBootstrap_ConfigMissing(t *testing.T) { + acc := &natRecordingAccessor{} + c := NewRemoteClient(acc, "lk", "zone-2") + + // Without WithExternalNetwork the bootstrap must fail clearly BEFORE any + // seam I/O. + if err := c.EnsureExternalNetworkBootstrap(context.Background()); err == nil { + t.Fatal("bootstrap without external network config must error") + } + if len(acc.creates)+len(acc.gets) != 0 { + t.Errorf("bootstrap touched the seam %d times despite missing config, want 0", len(acc.creates)+len(acc.gets)) + } +} + +// ── Remote client: the FULL NAT lifecycle works with NO dynamic client ──────── + +// TestRemoteClient_NATLifecycle_RoutesThroughSeam is the regression guard that +// pins the gap this slice closed: a REMOTE kubeovn client (c.dynamic == nil) +// runs the whole per-VPC NAT lifecycle purely through the seam — re-introducing +// a c.dynamic dereference would panic here, and re-introducing a localOnlyErr +// guard would fail the calls. +func TestRemoteClient_NATLifecycle_RoutesThroughSeam(t *testing.T) { + acc := &natRecordingAccessor{objects: convergedNATObjects("192.168.10.100")} + c := NewRemoteClient(acc, "lk", "zone-2") + if c.dynamic != nil { + t.Fatal("remote client must have a nil dynamic client") + } + ctx := context.Background() + + // Ensure: full chain through the seam. + ip, err := c.EnsureVpcNAT(ctx, "vnet-a", "10.0.1.0/24", "subnet-1", net.ParseIP("10.0.1.254")) + if err != nil { + t.Fatalf("EnsureVpcNAT (remote): %v", err) + } + if ip.String() != "192.168.10.100" { + t.Errorf("assigned EIP = %s, want 192.168.10.100", ip) + } + + // Presence check. + acc.mu.Lock() + acc.objects["natgw-vnet-a"] = &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "kubeovn.io/v1", "kind": "VpcNatGateway", + "metadata": map[string]interface{}{"name": "natgw-vnet-a"}, + }} + acc.mu.Unlock() + present, err := c.IsVpcNATPresent(ctx, "vnet-a") + if err != nil { + t.Fatalf("IsVpcNATPresent (remote): %v", err) + } + if !present { + t.Error("IsVpcNATPresent = false, want true") + } + + // Teardown + drain. + if err := c.DeleteVpcNAT(ctx, "vnet-a"); err != nil { + t.Fatalf("DeleteVpcNAT (remote): %v", err) + } + if err := c.WaitVpcNATPodsGone(ctx, "vnet-a", 5*time.Second); err != nil { + t.Fatalf("WaitVpcNATPodsGone (remote): %v", err) + } + + if len(acc.creates) != 3 { + t.Errorf("seam Create called %d times, want 3", len(acc.creates)) + } + if len(acc.deletes) != 3 { + t.Errorf("seam Delete called %d times, want 3", len(acc.deletes)) + } + if len(acc.lists) != 1 { + t.Errorf("seam List called %d times, want 1", len(acc.lists)) + } + // Two seam applies: the ensure default route + the delete route strip. + if len(acc.applies) != 2 { + t.Errorf("seam Apply called %d times, want 2 (route ensure + route strip)", len(acc.applies)) + } +} diff --git a/dc-api/internal/providers/kubeovn/remote_client_test.go b/dc-api/internal/providers/kubeovn/remote_client_test.go index 8f8fab58..d8d74574 100644 --- a/dc-api/internal/providers/kubeovn/remote_client_test.go +++ b/dc-api/internal/providers/kubeovn/remote_client_test.go @@ -19,12 +19,14 @@ import ( // This file proves the GAP the CRD-lifecycle slice closed: a REMOTE kubeovn // client (NewRemoteClient, c.dynamic == nil) routes the ONBOARDED CRD lifecycle // (Vpc/Subnet/NAD create/get/delete) through its agent-only Accessor instead of -// returning a local-only error — while the REMAINING local-only plumbing (NAT, -// per-VPC DNS) still returns localOnlyErr. The peering/route-table/NSG spec +// returning a local-only error — while the REMAINING local-only plumbing +// (per-VPC DNS) still returns localOnlyErr. The peering/route-table/NSG spec // writes are no longer in the local-only set — network_plumbing_seam_test.go -// proves they route through the seam on a remote client. It also proves a -// remote client built with a NoCreds-only accessor (no agent) fails CLOSED with -// a clear "no agent connected" error rather than nil-dereferencing c.dynamic. +// proves they route through the seam on a remote client — and neither is the +// F15 NAT lifecycle, whose remote-client coverage lives in nat_seam_test.go. +// It also proves a remote client built with a NoCreds-only accessor (no agent) +// fails CLOSED with a clear "no agent connected" error rather than +// nil-dereferencing c.dynamic. // // The remote client has c.dynamic == nil by construction, so these tests are the // regression guard against re-introducing a `c.dynamic == nil` guard on a routed @@ -235,10 +237,11 @@ func TestRemoteClient_DeleteSubnet_RoutesThroughAgent(t *testing.T) { // ── Remaining remote PLUMBING methods stay local-only ──────────────────────── // TestRemoteClient_PlumbingMethods_ReturnLocalOnlyErr pins the REMAINING -// local-only boundary after the spec-write plumbing slice: the per-VPC DNS -// zone/record ops (ConfigMap-backed — configmaps are not an onboarded family). -// The peering/route-table/NSG ops that used to sit here are now agent-routable; -// their remote-client coverage lives in network_plumbing_seam_test.go. +// local-only boundary after the spec-write plumbing and NAT slices: ONLY the +// per-VPC DNS zone/record ops (ConfigMap-backed — configmaps are not an +// onboarded family). The peering/route-table/NSG ops that used to sit here are +// now agent-routable (coverage in network_plumbing_seam_test.go), as is the +// F15 NAT lifecycle (coverage in nat_seam_test.go). func TestRemoteClient_PlumbingMethods_ReturnLocalOnlyErr(t *testing.T) { agent := &remoteAgentAccessor{} c := NewRemoteClient(agent, "lk", "zone-2") diff --git a/dc-api/internal/providers/registry.go b/dc-api/internal/providers/registry.go index 0461492a..c5defe33 100644 --- a/dc-api/internal/providers/registry.go +++ b/dc-api/internal/providers/registry.go @@ -313,8 +313,9 @@ func (r *Registry) buildLocalSet(cfg *config.Config) (*ProviderSet, error) { // yields a clear "no agent connected for zone" error, never a silent hit on // the LOCAL cluster. The harvester client routes the VM-object CRUD lifecycle // through it; the kubeovn client routes the onboarded CRD lifecycle (VPC/ -// Subnet/NAD create/get/delete) through it, while the VPC network plumbing -// (NAT/DNS/ACL/route/peering) stays local-only behind a clear error. +// Subnet/NAD create/get/delete), the ACL/route/peering spec writes, and the +// F15 NAT lifecycle through it, while the per-VPC DNS plumbing stays +// local-only behind a clear error. // - Cluster is the SAME global Rancher client every zone shares. // // buildRemoteSet performs NO network I/O (no kubeconfig to dial), so it is safe diff --git a/flux/platform/dc-agent/base/rbac.yaml b/flux/platform/dc-agent/base/rbac.yaml index 9aa3f572..f1c79083 100644 --- a/flux/platform/dc-agent/base/rbac.yaml +++ b/flux/platform/dc-agent/base/rbac.yaml @@ -26,6 +26,9 @@ rules: - apiGroups: [""] resources: ["nodes", "pods"] verbs: ["get", "list", "watch"] + - apiGroups: [""] + resources: ["pods"] + verbs: ["get", "list"] - apiGroups: [""] resources: ["secrets"] verbs: ["get", "create", "patch"] @@ -38,9 +41,24 @@ rules: - apiGroups: ["k8s.cni.cncf.io"] resources: ["network-attachment-definitions"] verbs: ["get", "list", "watch", "create", "patch", "delete"] + - apiGroups: ["kubeovn.io"] + resources: ["iptables-eips"] + verbs: ["get", "create", "patch", "delete"] + - apiGroups: ["kubeovn.io"] + resources: ["iptables-snat-rules"] + verbs: ["get", "create", "patch", "delete"] + - apiGroups: ["kubeovn.io"] + resources: ["provider-networks"] + verbs: ["create", "patch"] - apiGroups: ["kubeovn.io"] resources: ["subnets"] verbs: ["get", "list", "watch", "create", "patch", "delete"] + - apiGroups: ["kubeovn.io"] + resources: ["vlans"] + verbs: ["create", "patch"] + - apiGroups: ["kubeovn.io"] + resources: ["vpc-nat-gateways"] + verbs: ["get", "create", "patch", "delete"] - apiGroups: ["kubeovn.io"] resources: ["vpcs"] verbs: ["get", "list", "watch", "create", "patch", "delete"] From 216141e35c5865e854284eec7d757203f2b664c0 Mon Sep 17 00:00:00 2001 From: HiranAdikari Date: Mon, 6 Jul 2026 10:42:12 +0530 Subject: [PATCH 2/2] Drop RBAC rules subsumed by the base grant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01RC3BDHCFmJBdxvGVW69FAr --- dc-api/internal/agentrbac/render.go | 38 +++++++++++++++++++++++- dc-api/internal/agentrbac/render_test.go | 35 ++++++++++++++++++++++ flux/platform/dc-agent/base/rbac.yaml | 3 -- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/dc-api/internal/agentrbac/render.go b/dc-api/internal/agentrbac/render.go index 7029c8cf..44b7ec17 100644 --- a/dc-api/internal/agentrbac/render.go +++ b/dc-api/internal/agentrbac/render.go @@ -91,6 +91,13 @@ func RenderAgentRBAC(caps []clusteraccess.AgentCapability) []byte { // rule (nodes/pods get,list,watch — serves the agent's hard-coded get_inventory // op) followed by one rule per capability that grants RBAC (non-empty // AgentVerbs), grouped by apiGroup and sorted deterministically. +// +// A capability rule FULLY covered by a base rule is NOT emitted: RBAC rules are +// additive (union), so the duplicate would grant nothing — and a subset rule +// sitting next to its superset reads like a failed attempt to narrow the grant +// (review-flagged on the pods rule). Subsumption is computed at render time, so +// if a base rule is ever removed (the least-privilege pass), regeneration +// re-emits the capability rule that now carries the grant. func capabilityRules(caps []clusteraccess.AgentCapability) []rule { // Base inventory rule is a generator constant, NOT registry-driven: the // agent's get_inventory reads nodes/pods via the typed clientset. @@ -98,8 +105,22 @@ func capabilityRules(caps []clusteraccess.AgentCapability) []rule { {apiGroup: "", resources: []string{"nodes", "pods"}, verbs: []string{"get", "list", "watch"}}, } - // Merge capability verbs per (apiGroup, resource). + // Coverage granted by the base rules, per (apiGroup, resource). type key struct{ group, resource string } + baseCover := map[key]map[string]bool{} + for _, r := range rules { + for _, res := range r.resources { + k := key{group: r.apiGroup, resource: res} + if baseCover[k] == nil { + baseCover[k] = map[string]bool{} + } + for _, v := range r.verbs { + baseCover[k][v] = true + } + } + } + + // Merge capability verbs per (apiGroup, resource). verbsByKey := map[key]map[string]bool{} for _, c := range caps { if len(c.AgentVerbs) == 0 { @@ -130,6 +151,21 @@ func capabilityRules(caps []clusteraccess.AgentCapability) []rule { }) for _, k := range keys { + // Skip a rule the base already fully grants (see the doc comment). A + // PARTIALLY covered rule is emitted whole — splitting verbs across rules + // would obscure which capability asked for what. + if cover, ok := baseCover[k]; ok { + subsumed := true + for v := range verbsByKey[k] { + if !cover[v] { + subsumed = false + break + } + } + if subsumed { + continue + } + } rules = append(rules, rule{ apiGroup: k.group, resources: []string{k.resource}, diff --git a/dc-api/internal/agentrbac/render_test.go b/dc-api/internal/agentrbac/render_test.go index 1b4016c1..5b0c0645 100644 --- a/dc-api/internal/agentrbac/render_test.go +++ b/dc-api/internal/agentrbac/render_test.go @@ -5,8 +5,11 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" + "k8s.io/apimachinery/pkg/runtime/schema" + "github.com/wso2/dc-api/internal/providers/clusteraccess" ) @@ -50,3 +53,35 @@ func TestRBACByteStable(t *testing.T) { t.Fatalf("RenderAgentRBAC is not byte-stable:\n--- run 1 ---\n%s\n--- run 2 ---\n%s", a, b) } } + +// TestSubsumedCapabilityRuleNotEmitted pins the render-time subsumption rule: +// a capability whose k8s verbs the fixed base inventory rule already fully +// grants (pods get/list ⊂ nodes/pods get/list/watch) emits NO separate +// ClusterRole rule — RBAC is additive, so the duplicate would grant nothing and +// only reads as a failed narrowing. A capability with any verb OUTSIDE the base +// coverage still emits its full rule. +func TestSubsumedCapabilityRuleNotEmitted(t *testing.T) { + podsReadOnly := clusteraccess.AgentCapability{ + GVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + APIVersion: "v1", Kind: "Pod", Namespaced: true, + AgentVerbs: []clusteraccess.Verb{clusteraccess.VerbGet, clusteraccess.VerbList}, + } + out := string(RenderAgentRBAC([]clusteraccess.AgentCapability{podsReadOnly})) + if strings.Contains(out, `resources: ["pods"]`) { + t.Errorf("fully-subsumed pods rule was emitted:\n%s", out) + } + if !strings.Contains(out, `resources: ["nodes", "pods"]`) { + t.Errorf("base inventory rule missing:\n%s", out) + } + + // Widen one verb beyond the base rule (create) → the rule MUST be emitted. + podsWithCreate := podsReadOnly + podsWithCreate.AgentVerbs = []clusteraccess.Verb{clusteraccess.VerbGet, clusteraccess.VerbList, clusteraccess.VerbCreate} + out = string(RenderAgentRBAC([]clusteraccess.AgentCapability{podsWithCreate})) + if !strings.Contains(out, `resources: ["pods"]`) { + t.Errorf("partially-covered pods rule was NOT emitted:\n%s", out) + } + if !strings.Contains(out, `verbs: ["get", "list", "create"]`) { + t.Errorf("partially-covered pods rule lost verbs:\n%s", out) + } +} diff --git a/flux/platform/dc-agent/base/rbac.yaml b/flux/platform/dc-agent/base/rbac.yaml index f1c79083..f0a06417 100644 --- a/flux/platform/dc-agent/base/rbac.yaml +++ b/flux/platform/dc-agent/base/rbac.yaml @@ -26,9 +26,6 @@ rules: - apiGroups: [""] resources: ["nodes", "pods"] verbs: ["get", "list", "watch"] - - apiGroups: [""] - resources: ["pods"] - verbs: ["get", "list"] - apiGroups: [""] resources: ["secrets"] verbs: ["get", "create", "patch"]