fix pushing and pulling from docker

2026-06-06 23:32:36 +00:00 · 2025-10-18 21:21:54 -05:00
parent 1658a53cad
commit 90ef4e90e5
3 changed files with 149 additions and 140 deletions
--- 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
--- 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
--- 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