From 90ef4e90e521c39f8d40fb123fac40e2771c22f5 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Sat, 18 Oct 2025 21:21:54 -0500 Subject: [PATCH] fix pushing and pulling from docker --- pkg/appview/storage/proxy_blob_store.go | 125 +++++++++++------------- pkg/hold/pds/xrpc.go | 78 +++++---------- pkg/hold/pds/xrpc_test.go | 86 ++++++++++++---- 3 files changed, 149 insertions(+), 140 deletions(-) diff --git a/pkg/appview/storage/proxy_blob_store.go b/pkg/appview/storage/proxy_blob_store.go index f05adc1..5909c06 100644 --- a/pkg/appview/storage/proxy_blob_store.go +++ b/pkg/appview/storage/proxy_blob_store.go @@ -222,19 +222,21 @@ func (p *ProxyBlobStore) Stat(ctx context.Context, dgst digest.Digest) (distribu return distribution.Descriptor{}, err } - // Get presigned HEAD URL - url, err := p.getHeadURL(ctx, dgst) + method := "HEAD" + + url, err := p.getPresignedURL(ctx, method, dgst) if err != nil { return distribution.Descriptor{}, distribution.ErrBlobUnknown } - // Make HEAD request with service token authentication - req, err := http.NewRequestWithContext(ctx, "HEAD", url, nil) + // Make HEAD request to presigned URL + req, err := http.NewRequestWithContext(ctx, method, url, nil) if err != nil { return distribution.Descriptor{}, distribution.ErrBlobUnknown } - resp, err := p.doAuthenticatedRequest(ctx, req) + // Go directly to the presigned URL, no need to authenticate + resp, err := p.httpClient.Do(req) if err != nil { return distribution.Descriptor{}, distribution.ErrBlobUnknown } @@ -264,16 +266,24 @@ func (p *ProxyBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, e return nil, err } - url, err := p.getDownloadURL(ctx, dgst) + method := "GET" + + url, err := p.getPresignedURL(ctx, method, dgst) if err != nil { return nil, err } - // Download the blob - resp, err := http.Get(url) + // Download the blob with service token authentication + req, err := http.NewRequestWithContext(ctx, method, url, nil) if err != nil { return nil, err } + + resp, err := p.doAuthenticatedRequest(ctx, req) + if err != nil { + return nil, err + } + defer resp.Body.Close() if resp.StatusCode != http.StatusOK { @@ -290,13 +300,15 @@ func (p *ProxyBlobStore) Open(ctx context.Context, dgst digest.Digest) (io.ReadS return nil, err } - url, err := p.getDownloadURL(ctx, dgst) + method := "GET" + + url, err := p.getPresignedURL(ctx, method, dgst) if err != nil { return nil, err } // Download the blob with service token authentication - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + req, err := http.NewRequestWithContext(ctx, method, url, nil) if err != nil { return nil, err } @@ -370,47 +382,7 @@ func (p *ProxyBlobStore) ServeBlob(ctx context.Context, w http.ResponseWriter, r return err } - // For HEAD requests, proxy the response instead of redirecting - // This avoids authentication issues when client follows redirects - if r.Method == http.MethodHead { - url, err := p.getHeadURL(ctx, dgst) - if err != nil { - return err - } - - // Make authenticated HEAD request to hold service - req, err := http.NewRequestWithContext(ctx, "HEAD", url, nil) - if err != nil { - return err - } - - resp, err := p.doAuthenticatedRequest(ctx, req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("blob not found") - } - - // Copy response headers - if contentLength := resp.Header.Get("Content-Length"); contentLength != "" { - w.Header().Set("Content-Length", contentLength) - } - if contentType := resp.Header.Get("Content-Type"); contentType != "" { - w.Header().Set("Content-Type", contentType) - } - if etag := resp.Header.Get("ETag"); etag != "" { - w.Header().Set("ETag", etag) - } - - w.WriteHeader(http.StatusOK) - return nil - } - - // For GET requests, redirect to presigned URL for direct download - url, err := p.getDownloadURL(ctx, dgst) + url, err := p.getPresignedURL(ctx, r.Method, dgst) if err != nil { return err } @@ -483,25 +455,44 @@ func (p *ProxyBlobStore) Resume(ctx context.Context, id string) (distribution.Bl return writer, nil } -// getDownloadURL returns the XRPC getBlob URL for downloading a blob -// The hold service will redirect to a presigned S3 URL -func (p *ProxyBlobStore) getDownloadURL(ctx context.Context, dgst digest.Digest) (string, error) { - // Use XRPC endpoint: GET /xrpc/com.atproto.sync.getBlob?did={userDID}&cid={digest} +// getPresignedURL returns the XRPC endpoint URL for blob operations +func (p *ProxyBlobStore) getPresignedURL(ctx context.Context, operation string, dgst digest.Digest) (string, error) { + // Use XRPC endpoint: /xrpc/com.atproto.sync.getBlob?did={userDID}&cid={digest} // The 'did' parameter is the USER's DID (whose blob we're fetching), not the hold service DID // Per migration doc: hold accepts OCI digest directly as cid parameter (checks for sha256: prefix) - url := fmt.Sprintf("%s/xrpc/com.atproto.sync.getBlob?did=%s&cid=%s", - p.holdURL, p.ctx.DID, dgst.String()) - return url, nil -} + xrpcURL := fmt.Sprintf("%s/xrpc/com.atproto.sync.getBlob?did=%s&cid=%s&method=%s", + p.holdURL, p.ctx.DID, dgst.String(), operation) -// getHeadURL returns the XRPC getBlob URL for HEAD requests -// The hold service will redirect to a presigned S3 URL -func (p *ProxyBlobStore) getHeadURL(ctx context.Context, dgst digest.Digest) (string, error) { - // Same as GET - hold service handles HEAD method on getBlob endpoint - // The 'did' parameter is the USER's DID (whose blob we're checking), not the hold service DID - url := fmt.Sprintf("%s/xrpc/com.atproto.sync.getBlob?did=%s&cid=%s", - p.holdURL, p.ctx.DID, dgst.String()) - return url, nil + req, err := http.NewRequestWithContext(ctx, "GET", xrpcURL, nil) + if err != nil { + return "", fmt.Errorf("failed to create request: %w", err) + } + + resp, err := p.doAuthenticatedRequest(ctx, req) + if err != nil { + return "", fmt.Errorf("failed to call hold service: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + return "", fmt.Errorf("hold service returned error: status %d, body: %s", resp.StatusCode, string(bodyBytes)) + } + + // Parse JSON response to get presigned HEAD URL + var result struct { + URL string `json:"url"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return "", fmt.Errorf("failed to parse hold service response: %w", err) + } + + if result.URL == "" { + return "", fmt.Errorf("hold service returned empty URL") + } + + fmt.Printf("DEBUG [proxy_blob_store]: Got presigned HEAD URL from hold service: %s\n", result.URL) + return result.URL, nil } // startMultipartUpload initiates a multipart upload via XRPC uploadBlob endpoint diff --git a/pkg/hold/pds/xrpc.go b/pkg/hold/pds/xrpc.go index b870b20..4cced22 100644 --- a/pkg/hold/pds/xrpc.go +++ b/pkg/hold/pds/xrpc.go @@ -978,60 +978,34 @@ func (h *XRPCHandler) HandleGetBlob(w http.ResponseWriter, r *http.Request) { digest = cidOrDigest } - // Handle HEAD vs GET differently - they need different presigned URLs - if r.Method == http.MethodHead { - // For HEAD requests: generate HEAD presigned URL and proxy to S3 - // AppView expects 200 OK with Content-Length, not a redirect - // Note: HEAD presigned URLs have different signatures than GET URLs - - headURL, err := h.blobStore.GetPresignedURL("HEAD", digest, did) // TODO: Add GetPresignedHeadURL method - if err != nil { - log.Printf("[HandleGetBlob] Failed to get presigned HEAD URL: digest=%s, did=%s, err=%v", digest, did, err) - http.Error(w, "blob not found", http.StatusNotFound) - return + // Determine presigned URL operation + // Check for ?method=HEAD query parameter first (from AppView), then fall back to request method + // HEAD and GET need different presigned URL signatures + operation := r.URL.Query().Get("method") + if operation == "" { + operation = "GET" + if r.Method == http.MethodHead { + operation = "HEAD" } - - log.Printf("[HandleGetBlob] Proxying HEAD request to: %s", headURL) - - headResp, err := http.Head(headURL) - if err != nil { - log.Printf("[HandleGetBlob] HEAD request failed: %v", err) - http.Error(w, "blob not found", http.StatusNotFound) - return - } - defer headResp.Body.Close() - - if headResp.StatusCode != http.StatusOK { - log.Printf("[HandleGetBlob] HEAD request returned non-200: %d", headResp.StatusCode) - http.Error(w, "blob not found", http.StatusNotFound) - return - } - - // Copy relevant headers from S3 response - if contentLength := headResp.Header.Get("Content-Length"); contentLength != "" { - w.Header().Set("Content-Length", contentLength) - } - if contentType := headResp.Header.Get("Content-Type"); contentType != "" { - w.Header().Set("Content-Type", contentType) - } - if etag := headResp.Header.Get("ETag"); etag != "" { - w.Header().Set("ETag", etag) - } - - log.Printf("[HandleGetBlob] HEAD request successful, Content-Length: %s", headResp.Header.Get("Content-Length")) - w.WriteHeader(http.StatusOK) - } else { - // For GET requests: generate GET presigned URL and redirect for direct download from S3 - downloadURL, err := h.blobStore.GetPresignedURL("GET", digest, did) - if err != nil { - log.Printf("[HandleGetBlob] Failed to get presigned GET URL: digest=%s, did=%s, err=%v", digest, did, err) - http.Error(w, "failed to get download URL", http.StatusInternalServerError) - return - } - - log.Printf("[HandleGetBlob] Redirecting GET request to presigned URL: %s", downloadURL) - http.Redirect(w, r, downloadURL, http.StatusTemporaryRedirect) } + + // Generate presigned URL for the operation + presignedURL, err := h.blobStore.GetPresignedURL(operation, digest, did) + if err != nil { + log.Printf("[HandleGetBlob] Failed to get presigned %s URL: digest=%s, did=%s, err=%v", operation, digest, did, err) + http.Error(w, "failed to get presigned URL", http.StatusInternalServerError) + return + } + + log.Printf("[HandleGetBlob] Returning presigned %s URL: %s", operation, presignedURL) + + // Return JSON response with the presigned URL + // AppView will either redirect (GET) or proxy (HEAD) using this URL + response := map[string]string{ + "url": presignedURL, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(response) } // HandleListRepos lists all repositories in this PDS diff --git a/pkg/hold/pds/xrpc_test.go b/pkg/hold/pds/xrpc_test.go index 92326de..bfa698e 100644 --- a/pkg/hold/pds/xrpc_test.go +++ b/pkg/hold/pds/xrpc_test.go @@ -1386,7 +1386,8 @@ func newMockBlobStore() *mockBlobStore { } func (m *mockBlobStore) GetPresignedURL(operation, digest, did string) (string, error) { - if operation == "GET" { + if operation == "GET" || operation == "HEAD" { + // Both GET and HEAD are download operations, just different HTTP methods m.downloadCalls = append(m.downloadCalls, digest) if m.downloadURLError != nil { return "", m.downloadURLError @@ -1395,6 +1396,7 @@ func (m *mockBlobStore) GetPresignedURL(operation, digest, did string) (string, return "https://s3.example.com/download/" + digest, nil } + // PUT or other upload operations m.uploadCalls = append(m.uploadCalls, digest) if m.uploadURLError != nil { return "", m.uploadURLError @@ -1680,20 +1682,32 @@ func TestHandleGetBlob(t *testing.T) { handler.HandleGetBlob(w, req) - // Should redirect to presigned download URL (307 Temporary Redirect) - if w.Code != http.StatusTemporaryRedirect { - t.Errorf("Expected status 307 (Temporary Redirect), got %d", w.Code) + // Should return 200 OK with JSON response containing presigned URL + if w.Code != http.StatusOK { + t.Errorf("Expected status 200 OK, got %d", w.Code) } - location := w.Header().Get("Location") + // Verify Content-Type is JSON + contentType := w.Header().Get("Content-Type") + if contentType != "application/json" { + t.Errorf("Expected Content-Type application/json, got %s", contentType) + } + + // Parse JSON response + var response map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to parse JSON response: %v", err) + } + + // Verify URL field exists expectedURL := "https://s3.example.com/download/" + cid - if location != expectedURL { - t.Errorf("Expected redirect to %s, got %s", expectedURL, location) + if response["url"] != expectedURL { + t.Errorf("Expected url to be %s, got %s", expectedURL, response["url"]) } // Verify blob store was called if len(blobStore.downloadCalls) != 1 || blobStore.downloadCalls[0] != cid { - t.Errorf("Expected GetPresignedDownloadURL to be called with %s", cid) + t.Errorf("Expected GetPresignedURL to be called with %s", cid) } } @@ -1713,23 +1727,34 @@ func TestHandleGetBlob_SHA256Digest(t *testing.T) { handler.HandleGetBlob(w, req) - // Should redirect to presigned download URL - if w.Code != http.StatusTemporaryRedirect { - t.Errorf("Expected status 307, got %d", w.Code) + // Should return 200 OK with JSON response + if w.Code != http.StatusOK { + t.Errorf("Expected status 200 OK, got %d", w.Code) + } + + // Parse JSON response + var response map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to parse JSON response: %v", err) + } + + // Verify URL field exists + if response["url"] == "" { + t.Errorf("Expected url field in response, got empty") } // Verify blob store received the sha256 digest if len(blobStore.downloadCalls) != 1 || blobStore.downloadCalls[0] != digest { - t.Errorf("Expected GetPresignedDownloadURL to be called with %s, got %v", digest, blobStore.downloadCalls) + t.Errorf("Expected GetPresignedURL to be called with %s, got %v", digest, blobStore.downloadCalls) } } // TestHandleGetBlob_HeadMethod tests HEAD request support -// HEAD requests are proxied (not redirected) to avoid S3 presigned URL signature issues -// The hold service makes the HEAD request itself and returns 200 OK with headers +// HEAD requests now return JSON with presigned HEAD URL (same as GET) +// AppView is responsible for making the actual HEAD request to S3 // Spec: https://docs.bsky.app/docs/api/com-atproto-sync-get-blob func TestHandleGetBlob_HeadMethod(t *testing.T) { - handler, _, _ := setupTestXRPCHandlerWithBlobs(t) + handler, blobStore, _ := setupTestXRPCHandlerWithBlobs(t) holdDID := "did:web:hold.example.com" cid := "bafyreib2rxk3rkhh5ylyxj3x3gathxt3s32qvwj2lf3qg4kmzr6b7teqke" @@ -1740,14 +1765,33 @@ func TestHandleGetBlob_HeadMethod(t *testing.T) { handler.HandleGetBlob(w, req) - // HEAD requests are now proxied - the handler makes an HTTP HEAD to the presigned URL - // In the test environment, this will fail because the mock returns a fake URL - // Expect 404 because the handler couldn't reach the mock S3 URL - if w.Code != http.StatusNotFound { - t.Errorf("Expected status 404 (mock S3 unreachable), got %d", w.Code) + // Should return 200 OK with JSON response containing presigned HEAD URL + if w.Code != http.StatusOK { + t.Errorf("Expected status 200 OK, got %d", w.Code) } - // Note: In production with real S3, HEAD would return 200 OK with Content-Length header + // Verify Content-Type is JSON + contentType := w.Header().Get("Content-Type") + if contentType != "application/json" { + t.Errorf("Expected Content-Type application/json, got %s", contentType) + } + + // Parse JSON response + var response map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to parse JSON response: %v", err) + } + + // Verify URL field exists + expectedURL := "https://s3.example.com/download/" + cid + if response["url"] != expectedURL { + t.Errorf("Expected url to be %s, got %s", expectedURL, response["url"]) + } + + // Verify blob store was called with HEAD operation + if len(blobStore.downloadCalls) != 1 || blobStore.downloadCalls[0] != cid { + t.Errorf("Expected GetPresignedURL to be called with %s", cid) + } } // TestHandleGetBlob_MissingParameters tests missing required parameters