From a86e55df6def8ef424ac310f90c1f4da9fc810e8 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 27 Aug 2026 16:34:21 +0100 Subject: [PATCH 1/2] support image pull secret Signed-off-by: kerthcet --- pkg/provider/aws/aws.go | 12 ++ pkg/provider/errors.go | 12 +- pkg/provider/modal/client.go | 58 +++++- pkg/provider/modal/modal.go | 99 +++++++++-- pkg/provider/modal/modal_test.go | 155 ++++++++++++++++ pkg/provider/provider.go | 10 +- pkg/provider/registryauth.go | 119 +++++++++++++ pkg/provider/registryauth_test.go | 108 ++++++++++++ pkg/vnode/handler.go | 12 ++ pkg/vnode/imagepull.go | 197 +++++++++++++++++++++ pkg/vnode/imagepull_test.go | 284 ++++++++++++++++++++++++++++++ 11 files changed, 1047 insertions(+), 19 deletions(-) create mode 100644 pkg/provider/registryauth.go create mode 100644 pkg/provider/registryauth_test.go create mode 100644 pkg/vnode/imagepull.go create mode 100644 pkg/vnode/imagepull_test.go diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index e0ff5d1..d686b3d 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -722,6 +722,18 @@ func (p *Provider) instanceSpecFromPod( } c := pod.Spec.Containers[0] + // Refused, not ignored: buildUserData's `docker pull` is anonymous, so honouring this + // needs a bootstrap that logs in first (`aws ecr get-login-password` for a role — the + // instance profile is already there). Until then, silently pulling without the credential + // would either 401 or fetch a PUBLIC image of the same name. + // + // TODO: implement the role path in buildUserData. A basic credential needs more care — + // user-data is readable via DescribeInstanceAttribute (see buildSpec's env caveat). + if req.RegistryAuth != nil { + // No kind is wired here yet, so the shared refusal covers every one of them. + return InstanceSpec{}, req.RegistryAuth.Unsupported("aws") + } + // 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 diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index 2a6911e..86fe186 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -48,6 +48,13 @@ var ( // nothing about the same request in another. The adapter confines it to the // failing region (see aws.ClassifyProvisionError). Transient until quota frees up. ErrQuota = errors.New("provider: quota exceeded") + // ErrImagePull: the image could not be pulled — a credential the provider cannot honour, + // one it was not given, or a registry that refused it. + // + // Deliberately NOT ErrAuth, though both are authentication: ErrAuth widens to DenyAll, + // which would fence off the whole provider because ONE Pod named a role it cannot + // assume. This is a property of the Pod, so it is scoped like capacity. + ErrImagePull = errors.New("provider: cannot pull image") ) // ClassifyError maps a provision error to the BlockScope it should be blocklisted at, @@ -114,7 +121,7 @@ const ( // catAuth: credentials or authorization failed, so nothing on the provider works. catAuth // catCapacity: the provider refused THIS request — no capacity, quota exhausted, - // or an accelerator it does not offer. + // an accelerator it does not offer, or an image it cannot pull. catCapacity ) @@ -125,7 +132,8 @@ func categorize(err error) failureCategory { switch { case errors.Is(err, ErrAuth): return catAuth - case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), errors.Is(err, ErrQuota): + case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), + errors.Is(err, ErrQuota), errors.Is(err, ErrImagePull): return catCapacity } diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index f4fcd6e..34d5414 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -139,7 +139,10 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if spec.Image == "" { return "", Credential{}, fmt.Errorf("modal: empty image in sandbox spec") } - image := c.mc.Images.FromRegistry(spec.Image, nil) + image, err := c.imageFor(ctx, spec) + if err != nil { + return "", Credential{}, err + } probe, err := modalProbe(spec.ReadinessProbe) if err != nil { @@ -161,6 +164,8 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string GPU: gpuReservation(spec.GPU, spec.GPUCount), CPU: spec.CPU, MemoryMiB: spec.MemoryMiB, + CPULimit: spec.CPULimit, + MemoryLimitMiB: spec.MemoryLimitMiB, EncryptedPorts: spec.Ports, // Nil leaves Modal's SchedulerPlacement unset entirely (the SDK only builds one // when Regions is non-empty), which is the unconstrained, un-multiplied case. @@ -182,6 +187,57 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string return sb.SandboxID, c.mintCredential(ctx, sb, spec), nil } +// imageFor resolves the sandbox's image, attaching pull credentials when the spec carries +// them. Modal takes them as a Secret, so this is where the spec's data becomes an SDK object. +func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Image, error) { + a := spec.RegistryAuth + switch { + case a == nil: + return c.mc.Images.FromRegistry(spec.Image, nil), nil + + case a.AWSRole != nil: + // REGISTRY_AUTH_TYPE_AWS: Modal assumes the role via its own OIDC identity, so the + // role's trust policy must trust this workspace — workspace-admin setup that a + // provision cannot arrange, hence a failure here is terminal for the Pod. + // + // Region is guaranteed non-empty by checkRegistryAuth; Modal always sends it. + secret, err := c.registrySecret(ctx, map[string]string{ + "AWS_ROLE_ARN": a.AWSRole.RoleARN, + "AWS_REGION": a.AWSRole.Region, + }) + if err != nil { + return nil, fmt.Errorf("modal: ECR pull secret: %w", err) + } + return c.mc.Images.FromAwsEcr(spec.Image, secret, nil), nil + + case a.Basic != nil: + secret, err := c.registrySecret(ctx, map[string]string{ + "REGISTRY_USERNAME": a.Basic.Username, + "REGISTRY_PASSWORD": a.Basic.Password, + }) + if err != nil { + return nil, fmt.Errorf("modal: registry pull secret: %w", err) + } + return c.mc.Images.FromRegistry(spec.Image, &modal.ImageFromRegistryParams{Secret: secret}), nil + + default: + // Refuse rather than pull anonymously; see provider.RegistryAuth. + return nil, fmt.Errorf("modal: unsupported image pull credential: %w", provider.ErrImagePull) + } +} + +// registrySecret builds a lazily-hydrated ephemeral Modal Secret for these values. +// +// Per sandbox, and cached nowhere: the credential belongs to the Pod, is re-read from the +// Pod's Kubernetes Secret on every provision, and must not outlive the call. The cost is that +// a distinct SecretID gives the image a distinct hash, so Modal may re-pull an image it has +// already built — layer caching still applies, and a credential kept in process memory for +// the manager's lifetime is the worse trade. +func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (*modal.Secret, error) { + // The server-side create happens later, when the image build hydrates this. + return c.mc.Secrets.FromMap(ctx, kv, nil) +} + // mintCredential issues the sandbox's connect credential and RETURNS it, storing nothing. // Not in a tag, since Modal's tags are plaintext and bulk-listable — one ListSandboxes // would hand over every workload's token. Not in memory, which is not durable. A diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 4490d32..6b748cc 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -127,6 +127,16 @@ type SandboxSpec struct { // MemoryMiB is the requested memory in MiB, from the Pod's request. Zero lets // Modal apply its own default. MemoryMiB int + // CPULimit and MemoryLimitMiB are the HARD caps, from the Pod's limits only — + // never from its requests, unlike CPU/MemoryMiB above, which fall back to limits + // when no request is given. + // + // Zero means no cap, which is also what a Pod that declares no limit means, so the + // two vocabularies line up without a special case. Without these a Pod's limits + // reached Modal as nothing at all: a limits-only Pod became a RESERVATION of that + // size with an unbounded ceiling — the inverse of what it asked for, and billable. + CPULimit float64 + MemoryLimitMiB int // Ports are the container ports to expose, from the Pod's containerPorts. They // declare to Modal which ports may receive traffic at all, and the connect URL // routes to the first of them (see firstPort) — one token routes to one port. @@ -170,6 +180,14 @@ type SandboxSpec struct { // sandbox is reported statusInitializing rather than statusRunning. // We only ever pass a user-supplied probe; the adapter never fabricates one. ReadinessProbe *corev1.Probe + // RegistryAuth is the pull credential, or nil for an anonymous pull — the canonical form + // verbatim, since its two kinds already map 1:1 onto the SDK entry points (AWSRole → + // Images.FromAwsEcr, Basic → Images.FromRegistry). Data, not a *modal.Secret: minting one + // is an SDK call, and this spec stays SDK-free. + // + // Whether Modal can express it at all is settled before this is set; see + // checkRegistryAuth. Its own String redacts the password. + RegistryAuth *provider.RegistryAuth } // String redacts Env so a spec can be logged or wrapped in an error safely: key names print @@ -177,10 +195,11 @@ type SandboxSpec struct { // 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 Egress:%s EgressTargets:%v Timeout:%s Tags:%v ReadinessProbe:%t}", + "CPULimit:%g MemoryMiB:%d MemoryLimitMiB:%d Ports:%v Regions:%v Egress:%s "+ + "EgressTargets:%v Timeout:%s Tags:%v ReadinessProbe:%t RegistryAuth:%s}", s.Image, s.Command, provider.RedactedEnv(s.Env), s.GPU, s.GPUCount, s.CPU, - s.MemoryMiB, s.Ports, s.Regions, s.EgressMode, s.EgressTargets, - s.Timeout, s.Tags, s.ReadinessProbe != nil) + s.CPULimit, s.MemoryMiB, s.MemoryLimitMiB, s.Ports, s.Regions, s.EgressMode, + s.EgressTargets, s.Timeout, s.Tags, s.ReadinessProbe != nil, s.RegistryAuth) } // GoString implements fmt.GoStringer so %#v is redacted too. @@ -503,10 +522,12 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq // 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), + Env: req.Env, + CPU: cpuCores(&c), + MemoryMiB: memoryMiB(&c), + CPULimit: cpuLimitCores(&c), + MemoryLimitMiB: memoryLimitMiB(&c), + Ports: containerPorts(&c), // An empty request region stays an empty slice, not a one-element [""]: that // is the unconstrained case (no region declared on the pool), and it must // reach Modal as "no placement constraint" — its widest pool and its @@ -519,6 +540,16 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq ReadinessProbe: c.ReadinessProbe, } + // Vet the resolved pull credential HERE, with the Pod in hand and before any API call, so + // one Modal cannot express costs nothing — the same place and for the same reason as the + // MapAccelerator check below. + if req.RegistryAuth != nil { + if err := checkRegistryAuth(req.RegistryAuth); err != nil { + return SandboxSpec{}, err + } + spec.RegistryAuth = req.RegistryAuth + } + // Accelerator type comes from the AcceleratorTypeLabel; count from the // container's nvidia.com/gpu resource (see util.AcceleratorRequest). canonical, count, err := util.AcceleratorRequest(pod) @@ -561,22 +592,54 @@ func regionsOf(region string) []string { return out } +// checkRegistryAuth reports whether Modal can honour a pull credential, so the spec carries +// only ones the Client is able to attach. Nothing to translate — the kinds map 1:1 onto the +// SDK entry points — so this is only the supported-kinds decision plus the shared +// well-formedness check. +// +// A refusal, never a fallback: an anonymous pull of a private image either 401s opaquely or +// succeeds against a PUBLIC image of the same name. +func checkRegistryAuth(a *provider.RegistryAuth) error { + switch { + case a.AWSRole != nil, a.Basic != nil: + if err := a.Validate(); err != nil { + return fmt.Errorf("modal: %w", err) // every error out of this adapter is prefixed + } + return nil + default: + // A kind the canonical form carries but this adapter has not wired — GCP, which + // Modal reaches via Images.FromGcpArtifactRegistry. + return a.Unsupported("modal") + } +} + // cpuCores reads the container's CPU request as fractional physical cores (Modal's // unit). It prefers requests, falling back to limits, and returns 0 (→ Modal // default) when neither is set. -func cpuCores(c *corev1.Container) float64 { - q := resourceQty(c, corev1.ResourceCPU) +func cpuCores(c *corev1.Container) float64 { return cores(resourceQty(c, corev1.ResourceCPU)) } + +// memoryMiB reads the container's memory request in MiB (Modal's unit), preferring +// requests over limits. Returns 0 (→ Modal default) when neither is set. +func memoryMiB(c *corev1.Container) int { return mib(resourceQty(c, corev1.ResourceMemory)) } + +// cpuLimitCores and memoryLimitMiB read the LIMITS, with no fallback to the request: a +// request is a floor, and reusing it as a ceiling would cap a burstable Pod that never +// asked to be capped. Zero (no limit declared) reaches Modal as "no limit", matching +// Kubernetes. The request/limit asymmetry is entirely in which lookup they use. +func cpuLimitCores(c *corev1.Container) float64 { return cores(limitQty(c, corev1.ResourceCPU)) } +func memoryLimitMiB(c *corev1.Container) int { return mib(limitQty(c, corev1.ResourceMemory)) } + +// cores converts a CPU quantity to Modal's unit, fractional physical cores. MilliValue +// is cores*1000. A nil quantity (unset) is 0, which lets Modal apply its own default. +func cores(q *resource.Quantity) float64 { if q == nil { return 0 } - // MilliValue is cores*1000; convert to fractional cores. return float64(q.MilliValue()) / 1000.0 } -// memoryMiB reads the container's memory request in MiB (Modal's unit), preferring -// requests over limits. Returns 0 (→ Modal default) when neither is set. -func memoryMiB(c *corev1.Container) int { - q := resourceQty(c, corev1.ResourceMemory) +// mib converts a memory quantity to Modal's unit, MiB. Nil is 0, as in cores. +func mib(q *resource.Quantity) int { if q == nil { return 0 } @@ -596,6 +659,14 @@ func resourceQty(c *corev1.Container, name corev1.ResourceName) *resource.Quanti return nil } +// limitQty returns the container's limit for name, or nil when it has none. +func limitQty(c *corev1.Container, name corev1.ResourceName) *resource.Quantity { + if q, ok := c.Resources.Limits[name]; ok { + return &q + } + return nil +} + // containerPorts collects the container's declared ports, which is what tells Modal // which ports may receive traffic at all. The connect URL then routes to one of them // (see firstPort). diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 497b048..28d8b35 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -18,6 +18,7 @@ package modal import ( "context" + "errors" "fmt" "io" "slices" @@ -246,6 +247,66 @@ func TestProvision_MapsResourcesPortsAndTimeout(t *testing.T) { } } +func TestProvision_MapsResourceLimits(t *testing.T) { + cases := []struct { + name string + requests, limits corev1.ResourceList + wantCPU, wantCPULimit float64 + wantMemMiB, wantMemLimMiB int + }{ + { + name: "limits only: limit is the ceiling AND the request falls back to it", + limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("2"), corev1.ResourceMemory: resource.MustParse("8Gi")}, + wantCPU: 2, wantCPULimit: 2, + wantMemMiB: 8192, wantMemLimMiB: 8192, + }, + { + name: "both: burstable, request below the ceiling", + requests: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("500m"), corev1.ResourceMemory: resource.MustParse("1Gi")}, + limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("4"), corev1.ResourceMemory: resource.MustParse("16Gi")}, + wantCPU: 0.5, wantCPULimit: 4, + wantMemMiB: 1024, wantMemLimMiB: 16384, + }, + { + name: "neither: Modal applies its own defaults, uncapped", + wantCPU: 0, wantCPULimit: 0, + wantMemMiB: 0, wantMemLimMiB: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := &fakeClient{createID: "sb-lim"} + p := newTestProvider(f) + + pod := gpuPod("claim-lim", "H100", 1) + c := &pod.Spec.Containers[0] + // MERGE into the limits gpuPod already set: nvidia.com/gpu lives there, and + // replacing the map would drop it, silently making these CPU-only Pods and + // leaving the GPU-plus-limits combination untested. + c.Resources.Requests = tc.requests + for name, q := range tc.limits { + if c.Resources.Limits == nil { + c.Resources.Limits = corev1.ResourceList{} + } + c.Resources.Limits[name] = q + } + + if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-lim"}); err != nil { + t.Fatalf("Provision: %v", err) + } + if f.lastSpec.CPU != tc.wantCPU || f.lastSpec.CPULimit != tc.wantCPULimit { + t.Fatalf("CPU/CPULimit = (%v, %v), want (%v, %v)", + f.lastSpec.CPU, f.lastSpec.CPULimit, tc.wantCPU, tc.wantCPULimit) + } + if f.lastSpec.MemoryMiB != tc.wantMemMiB || f.lastSpec.MemoryLimitMiB != tc.wantMemLimMiB { + t.Fatalf("MemoryMiB/MemoryLimitMiB = (%d, %d), want (%d, %d)", + f.lastSpec.MemoryMiB, f.lastSpec.MemoryLimitMiB, tc.wantMemMiB, tc.wantMemLimMiB) + } + }) + } +} + func TestProvision_DefaultsTimeoutWhenNoDeadline(t *testing.T) { f := &fakeClient{createID: "sb-dt"} p := newTestProvider(f) @@ -1442,3 +1503,97 @@ func TestProvision_CarriesEgressPolicy(t *testing.T) { t.Errorf("spec.EgressMode = %q, want %q for a nil policy", got, nebulav1alpha1.EgressOpen) } } + +func TestCheckRegistryAuth(t *testing.T) { + const arn = "arn:aws:iam::123456789012:role/pull" + + // Both kinds are wired here, so a well-formed one of either passes. + if err := checkRegistryAuth(&provider.RegistryAuth{ + AWSRole: &provider.AWSRoleAuth{RoleARN: arn, Region: "eu-central-1"}, + }); err != nil { + t.Fatalf("AWS role: %v", err) + } + if err := checkRegistryAuth(&provider.RegistryAuth{ + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + }); err != nil { + t.Fatalf("basic: %v", err) + } + + // Well-formedness is the shared check's, but it must reach the caller through here — + // and as ErrImagePull, so it fails this Pod rather than blocklisting the whole provider + // the way ErrAuth would. + err := checkRegistryAuth(&provider.RegistryAuth{ + AWSRole: &provider.AWSRoleAuth{RoleARN: arn}, // no region + }) + if !errors.Is(err, provider.ErrImagePull) { + t.Fatalf("err = %v, want ErrImagePull", err) + } + if !strings.HasPrefix(err.Error(), "modal: ") { + t.Errorf("err = %q, want the adapter's prefix", err) + } + + // A kind Modal is not wired for must not silently become an anonymous pull. + err = checkRegistryAuth(&provider.RegistryAuth{Registry: "gcr.io"}) + if !errors.Is(err, provider.ErrImagePull) { + t.Fatalf("err = %v, want ErrImagePull for an unwired kind", err) + } +} + +// TestProvision_CarriesRegistryAuth pins the whole seam: what the resolver put on the +// request reaches the SandboxSpec, and the spec never prints the password. +func TestProvision_CarriesRegistryAuth(t *testing.T) { + f := &fakeClient{createID: "sb-auth"} + p := newTestProvider(f) + + pod := gpuPod("claim-auth", "H100", 1) + pod.Spec.Containers[0].Image = "123456789012.dkr.ecr.us-west-2.amazonaws.com/team/trainer:v3" + _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ + ClaimName: "claim-auth", + RegistryAuth: &provider.RegistryAuth{ + Registry: "123456789012.dkr.ecr.us-west-2.amazonaws.com", + AWSRole: &provider.AWSRoleAuth{ + RoleARN: "arn:aws:iam::123456789012:role/pull", + Region: "us-west-2", + }, + }, + }) + if err != nil { + t.Fatalf("Provision: %v", err) + } + // The canonical credential reaches the spec verbatim — no per-adapter copy to drift. + a := f.lastSpec.RegistryAuth + if a == nil || a.AWSRole == nil { + t.Fatalf("spec.RegistryAuth = %v, want an AWSRole credential", a) + } + if a.AWSRole.Region != "us-west-2" { + t.Errorf("region = %q, want us-west-2", a.AWSRole.Region) + } + + // A Pod naming a role Modal cannot use fails the Pod, and does so before any API call: + // a rejected credential must not leave a sandbox running. + f2 := &fakeClient{createID: "sb-none"} + pod2 := gpuPod("claim-bad", "H100", 1) + pod2.Spec.Containers[0].Image = "123456789012.dkr.ecr.us-west-2.amazonaws.com/a" + _, err = newTestProvider(f2).Provision(context.Background(), pod2, provider.ProvisionRequest{ + ClaimName: "claim-bad", + RegistryAuth: &provider.RegistryAuth{AWSRole: &provider.AWSRoleAuth{RoleARN: "arn:x"}}, + }) + if !errors.Is(err, provider.ErrImagePull) { + t.Fatalf("err = %v, want ErrImagePull", err) + } + if f2.createCnt != 0 { + t.Errorf("CreateSandbox called %d times, want 0", f2.createCnt) + } +} + +func TestSandboxSpecStringRedactsRegistryAuth(t *testing.T) { + s := SandboxSpec{ + Image: "ghcr.io/org/app:v1", + RegistryAuth: &provider.RegistryAuth{ + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + }, + } + if got := s.String(); strings.Contains(got, "hunter2") { + t.Errorf("String() = %q, must not contain the password", got) + } +} diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 43470a3..ae6e52c 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -256,14 +256,20 @@ type ProvisionRequest struct { // the same rule as ProvisionResult.ConnectToken. Nil is normal: no env, or an // unresolving caller. Env map[string]string + // RegistryAuth authenticates the pull of the container image, from the Pod's + // imagePullSecrets; nil is an anonymous pull. Resolved by the caller (pkg/vnode) for the + // same reason as Env — it names a Secret an adapter cannot read. + RegistryAuth *RegistryAuth } // 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 Egress:%s Env:%s}", - r.ClaimName, r.CapacityType, r.Region, r.Egress.ModeOrOpen(), RedactedEnv(r.Env)) + return fmt.Sprintf("ProvisionRequest{ClaimName:%s CapacityType:%s Region:%s Egress:%s Env:%s "+ + "RegistryAuth:%s}", + r.ClaimName, r.CapacityType, r.Region, r.Egress.ModeOrOpen(), RedactedEnv(r.Env), + r.RegistryAuth) } // GoString implements fmt.GoStringer so %#v is redacted too. diff --git a/pkg/provider/registryauth.go b/pkg/provider/registryauth.go new file mode 100644 index 0000000..fdea026 --- /dev/null +++ b/pkg/provider/registryauth.go @@ -0,0 +1,119 @@ +/* +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 provider + +import "fmt" + +// RegistryAuth is the resolved credential for pulling the workload's image, in a +// provider-neutral form each adapter translates into its own mechanism (Modal calls +// Images.FromAwsEcr; an EC2 adapter would run `aws ecr get-login-password`). +// +// Exactly one source field is set, and which one IS the kind — the shape +// corev1.EnvVarSource uses. No kind enum, which could disagree with the fields, and no flat +// struct, whose inapplicable halves invite reading one that was never set. +// +// The contract every adapter owes it: honour it or FAIL the Provision, as with +// ProvisionRequest.Egress. A silent fall-through to an anonymous pull either 401s opaquely +// or succeeds against a PUBLIC image of the same name. +type RegistryAuth struct { + // Registry is the host the credential applies to, from the image reference. + Registry string + // AWSRole authenticates by role assumption; see AWSRoleAuth. + AWSRole *AWSRoleAuth + // Basic authenticates with a username and password; see BasicAuth. + Basic *BasicAuth +} + +// AWSRoleAuth is an AWS IAM role the PROVIDER assumes to pull from a private ECR registry — +// a delegation, not a credential: neither field is secret, and the role only works where its +// trust policy trusts that provider's identity (for Modal, its OIDC provider). +type AWSRoleAuth struct { + RoleARN string + // Region is where GetAuthorizationToken is called: the REGISTRY's region, not where the + // workload runs. Required, and stated rather than parsed out of the image reference: the + // ECR host encodes a region, but a pull-through or replicated reference makes that a + // guess, and the guess is only discovered wrong as an opaque auth failure. + Region string +} + +// BasicAuth is HTTP Basic against the registry, which all of them accept. An ECR "password" +// is a 12-hour token though, so a fixed one there rots — use AWSRoleAuth. +// +// SECRET-BEARING in Password, hence the redaction in RegistryAuth.String. +type BasicAuth struct { + Username string + Password string +} + +// Validate reports whether a is WELL-FORMED: exactly one kind set, and that kind's fields +// all present. Not whether any given adapter can honour it — that is per-adapter (see +// Unsupported) — so every adapter calls this for the kinds it does support. +// +// Here rather than in each adapter because these are invariants of the TYPE: an ECR pull +// needs the role and the registry's region wherever it runs, and a half-filled credential +// only ever surfaces as an opaque 401 from the registry. +// +// Callers upstream (vnode.resolveRegistryAuth) reject the same things earlier with a better +// message; this is the adapter boundary's own guarantee, since ProvisionRequest can be built +// by anyone. +func (a *RegistryAuth) Validate() error { + switch { + case a == nil: + return nil // no credential is not a malformed one; an anonymous pull is legal + case a.AWSRole != nil && a.Basic != nil: + return fmt.Errorf("registry auth for %q sets two kinds at once: %w", + a.Registry, ErrImagePull) + case a.AWSRole != nil: + if a.AWSRole.RoleARN == "" || a.AWSRole.Region == "" { + return fmt.Errorf("AWS role auth for %q needs both a role ARN and a region: %w", + a.Registry, ErrImagePull) + } + case a.Basic != nil: + if a.Basic.Username == "" || a.Basic.Password == "" { + return fmt.Errorf("basic auth for %q needs both a username and a password: %w", + a.Registry, ErrImagePull) + } + default: + return fmt.Errorf("registry auth for %q sets no credential: %w", a.Registry, ErrImagePull) + } + return nil +} + +// Unsupported is the error an adapter returns for a credential kind it cannot honour, named +// so the message says which provider refused. Shared so the ErrImagePull wrap cannot be +// forgotten: unwrapped, the same refusal reads as unattributable and the Pod retries against +// a provider that will never accept it (see ErrImagePull, IsRejection). +func (a *RegistryAuth) Unsupported(providerName string) error { + return fmt.Errorf("%s: unsupported image pull credential %s: %w", providerName, a, ErrImagePull) +} + +// String names the role ARN — an identifier, and what makes "cannot assume this role" +// actionable — and never a password. +func (a *RegistryAuth) String() string { + switch { + case a == nil: + return "none" + case a.AWSRole != nil: + return fmt.Sprintf("AWSRole(%s,role=%s,region=%s)", + a.Registry, a.AWSRole.RoleARN, a.AWSRole.Region) + case a.Basic != nil: + return fmt.Sprintf("Basic(%s,user=%s,password=%s)", + a.Registry, a.Basic.Username, redacted(a.Basic.Password)) + default: + return fmt.Sprintf("unknown(%s)", a.Registry) + } +} diff --git a/pkg/provider/registryauth_test.go b/pkg/provider/registryauth_test.go new file mode 100644 index 0000000..4ea19aa --- /dev/null +++ b/pkg/provider/registryauth_test.go @@ -0,0 +1,108 @@ +/* +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 provider + +import ( + "errors" + "strings" + "testing" +) + +func TestRegistryAuthValidate(t *testing.T) { + role := func(arn, region string) *RegistryAuth { + return &RegistryAuth{Registry: "r", AWSRole: &AWSRoleAuth{RoleARN: arn, Region: region}} + } + basic := func(user, pass string) *RegistryAuth { + return &RegistryAuth{Registry: "r", Basic: &BasicAuth{Username: user, Password: pass}} + } + + cases := []struct { + name string + auth *RegistryAuth + ok bool + }{ + // An anonymous pull is legal, so nil is well-formed rather than malformed. + {"nil", nil, true}, + {"role and region", role("arn:aws:iam::1:role/pull", "us-west-2"), true}, + {"user and password", basic("bot", "hunter2"), true}, + + // A half-filled credential is the case worth catching: it reaches the registry and + // comes back as an opaque 401 that names nothing. + {"role without a region", role("arn:aws:iam::1:role/pull", ""), false}, + {"region without a role", role("", "us-west-2"), false}, + {"user without a password", basic("bot", ""), false}, + {"password without a user", basic("", "hunter2"), false}, + {"no kind at all", &RegistryAuth{Registry: "r"}, false}, + { + // Ambiguous, and which one an adapter would pick is arbitrary. + name: "two kinds at once", + auth: &RegistryAuth{ + Registry: "r", + AWSRole: &AWSRoleAuth{RoleARN: "arn:aws:iam::1:role/pull", Region: "us-west-2"}, + Basic: &BasicAuth{Username: "bot", Password: "hunter2"}, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := tc.auth.Validate() + if tc.ok { + if err != nil { + t.Fatalf("Validate() = %v, want nil", err) + } + return + } + if err == nil { + t.Fatal("Validate() = nil, want an error") + } + // The sentinel is the contract: ErrImagePull scopes the block to this Pod's + // request, where ErrAuth would fence off the entire provider. + if !errors.Is(err, ErrImagePull) { + t.Errorf("Validate() = %v, want it to wrap ErrImagePull", err) + } + if errors.Is(err, ErrAuth) { + t.Errorf("Validate() = %v, must NOT wrap ErrAuth (it widens to DenyAll)", err) + } + if strings.Contains(err.Error(), "hunter2") { + t.Errorf("Validate() = %q, must not leak the password", err) + } + }) + } +} + +func TestRegistryAuthUnsupported(t *testing.T) { + err := (&RegistryAuth{ + Registry: "ghcr.io", + Basic: &BasicAuth{Username: "bot", Password: "hunter2"}, + }).Unsupported("aws") + + if !errors.Is(err, ErrImagePull) { + t.Errorf("err = %v, want it to wrap ErrImagePull", err) + } + // A refusal must be a rejection, not an unattributable failure: the latter leaves the + // Pod retrying against a provider that will never accept the credential. + if !IsRejection(err) { + t.Errorf("IsRejection(%v) = false, want true", err) + } + if !strings.Contains(err.Error(), "aws") { + t.Errorf("err = %q, want the refusing provider named", err) + } + if strings.Contains(err.Error(), "hunter2") { + t.Errorf("err = %q, must not leak the password", err) + } +} diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index ceff1ad..6700687 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -307,6 +307,18 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { } req.Env = env + // Same resolve-before-requesting rule, and the same non-terminal treatment, for the + // image pull credential: an imagePullSecret names a Secret in the Pod's namespace that + // an adapter cannot read either (see resolveRegistryAuth). + auth, err := resolveRegistryAuth(ctx, h.client, pod) + if err != nil { + log.Error(err, "cannot resolve the Pod's image pull credential; nothing provisioned, retrying") + h.markStatus(pod, corev1.PodPending, reasonConfigError, err.Error()) + h.emit(pod) + return err + } + req.RegistryAuth = auth + // 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). diff --git a/pkg/vnode/imagepull.go b/pkg/vnode/imagepull.go new file mode 100644 index 0000000..5c7409a --- /dev/null +++ b/pkg/vnode/imagepull.go @@ -0,0 +1,197 @@ +/* +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" + "encoding/base64" + "encoding/json" + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + + "github.com/InftyAI/Nebula/pkg/provider" +) + +// SecretTypeAWSRole marks a Secret carrying the AWS IAM role a provider assumes to pull a +// private ECR image: +// +// roleARN: arn:aws:iam::123456789012:role/nebula-ecr-pull # required +// region: us-west-2 # required, the REGISTRY's region +// +// A dedicated TYPE rather than an Opaque Secret sniffed for known keys: one declared field to +// dispatch on, and room for the other delegated-identity kinds (GCP, Azure) to arrive as +// their own types. It holds no actual secret — an ARN is an identifier — and is a Secret only +// because imagePullSecrets can reference nothing else. +const SecretTypeAWSRole corev1.SecretType = "nebula.inftyai.com/aws-role" + +// resolveRegistryAuth builds ProvisionRequest.RegistryAuth from the Pod's imagePullSecrets, +// which name Secrets in the Pod's namespace that an adapter cannot read — the same reason +// resolveEnv lives here. (nil, nil) is an anonymous pull. +// +// The FIRST entry of a recognized type wins, never a merge: a provider attaches ONE +// credential to one image, and the kubelet's own rule (try each until a pull succeeds) is +// unavailable, since the pull happens inside the provider after this returns. +// +// Every other outcome is an error. imagePullSecrets states that this image needs a +// credential, and pulling without one either 401s opaquely or succeeds against a PUBLIC image +// of the same name. +func resolveRegistryAuth( + ctx context.Context, client kubernetes.Interface, pod *corev1.Pod, +) (*provider.RegistryAuth, error) { + if len(pod.Spec.ImagePullSecrets) == 0 || len(pod.Spec.Containers) == 0 { + return nil, nil + } + if client == nil { + // No literals-only path to degrade to, unlike resolveEnv: this is a read or nothing. + return nil, fmt.Errorf("imagePullSecrets are set but this node has no cluster client") + } + // One container per Nebula Pod, as everywhere else that reads the workload. + registry := registryHost(pod.Spec.Containers[0].Image) + + for _, ref := range pod.Spec.ImagePullSecrets { + s, err := client.CoreV1().Secrets(pod.Namespace).Get(ctx, ref.Name, metav1.GetOptions{}) + if err != nil { + // Includes NotFound, and non-terminally so: the caller stamps ConfigError and + // retries, which is right for a Secret written moments after the Pod. + return nil, fmt.Errorf("read imagePullSecret %q: %w", ref.Name, err) + } + switch s.Type { + case SecretTypeAWSRole: + arn := strings.TrimSpace(string(s.Data[secretKeyRoleARN])) + region := strings.TrimSpace(string(s.Data[secretKeyRegion])) + // Both required. The region is the REGISTRY's, which the ECR host does encode, + // but a pull-through or replicated reference makes reading it off the image a + // guess — and a wrong one only ever surfaces as an opaque auth failure. + if arn == "" || region == "" { + return nil, fmt.Errorf("imagePullSecret %q of type %s needs a non-empty %q and %q", + ref.Name, s.Type, secretKeyRoleARN, secretKeyRegion) + } + return &provider.RegistryAuth{ + Registry: registry, + AWSRole: &provider.AWSRoleAuth{RoleARN: arn, Region: region}, + }, nil + + case corev1.SecretTypeDockerConfigJson: + auth, err := dockerConfigAuth(s, registry) + if err != nil { + return nil, fmt.Errorf("imagePullSecret %q: %w", ref.Name, err) + } + return auth, nil + } + } + return nil, fmt.Errorf("no imagePullSecret of a supported type (%s, %s) among %v", + SecretTypeAWSRole, corev1.SecretTypeDockerConfigJson, pullSecretNames(pod)) +} + +// The keys SecretTypeAWSRole carries, both required. +const ( + secretKeyRoleARN = "roleARN" + secretKeyRegion = "region" +) + +// dockerConfigAuth reads registry's entry from a kubernetes.io/dockerconfigjson Secret. Only +// the auths map, never credsStore/credHelpers: a helper names a binary on the machine holding +// the config, and there is no such machine here. +func dockerConfigAuth(s *corev1.Secret, registry string) (*provider.RegistryAuth, error) { + raw, ok := s.Data[corev1.DockerConfigJsonKey] + if !ok { + return nil, fmt.Errorf("no %q key", corev1.DockerConfigJsonKey) + } + var cfg struct { + Auths map[string]struct { + Username string `json:"username"` + Password string `json:"password"` + Auth string `json:"auth"` + } `json:"auths"` + } + if err := json.Unmarshal(raw, &cfg); err != nil { + return nil, fmt.Errorf("parse %q: %w", corev1.DockerConfigJsonKey, err) + } + + entry, ok := cfg.Auths[registry] + if !ok { + // Docker Hub is the one registry whose config key is historically not its host. + // Everything else must match exactly — guessing which entry covers a host is how a + // credential reaches the wrong registry. + for _, alias := range dockerHubAliases(registry) { + if entry, ok = cfg.Auths[alias]; ok { + break + } + } + } + if !ok { + return nil, fmt.Errorf("no auths entry for registry %q", registry) + } + + username, password := entry.Username, entry.Password + if username == "" && entry.Auth != "" { + // base64("user:password"), which is what `docker login` writes; the explicit + // username/password pair is the older form. Both are valid. + decoded, err := base64.StdEncoding.DecodeString(entry.Auth) + if err != nil { + return nil, fmt.Errorf("decode auth for registry %q: %w", registry, err) + } + username, password, _ = strings.Cut(string(decoded), ":") + } + if username == "" || password == "" { + return nil, fmt.Errorf("auths entry for registry %q has no usable credential", registry) + } + return &provider.RegistryAuth{ + Registry: registry, + Basic: &provider.BasicAuth{Username: username, Password: password}, + }, nil +} + +// registryHost extracts the registry host from an image reference, by Docker's rule: the +// first segment is a registry only if it looks like a host or is "localhost"; otherwise the +// reference is a Docker Hub name (library/nginx, myorg/app). +func registryHost(image string) string { + first, _, found := strings.Cut(image, "/") + if !found { + return dockerHubRegistry // bare "nginx:1.27" + } + if first == "localhost" || strings.ContainsAny(first, ".:") { + return first + } + return dockerHubRegistry +} + +// dockerHubRegistry is what an image with no registry resolves to. +const dockerHubRegistry = "docker.io" + +// dockerHubAliases returns the other keys a dockerconfigjson may hold Docker Hub credentials +// under — `docker login` writes the v1 index URL to this day. Nil for anything else. +func dockerHubAliases(registry string) []string { + if registry != dockerHubRegistry { + return nil + } + return []string{"https://index.docker.io/v1/", "index.docker.io", "registry-1.docker.io"} +} + +// pullSecretNames lists the Pod's imagePullSecret names, so the "no supported type" error +// names what was found and not only what was wanted. +func pullSecretNames(pod *corev1.Pod) []string { + out := make([]string, 0, len(pod.Spec.ImagePullSecrets)) + for _, ref := range pod.Spec.ImagePullSecrets { + out = append(out, ref.Name) + } + return out +} diff --git a/pkg/vnode/imagepull_test.go b/pkg/vnode/imagepull_test.go new file mode 100644 index 0000000..07106bf --- /dev/null +++ b/pkg/vnode/imagepull_test.go @@ -0,0 +1,284 @@ +/* +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" + "encoding/base64" + "fmt" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + + "github.com/InftyAI/Nebula/pkg/provider" +) + +// pullPod is a Pod pulling image with the named imagePullSecrets. +func pullPod(image string, secretNames ...string) *corev1.Pod { + pod := testPod("default", "p1") + pod.Spec.Containers[0].Image = image + for _, n := range secretNames { + pod.Spec.ImagePullSecrets = append(pod.Spec.ImagePullSecrets, + corev1.LocalObjectReference{Name: n}) + } + return pod +} + +// typedSecret is a Secret of an explicit type, which is what resolveRegistryAuth dispatches +// on (secretObj in env_test.go leaves the type empty). +func typedSecret(name string, t corev1.SecretType, data map[string]string) *corev1.Secret { + s := secretObj(name, data) + s.Type = t + return s +} + +// dockerConfigSecret renders a kubernetes.io/dockerconfigjson Secret for one registry. Uses +// the base64 auth form, since that is what `docker login` writes. +func dockerConfigSecret(name, registry, user, password string) *corev1.Secret { + auth := base64.StdEncoding.EncodeToString([]byte(user + ":" + password)) + return typedSecret(name, corev1.SecretTypeDockerConfigJson, map[string]string{ + corev1.DockerConfigJsonKey: fmt.Sprintf(`{"auths":{%q:{"auth":%q}}}`, registry, auth), + }) +} + +const ecrImage = "123456789012.dkr.ecr.us-west-2.amazonaws.com/team/trainer:v3" + +func TestResolveRegistryAuth(t *testing.T) { + cases := []struct { + name string + pod *corev1.Pod + objs []runtime.Object + want *provider.RegistryAuth + wantErr string + }{ + { + // The overwhelmingly common case: a public image and no credential at all. + name: "no imagePullSecrets is an anonymous pull", + pod: pullPod("nginx:1.27"), + }, + { + name: "aws-role Secret yields the role, region from the Secret", + pod: pullPod(ecrImage, "ecr-pull"), + objs: []runtime.Object{typedSecret("ecr-pull", SecretTypeAWSRole, map[string]string{ + "roleARN": "arn:aws:iam::123456789012:role/pull", + "region": "eu-central-1", + })}, + want: &provider.RegistryAuth{ + Registry: "123456789012.dkr.ecr.us-west-2.amazonaws.com", + AWSRole: &provider.AWSRoleAuth{ + RoleARN: "arn:aws:iam::123456789012:role/pull", + Region: "eu-central-1", + }, + }, + }, + { + // Not derived from the image host: the region is stated or it is an error. + name: "an aws-role Secret without a region is an error", + pod: pullPod(ecrImage, "ecr-pull"), + objs: []runtime.Object{typedSecret("ecr-pull", SecretTypeAWSRole, map[string]string{ + "roleARN": "arn:aws:iam::123456789012:role/pull", + })}, + wantErr: `needs a non-empty "roleARN" and "region"`, + }, + { + name: "whitespace around the values is trimmed", + pod: pullPod(ecrImage, "ecr-pull"), + objs: []runtime.Object{typedSecret("ecr-pull", SecretTypeAWSRole, map[string]string{ + "roleARN": " arn:aws:iam::123456789012:role/pull\n", + "region": " us-west-2 ", + })}, + want: &provider.RegistryAuth{ + Registry: "123456789012.dkr.ecr.us-west-2.amazonaws.com", + AWSRole: &provider.AWSRoleAuth{ + RoleARN: "arn:aws:iam::123456789012:role/pull", + Region: "us-west-2", + }, + }, + }, + { + name: "dockerconfigjson yields a basic credential", + pod: pullPod("ghcr.io/org/app:v1", "ghcr"), + objs: []runtime.Object{dockerConfigSecret("ghcr", "ghcr.io", "bot", "hunter2")}, + want: &provider.RegistryAuth{ + Registry: "ghcr.io", + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + }, + }, + { + // A registry-less image is Docker Hub, whose config key is the legacy index URL. + name: "docker hub matches its legacy index key", + pod: pullPod("myorg/app:v1", "hub"), + objs: []runtime.Object{ + dockerConfigSecret("hub", "https://index.docker.io/v1/", "u", "p"), + }, + want: &provider.RegistryAuth{ + Registry: "docker.io", + Basic: &provider.BasicAuth{Username: "u", Password: "p"}, + }, + }, + { + // A credential for another registry must not be handed to this one. + name: "a dockerconfigjson for a different registry is an error", + pod: pullPod("ghcr.io/org/app:v1", "hub"), + objs: []runtime.Object{dockerConfigSecret("hub", "docker.io", "u", "p")}, + wantErr: `no auths entry for registry "ghcr.io"`, + }, + { + // The first RECOGNIZED type wins, so an unknown type earlier is skipped over. + name: "the first recognized type wins", + pod: pullPod(ecrImage, "opaque", "ecr-pull"), + objs: []runtime.Object{ + secretObj("opaque", map[string]string{"roleARN": "arn:aws:iam::1:role/ignored"}), + typedSecret("ecr-pull", SecretTypeAWSRole, map[string]string{ + "roleARN": "arn:aws:iam::123456789012:role/pull", + "region": "us-west-2", + }), + }, + want: &provider.RegistryAuth{ + Registry: "123456789012.dkr.ecr.us-west-2.amazonaws.com", + AWSRole: &provider.AWSRoleAuth{ + RoleARN: "arn:aws:iam::123456789012:role/pull", + Region: "us-west-2", + }, + }, + }, + { + // NOT an anonymous-pull downgrade: the Pod asked for a credential. + name: "a missing Secret is an error", + pod: pullPod(ecrImage, "ecr-pull"), + wantErr: `read imagePullSecret "ecr-pull"`, + }, + { + name: "an aws-role Secret without a roleARN is an error", + pod: pullPod(ecrImage, "ecr-pull"), + objs: []runtime.Object{ + typedSecret("ecr-pull", SecretTypeAWSRole, map[string]string{"region": "us-west-2"}), + }, + wantErr: `needs a non-empty "roleARN" and "region"`, + }, + { + name: "no Secret of a supported type is an error", + pod: pullPod(ecrImage, "opaque"), + objs: []runtime.Object{secretObj("opaque", map[string]string{"roleARN": "arn:x"})}, + wantErr: "no imagePullSecret of a supported type", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveRegistryAuth(context.Background(), fake.NewSimpleClientset(tc.objs...), tc.pod) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("got nil error, want one containing %q", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %v, want it to contain %q", err, tc.wantErr) + } + if got != nil { + t.Errorf("auth = %v on error, want nil — a partial credential must not reach a provider", got) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if diff := authDiff(got, tc.want); diff != "" { + t.Errorf("auth mismatch: %s", diff) + } + }) + } +} + +// TestResolveRegistryAuthNoClient covers the seam other vnode tests rely on: a nil client is +// fine until a Pod actually references a Secret, which is a read or nothing. +func TestResolveRegistryAuthNoClient(t *testing.T) { + if _, err := resolveRegistryAuth(context.Background(), nil, pullPod("nginx:1.27")); err != nil { + t.Fatalf("no imagePullSecrets with a nil client: %v", err) + } + _, err := resolveRegistryAuth(context.Background(), nil, pullPod(ecrImage, "ecr-pull")) + if err == nil { + t.Fatal("imagePullSecrets with a nil client: got nil error, want one") + } +} + +func TestRegistryHost(t *testing.T) { + cases := map[string]string{ + "nginx:1.27": "docker.io", + "myorg/app:v1": "docker.io", + "library/nginx": "docker.io", + "ghcr.io/org/app:v1": "ghcr.io", + "localhost:5000/app": "localhost:5000", + "localhost/app": "localhost", + ecrImage: "123456789012.dkr.ecr.us-west-2.amazonaws.com", + "registry.example.com:443/a/b": "registry.example.com:443", + } + for image, want := range cases { + if got := registryHost(image); got != want { + t.Errorf("registryHost(%q) = %q, want %q", image, got, want) + } + } +} + +// TestRegistryAuthStringRedacts pins the one thing that must never regress: a password does +// not print, while the role ARN does (it is an identifier, and the actionable part of a +// "cannot assume this role" report). +func TestRegistryAuthStringRedacts(t *testing.T) { + basic := &provider.RegistryAuth{ + Registry: "ghcr.io", + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + } + if s := basic.String(); strings.Contains(s, "hunter2") { + t.Errorf("String() = %q, must not contain the password", s) + } + role := &provider.RegistryAuth{ + Registry: "x.dkr.ecr.us-west-2.amazonaws.com", + AWSRole: &provider.AWSRoleAuth{RoleARN: "arn:aws:iam::1:role/pull"}, + } + if s := role.String(); !strings.Contains(s, "arn:aws:iam::1:role/pull") { + t.Errorf("String() = %q, want the role ARN named", s) + } + if s := (*provider.RegistryAuth)(nil).String(); s != "none" { + t.Errorf("nil String() = %q, want %q", s, "none") + } +} + +// authDiff renders the difference between two credentials, without printing a password. +func authDiff(got, want *provider.RegistryAuth) string { + switch { + case got == nil && want == nil: + return "" + case got == nil || want == nil: + return fmt.Sprintf("got %v, want %v", got, want) + case got.Registry != want.Registry: + return fmt.Sprintf("registry = %q, want %q", got.Registry, want.Registry) + } + switch { + case want.AWSRole != nil: + if got.AWSRole == nil || *got.AWSRole != *want.AWSRole { + return fmt.Sprintf("role = %v, want %v", got.AWSRole, want.AWSRole) + } + case want.Basic != nil: + if got.Basic == nil || *got.Basic != *want.Basic { + // Both are test fixtures, so printing them is safe here. + return fmt.Sprintf("basic = %v, want %v", got.Basic, want.Basic) + } + } + return "" +} From 790dd6c9b90868c07cb5dfb6e9b6f38b34ddaedd Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 27 Aug 2026 20:46:38 +0100 Subject: [PATCH 2/2] address comments Signed-off-by: kerthcet --- pkg/provider/aws/aws.go | 7 ++ pkg/provider/aws/aws_test.go | 7 ++ pkg/provider/errors.go | 54 +++++++--- pkg/provider/errors_test.go | 14 +++ pkg/provider/modal/modal.go | 25 ++++- pkg/provider/modal/modal_test.go | 44 ++++++-- pkg/vnode/imagepull.go | 113 ++++++++++++-------- pkg/vnode/imagepull_test.go | 175 +++++++++++++++++++++++++++++-- 8 files changed, 365 insertions(+), 74 deletions(-) diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index d686b3d..7b7eb7c 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -682,6 +682,13 @@ func (p *Provider) ClassifyProvisionError(err error, accelerator, region string) tier = nebulav1alpha1.CapacitySpot } scope := provider.ClassifyError(err, tier, accelerator) + // The zero scope means BLOCK NOTHING — a rejection of this request that says nothing + // about the candidate, such as an image pull credential this adapter cannot honour. + // Stamping a region onto it would make it non-empty, and recordBlock would then install a + // block covering every accelerator and tier in that region. + if scope == (provider.BlockScope{}) { + return scope + } // Confine an accelerator/capacity/quota block to the region that failed. A // DenyAll (auth) fails in every region, so it stays region-wide (Region left nil). // An empty region (should not happen — every request carries one) maps to &"" = diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index e350f36..e42b02a 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -572,6 +572,13 @@ func TestClassifyProvisionError(t *testing.T) { &smithy.GenericAPIError{Code: "InvalidFleetConfiguration", Message: "not supported in AZ"}, onDemandRegional}, {"nil", nil, provider.BlockScope{}}, + // An image pull credential this adapter cannot honour is a fact about the POD, so it + // blocks nothing. Region must NOT be stamped on: a scope carrying only a region + // fences off every accelerator and tier there, on behalf of one Pod. + {"image pull blocks nothing", provider.ErrImagePull, provider.BlockScope{}}, + {"wrapped image pull blocks nothing", + fmt.Errorf("aws: unsupported image pull credential: %w", provider.ErrImagePull), + provider.BlockScope{}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index 86fe186..2b03579 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -51,22 +51,27 @@ var ( // ErrImagePull: the image could not be pulled — a credential the provider cannot honour, // one it was not given, or a registry that refused it. // - // Deliberately NOT ErrAuth, though both are authentication: ErrAuth widens to DenyAll, - // which would fence off the whole provider because ONE Pod named a role it cannot - // assume. This is a property of the Pod, so it is scoped like capacity. + // REQUEST-scoped, and the only sentinel that is: it describes the Pod, not the candidate. + // So it blocklists NOTHING (see ClassifyError). Both alternatives are wrong — ErrAuth + // widens to DenyAll, fencing off the whole provider because one Pod named a role it + // cannot assume, and a capacity scope evicts an accelerator/tier/region that is serving + // other Pods perfectly well, since the blocklist key carries no Pod, image or credential + // identity. A failure that belongs to one request cannot be recorded against a candidate. ErrImagePull = errors.New("provider: cannot pull image") ) // ClassifyError maps a provision error to the BlockScope it should be blocklisted at, // checking the shared sentinels first and falling back to string heuristics for raw API // messages. The rule it encodes: a narrow failure must not disqualify other accelerators, or -// other regions. Only auth widens to the whole provider via DenyAll; everything else — -// including an unrecognized error — is scoped to the failing accelerator/tier/region so -// failover can route around it. +// other regions. Only auth widens to the whole provider via DenyAll; a failure that belongs to +// the REQUEST rather than the candidate blocks nothing at all; everything else — including an +// unrecognized error — is scoped to the failing accelerator/tier/region so failover can route +// around it. // // This is the single place the SHARED part of a scope is derived, so every adapter delegates // here and then only adds what is provider-specific (AWS adds its region). Nothing assembles -// a scope elsewhere. +// a scope elsewhere. An adapter that decorates the result must leave the ZERO scope alone — +// it means "block nothing", and any field added to it becomes a block. // // capacityType is stamped onto accelerator-scoped blocks so a Spot failure does not block // OnDemand. accelerator is the request's POOL identity (type:count, "" for a CPU-only Pod), @@ -93,6 +98,19 @@ func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, accelera return BlockScope{DenyAll: true} case catCapacity: return capacityScope() + case catRequest: + // Nothing is blocklisted: the request was refused, the candidate is fine. A block + // here would be recorded against provider/accelerator/tier/region — a key with no + // Pod, image or credential in it — so one Pod's unusable pull credential would + // exclude that candidate for every OTHER Pod until the TTL lapsed. + // + // The zero scope makes recordBlock a no-op, which is the whole mechanism. An adapter + // must therefore not decorate this scope (see ClassifyProvisionError in each + // adapter): adding a region would make it non-empty and install a region-wide block. + // + // Still a rejection, so the Pod fails with the reason rather than retrying forever — + // the two questions are separate. See IsRejection. + return BlockScope{} default: // An unrecognized error is scoped like capacity (this accelerator + tier, and // per region once the adapter confines it), NOT DenyAll. A DenyAll on an @@ -120,9 +138,14 @@ const ( catUnattributable failureCategory = iota // catAuth: credentials or authorization failed, so nothing on the provider works. catAuth - // catCapacity: the provider refused THIS request — no capacity, quota exhausted, - // an accelerator it does not offer, or an image it cannot pull. + // catCapacity: the provider refused this request because of something about the + // CANDIDATE — no capacity, quota exhausted, an accelerator it does not offer. Blocking + // it is meaningful, because the next Pod asking for the same candidate would fail too. catCapacity + // catRequest: the provider refused this request because of something about the REQUEST + // itself — today only an image it cannot pull. A decision, so a rejection, but it says + // nothing about the candidate and must not blocklist one. + catRequest ) // categorize buckets a provision error, sentinels first and string heuristics after. @@ -132,8 +155,10 @@ func categorize(err error) failureCategory { switch { case errors.Is(err, ErrAuth): return catAuth + case errors.Is(err, ErrImagePull): + return catRequest case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), - errors.Is(err, ErrQuota), errors.Is(err, ErrImagePull): + errors.Is(err, ErrQuota): return catCapacity } @@ -175,9 +200,12 @@ func categorize(err error) failureCategory { // error, a timeout, a 503, an unparseable response. // // The distinction exists because the two call for opposite handling and the costs are -// asymmetric. A rejection is authoritative, so the Pod is failed and the candidate -// blocklisted, and failover routes around it — all correct, because the provider said -// no. An unattributable failure is authoritative about nothing, so the same writes +// asymmetric. A rejection is authoritative, so the Pod is failed with the reason — correct, +// because the provider said no. Whether a CANDIDATE is also blocklisted is a separate +// question, answered by ClassifyError: a capacity rejection blocks one and failover routes +// around it, while a request-scoped one (an unusable image credential) blocks nothing, because +// the candidate did nothing wrong. An unattributable failure is authoritative about nothing, +// so the same writes // stamp a terminal status onto a request the provider may have accepted (leaving a // paid instance running behind a Pod that is about to be reaped) and fence off a // candidate that never misbehaved. Retrying costs a request and Provision is diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 3727003..330c854 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -45,6 +45,16 @@ func TestClassifyError(t *testing.T) { {"quota sentinel", ErrQuota, capacityScope}, {"no-capacity sentinel", ErrNoCapacity, capacityScope}, {"unsupported sentinel", ErrUnsupportedAccelerator, capacityScope}, + // An unusable image credential belongs to the POD. The blocklist key carries no Pod, + // image or credential identity, so ANY scope here would exclude the candidate for + // unrelated Pods that pull perfectly well — hence the zero scope, which blocks + // nothing. Still a rejection: see TestIsRejection. + {"image-pull sentinel blocks nothing", ErrImagePull, BlockScope{}}, + { + "wrapped image-pull sentinel blocks nothing", + fmt.Errorf("modal: unsupported image pull credential: %w", ErrImagePull), + BlockScope{}, + }, {"wrapped sentinel", fmt.Errorf("provision failed: %w", ErrNoCapacity), capacityScope}, {"string unauthorized", fmt.Errorf("HTTP 401 unauthorized"), BlockScope{DenyAll: true}}, {"string quota", fmt.Errorf("account limit exceeded"), capacityScope}, @@ -102,6 +112,10 @@ func TestIsRejection(t *testing.T) { {"wrapped sentinel", fmt.Errorf("create sandbox: %w", ErrNoCapacity), true}, {"string capacity", errors.New("InsufficientInstanceCapacity"), true}, {"string auth", errors.New("HTTP 401 unauthorized"), true}, + // Blocks nothing (see TestClassifyError), yet still a rejection: the provider DID + // decide, and a Pod whose credential it cannot use must fail with that reason rather + // than retry it forever. Blocking and rejecting are separate questions. + {"image-pull sentinel", ErrImagePull, true}, // The failures this predicate exists for. {"deadline exceeded", context.DeadlineExceeded, false}, diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 6b748cc..b7f66b5 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -132,7 +132,8 @@ type SandboxSpec struct { // when no request is given. // // Zero means no cap, which is also what a Pod that declares no limit means, so the - // two vocabularies line up without a special case. Without these a Pod's limits + // two vocabularies line up on everything but one case: a positive limit smaller than + // Modal's unit must not truncate into that sentinel (see limitMiB). Without these a Pod's limits // reached Modal as nothing at all: a limits-only Pod became a RESERVATION of that // size with an unbounded ceiling — the inverse of what it asked for, and billable. CPULimit float64 @@ -471,6 +472,13 @@ func (p *Provider) ClassifyProvisionError(err error, accelerator, region string) return provider.BlockScope{} } scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, accelerator) + // The zero scope means BLOCK NOTHING — a rejection of this request that says nothing + // about the candidate, such as an image credential Modal cannot use. Stamping a region + // onto it would make it non-empty, and recordBlock would install a region-wide block + // across every accelerator: the same trap as the err == nil guard above. + if scope == (provider.BlockScope{}) { + return scope + } // DenyAll already covers every region (auth fails everywhere), so narrowing it // would contradict the category. if region != "" && !scope.DenyAll { @@ -627,7 +635,7 @@ func memoryMiB(c *corev1.Container) int { return mib(resourceQty(c, corev1.Resou // asked to be capped. Zero (no limit declared) reaches Modal as "no limit", matching // Kubernetes. The request/limit asymmetry is entirely in which lookup they use. func cpuLimitCores(c *corev1.Container) float64 { return cores(limitQty(c, corev1.ResourceCPU)) } -func memoryLimitMiB(c *corev1.Container) int { return mib(limitQty(c, corev1.ResourceMemory)) } +func memoryLimitMiB(c *corev1.Container) int { return limitMiB(limitQty(c, corev1.ResourceMemory)) } // cores converts a CPU quantity to Modal's unit, fractional physical cores. MilliValue // is cores*1000. A nil quantity (unset) is 0, which lets Modal apply its own default. @@ -647,6 +655,19 @@ func mib(q *resource.Quantity) int { return int(q.Value() / miB) } +// limitMiB is mib for a LIMIT, where 0 does not mean "unset" but "no cap". A positive +// quantity below 1 MiB truncates to 0 there, so the plain conversion would hand an +// UNBOUNDED sandbox to the one Pod that asked for the tightest ceiling — the inverse +// of its declaration. Any positive limit therefore floors at 1 MiB, the smallest cap +// Modal's unit can express. Modal may then refuse it as below its own minimum, which is +// the honest answer for a limit it cannot honour, and is not silently unlimited. +func limitMiB(q *resource.Quantity) int { + if m := mib(q); m != 0 || q == nil || q.Sign() <= 0 { + return m + } + return 1 +} + // resourceQty returns the container's request for name, falling back to its limit, // or nil when neither is present. func resourceQty(c *corev1.Container, name corev1.ResourceName) *resource.Quantity { diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 28d8b35..d3c65b9 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -255,16 +255,25 @@ func TestProvision_MapsResourceLimits(t *testing.T) { wantMemMiB, wantMemLimMiB int }{ { - name: "limits only: limit is the ceiling AND the request falls back to it", - limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("2"), corev1.ResourceMemory: resource.MustParse("8Gi")}, + name: "limits only: limit is the ceiling AND the request falls back to it", + limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("2"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, wantCPU: 2, wantCPULimit: 2, wantMemMiB: 8192, wantMemLimMiB: 8192, }, { - name: "both: burstable, request below the ceiling", - requests: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("500m"), corev1.ResourceMemory: resource.MustParse("1Gi")}, - limits: corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("4"), corev1.ResourceMemory: resource.MustParse("16Gi")}, - wantCPU: 0.5, wantCPULimit: 4, + name: "both: burstable, request below the ceiling", + requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("16Gi"), + }, + wantCPU: 0.5, wantCPULimit: 4, wantMemMiB: 1024, wantMemLimMiB: 16384, }, { @@ -272,6 +281,20 @@ func TestProvision_MapsResourceLimits(t *testing.T) { wantCPU: 0, wantCPULimit: 0, wantMemMiB: 0, wantMemLimMiB: 0, }, + { + // A ceiling below Modal's unit must not truncate into the zero that means + // "no cap" on the limit fields: it would leave the Pod asking for the + // TIGHTEST ceiling running unbounded. The matching request is a different + // question — zero there means "Modal's default", so falling back to 0 is + // correct and the asymmetry is deliberate. + name: "sub-MiB ceiling floors at 1 MiB instead of becoming uncapped", + limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1m"), + corev1.ResourceMemory: resource.MustParse("500Ki"), + }, + wantCPU: 0.001, wantCPULimit: 0.001, + wantMemMiB: 0, wantMemLimMiB: 1, + }, } for _, tc := range cases { @@ -913,6 +936,15 @@ func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { if got := p.ClassifyProvisionError(nil, "H100:1", "us-east"); got != (provider.BlockScope{}) { t.Fatalf("a nil error must classify to the zero scope, got %+v", got) } + + // Same trap, and the reason the guard is not just the nil check above: an unusable image + // credential belongs to one POD, so it blocks nothing. Stamping the region here would + // make the scope non-empty and fence off every accelerator in us-east because one Pod + // named a role Modal cannot assume. + if got := p.ClassifyProvisionError(provider.ErrImagePull, "H100:1", "us-east"); got != + (provider.BlockScope{}) { + t.Fatalf("an image-pull rejection must block nothing, got %+v", got) + } } func TestProvision_CarriesDeclaredPorts(t *testing.T) { diff --git a/pkg/vnode/imagepull.go b/pkg/vnode/imagepull.go index 5c7409a..1a350de 100644 --- a/pkg/vnode/imagepull.go +++ b/pkg/vnode/imagepull.go @@ -46,13 +46,17 @@ const SecretTypeAWSRole corev1.SecretType = "nebula.inftyai.com/aws-role" // which name Secrets in the Pod's namespace that an adapter cannot read — the same reason // resolveEnv lives here. (nil, nil) is an anonymous pull. // -// The FIRST entry of a recognized type wins, never a merge: a provider attaches ONE -// credential to one image, and the kubelet's own rule (try each until a pull succeeds) is -// unavailable, since the pull happens inside the provider after this returns. +// The first USABLE entry wins, never a merge: a provider attaches ONE credential to one image. +// Every earlier entry that did not work out — unreadable, mistyped, malformed, or holding +// credentials for a different registry — is recorded and skipped, not raised. Kubernetes lets +// a Pod list one imagePullSecret per registry precisely so they can be tried in turn, so +// [docker-hub, ghcr] has to pull from GHCR; failing at the first entry made the ORDER decide +// whether a pull worked. // -// Every other outcome is an error. imagePullSecrets states that this image needs a -// credential, and pulling without one either 401s opaquely or succeeds against a PUBLIC image -// of the same name. +// An error only when NOTHING works, and it then carries every reason collected, since which +// one the user meant to be the working entry is unknowable from here. It cannot degrade to an +// anonymous pull: imagePullSecrets states that this image needs a credential, and pulling +// without one either 401s opaquely or succeeds against a PUBLIC image of the same name. func resolveRegistryAuth( ctx context.Context, client kubernetes.Interface, pod *corev1.Pod, ) (*provider.RegistryAuth, error) { @@ -66,39 +70,68 @@ func resolveRegistryAuth( // One container per Nebula Pod, as everywhere else that reads the workload. registry := registryHost(pod.Spec.Containers[0].Image) + // Why each entry so far was unusable, in the order tried. Collected rather than returned + // because a later imagePullSecret may still be the right one, and then none of this + // mattered. + var problems []string + for _, ref := range pod.Spec.ImagePullSecrets { - s, err := client.CoreV1().Secrets(pod.Namespace).Get(ctx, ref.Name, metav1.GetOptions{}) + auth, err := registryAuthFromSecret(ctx, client, pod.Namespace, ref.Name, registry) if err != nil { - // Includes NotFound, and non-terminally so: the caller stamps ConfigError and - // retries, which is right for a Secret written moments after the Pod. - return nil, fmt.Errorf("read imagePullSecret %q: %w", ref.Name, err) + problems = append(problems, err.Error()) + continue } - switch s.Type { - case SecretTypeAWSRole: - arn := strings.TrimSpace(string(s.Data[secretKeyRoleARN])) - region := strings.TrimSpace(string(s.Data[secretKeyRegion])) - // Both required. The region is the REGISTRY's, which the ECR host does encode, - // but a pull-through or replicated reference makes reading it off the image a - // guess — and a wrong one only ever surfaces as an opaque auth failure. - if arn == "" || region == "" { - return nil, fmt.Errorf("imagePullSecret %q of type %s needs a non-empty %q and %q", - ref.Name, s.Type, secretKeyRoleARN, secretKeyRegion) - } - return &provider.RegistryAuth{ - Registry: registry, - AWSRole: &provider.AWSRoleAuth{RoleARN: arn, Region: region}, - }, nil - - case corev1.SecretTypeDockerConfigJson: - auth, err := dockerConfigAuth(s, registry) - if err != nil { - return nil, fmt.Errorf("imagePullSecret %q: %w", ref.Name, err) - } - return auth, nil + return auth, nil + } + // Non-empty by construction: the loop either returned or appended on every entry, and + // there is at least one. + return nil, fmt.Errorf("no imagePullSecret yielded a credential for registry %q: %s", + registry, strings.Join(problems, "; ")) +} + +// registryAuthFromSecret resolves ONE imagePullSecret, or says why it cannot be used. +// +// It never returns (nil, nil): "this Secret is not the one" is an error here so the caller can +// report it, and the caller decides whether it matters — none of these failures is fatal while +// another imagePullSecret is left to try. +func registryAuthFromSecret( + ctx context.Context, client kubernetes.Interface, namespace, name, registry string, +) (*provider.RegistryAuth, error) { + s, err := client.CoreV1().Secrets(namespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + // Includes NotFound. Still worth retrying when it is the only entry, which the + // caller's aggregate error gets for free: it stamps ConfigError and comes back, and a + // Secret written moments after the Pod then resolves. + return nil, fmt.Errorf("read imagePullSecret %q: %w", name, err) + } + + switch s.Type { + case SecretTypeAWSRole: + arn := strings.TrimSpace(string(s.Data[secretKeyRoleARN])) + region := strings.TrimSpace(string(s.Data[secretKeyRegion])) + // Both required. The region is the REGISTRY's, which the ECR host does encode, but a + // pull-through or replicated reference makes reading it off the image a guess — and a + // wrong one only ever surfaces as an opaque auth failure. + if arn == "" || region == "" { + return nil, fmt.Errorf("imagePullSecret %q of type %s needs a non-empty %q and %q", + name, s.Type, secretKeyRoleARN, secretKeyRegion) } + return &provider.RegistryAuth{ + Registry: registry, + AWSRole: &provider.AWSRoleAuth{RoleARN: arn, Region: region}, + }, nil + + case corev1.SecretTypeDockerConfigJson: + auth, err := dockerConfigAuth(s, registry) + if err != nil { + return nil, fmt.Errorf("imagePullSecret %q: %w", name, err) + } + return auth, nil + + default: + return nil, fmt.Errorf("imagePullSecret %q has unsupported type %s, want %s or %s", + name, s.Type, SecretTypeAWSRole, corev1.SecretTypeDockerConfigJson) } - return nil, fmt.Errorf("no imagePullSecret of a supported type (%s, %s) among %v", - SecretTypeAWSRole, corev1.SecretTypeDockerConfigJson, pullSecretNames(pod)) } // The keys SecretTypeAWSRole carries, both required. @@ -138,6 +171,8 @@ func dockerConfigAuth(s *corev1.Secret, registry string) (*provider.RegistryAuth } } if !ok { + // A miss, not a fault — the commonest reason a Pod lists more than one + // imagePullSecret. The caller simply moves on to the next. return nil, fmt.Errorf("no auths entry for registry %q", registry) } @@ -185,13 +220,3 @@ func dockerHubAliases(registry string) []string { } return []string{"https://index.docker.io/v1/", "index.docker.io", "registry-1.docker.io"} } - -// pullSecretNames lists the Pod's imagePullSecret names, so the "no supported type" error -// names what was found and not only what was wanted. -func pullSecretNames(pod *corev1.Pod) []string { - out := make([]string, 0, len(pod.Spec.ImagePullSecrets)) - for _, ref := range pod.Spec.ImagePullSecrets { - out = append(out, ref.Name) - } - return out -} diff --git a/pkg/vnode/imagepull_test.go b/pkg/vnode/imagepull_test.go index 07106bf..c977e19 100644 --- a/pkg/vnode/imagepull_test.go +++ b/pkg/vnode/imagepull_test.go @@ -62,11 +62,14 @@ const ecrImage = "123456789012.dkr.ecr.us-west-2.amazonaws.com/team/trainer:v3" func TestResolveRegistryAuth(t *testing.T) { cases := []struct { - name string - pod *corev1.Pod - objs []runtime.Object - want *provider.RegistryAuth - wantErr string + name string + pod *corev1.Pod + objs []runtime.Object + want *provider.RegistryAuth + // wantErr is one substring the error must contain; wantErrAll is several, for the + // aggregate error that reports every entry it tried. + wantErr string + wantErrAll []string }{ { // The overwhelmingly common case: a public image and no credential at all. @@ -140,6 +143,50 @@ func TestResolveRegistryAuth(t *testing.T) { objs: []runtime.Object{dockerConfigSecret("hub", "docker.io", "u", "p")}, wantErr: `no auths entry for registry "ghcr.io"`, }, + { + // The reported bug: Kubernetes allows one imagePullSecret per registry, so a + // Docker Hub entry listed first must not decide a GHCR pull. + name: "a Secret for another registry falls through to the next one", + pod: pullPod("ghcr.io/org/app:v1", "hub", "ghcr"), + objs: []runtime.Object{ + dockerConfigSecret("hub", "docker.io", "u", "p"), + dockerConfigSecret("ghcr", "ghcr.io", "bot", "hunter2"), + }, + want: &provider.RegistryAuth{ + Registry: "ghcr.io", + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + }, + }, + { + // Order must not matter for a BROKEN entry either: if a later Secret works, the + // earlier failures were noise. + name: "a malformed Secret does not stop a later working one", + pod: pullPod("ghcr.io/org/app:v1", "corrupt", "missing", "ghcr"), + objs: []runtime.Object{ + typedSecret("corrupt", corev1.SecretTypeDockerConfigJson, map[string]string{ + corev1.DockerConfigJsonKey: "{not json", + }), + dockerConfigSecret("ghcr", "ghcr.io", "bot", "hunter2"), + }, + want: &provider.RegistryAuth{ + Registry: "ghcr.io", + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + }, + }, + { + // Nothing worked, so every reason is reported — which one was meant to be the + // working entry cannot be known from here. + name: "when nothing works, every reason is reported", + pod: pullPod("ghcr.io/org/app:v1", "hub", "opaque"), + objs: []runtime.Object{ + dockerConfigSecret("hub", "docker.io", "u", "p"), + secretObj("opaque", map[string]string{"roleARN": "arn:x"}), + }, + wantErrAll: []string{ + `no auths entry for registry "ghcr.io"`, + `"opaque" has unsupported type`, + }, + }, { // The first RECOGNIZED type wins, so an unknown type earlier is skipped over. name: "the first recognized type wins", @@ -177,19 +224,25 @@ func TestResolveRegistryAuth(t *testing.T) { name: "no Secret of a supported type is an error", pod: pullPod(ecrImage, "opaque"), objs: []runtime.Object{secretObj("opaque", map[string]string{"roleARN": "arn:x"})}, - wantErr: "no imagePullSecret of a supported type", + wantErr: `"opaque" has unsupported type`, }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { got, err := resolveRegistryAuth(context.Background(), fake.NewSimpleClientset(tc.objs...), tc.pod) + wantErrs := tc.wantErrAll if tc.wantErr != "" { + wantErrs = append(wantErrs, tc.wantErr) + } + if len(wantErrs) > 0 { if err == nil { - t.Fatalf("got nil error, want one containing %q", tc.wantErr) + t.Fatalf("got nil error, want one containing %q", wantErrs) } - if !strings.Contains(err.Error(), tc.wantErr) { - t.Fatalf("error = %v, want it to contain %q", err, tc.wantErr) + for _, want := range wantErrs { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %v, want it to contain %q", err, want) + } } if got != nil { t.Errorf("auth = %v on error, want nil — a partial credential must not reach a provider", got) @@ -218,6 +271,110 @@ func TestResolveRegistryAuthNoClient(t *testing.T) { } } +// TestCreatePod_PassesResolvedRegistryAuthToProvider is the handler seam, the mirror of +// TestCreatePod_PassesResolvedEnvToProvider: the virtual node reads the imagePullSecret, the +// provider receives a credential. Without it the resolver could be perfect while CreatePod +// handed every adapter a nil, turning every private pull anonymous — which fails opaquely, or +// worse, succeeds against a PUBLIC image of the same name. +func TestCreatePod_PassesResolvedRegistryAuthToProvider(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + pod := pullPod("ghcr.io/org/app:v1", "ghcr") + client := fake.NewSimpleClientset(pod, dockerConfigSecret("ghcr", "ghcr.io", "bot", "hunter2")) + h := NewHandler(fp, client, nil, openCluster()) + + if err := h.CreatePod(context.Background(), pod); err != nil { + t.Fatalf("CreatePod: %v", err) + } + want := &provider.RegistryAuth{ + Registry: "ghcr.io", + Basic: &provider.BasicAuth{Username: "bot", Password: "hunter2"}, + } + if diff := authDiff(fp.lastReq.RegistryAuth, want); diff != "" { + t.Fatalf("provider got the wrong credential: %s", diff) + } + // The Pod keeps its REFERENCE, as with env: VK compares specs on every sync, and the + // resolved credential has no business in an object this package copies, emits and patches. + if len(pod.Spec.ImagePullSecrets) != 1 || pod.Spec.ImagePullSecrets[0].Name != "ghcr" { + t.Fatalf("the Pod's imagePullSecrets must be left alone, got %v", pod.Spec.ImagePullSecrets) + } +} + +// TestCreatePod_AnonymousPullPassesNoCredential is the same seam for the common case: the +// handler must forward what the resolver decided, including "nothing", rather than fabricate a +// credential for a public image. +func TestCreatePod_AnonymousPullPassesNoCredential(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + pod := pullPod("nginx:1.27") + h := NewHandler(fp, fake.NewSimpleClientset(pod), nil, openCluster()) + + if err := h.CreatePod(context.Background(), pod); err != nil { + t.Fatalf("CreatePod: %v", err) + } + if fp.lastReq.RegistryAuth != nil { + t.Fatalf("RegistryAuth = %v, want nil for a Pod with no imagePullSecrets", fp.lastReq.RegistryAuth) + } +} + +// TestCreatePod_UnresolvableRegistryAuthIsNonTerminal mirrors +// TestCreatePod_UnresolvableEnvIsNonTerminal for the pull credential, and is why resolution +// runs before the provider call. A credential problem belongs to the Pod, so no other provider +// or region can fix it: nothing is provisioned, nothing is blocklisted, the Pod waits at +// ConfigError, and the error goes back for VK to retry. +func TestCreatePod_UnresolvableRegistryAuthIsNonTerminal(t *testing.T) { + cases := []struct { + name string + pod *corev1.Pod + objs []runtime.Object + }{ + { + // The read failure: the Secret has not landed yet (a bootstrap job, an + // external-secrets sync). Failing the Pod would reap a workload over a race. + name: "the Secret does not exist yet", + pod: pullPod(ecrImage, "ecr-pull"), + }, + { + // Readable, but not a credential for THIS registry — equally not grounds to go + // provision an anonymous pull. + name: "the Secret holds a credential for another registry", + pod: pullPod("ghcr.io/org/app:v1", "hub"), + objs: []runtime.Object{dockerConfigSecret("hub", "docker.io", "u", "hunter2")}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + bl := &recordingBlocklist{} + h := NewHandler(fp, fake.NewSimpleClientset(append(tc.objs, tc.pod)...), bl, openCluster()) + + if err := h.CreatePod(context.Background(), tc.pod); err == nil { + t.Fatal("expected CreatePod to fail so VK retries the sync") + } + if fp.provisionCnt != 0 { + t.Errorf("provision calls = %d, want 0; an unusable credential must not reach the provider", + fp.provisionCnt) + } + if bl.calls != 0 { + t.Errorf("Record calls = %d, want 0; a Pod-spec problem must not blocklist a candidate", bl.calls) + } + if tc.pod.Status.Phase != corev1.PodPending || tc.pod.Status.Reason != reasonConfigError { + t.Errorf("expected Pending/%s, got %s/%s", + reasonConfigError, tc.pod.Status.Phase, tc.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(tc.pod.Namespace, tc.pod.Name) { + t.Error("a pod that never reached the provider must not be tracked") + } + // The reason lands in a status the API server stores and every pod reader can see, + // so it must name the problem without quoting the credential it read. + if strings.Contains(tc.pod.Status.Message, "hunter2") { + t.Errorf("status message leaks a password: %q", tc.pod.Status.Message) + } + }) + } +} + func TestRegistryHost(t *testing.T) { cases := map[string]string{ "nginx:1.27": "docker.io",