Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support putifabsent #8428

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
d92b29d
works
ItamarYuran Dec 16, 2024
f18a67c
multi if none mash
ItamarYuran Dec 17, 2024
81de1fc
trimmmin' them
ItamarYuran Dec 17, 2024
7d2a511
yalla
ItamarYuran Dec 17, 2024
768ef8c
only if absent babe
ItamarYuran Dec 22, 2024
b07a5ab
erroring
ItamarYuran Dec 22, 2024
c8a508c
tests v1
ItamarYuran Dec 24, 2024
81c8a79
tests v2
ItamarYuran Dec 24, 2024
5fc97da
test v3
ItamarYuran Dec 24, 2024
1dd06fd
test v4
ItamarYuran Dec 24, 2024
a635bb4
test v5
ItamarYuran Dec 24, 2024
717f224
test yalla
ItamarYuran Dec 24, 2024
d558f57
user metadata
ItamarYuran Dec 25, 2024
150776e
beginning s3 client
ItamarYuran Dec 25, 2024
d7e22f4
beginning s3 client
ItamarYuran Dec 25, 2024
4b1533b
local host
ItamarYuran Dec 25, 2024
1528c90
local host
ItamarYuran Dec 25, 2024
08cc251
amen
ItamarYuran Dec 25, 2024
2873d3a
main/object
ItamarYuran Dec 25, 2024
7e42c88
yalla
ItamarYuran Dec 25, 2024
b46deb3
yalla kadima
ItamarYuran Dec 25, 2024
126144f
debug
ItamarYuran Dec 25, 2024
4a56ace
pront all headers
ItamarYuran Dec 25, 2024
c4387d9
I really think it will work now
ItamarYuran Dec 26, 2024
51d8105
this time bby
ItamarYuran Dec 26, 2024
6f2431a
now is the time
ItamarYuran Dec 26, 2024
047bd48
yalla
ItamarYuran Dec 26, 2024
55a2d72
lets see
ItamarYuran Dec 26, 2024
c620816
maybe now
ItamarYuran Dec 26, 2024
69764b1
with multipart test
ItamarYuran Dec 26, 2024
0b2e2b4
test will now pass
ItamarYuran Dec 26, 2024
8b4d12f
svc to s3
ItamarYuran Dec 26, 2024
67da789
smol change
ItamarYuran Dec 26, 2024
005f174
its gonna work i tell u
ItamarYuran Dec 26, 2024
278a345
nooooow
ItamarYuran Dec 26, 2024
94ed11f
looks like we got it
ItamarYuran Dec 26, 2024
e7508d5
formatted
ItamarYuran Dec 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkg/gateway/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ const (

// S3 extended errors.
ErrContentSHA256Mismatch
ErrObjectExists

// Lakefs errors
ERRLakeFSNotSupported
Expand Down Expand Up @@ -756,7 +757,11 @@ var Codes = errorCodeMap{
Description: "Invalid version found in the request",
HTTPStatusCode: http.StatusNotFound,
},

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
// LakeFS errors
ERRLakeFSNotSupported: {
Code: "ERRLakeFSNotSupported",
Expand Down
5 changes: 5 additions & 0 deletions pkg/gateway/operations/postobject.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ func (controller *PostObject) HandleCompleteMultipartUpload(w http.ResponseWrite
_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrInternalError))
return
}
err = o.checkIfAbsent(req)
if err != nil {
_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrObjectExists))
return
}
objName := multiPart.PhysicalAddress
req = req.WithContext(logging.AddFields(req.Context(), logging.Fields{logging.PhysicalAddressFieldKey: objName}))
xmlMultipartComplete, err := io.ReadAll(req.Body)
Expand Down
26 changes: 25 additions & 1 deletion pkg/gateway/operations/putobject.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
)

const (
IfNoneMatchHeader = "If-None-Match"
CopySourceHeader = "x-amz-copy-source"
CopySourceRangeHeader = "x-amz-copy-source-range"
QueryParamUploadID = "uploadId"
Expand All @@ -30,7 +31,6 @@ type PutObject struct{}

func (controller *PutObject) RequiredPermissions(req *http.Request, repoID, _, destPath string) (permissions.Node, error) {
copySource := req.Header.Get(CopySourceHeader)

if len(copySource) == 0 {
return permissions.Node{
Permission: permissions.Permission{
Expand Down Expand Up @@ -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)
Copy link
Contributor

@Isan-Rivkin Isan-Rivkin Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. add additional field to opts := block.PutOpts{StorageClass: storageClass, IfNoneMatch} (one line above) that check if the header exist and sets it.
  2. 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.

NOTE: Same should apply for Multipart Uploads

if err != nil {
_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrObjectExists))
return
}
address := o.PathProvider.NewPath()
blob, err := upload.WriteBlob(req.Context(), o.BlockStore, o.Repository.StorageNamespace, address, req.Body, req.ContentLength, opts)
if err != nil {
Expand Down Expand Up @@ -325,3 +330,22 @@ 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) error {
Header := req.Header.Get(IfNoneMatchHeader)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use lowercase for variable names
  • Be explicit in variable name
Suggested change
Header := req.Header.Get(IfNoneMatchHeader)
headerValue := req.Header.Get(IfNoneMatchHeader)

switch Header {
case "":
return nil
case "*":
_, err := o.Catalog.GetEntry(req.Context(), o.Repository.Name, o.Reference, o.Path, catalog.GetEntryParams{})
if err == nil {
return gatewayErrors.ErrObjectExists
}
default:
_, err := o.Catalog.GetEntry(req.Context(), o.Repository.Name, o.Reference, Header, catalog.GetEntryParams{})
if err == nil {
return gatewayErrors.ErrObjectExists
}
}
return nil
}
Loading