Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/v1alpha1/groupversion_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
33 changes: 19 additions & 14 deletions pkg/provider/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ticketed an issue: #67

// 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
}

Expand Down
33 changes: 33 additions & 0 deletions pkg/provider/aws/aws_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 5 additions & 1 deletion pkg/provider/modal/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
38 changes: 25 additions & 13 deletions pkg/provider/modal/modal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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),
Expand Down
51 changes: 51 additions & 0 deletions pkg/provider/modal/modal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
50 changes: 46 additions & 4 deletions pkg/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"context"
"fmt"
"io"
"sort"
"strings"
"time"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading