feat(build): make the user-modpack build lane read its context (closes the last functional gap)
A submitted modpack was durable but unreadable: the uploads PVC cannot cross
namespaces (felis-api mounts it; Kaniko runs in felis-build) and the s3 lane
handed the sandboxed build Pod no credentials, so NO user build could ever
consume its context. The transport is now the API itself:
- submit: derived context refs become the internal-face URL
/api/v1/internal/submissions/{id}/context (service-token gated), and Blobs
gains Open (local + s3) with an ErrBlobNotFound sentinel for the route's 404.
- api: serves that route on the internal face only (openapi.yaml updated; the
route-coverage test enforces it).
- build: an http(s) context renders a context-fetch initContainer (the felis
image's new fetch-context entrypoint) that streams the blob with the
namespace-local service-token Secret — never mounted into Kaniko — and
extracts it under a zip-slip guard into a size-limited emptyDir that Kaniko
reads read-only as --context=/context.
- platform/install: the api Deployment carries its own internal base URL; the
build namespace gets the token Secret through the existing replica mechanism
(bootstrap.sh + felis setup); the build egress lock opens exactly the control
namespace on the internal port.
- cmd/felis: fetch-context entrypoint (registered, documented, unit-tested for
escapes/symlinks/non-gzip).
Tests cover rendering, hardening, the s3/local Open paths, and the route's
404/503 mapping. Verified next on the real single-node cluster with Kaniko.
This commit is contained in:
26 files changed
+1080
-72
No files matched your search
@@ -111,5 +111,23 @@ func (s *LocalContextStore) Exists(_ context.Context, id string) (bool, error) {
|
||||
}
|
||||
}
|
||||
|
||||
// Open returns the stored context blob for id — the read side of the transport the
|
||||
// build Pod's fetch initContainer uses. A missing blob is ErrBlobNotFound (404 on
|
||||
// the route), never a bare os error, so the API keeps its status mapping.
|
||||
func (s *LocalContextStore) Open(_ context.Context, id string) (io.ReadCloser, error) {
|
||||
dir, err := s.dir(id)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
f, err := os.Open(filepath.Join(dir, contextBlobName))
|
||||
if err != nil {
|
||||
if os.IsNotExist(err) {
|
||||
return nil, fmt.Errorf("%w: %v", ErrBlobNotFound, err)
|
||||
}
|
||||
return nil, fmt.Errorf("submit: open context blob: %w", err)
|
||||
}
|
||||
return f, nil
|
||||
}
|
||||
|
||||
// Compile-time proof that the filesystem store satisfies the Blobs transport.
|
||||
var _ Blobs = (*LocalContextStore)(nil)
|
||||
@@ -2,6 +2,8 @@ package submit
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"io"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
@@ -54,6 +56,39 @@ func TestLocalContextStorePutAndExists(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Open is the internal context-fetch route's read path: it serves exactly the
|
||||
// stored bytes, and a missing blob is ErrBlobNotFound (404), never a bare os error.
|
||||
func TestLocalContextStoreOpen(t *testing.T) {
|
||||
base := t.TempDir()
|
||||
s := &LocalContextStore{Base: base}
|
||||
ctx := context.Background()
|
||||
|
||||
if _, err := s.Open(ctx, "sub-gone"); !errors.Is(err, ErrBlobNotFound) {
|
||||
t.Fatalf("Open of a missing blob = %v, want ErrBlobNotFound", err)
|
||||
}
|
||||
|
||||
payload := "\x1f\x8b\x08\x00the modpack context"
|
||||
if _, err := s.Put(ctx, "sub-abc", strings.NewReader(payload)); err != nil {
|
||||
t.Fatalf("Put: %v", err)
|
||||
}
|
||||
rc, err := s.Open(ctx, "sub-abc")
|
||||
if err != nil {
|
||||
t.Fatalf("Open: %v", err)
|
||||
}
|
||||
defer rc.Close()
|
||||
got, err := io.ReadAll(rc)
|
||||
if err != nil {
|
||||
t.Fatalf("read: %v", err)
|
||||
}
|
||||
if string(got) != payload {
|
||||
t.Fatalf("Open served %q, want %q", got, payload)
|
||||
}
|
||||
// The same path guard as Put: an id that could escape Base is refused.
|
||||
if _, err := s.Open(ctx, "../etc/passwd"); err == nil {
|
||||
t.Fatal("Open must reject an unsafe id")
|
||||
}
|
||||
}
|
||||
|
||||
func TestLocalContextStorePutOverwrites(t *testing.T) {
|
||||
base := t.TempDir()
|
||||
s := &LocalContextStore{Base: base}
|
||||
|
||||
@@ -14,11 +14,30 @@ import (
|
||||
)
|
||||
|
||||
// s3Client is the minimal object-store surface S3ContextStore needs. *minio.Client
|
||||
// satisfies it, and a fake satisfies it in tests — so the store's key derivation
|
||||
// and not-found handling are unit-verifiable without a live bucket.
|
||||
// satisfies it through minioStoreClient, and a fake satisfies it in tests — so the
|
||||
// store's key derivation and not-found handling are unit-verifiable without a live
|
||||
// bucket.
|
||||
type s3Client interface {
|
||||
PutObject(ctx context.Context, bucket, object string, reader io.Reader, size int64, opts minio.PutObjectOptions) (minio.UploadInfo, error)
|
||||
StatObject(ctx context.Context, bucket, object string, opts minio.StatObjectOptions) (minio.ObjectInfo, error)
|
||||
GetObject(ctx context.Context, bucket, object string, opts minio.GetObjectOptions) (s3Object, error)
|
||||
}
|
||||
|
||||
// s3Object is the handle GetObject yields: a stream whose Stat performs the HEAD
|
||||
// eagerly, so a missing object surfaces before the first byte is read.
|
||||
type s3Object interface {
|
||||
io.ReadCloser
|
||||
Stat() (minio.ObjectInfo, error)
|
||||
}
|
||||
|
||||
// minioStoreClient adapts *minio.Client to s3Client. The adapter exists because a
|
||||
// method's return type cannot be narrowed by an interface: GetObject on the real
|
||||
// client returns a concrete *minio.Object, which does not satisfy a method declared
|
||||
// to return s3Object.
|
||||
type minioStoreClient struct{ *minio.Client }
|
||||
|
||||
func (m minioStoreClient) GetObject(ctx context.Context, bucket, object string, opts minio.GetObjectOptions) (s3Object, error) {
|
||||
return m.Client.GetObject(ctx, bucket, object, opts)
|
||||
}
|
||||
|
||||
// S3ContextStore is the object-store-backed build-context blob store: it writes
|
||||
@@ -27,16 +46,17 @@ type s3Client interface {
|
||||
// selected by cmd/felis when user_uploads_context is an s3:// base.
|
||||
//
|
||||
// The bucket + key prefix are parsed from that same base (parseS3Base), so an
|
||||
// object written here lands at exactly s3://{bucket}/{prefix}/{id}/context.tar.gz —
|
||||
// the ref deriveContextRef records and Kaniko's native s3:// --context reads.
|
||||
// object written here lands at exactly s3://{bucket}/{prefix}/{id}/context.tar.gz.
|
||||
// Credentials are static V4 keys resolved by cmd/felis from the environment (the
|
||||
// setup wizard injects them into felis-api from the felis-uploads-s3 Secret); they
|
||||
// never touch felis.toml.
|
||||
//
|
||||
// Kaniko reading the S3 context at build time needs its own credentials + egress
|
||||
// on the sandboxed build Job — a separate deployment integration, exactly like the
|
||||
// LocalContextStore PVC mount. This transport only makes the upload durable at the
|
||||
// derived location.
|
||||
// The sandboxed build Job never needs S3 credentials of its own: the api reads the
|
||||
// object back here (Open) and streams it over the internal face, which is the
|
||||
// transport every in-cluster build uses (Manager.ContextBaseURL). Only a
|
||||
// deployment that leaves ContextBaseURL empty would fall back to Kaniko reading
|
||||
// s3:// natively — and such a deployment would still need to hand the build Pod
|
||||
// credentials + egress itself.
|
||||
type S3ContextStore struct {
|
||||
client s3Client
|
||||
bucket string
|
||||
@@ -79,7 +99,7 @@ func NewS3ContextStore(cfg S3StoreConfig) (*S3ContextStore, error) {
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("submit: s3 client: %w", err)
|
||||
}
|
||||
return &S3ContextStore{client: client, bucket: bucket, prefix: prefix}, nil
|
||||
return &S3ContextStore{client: minioStoreClient{client}, bucket: bucket, prefix: prefix}, nil
|
||||
}
|
||||
|
||||
// CheckS3Access verifies the S3 coordinates before they are committed to config:
|
||||
@@ -167,6 +187,35 @@ func (s *S3ContextStore) Exists(ctx context.Context, id string) (bool, error) {
|
||||
return true, nil
|
||||
}
|
||||
|
||||
// Open returns the stored context blob for id — the read side of the transport the
|
||||
// build Pod's fetch initContainer uses. minio's GetObject returns only once the
|
||||
// server answered with an object (it surfaces NoSuchKey up front), so a missing
|
||||
// object maps to ErrBlobNotFound right here and the route answers 404.
|
||||
func (s *S3ContextStore) Open(ctx context.Context, id string) (io.ReadCloser, error) {
|
||||
key, err := s.keyFor(id)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
obj, err := s.client.GetObject(ctx, s.bucket, key, minio.GetObjectOptions{})
|
||||
if err != nil {
|
||||
if isS3NotFound(err) {
|
||||
return nil, fmt.Errorf("%w: %v", ErrBlobNotFound, err)
|
||||
}
|
||||
return nil, fmt.Errorf("submit: open context blob: %w", err)
|
||||
}
|
||||
// minio.Object is lazy: the first Read triggers the GET and is where a missing
|
||||
// key actually surfaces, so stat it once here to translate that case eagerly
|
||||
// (the caller can then trust the io.ReadCloser belongs to a real object).
|
||||
if _, err := obj.Stat(); err != nil {
|
||||
_ = obj.Close()
|
||||
if isS3NotFound(err) {
|
||||
return nil, fmt.Errorf("%w: %v", ErrBlobNotFound, err)
|
||||
}
|
||||
return nil, fmt.Errorf("submit: open context blob: %w", err)
|
||||
}
|
||||
return obj, nil
|
||||
}
|
||||
|
||||
// isS3NotFound recognizes the "object is absent" outcome across S3
|
||||
// implementations: a GET-shaped NoSuchKey code or a bare 404 from the HEAD that
|
||||
// StatObject issues.
|
||||
|
||||
@@ -45,6 +45,41 @@ func (f *fakeS3) StatObject(_ context.Context, bucket, object string, _ minio.St
|
||||
return minio.ObjectInfo{}, minio.ErrorResponse{Code: "NoSuchKey", StatusCode: http.StatusNotFound}
|
||||
}
|
||||
|
||||
// fakeS3Object is the object handle fakeS3.GetObject yields: Stat mirrors
|
||||
// StatObject's not-found behaviour, Read serves the stored bytes.
|
||||
type fakeS3Object struct {
|
||||
data []byte
|
||||
err error
|
||||
}
|
||||
|
||||
func (o *fakeS3Object) Read(p []byte) (int, error) {
|
||||
if o.err != nil {
|
||||
return 0, o.err
|
||||
}
|
||||
if len(o.data) == 0 {
|
||||
return 0, io.EOF
|
||||
}
|
||||
n := copy(p, o.data)
|
||||
o.data = o.data[n:]
|
||||
return n, nil
|
||||
}
|
||||
|
||||
func (o *fakeS3Object) Close() error { return nil }
|
||||
|
||||
func (o *fakeS3Object) Stat() (minio.ObjectInfo, error) {
|
||||
if o.err != nil {
|
||||
return minio.ObjectInfo{}, o.err
|
||||
}
|
||||
return minio.ObjectInfo{Size: int64(len(o.data))}, nil
|
||||
}
|
||||
|
||||
func (f *fakeS3) GetObject(_ context.Context, bucket, object string, _ minio.GetObjectOptions) (s3Object, error) {
|
||||
if data, ok := f.objects[bucket+"/"+object]; ok {
|
||||
return &fakeS3Object{data: append([]byte(nil), data...)}, nil
|
||||
}
|
||||
return &fakeS3Object{err: minio.ErrorResponse{Code: "NoSuchKey", StatusCode: http.StatusNotFound}}, nil
|
||||
}
|
||||
|
||||
func TestCheckBucketAccess(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
@@ -107,6 +142,34 @@ func TestS3ContextStorePutAndExists(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Open serves the stored object's bytes and maps a missing key to ErrBlobNotFound
|
||||
// (the internal fetch route's 404), eagerly — before the caller reads a byte.
|
||||
func TestS3ContextStoreOpen(t *testing.T) {
|
||||
fake := &fakeS3{}
|
||||
s := &S3ContextStore{client: fake, bucket: "felis-uploads", prefix: "builds"}
|
||||
ctx := context.Background()
|
||||
|
||||
if _, err := s.Open(ctx, "sub-gone"); !errors.Is(err, ErrBlobNotFound) {
|
||||
t.Fatalf("Open of a missing object = %v, want ErrBlobNotFound", err)
|
||||
}
|
||||
payload := "\x1f\x8b\x08\x00the modpack context"
|
||||
if _, err := s.Put(ctx, "sub-abc", strings.NewReader(payload)); err != nil {
|
||||
t.Fatalf("Put: %v", err)
|
||||
}
|
||||
rc, err := s.Open(ctx, "sub-abc")
|
||||
if err != nil {
|
||||
t.Fatalf("Open: %v", err)
|
||||
}
|
||||
defer rc.Close()
|
||||
got, err := io.ReadAll(rc)
|
||||
if err != nil {
|
||||
t.Fatalf("read: %v", err)
|
||||
}
|
||||
if string(got) != payload {
|
||||
t.Fatalf("Open served %q, want %q", got, payload)
|
||||
}
|
||||
}
|
||||
|
||||
func TestS3ContextStoreEmptyPrefix(t *testing.T) {
|
||||
fake := &fakeS3{}
|
||||
s := &S3ContextStore{client: fake, bucket: "b", prefix: ""}
|
||||
|
||||
@@ -37,7 +37,8 @@
|
||||
// chosen ref would let an untrusted origin point the build at an arbitrary
|
||||
// source. By deriving it from the submission id the user selects nothing
|
||||
// that reaches the executor — only the modpack blob behind the pinned,
|
||||
// id-namespaced location (uploaded by a separate, deferred transport).
|
||||
// id-namespaced location (uploaded through the Blobs transport, and served
|
||||
// back to the build Pod over the service-token-gated internal face).
|
||||
//
|
||||
// Source of truth. submissions is a NEW Postgres business-truth domain, added to
|
||||
// §1 invariant 2's enumeration (owner/claim/accounts/audit/quota/images/builds/
|
||||
@@ -91,6 +92,10 @@ var (
|
||||
// an honest 503, never a 500, exactly as the restore executor does when its
|
||||
// integration is not wired.
|
||||
ErrUploadsUnavailable = errors.New("submit: context upload transport not configured")
|
||||
// ErrBlobNotFound reports that a submission has no stored context blob (or it
|
||||
// was never uploaded). The internal context-fetch route maps it to 404, the
|
||||
// same distinction Exists draws for Approve.
|
||||
ErrBlobNotFound = errors.New("submit: context blob not found")
|
||||
)
|
||||
|
||||
// invalidf wraps ErrInvalid so every malformed-request case maps to one 400.
|
||||
@@ -176,11 +181,12 @@ type Builds interface {
|
||||
|
||||
// Blobs is the build-context blob transport the lane depends on to place a
|
||||
// submitter's uploaded modpack at the platform-derived, id-namespaced location
|
||||
// deriveContextRef points Kaniko at. It is the piece the package doc calls a
|
||||
// "separate, deferred transport": creation only derives and records the ref, and
|
||||
// the bytes behind it arrive through Put here. It is an interface so the Manager
|
||||
// is unit-tested against an in-memory fake; the production implementation is the
|
||||
// filesystem-backed LocalContextStore.
|
||||
// deriveContextRef points Kaniko at. Creation only derives and records the ref;
|
||||
// the bytes behind it arrive through Put here, and the build Pod reads them back
|
||||
// through Open (the manager exposes it as OpenContext, which the API's internal
|
||||
// context route serves). It is an interface so the Manager is unit-tested against
|
||||
// an in-memory fake; the production implementations are LocalContextStore
|
||||
// (filesystem) and S3ContextStore (object store).
|
||||
//
|
||||
// Both methods key off the submission id, never a caller-supplied path, so the
|
||||
// write target is as platform-pinned as the derived ref itself. Put stores (and
|
||||
@@ -190,6 +196,11 @@ type Builds interface {
|
||||
type Blobs interface {
|
||||
Put(ctx context.Context, id string, r io.Reader) (int64, error)
|
||||
Exists(ctx context.Context, id string) (bool, error)
|
||||
// Open returns the stored blob's bytes for the internal context-fetch route
|
||||
// the build Pod's initContainer dials (cmd/felis fetch-context). It returns an
|
||||
// error wrapping ErrBlobNotFound when no blob exists, so the route can answer
|
||||
// 404 without leaking which ids do exist.
|
||||
Open(ctx context.Context, id string) (io.ReadCloser, error)
|
||||
}
|
||||
|
||||
// Manager orchestrates the approval lane. It holds no mutable state; the clock
|
||||
@@ -210,6 +221,15 @@ type Manager struct {
|
||||
// "s3://felis-user-uploads" (an object store) or a local uploads PVC path. The
|
||||
// derived context ref is {ContextStore}/{id}/context.tar.gz.
|
||||
ContextStore string
|
||||
// ContextBaseURL, when set, is the platform's internal-face base URL
|
||||
// (platform.InternalAPIBaseURL). It makes the derived context ref an HTTP URL
|
||||
// on that face — {ContextBaseURL}/api/v1/internal/submissions/{id}/context —
|
||||
// instead of a filesystem/object-store location: the build Pod cannot mount
|
||||
// the uploads PVC (builds run in another namespace) and carries no object-store
|
||||
// credentials, so the API streams the blob it stored at ContextStore over the
|
||||
// service-token-gated internal face. Empty keeps the legacy ref shape for a
|
||||
// deployment that predates the transport.
|
||||
ContextBaseURL string
|
||||
// Blobs is the upload transport that persists the modpack behind the derived
|
||||
// context ref. When nil (a store with no implemented transport, e.g. an
|
||||
// object-store base with no client), UploadContext returns ErrUploadsUnavailable
|
||||
@@ -269,9 +289,23 @@ func (m *Manager) deriveImageRef(id string) string {
|
||||
// selects nothing that reaches Kaniko's --context argument; only the blob behind
|
||||
// this pinned, id-namespaced location (placed by the upload transport) varies.
|
||||
func (m *Manager) deriveContextRef(id string) string {
|
||||
if m.ContextBaseURL != "" {
|
||||
return fmt.Sprintf("%s/api/v1/internal/submissions/%s/context", strings.TrimRight(m.ContextBaseURL, "/"), id)
|
||||
}
|
||||
return fmt.Sprintf("%s/%s/%s", strings.TrimRight(m.ContextStore, "/"), id, contextBlobName)
|
||||
}
|
||||
|
||||
// OpenContext returns the stored build context for id — the read path behind the
|
||||
// internal context-fetch route. It requires the upload transport (Blobs): with no
|
||||
// transport there is no blob to read, so it reports ErrUploadsUnavailable, the
|
||||
// same honest 503 the upload endpoint gives.
|
||||
func (m *Manager) OpenContext(ctx context.Context, id string) (io.ReadCloser, error) {
|
||||
if m.Blobs == nil {
|
||||
return nil, ErrUploadsUnavailable
|
||||
}
|
||||
return m.Blobs.Open(ctx, id)
|
||||
}
|
||||
|
||||
// auditDockerfile is the audit-archive Dockerfile recorded on the build row. It
|
||||
// is NOT what Kaniko executes — Kaniko reads the real Dockerfile from inside the
|
||||
// uploaded context (build/jobspec.go) — so this honestly documents the
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
package submit
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"strings"
|
||||
"testing"
|
||||
@@ -50,6 +52,14 @@ func (f *fakeBlobs) Exists(_ context.Context, id string) (bool, error) {
|
||||
return ok, nil
|
||||
}
|
||||
|
||||
func (f *fakeBlobs) Open(_ context.Context, id string) (io.ReadCloser, error) {
|
||||
b, ok := f.stored[id]
|
||||
if !ok {
|
||||
return nil, fmt.Errorf("%w: no blob for %s", ErrBlobNotFound, id)
|
||||
}
|
||||
return io.NopCloser(bytes.NewReader(b)), nil
|
||||
}
|
||||
|
||||
// testNow is the frozen clock for hermetic assertions.
|
||||
var testNow = time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC)
|
||||
|
||||
@@ -217,6 +227,52 @@ func TestCreatePendingDoesNotBuild(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// With a ContextBaseURL the derived ref is the internal-face URL the build Pod's
|
||||
// fetch initContainer dials — not a filesystem path it could never read across
|
||||
// namespaces. OpenContext then serves whatever the Blobs transport stored.
|
||||
func TestContextRefIsFetchURLAndOpenContextServesIt(t *testing.T) {
|
||||
m, _, _ := newManager()
|
||||
m.ContextBaseURL = "http://felis-api-internal.felis.svc.cluster.local:8081/"
|
||||
m.Blobs = newFakeBlobs()
|
||||
ctx := context.Background()
|
||||
|
||||
sub, err := m.Create(ctx, CreateRequest{DisplayName: "Pack", SubmittedBy: "user-1"})
|
||||
if err != nil {
|
||||
t.Fatalf("Create: %v", err)
|
||||
}
|
||||
want := "http://felis-api-internal.felis.svc.cluster.local:8081/api/v1/internal/submissions/sub-1/context"
|
||||
if sub.ContextRef != want {
|
||||
t.Fatalf("context_ref = %q, want the internal fetch URL %q", sub.ContextRef, want)
|
||||
}
|
||||
|
||||
// Before any upload the read path reports not-found (the route's 404).
|
||||
if _, err := m.OpenContext(ctx, sub.ID); !errors.Is(err, ErrBlobNotFound) {
|
||||
t.Fatalf("OpenContext before upload = %v, want ErrBlobNotFound", err)
|
||||
}
|
||||
payload := "\x1f\x8b\x08\x00payload"
|
||||
if _, err := m.UploadContext(ctx, sub.ID, "user-1", strings.NewReader(payload)); err != nil {
|
||||
t.Fatalf("UploadContext: %v", err)
|
||||
}
|
||||
rc, err := m.OpenContext(ctx, sub.ID)
|
||||
if err != nil {
|
||||
t.Fatalf("OpenContext: %v", err)
|
||||
}
|
||||
defer rc.Close()
|
||||
got, _ := io.ReadAll(rc)
|
||||
if string(got) != payload {
|
||||
t.Fatalf("OpenContext served %q, want %q", got, payload)
|
||||
}
|
||||
}
|
||||
|
||||
// No upload transport ⇒ no readable blob: the route reports the same 503 the
|
||||
// upload endpoint does, rather than a misleading 404.
|
||||
func TestOpenContextWithoutTransportIsUnavailable(t *testing.T) {
|
||||
m, _, _ := newManager()
|
||||
if _, err := m.OpenContext(context.Background(), "sub-1"); !errors.Is(err, ErrUploadsUnavailable) {
|
||||
t.Fatalf("OpenContext with nil Blobs = %v, want ErrUploadsUnavailable", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCreateValidation(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
|
||||
Reference in new issue
Block a user