diff --git a/internal/orca/inttest/client.go b/internal/orca/inttest/client.go index b453152e..b9f67afa 100644 --- a/internal/orca/inttest/client.go +++ b/internal/orca/inttest/client.go @@ -7,6 +7,7 @@ package inttest import ( "context" + "encoding/xml" "fmt" "io" "net/http" @@ -36,6 +37,32 @@ type GetResponse struct { Body []byte } +// S3Error is the parsed shape of an S3 envelope as returned +// by orca's edge handler on non-2xx responses. Field names mirror the +// AWS S3 REST API. +type S3Error struct { + XMLName xml.Name `xml:"Error"` + Code string `xml:"Code"` + Message string `xml:"Message"` + Resource string `xml:"Resource,omitempty"` + BucketName string `xml:"BucketName,omitempty"` + Key string `xml:"Key,omitempty"` +} + +// ParseS3Error decodes r.Body as an S3 envelope. Fails the +// test on parse error; callers should already know the response was +// non-2xx. +func (r GetResponse) ParseS3Error(t *testing.T) S3Error { + t.Helper() + + var e S3Error + if err := xml.Unmarshal(r.Body, &e); err != nil { + t.Fatalf("parse S3 error: %v; body=%q", err, string(r.Body)) + } + + return e +} + // Get fetches the full body of /bucket/key. func (c *Client) Get(ctx context.Context, t *testing.T, bucket, key string) GetResponse { t.Helper() @@ -60,6 +87,16 @@ func (c *Client) Head(ctx context.Context, t *testing.T, bucket, key string) Get return c.do(ctx, t, http.MethodHead, fmt.Sprintf("/%s/%s", bucket, key), nil) } +// Do issues a raw request against an arbitrary path. Used by tests +// that exercise non-object surfaces (e.g. GET /bucket/ -> 501) or +// that need to send headers (e.g. an out-of-range Range) the typed +// helpers do not provide. +func (c *Client) Do(ctx context.Context, t *testing.T, method, path string, hdr http.Header) GetResponse { + t.Helper() + + return c.do(ctx, t, method, path, hdr) +} + func (c *Client) do(ctx context.Context, t *testing.T, method, path string, hdr http.Header) GetResponse { t.Helper() diff --git a/internal/orca/inttest/error_test.go b/internal/orca/inttest/error_test.go new file mode 100644 index 00000000..d2972089 --- /dev/null +++ b/internal/orca/inttest/error_test.go @@ -0,0 +1,119 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//go:build integrationtest + +package inttest + +import ( + "context" + "net/http" + "testing" + "time" +) + +// TestS3Errors exercises orca's edge error surface on the wire (raw +// net/http requests). It verifies that the S3-compatible XML +// envelope, status codes, and HEAD-no-body behavior reach a real +// HTTP client correctly. Companion suite TestS3SDK (s3sdk_test.go) +// re-checks the same surface through an actual S3 SDK so we know +// SDKs unmarshal the envelope into typed errors. +// +// A single cluster is shared across all subtests; subtests are +// independent and run in parallel. +func TestS3Errors(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 120*time.Second) + // Parallel subtests are paused until the parent returns, then + // resumed; a `defer cancel()` would cancel the context before + // they ever run. Use t.Cleanup so the cancel fires after all + // subtests finish instead. + t.Cleanup(cancel) + + bucket := pkgLocalStack.NewBucket(ctx, t, "orca-origin") + blob := SmallBlob() + SeedS3(ctx, t, pkgLocalStack.NewS3Client(ctx, t), bucket, []SeedBlob{blob}) + + cl := StartCluster(ctx, t, ClusterOptions{ + LocalStack: pkgLocalStack, + OriginBucket: bucket, + }) + + httpClient := cl.Get(1).HTTP + + // NoSuchKey_XML: GET against a missing object returns 404 with + // the standard S3 envelope and Code "NoSuchKey". + t.Run("NoSuchKey_XML", func(t *testing.T) { + t.Parallel() + + resp := httpClient.Get(ctx, t, bucket, "does-not-exist") + if resp.Status != http.StatusNotFound { + t.Fatalf("status=%d want 404; body=%s", resp.Status, string(resp.Body)) + } + + if got := resp.Header.Get("Content-Type"); got != "application/xml" { + t.Errorf("Content-Type=%q want application/xml", got) + } + + e := resp.ParseS3Error(t) + if e.Code != "NoSuchKey" { + t.Errorf("Code=%q want NoSuchKey", e.Code) + } + + if e.Message == "" { + t.Error("Message is empty") + } + }) + + // HeadNoSuchKey_NoBody: HEAD against a missing object returns + // 404 with an empty body, mirroring real S3 (HEAD must not have + // a body per RFC 9110; SDKs key off the status code). + t.Run("HeadNoSuchKey_NoBody", func(t *testing.T) { + t.Parallel() + + resp := httpClient.Head(ctx, t, bucket, "does-not-exist") + if resp.Status != http.StatusNotFound { + t.Fatalf("status=%d want 404", resp.Status) + } + + if len(resp.Body) != 0 { + t.Errorf("HEAD body must be empty; got %d bytes: %q", len(resp.Body), string(resp.Body)) + } + }) + + // InvalidRange_XML: an unsatisfiable Range returns 416 with + // Code "InvalidRange". + t.Run("InvalidRange_XML", func(t *testing.T) { + t.Parallel() + + hdr := http.Header{} + // Start position past EOF: unambiguously unsatisfiable per RFC. + hdr.Set("Range", "bytes=99999999-") + + resp := httpClient.Do(ctx, t, http.MethodGet, "/"+bucket+"/"+blob.Key, hdr) + if resp.Status != http.StatusRequestedRangeNotSatisfiable { + t.Fatalf("status=%d want 416; body=%s", resp.Status, string(resp.Body)) + } + + e := resp.ParseS3Error(t) + if e.Code != "InvalidRange" { + t.Errorf("Code=%q want InvalidRange", e.Code) + } + }) + + // ListObjectsV2_NotImplemented: GET against a bucket root + // returns 501 with Code "NotImplemented" and a message naming + // ListObjectsV2 (per the routing split in EdgeHandler.ServeHTTP). + t.Run("ListObjectsV2_NotImplemented", func(t *testing.T) { + t.Parallel() + + resp := httpClient.Do(ctx, t, http.MethodGet, "/"+bucket+"/", nil) + if resp.Status != http.StatusNotImplemented { + t.Fatalf("status=%d want 501; body=%s", resp.Status, string(resp.Body)) + } + + e := resp.ParseS3Error(t) + if e.Code != "NotImplemented" { + t.Errorf("Code=%q want NotImplemented", e.Code) + } + }) +} diff --git a/internal/orca/inttest/s3sdk_test.go b/internal/orca/inttest/s3sdk_test.go new file mode 100644 index 00000000..4c8b1f25 --- /dev/null +++ b/internal/orca/inttest/s3sdk_test.go @@ -0,0 +1,306 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//go:build integrationtest + +package inttest + +import ( + "bytes" + "context" + "errors" + "io" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + awsconfig "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" + "github.com/aws/aws-sdk-go-v2/service/s3" + s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" + "github.com/aws/smithy-go" +) + +// newOrcaS3Client returns an aws-sdk-go-v2 S3 client configured to +// talk to orca's edge listener. Static dummy credentials are supplied +// because the SDK refuses to run without any, but orca currently +// ignores the Authorization header (auth.enabled=false). Path-style +// addressing is required: orca does not implement virtual-hosted +// bucket parsing. Retries are disabled so test failures surface +// immediately rather than after exponential backoff. +// +// Checksum opt-out mirrors the same setting used by orca's own +// awss3 origin adapter: aws-sdk-go-v2 added default checksum +// computation that some servers (including orca's edge, which does +// not echo CRC headers) reject or ignore in confusing ways. +func newOrcaS3Client(t *testing.T, baseURL string) *s3.Client { + t.Helper() + + cfg, err := awsconfig.LoadDefaultConfig(t.Context(), + awsconfig.WithRegion("us-east-1"), + awsconfig.WithCredentialsProvider( + credentials.NewStaticCredentialsProvider("orca-test", "orca-test", ""), + ), + ) + if err != nil { + t.Fatalf("aws config: %v", err) + } + + return s3.NewFromConfig(cfg, func(o *s3.Options) { + o.BaseEndpoint = aws.String(baseURL) + o.UsePathStyle = true + o.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired + o.ResponseChecksumValidation = aws.ResponseChecksumValidationWhenRequired + o.RetryMaxAttempts = 1 + }) +} + +// TestS3SDK verifies that orca's edge surface is consumable by a +// real S3 SDK (aws-sdk-go-v2). This is the headline contract test +// for the XML envelope: TestS3Errors confirms the bytes on +// the wire are well-formed, while this suite confirms the SDK +// successfully unmarshals them into typed errors (*s3types.NoSuchKey, +// *s3types.NotFound) or surfaces the Code via smithy.APIError. +// +// Regression here means external S3 clients (CLI, boto3, MinIO, +// Java SDK, etc.) cannot do typed error handling against orca even +// though the response bytes look correct. +// +// A single cluster is shared across all subtests; subtests are +// independent and run in parallel. +func TestS3SDK(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 120*time.Second) + // Parallel subtests are paused until the parent returns, then + // resumed; a `defer cancel()` would cancel the context before + // they ever run. Use t.Cleanup so the cancel fires after all + // subtests finish instead. + t.Cleanup(cancel) + + bucket := pkgLocalStack.NewBucket(ctx, t, "orca-origin") + blob := SmallBlob() + SeedS3(ctx, t, pkgLocalStack.NewS3Client(ctx, t), bucket, []SeedBlob{blob}) + + cl := StartCluster(ctx, t, ClusterOptions{ + LocalStack: pkgLocalStack, + OriginBucket: bucket, + }) + + client := newOrcaS3Client(t, cl.Get(1).HTTP.BaseURL) + + // GetObject_Success: positive control. If this fails the rest + // of the suite's error assertions cannot be trusted. + t.Run("GetObject_Success", func(t *testing.T) { + t.Parallel() + + out, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(blob.Key), + }) + if err != nil { + t.Fatalf("GetObject: %v", err) + } + defer out.Body.Close() //nolint:errcheck // body close best-effort in tests + + body, err := io.ReadAll(out.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + + if !bytes.Equal(body, blob.Data) { + t.Fatalf("body mismatch: got %d bytes, want %d", len(body), len(blob.Data)) + } + + if out.ETag == nil || *out.ETag == "" { + t.Error("ETag missing on successful GetObject") + } + }) + + // HeadObject_Size_TypedFields: verifies HEAD on a present + // object returns the correct object size via the SDK's typed + // *out.ContentLength field. Prefetchers and range planners + // depend on this; a regression in setObjectHeaders or the + // Content-Length formatting in handleHead would silently break + // clients that key off the typed field rather than re-parsing + // the header. + t.Run("HeadObject_Size_TypedFields", func(t *testing.T) { + t.Parallel() + + out, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(blob.Key), + }) + if err != nil { + t.Fatalf("HeadObject: %v", err) + } + + if out.ContentLength == nil { + t.Fatal("ContentLength is nil") + } + + if got, want := *out.ContentLength, int64(len(blob.Data)); got != want { + t.Errorf("ContentLength=%d want %d", got, want) + } + + if out.ETag == nil || *out.ETag == "" { + t.Error("ETag missing on HeadObject success") + } + }) + + // GetObject_NoSuchKey_TypedError: the headline assertion. SDK + // must surface *s3types.NoSuchKey, which requires orca to emit + // a well-formed NoSuchKey body with + // the right Content-Type. + t.Run("GetObject_NoSuchKey_TypedError", func(t *testing.T) { + t.Parallel() + + _, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String("does-not-exist"), + }) + if err == nil { + t.Fatal("GetObject on missing key returned no error") + } + + var nsk *s3types.NoSuchKey + if !errors.As(err, &nsk) { + t.Fatalf("err is not *s3types.NoSuchKey: %T: %v", err, err) + } + }) + + // HeadObject_NotFound_TypedError: HEAD has an empty body, so + // the SDK relies on the 404 status to surface *s3types.NotFound. + // This is the contract test for the HEAD-no-body branch in + // writeS3Error: if we ever accidentally write a body on HEAD, + // some SDKs would fail to parse the (empty) Content-Type + // response and surface a generic error instead. + t.Run("HeadObject_NotFound_TypedError", func(t *testing.T) { + t.Parallel() + + _, err := client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String("does-not-exist"), + }) + if err == nil { + t.Fatal("HeadObject on missing key returned no error") + } + + var nf *s3types.NotFound + if !errors.As(err, &nf) { + t.Fatalf("err is not *s3types.NotFound: %T: %v", err, err) + } + }) + + // GetObject_InvalidRange_APICode: there is no typed + // *s3types.InvalidRange error in aws-sdk-go-v2, so SDK callers + // extract the code via smithy.APIError. Verify the Code reaches + // them intact. + t.Run("GetObject_InvalidRange_APICode", func(t *testing.T) { + t.Parallel() + + _, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(blob.Key), + Range: aws.String("bytes=99999999-"), + }) + if err == nil { + t.Fatal("GetObject with out-of-range Range returned no error") + } + + var apiErr smithy.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("err is not smithy.APIError: %T: %v", err, err) + } + + if apiErr.ErrorCode() != "InvalidRange" { + t.Errorf("ErrorCode=%q want InvalidRange", apiErr.ErrorCode()) + } + }) + + // GetObject_RangeRequest: positive range; verifies the SDK's + // success-path 206 handling round-trips correctly through + // orca's range slicing. + t.Run("GetObject_RangeRequest", func(t *testing.T) { + t.Parallel() + + const n = 100 + out, err := client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(blob.Key), + Range: aws.String("bytes=0-99"), + }) + if err != nil { + t.Fatalf("GetObject range: %v", err) + } + defer out.Body.Close() //nolint:errcheck // body close best-effort in tests + + body, err := io.ReadAll(out.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + + if !bytes.Equal(body, blob.Data[:n]) { + t.Fatalf("range body mismatch: got %d bytes, want %d", len(body), n) + } + + if out.ContentRange == nil || !strings.HasPrefix(*out.ContentRange, "bytes 0-99/") { + t.Errorf("ContentRange=%v want bytes 0-99/...", out.ContentRange) + } + }) + + // ListObjectsV2_NotImplemented: verifies SDK surfaces orca's + // 501 NotImplemented response for the bucket-level GET via + // smithy.APIError with Code=NotImplemented. + // + // We intentionally do not assert ErrorFault: aws-sdk-go-v2 + // classifies the fault from the (untyped) deserialized error, + // which for our XML envelope produces Fault=unknown rather than + // server. The status code (501) is what S3 client retry + // classifiers actually consult, and that path is exercised via + // TestS3Errors. + t.Run("ListObjectsV2_NotImplemented", func(t *testing.T) { + t.Parallel() + + _, err := client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + }) + if err == nil { + t.Fatal("ListObjectsV2 returned no error") + } + + var apiErr smithy.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("err is not smithy.APIError: %T: %v", err, err) + } + + if apiErr.ErrorCode() != "NotImplemented" { + t.Errorf("ErrorCode=%q want NotImplemented", apiErr.ErrorCode()) + } + }) + + // PutObject_MethodNotAllowed: orca is read-only. Verify the + // SDK surfaces 405 MethodNotAllowed cleanly so write attempts + // fail with a recognizable code rather than a parse error or + // silent retry storm. + t.Run("PutObject_MethodNotAllowed", func(t *testing.T) { + t.Parallel() + + _, err := client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String("new-object"), + Body: bytes.NewReader([]byte("hello")), + }) + if err == nil { + t.Fatal("PutObject returned no error against read-only orca") + } + + var apiErr smithy.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("err is not smithy.APIError: %T: %v", err, err) + } + + if apiErr.ErrorCode() != "MethodNotAllowed" { + t.Errorf("ErrorCode=%q want MethodNotAllowed", apiErr.ErrorCode()) + } + }) +} diff --git a/internal/orca/server/s3error.go b/internal/orca/server/s3error.go new file mode 100644 index 00000000..61ad7a02 --- /dev/null +++ b/internal/orca/server/s3error.go @@ -0,0 +1,111 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package server + +import ( + "encoding/xml" + "net/http" +) + +// S3 standard error code names mirrored from the AWS S3 REST API. +// SDKs (aws-sdk-go-v2, boto3, MinIO client, etc.) branch on the +// element of the error envelope below to surface a typed +// error to callers. Using the canonical AWS names means existing S3 +// client code "just works" against orca's edge surface. +// +// See: +// https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html +const ( + s3ErrNoSuchKey = "NoSuchKey" + s3ErrInvalidRange = "InvalidRange" + s3ErrMethodNotAllowed = "MethodNotAllowed" + s3ErrNotImplemented = "NotImplemented" + s3ErrAccessDenied = "AccessDenied" + + // Orca-specific extension codes. AWS S3 does not define these, + // but the response shape is still a standard envelope so + // SDKs that look only at the HTTP status (502) for retry + // classification continue to behave correctly. Operators reading + // orca logs can use the Code to distinguish failure modes. + s3ErrOriginUnauthorized = "OriginUnauthorized" + s3ErrOriginUnsupported = "OriginUnsupported" + s3ErrOriginETagChanged = "OriginETagChanged" + s3ErrOriginMissingETag = "OriginMissingETag" + s3ErrOriginUnreachable = "OriginUnreachable" +) + +// s3ErrorBody is the standard envelope returned by AWS S3 on +// non-2xx responses. Field order matches the AWS documented response. +// Optional fields are omitted when empty. +// +// Orca does not populate RequestId/HostId; SDKs tolerate their +// absence and there is presently no operational use for them in a +// single-cluster deployment. +type s3ErrorBody struct { + XMLName xml.Name `xml:"Error"` + Code string `xml:"Code"` + Message string `xml:"Message"` + Resource string `xml:"Resource,omitempty"` + BucketName string `xml:"BucketName,omitempty"` + Key string `xml:"Key,omitempty"` +} + +// s3ErrorOpt customizes optional fields on the error envelope. +type s3ErrorOpt func(*s3ErrorBody) + +// withBucketKey sets the and elements. Either may +// be empty. +func withBucketKey(bucket, key string) s3ErrorOpt { + return func(b *s3ErrorBody) { + b.BucketName = bucket + b.Key = key + } +} + +// writeS3Error writes an S3-compatible error response. +// +// For HEAD requests the body is suppressed (mirroring real S3 +// behavior: HEAD responses MUST NOT include a body per RFC 9110, and +// AWS S3 communicates the failure entirely via the status code and +// response headers). +// +// For all other methods an XML envelope is written with the +// supplied Code and Message. The envelope shape is the one S3 SDKs +// parse to extract a typed error code. +func writeS3Error(w http.ResponseWriter, r *http.Request, status int, code, message string, opts ...s3ErrorOpt) { + w.Header().Set("Server", "orca") + + if r != nil && r.Method == http.MethodHead { + // HEAD: status code + headers only. + w.WriteHeader(status) + return + } + + body := s3ErrorBody{Code: code, Message: message} + if r != nil { + body.Resource = r.URL.Path + } + + for _, opt := range opts { + opt(&body) + } + + xmlBytes, err := xml.Marshal(body) + if err != nil { + // Should be unreachable: s3ErrorBody is a fixed struct with + // only string fields. Fall back to plain text on the off + // chance the stdlib ever surprises us so the client at least + // receives a meaningful status code + message. + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(status) + _, _ = w.Write([]byte(code + ": " + message)) //nolint:errcheck // best-effort fallback + + return + } + + w.Header().Set("Content-Type", "application/xml") + w.WriteHeader(status) + _, _ = w.Write([]byte(xml.Header)) //nolint:errcheck // body write best-effort + _, _ = w.Write(xmlBytes) //nolint:errcheck // body write best-effort +} diff --git a/internal/orca/server/s3error_test.go b/internal/orca/server/s3error_test.go new file mode 100644 index 00000000..09c040cd --- /dev/null +++ b/internal/orca/server/s3error_test.go @@ -0,0 +1,135 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package server + +import ( + "encoding/xml" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// parseS3Error parses the body of a httptest.ResponseRecorder as an +// S3 envelope. Test helper; fails the test on parse error. +func parseS3Error(t *testing.T, rr *httptest.ResponseRecorder) s3ErrorBody { + t.Helper() + + body := rr.Body.Bytes() + if !strings.HasPrefix(string(body), xml.Header) { + t.Errorf("body missing XML declaration; got %q", string(body)) + } + + var e s3ErrorBody + if err := xml.Unmarshal(body, &e); err != nil { + t.Fatalf("unmarshal s3 error: %v; body=%q", err, string(body)) + } + + return e +} + +func TestWriteS3Error_GET(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + rr := httptest.NewRecorder() + + writeS3Error(rr, req, http.StatusNotFound, s3ErrNoSuchKey, + "The specified key does not exist.", + withBucketKey("bucket", "key")) + + if rr.Code != http.StatusNotFound { + t.Errorf("status=%d want %d", rr.Code, http.StatusNotFound) + } + + if got := rr.Header().Get("Content-Type"); got != "application/xml" { + t.Errorf("Content-Type=%q want application/xml", got) + } + + if got := rr.Header().Get("Server"); got != "orca" { + t.Errorf("Server=%q want orca", got) + } + + body := parseS3Error(t, rr) + if body.Code != s3ErrNoSuchKey { + t.Errorf("Code=%q want %q", body.Code, s3ErrNoSuchKey) + } + + if body.Message == "" { + t.Error("Message is empty") + } + + if body.BucketName != "bucket" || body.Key != "key" { + t.Errorf("bucket/key=%q/%q want bucket/key", body.BucketName, body.Key) + } + + if body.Resource != "/bucket/key" { + t.Errorf("Resource=%q want /bucket/key", body.Resource) + } +} + +func TestWriteS3Error_HEAD_NoBody(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodHead, "/bucket/key", nil) + rr := httptest.NewRecorder() + + writeS3Error(rr, req, http.StatusNotFound, s3ErrNoSuchKey, + "The specified key does not exist.") + + if rr.Code != http.StatusNotFound { + t.Errorf("status=%d want %d", rr.Code, http.StatusNotFound) + } + + if got := rr.Header().Get("Server"); got != "orca" { + t.Errorf("Server=%q want orca", got) + } + + if rr.Body.Len() != 0 { + t.Errorf("HEAD body must be empty; got %d bytes: %q", rr.Body.Len(), rr.Body.String()) + } + + // HEAD must not advertise an XML content-type since there is no + // body. (A non-empty Content-Type with a zero-length body would + // confuse some SDKs into trying to parse.) + if got := rr.Header().Get("Content-Type"); got != "" { + t.Errorf("Content-Type=%q want empty on HEAD", got) + } +} + +func TestWriteS3Error_OmitsEmptyOptionalFields(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodGet, "/", nil) + rr := httptest.NewRecorder() + + writeS3Error(rr, req, http.StatusNotImplemented, s3ErrNotImplemented, + "ListBuckets is not implemented by orca.") + + body := rr.Body.String() + for _, tag := range []string{"", ""} { + if strings.Contains(body, tag) { + t.Errorf("body should omit %s when empty; got %q", tag, body) + } + } +} + +func TestWriteS3Error_NilRequest(t *testing.T) { + t.Parallel() + + // Defensive: helper must tolerate a nil request (e.g. from + // internal tests that don't construct one). The body should + // still be written and HEAD-suppression should default to false. + rr := httptest.NewRecorder() + writeS3Error(rr, nil, http.StatusInternalServerError, "InternalError", "boom") + + if rr.Code != http.StatusInternalServerError { + t.Errorf("status=%d", rr.Code) + } + + body := parseS3Error(t, rr) + if body.Code != "InternalError" || body.Message != "boom" { + t.Errorf("body=%+v", body) + } +} diff --git a/internal/orca/server/server.go b/internal/orca/server/server.go index 1327f532..fe73b9cb 100644 --- a/internal/orca/server/server.go +++ b/internal/orca/server/server.go @@ -4,11 +4,16 @@ // Package server holds the HTTP handlers for the client edge and the // internal-listener. // -// Client edge (8443): GET /{bucket}/{key} (with optional Range), HEAD, -// LIST. No auth in dev (server.auth.enabled=false). +// Client edge (8443): GET /{bucket}/{key} (with optional Range) and +// HEAD /{bucket}/{key}. No auth in dev (server.auth.enabled=false). +// Errors are returned as S3-compatible XML envelopes so that +// AWS S3 SDKs surface a typed error code to the caller; HEAD errors +// carry status + headers only (no body), matching real S3 behavior. // // Internal listener (8444): GET /internal/fill?. No mTLS in -// dev (cluster.internal_tls.enabled=false). +// dev (cluster.internal_tls.enabled=false). Internal-listener errors +// are plain text or JSON; the internal API is peer-to-peer between +// orca replicas and is never consumed by an S3 SDK. package server import ( @@ -53,17 +58,18 @@ func NewEdgeHandler(fc edgeFetchAPI, cfg *config.Config, log *slog.Logger) *Edge // Routing (path-style only, since LocalStack and most dev clients // use path-style): // -// GET / -> ListBuckets (not supported; 405) +// GET / -> ListBuckets (not supported; 501) // GET /{bucket}/ -> ListObjectsV2 (not supported; 501) // GET /{bucket}/{key} -> GetObject (with optional Range) +// HEAD / -> (not supported; 501) +// HEAD /{bucket}/ -> HeadBucket (not supported; 501) // HEAD /{bucket}/{key} -> HeadObject -// HEAD /{bucket}/ -> HeadBucket (not supported; 405) func (h *EdgeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if h.cfg.Server.Auth.Enabled { // Stub: production would dispatch to bearer/mTLS validation. // In dev (auth.enabled=false) we skip entirely. - http.Error(w, "auth required (server.auth.enabled=true) but not implemented in MVP", - http.StatusUnauthorized) + writeS3Error(w, r, http.StatusUnauthorized, s3ErrAccessDenied, + "Server auth is enabled but no auth handler is implemented in this build.") return } @@ -82,27 +88,34 @@ func (h *EdgeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodHead: if key == "" { - h.notImplemented(w, "HeadBucket") + // Both HEAD / and HEAD /{bucket}/ are reported as + // HeadBucket. HEAD / is not a real S3 operation, but + // labelling it HeadBucket keeps the surface uniform and + // makes the 501 self-explanatory. + h.notImplemented(w, r, "HeadBucket") return } h.handleHead(w, r, bucket, key) case http.MethodGet: - if key == "" { - h.notImplemented(w, "ListObjectsV2") - return + switch { + case bucket == "": + h.notImplemented(w, r, "ListBuckets") + case key == "": + h.notImplemented(w, r, "ListObjectsV2") + default: + h.handleGet(w, r, bucket, key) } - - h.handleGet(w, r, bucket, key) default: - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + writeS3Error(w, r, http.StatusMethodNotAllowed, s3ErrMethodNotAllowed, + "The specified method is not allowed against this resource.") } } func (h *EdgeHandler) handleHead(w http.ResponseWriter, r *http.Request, bucket, key string) { info, err := h.fc.HeadObject(r.Context(), bucket, key) if err != nil { - h.writeOriginError(w, err) + h.writeOriginError(w, r, err) return } @@ -122,7 +135,7 @@ func (h *EdgeHandler) handleHead(w http.ResponseWriter, r *http.Request, bucket, func (h *EdgeHandler) handleGet(w http.ResponseWriter, r *http.Request, bucket, key string) { info, err := h.fc.HeadObject(r.Context(), bucket, key) if err != nil { - h.writeOriginError(w, err) + h.writeOriginError(w, r, err) return } @@ -134,7 +147,9 @@ func (h *EdgeHandler) handleGet(w http.ResponseWriter, r *http.Request, bucket, // and remains a 416 (RFC 7233). if info.Size == 0 { if r.Header.Get("Range") != "" { - http.Error(w, "range not satisfiable", http.StatusRequestedRangeNotSatisfiable) + writeS3Error(w, r, http.StatusRequestedRangeNotSatisfiable, s3ErrInvalidRange, + "The requested range is not satisfiable.", + withBucketKey(bucket, key)) return } @@ -160,7 +175,9 @@ func (h *EdgeHandler) handleGet(w http.ResponseWriter, r *http.Request, bucket, if rh := r.Header.Get("Range"); rh != "" { s, e, ok := parseSimpleByteRange(rh, info.Size) if !ok { - http.Error(w, "invalid Range", http.StatusRequestedRangeNotSatisfiable) + writeS3Error(w, r, http.StatusRequestedRangeNotSatisfiable, s3ErrInvalidRange, + "The requested range is not valid for the resource.", + withBucketKey(bucket, key)) return } @@ -170,7 +187,9 @@ func (h *EdgeHandler) handleGet(w http.ResponseWriter, r *http.Request, bucket, } if rangeStart > rangeEnd { - http.Error(w, "range not satisfiable", http.StatusRequestedRangeNotSatisfiable) + writeS3Error(w, r, http.StatusRequestedRangeNotSatisfiable, s3ErrInvalidRange, + "The requested range is not satisfiable.", + withBucketKey(bucket, key)) return } @@ -206,7 +225,7 @@ func (h *EdgeHandler) handleGet(w http.ResponseWriter, r *http.Request, bucket, firstBody, err := h.fc.GetChunk(r.Context(), firstKey, info.Size) if err != nil { - h.writeOriginError(w, err) + h.writeOriginError(w, r, err) return } // Peek a single byte to drain any first-read errors from the @@ -216,7 +235,7 @@ func (h *EdgeHandler) handleGet(w http.ResponseWriter, r *http.Request, bucket, firstReader := bufio.NewReader(firstBody) if _, err := firstReader.Peek(1); err != nil && !errors.Is(err, io.EOF) { firstBody.Close() //nolint:errcheck // closing on error path - h.writeOriginError(w, err) + h.writeOriginError(w, r, err) return } @@ -692,16 +711,34 @@ func streamSlice(dst io.Writer, src io.Reader, off, length int64) error { return nil } -func (h *EdgeHandler) notImplemented(w http.ResponseWriter, op string) { - http.Error(w, op+" not implemented in MVP", http.StatusNotImplemented) +func (h *EdgeHandler) notImplemented(w http.ResponseWriter, r *http.Request, op string) { + writeS3Error(w, r, http.StatusNotImplemented, s3ErrNotImplemented, + op+" is not implemented by orca.") } -func (h *EdgeHandler) writeOriginError(w http.ResponseWriter, err error) { +func (h *EdgeHandler) writeOriginError(w http.ResponseWriter, r *http.Request, err error) { switch { case errors.Is(err, origin.ErrNotFound): - http.Error(w, "NoSuchKey", http.StatusNotFound) + writeS3Error(w, r, http.StatusNotFound, s3ErrNoSuchKey, + "The specified key does not exist.") case errors.Is(err, origin.ErrAuth): - http.Error(w, "Unauthorized origin", http.StatusBadGateway) + // ErrAuth maps to 502 BadGateway, NOT 403 AccessDenied. + // + // A 401/403 from the upstream origin means *orca's* own + // credentials were rejected by the origin; the calling + // client's credentials are not at fault. Returning 403 to the + // client would falsely imply the client should rotate its + // own credentials, which would not fix anything. + // + // From the client's perspective an orca-vs-origin auth + // failure is functionally an upstream outage: orca cannot + // satisfy the request through no fault of the client. + // 502 BadGateway communicates that accurately, and the + // orca-specific OriginUnauthorized code lets operators with + // access to orca logs tell this case apart from a generic + // origin failure. + writeS3Error(w, r, http.StatusBadGateway, s3ErrOriginUnauthorized, + "Origin rejected orca's credentials. This is an orca/origin configuration issue, not a client problem.") default: var ( ube *origin.UnsupportedBlobTypeError @@ -711,16 +748,18 @@ func (h *EdgeHandler) writeOriginError(w http.ResponseWriter, err error) { switch { case errors.As(err, &ube): - http.Error(w, "OriginUnsupported: "+ube.Error(), http.StatusBadGateway) + writeS3Error(w, r, http.StatusBadGateway, s3ErrOriginUnsupported, ube.Error()) case errors.As(err, &ec): - http.Error(w, "OriginETagChanged", http.StatusBadGateway) + writeS3Error(w, r, http.StatusBadGateway, s3ErrOriginETagChanged, + "Object changed at origin mid-fetch.") case errors.As(err, &mte): - http.Error(w, "OriginMissingETag: "+mte.Error(), http.StatusBadGateway) + writeS3Error(w, r, http.StatusBadGateway, s3ErrOriginMissingETag, mte.Error()) default: h.log.LogAttrs(context.Background(), slog.LevelWarn, "origin error", slog.Any("err", err), ) - http.Error(w, "OriginUnreachable", http.StatusBadGateway) + writeS3Error(w, r, http.StatusBadGateway, s3ErrOriginUnreachable, + "Origin request failed; see orca logs for details.") } } } diff --git a/internal/orca/server/server_test.go b/internal/orca/server/server_test.go index d7467a2c..36707ab1 100644 --- a/internal/orca/server/server_test.go +++ b/internal/orca/server/server_test.go @@ -41,7 +41,9 @@ func (f *fakeEdgeAPI) GetChunk(ctx context.Context, k chunk.Key, objectSize int6 } // TestWriteOriginError covers all five branches of the error mapping. -// Previously only ErrNotFound was exercised (via integration test). +// Each branch returns an S3 XML envelope; we assert both the +// HTTP status and the S3 Code so callers using typed SDK error +// branching continue to see the correct code. func TestWriteOriginError(t *testing.T) { t.Parallel() @@ -49,19 +51,19 @@ func TestWriteOriginError(t *testing.T) { name string err error wantStatus int - wantBody string + wantCode string }{ { name: "not found", err: origin.ErrNotFound, wantStatus: http.StatusNotFound, - wantBody: "NoSuchKey", + wantCode: "NoSuchKey", }, { name: "auth", err: origin.ErrAuth, wantStatus: http.StatusBadGateway, - wantBody: "Unauthorized origin", + wantCode: "OriginUnauthorized", }, { name: "unsupported blob type", @@ -71,7 +73,7 @@ func TestWriteOriginError(t *testing.T) { BlobType: "PageBlob", }, wantStatus: http.StatusBadGateway, - wantBody: "OriginUnsupported", + wantCode: "OriginUnsupported", }, { name: "etag changed", @@ -79,13 +81,13 @@ func TestWriteOriginError(t *testing.T) { Bucket: "b", Key: "k", Want: "old", }, wantStatus: http.StatusBadGateway, - wantBody: "OriginETagChanged", + wantCode: "OriginETagChanged", }, { name: "generic error", err: errors.New("unexpected"), wantStatus: http.StatusBadGateway, - wantBody: "OriginUnreachable", + wantCode: "OriginUnreachable", }, } @@ -93,15 +95,21 @@ func TestWriteOriginError(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/b/k", nil) rr := httptest.NewRecorder() - h.writeOriginError(rr, tt.err) + h.writeOriginError(rr, req, tt.err) if rr.Code != tt.wantStatus { t.Errorf("status=%d want %d", rr.Code, tt.wantStatus) } - if !strings.Contains(rr.Body.String(), tt.wantBody) { - t.Errorf("body %q does not contain %q", rr.Body.String(), tt.wantBody) + body := parseS3Error(t, rr) + if body.Code != tt.wantCode { + t.Errorf("Code=%q want %q", body.Code, tt.wantCode) + } + + if body.Message == "" { + t.Error("Message is empty") } }) } @@ -188,6 +196,127 @@ func TestHandleHead(t *testing.T) { } } +// TestRouting_GetRoot_ListBuckets verifies that GET / returns 501 +// with a NotImplemented S3 error code naming ListBuckets. Regression +// test for the doc/code mismatch where the routing comment said 405 +// but the handler actually returned 501; also locks in the 1b split +// between root and bucket-level GETs so the operation name in the +// error message is accurate. +func TestRouting_GetRoot_ListBuckets(t *testing.T) { + t.Parallel() + + h := NewEdgeHandler(&fakeEdgeAPI{}, &config.Config{}, discardLogger()) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + if rr.Code != http.StatusNotImplemented { + t.Errorf("status=%d want 501", rr.Code) + } + + body := parseS3Error(t, rr) + if body.Code != "NotImplemented" { + t.Errorf("Code=%q want NotImplemented", body.Code) + } + + if !strings.Contains(body.Message, "ListBuckets") { + t.Errorf("Message=%q should mention ListBuckets", body.Message) + } +} + +// TestRouting_GetBucket_ListObjectsV2 verifies that GET /{bucket}/ +// returns 501 with a NotImplemented code naming ListObjectsV2. +func TestRouting_GetBucket_ListObjectsV2(t *testing.T) { + t.Parallel() + + h := NewEdgeHandler(&fakeEdgeAPI{}, &config.Config{}, discardLogger()) + + req := httptest.NewRequest(http.MethodGet, "/bucket/", nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + if rr.Code != http.StatusNotImplemented { + t.Errorf("status=%d want 501", rr.Code) + } + + body := parseS3Error(t, rr) + if body.Code != "NotImplemented" { + t.Errorf("Code=%q want NotImplemented", body.Code) + } + + if !strings.Contains(body.Message, "ListObjectsV2") { + t.Errorf("Message=%q should mention ListObjectsV2", body.Message) + } +} + +// TestRouting_HeadBucket_NoBody verifies that HEAD /{bucket}/ returns +// 501 with no body (HEAD must not have a body per RFC 9110; AWS S3 +// communicates failure via status code only on HEAD). +func TestRouting_HeadBucket_NoBody(t *testing.T) { + t.Parallel() + + h := NewEdgeHandler(&fakeEdgeAPI{}, &config.Config{}, discardLogger()) + + req := httptest.NewRequest(http.MethodHead, "/bucket/", nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + if rr.Code != http.StatusNotImplemented { + t.Errorf("status=%d want 501", rr.Code) + } + + if rr.Body.Len() != 0 { + t.Errorf("HEAD body must be empty; got %d bytes", rr.Body.Len()) + } +} + +// TestRouting_MethodNotAllowed verifies that unsupported HTTP methods +// (PUT, DELETE, POST, etc.) return 405 with an S3 MethodNotAllowed +// error code. +func TestRouting_MethodNotAllowed(t *testing.T) { + t.Parallel() + + h := NewEdgeHandler(&fakeEdgeAPI{}, &config.Config{}, discardLogger()) + + req := httptest.NewRequest(http.MethodPut, "/bucket/key", nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + if rr.Code != http.StatusMethodNotAllowed { + t.Errorf("status=%d want 405", rr.Code) + } + + body := parseS3Error(t, rr) + if body.Code != "MethodNotAllowed" { + t.Errorf("Code=%q want MethodNotAllowed", body.Code) + } +} + +// TestRouting_AuthEnabled_AccessDenied verifies that when the auth +// stub is enabled (production-shaped config) the handler returns 401 +// with an S3 AccessDenied code, not a plain-text error. +func TestRouting_AuthEnabled_AccessDenied(t *testing.T) { + t.Parallel() + + cfg := &config.Config{} + cfg.Server.Auth.Enabled = true + h := NewEdgeHandler(&fakeEdgeAPI{}, cfg, discardLogger()) + + req := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + if rr.Code != http.StatusUnauthorized { + t.Errorf("status=%d want 401", rr.Code) + } + + body := parseS3Error(t, rr) + if body.Code != "AccessDenied" { + t.Errorf("Code=%q want AccessDenied", body.Code) + } +} + // TestParseSimpleByteRange covers all parser branches. func TestParseSimpleByteRange(t *testing.T) { t.Parallel() @@ -388,6 +517,11 @@ func TestHandleGet_EmptyObject_WithRange_Returns416(t *testing.T) { if rr.Code != http.StatusRequestedRangeNotSatisfiable { t.Errorf("status=%d want %d", rr.Code, http.StatusRequestedRangeNotSatisfiable) } + + body := parseS3Error(t, rr) + if body.Code != "InvalidRange" { + t.Errorf("Code=%q want InvalidRange", body.Code) + } } // TestHandleGet_FirstChunkErrorReturnsCleanError verifies that when @@ -405,25 +539,25 @@ func TestHandleGet_FirstChunkErrorReturnsCleanError(t *testing.T) { fetchErr error peekErr error // non-nil means GetChunk succeeds but first Read fails wantStatus int - wantBody string // substring assertion on the error body + wantCode string // S3 assertion }{ { name: "GetChunk returns NotFound", fetchErr: origin.ErrNotFound, wantStatus: http.StatusNotFound, - wantBody: "NoSuchKey", + wantCode: "NoSuchKey", }, { name: "GetChunk returns generic origin error", fetchErr: errors.New("origin: connect: timeout"), wantStatus: http.StatusBadGateway, - wantBody: "OriginUnreachable", + wantCode: "OriginUnreachable", }, { name: "GetChunk succeeds but first Read fails", peekErr: errors.New("cachestore: blob fetch 503"), wantStatus: http.StatusBadGateway, - wantBody: "OriginUnreachable", + wantCode: "OriginUnreachable", }, } @@ -459,8 +593,9 @@ func TestHandleGet_FirstChunkErrorReturnsCleanError(t *testing.T) { t.Errorf("status=%d want %d; body=%q", rr.Code, tt.wantStatus, rr.Body.String()) } - if !strings.Contains(rr.Body.String(), tt.wantBody) { - t.Errorf("body=%q want substring %q", rr.Body.String(), tt.wantBody) + body := parseS3Error(t, rr) + if body.Code != tt.wantCode { + t.Errorf("Code=%q want %q", body.Code, tt.wantCode) } // A bug here would 200 first, then write nothing or // partial bytes; verify the response did not commit a