diff --git a/buildkite.go b/buildkite.go index c38a51a5..97804f75 100644 --- a/buildkite.go +++ b/buildkite.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "github.com/buildkite/go-buildkite/v3/internal/bkmultipart" "github.com/cenkalti/backoff" "github.com/google/go-querystring/query" ) @@ -190,6 +191,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 diff --git a/examples/packages/create/main.go b/examples/packages/create/main.go index 276ff27c..2c0fd420 100644 --- a/examples/packages/create/main.go +++ b/examples/packages/create/main.go @@ -31,8 +31,6 @@ func main() { log.Fatalf("opening file %s failed: %v", *filePath, err) } - log.Println(file.Name()) - pkg, _, err := client.PackagesService.Create(context.Background(), *org, *registrySlug, buildkite.CreatePackageInput{Package: file}) if err != nil { log.Fatalf("Creating package failed: %v", err) diff --git a/examples/packages/create_presigned_upload/main.go b/examples/packages/create_presigned_upload/main.go new file mode 100644 index 00000000..d9df6ee3 --- /dev/null +++ b/examples/packages/create_presigned_upload/main.go @@ -0,0 +1,50 @@ +package main + +import ( + "context" + "log" + "os" + + "github.com/buildkite/go-buildkite/v3" + "gopkg.in/alecthomas/kingpin.v2" +) + +var ( + apiToken = kingpin.Flag("token", "API token").Required().String() + org = kingpin.Flag("org", "Orginization slug").Required().String() + registry = kingpin.Flag("registry", "Registry Slug").Required().String() + filePath = kingpin.Flag("file-path", "File path").Required().String() +) + +func main() { + kingpin.Parse() + + client, err := buildkite.NewOpts(buildkite.WithTokenAuth(*apiToken)) + if err != nil { + log.Fatalf("creating buildkite API client failed: %v", err) + } + + file, err := os.Open(*filePath) + if err != nil { + log.Fatalf("opening file %s failed: %v", *filePath, err) + } + + ppu, _, err := client.PackagesService.RequestPresignedUpload(context.Background(), *org, *registry) + if err != nil { + log.Fatalf("Creating package failed: %v", err) + } + + url, err := ppu.Perform(context.Background(), file) + if err != nil { + log.Fatalf("Package upload to S3 failed: %v", err) + } + + log.Println("Uploaded package to: " + url) + + pkg, _, err := ppu.Finalize(context.Background(), url) + if err != nil { + log.Fatalf("Finalizing package failed: %v", err) + } + + log.Printf("Package uploaded: %s", pkg.Name) +} diff --git a/package_uploads.go b/package_uploads.go new file mode 100644 index 00000000..8a919d23 --- /dev/null +++ b/package_uploads.go @@ -0,0 +1,220 @@ +package buildkite + +import ( + "context" + "fmt" + "io" + "net/http" + "net/url" + "os" + "path/filepath" + "strings" + + "github.com/buildkite/go-buildkite/v3/internal/bkmultipart" +) + +// CreatePackageInput specifies the input parameters for the Create method. +// All fields are required, but if PackageFile is an [os.File], Filename can be omitted. +type CreatePackageInput struct { + Package io.Reader // The package to upload. This can be an [os.File], or any other [io.Reader]. + Filename string // The name of the file to upload. If PackageFile is an [os.File], this can be omitted, and the file's name will be used. +} + +// Create creates a package in a registry for an organization +func (ps *PackagesService) Create(ctx context.Context, organizationSlug, registrySlug string, cpi CreatePackageInput) (Package, *Response, error) { + var file *os.File + switch f := cpi.Package.(type) { + case *os.File: + file = f + + default: + var err error + file, err = readIntoTempFile(cpi.Package, cpi.Filename) + if err != nil { + return Package{}, nil, fmt.Errorf("writing package to tempfile: %v", err) + } + + defer func() { + file.Close() + os.Remove(file.Name()) + }() + } + + ppu, _, err := ps.RequestPresignedUpload(ctx, organizationSlug, registrySlug) + if err != nil { + return Package{}, nil, fmt.Errorf("requesting presigned upload: %v", err) + } + + s3URL, err := ppu.Perform(ctx, file) + if err != nil { + return Package{}, nil, fmt.Errorf("performing presigned upload: %v", err) + } + + p, resp, err := ppu.Finalize(ctx, s3URL) + if err != nil { + return Package{}, nil, fmt.Errorf("finalizing package: %v", err) + } + + return p, resp, nil +} + +// readIntoTempFile takes an io.Reader and writes it to a temporary file, returning the file handle. +// The file is written to a temporary directory, and then renamed to the desired filename. +// We do this normalization to ensure that we can accurately calculate the Content-Length of the request body, which is +// required by S3. We write to disk (instead of buffering in memory) to avoid memory exhaustion for large files. +func readIntoTempFile(r io.Reader, filename string) (*os.File, error) { + basename := filepath.Base(filename) + f, err := os.CreateTemp("", basename) + if err != nil { + return nil, fmt.Errorf("creating temporary file: %v", err) + } + + _, err = io.Copy(f, r) + if err != nil { + return nil, fmt.Errorf("writing to temporary file: %v", err) + } + + 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("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 { + bkClient *Client + + OrganizationSlug string `json:"-"` + RegistrySlug string `json:"-"` + + 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) + if err != nil { + return nil, nil, fmt.Errorf("creating POST presigned upload request: %v", err) + } + + var p *PackagePresignedUpload + resp, err := ps.client.Do(req, &p) + if err != nil { + return nil, resp, fmt.Errorf("executing POST presigned upload request: %v", err) + } + + p.bkClient = ps.client + p.OrganizationSlug = organizationSlug + p.RegistrySlug = registrySlug + + 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 the package host. 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, file *os.File) (string, error) { + if _, ok := ppu.Form.Data["key"]; !ok { + return "", fmt.Errorf("missing 'key' in presigned upload form data") + } + + baseFilePath := filepath.Base(file.Name()) + + s := bkmultipart.NewStreamer() + 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) + } + + // note NOT using client.NewRequest here, as it'll add buildkite-specific stuff that we don't want + req, err := http.NewRequestWithContext(ctx, ppu.Form.Method, ppu.Form.URL, s.Reader()) + if err != nil { + return "", fmt.Errorf("creating %s request: %v", ppu.Form.Method, err) + } + + req.Header.Set("Content-Type", s.ContentType) + + // Don't set the Content-Length header here, you fool, you absolute buffoon + // When passed an io.Reader, http.NewRequestWithContext will not set the Content-Length header, and will instead + // stream the request body. This _would_ be exactly what we want, except that S3 uploads infuriatingly require a + // Content-Length header. So we have to calculate the length of the request body ourselves and set it manually on the + // request. Adding: + // req.Header.Set("Content-Length", fmt.Sprintf("%d", s.Len())) + // is not sufficient, as the Content-Length header is stripped by the http client when the request body is an io.Reader. + req.ContentLength = s.Len() + + resp, err := ppu.bkClient.client.Do(req) + if err != nil { + return "", fmt.Errorf("executing %s request: %v", ppu.Form.Method, err) + } + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("S3 rejected upload with unexpected status code %d. Error reading response body: %v", resp.StatusCode, err) + } + + return "", fmt.Errorf("S3 rejected upload with unexpected status code %d. Response body %s", resp.StatusCode, string(body)) + } + + uploadPath, err := url.JoinPath(ppu.Form.URL, strings.ReplaceAll(ppu.Form.Data["key"], "${filename}", baseFilePath)) + if err != nil { + return "", fmt.Errorf("joining URL path: %v", err) + } + + 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, s3URL string) (Package, *Response, error) { + s := bkmultipart.NewStreamer() + 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 := ppu.bkClient.NewRequest(ctx, "POST", url, s.Reader()) + if err != nil { + return Package{}, nil, fmt.Errorf("creating POST package request: %v", err) + } + + req.Header.Set("Content-Type", s.ContentType) + req.ContentLength = s.Len() + + var p Package + resp, err := ppu.bkClient.Do(req, &p) + if err != nil { + return Package{}, resp, fmt.Errorf("executing POST package request: %v", err) + } + + return p, resp, err +} diff --git a/package_uploads_test.go b/package_uploads_test.go new file mode 100644 index 00000000..ff8430e8 --- /dev/null +++ b/package_uploads_test.go @@ -0,0 +1,201 @@ +package buildkite + +import ( + "bytes" + "context" + "encoding/json" + "io" + "mime" + "net/http" + "net/url" + "os" + "path/filepath" + "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-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) + } + + 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) + } + + wantHTTPCalls := []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(wantHTTPCalls, 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) + } + + // If we get passed in a file, we really don't want to delete it in the upload process. If this stat fails, + // the file was deleted. + if p, ok := tc.in.Package.(*os.File); ok { + _, err := os.Stat(p.Name()) + if err != nil { + t.Fatalf("expected stat file to have nil error, got: %v", err) + } + } + }) + } +} diff --git a/packages.go b/packages.go index ebc1f473..dd834645 100644 --- a/packages.go +++ b/packages.go @@ -3,11 +3,6 @@ package buildkite import ( "context" "fmt" - "io" - "os" - "path/filepath" - - "github.com/buildkite/go-buildkite/v3/internal/bkmultipart" ) const fileFormKey = "file" @@ -42,74 +37,3 @@ func (ps *PackagesService) Get(ctx context.Context, organizationSlug, registrySl return p, resp, err } - -// CreatePackageInput specifies the input parameters for the Create method. -// All fields are required, but if PackageFile is an [os.File], Filename can be omitted. -type CreatePackageInput struct { - Package io.Reader // The package to upload. This can be an [os.File], or any other [io.Reader]. - Filename string // The name of the file to upload. If PackageFile is an [os.File], this can be omitted, and the file's name will be used. -} - -// Create creates a package in a registry for an organization -func (ps *PackagesService) Create(ctx context.Context, organizationSlug, registrySlug string, cpi CreatePackageInput) (Package, *Response, error) { - filename := cpi.Filename - if f, ok := cpi.Package.(*os.File); ok && filename == "" { - filename = f.Name() - } - - packageTempFile, err := normalizeToFile(cpi.Package, filename) - if err != nil { - return Package{}, nil, fmt.Errorf("writing package to tempfile: %v", err) - } - defer func() { - packageTempFile.Close() - os.Remove(packageTempFile.Name()) - }() - - s := bkmultipart.NewStreamer() - err = s.WriteFile(fileFormKey, packageTempFile, filename) - if err != nil { - return Package{}, nil, fmt.Errorf("writing package to multipart stream: %v", err) - } - - url := fmt.Sprintf("v2/packages/organizations/%s/registries/%s/packages", organizationSlug, registrySlug) - req, err := ps.client.NewRequest(ctx, "POST", url, s.Reader()) - if err != nil { - return Package{}, nil, fmt.Errorf("creating POST package request: %v", err) - } - - req.Header.Set("Content-Type", s.ContentType) - req.Header.Set("Content-Length", fmt.Sprintf("%d", s.Len())) - - var p Package - resp, err := ps.client.Do(req, &p) - if err != nil { - return Package{}, resp, fmt.Errorf("executing POST package request: %v", err) - } - - return p, resp, err -} - -// normalizeToFile takes an io.Reader (which might itself already be a file, but could be a stream or other source) and -// writes it to a temporary file, returning the file handle. -// We do this normalization to ensure that we can accurately calculate the Content-Length of the request body, which is -// required by S3. We write to disk (instead of buffering in memory) to avoid memory exhaustion for large files. -func normalizeToFile(r io.Reader, filename string) (*os.File, error) { - basename := filepath.Base(filename) - f, err := os.CreateTemp("", basename) - if err != nil { - return nil, fmt.Errorf("creating temporary file: %v", err) - } - - _, err = io.Copy(f, r) - if err != nil { - return nil, fmt.Errorf("writing to temporary file: %v", err) - } - - _, err = f.Seek(0, io.SeekStart) - if err != nil { - return nil, fmt.Errorf("seeking to beginning of temporary file: %v", err) - } - - return f, nil -} diff --git a/packages_test.go b/packages_test.go index 3de2400d..b8c860c2 100644 --- a/packages_test.go +++ b/packages_test.go @@ -1,15 +1,10 @@ package buildkite import ( - "bytes" "context" "encoding/json" "fmt" - "io" "net/http" - "os" - "path/filepath" - "strings" "testing" "github.com/google/go-cmp/cmp" @@ -56,100 +51,3 @@ func TestGetPackage(t *testing.T) { t.Fatalf("client.PackagesService.Get(context.Background(),%q, %q, %q) diff: (-got +want)\n%s", "test-org", "my-cool-registry", pkg.ID, diff) } } - -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) - - 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") - - 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) - } - - 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) - } - - if diff := cmp.Diff(p, pkg); diff != "" { - t.Fatalf("client.PackagesService.Create(context.Background(),%q, %q, %v) diff: (-got +want)\n%s", "test-org", "my-cool-registry", tc.in, diff) - } - }) - } -}