From f061cb0e73db99568fd0f814b1ab269edc72e28e Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Thu, 7 Nov 2024 17:28:16 +0300 Subject: [PATCH 1/3] [#536] Add prefix to lifecycle rule Signed-off-by: Marina Biryukova --- api/data/lifecycle.go | 1 + api/handler/lifecycle.go | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/api/data/lifecycle.go b/api/data/lifecycle.go index 4d0828f..cd79423 100644 --- a/api/data/lifecycle.go +++ b/api/data/lifecycle.go @@ -20,6 +20,7 @@ type ( Filter *LifecycleRuleFilter `xml:"Filter,omitempty"` ID string `xml:"ID,omitempty"` NonCurrentVersionExpiration *NonCurrentVersionExpiration `xml:"NoncurrentVersionExpiration,omitempty"` + Prefix string `xml:"Prefix,omitempty"` } AbortIncompleteMultipartUpload struct { diff --git a/api/handler/lifecycle.go b/api/handler/lifecycle.go index 905c068..85a85f9 100644 --- a/api/handler/lifecycle.go +++ b/api/handler/lifecycle.go @@ -218,6 +218,10 @@ func checkLifecycleConfiguration(ctx context.Context, cfg *data.LifecycleConfigu if err := checkLifecycleRuleFilter(rule.Filter); err != nil { return err } + + if rule.Filter != nil && rule.Filter.Prefix != "" && rule.Prefix != "" { + return fmt.Errorf("%w: rule cannot have two prefixes", apierr.GetAPIError(apierr.ErrMalformedXML)) + } } return nil -- 2.45.2 From cdc07f4d648ec4f8a9055d9383cfd57b876378a7 Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Fri, 8 Nov 2024 11:45:10 +0300 Subject: [PATCH 2/3] [#536] Fix error codes in lifecycle configuration check Signed-off-by: Marina Biryukova --- api/handler/lifecycle.go | 49 +++++++++----- api/handler/lifecycle_test.go | 123 ++++++++++++++++++++++++++-------- 2 files changed, 127 insertions(+), 45 deletions(-) diff --git a/api/handler/lifecycle.go b/api/handler/lifecycle.go index 85a85f9..e559365 100644 --- a/api/handler/lifecycle.go +++ b/api/handler/lifecycle.go @@ -97,7 +97,7 @@ func (h *handler) PutBucketLifecycleHandler(w http.ResponseWriter, r *http.Reque } if err = checkLifecycleConfiguration(ctx, cfg, &networkInfo); err != nil { - h.logAndSendError(ctx, w, "invalid lifecycle configuration", reqInfo, fmt.Errorf("%w: %s", apierr.GetAPIError(apierr.ErrMalformedXML), err.Error())) + h.logAndSendError(ctx, w, "invalid lifecycle configuration", reqInfo, err) return } @@ -140,58 +140,58 @@ func checkLifecycleConfiguration(ctx context.Context, cfg *data.LifecycleConfigu now := layer.TimeNow(ctx) if len(cfg.Rules) > maxRules { - return fmt.Errorf("number of rules cannot be greater than %d", maxRules) + return fmt.Errorf("%w: number of rules cannot be greater than %d", apierr.GetAPIError(apierr.ErrInvalidRequest), maxRules) } ids := make(map[string]struct{}, len(cfg.Rules)) for i, rule := range cfg.Rules { if _, ok := ids[rule.ID]; ok && rule.ID != "" { - return fmt.Errorf("duplicate 'ID': %s", rule.ID) + return fmt.Errorf("%w: duplicate 'ID': %s", apierr.GetAPIError(apierr.ErrInvalidArgument), rule.ID) } ids[rule.ID] = struct{}{} if len(rule.ID) > maxRuleIDLen { - return fmt.Errorf("'ID' value cannot be longer than %d characters", maxRuleIDLen) + return fmt.Errorf("%w: 'ID' value cannot be longer than %d characters", apierr.GetAPIError(apierr.ErrInvalidArgument), maxRuleIDLen) } if rule.Status != data.LifecycleStatusEnabled && rule.Status != data.LifecycleStatusDisabled { - return fmt.Errorf("invalid lifecycle status: %s", rule.Status) + return fmt.Errorf("%w: invalid lifecycle status: %s", apierr.GetAPIError(apierr.ErrMalformedXML), rule.Status) } if rule.AbortIncompleteMultipartUpload == nil && rule.Expiration == nil && rule.NonCurrentVersionExpiration == nil { - return fmt.Errorf("at least one action needs to be specified in a rule") + return fmt.Errorf("%w: at least one action needs to be specified in a rule", apierr.GetAPIError(apierr.ErrInvalidRequest)) } if rule.AbortIncompleteMultipartUpload != nil { if rule.AbortIncompleteMultipartUpload.DaysAfterInitiation != nil && *rule.AbortIncompleteMultipartUpload.DaysAfterInitiation <= 0 { - return fmt.Errorf("days after initiation must be a positive integer: %d", *rule.AbortIncompleteMultipartUpload.DaysAfterInitiation) + return fmt.Errorf("%w: days after initiation must be a positive integer", apierr.GetAPIError(apierr.ErrInvalidArgument)) } if rule.Filter != nil && (rule.Filter.Tag != nil || (rule.Filter.And != nil && len(rule.Filter.And.Tags) > 0)) { - return fmt.Errorf("abort incomplete multipart upload cannot be specified with tags") + return fmt.Errorf("%w: abort incomplete multipart upload cannot be specified with tags", apierr.GetAPIError(apierr.ErrInvalidRequest)) } } if rule.Expiration != nil { if rule.Expiration.ExpiredObjectDeleteMarker != nil { if rule.Expiration.Days != nil || rule.Expiration.Date != "" { - return fmt.Errorf("expired object delete marker cannot be specified with days or date") + return fmt.Errorf("%w: expired object delete marker cannot be specified with days or date", apierr.GetAPIError(apierr.ErrMalformedXML)) } if rule.Filter != nil && (rule.Filter.Tag != nil || (rule.Filter.And != nil && len(rule.Filter.And.Tags) > 0)) { - return fmt.Errorf("expired object delete marker cannot be specified with tags") + return fmt.Errorf("%w: expired object delete marker cannot be specified with tags", apierr.GetAPIError(apierr.ErrInvalidRequest)) } } if rule.Expiration.Days != nil && *rule.Expiration.Days <= 0 { - return fmt.Errorf("expiration days must be a positive integer: %d", *rule.Expiration.Days) + return fmt.Errorf("%w: expiration days must be a positive integer", apierr.GetAPIError(apierr.ErrInvalidArgument)) } if rule.Expiration.Date != "" { parsedTime, err := time.Parse("2006-01-02T15:04:05Z", rule.Expiration.Date) if err != nil { - return fmt.Errorf("invalid value of expiration date: %s", rule.Expiration.Date) + return fmt.Errorf("%w: invalid value of expiration date: %s", apierr.GetAPIError(apierr.ErrInvalidArgument), rule.Expiration.Date) } epoch, err := util.TimeToEpoch(ni, now, parsedTime) @@ -204,14 +204,19 @@ func checkLifecycleConfiguration(ctx context.Context, cfg *data.LifecycleConfigu } if rule.NonCurrentVersionExpiration != nil { + if rule.NonCurrentVersionExpiration.NewerNonCurrentVersions != nil && rule.NonCurrentVersionExpiration.NonCurrentDays == nil { + return fmt.Errorf("%w: newer noncurrent versions cannot be specified without noncurrent days", apierr.GetAPIError(apierr.ErrMalformedXML)) + } + if rule.NonCurrentVersionExpiration.NewerNonCurrentVersions != nil && (*rule.NonCurrentVersionExpiration.NewerNonCurrentVersions > maxNewerNoncurrentVersions || *rule.NonCurrentVersionExpiration.NewerNonCurrentVersions <= 0) { - return fmt.Errorf("invalid value of newer noncurrent versions: %d", *rule.NonCurrentVersionExpiration.NewerNonCurrentVersions) + return fmt.Errorf("%w: newer noncurrent versions must be a positive integer up to %d", apierr.GetAPIError(apierr.ErrInvalidArgument), + maxNewerNoncurrentVersions) } if rule.NonCurrentVersionExpiration.NonCurrentDays != nil && *rule.NonCurrentVersionExpiration.NonCurrentDays <= 0 { - return fmt.Errorf("invalid value of noncurrent days: %d", *rule.NonCurrentVersionExpiration.NonCurrentDays) + return fmt.Errorf("%w: noncurrent days must be a positive integer", apierr.GetAPIError(apierr.ErrInvalidArgument)) } } @@ -243,9 +248,14 @@ func checkLifecycleRuleFilter(filter *data.LifecycleRuleFilter) error { } } - if filter.And.ObjectSizeGreaterThan != nil && filter.And.ObjectSizeLessThan != nil && - *filter.And.ObjectSizeLessThan <= *filter.And.ObjectSizeGreaterThan { - return fmt.Errorf("the maximum object size must be larger than the minimum object size") + if filter.And.ObjectSizeLessThan != nil { + if *filter.And.ObjectSizeLessThan == 0 { + return fmt.Errorf("%w: the maximum object size must be more than 0", apierr.GetAPIError(apierr.ErrInvalidRequest)) + } + + if filter.And.ObjectSizeGreaterThan != nil && *filter.And.ObjectSizeLessThan <= *filter.And.ObjectSizeGreaterThan { + return fmt.Errorf("%w: the maximum object size must be larger than the minimum object size", apierr.GetAPIError(apierr.ErrInvalidRequest)) + } } } @@ -254,6 +264,9 @@ func checkLifecycleRuleFilter(filter *data.LifecycleRuleFilter) error { } if filter.ObjectSizeLessThan != nil { + if *filter.ObjectSizeLessThan == 0 { + return fmt.Errorf("%w: the maximum object size must be more than 0", apierr.GetAPIError(apierr.ErrInvalidRequest)) + } fields++ } @@ -270,7 +283,7 @@ func checkLifecycleRuleFilter(filter *data.LifecycleRuleFilter) error { } if fields > 1 { - return fmt.Errorf("filter cannot have more than one field") + return fmt.Errorf("%w: filter cannot have more than one field", apierr.GetAPIError(apierr.ErrMalformedXML)) } return nil diff --git a/api/handler/lifecycle_test.go b/api/handler/lifecycle_test.go index 0794d55..5fb8bc8 100644 --- a/api/handler/lifecycle_test.go +++ b/api/handler/lifecycle_test.go @@ -27,19 +27,16 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { createBucket(hc, bktName) for _, tc := range []struct { - name string - body *data.LifecycleConfiguration - error bool + name string + body *data.LifecycleConfiguration + errorCode apierr.ErrorCode }{ { name: "correct configuration", body: &data.LifecycleConfiguration{ - XMLName: xml.Name{ - Space: `http://s3.amazonaws.com/doc/2006-03-01/`, - Local: "LifecycleConfiguration", - }, Rules: []data.LifecycleRule{ { + ID: "rule-1", Status: data.LifecycleStatusEnabled, Expiration: &data.LifecycleExpiration{ Days: ptr(21), @@ -54,6 +51,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, { + ID: "rule-2", Status: data.LifecycleStatusEnabled, AbortIncompleteMultipartUpload: &data.AbortIncompleteMultipartUpload{ DaysAfterInitiation: ptr(14), @@ -83,7 +81,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { } return lifecycle }(), - error: true, + errorCode: apierr.ErrInvalidRequest, }, { name: "duplicate rule ID", @@ -105,7 +103,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "too long rule ID", @@ -121,7 +119,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, } }(), - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "invalid status", @@ -132,7 +130,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrMalformedXML, }, { name: "no actions", @@ -146,7 +144,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidRequest, }, { name: "invalid days after initiation", @@ -160,7 +158,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "invalid expired object delete marker declaration", @@ -175,7 +173,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrMalformedXML, }, { name: "invalid expiration days", @@ -189,7 +187,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "invalid expiration date", @@ -203,7 +201,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "newer noncurrent versions is too small", @@ -212,12 +210,13 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { { Status: data.LifecycleStatusEnabled, NonCurrentVersionExpiration: &data.NonCurrentVersionExpiration{ + NonCurrentDays: ptr(1), NewerNonCurrentVersions: ptr(0), }, }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "newer noncurrent versions is too large", @@ -226,12 +225,13 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { { Status: data.LifecycleStatusEnabled, NonCurrentVersionExpiration: &data.NonCurrentVersionExpiration{ + NonCurrentDays: ptr(1), NewerNonCurrentVersions: ptr(101), }, }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "invalid noncurrent days", @@ -245,7 +245,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidArgument, }, { name: "more than one filter field", @@ -263,7 +263,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrMalformedXML, }, { name: "invalid tag in filter", @@ -280,7 +280,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidTagKey, }, { name: "abort incomplete multipart upload with tag", @@ -297,7 +297,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidRequest, }, { name: "expired object delete marker with tag", @@ -316,7 +316,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidRequest, }, { name: "invalid size range", @@ -336,19 +336,88 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + errorCode: apierr.ErrInvalidRequest, + }, + { + name: "two prefixes", + body: &data.LifecycleConfiguration{ + Rules: []data.LifecycleRule{ + { + Status: data.LifecycleStatusEnabled, + Expiration: &data.LifecycleExpiration{ + Days: ptr(21), + }, + Filter: &data.LifecycleRuleFilter{ + Prefix: "prefix-1/", + }, + Prefix: "prefix-2/", + }, + }, + }, + errorCode: apierr.ErrMalformedXML, + }, + { + name: "newer noncurrent versions without noncurrent days", + body: &data.LifecycleConfiguration{ + Rules: []data.LifecycleRule{ + { + Status: data.LifecycleStatusEnabled, + NonCurrentVersionExpiration: &data.NonCurrentVersionExpiration{ + NewerNonCurrentVersions: ptr(10), + }, + }, + }, + }, + errorCode: apierr.ErrMalformedXML, + }, + { + name: "invalid maximum object size in filter", + body: &data.LifecycleConfiguration{ + Rules: []data.LifecycleRule{ + { + Status: data.LifecycleStatusEnabled, + Expiration: &data.LifecycleExpiration{ + Days: ptr(21), + }, + Filter: &data.LifecycleRuleFilter{ + ObjectSizeLessThan: ptr(uint64(0)), + }, + }, + }, + }, + errorCode: apierr.ErrInvalidRequest, + }, + { + name: "invalid maximum object size in filter and", + body: &data.LifecycleConfiguration{ + Rules: []data.LifecycleRule{ + { + Status: data.LifecycleStatusEnabled, + Expiration: &data.LifecycleExpiration{ + Days: ptr(21), + }, + Filter: &data.LifecycleRuleFilter{ + And: &data.LifecycleRuleAndOperator{ + Prefix: "prefix/", + ObjectSizeLessThan: ptr(uint64(0)), + }, + }, + }, + }, + }, + errorCode: apierr.ErrInvalidRequest, }, } { t.Run(tc.name, func(t *testing.T) { - if tc.error { - putBucketLifecycleConfigurationErr(hc, bktName, tc.body, apierr.GetAPIError(apierr.ErrMalformedXML)) + if tc.errorCode > 0 { + putBucketLifecycleConfigurationErr(hc, bktName, tc.body, apierr.GetAPIError(tc.errorCode)) return } putBucketLifecycleConfiguration(hc, bktName, tc.body) cfg := getBucketLifecycleConfiguration(hc, bktName) - require.Equal(t, *tc.body, *cfg) + require.Equal(t, tc.body.Rules, cfg.Rules) deleteBucketLifecycleConfiguration(hc, bktName) getBucketLifecycleConfigurationErr(hc, bktName, apierr.GetAPIError(apierr.ErrNoSuchLifecycleConfiguration)) -- 2.45.2 From af7fa5f62ec4a948d3e9e124cc5f819dc795c479 Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Fri, 8 Nov 2024 11:47:10 +0300 Subject: [PATCH 3/3] [#536] Add rule ID generation Signed-off-by: Marina Biryukova --- api/handler/lifecycle.go | 12 +++++++++++- api/handler/lifecycle_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/api/handler/lifecycle.go b/api/handler/lifecycle.go index e559365..3aa3ccc 100644 --- a/api/handler/lifecycle.go +++ b/api/handler/lifecycle.go @@ -17,6 +17,7 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/util" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/netmap" + "github.com/google/uuid" ) const ( @@ -145,7 +146,16 @@ func checkLifecycleConfiguration(ctx context.Context, cfg *data.LifecycleConfigu ids := make(map[string]struct{}, len(cfg.Rules)) for i, rule := range cfg.Rules { - if _, ok := ids[rule.ID]; ok && rule.ID != "" { + if rule.ID == "" { + id, err := uuid.NewRandom() + if err != nil { + return fmt.Errorf("generate uuid: %w", err) + } + cfg.Rules[i].ID = id.String() + rule.ID = id.String() + } + + if _, ok := ids[rule.ID]; ok { return fmt.Errorf("%w: duplicate 'ID': %s", apierr.GetAPIError(apierr.ErrInvalidArgument), rule.ID) } ids[rule.ID] = struct{}{} diff --git a/api/handler/lifecycle_test.go b/api/handler/lifecycle_test.go index 5fb8bc8..69382ab 100644 --- a/api/handler/lifecycle_test.go +++ b/api/handler/lifecycle_test.go @@ -425,6 +425,36 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { } } +func TestPutBucketLifecycleIDGeneration(t *testing.T) { + hc := prepareHandlerContext(t) + + bktName := "bucket-lifecycle-id" + createBucket(hc, bktName) + + lifecycle := &data.LifecycleConfiguration{ + Rules: []data.LifecycleRule{ + { + Status: data.LifecycleStatusEnabled, + Expiration: &data.LifecycleExpiration{ + Days: ptr(21), + }, + }, + { + Status: data.LifecycleStatusEnabled, + AbortIncompleteMultipartUpload: &data.AbortIncompleteMultipartUpload{ + DaysAfterInitiation: ptr(14), + }, + }, + }, + } + + putBucketLifecycleConfiguration(hc, bktName, lifecycle) + cfg := getBucketLifecycleConfiguration(hc, bktName) + require.Len(t, cfg.Rules, 2) + require.NotEmpty(t, cfg.Rules[0].ID) + require.NotEmpty(t, cfg.Rules[1].ID) +} + func TestPutBucketLifecycleInvalidMD5(t *testing.T) { hc := prepareHandlerContext(t) -- 2.45.2