Skip to content

Commit

Permalink
Update PackageService.Create to use presigned blob uploads instead of…
Browse files Browse the repository at this point in the history
… pushing file directly to buildkite
  • Loading branch information
moskyb committed Oct 17, 2024
1 parent f86816b commit 8739d98
Show file tree
Hide file tree
Showing 4 changed files with 257 additions and 125 deletions.
4 changes: 4 additions & 0 deletions buildkite.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"strings"
"time"

"github.com/buildkite/go-buildkite/v3/internal/bkmultipart"
"github.com/cenkalti/backoff"
"github.com/google/go-querystring/query"
)
Expand Down Expand Up @@ -223,6 +224,9 @@ func (c *Client) NewRequest(ctx context.Context, method, urlStr string, body int
var reqBody io.Reader
if body != nil {
switch v := body.(type) {
case *bkmultipart.Streamer:
panic("bkmultipart.Streamer passed directly to NewRequest. Did you mean to pass bkstreamer.Streamer.Reader() instead?")

case io.Reader: // If body is an io.Reader, use it directly, the caller is responsible for encoding
reqBody = v

Expand Down
75 changes: 52 additions & 23 deletions package_uploads.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,22 @@ func (ps *PackagesService) Create(ctx context.Context, organizationSlug, registr
s := bkmultipart.NewStreamer()
s.WriteFile(fileFormKey, packageTempFile, filename)

url := fmt.Sprintf("v2/packages/organizations/%s/registries/%s/packages", organizationSlug, registrySlug)
req, err := ps.client.NewRequest(ctx, "POST", url, s.Reader())
ppu, _, err := ps.RequestPresignedUpload(ctx, organizationSlug, registrySlug)
if err != nil {
return Package{}, nil, fmt.Errorf("creating POST package request: %v", err)
return Package{}, nil, fmt.Errorf("requesting presigned upload: %v", err)
}

req.Header.Set("Content-Type", s.ContentType)
req.Header.Set("Content-Length", fmt.Sprintf("%d", s.Len()))
s3URL, err := ppu.Perform(ctx, ps, packageTempFile)
if err != nil {
return Package{}, nil, fmt.Errorf("performing presigned upload: %v", err)
}

var p Package
resp, err := ps.client.Do(req, &p)
p, resp, err := ppu.Finalize(ctx, ps, s3URL)
if err != nil {
return Package{}, resp, fmt.Errorf("executing POST package request: %v", err)
return Package{}, nil, fmt.Errorf("finalizing package: %v", err)
}

return p, resp, err
return p, resp, nil
}

// normalizeToFile takes and io.Reader (which might itself already be a file, but could be a stream or other source) and
Expand All @@ -74,27 +74,43 @@ func normalizeToFile(r io.Reader, filename string) (*os.File, error) {
return nil, fmt.Errorf("writing to temporary file: %v", err)
}

_, err = f.Seek(0, 0)
err = f.Close()
if err != nil {
return nil, fmt.Errorf("closing temporary file: %v", err)
}

// Rename the temporary file to the desired filename, which is important for Buildkite Package indexing
newFileName := filepath.Join(filepath.Dir(f.Name()), basename)
err = os.Rename(f.Name(), newFileName)
if err != nil {
return nil, fmt.Errorf("seeking to beginning of temporary file: %v", err)
return nil, fmt.Errorf("renaming temporary file: %v", err)
}

f, err = os.Open(newFileName)
if err != nil {
return nil, fmt.Errorf("opening renamed file: %v", err)
}

return f, nil
}

// PackagePresignedUpload represents a presigned upload URL for a Buildkite package, returned by the Buildkite API
type PackagePresignedUpload struct {
OrganizationSlug string `json:"-"`
RegistrySlug string `json:"-"`

URI string `json:"uri"`
Form struct {
FileInput string `json:"file_input"`
Method string `json:"method"`
URL string `json:"url"`
Data map[string]string `json:"data"`
} `json:"form"`
URI string `json:"uri"`
Form PackagePresignedUploadForm `json:"form"`
}

type PackagePresignedUploadForm struct {
FileInput string `json:"file_input"`
Method string `json:"method"`
URL string `json:"url"`
Data map[string]string `json:"data"`
}

// RequestPresignedUpload requests a presigned upload URL for a Buildkite package from the buildkite API
func (ps *PackagesService) RequestPresignedUpload(ctx context.Context, organizationSlug, registrySlug string) (PackagePresignedUpload, *Response, error) {
url := fmt.Sprintf("v2/packages/organizations/%s/registries/%s/packages/upload", organizationSlug, registrySlug)
req, err := ps.client.NewRequest(ctx, "POST", url, nil)
Expand All @@ -105,7 +121,6 @@ func (ps *PackagesService) RequestPresignedUpload(ctx context.Context, organizat
var p PackagePresignedUpload
resp, err := ps.client.Do(req, &p)
if err != nil {
fmt.Println(string(err.(*ErrorResponse).RawBody))
return PackagePresignedUpload{}, resp, fmt.Errorf("executing POST presigned upload request: %v", err)
}

Expand All @@ -115,6 +130,9 @@ func (ps *PackagesService) RequestPresignedUpload(ctx context.Context, organizat
return p, resp, err
}

// Perform performs uploads the package file referred to by `file` to the presigned upload URL.
// It does not create the package in the registry, only uploads the file to S3. The returned string is the URL of the
// uploaded file in S3, which can then be passed to [Finalize] to create the package in the registry.
func (ppu PackagePresignedUpload) Perform(ctx context.Context, ps *PackagesService, file *os.File) (string, error) {
if _, ok := ppu.Form.Data["key"]; !ok {
return "", fmt.Errorf("missing 'key' in presigned upload form data")
Expand All @@ -123,8 +141,15 @@ func (ppu PackagePresignedUpload) Perform(ctx context.Context, ps *PackagesServi
baseFilePath := filepath.Base(file.Name())

s := bkmultipart.NewStreamer()
s.WriteFields(ppu.Form.Data)
s.WriteFile(ppu.Form.FileInput, file, baseFilePath)
err := s.WriteFields(ppu.Form.Data)
if err != nil {
return "", fmt.Errorf("writing form fields: %v", err)
}

err = s.WriteFile(ppu.Form.FileInput, file, baseFilePath)
if err != nil {
return "", fmt.Errorf("writing form file: %v", err)
}

req, err := http.NewRequestWithContext(ctx, ppu.Form.Method, ppu.Form.URL, s.Reader())
if err != nil {
Expand Down Expand Up @@ -164,12 +189,16 @@ func (ppu PackagePresignedUpload) Perform(ctx context.Context, ps *PackagesServi
return uploadPath, nil
}

// Finalize creates a package in the registry for the organization, using the S3 URL of the uploaded package file.
func (ppu PackagePresignedUpload) Finalize(ctx context.Context, ps *PackagesService, s3URL string) (Package, *Response, error) {
s := bkmultipart.NewStreamer()
s.WriteField("package_url", s3URL)
err := s.WriteField("package_url", s3URL)
if err != nil {
return Package{}, nil, fmt.Errorf("writing package_url field: %v", err)
}

url := fmt.Sprintf("v2/packages/organizations/%s/registries/%s/packages", ppu.OrganizationSlug, ppu.RegistrySlug)
req, err := ps.client.NewRequest(ctx, "POST", url, s)
req, err := ps.client.NewRequest(ctx, "POST", url, s.Reader())
if err != nil {
return Package{}, nil, fmt.Errorf("creating POST package request: %v", err)
}
Expand Down
201 changes: 201 additions & 0 deletions package_uploads_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
package buildkite

import (
"bytes"
"context"
"encoding/json"
"io"
"mime"
"net/http"
"net/url"
"os"
"path/filepath"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
)

func TestCreatePackage(t *testing.T) {
t.Parallel()

testPackage, err := os.CreateTemp("", "test-package")
if err != nil {
t.Fatalf("creating temporary package file: %v", err)
}
t.Cleanup(func() { os.Remove(testPackage.Name()) })

packageContents := "this is totally a valid package! look, i'm a rubygem!"
_, err = testPackage.WriteString(packageContents)
if err != nil {
t.Fatalf("writing to temporary package file: %v", err)
}

if _, err := testPackage.Seek(0, io.SeekStart); err != nil {
t.Fatalf("seeking to start of temporary package file: %v", err)
}

cases := []struct {
name string
in CreatePackageInput
wantContents string
wantFileName string
}{
{
name: "file",
in: CreatePackageInput{Package: testPackage},
wantContents: packageContents,
wantFileName: testPackage.Name(),
},
{
name: "io.Reader with filename",
in: CreatePackageInput{
Package: bytes.NewBufferString(packageContents),
Filename: "cool-package.gem",
},
wantContents: packageContents,
wantFileName: "cool-package.gem",
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

server, client, teardown := newMockServerAndClient(t)
t.Cleanup(teardown)

s3Endpoint := "/s3"
s3Path := "/fake/path"

postData := map[string]string{
"key": s3Path + "/${filename}",
"acl": "private",
"policy": "bWFkZSB5b3UgbG9vayE=",
"x-amz-credential": "AKIAS000000000000000/20241007/ap-southeast-2/s3/aws4_request",
"x-amz-algorithm": "AWS4-HMAC-SHA256",
"x-amz-date": "20241007T031838Z",
"x-amz-signature": "f6d24942026ffe7ec32b5f57beb46a2679b7a74a87673e1614b92c15ee2661f2",
}

// Signed Upload Request
server.HandleFunc("/v2/packages/organizations/my-org/registries/my-registry/packages/upload", func(w http.ResponseWriter, r *http.Request) {
defer r.Body.Close()

testMethod(t, r, "POST")

ppu := PackagePresignedUpload{
URI: "s3://fake-s3-bucket/fake-s3-path", // URI is unused by go-buildkite, but here for completeness
Form: PackagePresignedUploadForm{
FileInput: "file",
Method: "POST",
URL: "http://" + r.Host + s3Endpoint,
Data: postData,
},
}

w.Header().Set("Content-Type", "application/json")
err := json.NewEncoder(w).Encode(ppu)
if err != nil {
t.Fatalf("encoding presigned upload to json: %v", err)
}
})

// "S3" Upload
server.HandleFunc(s3Endpoint, func(w http.ResponseWriter, r *http.Request) {
defer r.Body.Close()

testMethod(t, r, "POST")

if r.Header.Get("Content-Type") == "" {
t.Fatalf("missing Content-Type header - S3 requires it")
}

if r.Header.Get("Content-Length") == "" {
t.Fatalf("missing Content-Length header - S3 requires it")
}

ct := r.Header.Get("Content-Type")
mt, _, err := mime.ParseMediaType(ct)
if err != nil {
t.Fatalf("parsing Content-Type: %v", err)
}

if got, want := mt, "multipart/form-data"; got != want {
t.Fatalf("unexpected media type: got %q, want %q", got, want)
}

if !strings.HasPrefix(r.Header.Get("Content-Type"), "multipart/form-data") {
t.Fatalf("unexpected Content-Type: %q", r.Header.Get("Content-Type"))
}

fi, header, err := r.FormFile(fileFormKey)
if err != nil {
t.Fatalf("getting file from request: %v", err)
}
defer fi.Close()

// RFC 7578 says that the any path information should be stripped from the file name, which is what
// r.FormFile does - see https://github.com/golang/go/blob/d9f9746/src/mime/multipart/multipart.go#L99-L100
if header.Filename != filepath.Base(tc.wantFileName) {
t.Fatalf("file name mismatch: got %q, want %q", header.Filename, tc.wantFileName)
}

fileContents, err := io.ReadAll(fi)
if err != nil {
t.Fatalf("reading file contents: %v", err)
}

if string(fileContents) != tc.wantContents {
t.Fatalf("file contents mismatch: got %q, want %q", string(fileContents), tc.wantContents)
}
})

// Create Package / Presigned upload finalization
server.HandleFunc("/v2/packages/organizations/my-org/registries/my-registry/packages", func(w http.ResponseWriter, r *http.Request) {
defer r.Body.Close()

testMethod(t, r, "POST")

err := r.ParseMultipartForm(2 << 10)
if err != nil {
t.Fatalf("parsing multipart form: %v", err)
}

wantPath, err := url.JoinPath(s3Endpoint, s3Path, filepath.Base(tc.wantFileName))
if err != nil {
t.Fatalf("joining URL path: %v", err)
}

wantURL := "http://" + r.Host + wantPath
if got, want := r.Form["package_url"][0], wantURL; got != want {
t.Fatalf("unexpected package URL: got %q, want %q", got, want)
}

err = json.NewEncoder(w).Encode(pkg)
if err != nil {
t.Fatalf("encoding package to json: %v", err)
}
})

p, _, err := client.PackagesService.Create(context.Background(), "my-org", "my-registry", tc.in)
if err != nil {
t.Fatalf("Packages.Create returned error: %v", err)
}

expectedHTTPCalls := []httpCall{
{Method: "POST", Path: "/v2/packages/organizations/my-org/registries/my-registry/packages/upload"},
{Method: "POST", Path: "/s3"},
{Method: "POST", Path: "/v2/packages/organizations/my-org/registries/my-registry/packages"},
}

if diff := cmp.Diff(expectedHTTPCalls, server.calls); diff != "" {
t.Fatalf("unexpected HTTP calls (-want +got):\n%s", diff)
}

if diff := cmp.Diff(p, pkg); diff != "" {
t.Fatalf("client.PackagesService.Create(%q, %q, %v) diff: (-got +want)\n%s", "test-org", "my-cool-registry", tc.in, diff)
}
})
}
}
Loading

0 comments on commit 8739d98

Please sign in to comment.