support putifabsent #8428

ItamarYuran · 2024-12-17T18:03:10Z

Closes #8380

Change Description

Added support for if-none-match header.
The header can be used for putObject or complete-multipart-upload operations.

changes were tested manually. In case of a collision an error with http status 304 will return, as in aws cli.

github-actions · 2024-12-17T18:11:39Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-12-17T18:12:04Z

E2E Test Results - Quickstart

Isan-Rivkin

Thanks, added 2 important comments.

Isan-Rivkin · 2024-12-19T11:30:21Z

pkg/gateway/errors/errors.go

+	ErrObjectExists: {
+		Code:           "ErrObjectExists",
+		Description:    "Object path already exists in DB",
+		HTTPStatusCode: http.StatusNotModified,


The correct error is PreconditionFailed with status code 412 (StatusPreconditionFailed).
As for the description, the exact AWS message is:

At least one of the pre-conditions you specified did not hold

let's stick to what AWS returns.

Isan-Rivkin · 2024-12-19T12:09:14Z

pkg/gateway/operations/putobject.go

@@ -298,6 +298,11 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	o.Incr("put_object", o.Principal, o.Repository.Name, o.Reference)
 	storageClass := StorageClassFromHeader(req.Header)
 	opts := block.PutOpts{StorageClass: storageClass}
+	err := o.checkIfAbsent(req)


This intention is right but not the approach.
In the current version the function checkIfAbsent implements checkIfAbsent by doing Get to the object and then Put. This is not gonna work because the Get->Put operation is not atomic.
The idea of the feature is that AWS implements this and provide atomicity out of the box.

Instead, our S3 Gateway should only forward the request to AWS and let them handle the precondition check.
Example implementation:

add additional field to opts := block.PutOpts{StorageClass: storageClass, IfNoneMatch} (one line above) that check if the header exist and sets it.

let the ops propagate down to the blockstore adapter and then use it when calling PutObject as an option. We have a challenge here see below 👇 .

Challenge with PutObject call:

Our AWS SDK is outdated, so it does not contain the built-in option in the new version IfNoneMatch.

We have 2 options:

a. Upgrade the AWS SDK (preferable) after verifying there no breaking changes (separate PR)
b. Use the request middleware options that the current AWS SDK version provides to propagate the header.

in AWS SDK introduces in service/s3/v1.60.0

NOTE: Same should apply for Multipart Uploads

Isan-Rivkin · 2024-12-24T12:26:05Z

pkg/gateway/operations/putobject.go

@@ -325,3 +338,20 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	o.SetHeader(w, "ETag", httputil.ETag(blob.Checksum))
 	w.WriteHeader(http.StatusOK)
 }
+
+func (o *PathOperation) checkIfAbsent(req *http.Request) (bool, error) {
+	Header := req.Header.Get(IfNoneMatchHeader)


Use lowercase for variable names

Be explicit in variable name

Suggested change

Header := req.Header.Get(IfNoneMatchHeader)

headerValue := req.Header.Get(IfNoneMatchHeader)

Isan-Rivkin · 2024-12-24T12:35:46Z

pkg/gateway/operations/putobject.go

@@ -298,6 +298,15 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	o.Incr("put_object", o.Principal, o.Repository.Name, o.Reference)
 	storageClass := StorageClassFromHeader(req.Header)
 	opts := block.PutOpts{StorageClass: storageClass}
+	allowOverWrite, err := o.checkIfAbsent(req)


For future generations, please add comment explaining this, something or exactly like this otherwise it's confusing, since this does not actually the verify core part, it's more optimization (since not atomicity here)

pkg/gateway/operations/putobject.go

Isan-Rivkin · 2024-12-24T12:52:04Z

pkg/gateway/operations/putobject.go

@@ -298,6 +298,15 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	o.Incr("put_object", o.Principal, o.Repository.Name, o.Reference)
 	storageClass := StorageClassFromHeader(req.Header)
 	opts := block.PutOpts{StorageClass: storageClass}
+	allowOverWrite, err := o.checkIfAbsent(req)


Suggested change

allowOverWrite, err := o.checkIfAbsent(req)

allowOverwrite, err := o.checkIfAbsent(req)

pkg/gateway/operations/putobject.go

Isan-Rivkin · 2024-12-29T11:24:52Z

esti/s3_gateway_test.go

+		{Path: "main/object1", Content: "data", IfNoneMatch: "", ExpectError: false},
+		{Path: "main/object1", Content: "data", IfNoneMatch: "", ExpectError: false},


Both lines seems redundant, any other intention here? or just delete one?

Isan-Rivkin · 2024-12-29T11:28:14Z

esti/s3_gateway_test.go

+			Key:    aws.String(tc.Path),
+			Body:   strings.NewReader(tc.Content),
+		}
+		logger.Info("Sending PutObject request for Path: %s with If-None-Match: %s\n", tc.Path, tc.IfNoneMatch)


No need in log message here, especially not at Info level, but really no need at all.
if PutObject fails you'll see in the require assertion, if it doesn't - we don't care about the log then.

We try to avoid it as much as possible because in the context of the CI, all tests run together and there are TONS of logs.

esti/s3_gateway_test.go

Isan-Rivkin · 2024-12-29T11:53:55Z

esti/multipart_test.go

@@ -166,7 +166,7 @@ func reverse(s string) string {
 	return string(runes)
 }

-func uploadMultipartParts(t *testing.T, ctx context.Context, logger logging.Logger, resp *s3.CreateMultipartUploadOutput, parts [][]byte, firstIndex int) []types.CompletedPart {
+func uploadMultipartParts(t *testing.T, ctx context.Context, svc *s3.Client, logger logging.Logger, resp *s3.CreateMultipartUploadOutput, parts [][]byte, firstIndex int) []types.CompletedPart {


What's the reason here to override the global svc and pass here a custom client svc *s3.Client?
(Even if required please don't use the same name as the global svc since it's confusing)

The override is so i can use that func, since my test doesn't use that global client

esti/s3_gateway_test.go

Isan-Rivkin

Thanks! Added small comments nothing big - looks very good!

esti/s3_gateway_test.go

pkg/gateway/operations/putobject.go

pkg/gateway/operations/postobject.go

pkg/gateway/operations/putobject.go

pkg/gateway/operations/operation_utils.go

Isan-Rivkin

LGTM!!! 🥳 🥳 🥳

ItamarYuran added 3 commits December 16, 2024 14:21

works

d92b29d

multi if none mash

f18a67c

trimmmin' them

81de1fc

ItamarYuran added the include-changelog PR description should be included in next release changelog label Dec 17, 2024

ItamarYuran requested a review from a team December 17, 2024 18:03

ItamarYuran linked an issue Dec 17, 2024 that may be closed by this pull request

Support putIfAbsent #8380

Closed

yalla

7d2a511

Isan-Rivkin requested changes Dec 19, 2024

View reviewed changes

ItamarYuran mentioned this pull request Dec 19, 2024

Upgrade AWS SDK #8436

Open

ItamarYuran added 3 commits December 22, 2024 15:00

only if absent babe

768ef8c

erroring

b07a5ab

tests v1

c8a508c

Isan-Rivkin requested changes Dec 24, 2024

View reviewed changes

ItamarYuran added 15 commits December 24, 2024 14:09

tests v2

81c8a79

test v3

5fc97da

test v4

1dd06fd

test v5

a635bb4

test yalla

717f224

user metadata

d558f57

beginning s3 client

150776e

beginning s3 client

d7e22f4

local host

4b1533b

local host

1528c90

amen

08cc251

main/object

2873d3a

yalla

7e42c88

yalla kadima

b46deb3

debug

126144f

pront all headers

4a56ace

github-advanced-security bot found potential problems Dec 25, 2024

View reviewed changes

pkg/gateway/operations/putobject.go Fixed Show fixed Hide fixed

ItamarYuran added 14 commits December 26, 2024 09:04

I really think it will work now

c4387d9

this time bby

51d8105

now is the time

6f2431a

yalla

047bd48

lets see

55a2d72

maybe now

c620816

with multipart test

69764b1

test will now pass

0b2e2b4

svc to s3

8b4d12f

smol change

67da789

its gonna work i tell u

005f174

nooooow

278a345

looks like we got it

94ed11f

formatted

e7508d5

ItamarYuran requested a review from Isan-Rivkin December 29, 2024 09:27

Isan-Rivkin requested changes Dec 30, 2024

View reviewed changes

ItamarYuran added 3 commits December 30, 2024 22:37

changes

f1518b9

changes

5007413

changes

1d9ad3a

ItamarYuran requested a review from Isan-Rivkin December 30, 2024 22:34

completed tests

fb59b43

Isan-Rivkin requested changes Jan 1, 2025

View reviewed changes

final adjustments

fa73859

Isan-Rivkin self-requested a review January 1, 2025 13:15

Isan-Rivkin approved these changes Jan 1, 2025

View reviewed changes

ItamarYuran merged commit bde54c0 into master Jan 1, 2025
38 checks passed

ItamarYuran deleted the 8380-support-putifabsent branch January 1, 2025 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support putifabsent #8428

support putifabsent #8428

ItamarYuran commented Dec 17, 2024

github-actions bot commented Dec 17, 2024

github-actions bot commented Dec 17, 2024 •

edited

Loading

Isan-Rivkin left a comment

Isan-Rivkin Dec 19, 2024

Isan-Rivkin Dec 19, 2024 •

edited

Loading

Isan-Rivkin Dec 24, 2024

Isan-Rivkin Dec 24, 2024

Isan-Rivkin Dec 24, 2024

Isan-Rivkin Dec 29, 2024

Isan-Rivkin Dec 29, 2024

Isan-Rivkin Dec 29, 2024

ItamarYuran Dec 30, 2024 •

edited

Loading

Isan-Rivkin left a comment

Isan-Rivkin left a comment

	Header := req.Header.Get(IfNoneMatchHeader)
	headerValue := req.Header.Get(IfNoneMatchHeader)

	allowOverWrite, err := o.checkIfAbsent(req)
	allowOverwrite, err := o.checkIfAbsent(req)

		{Path: "main/object1", Content: "data", IfNoneMatch: "", ExpectError: false},
		{Path: "main/object1", Content: "data", IfNoneMatch: "", ExpectError: false},

support putifabsent #8428

support putifabsent #8428

Conversation

ItamarYuran commented Dec 17, 2024

Change Description

github-actions bot commented Dec 17, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Dec 17, 2024 • edited Loading

E2E Test Results - Quickstart

Isan-Rivkin left a comment

Choose a reason for hiding this comment

Isan-Rivkin Dec 19, 2024

Choose a reason for hiding this comment

Isan-Rivkin Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Challenge with PutObject call:

We have 2 options:

NOTE: Same should apply for Multipart Uploads

Isan-Rivkin Dec 24, 2024

Choose a reason for hiding this comment

Isan-Rivkin Dec 24, 2024

Choose a reason for hiding this comment

Isan-Rivkin Dec 24, 2024

Choose a reason for hiding this comment

Isan-Rivkin Dec 29, 2024

Choose a reason for hiding this comment

Isan-Rivkin Dec 29, 2024

Choose a reason for hiding this comment

Isan-Rivkin Dec 29, 2024

Choose a reason for hiding this comment

ItamarYuran Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Isan-Rivkin left a comment

Choose a reason for hiding this comment

Isan-Rivkin left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024 •

edited

Loading

Isan-Rivkin Dec 19, 2024 •

edited

Loading

ItamarYuran Dec 30, 2024 •

edited

Loading