diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index eff97d4..0cf5b54 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -193,6 +193,12 @@ const ( PodReasonRunning = "Running" // PodReasonProvisionFailed: the provider rejected or failed the Provision call. PodReasonProvisionFailed = "ProvisionFailed" + // PodReasonConfigError: the Pod references something unreadable — a missing Secret or + // ConfigMap behind an env var, or a downward-API field this node cannot answer — so + // nothing was requested from the provider. The kubelet's CreateContainerConfigError, + // and non-terminal for the same reason: the reference usually appears moments later, and + // waiting is free while nothing exists to bill. + PodReasonConfigError = "ConfigError" // PodReasonFailed: the provider reports the instance in a failed state. PodReasonFailed = "Failed" // PodReasonTerminated: the instance is gone from the provider (torn down, diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index 629e32c..3288a7f 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -153,7 +153,9 @@ type InstanceSpec struct { // Args is the Pod container's args, appended after the entrypoint just as CMD // arguments. Empty means "use the image's own CMD". Args []string - // Env is the environment, flattened from the Pod's container env. + // Env is the environment, taken whole from provider.ProvisionRequest.Env: literals plus + // everything envFrom/valueFrom referenced, already resolved by the caller. See where it is + // set for the user-data exposure this implies. Env map[string]string // Spot requests interruptible capacity when true (OnDemand otherwise). Spot bool @@ -749,15 +751,6 @@ func (p *Provider) instanceSpecFromPod( } c := pod.Spec.Containers[0] - env := make(map[string]string, len(c.Env)) - for _, e := range c.Env { - // ValueFrom (secrets/configmaps) is not resolved here; the real Client - // wiring must project those. Plain values are copied through. - if e.ValueFrom == nil { - env[e.Name] = e.Value - } - } - // Accelerator type comes from the AcceleratorTypeLabel; the count rides on the // nvidia.com/gpu resource. On EC2 both are lookup keys: the instance type is the // one whose (accelerator_type, gpu_count) pair matches, since the GPU count is @@ -784,10 +777,22 @@ func (p *Provider) instanceSpecFromPod( Image: c.Image, Command: append([]string{}, c.Command...), Args: append([]string{}, c.Args...), - Env: env, - Spot: req.CapacityType == nebulav1alpha1.CapacitySpot, - Region: req.Region, - Tags: map[string]string{ClaimTagKey: req.ClaimName}, + // The caller's resolved environment, whole (provider.ProvisionRequest.Env). The Pod's + // own env is not read: it holds references this adapter cannot follow. + // + // CAVEAT, and the reason this is called out: buildUserData renders env into cloud-init + // user-data, which EC2 stores unencrypted and serves through IMDS and + // DescribeInstanceAttribute. A value resolved from a Secret therefore lands somewhere + // readable by anything on the instance and by any principal holding that IAM + // permission. Acceptable while nothing sensitive rides on it; not a place to put a + // long-lived credential. + // TODO: deliver Secret-derived values out-of-band — SSM Parameter Store / Secrets + // Manager under the claim, fetched at boot with the instance profile — and keep only + // non-sensitive values in user-data. + Env: req.Env, + Spot: req.CapacityType == nebulav1alpha1.CapacitySpot, + Region: req.Region, + Tags: map[string]string{ClaimTagKey: req.ClaimName}, }, nil } diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index 7443ecf..cb191a1 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -162,6 +162,39 @@ func gpuPod(accel string, count int64) *corev1.Pod { return pod } +// TestProvision_UsesResolvedEnv pins the env contract: the request's resolved map is the whole +// environment, and the Pod's own env is not read — it holds references this adapter cannot +// follow, so a caller that does not resolve gets nothing rather than half a workload. +func TestProvision_UsesResolvedEnv(t *testing.T) { + f := &fakeClient{runID: "i-env"} + p := newTestProvider(f) + pod := gpuPod("H100", 8) // carries FOO=bar literally + + if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ + ClaimName: "claim-env", + Region: "us-west-2", // EC2 needs one; the test client is not configured with a default + Env: map[string]string{"FOO": "bar", "TOKEN": "t0ken"}, + }); err != nil { + t.Fatalf("Provision: %v", err) + } + if got := f.lastSpec.Env; got["FOO"] != "bar" || got["TOKEN"] != "t0ken" || len(got) != 2 { + t.Fatalf("spec env = %v, want FOO=bar TOKEN=t0ken", got) + } + + // No resolved map: nothing is set, even though the Pod carries FOO=bar literally. + f = &fakeClient{runID: "i-env-2"} + p = newTestProvider(f) + if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ + ClaimName: "claim-env", + Region: "us-west-2", + }); err != nil { + t.Fatalf("Provision: %v", err) + } + if got := f.lastSpec.Env; len(got) != 0 { + t.Fatalf("spec env = %v, want empty: the Pod's env is not a source here", got) + } +} + func TestProvision_MapsAcceleratorToInstanceType(t *testing.T) { f := &fakeClient{runID: "i-1"} p := newTestProvider(f) diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 48c279b..dbcbae6 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -124,7 +124,11 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string } sb, err := c.mc.Sandboxes.Create(ctx, app, image, &modal.SandboxCreateParams{ - Command: spec.Command, + Command: spec.Command, + // Env is the whole environment, including values resolved from this cluster's + // Secrets (see provider.ProvisionRequest.Env). No Secrets field alongside it: the + // SDK hydrates this map into an ephemeral server-side Modal Secret before the + // create (mergeEnvIntoSecrets), so nothing named is left in the workspace. Env: spec.Env, GPU: gpuReservation(spec.GPU, spec.GPUCount), CPU: spec.CPU, diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 715548e..1ef4055 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -110,7 +110,11 @@ type SandboxSpec struct { Image string // Command is the container command+args, from the Pod. Command []string - // Env is the environment, flattened from the Pod's container env. + // Env is the environment, taken whole from provider.ProvisionRequest.Env: literals plus + // everything envFrom/valueFrom referenced, already resolved by the caller. + // + // SECRET-BEARING, hence the redacting String below. Needs no Modal Secret of its own — + // the SDK hydrates it into an ephemeral server-side one (mergeEnvIntoSecrets). Env map[string]string // GPU is Modal's accelerator identifier (e.g. "H100", "A100-80GB"), or "" // for a CPU-only sandbox. @@ -160,6 +164,19 @@ type SandboxSpec struct { ReadinessProbe *corev1.Probe } +// String redacts Env so a spec can be logged or wrapped in an error safely: key names print +// (they are in the Pod spec already), values never do. The probe renders as set/unset — it is +// a pointer, so %v would print an address, and only its presence matters. +func (s SandboxSpec) String() string { + return fmt.Sprintf("SandboxSpec{Image:%s Command:%v Env:%s GPU:%s GPUCount:%d CPU:%g "+ + "MemoryMiB:%d Ports:%v Regions:%v Timeout:%s Tags:%v ReadinessProbe:%t}", + s.Image, s.Command, provider.RedactedEnv(s.Env), s.GPU, s.GPUCount, s.CPU, + s.MemoryMiB, s.Ports, s.Regions, s.Timeout, s.Tags, s.ReadinessProbe != nil) +} + +// GoString implements fmt.GoStringer so %#v is redacted too. +func (s SandboxSpec) GoString() string { return s.String() } + // Sandbox is the adapter-level view of a Modal sandbox as observed. // // No endpoint here, deliberately: Modal's reachable address is the connect URL, minted at @@ -456,15 +473,6 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq } c := pod.Spec.Containers[0] - env := make(map[string]string, len(c.Env)) - for _, e := range c.Env { - // ValueFrom (secrets/configmaps) is not resolved here; the real Client - // wiring must project those. Plain values are copied through. - if e.ValueFrom == nil { - env[e.Name] = e.Value - } - } - tags := map[string]string{ClaimTagKey: req.ClaimName} // Record probe-ness alongside identity so observe can recover it later; see // ProbeTagKey for why this cannot be re-derived at observation time. The tag @@ -477,9 +485,13 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq } spec := SandboxSpec{ - Image: c.Image, - Command: append(append([]string{}, c.Command...), c.Args...), - Env: env, + Image: c.Image, + Command: append(append([]string{}, c.Command...), c.Args...), + // The caller's resolved map is the whole environment — the Pod's literals plus + // everything envFrom/valueFrom referenced. pod.Spec.Containers[0].Env is NOT read + // here: it holds references this adapter has no cluster access to follow. See + // provider.ProvisionRequest.Env. + Env: req.Env, CPU: cpuCores(&c), MemoryMiB: memoryMiB(&c), Ports: containerPorts(&c), diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 36d8fc0..0520a77 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -1299,3 +1299,54 @@ func TestMergeStreams_TeardownIsNotAFailure(t *testing.T) { } _ = rc.Close() } + +// TestProvision_UsesResolvedEnv pins the env contract here: the request's resolved map is the +// whole environment, and the Pod's own env is not read — it holds references this adapter +// cannot follow, so a caller that does not resolve gets nothing rather than half a workload. +func TestProvision_UsesResolvedEnv(t *testing.T) { + f := &fakeClient{createID: "sb-env"} + p := newTestProvider(f) + pod := gpuPod("claim-env", "H100", 1) // carries FOO=bar literally + pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, + corev1.EnvVar{Name: "TOKEN", ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}, Key: "K"}, + }}) + + if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ + ClaimName: "claim-env", + Env: map[string]string{"FOO": "bar", "TOKEN": "t0ken"}, + }); err != nil { + t.Fatalf("Provision: %v", err) + } + if got := f.lastSpec.Env; got["FOO"] != "bar" || got["TOKEN"] != "t0ken" || len(got) != 2 { + t.Fatalf("spec env = %v, want FOO=bar TOKEN=t0ken", got) + } + + // No resolved map: nothing is set, even though the Pod carries FOO=bar literally. A fresh + // client, because Provision is idempotent on ClaimName and would otherwise adopt the + // sandbox above instead of creating one. + f = &fakeClient{createID: "sb-env-2"} + p = newTestProvider(f) + if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ + ClaimName: "claim-env", + }); err != nil { + t.Fatalf("Provision: %v", err) + } + if got := f.lastSpec.Env; len(got) != 0 { + t.Fatalf("spec env = %v, want empty: the Pod's env is not a source here", got) + } +} + +// TestSandboxSpec_StringRedactsEnv is the guard on the leak: the spec now carries Secret +// values, so any %v of it — a log line, an error wrap — must print key names only. +func TestSandboxSpec_StringRedactsEnv(t *testing.T) { + spec := SandboxSpec{Image: "img", Env: map[string]string{"TOKEN": "t0ken", "FOO": "bar"}} + got := fmt.Sprintf("%v %s %#v", spec, spec, spec) + if strings.Contains(got, "t0ken") || strings.Contains(got, "bar") { + t.Fatalf("spec rendering leaked a value: %s", got) + } + if !strings.Contains(got, "TOKEN") || !strings.Contains(got, "FOO") { + t.Fatalf("expected key names to survive redaction: %s", got) + } +} diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 1efe911..1378dcd 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -22,6 +22,8 @@ import ( "context" "fmt" "io" + "sort" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -208,10 +210,10 @@ type Process interface { Close() error } -// ProvisionRequest carries only the placement decisions that are NOT already on the Pod. -// Everything about the workload — image, command, env, ports, cpu/memory, accelerator type -// and count — is read from the Pod, the single source of truth. That leaves the tier, the -// region, and the claim identity. +// ProvisionRequest carries what a provider cannot read off the Pod: the placement decisions +// (tier, region), the claim identity, and the one part of the workload whose value is not in +// the Pod — env behind a reference. Everything else — image, command, ports, cpu/memory, +// accelerator type and count — comes from the Pod, the single source of truth. type ProvisionRequest struct { // ClaimName is the NodeClaim name; providers without native tags encode it // into the instance name so List/Terminate can find the instance later. @@ -229,8 +231,32 @@ type ProvisionRequest struct { // regions leaves it empty, which on Modal is the widest and cheapest option (pinning // costs 1.5-1.75x). AWS cannot honour it, but its ExpandRegions never produces it. Region string + // Env is the container's environment, fully RESOLVED: literals plus everything + // envFrom/valueFrom referenced, merged in kubelet precedence (envFrom in listed order, + // then env overriding it). A provider forwards it and never re-reads the Pod's env, + // which holds references it cannot follow. + // + // Resolved by the caller (pkg/vnode): reading a Secret needs cluster access an adapter + // deliberately lacks, and kubelet precedence should be implemented once. + // + // SECRET-BEARING, and why this struct redacts itself — a secretKeyRef's value lands here + // in the clear. Never log the map, never put it on the Pod, never in an error string, + // the same rule as ProvisionResult.ConnectToken. Nil is normal: no env, or an + // unresolving caller. + Env map[string]string } +// String redacts Env so a ProvisionRequest can be logged safely — nothing stops a future +// log.Info("...", "req", req). Key names print, since they are in the Pod spec already and +// are what makes a "wrong env" report actionable; only values are withheld. +func (r ProvisionRequest) String() string { + return fmt.Sprintf("ProvisionRequest{ClaimName:%s CapacityType:%s Region:%s Env:%s}", + r.ClaimName, r.CapacityType, r.Region, RedactedEnv(r.Env)) +} + +// GoString implements fmt.GoStringer so %#v is redacted too. +func (r ProvisionRequest) GoString() string { return r.String() } + // ProvisionResult is what one Provision call produced. A struct rather than more // positional returns because the credential below is delivered ONCE, and a value that can // only be observed here deserves a name. @@ -323,6 +349,22 @@ func redacted(s string) string { return "[REDACTED]" } +// RedactedEnv renders an env map's shape — size and key names, sorted so two logs of the +// same map read alike — with every value withheld: keys are in the Pod spec, values may +// come from a Secret. Exported because every adapter carrying a resolved environment needs +// this in its own String(), and a second implementation is a second chance to leak. +func RedactedEnv(m map[string]string) string { + if len(m) == 0 { + return "{}" + } + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return fmt.Sprintf("{%d keys: %s}", len(keys), strings.Join(keys, ",")) +} + // InstanceState is the provider-agnostic lifecycle state, normalized from each // provider's own status strings. type InstanceState string diff --git a/pkg/vnode/env.go b/pkg/vnode/env.go new file mode 100644 index 0000000..5f0b711 --- /dev/null +++ b/pkg/vnode/env.go @@ -0,0 +1,308 @@ +/* +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" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/client-go/kubernetes" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +// resolveEnv builds the environment a provider gets as ProvisionRequest.Env, following the +// references the Pod makes. The only place envFrom/valueFrom are read, since a provider has +// no cluster access to follow them with. +// +// Kubelet rules: envFrom first in listed order with each Prefix, then env overriding it. +// optional means "the object or key may not exist" — NOT "we may fail to look", so a read +// failure is an error either way. A required miss is an error too, never a silent omission: +// booting a GPU without its credentials bills for a workload that cannot run. +// +// Two divergences, both because there is no machine here: resourceFieldRef and status.* +// fieldRefs are refused (see fieldRefValue), and $(VAR) is not expanded — values pass +// through verbatim. +// TODO: expand $(VAR) here if a workload needs it; the provider only sees the resolved map. +// +// Reads bypass node.go's Secret/ConfigMap informers on purpose: VK waits only for the POD +// informer to sync before CreatePod, and a cold cache reports an existing Secret as absent — +// the one signal this function acts on. A nil client is the usual test seam (literals only). +func resolveEnv(ctx context.Context, client kubernetes.Interface, pod *corev1.Pod) (map[string]string, error) { + if len(pod.Spec.Containers) == 0 { + return nil, nil + } + // One container per Nebula Pod, as everywhere else that reads the workload. + c := pod.Spec.Containers[0] + if len(c.Env) == 0 && len(c.EnvFrom) == 0 { + return nil, nil + } + + r := &envResolver{client: client, pod: pod} + out := make(map[string]string, len(c.Env)) + if err := r.fromSources(ctx, out, c.EnvFrom); err != nil { + return nil, err + } + if err := r.fromVars(ctx, out, c.Env); err != nil { + return nil, err + } + return out, nil +} + +// envResolver resolves one container's references, memoizing what it reads so ten +// secretKeyRefs into one Secret cost one GET. Misses are memoized too (a nil entry). It +// lives for a single resolveEnv call, so nothing here can go stale. +type envResolver struct { + client kubernetes.Interface + pod *corev1.Pod + secrets map[string]*corev1.Secret + configs map[string]*corev1.ConfigMap +} + +// fromSources applies envFrom in listed order. Every key of each source becomes a variable, +// with the source's Prefix prepended. +func (r *envResolver) fromSources(ctx context.Context, out map[string]string, sources []corev1.EnvFromSource) error { + for _, src := range sources { + switch { + case src.SecretRef != nil: + s, err := r.secret(ctx, src.SecretRef.Name, optional(src.SecretRef.Optional)) + if err != nil { + return err + } + if s == nil { + continue // optional and absent + } + // Data is the whole content: the API server folds write-only StringData into it. + for k, v := range s.Data { + r.put(ctx, out, src.Prefix+k, string(v), "Secret", src.SecretRef.Name) + } + case src.ConfigMapRef != nil: + cm, err := r.configMap(ctx, src.ConfigMapRef.Name, optional(src.ConfigMapRef.Optional)) + if err != nil { + return err + } + if cm == nil { + continue // optional and absent + } + // Data only, as in the kubelet: BinaryData is not text and has no env rendering. + for k, v := range cm.Data { + r.put(ctx, out, src.Prefix+k, v, "ConfigMap", src.ConfigMapRef.Name) + } + } + } + return nil +} + +// fromVars applies the explicit env list, overriding anything envFrom contributed. +func (r *envResolver) fromVars(ctx context.Context, out map[string]string, vars []corev1.EnvVar) error { + for _, e := range vars { + if e.ValueFrom == nil { + out[e.Name] = e.Value + continue + } + v, found, err := r.valueFrom(ctx, e.ValueFrom) + if err != nil { + return fmt.Errorf("env %q: %w", e.Name, err) + } + // Not found means optional-and-absent (a required miss errored), so leave the + // variable unset, as the kubelet does. + if found { + out[e.Name] = v + } + } + return nil +} + +// valueFrom resolves one env[].valueFrom. found=false means the reference was optional and +// its object or key is absent; an err means it was required, or the source is one this node +// cannot answer. +func (r *envResolver) valueFrom(ctx context.Context, src *corev1.EnvVarSource) (string, bool, error) { + switch { + case src.SecretKeyRef != nil: + ref := src.SecretKeyRef + opt := optional(ref.Optional) + s, err := r.secret(ctx, ref.Name, opt) + if err != nil || s == nil { + return "", false, err + } + v, ok := s.Data[ref.Key] + if !ok { + // Object present, key absent: its own message, because the fix differs. + if opt { + return "", false, nil + } + return "", false, fmt.Errorf("key %q not in Secret %q", ref.Key, ref.Name) + } + return string(v), true, nil + + case src.ConfigMapKeyRef != nil: + ref := src.ConfigMapKeyRef + opt := optional(ref.Optional) + cm, err := r.configMap(ctx, ref.Name, opt) + if err != nil || cm == nil { + return "", false, err + } + v, ok := cm.Data[ref.Key] + if !ok { + if opt { + return "", false, nil + } + return "", false, fmt.Errorf("key %q not in ConfigMap %q", ref.Key, ref.Name) + } + return v, true, nil + + case src.FieldRef != nil: + v, err := fieldRefValue(r.pod, src.FieldRef) + return v, err == nil, err + + case src.ResourceFieldRef != nil: + // The kubelet defaults an unset limit to the node's allocatable, and this node + // advertises a synthetic 1k CPU / 10Ti (virtualCapacity) — so a plausible wrong + // number would end up in the workload's own sizing (GOMEMLIMIT) unflagged. + // TODO: answer from the container's explicit requests/limits if a workload needs it. + return "", false, fmt.Errorf("resourceFieldRef %q is not supported on a virtual node", + src.ResourceFieldRef.Resource) + } + return "", false, fmt.Errorf("valueFrom has no recognized source") +} + +// fieldRefValue answers a downward-API fieldRef from the Pod: metadata, plus the two spec +// fields a virtual node knows. status.* is refused — there is no machine here, and a workload +// advertising a placeholder address fails looking like a network problem, not a config one. +func fieldRefValue(pod *corev1.Pod, ref *corev1.ObjectFieldSelector) (string, error) { + // v1 is the only schema these paths are defined against; anything else is a Pod written + // against an API this code does not implement. + if ref.APIVersion != "" && ref.APIVersion != "v1" { + return "", fmt.Errorf("fieldRef apiVersion %q is not supported", ref.APIVersion) + } + switch path := ref.FieldPath; path { + case "metadata.name": + return pod.Name, nil + case "metadata.namespace": + return pod.Namespace, nil + case "metadata.uid": + return string(pod.UID), nil + case "spec.nodeName": + // True and useful: the Pod really is bound to this virtual node. + return pod.Spec.NodeName, nil + case "spec.serviceAccountName": + return pod.Spec.ServiceAccountName, nil + default: + if k, ok := subscript(path, "metadata.labels"); ok { + return pod.Labels[k], nil + } + if k, ok := subscript(path, "metadata.annotations"); ok { + return pod.Annotations[k], nil + } + return "", fmt.Errorf("fieldRef %q is not supported on a virtual node", path) + } +} + +// subscript parses the metadata.labels['key'] form. A bare metadata.labels (the whole map) +// is volume-only in the downward API, so it yields ok=false as it would on a real kubelet. +func subscript(path, prefix string) (string, bool) { + rest, ok := strings.CutPrefix(path, prefix+"['") + if !ok { + return "", false + } + return strings.CutSuffix(rest, "']") +} + +// secret reads one Secret from the Pod's namespace, memoized. (nil, nil) means optional and +// absent; a NotFound on a required ref is an error, as is any other read failure whether or +// not the ref is optional. +func (r *envResolver) secret(ctx context.Context, name string, opt bool) (*corev1.Secret, error) { + s, memoized := r.secrets[name] + if !memoized { + if r.client == nil { + return nil, nil // test seam: no cluster to read from + } + got, err := r.client.CoreV1().Secrets(r.pod.Namespace).Get(ctx, name, metav1.GetOptions{}) + switch { + case apierrors.IsNotFound(err): + s = nil + case err != nil: + // Not memoized: a transport failure says nothing about the object. + return nil, fmt.Errorf("read Secret %q: %w", name, err) + default: + s = got + } + if r.secrets == nil { + r.secrets = map[string]*corev1.Secret{} + } + r.secrets[name] = s + } + if s == nil && !opt { + return nil, r.absentErr("Secret", name) + } + return s, nil +} + +// configMap is the ConfigMap half of secret, with the same contract. +func (r *envResolver) configMap(ctx context.Context, name string, opt bool) (*corev1.ConfigMap, error) { + cm, memoized := r.configs[name] + if !memoized { + if r.client == nil { + return nil, nil // test seam: no cluster to read from + } + got, err := r.client.CoreV1().ConfigMaps(r.pod.Namespace).Get(ctx, name, metav1.GetOptions{}) + switch { + case apierrors.IsNotFound(err): + cm = nil + case err != nil: + return nil, fmt.Errorf("read ConfigMap %q: %w", name, err) + default: + cm = got + } + if r.configs == nil { + r.configs = map[string]*corev1.ConfigMap{} + } + r.configs[name] = cm + } + if cm == nil && !opt { + return nil, r.absentErr("ConfigMap", name) + } + return cm, nil +} + +// absentErr is the error for a required reference whose object does not exist. +func (r *envResolver) absentErr(kind, name string) error { + return fmt.Errorf("%s %q not found in namespace %q", kind, name, r.pod.Namespace) +} + +// put writes one envFrom-derived variable, dropping a name env cannot carry. +// +// Dropped rather than fatal, as the kubelet does (InvalidEnvironmentVariableNames, container +// still starts): a ConfigMap consumed wholesale carries whatever keys it was written with. +// Logged, because this is the one place a variable disappears without an error. The bar is +// Kubernetes' own, laxer than C_IDENTIFIER — "app.conf" passes, a leading digit does not. +func (r *envResolver) put(ctx context.Context, out map[string]string, name, value, kind, source string) { + if errs := validation.IsEnvVarName(name); len(errs) > 0 { + logf.FromContext(ctx).WithName("vnode-env").Info("dropping environment variable with an illegal name", + "pod", key(r.pod.Namespace, r.pod.Name), "name", name, + "source", kind+"/"+source, "reason", strings.Join(errs, "; ")) + return + } + out[name] = value +} + +// optional dereferences the *bool the API uses for optional, where nil means false. +func optional(b *bool) bool { return b != nil && *b } diff --git a/pkg/vnode/env_test.go b/pkg/vnode/env_test.go new file mode 100644 index 0000000..2ef9f94 --- /dev/null +++ b/pkg/vnode/env_test.go @@ -0,0 +1,425 @@ +/* +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" + "errors" + "maps" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" +) + +// envPod is a Pod whose single container carries the given env sources. +func envPod(envFrom []corev1.EnvFromSource, env []corev1.EnvVar) *corev1.Pod { + pod := testPod("default", "p1") + pod.Spec.Containers[0].EnvFrom = envFrom + pod.Spec.Containers[0].Env = env + return pod +} + +func secretObj(name string, data map[string]string) *corev1.Secret { + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: name}, + Data: map[string][]byte{}, + } + for k, v := range data { + s.Data[k] = []byte(v) + } + return s +} + +func configMapObj(name string, data map[string]string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: name}, + Data: data, + } +} + +func ptrBool(b bool) *bool { return &b } + +// TestResolveEnv_Precedence pins the kubelet's ordering: envFrom in listed order, each later +// source overwriting the earlier, then the explicit env list beating all of them. Getting it +// backwards looks like a working deployment until a value comes from the wrong place. +func TestResolveEnv_Precedence(t *testing.T) { + client := fake.NewSimpleClientset( + configMapObj("cm", map[string]string{"SHARED": "from-cm", "ONLY_CM": "cm"}), + secretObj("sec", map[string]string{"SHARED": "from-secret", "ONLY_SECRET": "sec"}), + ) + pod := envPod( + []corev1.EnvFromSource{ + {ConfigMapRef: &corev1.ConfigMapEnvSource{LocalObjectReference: corev1.LocalObjectReference{Name: "cm"}}}, + {SecretRef: &corev1.SecretEnvSource{LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}}}, + }, + []corev1.EnvVar{{Name: "SHARED", Value: "explicit"}, {Name: "PLAIN", Value: "p"}}, + ) + + got, err := resolveEnv(context.Background(), client, pod) + if err != nil { + t.Fatalf("resolveEnv: %v", err) + } + want := map[string]string{ + "SHARED": "explicit", // env beats both envFrom sources + "ONLY_CM": "cm", // envFrom configMapRef + "ONLY_SECRET": "sec", // envFrom secretRef + "PLAIN": "p", // literal + } + if !maps.Equal(got, want) { + t.Fatalf("env mismatch\n got: %v\nwant: %v", got, want) + } +} + +// TestResolveEnv_Prefix checks each source's Prefix is prepended, which is the only way two +// ConfigMaps with the same keys can both be consumed. +func TestResolveEnv_Prefix(t *testing.T) { + client := fake.NewSimpleClientset(configMapObj("cm", map[string]string{"KEY": "v"})) + pod := envPod([]corev1.EnvFromSource{{ + Prefix: "APP_", + ConfigMapRef: &corev1.ConfigMapEnvSource{LocalObjectReference: corev1.LocalObjectReference{Name: "cm"}}, + }}, nil) + + got, err := resolveEnv(context.Background(), client, pod) + if err != nil { + t.Fatalf("resolveEnv: %v", err) + } + if got["APP_KEY"] != "v" || len(got) != 1 { + t.Fatalf("expected only APP_KEY=v, got %v", got) + } +} + +// TestResolveEnv_SecretKeyRef covers the whole secretKeyRef matrix, since these four cases +// are what separate "wait for the Secret" from "boot a GPU without its token". +func TestResolveEnv_SecretKeyRef(t *testing.T) { + client := fake.NewSimpleClientset(secretObj("sec", map[string]string{"TOKEN": "t0ken"})) + ref := func(name, k string, optional bool) []corev1.EnvVar { + return []corev1.EnvVar{{Name: "TOKEN", ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: name}, + Key: k, + Optional: ptrBool(optional), + }, + }}} + } + + cases := []struct { + name string + env []corev1.EnvVar + want string // "" means the variable must be unset + wantErr string // substring; "" means no error + }{ + {name: "resolved", env: ref("sec", "TOKEN", false), want: "t0ken"}, + {name: "missing secret, required", env: ref("nope", "TOKEN", false), wantErr: `Secret "nope" not found`}, + {name: "missing secret, optional", env: ref("nope", "TOKEN", true)}, + {name: "missing key, required", env: ref("sec", "OTHER", false), wantErr: `key "OTHER" not in Secret "sec"`}, + {name: "missing key, optional", env: ref("sec", "OTHER", true)}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveEnv(context.Background(), client, envPod(nil, tc.env)) + switch { + case tc.wantErr != "": + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected error containing %q, got %v", tc.wantErr, err) + } + // A failed resolve must yield NOTHING: a partial map handed to a provider + // would boot an instance with half its configuration. + if got != nil { + t.Fatalf("expected no env on error, got %v", got) + } + case err != nil: + t.Fatalf("resolveEnv: %v", err) + case got["TOKEN"] != tc.want: + t.Fatalf("TOKEN = %q, want %q", got["TOKEN"], tc.want) + } + }) + } +} + +// TestResolveEnv_ReadFailureIsNeverOptional pins what `optional` does NOT license: it says +// the object may not exist, not that we may fail to look. A transport failure must fail the +// resolve even for an optional ref, or an API outage would silently strip credentials. +func TestResolveEnv_ReadFailureIsNeverOptional(t *testing.T) { + client := fake.NewSimpleClientset() + client.PrependReactor("get", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("apiserver is having a day") + }) + pod := envPod([]corev1.EnvFromSource{{ + SecretRef: &corev1.SecretEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}, + Optional: ptrBool(true), + }, + }}, nil) + + if _, err := resolveEnv(context.Background(), client, pod); err == nil { + t.Fatal("expected a read failure to fail the resolve even though the ref is optional") + } +} + +// TestResolveEnv_MemoizesReads asserts a Pod referencing one Secret many times costs one GET: +// at fleet scale that is one read per Pod instead of one per variable. +func TestResolveEnv_MemoizesReads(t *testing.T) { + client := fake.NewSimpleClientset(secretObj("sec", map[string]string{"A": "1", "B": "2"})) + gets := 0 + client.PrependReactor("get", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + gets++ + return false, nil, nil // fall through to the tracker + }) + + keyRef := func(varName, k string) corev1.EnvVar { + return corev1.EnvVar{Name: varName, ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}, Key: k, + }, + }} + } + pod := envPod( + []corev1.EnvFromSource{{SecretRef: &corev1.SecretEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}}}}, + []corev1.EnvVar{keyRef("X", "A"), keyRef("Y", "B")}, + ) + + got, err := resolveEnv(context.Background(), client, pod) + if err != nil { + t.Fatalf("resolveEnv: %v", err) + } + if gets != 1 { + t.Fatalf("expected 1 Secret GET for 3 references, got %d", gets) + } + if got["X"] != "1" || got["Y"] != "2" || got["A"] != "1" { + t.Fatalf("unexpected env: %v", got) + } +} + +// TestResolveEnv_MemoizesMisses is the same guarantee for an absent object: several optional +// references to one missing Secret must not re-ask per variable. +func TestResolveEnv_MemoizesMisses(t *testing.T) { + client := fake.NewSimpleClientset() + gets := 0 + client.PrependReactor("get", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + gets++ + return false, nil, nil + }) + optRef := func(varName string) corev1.EnvVar { + return corev1.EnvVar{Name: varName, ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "gone"}, + Key: "K", + Optional: ptrBool(true), + }, + }} + } + + got, err := resolveEnv(context.Background(), client, envPod(nil, []corev1.EnvVar{optRef("A"), optRef("B")})) + if err != nil { + t.Fatalf("resolveEnv: %v", err) + } + if gets != 1 { + t.Fatalf("expected 1 GET for 2 optional refs to the same missing Secret, got %d", gets) + } + if len(got) != 0 { + t.Fatalf("expected no variables set, got %v", got) + } +} + +// TestResolveEnv_FieldRef covers the downward-API subset a virtual node can answer, and the +// refusal of the rest. status.podIP is the one that matters: answering it with a placeholder +// would make a workload advertise an address nothing can reach. +func TestResolveEnv_FieldRef(t *testing.T) { + pod := envPod(nil, nil) + pod.UID = "uid-1" + pod.Labels = map[string]string{"app": "vllm"} + pod.Annotations = map[string]string{"team": "infra"} + pod.Spec.NodeName = "nebula-fake" + pod.Spec.ServiceAccountName = "sa" + + cases := []struct { + path string + want string + wantErr bool + }{ + {path: "metadata.name", want: "p1"}, + {path: "metadata.namespace", want: "default"}, + {path: "metadata.uid", want: "uid-1"}, + {path: "spec.nodeName", want: "nebula-fake"}, + {path: "spec.serviceAccountName", want: "sa"}, + {path: "metadata.labels['app']", want: "vllm"}, + {path: "metadata.annotations['team']", want: "infra"}, + // Absent label: the empty string, as on a real kubelet — the PATH is supported. + {path: "metadata.labels['nope']", want: ""}, + {path: "status.podIP", wantErr: true}, + {path: "metadata.labels", wantErr: true}, // whole-map form is volume-only + } + for _, tc := range cases { + t.Run(tc.path, func(t *testing.T) { + got, err := fieldRefValue(pod, &corev1.ObjectFieldSelector{FieldPath: tc.path}) + if tc.wantErr { + if err == nil { + t.Fatalf("expected %q to be refused, got %q", tc.path, got) + } + return + } + if err != nil { + t.Fatalf("fieldRefValue(%q): %v", tc.path, err) + } + if got != tc.want { + t.Fatalf("fieldRefValue(%q) = %q, want %q", tc.path, got, tc.want) + } + }) + } +} + +// TestResolveEnv_ResourceFieldRefRefused pins the refusal: this node advertises synthetic +// capacity (virtualCapacity), so the kubelet's "default an unset limit to the node's +// allocatable" rule would hand a workload a number no instance has. +func TestResolveEnv_ResourceFieldRefRefused(t *testing.T) { + pod := envPod(nil, []corev1.EnvVar{{Name: "MEM", ValueFrom: &corev1.EnvVarSource{ + ResourceFieldRef: &corev1.ResourceFieldSelector{Resource: "limits.memory"}, + }}}) + + _, err := resolveEnv(context.Background(), fake.NewSimpleClientset(), pod) + if err == nil || !strings.Contains(err.Error(), "limits.memory") { + t.Fatalf("expected a resourceFieldRef refusal naming the resource, got %v", err) + } + // The variable name belongs in the message too: a Pod with several refs needs to know + // which one to fix. + if !strings.Contains(err.Error(), `env "MEM"`) { + t.Fatalf("expected the error to name the variable, got %v", err) + } +} + +// TestResolveEnv_DropsIllegalNames matches the kubelet: a ConfigMap consumed wholesale +// carries whatever keys it was written with, and dropping an unusable one beats failing the +// Pod over it. The legal keys in the same source must still arrive — including "app.conf", +// since Kubernetes' env-name rule permits dots even though C_IDENTIFIER does not. +func TestResolveEnv_DropsIllegalNames(t *testing.T) { + client := fake.NewSimpleClientset(configMapObj("cm", map[string]string{ + "1_LEADING_DIGIT": "ignored", "GOOD": "v", "app.conf": "legal-here", + })) + pod := envPod([]corev1.EnvFromSource{{ + ConfigMapRef: &corev1.ConfigMapEnvSource{LocalObjectReference: corev1.LocalObjectReference{Name: "cm"}}, + }}, nil) + + got, err := resolveEnv(context.Background(), client, pod) + if err != nil { + t.Fatalf("resolveEnv: %v", err) + } + if _, bad := got["1_LEADING_DIGIT"]; bad { + t.Fatalf("expected the illegal name to be dropped, got %v", got) + } + if got["GOOD"] != "v" || got["app.conf"] != "legal-here" { + t.Fatalf("expected the legal keys to survive, got %v", got) + } +} + +// TestResolveEnv_NilClient documents the test seam: with no cluster to read, literals still +// resolve and references are skipped rather than erroring. +func TestResolveEnv_NilClient(t *testing.T) { + pod := envPod( + []corev1.EnvFromSource{{SecretRef: &corev1.SecretEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}}}}, + []corev1.EnvVar{ + {Name: "PLAIN", Value: "p"}, + {Name: "REF", ValueFrom: &corev1.EnvVarSource{SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}, Key: "K"}}}, + }, + ) + + got, err := resolveEnv(context.Background(), nil, pod) + if err != nil { + t.Fatalf("resolveEnv: %v", err) + } + if !maps.Equal(got, map[string]string{"PLAIN": "p"}) { + t.Fatalf("expected literals only, got %v", got) + } +} + +// TestCreatePod_PassesResolvedEnvToProvider is the contract between the two halves: the +// virtual node resolves, the provider receives values. +// +// It also pins what must NOT happen — the Pod's spec keeps its reference. VK compares +// Spec.Containers between the API server's Pod and the one GetPod returns (podsEqual), so a +// rewritten env list would look like a spec change on every sync; and it would put Secret +// values in an object this package copies, emits and patches. +func TestCreatePod_PassesResolvedEnvToProvider(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + pod := envPod(nil, []corev1.EnvVar{ + {Name: "PLAIN", Value: "p"}, + {Name: "TOKEN", ValueFrom: &corev1.EnvVarSource{SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "sec"}, Key: "K"}}}, + }) + client := fake.NewSimpleClientset(pod, secretObj("sec", map[string]string{"K": "t0ken"})) + h := NewHandler(fp, client, nil) + + if err := h.CreatePod(context.Background(), pod); err != nil { + t.Fatalf("CreatePod: %v", err) + } + if !maps.Equal(fp.lastReq.Env, map[string]string{"PLAIN": "p", "TOKEN": "t0ken"}) { + t.Fatalf("provider got env %v", fp.lastReq.Env) + } + if vf := pod.Spec.Containers[0].Env[1].ValueFrom; vf == nil || vf.SecretKeyRef == nil { + t.Fatal("the Pod's env must keep its reference; resolution belongs on the request") + } +} + +// TestCreatePod_UnresolvableEnvIsNonTerminal is why resolution runs before the provider call. +// A referenced Secret often lands moments after the Pod, so this behaves like the kubelet's +// CreateContainerConfigError: nothing provisioned, nothing blocklisted, the Pod waiting at +// ConfigError, and the error returned for VK to retry. +func TestCreatePod_UnresolvableEnvIsNonTerminal(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + bl := &recordingBlocklist{} + pod := envPod(nil, []corev1.EnvVar{{Name: "TOKEN", ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "not-yet"}, Key: "K"}, + }}}) + client := fake.NewSimpleClientset(pod) + h := NewHandler(fp, client, bl) + + err := h.CreatePod(context.Background(), pod) + if err == nil { + t.Fatal("expected CreatePod to fail so VK retries the sync") + } + if fp.provisionCnt != 0 { + t.Fatalf("expected no provision attempt, got %d", fp.provisionCnt) + } + if bl.calls != 0 { + t.Fatalf("a Pod-spec problem must not blocklist a candidate, got %d records", bl.calls) + } + if pod.Status.Phase != corev1.PodPending || pod.Status.Reason != reasonConfigError { + t.Fatalf("expected Pending/%s, got %s/%s", reasonConfigError, pod.Status.Phase, pod.Status.Reason) + } + // Untracked, like every other pre-instance failure: a tracked pod with no instance id + // reads as absent from List and gets written Terminated. + if h.Tracks(pod.Namespace, pod.Name) { + t.Fatal("a pod that never reached the provider must not be tracked") + } +} + +// TestResolveEnv_NoEnvIsNil keeps the common case allocation-free and the request field nil, +// so a provider can tell "nothing to set" from "an empty environment". +func TestResolveEnv_NoEnvIsNil(t *testing.T) { + got, err := resolveEnv(context.Background(), fake.NewSimpleClientset(), testPod("default", "p1")) + if err != nil || got != nil { + t.Fatalf("expected (nil, nil) for a Pod with no env, got (%v, %v)", got, err) + } +} diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index e17fc99..6bf2288 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -233,6 +233,26 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { log := logf.FromContext(ctx).WithName("vnode-handler").WithValues( "provider", h.prov.Name(), "pod", key(pod.Namespace, pod.Name), "claim", claim) + // 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. + // + // NON-TERMINAL, like the kubelet's CreateContainerConfigError. A referenced Secret is + // often written moments after the Pod (a bootstrap job, an external-secrets sync), so the + // Pod waits at ConfigError with the reason on it and VK retries with backoff. Failing it + // would reap a workload over a race, and would run recordBlock's failover machinery over + // a Pod-spec problem no other provider or region can fix. + // + // Deliberately NOT stored, as in the transport-failure branch below: a tracked pod with + // no instance id reads as absent from List and gets written Terminated. + env, err := resolveEnv(ctx, h.client, pod) + if err != nil { + log.Error(err, "cannot resolve the Pod's environment; nothing provisioned, retrying") + h.markStatus(pod, corev1.PodPending, reasonConfigError, err.Error()) + h.emit(pod) + return err + } + req.Env = env + // Bound the provision call so a wedged backend cannot pin this worker forever. A // provider may raise the deadline via Capabilities.ProvisionTimeout (AWS does, for // cross-zone failover). @@ -248,8 +268,10 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { provisionCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() + // The env COUNT, never its content: a value here may have come from a Secret. log.Info("provisioning external instance", - "capacityType", req.CapacityType, "region", req.Region, "timeout", timeout.String()) + "capacityType", req.CapacityType, "region", req.Region, + "envVars", len(req.Env), "timeout", timeout.String()) // Two clocks for two different waits: provisionStart measures the end-to-end wait a // user feels (until the instance reports Running; handed to store below), callStart diff --git a/pkg/vnode/status.go b/pkg/vnode/status.go index cc9a198..d5692cd 100644 --- a/pkg/vnode/status.go +++ b/pkg/vnode/status.go @@ -35,6 +35,7 @@ const ( reasonInitializing = nebulav1alpha1.PodReasonInitializing reasonRunning = nebulav1alpha1.PodReasonRunning reasonProvisionFailed = nebulav1alpha1.PodReasonProvisionFailed + reasonConfigError = nebulav1alpha1.PodReasonConfigError reasonFailed = nebulav1alpha1.PodReasonFailed reasonTerminated = nebulav1alpha1.PodReasonTerminated )