S3 support by or-shachar · Pull Request #7 · bradfitz/go-tool-cache

or-shachar · 2023-11-16T18:06:55Z

My humble attempt to do #6

Signed-off-by: or-shachar <or.shachar@wiz.io>

klauspost

Was going to write my own, but saw you had already made the bulk of the work.

klauspost · 2024-11-14T14:44:17Z

cachers/s3.go

+	body, err := io.ReadAll(result.Body)
+	if err != nil {
+		return "", "", err
+	}
+	if err := json.Unmarshal(body, &av); err != nil {


Let's stream the response:

Suggested change

body, err := io.ReadAll(result.Body)

if err != nil {

return "", "", err

}

if err := json.Unmarshal(body, &av); err != nil {

dec := json.NewDecoder(result.Body)

if err := dec.Decode(&av); err != nil {

(but maybe drop, see later)

changed implementation here - or-shachar@0bc8aa9#diff-3996ec7951ff9218f18b180f064a24e55ee57da5f81810fcb063e37f339896caR69

klauspost · 2024-11-14T14:44:29Z

cachers/s3.go

+		if c.verbose {
+			log.Printf("error S3 get for %s:  %v", actionKey, err)
+		}
+		return "", "", fmt.Errorf("unexpected S3 get for %s:  %v", actionKey, err)


Suggested change

return "", "", fmt.Errorf("unexpected S3 get for %s: %v", actionKey, err)

return "", "", fmt.Errorf("unexpected S3 get for %s: %w", actionKey, err)

klauspost · 2024-11-14T14:45:30Z

cachers/s3.go

+		var ae smithy.APIError
+		if errors.As(err, &ae) {
+			code := ae.ErrorCode()
+			return code == "AccessDenied" || code == "NoSuchKey"


AccessDenied?

Good question. Not sure why I chose AccessDenied, maybe my original permissions were only to s3:GetObject without s3:ListBucket which results in AccessDenied in case key cannot be found.

klauspost · 2024-11-14T14:51:54Z

cachers/s3.go

+			if c.verbose {
+				log.Printf("error S3 get for %s:  %v", outputKey, getOutputErr)
+			}
+			return "", "", fmt.Errorf("unexpected S3 get for %s:  %v", outputKey, getOutputErr)


Suggested change

return "", "", fmt.Errorf("unexpected S3 get for %s: %v", outputKey, getOutputErr)

return "", "", fmt.Errorf("unexpected S3 get for %s: %w", outputKey, getOutputErr)

klauspost · 2024-11-18T10:36:06Z

README.md

+We support S3 backend for caching.
+You can connect to S3 backend by setting the following parameters:
+- `GOCACHE_S3_BUCKET` - Name of S3 bucket
+- `GOCACHE_AWS_REGION` - AWS Region of bucket


Suggested change

- `GOCACHE_AWS_REGION` - AWS Region of bucket

- `GOCACHE_S3_REGION` - AWS Region of bucket

I think having the same prefix makes it clearer. I suggest using the service name rather than the vendor name. (same goes for other env vars)

klauspost · 2024-11-18T10:55:51Z

cachers/s3.go

+	os := runtime.GOOS
+	// get current version of Go
+	ver := strings.ReplaceAll(strings.ReplaceAll(runtime.Version(), " ", "-"), ":", "-")
+	prefix := fmt.Sprintf("cache/%s/%s/%s/%s", cacheKey, arc, os, ver)


It would be good behavior to make the cache prefix configurable. No biggie. Consider the default to be gocache to make it a bit easier to identify.

Handled by @nfi-hashicorp in https://github.com/or-shachar/go-tool-cache/pull/12/files#diff-280fcccb6cf44da94fc69a76ba7c51472a2638841030b96f92ab6e3b525da375R253

klauspost · 2024-11-18T11:10:48Z

cachers/s3.go

+
+		putBody = outputResult.Body
+	}
+	diskPath, err = c.diskCache.Put(ctx, actionID, outputID, av.Size, putBody)


Need help: Documentation indicates that modtime of the file is used for "something".

You may need some way to set the mod-time to the object write time. This should be available from S3.

Can you please reference the relevant documentation?

klauspost · 2024-11-18T11:12:00Z

cachers/s3.go

+		OutputID: outputID,
+		Size:     size,
+	}
+	avj, err := json.Marshal(av)


2 PutObject calls per write seems like one to many. And a place where inconsistencies could creep in. Having 1 write will make it atomic, so probably that is a win here.

I suggest you store the object using the actionID only and add objectID as a custom header.

There is a 2KB limit on metadata, so maybe check if that is likely to be exceeded.

This was done on the main branch of my fork

klauspost · 2024-11-18T11:13:05Z

cachers/s3.go

+		_, err = client.PutObject(ctx, &s3.PutObjectInput{
+			Bucket:        &c.Bucket,
+			Key:           &outputKey,
+			Body:          &readerForS3,
+			ContentLength: size,
+		})


Suggested change

_, err = client.PutObject(ctx, &s3.PutObjectInput{

Bucket: &c.Bucket,

Key: &outputKey,

Body: &readerForS3,

ContentLength: size,

})

_, err = client.PutObject(ctx, &s3.PutObjectInput{

Bucket: &c.Bucket,

Key: &outputKey,

Body: &readerForS3,

ContentLength: size,

Metadata: map[string]string{"gocache-output-id": outputID},

})

done here or-shachar@0bc8aa9#diff-3996ec7951ff9218f18b180f064a24e55ee57da5f81810fcb063e37f339896caR18

klauspost · 2024-11-18T11:14:19Z

cachers/s3.go

+		})
+	}
+	if size > 0 && err == nil {
+		outputKey := c.outputKey(outputID)


Suggested change

outputKey := c.outputKey(outputID)

outputKey := c.actionKey(actionID)

klauspost · 2024-11-18T14:41:32Z

@or-shachar @bradfitz Let me know if you are not interested in pursuing this, and I will just do a fork.

or-shachar · 2024-11-18T22:54:00Z

Oh seeing this only now. I can probably work on this on Friday but not earlier unfortunately...

Thanks for the feedback! 🙏

klauspost · 2024-11-21T10:08:22Z

@or-shachar Wasn't pushing - just asking, since I didn't know if you were still interested after having it open for a year.

Either way, you can just ping me for S3 questions. And we can decide if we fork or not.

If you want a custom endpoint to test against, you can use https://play.min.io, access/secret key: minioadmin/minioadmin - It has TLS enabled as may be visible from the URL.

To create a bucket, using our commandline client, use the default config and mc mb play/shachar should create it. We wipe it on regular intervals, but should allow you to test.

or-shachar · 2024-11-21T13:04:49Z

We use my flavor of S3 for more than a year. A major improvement over traditional CI cache.

or-shachar · 2024-12-01T17:28:34Z

Hey @klauspost - thanks for your kind feedback... I did made some progress on my fork and started working on main branch there. I'll incorporate your feedback there.

Generally - I understand you want to abstract AWS terms so it can work with any S3 compatible API, right? Feel free to open another PR to the fork as I don't think @bradfitz is here and I'm not the maintainer of this repo.

or-shachar · 2024-12-01T17:56:14Z

To avoid confusing - I'm closing this PR as we've made a lot of progress on my fork, main branch, since then.

or-shachar force-pushed the s3-support branch from 1513f8b to 5ab32eb Compare November 16, 2023 18:07

Added S3 support

66b2de7

Signed-off-by: or-shachar <or.shachar@wiz.io>

or-shachar force-pushed the s3-support branch from 5ab32eb to 66b2de7 Compare November 16, 2023 19:49

klauspost reviewed Nov 18, 2024

View reviewed changes

or-shachar closed this Dec 1, 2024

or-shachar mentioned this pull request Dec 2, 2024

fix: error wrapping and path building or-shachar/go-tool-cache#14

Merged

or-shachar deleted the s3-support branch November 25, 2025 22:24

	return "", "", fmt.Errorf("unexpected S3 get for %s: %v", actionKey, err)
	return "", "", fmt.Errorf("unexpected S3 get for %s: %w", actionKey, err)

	- `GOCACHE_AWS_REGION` - AWS Region of bucket
	- `GOCACHE_S3_REGION` - AWS Region of bucket

	outputKey := c.outputKey(outputID)
	outputKey := c.actionKey(actionID)

Conversation

or-shachar commented Nov 16, 2023

Uh oh!

klauspost left a comment

Choose a reason for hiding this comment

Uh oh!

klauspost Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klauspost Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klauspost commented Nov 18, 2024

Uh oh!

or-shachar commented Nov 18, 2024

Uh oh!

klauspost commented Nov 21, 2024

Uh oh!

or-shachar commented Nov 21, 2024

Uh oh!

or-shachar commented Dec 1, 2024

Uh oh!

or-shachar commented Dec 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

klauspost Nov 14, 2024 •

edited

Loading

klauspost Nov 18, 2024 •

edited

Loading