TrueCloudLab/frostfs-s3-gw
17
3
Fork 19
Code Issues 28 Pull requests 6 Projects Releases 18 Packages Activity Actions

[#42] Support expiration lifecycle #418

Merged
alexvanin merged 1 commit from mbiryukova/frostfs-s3-gw:feature/support_lifecycle into feature/lifecycle 2024-07-11 11:41:51 +00:00
Conversation 0 Commits 1 Files changed 25 +1260 -61
mbiryukova commented 2024-07-03 14:35:00 +00:00
Member
Copy link

Closes #42

Signed-off-by: Marina Biryukova m.biryukova@yadro.com

Closes #42 Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>
mbiryukova self-assigned this 2024-07-03 14:35:06 +00:00
mbiryukova requested review from storage-services-committers 2024-07-03 14:43:40 +00:00
mbiryukova requested review from storage-services-developers 2024-07-03 14:43:47 +00:00
mbiryukova changed title from [#42] Support expiration lifecycle to WIP: [#42] Support expiration lifecycle 2024-07-04 13:14:25 +00:00
mbiryukova force-pushed feature/support_lifecycle from 7f3fba1f55 to 1c61e59389 2024-07-04 16:40:36 +00:00 Compare
mbiryukova changed title from WIP: [#42] Support expiration lifecycle to [#42] Support expiration lifecycle 2024-07-04 16:48:49 +00:00
dkirillov reviewed 2024-07-05 07:53:09 +00:00
api/data/info.go Outdated
@ -90,2 +91,4 @@
func (b *BucketInfo) CORSObjectName() string { return bktCORSConfigurationObject }
func (b *BucketInfo) LifecycleConfigurationObjectName() string {
return bktLifecycleConfigurationObject
dkirillov commented 2024-07-05 06:42:32 +00:00
Member
Copy link

Let's also use container id or at least bucket name to form lifecycle object name

Let's also use container id or at least bucket name to form lifecycle object name
dkirillov marked this conversation as resolved
api/data/lifecycle.go Outdated
@ -0,0 +19,4 @@
Expiration *LifecycleExpiration `xml:"Expiration,omitempty"`
Filter *LifecycleRuleFilter `xml:"Filter,omitempty"`
ID string `xml:"ID,omitempty"`
NoncurrentVersionExpiration *NoncurrentVersionExpiration `xml:"NoncurrentVersionExpiration,omitempty"`
dkirillov commented 2024-07-05 06:38:23 +00:00
Member
Copy link

Maybe Noncurrent* -> NonCurrent*?

Maybe `Noncurrent*` -> `NonCurrent*`?
dkirillov marked this conversation as resolved
api/handler/lifecycle.go Outdated
@ -0,0 +24,4 @@
var (
ctx = r.Context()
reqInfo = middleware.GetReqInfo(ctx)
)
dkirillov commented 2024-07-05 06:44:01 +00:00
Member
Copy link

I suppose we can write just

ctx := r.Context()
reqInfo := middleware.GetReqInfo(ctx)
I suppose we can write just ```golang ctx := r.Context() reqInfo := middleware.GetReqInfo(ctx) ```
dkirillov marked this conversation as resolved
api/handler/lifecycle.go Outdated
@ -0,0 +39,4 @@
}
if err = middleware.EncodeToResponse(w, cfg); err != nil {
h.logAndSendError(w, "something went wrong", reqInfo, err)
dkirillov commented 2024-07-05 06:45:30 +00:00
Member
Copy link

Let's write more meaningful message (e.g. could not encode GetBucketLifecycle response)

Let's write more meaningful message (e.g. `could not encode GetBucketLifecycle response`)
mbiryukova commented 2024-07-05 13:58:06 +00:00
Author
Member
Copy link

Then maybe it’s worth fixing in other places too

Then maybe it’s worth fixing in other places too
alexvanin commented 2024-07-08 14:50:41 +00:00
Owner
Copy link

Big things start small

Big things start small
dkirillov marked this conversation as resolved
api/handler/lifecycle.go Outdated
@ -0,0 +52,4 @@
tee = io.TeeReader(r.Body, &buf)
)
// Content-Md5 is required and should be set
dkirillov commented 2024-07-05 06:34:08 +00:00
Member
Copy link

Why?

Why?
mbiryukova commented 2024-07-05 13:59:35 +00:00
Author
Member
Copy link

Will add link to spec

Will add link to [spec](https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketLifecycleConfiguration.html)
dkirillov commented 2024-07-05 14:31:37 +00:00
Member
Copy link

Oh, sorry.
I was seeing wrong (deprecated method) spec https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketLifecycle.html

Oh, sorry. I was seeing wrong (deprecated method) spec https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketLifecycle.html
dkirillov marked this conversation as resolved
api/layer/lifecycle.go Outdated
@ -0,0 +36,4 @@
lifecycleBkt = p.BktInfo
prm.CopiesNumber = p.CopiesNumbers
} else {
lifecycleBkt = &data.BucketInfo{CID: n.lifecycleCnr}
dkirillov commented 2024-07-05 07:00:37 +00:00
Member
Copy link

It seems we have to fill more info about lifecycle container because in objectPutAndHash the HomomorphicHashDisabled is used.
Also when we use separate container to store lifecycle configuration we should use gateway key (in objectPutAndHash function auth params for put request is being filled with user bearer token from access box or anonymous key that slightly incorrect)

The same for other methods

It seems we have to fill more info about lifecycle container because in `objectPutAndHash` the `HomomorphicHashDisabled` is used. Also when we use separate container to store lifecycle configuration we should use gateway key (in `objectPutAndHash` function auth params for put request is being filled with user bearer token from access box or anonymous key that slightly incorrect) The same for other methods
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -172,2 +176,4 @@
user.IDFromKey(&gateOwner, a.key.PrivateKey.PublicKey)
nnsClient := ns.NNS{}
err = nnsClient.Dial(a.cfg.GetString(cfgRPCEndpoint))
dkirillov commented 2024-07-05 07:46:01 +00:00
Member
Copy link

It seems we can use a.bucketResolver

It seems we can use `a.bucketResolver`
🚀 1
dkirillov marked this conversation as resolved
alexvanin changed title from [#42] Support expiration lifecycle to [#42] Support expiration lifecycle 2024-07-08 09:04:23 +00:00
alexvanin changed target branch from master to feature/lifecycle 2024-07-08 09:04:43 +00:00
mbiryukova force-pushed feature/support_lifecycle from 1c61e59389 to dfaa180995 2024-07-08 12:42:35 +00:00 Compare
alexvanin reviewed 2024-07-08 14:49:32 +00:00
api/layer/lifecycle.go Outdated
@ -0,0 +52,4 @@
return apiErr.GetAPIError(apiErr.ErrInvalidDigest)
}
if hex.EncodeToString(hashBytes) != hex.EncodeToString(md5) {
alexvanin commented 2024-07-08 14:48:08 +00:00
Owner
Copy link

I don't quite understand this condition. Can you elaborate what are we checking here?

I don't quite understand this condition. Can you elaborate what are we checking here?
mbiryukova commented 2024-07-08 15:15:19 +00:00
Author
Member
Copy link

Seems that just hash bytes can be compared here

Seems that just hash bytes can be compared here
alexvanin marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -1037,0 +1054,4 @@
}
var id cid.ID
if strings.Contains(containerString, ".") {
alexvanin commented 2024-07-08 14:39:01 +00:00
Owner
Copy link

Is this pattern is being used somewhere else?
I would expect something like

err = id.DecodeString(containerString)
if err != nil {
  id, err = a.bucketResolver.Resolve(ctx, containerString)
}
if err != nil {
  return nil, fmt.Errorf("resolve container name %s: %w", containerString, err)
}
return getContainerInfo(ctx, id, a.pool)
Is this pattern is being used somewhere else? I would expect something like ```go err = id.DecodeString(containerString) if err != nil { id, err = a.bucketResolver.Resolve(ctx, containerString) } if err != nil { return nil, fmt.Errorf("resolve container name %s: %w", containerString, err) } return getContainerInfo(ctx, id, a.pool) ```
docs/configuration.md Outdated
@ -710,1 +711,4 @@
# `containers` section
Containers configuration.
alexvanin commented 2024-07-08 14:31:52 +00:00
Owner
Copy link

Let's add a bit more details.

Section for well-known containers to store s3-related data and settings.
Let's add a bit more details. ``` Section for well-known containers to store s3-related data and settings. ```
alexvanin marked this conversation as resolved
docs/configuration.md Outdated
@ -711,0 +720,4 @@
| Parameter | Type | SIGHUP reload | Default value | Description |
|-------------|----------|---------------|---------------|----------------------------------------------|
| `lifecycle` | `string` | no | | Container name for lifecycle configurations. |
alexvanin commented 2024-07-08 14:33:07 +00:00
Owner
Copy link

Add If not set, container of the bucket is used

Add `If not set, container of the bucket is used`
alexvanin marked this conversation as resolved
mbiryukova force-pushed feature/support_lifecycle from dfaa180995 to d54b26dbfd 2024-07-08 15:52:29 +00:00 Compare
alexvanin approved these changes 2024-07-09 06:48:00 +00:00
alexvanin left a comment
Owner
Copy link

Looks good to me, lets see how it works in feature branch together with new lifecycler component.

Looks good to me, lets see how it works in feature branch together with new lifecycler component.
dkirillov reviewed 2024-07-09 07:51:43 +00:00
dkirillov reviewed 2024-07-09 08:05:11 +00:00
api/layer/lifecycle.go Outdated
@ -0,0 +65,4 @@
zap.String("oid", objID.EncodeToString()))
}
return apiErr.GetAPIError(apiErr.ErrInvalidDigest)
dkirillov commented 2024-07-09 08:05:11 +00:00
Member
Copy link

Why do we return this error?

By the way, why we don't update tree right after successful object uploading?

Why do we return this error? By the way, why we don't update tree right after successful object uploading?
mbiryukova commented 2024-07-09 08:56:16 +00:00
Author
Member
Copy link

Because Content-MD5 value doesn't match hash received after object uploading, configuration shouldn't be saved in this case

Because `Content-MD5` value doesn't match hash received after object uploading, configuration shouldn't be saved in this case
👍 1
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-07-09 08:06:37 +00:00
api/layer/lifecycle.go Outdated
@ -0,0 +75,4 @@
}
if !objIDToDeleteNotFound {
if n.lifecycleCnrInfo == nil {
dkirillov commented 2024-07-09 08:06:37 +00:00
Member
Copy link

Can we put extract these lines (78-88) to separate function (we have some duplication now)?

Can we put extract these lines (78-88) to separate function (we have some duplication now)?
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-07-09 08:09:35 +00:00
cmd/s3-gw/app.go Outdated
@ -1037,0 +1057,4 @@
if err != nil {
id, err = a.bucketResolver.Resolve(ctx, containerString)
}
if err != nil {
dkirillov commented 2024-07-09 08:09:35 +00:00
Member
Copy link

It seems we can move this condition into previous one. Now it looks a little odd.

For example:

if err = id.DecodeString(containerString); err != nil {
	if id, err = a.bucketResolver.Resolve(ctx, containerString); err != nil {
		return nil, fmt.Errorf("resolve container name %s: %w", containerString, err)
	}
}
It seems we can move this condition into previous one. Now it looks a little odd. For example: ```golang if err = id.DecodeString(containerString); err != nil { if id, err = a.bucketResolver.Resolve(ctx, containerString); err != nil { return nil, fmt.Errorf("resolve container name %s: %w", containerString, err) } } ```
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-07-09 08:13:57 +00:00
cmd/s3-gw/app.go Outdated
@ -171,15 +174,22 @@ func (a *App) initLayer() {
var gateOwner user.ID
user.IDFromKey(&gateOwner, a.key.PrivateKey.PublicKey)
lifecycleCnrInfo, err := a.fetchContainerInfo(ctx, cfgContainersLifecycle)
dkirillov commented 2024-07-09 08:13:56 +00:00
Member
Copy link

Can we return exactly one non nil value from the fetchContainerInfo method? When you briefly look at this it seems we must configure container.lifecycle (but actually we must not).

We can write something like this:

	var lifecycleCnrInfo *data.BucketInfo
	if a.cfg.IsSet(cfgContainersLifecycle) {
		lifecycleCnrInfo, err = a.fetchContainerInfo(ctx, cfgContainersLifecycle)
		if err != nil {
			a.log.Fatal(logs.CouldNotFetchLifecycleContainerInfo, zap.Error(err))
		}
	}
Can we return exactly one non nil value from the `fetchContainerInfo` method? When you briefly look at this it seems we must configure `container.lifecycle` (but actually we must not). We can write something like this: ```golang var lifecycleCnrInfo *data.BucketInfo if a.cfg.IsSet(cfgContainersLifecycle) { lifecycleCnrInfo, err = a.fetchContainerInfo(ctx, cfgContainersLifecycle) if err != nil { a.log.Fatal(logs.CouldNotFetchLifecycleContainerInfo, zap.Error(err)) } } ```
dkirillov marked this conversation as resolved
mbiryukova force-pushed feature/support_lifecycle from d54b26dbfd to 0e1ab11a1b 2024-07-09 10:32:09 +00:00 Compare
dkirillov approved these changes 2024-07-09 11:09:08 +00:00
alexvanin merged commit 0e1ab11a1b into feature/lifecycle 2024-07-11 11:41:51 +00:00
alexvanin deleted branch feature/support_lifecycle 2024-07-11 11:42:07 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
dkirillov
TrueCloudLab/storage-services-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
mbiryukova
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#418
No description provided.
Delete branch "mbiryukova/frostfs-s3-gw:feature/support_lifecycle"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?