diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index af8f440..df9673d 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -112,22 +112,6 @@ const ( // region-simple providers (Modal, RunPod) ignore it. RegionAnnotation = "nebula.inftyai.com/region" - // BlocklistTTLAnnotation carries the pool's FailoverPolicy.BlocklistTTL down to the - // virtual kubelet. The TTL is NodePool policy, but the VK handler provisions per-Pod - // and never sees the pool, so it needs the value here to bound the block it records - // after a Provision failure. Stamped at ungate; absent/unparseable means the - // handler's built-in default. - BlocklistTTLAnnotation = "nebula.inftyai.com/blocklist-ttl" - - // EgressAnnotation carries the pool's EgressPolicy.Mode and EgressTargetsAnnotation its - // Targets, comma-separated (no CIDR or hostname contains one). Same flow and - // reason as BlocklistTTLAnnotation: pool policy the VK handler must honour but never - // sees the pool to read. - // - // Absent means Open. Mode is authoritative — without it the target list is ignored. - EgressAnnotation = "nebula.inftyai.com/egress" - EgressTargetsAnnotation = "nebula.inftyai.com/egress-targets" - // EndpointAnnotation carries the reachable address of the external instance (a DNS // name, an IP, or a URL, in the provider's own form). It is the only way to reach // the workload, and PodIP cannot hold it — the API server validates PodIP as a diff --git a/api/v1alpha1/nodepool_types.go b/api/v1alpha1/nodepool_types.go index 83861ed..0409723 100644 --- a/api/v1alpha1/nodepool_types.go +++ b/api/v1alpha1/nodepool_types.go @@ -82,6 +82,7 @@ type NodePoolSpec struct { // Allowlist disjoint, so "no egress" has one spelling instead of three. // +kubebuilder:validation:XValidation:rule="self.mode == 'Allowlist' || !has(self.targets)",message="targets is only valid with mode Allowlist" // +kubebuilder:validation:XValidation:rule="self.mode != 'Allowlist' || (has(self.targets) && self.targets.size() > 0)",message="mode Allowlist requires at least one target; use mode Blocked to permit nothing" +// +kubebuilder:validation:XValidation:rule="!has(self.targets) || self.targets.all(t, !t.contains(','))",message="a target must not contain a comma; list each target as its own entry" type EgressPolicy struct { // Mode is required once spec.egress is set, so a half-written policy is rejected // rather than defaulted into a weaker one. @@ -89,6 +90,7 @@ type EgressPolicy struct { // Targets is what mode Allowlist permits: CIDRs, bare IPs and domain names with an // optional wildcard, mixed in one list, e.g. ["10.0.0.0/8", "*.huggingface.co"]. + // // +optional // +kubebuilder:validation:MaxItems=64 // +kubebuilder:validation:items:MaxLength=253 @@ -216,6 +218,7 @@ type FailoverPolicy struct { // (up to 30s) on top so Pods that failed for the same reason do not all retry the // just-freed candidate in lockstep, so the effective exclusion is this value plus // that jitter. + // // +kubebuilder:default="30s" BlocklistTTL metav1.Duration `json:"blocklistTTL,omitempty"` } diff --git a/cmd/main.go b/cmd/main.go index 204bb4e..0a5b1d4 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -458,7 +458,10 @@ func setupVirtualNodes(mgr ctrl.Manager, blocklist vnode.Blocklister, kubeletSrv if !ok { continue } - if err := mgr.Add(vnode.NewRunner(prov, clientset, blocklist, kubeletSrv)); err != nil { + // The manager's client, so the pool read on the provisioning path hits the shared + // cache the controllers already keep warm. + pools := vnode.NewCachedPoolReader(mgr.GetClient()) + if err := mgr.Add(vnode.NewRunner(prov, clientset, blocklist, kubeletSrv, pools)); err != nil { return err } setupLog.Info("registered virtual node", "provider", name, "node", vnode.NodeName(name)) diff --git a/config/crd/bases/nebula.inftyai.com_nodepools.yaml b/config/crd/bases/nebula.inftyai.com_nodepools.yaml index b0c58f6..b084570 100644 --- a/config/crd/bases/nebula.inftyai.com_nodepools.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodepools.yaml @@ -128,6 +128,9 @@ spec: to permit nothing rule: self.mode != 'Allowlist' || (has(self.targets) && self.targets.size() > 0) + - message: a target must not contain a comma; list each target as + its own entry + rule: '!has(self.targets) || self.targets.all(t, !t.contains('',''))' failover: description: |- Failover controls how a provider that fails at provision time (e.g. diff --git a/internal/controller/nodepool_validation_test.go b/internal/controller/nodepool_validation_test.go index d3a25a2..15aa327 100644 --- a/internal/controller/nodepool_validation_test.go +++ b/internal/controller/nodepool_validation_test.go @@ -117,6 +117,60 @@ var _ = Describe("NodePool spec validation", func() { Expect(err.Error()).To(ContainSubstring("Unsupported value: \"Reserved\"")) }) + // The egress rules. The comma one is the load-bearing case: Targets rides to the VK + // handler as ONE comma-separated annotation, so an entry containing a comma would be + // decoded as several permitted targets rather than the single invalid one the author + // wrote — a policy wider than the pool declares, which is the wrong way for a + // containment control to fail. Admission is the only thing standing between the two. + newEgressPool := func(name string, egress *nebulav1alpha1.EgressPolicy) *nebulav1alpha1.NodePool { + return &nebulav1alpha1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: nebulav1alpha1.NodePoolSpec{ + Providers: []nebulav1alpha1.ProviderSpec{{Name: "modal"}}, + Strategy: nebulav1alpha1.StrategyOrdered, + Egress: egress, + }, + } + } + + It("rejects a target containing a comma", func() { + pool := newEgressPool("egress-packed-target", &nebulav1alpha1.EgressPolicy{ + Mode: nebulav1alpha1.EgressAllowlist, + Targets: []string{"10.0.0.0/8,api.openai.com"}, + }) + err := k8sClient.Create(ctx, pool) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("must not contain a comma")) + }) + + It("admits the same two targets listed separately", func() { + pool := newEgressPool("egress-split-targets", &nebulav1alpha1.EgressPolicy{ + Mode: nebulav1alpha1.EgressAllowlist, + Targets: []string{"10.0.0.0/8", "*.huggingface.co"}, + }) + Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + Expect(k8sClient.Delete(ctx, pool)).To(Succeed()) + }) + + It("rejects targets without mode Allowlist", func() { + pool := newEgressPool("egress-blocked-with-targets", &nebulav1alpha1.EgressPolicy{ + Mode: nebulav1alpha1.EgressBlocked, + Targets: []string{"10.0.0.0/8"}, + }) + err := k8sClient.Create(ctx, pool) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("targets is only valid with mode Allowlist")) + }) + + It("rejects mode Allowlist with no targets", func() { + pool := newEgressPool("egress-allowlist-empty", &nebulav1alpha1.EgressPolicy{ + Mode: nebulav1alpha1.EgressAllowlist, + }) + err := k8sClient.Create(ctx, pool) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("requires at least one target")) + }) + It("admits the Spot and OnDemand capacity tiers", func() { pool := &nebulav1alpha1.NodePool{ ObjectMeta: metav1.ObjectMeta{Name: "ok-capacity-types"}, diff --git a/internal/controller/pod_placement_controller.go b/internal/controller/pod_placement_controller.go index fd00ccb..d0b1bd1 100644 --- a/internal/controller/pod_placement_controller.go +++ b/internal/controller/pod_placement_controller.go @@ -176,7 +176,7 @@ func (r *PodPlacementReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Stamp the routing decision and release the Pod to the scheduler. - if err := r.place(ctx, &pod, pool, placement); err != nil { + if err := r.place(ctx, &pod, placement); err != nil { return ctrl.Result{}, err } // Counted only after the Update lands, which is also what keeps it from diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index 23f5a62..83cc392 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -19,6 +19,7 @@ package controller import ( "context" "slices" + "strings" "testing" "time" @@ -597,10 +598,7 @@ func TestPlacement_SpotOnlyPoolStaysGatedOnOnDemandOnlyProvider(t *testing.T) { } } -func TestPlacement_StampsEgressAnnotationsFromPool(t *testing.T) { - // The VK handler never sees the pool, so the policy has to ride the Pod. Both - // annotations must land: the mode alone is authoritative, and without the targets - // an Allowlist pool would reach the adapter as "permit nothing" — silently Blocked. +func TestPlacement_CopiesNoEgressPolicyOntoThePod(t *testing.T) { pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, provider.ProviderModal) pool.Spec.Egress = &nebulav1alpha1.EgressPolicy{ @@ -616,33 +614,18 @@ func TestPlacement_StampsEgressAnnotationsFromPool(t *testing.T) { if hasGateNamed(got) { t.Fatal("expected the Pod placed on a provider that enforces egress") } - if v := got.Annotations[nebulav1alpha1.EgressAnnotation]; v != string(nebulav1alpha1.EgressAllowlist) { - t.Errorf("egress annotation = %q, want %q", v, nebulav1alpha1.EgressAllowlist) - } - if v := got.Annotations[nebulav1alpha1.EgressTargetsAnnotation]; v != "10.0.0.0/8,*.huggingface.co" { - t.Errorf("egress-targets annotation = %q, want the comma-joined list", v) - } -} - -func TestPlacement_OpenPoolStampsNoEgressAnnotation(t *testing.T) { - // Absence IS Open (see EgressAnnotation), so an unrestricted pool must leave the Pod - // clean rather than stamping "Open" — otherwise every Pod in the cluster grows an - // annotation that means nothing. - pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") - pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, provider.ProviderModal) - // No egress on the pool at all, which is the common case. - prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"H100"}} // egress: false - r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov) - - reconcilePod(t, r, "default", "p1") - - got := getPod(t, c, "default", "p1") - if hasGateNamed(got) { - t.Fatal("expected an Open pool to place on a provider without egress support") + // Swept by substring rather than checked against the two retired keys, because the claim + // is that NO key carries the policy — a copy under a fresh spelling is the same bug and + // should fail here too. + for k, v := range got.Annotations { + if strings.Contains(k, "egress") { + t.Errorf("annotation %s=%q copies the pool's egress policy onto the Pod; the "+ + "handler must read it from the pool", k, v) + } } - if _, ok := got.Annotations[nebulav1alpha1.EgressAnnotation]; ok { - t.Errorf("expected no egress annotation for an Open pool, got %q", - got.Annotations[nebulav1alpha1.EgressAnnotation]) + // The pool label is what the handler resolves the policy through, so it must be set. + if got.Labels[nebulav1alpha1.PoolLabel] != "pool-a" { + t.Errorf("pool label = %q, want pool-a", got.Labels[nebulav1alpha1.PoolLabel]) } } @@ -764,23 +747,6 @@ func TestPlacement_NoServableCandidateDoesNotRequeue(t *testing.T) { } } -func TestPlacement_StampsBlocklistTTLAnnotation(t *testing.T) { - // The pool's FailoverPolicy.BlocklistTTL must reach the Pod so the VK handler - // knows how long to blocklist a placement that fails. - pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") - pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, provider.ProviderModal) - pool.Spec.Failover = &nebulav1alpha1.FailoverPolicy{BlocklistTTL: metav1.Duration{Duration: 7 * time.Minute}} - prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"H100"}} - r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov) - - reconcilePod(t, r, "default", "p1") - - got := getPod(t, c, "default", "p1") - if got.Annotations[nebulav1alpha1.BlocklistTTLAnnotation] != "7m0s" { - t.Fatalf("expected blocklist-ttl annotation 7m0s, got %q", got.Annotations[nebulav1alpha1.BlocklistTTLAnnotation]) - } -} - // terminalOwnedPod builds an opted-in Pod in a terminal phase. When ownedByRS is // true it carries a controlling ReplicaSet ownerReference (so a controller would // recreate it); otherwise it is a bare Pod. diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index 288987d..ce68c36 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -19,7 +19,6 @@ package controller import ( "context" "hash/fnv" - "strings" "time" corev1 "k8s.io/api/core/v1" @@ -358,7 +357,12 @@ func (r *PodPlacementReconciler) ensureClaim(ctx context.Context, pod *corev1.Po // place stamps the routing decision onto the Pod and removes the gate, atomically // from the Pod's perspective (one Update). After this, the scheduler is free to // bind the Pod to the chosen provider's virtual node. -func (r *PodPlacementReconciler) place(ctx context.Context, pod *corev1.Pod, pool *nebulav1alpha1.NodePool, p placement) error { +// +// It takes the DECISION, not the pool: nothing from the pool's own spec is copied onto the +// Pod any more. The egress policy and the failover TTL both used to be stamped here for the +// VK handler to read back, which made them patchable by whoever owns the Pod; the handler +// reads both from the NodePool at provision time instead. +func (r *PodPlacementReconciler) place(ctx context.Context, pod *corev1.Pod, p placement) error { // Route to the provider's virtual node. if pod.Spec.NodeSelector == nil { pod.Spec.NodeSelector = map[string]string{} @@ -373,22 +377,6 @@ func (r *PodPlacementReconciler) place(ctx context.Context, pod *corev1.Pod, poo if p.region != "" { setAnnotation(pod, nebulav1alpha1.RegionAnnotation, p.region) } - // Carry the pool's blocklist TTL so the VK handler (which never sees the pool) - // knows how long to exclude a placement that fails. Only stamp an explicit - // policy value; an unset policy leaves the handler on its own default. - if pool.Spec.Failover != nil && pool.Spec.Failover.BlocklistTTL.Duration > 0 { - setAnnotation(pod, nebulav1alpha1.BlocklistTTLAnnotation, pool.Spec.Failover.BlocklistTTL.Duration.String()) - } - // Same for the egress policy. Stamped only when it restricts something: the absence of - // the annotation IS Open, and selectPlacement has already ensured p.provider can enforce - // whatever is written here. - if pool.Spec.Egress.RestrictsEgress() { - setAnnotation(pod, nebulav1alpha1.EgressAnnotation, string(pool.Spec.Egress.ModeOrOpen())) - if len(pool.Spec.Egress.Targets) > 0 { - setAnnotation(pod, nebulav1alpha1.EgressTargetsAnnotation, strings.Join(pool.Spec.Egress.Targets, ",")) - } - } - // Remove our gate, releasing the Pod to the scheduler. Preserve any other // gates a different controller may hold. pod.Spec.SchedulingGates = removeGate(pod.Spec.SchedulingGates, nebulav1alpha1.ProviderSelectionGate) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 2e21fbd..d59ad48 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -235,7 +235,8 @@ type ProvisionRequest struct { // that this provider can enforce it (Capabilities.SupportsEgressPolicy), so an adapter // receiving a restrictive policy must apply it or fail the Provision — never silently // drop it, which would leave the workload on the open internet under a policy that says - // otherwise. Read off the Pod's annotations, not the pool; see EgressAnnotation. + // otherwise. Resolved from the NodePool at provision time, never from the Pod, which + // the workload's own owner can patch (see vnode.Handler.egressFor). Egress *nebulav1alpha1.EgressPolicy // Env is the container's environment, fully RESOLVED: literals plus everything // envFrom/valueFrom referenced, merged in kubelet precedence (envFrom in listed order, diff --git a/pkg/vnode/env_test.go b/pkg/vnode/env_test.go index 2ef9f94..d6c54b6 100644 --- a/pkg/vnode/env_test.go +++ b/pkg/vnode/env_test.go @@ -247,7 +247,7 @@ func TestResolveEnv_MemoizesMisses(t *testing.T) { func TestResolveEnv_FieldRef(t *testing.T) { pod := envPod(nil, nil) pod.UID = "uid-1" - pod.Labels = map[string]string{"app": "vllm"} + pod.Labels["app"] = "vllm" pod.Annotations = map[string]string{"team": "infra"} pod.Spec.NodeName = "nebula-fake" pod.Spec.ServiceAccountName = "sa" @@ -368,7 +368,7 @@ func TestCreatePod_PassesResolvedEnvToProvider(t *testing.T) { LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}, Key: "K"}}}, }) client := fake.NewSimpleClientset(pod, secretObj("sec", map[string]string{"K": "t0ken"})) - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod: %v", err) @@ -393,7 +393,7 @@ func TestCreatePod_UnresolvableEnvIsNonTerminal(t *testing.T) { LocalObjectReference: corev1.LocalObjectReference{Name: "not-yet"}, Key: "K"}, }}}) client := fake.NewSimpleClientset(pod) - h := NewHandler(fp, client, bl) + h := NewHandler(fp, client, bl, openPools()) err := h.CreatePod(context.Background(), pod) if err == nil { diff --git a/pkg/vnode/exec_test.go b/pkg/vnode/exec_test.go index f0d5baf..20f2f1e 100644 --- a/pkg/vnode/exec_test.go +++ b/pkg/vnode/exec_test.go @@ -281,7 +281,7 @@ func TestRunExec_CancelReleasesTheProcess(t *testing.T) { func TestRunInContainer_RunsInTrackedInstance(t *testing.T) { proc := newExecProcess("root@sandbox:/#\n", "", 0) ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, proc) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -311,7 +311,7 @@ func TestRunInContainer_RunsInTrackedInstance(t *testing.T) { func TestRunInContainer_IgnoresContainerName(t *testing.T) { for _, container := range []string{"", "main", "not-a-container"} { ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, newExecProcess("ok\n", "", 0)) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -328,7 +328,7 @@ func TestRunInContainer_IgnoresContainerName(t *testing.T) { func TestRunInContainer_NotFoundCases(t *testing.T) { // No exec support at all — a legitimate configuration (no agent, no key), not a bug. t.Run("provider does not support exec", func(t *testing.T) { - h := NewHandler(&fakeProvider{provisionID: "inst-1"}, nil, nil) + h := NewHandler(&fakeProvider{provisionID: "inst-1"}, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -340,7 +340,7 @@ func TestRunInContainer_NotFoundCases(t *testing.T) { // Another node's pod, or one this process never adopted. t.Run("pod not tracked", func(t *testing.T) { ep := newExecProvider(&fakeProvider{}, newExecProcess("", "", 0)) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) err := h.RunInContainer(context.Background(), "default", "ghost", "main", []string{"sh"}, &fakeAttach{stdout: newSyncBuffer()}) assertNotFound(t, err) @@ -353,7 +353,7 @@ func TestRunInContainer_NotFoundCases(t *testing.T) { // nothing to run in. t.Run("tracked without an instance", func(t *testing.T) { ep := newExecProvider(&fakeProvider{provisionErr: errors.New("no capacity")}, newExecProcess("", "", 0)) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { t.Fatal("CreatePod: expected the provision rejection to surface") } @@ -369,7 +369,7 @@ func TestRunInContainer_NotFoundCases(t *testing.T) { func TestRunInContainer_OnlyTheStartIsBounded(t *testing.T) { proc := newExecProcess("ok\n", "", 0) ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, proc) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -395,7 +395,7 @@ func TestRunInContainer_OnlyTheStartIsBounded(t *testing.T) { func TestRunInContainer_StartErrorIsNotNotFound(t *testing.T) { ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, newExecProcess("", "", 0)) ep.execErr = errors.New("timed out waiting for task id") - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -417,7 +417,7 @@ func TestRunInContainer_StartErrorIsNotNotFound(t *testing.T) { // provider — a provider that ran a default shell for it would be a surprise. func TestRunInContainer_EmptyCommandRejected(t *testing.T) { ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, newExecProcess("", "", 0)) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index d036540..c9d1178 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -22,7 +22,6 @@ import ( "fmt" "io" "math/rand/v2" - "strings" "sync" "time" @@ -49,9 +48,8 @@ import ( // signal. A provider can override the cadence via Capabilities.PollInterval. const defaultPollInterval = 15 * time.Second -// defaultBlocklistTTL is the base exclusion for a failed placement when the Pod -// carries no BlocklistTTLAnnotation. Short on purpose: the jitter added in -// recordBlock, not a long floor, is what spreads retries. +// defaultBlocklistTTL is the base exclusion for a failed placement when the pool sets no +// positive FailoverPolicy.BlocklistTTL (see blocklistTTLOf). const defaultBlocklistTTL = 30 * time.Second // blocklistJitter is the largest random delay added to a block's base TTL. Without @@ -93,6 +91,12 @@ type Handler struct { // no-op. blocklist Blocklister + // pools resolves the NodePool backing a Pod, which is where pool POLICY is read from + // rather than from the Pod carrying a copy of it. Unlike blocklist, nil is NOT a no-op: + // provisioning refuses without it, because the alternative is enforcing a containment + // policy nobody has read (see poolFor). + pools PoolReader + mu sync.Mutex // tracked is the poll loop's work list and what GetPod/GetPodStatus serve, keyed by @@ -193,8 +197,11 @@ func (m podMeta) minus(done podMeta) podMeta { // NewHandler builds a Handler for one provider backend. The poll cadence comes from // Capabilities.PollInterval, falling back to defaultPollInterval. blocklist (failover -// recording) and client (the endpoint patch) may both be nil. -func NewHandler(prov provider.Provider, client kubernetes.Interface, blocklist Blocklister) *Handler { +// recording) and client (the endpoint patch) may both be nil; pools may not, for anything +// that provisions — see poolFor. +func NewHandler( + prov provider.Provider, client kubernetes.Interface, blocklist Blocklister, pools PoolReader, +) *Handler { poll := prov.Capabilities().PollInterval if poll <= 0 { poll = defaultPollInterval @@ -203,6 +210,7 @@ func NewHandler(prov provider.Provider, client kubernetes.Interface, blocklist B prov: prov, client: client, blocklist: blocklist, + pools: pools, tracked: make(map[string]*trackedPod), nowFn: metav1.Now, pollEvery: poll, @@ -221,20 +229,33 @@ var ( func key(namespace, name string) string { return namespace + "/" + name } // CreatePod provisions an external instance for the Pod through the provider. -// The Pod carries the whole workload shape; the only out-of-band input, the -// optimizer's capacity tier, rides on CapacityTypeAnnotation. +// The Pod carries the whole workload shape, and placement's choices (capacity tier, region) +// ride on annotations. The egress policy is the exception: it is read from the NodePool, +// because it constrains the Pod rather than describing it (see poolFor). func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { claim := util.ClaimName(pod.Namespace, pod.Name) req := provider.ProvisionRequest{ ClaimName: claim, CapacityType: nebulav1alpha1.CapacityType(pod.Annotations[nebulav1alpha1.CapacityTypeAnnotation]), Region: pod.Annotations[nebulav1alpha1.RegionAnnotation], - Egress: egressFromPod(pod), } log := logf.FromContext(ctx).WithName("vnode-handler").WithValues( "provider", h.prov.Name(), "pod", key(pod.Namespace, pod.Name), "claim", claim) + // The pool, which is the trusted source for every policy below: the containment policy + // here, and the TTL of any block this provision's failure records. Resolved before + // anything is requested and with the same non-terminal treatment as the env below, so a + // pool we cannot read costs a retry rather than an unrestricted instance. + pool, err := h.poolFor(ctx, pod) + if err != nil { + log.Error(err, "cannot establish the pool's policy; nothing provisioned, retrying") + h.markStatus(pod, corev1.PodPending, reasonConfigError, err.Error()) + h.emit(pod) + return err + } + req.Egress = pool.Spec.Egress + // Resolve BEFORE anything is requested: the Pod's env may point at Secrets and ConfigMaps // a provider cannot read (see resolveEnv), and nothing exists yet, so failing here is free. // @@ -320,8 +341,8 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { // Record the failure so placement fails over to the next candidate (zone → region // → tier) instead of hot-looping here. The provider narrows its own error into a // BlockScope (a Spot shortage in one region blocks only that; auth/quota blocks - // the whole provider); the TTL rides on the Pod from the pool's FailoverPolicy. - h.recordBlock(ctx, pod, err) + // the whole provider); the TTL comes from the pool read at the top of this call. + h.recordBlock(ctx, pod, pool, err) // Surface the failure so placement can fail over, and return the error so the pod // controller retries with backoff. h.markStatus(pod, corev1.PodFailed, reasonProvisionFailed, err.Error()) @@ -940,7 +961,7 @@ func (h *Handler) markStatus(pod *corev1.Pod, phase corev1.PodPhase, reason, msg // from an instance-type shortage. The cost is one wasted re-probe by a sibling // accelerator, which is the right trade — over-broad would exclude serviceable // accelerators. DenyAll (auth/quota) ignores the accelerator: it fails for all. -func (h *Handler) recordBlock(ctx context.Context, pod *corev1.Pod, err error) { +func (h *Handler) recordBlock(ctx context.Context, pod *corev1.Pod, pool *nebulav1alpha1.NodePool, err error) { if h.blocklist == nil { return } @@ -968,7 +989,7 @@ func (h *Handler) recordBlock(ctx context.Context, pod *corev1.Pod, err error) { // TTL = base (pool policy or default) + jitter, so Pods that failed for the SAME scope // do not all re-probe the just-freed candidate at once. Coalescing keeps the latest // expiry, so jittered records spread the shared deadline instead of pinning it. - ttl := blocklistTTL(pod) + h.jitterFn() + ttl := blocklistTTLOf(pool) + h.jitterFn() // The ctx logger, because it carries the virtualNode/provider values attached // upstream; a fresh context.Background() would fall back to the global delegate and // could be dropped before the real sink is installed. @@ -978,43 +999,6 @@ func (h *Handler) recordBlock(ctx context.Context, pod *corev1.Pod, err error) { h.blocklist.Record(h.prov.Name(), scope, ttl) } -// blocklistTTL reads the pool's BlocklistTTL off the annotation placement stamped, -// falling back to defaultBlocklistTTL when it is absent or unparseable — including a -// non-positive value, which would otherwise install a permanent block. -func blocklistTTL(pod *corev1.Pod) time.Duration { - raw := pod.Annotations[nebulav1alpha1.BlocklistTTLAnnotation] - if raw == "" { - return defaultBlocklistTTL - } - d, err := time.ParseDuration(raw) - if err != nil || d <= 0 { - return defaultBlocklistTTL - } - return d -} - -// egressFromPod reads the pool's egress policy off the annotations placement stamped. nil -// (no annotation) means Open, which is what every pool that never set the field gets. -// -// The mode is taken verbatim rather than validated against the enum: admission already -// rejected anything else, and an unknown value must not silently become Open — the adapter -// fails the Provision instead, which is the safe direction for a containment policy. -func egressFromPod(pod *corev1.Pod) *nebulav1alpha1.EgressPolicy { - mode := pod.Annotations[nebulav1alpha1.EgressAnnotation] - if mode == "" { - return nil - } - policy := &nebulav1alpha1.EgressPolicy{Mode: nebulav1alpha1.EgressMode(mode)} - if raw := pod.Annotations[nebulav1alpha1.EgressTargetsAnnotation]; raw != "" { - for _, e := range strings.Split(raw, ",") { - if e = strings.TrimSpace(e); e != "" { - policy.Targets = append(policy.Targets, e) - } - } - } - return policy -} - func ptrNow(t metav1.Time) *metav1.Time { return &t } // Tracks reports whether this virtual node is the one running the Pod. The kubelet API diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index ab09ce3..634f581 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -30,6 +30,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -145,16 +146,68 @@ func (b *recordingBlocklist) Record(prov string, scope provider.BlockScope, ttl b.calls++ } +// testPoolName is the pool every testPod is placed against. Provisioning resolves the +// pool by this label, so a Pod without it cannot be provisioned at all (see egressFor). +const testPoolName = "pool-a" + func testPod(ns, name string) *corev1.Pod { return &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name}, - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "main", Image: "img"}}}, + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, Name: name, + Labels: map[string]string{nebulav1alpha1.PoolLabel: testPoolName}, + }, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "main", Image: "img"}}}, } } +// fakePools stands in for the manager's NodePool cache, serving pools from a map so a test +// can pin the policy the handler is supposed to read (and count that it read it). +type fakePools struct { + pools map[string]*nebulav1alpha1.NodePool + err error // when set, every Get fails with it + calls int +} + +func (f *fakePools) Get(_ context.Context, name string) (*nebulav1alpha1.NodePool, error) { + f.calls++ + if f.err != nil { + return nil, f.err + } + pool, ok := f.pools[name] + if !ok { + return nil, apierrors.NewNotFound( + schema.GroupResource{Group: nebulav1alpha1.GroupVersion.Group, Resource: "nodepools"}, name) + } + return pool, nil +} + +// testPools serves testPoolName with the given policy; nil is an unrestricted pool, which +// is what a pool that never set spec.egress means. +func testPools(egress *nebulav1alpha1.EgressPolicy) *fakePools { + return &fakePools{pools: map[string]*nebulav1alpha1.NodePool{ + testPoolName: { + ObjectMeta: metav1.ObjectMeta{Name: testPoolName}, + Spec: nebulav1alpha1.NodePoolSpec{Egress: egress}, + }, + }} +} + +// openPools is the default for the many tests that provision without caring about egress. +func openPools() *fakePools { return testPools(nil) } + +// poolsWithTTL serves testPoolName with an explicit failover TTL and no egress policy, for +// the blocklist tests: the TTL is pool policy, read when a block is recorded. +func poolsWithTTL(ttl time.Duration) *fakePools { + pools := openPools() + pools.pools[testPoolName].Spec.Failover = &nebulav1alpha1.FailoverPolicy{ + BlocklistTTL: metav1.Duration{Duration: ttl}, + } + return pools +} + func TestCreatePod_ProvisionsAndTracks(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") pod.Annotations = map[string]string{nebulav1alpha1.CapacityTypeAnnotation: string(nebulav1alpha1.CapacitySpot)} @@ -185,7 +238,7 @@ func TestCreatePod_ReservedAdvancesToInitializing(t *testing.T) { // allocated) means the instance is committed and booting, so the Pod may leave // Provisioning immediately instead of waiting a whole poll tick for the same news. fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") if err := h.CreatePod(context.Background(), pod); err != nil { @@ -214,7 +267,7 @@ func TestCreatePod_UnreservedStaysProvisioning(t *testing.T) { // exactly true. Advancing to Initializing here would claim a commitment the // provider has not made. fp := &fakeProvider{provisionID: "sb-1", provisionReserved: false} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") if err := h.CreatePod(context.Background(), pod); err != nil { @@ -243,7 +296,7 @@ func TestCreatePod_EmitsProvisioningWhileProvisionInFlight(t *testing.T) { // Emitting an untracked Pod is what makes this safe: the tracked invariant forbids // storing one mid-provision, not reporting one. fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) var mu sync.Mutex var reasons []string @@ -284,7 +337,7 @@ func TestCreatePod_PollTickDuringProvisionEmitsNoTerminalStatus(t *testing.T) { // correct either way. The damage is the emit — VK writes it to the API server, where // Pod phases are terminal-sticky, and the NodeClaim then reclaims a live instance. fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) var mu sync.Mutex var emitted []string @@ -321,7 +374,7 @@ func TestCreatePod_PollTickDuringProvisionEmitsNoTerminalStatus(t *testing.T) { func TestCreatePod_ProvisionErrorSurfaces(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity")} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") if err := h.CreatePod(context.Background(), pod); err == nil { @@ -353,7 +406,7 @@ func TestCreatePod_UnattributableErrorLeavesPodProvisioning(t *testing.T) { classifyScope: provider.BlockScope{Accelerator: &accel}, } bl := &recordingBlocklist{} - h := NewHandler(fp, nil, bl) + h := NewHandler(fp, nil, bl, openPools()) var mu sync.Mutex var emitted []string @@ -401,14 +454,13 @@ func TestCreatePod_ProvisionFailureRecordsBlock(t *testing.T) { } fp := &fakeProvider{provisionErr: errors.New("no capacity"), classifyScope: scope} bl := &recordingBlocklist{} - h := NewHandler(fp, nil, bl) + // A non-default TTL on the POOL, so this asserts the pool's policy is honored rather + // than that it happens to equal defaultBlocklistTTL. + h := NewHandler(fp, nil, bl, poolsWithTTL(7*time.Minute)) h.jitterFn = func() time.Duration { return 0 } // pin jitter so the base TTL is asserted exactly pod := testPod("default", "p1") - // A non-default TTL so this asserts the annotation is honored, not that it - // happens to equal defaultBlocklistTTL. - pod.Annotations = map[string]string{nebulav1alpha1.BlocklistTTLAnnotation: "7m"} - pod.Labels = map[string]string{nebulav1alpha1.AcceleratorTypeLabel: "H100"} + pod.Labels[nebulav1alpha1.AcceleratorTypeLabel] = "H100" if err := h.CreatePod(context.Background(), pod); err == nil { t.Fatal("expected CreatePod to return the provision error") @@ -428,9 +480,9 @@ func TestCreatePod_ProvisionFailureRecordsBlock(t *testing.T) { if bl.scope != scope { t.Fatalf("recorded scope = %+v, want %+v", bl.scope, scope) } - // TTL comes from the Pod annotation the placement controller stamped. + // TTL comes from the pool's FailoverPolicy, read at the moment the block is recorded. if bl.ttl != 7*time.Minute { - t.Fatalf("recorded ttl = %v, want 7m from the annotation", bl.ttl) + t.Fatalf("recorded ttl = %v, want 7m from the pool", bl.ttl) } } @@ -439,7 +491,7 @@ func TestCreatePod_EmptyScopeDoesNotBlock(t *testing.T) { // (which would exclude everything on the provider). fp := &fakeProvider{provisionErr: errors.New("weird"), classifyScope: provider.BlockScope{}} bl := &recordingBlocklist{} - h := NewHandler(fp, nil, bl) + h := NewHandler(fp, nil, bl, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { t.Fatal("expected CreatePod to return the provision error") @@ -449,32 +501,70 @@ func TestCreatePod_EmptyScopeDoesNotBlock(t *testing.T) { } } -func TestCreatePod_MissingTTLAnnotationUsesDefault(t *testing.T) { +// TestCreatePod_BlocklistTTLFallsBackToDefault covers the two ways a READABLE pool can fail +// to supply a usable TTL. An unreadable pool is not among them: poolFor fails closed before +// Provision is ever called, so there is no failure to blocklist — which is why the fallback +// is a pure function of the pool rather than a second read that would have to decide. +func TestCreatePod_BlocklistTTLFallsBackToDefault(t *testing.T) { + for _, tc := range []struct { + name string + pools PoolReader + }{{ + name: "pool sets no failover policy", + pools: openPools(), + }, { + // Zero would install a PERMANENT block, so it has to read as "unset". + name: "pool sets a zero TTL", + pools: poolsWithTTL(0), + }} { + t.Run(tc.name, func(t *testing.T) { + fp := &fakeProvider{provisionErr: errors.New("no capacity"), classifyScope: provider.BlockScope{DenyAll: true}} + bl := &recordingBlocklist{} + h := NewHandler(fp, nil, bl, tc.pools) + h.jitterFn = func() time.Duration { return 0 } // pin jitter so the base default is exact + + if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { + t.Fatal("expected CreatePod to return the provision error") + } + if bl.calls != 1 { + t.Fatalf("expected the block to be recorded anyway, got %d Record calls", bl.calls) + } + if bl.ttl != defaultBlocklistTTL { + t.Fatalf("recorded ttl = %v, want default %v", bl.ttl, defaultBlocklistTTL) + } + }) + } +} + +// TestCreatePod_UnreadablePoolBlocksNothing is the other half: a pool that cannot be read +// fails closed, so no instance is requested and — the part worth pinning — no blocklist entry +// is filed either. Blocking a candidate over OUR failure to read a pool would exclude +// serviceable capacity for every tenant sharing the blocklist. +func TestCreatePod_UnreadablePoolBlocksNothing(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity"), classifyScope: provider.BlockScope{DenyAll: true}} bl := &recordingBlocklist{} - h := NewHandler(fp, nil, bl) - h.jitterFn = func() time.Duration { return 0 } // pin jitter so the base default is asserted exactly + h := NewHandler(fp, nil, bl, &fakePools{err: errors.New("cache not synced")}) - // No BlocklistTTLAnnotation on the Pod => the handler's built-in default. if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { - t.Fatal("expected CreatePod to return the provision error") + t.Fatal("expected CreatePod to fail closed on an unreadable pool") } - if bl.ttl != defaultBlocklistTTL { - t.Fatalf("recorded ttl = %v, want default %v", bl.ttl, defaultBlocklistTTL) + if fp.provisionCnt != 0 { + t.Errorf("provision calls = %d, want 0", fp.provisionCnt) + } + if bl.calls != 0 { + t.Errorf("Record calls = %d, want 0; our own read failure must not blocklist a candidate", bl.calls) } } -// The recorded TTL is the base (annotation or default) PLUS the handler's jitter, -// so Pods failing for one scope do not re-probe the freed candidate in lockstep. +// The recorded TTL is the base (the pool's, or the default) PLUS the handler's jitter, so +// Pods failing for one scope do not re-probe the freed candidate in lockstep. func TestCreatePod_BlocklistTTLAddsJitter(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity"), classifyScope: provider.BlockScope{DenyAll: true}} bl := &recordingBlocklist{} - h := NewHandler(fp, nil, bl) + h := NewHandler(fp, nil, bl, poolsWithTTL(30*time.Second)) h.jitterFn = func() time.Duration { return 20 * time.Second } // deterministic jitter - pod := testPod("default", "p1") - pod.Annotations = map[string]string{nebulav1alpha1.BlocklistTTLAnnotation: "30s"} - if err := h.CreatePod(context.Background(), pod); err == nil { + if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { t.Fatal("expected CreatePod to return the provision error") } if want := 30*time.Second + 20*time.Second; bl.ttl != want { @@ -485,7 +575,7 @@ func TestCreatePod_BlocklistTTLAddsJitter(t *testing.T) { // The production jitter draw stays within [0, blocklistJitter) — never negative // (which would shorten the block below its base) and never at/over the bound. func TestProductionJitterInRange(t *testing.T) { - h := NewHandler(&fakeProvider{}, nil, nil) + h := NewHandler(&fakeProvider{}, nil, nil, openPools()) for i := 0; i < 1000; i++ { j := h.jitterFn() if j < 0 || j >= blocklistJitter { @@ -496,7 +586,7 @@ func TestProductionJitterInRange(t *testing.T) { func TestDeletePod_TerminatesAndUntracks(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") if err := h.CreatePod(context.Background(), pod); err != nil { @@ -518,7 +608,7 @@ func TestDeletePod_TerminatesAndUntracks(t *testing.T) { func TestDeletePod_Idempotent(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -532,7 +622,7 @@ func TestDeletePod_Idempotent(t *testing.T) { func TestReconcileOnce_ReportsRunning(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -579,7 +669,7 @@ func TestReconcileOnce_DNSEndpointNotWrittenToPodIP(t *testing.T) { // must be left empty. The reachable address is surfaced on the annotation // instead, which accepts any form. fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -620,7 +710,7 @@ func TestNotify_PersistsEndpointAnnotationOnce(t *testing.T) { }) fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) _ = h.CreatePod(context.Background(), pod) // Register the notify wrapper (this is where persistMetadata is injected). h.NotifyPods(context.Background(), func(*corev1.Pod) {}) @@ -670,7 +760,7 @@ func TestCreatePod_PersistsInstanceIDAlongsideEndpoint(t *testing.T) { }) fp := &fakeProvider{provisionID: "inst-1", provisionURL: url, provisionToken: "tok-abc"} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) // Wrapper registered BEFORE the create, so the create-path emit is the write. h.NotifyPods(context.Background(), func(*corev1.Pod) {}) @@ -734,7 +824,7 @@ func TestCreatePod_WritesConnectSecret(t *testing.T) { client := fake.NewSimpleClientset(pod) fp := &fakeProvider{provisionID: "inst-1", provisionURL: url, provisionToken: token} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) // Register the notifier BEFORE CreatePod, as VK does (it wires NotifyPods before // any pod work starts). The endpoint reaches the API server through it. h.NotifyPods(context.Background(), func(*corev1.Pod) {}) @@ -799,7 +889,7 @@ func TestConnectSecret_WrittenOnceNotPerTick(t *testing.T) { provisionURL: "https://sb-1.modal.host", provisionToken: "tok-abc", } - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -833,7 +923,7 @@ func TestCreatePod_NoSecretWithoutCredential(t *testing.T) { client := fake.NewSimpleClientset(pod) fp := &fakeProvider{provisionID: "inst-1"} // no URL, no token - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -853,7 +943,7 @@ func TestCreatePod_URLWithoutTokenPatchesEndpointOnly(t *testing.T) { client := fake.NewSimpleClientset(pod) fp := &fakeProvider{provisionID: "inst-1", provisionURL: url} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) h.NotifyPods(context.Background(), func(*corev1.Pod) {}) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod: %v", err) @@ -875,7 +965,7 @@ func TestCreatePod_URLWithoutTokenPatchesEndpointOnly(t *testing.T) { // would never be collected — leaking a live credential. Skip instead. func TestCreateConnectSecret_SkipsUIDLessPod(t *testing.T) { client := fake.NewSimpleClientset() - h := NewHandler(&fakeProvider{}, client, nil) + h := NewHandler(&fakeProvider{}, client, nil, openPools()) h.createConnectSecret(context.Background(), testPod("default", "p1"), // no UID "https://sb-9.modal.host", "tok-abc") @@ -893,7 +983,7 @@ func TestCreatePod_NoCredentialPersistedOnProvisionFailure(t *testing.T) { client := fake.NewSimpleClientset(pod) fp := &fakeProvider{provisionErr: errors.New("no capacity")} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) if err := h.CreatePod(context.Background(), pod); err == nil { t.Fatal("expected CreatePod to fail") } @@ -920,7 +1010,7 @@ func TestReconcileOnce_EmptyObservedEndpointDoesNotClearAnnotation(t *testing.T) client := fake.NewSimpleClientset(pod) fp := &fakeProvider{provisionID: "inst-1", provisionURL: url, provisionToken: "tok-abc"} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -964,7 +1054,7 @@ func TestCreatePod_FailedEndpointPatchIsRetriedByPollLoop(t *testing.T) { }) fp := &fakeProvider{provisionID: "inst-1", provisionURL: url, provisionToken: "tok-abc"} - h := NewHandler(fp, client, nil) + h := NewHandler(fp, client, nil, openPools()) h.NotifyPods(context.Background(), func(*corev1.Pod) {}) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod must not fail on a failed endpoint patch: %v", err) @@ -1068,7 +1158,7 @@ func TestCreatePod_ProvisionDeadlineDoesNotLeakIntoCredentialWrite(t *testing.T) // A tiny budget the call is guaranteed to exhaust. capabilities: provider.Capabilities{ProvisionTimeout: time.Millisecond}, } - h := NewHandler(fp, ctxRecordingClient{client, &secretCtx}, nil) + h := NewHandler(fp, ctxRecordingClient{client, &secretCtx}, nil, openPools()) h.NotifyPods(context.Background(), func(*corev1.Pod) {}) if err := h.CreatePod(context.Background(), pod); err != nil { t.Fatalf("CreatePod: %v", err) @@ -1110,7 +1200,7 @@ func TestReconcileOnce_NotifiesOnProvisioningToInitializing(t *testing.T) { // check would swallow this and strand the Pod on the stale "Provisioning" // reason; the reason must move and a notification must fire. fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -1153,7 +1243,7 @@ func TestReconcileOnce_NotifiesOnProvisioningToInitializing(t *testing.T) { func TestReconcileOnce_AbsentInstanceIsTerminated(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -1180,7 +1270,7 @@ func TestReconcileOnce_ListErrorLeavesStatusUntouched(t *testing.T) { // recoverable ClaimName cannot be reported at all (an empty claim and an omitted // sandbox both read as absent here). That choice is only safe because of this. fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) _ = h.CreatePod(context.Background(), testPod("default", "p1")) var mu sync.Mutex @@ -1212,11 +1302,11 @@ func TestReconcileOnce_ListErrorLeavesStatusUntouched(t *testing.T) { func TestNewHandler_PollIntervalFromCapabilities(t *testing.T) { // A provider that declares a cadence overrides the default. custom := &fakeProvider{capabilities: provider.Capabilities{PollInterval: 5 * time.Second}} - if got := NewHandler(custom, nil, nil).pollEvery; got != 5*time.Second { + if got := NewHandler(custom, nil, nil, openPools()).pollEvery; got != 5*time.Second { t.Fatalf("expected the provider's PollInterval, got %v", got) } // A provider that leaves it zero falls back to the vnode default. - if got := NewHandler(&fakeProvider{}, nil, nil).pollEvery; got != defaultPollInterval { + if got := NewHandler(&fakeProvider{}, nil, nil, openPools()).pollEvery; got != defaultPollInterval { t.Fatalf("expected the default cadence, got %v", got) } } @@ -1231,7 +1321,7 @@ func TestGetPod_ReAdoptsLiveInstanceAfterRestart(t *testing.T) { fp := &fakeProvider{list: []provider.Instance{{ ID: "inst-9", ClaimName: "default-p1", State: provider.InstanceRunning, Endpoint: "1.2.3.4", }}} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) got, err := h.GetPod(context.Background(), "default", "p1") if err != nil { @@ -1258,7 +1348,7 @@ func TestGetPod_ReAdoptsLiveInstanceAfterRestart(t *testing.T) { // the non-NotFound error makes VK's delete path requeue instead of terminating. func TestGetPod_ListErrorIsUnknownNotAbsent(t *testing.T) { fp := &fakeProvider{listErr: errors.New("rpc error: code = Unavailable")} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) got, err := h.GetPod(context.Background(), "default", "p1") if err == nil { @@ -1299,7 +1389,7 @@ func TestGetPod_UnknownClaimStaysNotFound(t *testing.T) { fp := &fakeProvider{list: []provider.Instance{{ ID: "inst-1", ClaimName: "default-other", State: provider.InstanceRunning, }}} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) if _, err := h.GetPod(context.Background(), "default", "p1"); !errdefs.IsNotFound(err) { t.Fatalf("expected NotFound for an unknown, unlisted claim, got %v", err) @@ -1308,7 +1398,7 @@ func TestGetPod_UnknownClaimStaysNotFound(t *testing.T) { func TestGetPods_ReturnsTracked(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) _ = h.CreatePod(context.Background(), testPod("default", "p1")) _ = h.CreatePod(context.Background(), testPod("default", "p2")) @@ -1321,55 +1411,99 @@ func TestGetPods_ReturnsTracked(t *testing.T) { } } -// TestCreatePod_ReadsEgressPolicyFromAnnotations covers the second half of the pinning -// decision behind EgressAnnotation: the handler never reads the NodePool, so if it does not -// reconstruct the policy from these annotations the adapter gets nil and provisions a -// workload with open egress under a pool that asked for containment. -func TestCreatePod_ReadsEgressPolicyFromAnnotations(t *testing.T) { +// TestCreatePod_ReadsEgressPolicyFromPool pins where the policy comes FROM. It reverses the +// earlier decision to reconstruct it from the Pod's annotations: they are writable by anyone +// with patch on the Pod, so the object being contained was deciding its own containment. +func TestCreatePod_ReadsEgressPolicyFromPool(t *testing.T) { for _, tc := range []struct { name string - annotations map[string]string + pool *nebulav1alpha1.EgressPolicy wantMode nebulav1alpha1.EgressMode wantTargets []string }{{ - // Absence is Open, which is what every Pod placed by a pool without the field - // carries — so nil here must not be mistaken for a missing value. - name: "no annotation is Open", - annotations: nil, - wantMode: nebulav1alpha1.EgressOpen, + // A pool that never set spec.egress, which is the common case. + name: "unset is Open", + pool: nil, + wantMode: nebulav1alpha1.EgressOpen, }, { - name: "blocked needs no targets", - annotations: map[string]string{nebulav1alpha1.EgressAnnotation: string(nebulav1alpha1.EgressBlocked)}, - wantMode: nebulav1alpha1.EgressBlocked, + name: "blocked needs no targets", + pool: &nebulav1alpha1.EgressPolicy{Mode: nebulav1alpha1.EgressBlocked}, + wantMode: nebulav1alpha1.EgressBlocked, }, { - name: "target entries are split back out", - annotations: map[string]string{ - nebulav1alpha1.EgressAnnotation: string(nebulav1alpha1.EgressAllowlist), - nebulav1alpha1.EgressTargetsAnnotation: "10.0.0.0/8,*.huggingface.co", + name: "allowlist targets reach the provider verbatim", + pool: &nebulav1alpha1.EgressPolicy{ + Mode: nebulav1alpha1.EgressAllowlist, + Targets: []string{"10.0.0.0/8", "*.huggingface.co"}, }, wantMode: nebulav1alpha1.EgressAllowlist, wantTargets: []string{"10.0.0.0/8", "*.huggingface.co"}, - }, { - // The mode is authoritative: a list with no mode cannot narrow anything on its - // own, so it is ignored rather than treated as an implicit Allowlist. - name: "targets without a mode are ignored", - annotations: map[string]string{nebulav1alpha1.EgressTargetsAnnotation: "10.0.0.0/8"}, - wantMode: nebulav1alpha1.EgressOpen, }} { t.Run(tc.name, func(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) - pod := testPod("default", "p1") - pod.Annotations = tc.annotations + pools := testPools(tc.pool) + h := NewHandler(fp, nil, nil, pools) - if err := h.CreatePod(context.Background(), pod); err != nil { + if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } + if pools.calls != 1 { + t.Errorf("pool reads = %d, want 1; the policy must be read from the pool", pools.calls) + } if got := fp.lastReq.Egress.ModeOrOpen(); got != tc.wantMode { t.Errorf("req.Egress mode = %q, want %q", got, tc.wantMode) } if got := fp.lastReq.Egress.GetTargets(); !slices.Equal(got, tc.wantTargets) { - t.Errorf("req.Egress allow = %v, want %v", got, tc.wantTargets) + t.Errorf("req.Egress targets = %v, want %v", got, tc.wantTargets) + } + }) + } +} + +// TestCreatePod_FailsClosedWithoutAPool covers the three ways the trusted state can be +// missing. None may fall back to a default: the pool IS the policy, so an unreadable pool +// means the policy is unknown, and provisioning under an unknown containment policy is the +// failure this whole path exists to prevent. Nothing is provisioned, and the Pod carries the +// reason rather than failing silently. +func TestCreatePod_FailsClosedWithoutAPool(t *testing.T) { + for _, tc := range []struct { + name string + pools PoolReader + pod func() *corev1.Pod + }{{ + // A Pod that never went through placement: a scheduling gate can be removed by + // anyone who can patch the Pod, so reaching a virtual node proves nothing. + name: "no pool label", + pools: openPools(), + pod: func() *corev1.Pod { + pod := testPod("default", "p1") + delete(pod.Labels, nebulav1alpha1.PoolLabel) + return pod + }, + }, { + name: "pool does not exist", + pools: &fakePools{}, + pod: func() *corev1.Pod { return testPod("default", "p1") }, + }, { + name: "no reader wired", + pools: nil, + pod: func() *corev1.Pod { return testPod("default", "p1") }, + }} { + t.Run(tc.name, func(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + h := NewHandler(fp, nil, nil, tc.pools) + pod := tc.pod() + + if err := h.CreatePod(context.Background(), pod); err == nil { + t.Fatal("CreatePod succeeded; an unresolvable egress policy must refuse to provision") + } + if fp.provisionCnt != 0 { + t.Errorf("provision calls = %d, want 0; nothing may be requested", fp.provisionCnt) + } + if pod.Status.Reason != nebulav1alpha1.PodReasonConfigError { + t.Errorf("pod reason = %q, want %q", pod.Status.Reason, nebulav1alpha1.PodReasonConfigError) + } + if _, tracked := h.tracked[key(pod.Namespace, pod.Name)]; tracked { + t.Error("pod is tracked; nothing was provisioned for it") } }) } diff --git a/pkg/vnode/kubelet_test.go b/pkg/vnode/kubelet_test.go index 4f099e1..aebf7a3 100644 --- a/pkg/vnode/kubelet_test.go +++ b/pkg/vnode/kubelet_test.go @@ -100,7 +100,7 @@ func TestKubeletServer_AdvertisesInternalIPOnly(t *testing.T) { // the provider's bytes. func TestKubeletServer_ServesLogsOverTLS(t *testing.T) { lp := newLoggingProvider(&fakeProvider{provisionID: "inst-1"}, "hello over tls\n") - h := NewHandler(lp, nil, nil) + h := NewHandler(lp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -137,7 +137,7 @@ func TestKubeletServer_ServesLogsOverTLS(t *testing.T) { // negotiated, and the command's output and exit code both survive the wire. func TestKubeletServer_ServesExecOverTLS(t *testing.T) { ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, newExecProcess("hi from exec\n", "", 0)) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -165,7 +165,7 @@ func TestKubeletServer_ServesExecOverTLS(t *testing.T) { // failed to the client, with its own status, not like a broken kubelet. func TestKubeletServer_ExecReportsExitCode(t *testing.T) { ep := newExecProvider(&fakeProvider{provisionID: "inst-1"}, newExecProcess("", "boom\n", 3)) - h := NewHandler(ep, nil, nil) + h := NewHandler(ep, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -212,7 +212,7 @@ func execCommand(t *testing.T, base, namespace, pod string, cmd []string) (strin func TestKubeletServer_ResolvesAcrossProviders(t *testing.T) { modalish := newLoggingProvider(&fakeProvider{provisionID: "sb-1"}, "from modal\n") awsish := newLoggingProvider(&fakeProvider{provisionID: "i-1"}, "from aws\n") - hModal, hAWS := NewHandler(modalish, nil, nil), NewHandler(awsish, nil, nil) + hModal, hAWS := NewHandler(modalish, nil, nil, openPools()), NewHandler(awsish, nil, nil, openPools()) if err := hModal.CreatePod(context.Background(), testPod("default", "on-modal")); err != nil { t.Fatalf("CreatePod(modal): %v", err) } @@ -238,7 +238,7 @@ func TestKubeletServer_ResolvesAcrossProviders(t *testing.T) { func TestKubeletServer_ProviderErrorIsNotSwallowed(t *testing.T) { broken := newLoggingProvider(&fakeProvider{provisionID: "inst-1"}, "") broken.logsErr = fmt.Errorf("provider API unreachable") - h := NewHandler(broken, nil, nil) + h := NewHandler(broken, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } diff --git a/pkg/vnode/logs_test.go b/pkg/vnode/logs_test.go index 5ba6ec2..8aa51c0 100644 --- a/pkg/vnode/logs_test.go +++ b/pkg/vnode/logs_test.go @@ -343,7 +343,7 @@ func (p *loggingProvider) Logs(_ context.Context, instanceID string) (io.ReadClo func TestGetContainerLogs_StreamsTrackedPod(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} lp := newLoggingProvider(fp, "hello from the sandbox\n") - h := NewHandler(lp, nil, nil) + h := NewHandler(lp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -370,7 +370,7 @@ func TestGetContainerLogs_StreamsTrackedPod(t *testing.T) { // stream behind it. func TestGetContainerLogs_IgnoresContainerName(t *testing.T) { lp := newLoggingProvider(&fakeProvider{provisionID: "inst-1"}, "out\n") - h := NewHandler(lp, nil, nil) + h := NewHandler(lp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -396,7 +396,7 @@ func TestGetContainerLogs_IgnoresContainerName(t *testing.T) { func TestGetContainerLogs_NotFoundCases(t *testing.T) { // No log support at all — a legitimate configuration, not an internal error. t.Run("provider does not stream logs", func(t *testing.T) { - h := NewHandler(&fakeProvider{provisionID: "inst-1"}, nil, nil) + h := NewHandler(&fakeProvider{provisionID: "inst-1"}, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -406,7 +406,7 @@ func TestGetContainerLogs_NotFoundCases(t *testing.T) { // Another node's pod, or one this process never adopted. t.Run("pod not tracked", func(t *testing.T) { - h := NewHandler(newLoggingProvider(&fakeProvider{}, "x\n"), nil, nil) + h := NewHandler(newLoggingProvider(&fakeProvider{}, "x\n"), nil, nil, openPools()) _, err := h.GetContainerLogs(context.Background(), "default", "ghost", "main", vkapi.ContainerLogOpts{}) assertNotFound(t, err) }) @@ -416,7 +416,7 @@ func TestGetContainerLogs_NotFoundCases(t *testing.T) { t.Run("tracked without an instance", func(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity")} lp := newLoggingProvider(fp, "x\n") - h := NewHandler(lp, nil, nil) + h := NewHandler(lp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { t.Fatal("CreatePod: expected the provision rejection to surface") } @@ -433,7 +433,7 @@ func TestGetContainerLogs_NotFoundCases(t *testing.T) { func TestGetContainerLogs_ProviderErrorIsNotNotFound(t *testing.T) { lp := newLoggingProvider(&fakeProvider{provisionID: "inst-1"}, "") lp.logsErr = errors.New("provider API unreachable") - h := NewHandler(lp, nil, nil) + h := NewHandler(lp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -454,7 +454,7 @@ func TestGetContainerLogs_ProviderErrorIsNotNotFound(t *testing.T) { // otherwise --tail/--limit-bytes would silently do nothing. func TestGetContainerLogs_AppliesOpts(t *testing.T) { lp := newLoggingProvider(&fakeProvider{provisionID: "inst-1"}, "a\nb\nc\n") - h := NewHandler(lp, nil, nil) + h := NewHandler(lp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { t.Fatalf("CreatePod: %v", err) } diff --git a/pkg/vnode/metrics_test.go b/pkg/vnode/metrics_test.go index 0aecd37..b70b95e 100644 --- a/pkg/vnode/metrics_test.go +++ b/pkg/vnode/metrics_test.go @@ -64,7 +64,7 @@ func metricPod(ns, name string) *corev1.Pod { nebulav1alpha1.CapacityTypeAnnotation: string(nebulav1alpha1.CapacitySpot), nebulav1alpha1.RegionAnnotation: "us-east-1", } - pod.Labels = map[string]string{nebulav1alpha1.AcceleratorTypeLabel: "H100"} + pod.Labels[nebulav1alpha1.AcceleratorTypeLabel] = "H100" return pod } @@ -90,7 +90,7 @@ func TestCreatePod_RecordsSuccessfulProvisionAttempt(t *testing.T) { beforeAttempts := testutil.ToFloat64(metrics.ProvisionAttempts.With(success)) beforeDuration := histCount(t, metrics.ProvisionDuration, success) - h := NewHandler(&fakeProvider{provisionID: "inst-1"}, nil, nil) + h := NewHandler(&fakeProvider{provisionID: "inst-1"}, nil, nil, openPools()) if err := h.CreatePod(context.Background(), metricPod("default", "m1")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -112,7 +112,7 @@ func TestCreatePod_RecordsRejectionReason(t *testing.T) { beforeFailures := testutil.ToFloat64(metrics.ProvisionFailures.With(capacity)) fp := &fakeProvider{provisionErr: provider.ErrNoCapacity} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), metricPod("default", "m2")); err == nil { t.Fatal("expected the provision error") } @@ -138,7 +138,7 @@ func TestCreatePod_UnreachableProviderCountedSeparately(t *testing.T) { beforeCapacity := testutil.ToFloat64(metrics.ProvisionFailures.With(capacity)) fp := &fakeProvider{provisionErr: errors.New("rpc error: code = Unavailable desc = transport is closing")} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), metricPod("default", "m3")); err == nil { t.Fatal("expected the provision error") } @@ -163,7 +163,7 @@ func TestReconcileOnce_ObservesReadyDurationExactlyOnce(t *testing.T) { before := histCount(t, metrics.InstanceReadyDuration, ready) fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) if err := h.CreatePod(context.Background(), metricPod("default", "m4")); err != nil { t.Fatalf("CreatePod: %v", err) } @@ -212,7 +212,7 @@ func TestGetPod_ReAdoptedPodIsNotReadyObserved(t *testing.T) { fp := &fakeProvider{list: []provider.Instance{{ ID: "inst-9", ClaimName: "default-m5", State: provider.InstanceRunning, }}} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) if _, err := h.GetPod(context.Background(), "default", "m5"); err != nil { t.Fatalf("GetPod: %v", err) } @@ -234,7 +234,7 @@ func TestObserveReady_IndependentOfPinnedStatusClock(t *testing.T) { before := histCount(t, metrics.InstanceReadyDuration, ready) fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) + h := NewHandler(fp, nil, nil, openPools()) // A status clock pinned far in the PAST: reusing it to measure would go negative. pinned := metav1.NewTime(time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC)) h.nowFn = func() metav1.Time { return pinned } diff --git a/pkg/vnode/node.go b/pkg/vnode/node.go index 0b21d07..cf87c81 100644 --- a/pkg/vnode/node.go +++ b/pkg/vnode/node.go @@ -108,7 +108,9 @@ func NodeName(providerName string) string { // RBAC for the virtual kubelet: the pod controller reports Pod status and reads the // config/secret/service objects a Pod references; the node controller maintains the -// Node, its lease, and events. +// Node, its lease, and events. NodePools are read because pool policy is resolved from the +// pool at provision time rather than from the Pod (see Handler.poolFor). +// +kubebuilder:rbac:groups=nebula.inftyai.com,resources=nodepools,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch;update;patch;delete // +kubebuilder:rbac:groups="",resources=pods/status,verbs=get;update;patch // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch;create;update;patch;delete @@ -129,6 +131,7 @@ type Runner struct { prov provider.Provider client kubernetes.Interface blocklist Blocklister + pools PoolReader nodeName string // kubelet is the shared endpoint serving `kubectl logs` for every node. Nil is @@ -138,14 +141,18 @@ type Runner struct { } // NewRunner builds the virtual-node runner for one provider. blocklist (Provision -// failures) and kubelet (the log endpoint) are both shared, and both may be nil. +// failures) and kubelet (the log endpoint) are both shared, and both may be nil. pools is +// how the handler reads pool policy from the pool instead of from the Pod, so a nil one +// leaves this node unable to provision at all (see Handler.poolFor). func NewRunner( - prov provider.Provider, client kubernetes.Interface, blocklist Blocklister, kubelet *KubeletServer, + prov provider.Provider, client kubernetes.Interface, blocklist Blocklister, + kubelet *KubeletServer, pools PoolReader, ) *Runner { return &Runner{ prov: prov, client: client, blocklist: blocklist, + pools: pools, nodeName: NodeName(prov.Name()), kubelet: kubelet, } @@ -158,7 +165,7 @@ var _ manager.Runnable = (*Runner)(nil) func (r *Runner) Start(ctx context.Context) error { log := logf.FromContext(ctx).WithValues("virtualNode", r.nodeName, "provider", r.prov.Name()) - handler := NewHandler(r.prov, r.client, r.blocklist) + handler := NewHandler(r.prov, r.client, r.blocklist, r.pools) nodeSpec := nodeSpec(r.nodeName, r.prov.Name()) // Register on the endpoint AND advertise it, so the API server can proxy `kubectl diff --git a/pkg/vnode/pool.go b/pkg/vnode/pool.go new file mode 100644 index 0000000..42f2ccc --- /dev/null +++ b/pkg/vnode/pool.go @@ -0,0 +1,99 @@ +/* +Copyright 2026 The InftyAI Team. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package vnode + +import ( + "context" + "fmt" + "time" + + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" +) + +// PoolReader reads the NodePool a Pod is placed against. It exists so the handler can +// resolve pool POLICY from the pool itself, instead of trusting a copy of it carried on +// the Pod (see Handler.poolFor). +// +// Narrower than client.Reader — one method, one type, no options — because that is the +// whole dependency: a fake in a test is three lines, and no test needs a scheme. +type PoolReader interface { + Get(ctx context.Context, name string) (*nebulav1alpha1.NodePool, error) +} + +// cachedPoolReader adapts the manager's client. NodePool is cluster-scoped, so the name +// is the whole key. +type cachedPoolReader struct{ reader client.Reader } + +// NewCachedPoolReader wraps a controller-runtime reader as a PoolReader. Pass the +// manager's client: its cache is already synced before runnables start, and the pool +// informer is shared with the controllers that watch NodePools, so the read on the +// provisioning path costs no API call. +func NewCachedPoolReader(reader client.Reader) PoolReader { + return &cachedPoolReader{reader: reader} +} + +func (c *cachedPoolReader) Get(ctx context.Context, name string) (*nebulav1alpha1.NodePool, error) { + var pool nebulav1alpha1.NodePool + if err := c.reader.Get(ctx, client.ObjectKey{Name: name}, &pool); err != nil { + return nil, err + } + return &pool, nil +} + +// poolFor resolves the NodePool a Pod is placed against — the trusted source for every +// policy the handler needs to provision (the egress policy, the failover TTL), none of which +// may come off the Pod, where the workload's own owner can patch it. +// +// FAIL-CLOSED: no pool means no policy, and provisioning under an unknown policy is the +// failure this path exists to prevent. Callers treat the error as non-terminal, so a pool +// not yet in cache costs a retry rather than an unrestricted instance. +// +// Read ONCE per provision and passed down, so the egress policy applied and the TTL of any +// resulting block come from the same observation of the pool. The returned pool is shared +// informer state — read it, never mutate it. +func (h *Handler) poolFor(ctx context.Context, pod *corev1.Pod) (*nebulav1alpha1.NodePool, error) { + name := pod.Labels[nebulav1alpha1.PoolLabel] + if name == "" { + return nil, fmt.Errorf("no %s label on the Pod, so its pool policy cannot be established", + nebulav1alpha1.PoolLabel) + } + if h.pools == nil { + // Wiring bug, not a user error: every production handler gets a reader (see + // NewRunner). Refusing keeps it a loud, immediate failure instead of a fleet that + // silently provisions unrestricted. + return nil, fmt.Errorf("no NodePool reader configured, cannot establish the policy for pool %q", name) + } + pool, err := h.pools.Get(ctx, name) + if err != nil { + return nil, fmt.Errorf("read NodePool %q for its policy: %w", name, err) + } + return pool, nil +} + +// blocklistTTLOf is the base exclusion a failed placement gets, from the pool's own +// FailoverPolicy. A non-positive TTL reads as unset, because zero would install a permanent +// block. Pure, and driven by the pool poolFor already returned, so the failure path never +// re-reads (and never has to decide what an unreadable pool means for a block). +func blocklistTTLOf(pool *nebulav1alpha1.NodePool) time.Duration { + if pool == nil || pool.Spec.Failover == nil || pool.Spec.Failover.BlocklistTTL.Duration <= 0 { + return defaultBlocklistTTL + } + return pool.Spec.Failover.BlocklistTTL.Duration +}