diff --git a/internal/cache/save.go b/internal/cache/save.go index 51b5a0a11e..c15b152fd3 100644 --- a/internal/cache/save.go +++ b/internal/cache/save.go @@ -5,6 +5,7 @@ import ( "fmt" "log/slog" "os" + "strings" "time" "github.com/buildkite/agent/v3/api" @@ -394,10 +395,12 @@ func validateCacheStore(storeType, bucketURL string) error { if bucketURL == "" { return fmt.Errorf("BUILDKITE_AGENT_CACHE_STORE_URL must be set for the %s cache store", storeType) } - // Note: We allow both s3:// and file:// for S3 store (file:// is for local testing) - case store.LocalHostedAgents: - if bucketURL != "" { - return fmt.Errorf("NSC store should not have bucket URL set, got: %s", bucketURL) + // s3:// for S3, nsc:// for Namespace artifact storage, and file:// for + // local testing. + switch scheme, _, _ := strings.Cut(bucketURL, "://"); scheme { + case "s3", "nsc", "file": + default: + return fmt.Errorf("unsupported cache store URL scheme %q for the %s cache store", scheme, storeType) } } diff --git a/internal/cache/store/blob.go b/internal/cache/store/blob.go index 5dfec6050f..6feffc6e41 100644 --- a/internal/cache/store/blob.go +++ b/internal/cache/store/blob.go @@ -3,6 +3,7 @@ package store import ( "context" "fmt" + "strings" ) // Blob interface defines the operations for blob storage @@ -17,9 +18,16 @@ type Blob interface { func NewBlobStore(ctx context.Context, store, bucketURL string) (Blob, error) { switch store { case AgentManaged: - return NewS3Blob(ctx, bucketURL) - case LocalHostedAgents: - return NewNscStore() + scheme, _, _ := strings.Cut(bucketURL, "://") + switch scheme { + case nscScheme: + return NewNscStore(bucketURL) + case "file": + // Supported only for local testing, kept consistent with validateCacheStore. + return NewLocalFileBlob(ctx, bucketURL) + default: + return NewS3Blob(ctx, bucketURL) + } case LocalFileStore: return NewLocalFileBlob(ctx, bucketURL) default: diff --git a/internal/cache/store/file_test.go b/internal/cache/store/file_test.go index 04ef65d644..7b9851a5b5 100644 --- a/internal/cache/store/file_test.go +++ b/internal/cache/store/file_test.go @@ -571,3 +571,15 @@ func TestNewBlobStoreLocalFile(t *testing.T) { t.Errorf("expected LocalFileBlob type, got %T", blob) } } + +// file:// is accepted for agent_managed (local testing) and must reach the local +// file store rather than the S3 store, matching validateCacheStore. +func TestNewBlobStoreAgentManagedFile(t *testing.T) { + blob, err := NewBlobStore(t.Context(), AgentManaged, fileURL(t.TempDir())) + if err != nil { + t.Fatalf("NewBlobStore: %v", err) + } + if _, ok := blob.(*LocalFileBlob); !ok { + t.Errorf("expected LocalFileBlob type, got %T", blob) + } +} diff --git a/internal/cache/store/nsc.go b/internal/cache/store/nsc.go index a58deaae8a..7dd20b1dfc 100644 --- a/internal/cache/store/nsc.go +++ b/internal/cache/store/nsc.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "net/url" "os" "os/exec" "path/filepath" @@ -16,13 +17,62 @@ import ( "go.opentelemetry.io/otel/attribute" ) +// nscScheme is the URL scheme that routes an agent-managed cache store to NSC. +const nscScheme = "nsc" + +// nscDefaultExpiry is the default artifact expiry passed to nsc artifact upload. +// Cache entries are content-addressed and short-lived, so we cap storage growth +// rather than relying on NSC's no-expiry default. +// +// TODO: 24h is a temporary stopgap. We ultimately want parity with the S3 path, +// where the expiry can be extended (e.g. refreshed on cache hits) instead of a +// fixed lifetime. +const nscDefaultExpiry = "24h" + +// commandRunner executes an external command. It is a seam so tests can assert +// the arguments passed to the nsc CLI without invoking the real binary. +type commandRunner func(ctx context.Context, workingDir string, args ...string) (*CommandResult, error) + // NscStore implements the Blob interface for NSC artifact storage which uses the nsc CLI tool // https://namespace.so/docs/reference/cli/artifact-download // https://namespace.so/docs/reference/cli/artifact-upload -type NscStore struct{} +type NscStore struct { + // namespace is taken from the nsc:// cache store URL and passed + // as --namespace to the nsc CLI. It is always non-empty. + namespace string + run commandRunner +} + +// NewNscStore creates a store backed by the nsc CLI. The namespace is parsed +// from the nsc:// cache store URL and is required. +func NewNscStore(bucketURL string) (*NscStore, error) { + namespace, err := parseNscNamespace(bucketURL) + if err != nil { + return nil, err + } + return &NscStore{namespace: namespace, run: runCommand}, nil +} + +// parseNscNamespace extracts the namespace from an nsc:// cache store +// URL. It errors on a non-nsc URL or a missing namespace. +func parseNscNamespace(bucketURL string) (string, error) { + u, err := url.Parse(bucketURL) + if err != nil { + return "", fmt.Errorf("invalid cache store URL %q: %w", bucketURL, err) + } + if u.Scheme != nscScheme { + return "", fmt.Errorf("expected %s:// cache store URL, got %q", nscScheme, bucketURL) + } + if u.Host == "" { + return "", fmt.Errorf("nsc:// URL must include a namespace, e.g. nsc://my-namespace") + } + return u.Host, nil +} -func NewNscStore() (*NscStore, error) { - return &NscStore{}, nil +// artifactArgs builds the nsc CLI argument list, including --namespace. +func (n *NscStore) artifactArgs(args ...string) []string { + cmd := append([]string{"nsc", "artifact"}, args...) + return append(cmd, "--namespace", n.namespace) } // validateFilePath validates that a file path is safe for use in commands @@ -98,7 +148,7 @@ func (n *NscStore) Upload(ctx context.Context, filePath, key string) (*TransferI start := time.Now() // Execute nsc artifact upload command - result, err := runCommand(ctx, "", "nsc", "artifact", "upload", filePath, key) + result, err := n.run(ctx, "", n.artifactArgs("upload", filePath, key, "--expires_in", nscDefaultExpiry)...) if err != nil { return nil, fmt.Errorf("failed to execute nsc upload command: %w", err) } @@ -146,7 +196,7 @@ func (n *NscStore) Download(ctx context.Context, key, filePath string) (*Transfe start := time.Now() // Execute nsc artifact download command - result, err := runCommand(ctx, "", "nsc", "artifact", "download", key, filePath) + result, err := n.run(ctx, "", n.artifactArgs("download", key, filePath)...) if err != nil { return nil, fmt.Errorf("failed to execute nsc download command: %w", err) } @@ -200,7 +250,7 @@ func runCommand(ctx context.Context, workingDir string, args ...string) (*Comman // #nosec G204 - args are validated by callers (validateFilePath, validateKey) // and this function is internal to the store package with controlled usage - cmd := exec.Command(args[0], args[1:]...) + cmd := exec.CommandContext(ctx, args[0], args[1:]...) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr diff --git a/internal/cache/store/nsc_test.go b/internal/cache/store/nsc_test.go index 0320ea2817..7257c5f1e9 100644 --- a/internal/cache/store/nsc_test.go +++ b/internal/cache/store/nsc_test.go @@ -1,10 +1,13 @@ package store import ( + "context" "os" "path/filepath" "strings" "testing" + + "github.com/google/go-cmp/cmp" ) func TestNscStore_Interface(t *testing.T) { @@ -211,107 +214,100 @@ func TestRunCommandValidation(t *testing.T) { } } -// TestNscStore_MockUpload tests the Upload method with mocked command execution -// Note: This test will fail if nsc is not installed, but shows the structure -func TestNscStore_Upload_Validation(t *testing.T) { - store, err := NewNscStore() - if err != nil { - t.Fatalf("NewNscStore: %v", err) +func TestParseNscNamespace(t *testing.T) { + tests := []struct { + name string + url string + namespace string + wantErr bool + }{ + {name: "nsc with namespace", url: "nsc://my-namespace", namespace: "my-namespace"}, + {name: "not nsc", url: "s3://my-bucket", wantErr: true}, + {name: "nsc without namespace", url: "nsc://", wantErr: true}, + {name: "invalid url", url: "nsc://host:notaport", wantErr: true}, } - ctx := t.Context() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + namespace, err := parseNscNamespace(tt.url) + if (err != nil) != tt.wantErr { + t.Fatalf("parseNscNamespace(%q) err = %v, wantErr %v", tt.url, err, tt.wantErr) + } + if namespace != tt.namespace { + t.Errorf("parseNscNamespace(%q) namespace = %q, want %q", tt.url, namespace, tt.namespace) + } + }) + } +} - // Create a temporary test file - tmpDir, err := os.MkdirTemp("", "nsc-test") - if err != nil { - t.Fatalf("MkdirTemp: %v", err) +// fakeRunner records the args of the last command and returns a successful result. +func fakeRunner(captured *[]string) commandRunner { + return func(_ context.Context, _ string, args ...string) (*CommandResult, error) { + *captured = args + return &CommandResult{}, nil } - defer func() { _ = os.RemoveAll(tmpDir) }() +} +func TestNscStore_PassesNamespace(t *testing.T) { + ctx := t.Context() + + tmpDir := t.TempDir() testFile := filepath.Join(tmpDir, "test.txt") - err = os.WriteFile(testFile, []byte("test content"), 0o600) - if err != nil { + if err := os.WriteFile(testFile, []byte("test content"), 0o600); err != nil { t.Fatalf("WriteFile: %v", err) } - // Test invalid file path - _, err = store.Upload(ctx, "invalid;path", "valid-key") - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "invalid file path") { - t.Errorf("error %q does not contain %q", err.Error(), "invalid file path") - } + var captured []string + store := &NscStore{namespace: "my-namespace", run: fakeRunner(&captured)} - // Test invalid key - _, err = store.Upload(ctx, testFile, "invalid key with spaces") - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "invalid key") { - t.Errorf("error %q does not contain %q", err.Error(), "invalid key") + if _, err := store.Upload(ctx, testFile, "key"); err != nil { + t.Fatalf("Upload: %v", err) } - // Test valid inputs (will fail with nsc command error, but validation passes) - _, err = store.Upload(ctx, testFile, "valid-key") - // This will error because nsc command likely doesn't exist or isn't configured - // but the error should be about command execution, not validation - if err != nil { - if strings.Contains(err.Error(), "invalid file path") { - t.Errorf("error %q should not contain %q", err.Error(), "invalid file path") - } - if strings.Contains(err.Error(), "invalid key") { - t.Errorf("error %q should not contain %q", err.Error(), "invalid key") - } + wantArgs := []string{"nsc", "artifact", "upload", testFile, "key", "--expires_in", "24h", "--namespace", "my-namespace"} + if diff := cmp.Diff(wantArgs, captured); diff != "" { + t.Errorf("upload args mismatch (-want +got):\n%s", diff) } } -// TestNscStore_Download_Validation tests the Download method validation -func TestNscStore_Download_Validation(t *testing.T) { - store, err := NewNscStore() - if err != nil { - t.Fatalf("NewNscStore: %v", err) +func TestNewNscStore_RequiresNamespace(t *testing.T) { + if _, err := NewNscStore("nsc://"); err == nil { + t.Error(`NewNscStore("nsc://"): expected error, got nil`) } +} +// TestNscStore_ValidationShortCircuits ensures unsafe inputs are rejected before +// the CLI is ever invoked. +func TestNscStore_ValidationShortCircuits(t *testing.T) { ctx := t.Context() + ran := false + store := &NscStore{namespace: "ns", run: func(context.Context, string, ...string) (*CommandResult, error) { + ran = true + return &CommandResult{}, nil + }} - tmpDir, err := os.MkdirTemp("", "nsc-test") - if err != nil { - t.Fatalf("MkdirTemp: %v", err) + if _, err := store.Upload(ctx, "invalid;path", "valid-key"); err == nil { + t.Error("Upload with unsafe path: expected error, got nil") } - defer func() { _ = os.RemoveAll(tmpDir) }() - - destFile := filepath.Join(tmpDir, "download.txt") - - // Test invalid key - _, err = store.Download(ctx, "invalid key with spaces", destFile) - if err == nil { - t.Fatal("expected error, got nil") + if _, err := store.Download(ctx, "invalid key with spaces", "dest.txt"); err == nil { + t.Error("Download with unsafe key: expected error, got nil") } - if !strings.Contains(err.Error(), "invalid key") { - t.Errorf("error %q does not contain %q", err.Error(), "invalid key") + if ran { + t.Error("nsc CLI should not run when input validation fails") } +} - // Test invalid file path - _, err = store.Download(ctx, "valid-key", "invalid;path") - if err == nil { - t.Fatal("expected error, got nil") +func TestNewBlobStore_NscScheme(t *testing.T) { + blob, err := NewBlobStore(t.Context(), AgentManaged, "nsc://my-namespace") + if err != nil { + t.Fatalf("NewBlobStore: %v", err) } - if !strings.Contains(err.Error(), "invalid file path") { - t.Errorf("error %q does not contain %q", err.Error(), "invalid file path") + nsc, ok := blob.(*NscStore) + if !ok { + t.Fatalf("NewBlobStore returned %T, want *NscStore", blob) } - - // Test valid inputs (will fail with nsc command error, but validation passes) - _, err = store.Download(ctx, "valid-key", destFile) - // This will error because nsc command likely doesn't exist or isn't configured - // but the error should be about command execution, not validation - if err != nil { - if strings.Contains(err.Error(), "invalid key") { - t.Errorf("error %q should not contain %q", err.Error(), "invalid key") - } - if strings.Contains(err.Error(), "invalid file path") { - t.Errorf("error %q should not contain %q", err.Error(), "invalid file path") - } + if nsc.namespace != "my-namespace" { + t.Errorf("namespace = %q, want %q", nsc.namespace, "my-namespace") } } @@ -323,7 +319,8 @@ func TestNscStore_Integration(t *testing.T) { t.Skip("Skipping NSC integration test (set NSC_INTEGRATION_TEST=1 to run)") } - store, err := NewNscStore() + // "main" is the nsc CLI's default namespace. + store, err := NewNscStore("nsc://main") if err != nil { t.Fatalf("NewNscStore: %v", err) } diff --git a/internal/cache/store/store.go b/internal/cache/store/store.go index d70a9c4b89..8ef78814f2 100644 --- a/internal/cache/store/store.go +++ b/internal/cache/store/store.go @@ -5,8 +5,6 @@ import ( ) const ( - // local hosted agents store type - LocalHostedAgents = "local_hosted_agents" // local file store type LocalFileStore = "local_file" // agent-managed store type (cache v2): the agent manages the blob store @@ -25,7 +23,7 @@ type TransferInfo struct { func IsValidStore(storeType string) bool { switch storeType { - case LocalHostedAgents, LocalFileStore, AgentManaged: + case LocalFileStore, AgentManaged: return true default: return false