From 4a19a8911ff7dfe8160d5ca209fb3519566c41c3 Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Fri, 8 Nov 2024 11:45:10 +0300 Subject: [PATCH] [#536] Fix error codes in lifecycle configuration check Signed-off-by: Marina Biryukova --- api/handler/lifecycle.go | 49 +++++++++----- api/handler/lifecycle_test.go | 119 +++++++++++++++++++++++++++------- 2 files changed, 125 insertions(+), 43 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..e667170 100644 --- a/api/handler/lifecycle_test.go +++ b/api/handler/lifecycle_test.go @@ -29,17 +29,14 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { for _, tc := range []struct { name string body *data.LifecycleConfiguration - error bool + error 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, + error: apierr.ErrInvalidRequest, }, { name: "duplicate rule ID", @@ -105,7 +103,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidArgument, }, { name: "too long rule ID", @@ -121,7 +119,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, } }(), - error: true, + error: apierr.ErrInvalidArgument, }, { name: "invalid status", @@ -132,7 +130,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrMalformedXML, }, { name: "no actions", @@ -146,7 +144,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidRequest, }, { name: "invalid days after initiation", @@ -160,7 +158,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidArgument, }, { name: "invalid expired object delete marker declaration", @@ -175,7 +173,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrMalformedXML, }, { name: "invalid expiration days", @@ -189,7 +187,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidArgument, }, { name: "invalid expiration date", @@ -203,7 +201,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: 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, + error: 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, + error: apierr.ErrInvalidArgument, }, { name: "invalid noncurrent days", @@ -245,7 +245,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidArgument, }, { name: "more than one filter field", @@ -263,7 +263,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrMalformedXML, }, { name: "invalid tag in filter", @@ -280,7 +280,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidTagKey, }, { name: "abort incomplete multipart upload with tag", @@ -297,7 +297,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidRequest, }, { name: "expired object delete marker with tag", @@ -316,7 +316,7 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: apierr.ErrInvalidRequest, }, { name: "invalid size range", @@ -336,19 +336,88 @@ func TestPutBucketLifecycleConfiguration(t *testing.T) { }, }, }, - error: true, + error: 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/", + }, + }, + }, + error: apierr.ErrMalformedXML, + }, + { + name: "newer noncurrent versions without noncurrent days", + body: &data.LifecycleConfiguration{ + Rules: []data.LifecycleRule{ + { + Status: data.LifecycleStatusEnabled, + NonCurrentVersionExpiration: &data.NonCurrentVersionExpiration{ + NewerNonCurrentVersions: ptr(10), + }, + }, + }, + }, + error: 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)), + }, + }, + }, + }, + error: 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)), + }, + }, + }, + }, + }, + error: apierr.ErrInvalidRequest, }, } { t.Run(tc.name, func(t *testing.T) { - if tc.error { - putBucketLifecycleConfigurationErr(hc, bktName, tc.body, apierr.GetAPIError(apierr.ErrMalformedXML)) + if tc.error > 0 { + putBucketLifecycleConfigurationErr(hc, bktName, tc.body, apierr.GetAPIError(tc.error)) 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))