Added restriction to limit the ID length to 255 characters in bucket lifecycle rules #8628

achouhan09 · 2024-12-26T13:41:05Z

Explain the changes

Currently, any length of ID can be accepted, as a fix we have added restriction to limit the length of ID to 255 characters only in bucket lifecycle rules.

Issues: Fixed #xxx / Gap #xxx

Fixed: NSFS | S3 | Lifecycle: Bucket lifecycle configuration accepts long rule ID #8549

Testing Instructions:

Tests added

romayalon · 2024-12-30T15:39:25Z

Hey @achouhan09

Can you please link to the Issues section the bug you are fixing?
Please add tests
Throwing InvalidArgument error code is based on the AWS S3 protocol?

achouhan09 · 2025-01-03T07:32:46Z

Hi @romayalon
I think the InvalidArgument error code is suitable for this case as there is issue found in parameters passed. check this

Added issue link and tests. Thanks

src/endpoint/s3/ops/s3_put_bucket_lifecycle.js

src/test/lifecycle/common.js

shirady · 2025-01-06T13:24:07Z

src/endpoint/s3/ops/s3_put_bucket_lifecycle.js

@@ -7,6 +7,7 @@ const dbg = require('../../../util/debug_module')(__filename);
 const S3Error = require('../s3_errors').S3Error;

 const true_regex = /true/i;
+const max_length_rule_id = 255;


I prefer using capital letters (so it is easier to know when reading the statement that it is a const without jumping to the declaration).

Suggested change

const max_length_rule_id = 255;

const MAX_LENGTH_RULE_ID = 255;

Done, Thanks

shirady · 2025-01-06T13:25:18Z

src/test/lifecycle/common.js

@@ -3,6 +3,7 @@

 const assert = require('assert');

+const max_length_rule_id = 255;


Can you declare the max in a place where we can import it and reuse it?

We can use config.js here. Updated, Thanks

I'm not sure config.js is the right place, as it should not be configured.
I can suggest existing s3_utils (or if you want I think we can create s3_constants under src/endpoint/s3/).

src/test/lifecycle/common.js

shirady · 2025-01-06T13:30:04Z

Hi @romayalon I think the InvalidArgument error code is suitable for this case as there is issue found in parameters passed. check this

Could you test it in AWS and attach the results here?

achouhan09 · 2025-01-07T10:48:13Z

Hi @romayalon I think the InvalidArgument error code is suitable for this case as there is issue found in parameters passed. check this

Could you test it in AWS and attach the results here?

…lifecycle rules Signed-off-by: Aayush Chouhan <[email protected]>

Signed-off-by: Aayush Chouhan <[email protected]>

shirady · 2025-01-07T11:11:10Z

replied to comment
@achouhan09 does noobaa use the same error message?
I mean this: "ID length should not exceed allowed limit of 255"

achouhan09 · 2025-01-07T11:25:13Z

replied to comment @achouhan09 does noobaa use the same error message? I mean this: "ID length should not exceed allowed limit of 255"

For NooBaa, we will only get InvalidArgument error and no error msg on the command line. But we can check the error logs in the endpoint pod which is similar to what you are referring. Below is the NooBaa error:

An error occurred (InvalidArgument) when calling the PutBucketLifecycleConfiguration operation: Invalid Argument

Signed-off-by: Aayush Chouhan <[email protected]>

achouhan09 · 2025-01-07T13:24:10Z

replied to comment @achouhan09 does noobaa use the same error message? I mean this: "ID length should not exceed allowed limit of 255"

@shirady updated the error msg. Thanks

shirady · 2025-01-07T14:05:37Z

src/endpoint/s3/ops/s3_put_bucket_lifecycle.js

-            current_rule.id = rule.ID[0];
+            if (rule.ID[0].length > config.MAX_RULE_ID_LENGTH) {
+                dbg.error('Rule should not have ID length exceed allowed limit of ', config.MAX_RULE_ID_LENGTH, ' characters', rule);
+                throw new S3Error({ ...S3Error.InvalidArgument, message: 'ID length should not exceed allowed limit of 255' });


Suggested change

throw new S3Error({ ...S3Error.InvalidArgument, message: 'ID length should not exceed allowed limit of 255' });

throw new S3Error({ ...S3Error.InvalidArgument, message: `ID length should not exceed allowed limit of ${config.MAX_RULE_ID_LENGTH}` });

achouhan09 requested review from liranmauda, shirady and naveenpaul1 December 26, 2024 13:41

pull-request-size bot added the size/XS label Dec 26, 2024

achouhan09 force-pushed the id-fix branch from 6d37283 to ebab567 Compare December 26, 2024 16:41

achouhan09 requested review from a team and aspandey and removed request for a team December 26, 2024 16:43

achouhan09 changed the title ~~NSFS| Added restriction to limit the ID length to 255 characters in bucket lifecycle rules~~ Added restriction to limit the ID length to 255 characters in bucket lifecycle rules Dec 30, 2024

achouhan09 requested a review from romayalon December 30, 2024 12:05

achouhan09 force-pushed the id-fix branch from ebab567 to 7cb08df Compare January 3, 2025 07:23

pull-request-size bot added size/M and removed size/XS labels Jan 3, 2025

achouhan09 force-pushed the id-fix branch from 7cb08df to 0e4c052 Compare January 3, 2025 10:19

shirady reviewed Jan 5, 2025

View reviewed changes

src/endpoint/s3/ops/s3_put_bucket_lifecycle.js Outdated Show resolved Hide resolved

src/test/lifecycle/common.js Outdated Show resolved Hide resolved

pull-request-size bot added size/S and removed size/M labels Jan 6, 2025

achouhan09 force-pushed the id-fix branch from 018affa to 94b1d5a Compare January 6, 2025 13:06

achouhan09 requested a review from shirady January 6, 2025 13:07

shirady reviewed Jan 6, 2025

View reviewed changes

achouhan09 added 4 commits January 7, 2025 16:38

Added restriction to limit the ID length to 255 characters in bucket …

81f5806

…lifecycle rules Signed-off-by: Aayush Chouhan <[email protected]>

Added test for id length exceeding 255 chars

1ece21f

Signed-off-by: Aayush Chouhan <[email protected]>

Updated to use var for character length and removed promise chain

0c464d5

Signed-off-by: Aayush Chouhan <[email protected]>

used config.js for declaration of max length and updated the error msg

dd19d7b

Signed-off-by: Aayush Chouhan <[email protected]>

achouhan09 force-pushed the id-fix branch from 94b1d5a to dd19d7b Compare January 7, 2025 11:08

achouhan09 requested a review from shirady January 7, 2025 11:09

Updated the s3 error msg in InvalidArgument error

9a7934b

Signed-off-by: Aayush Chouhan <[email protected]>

shirady reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added restriction to limit the ID length to 255 characters in bucket lifecycle rules #8628

Added restriction to limit the ID length to 255 characters in bucket lifecycle rules #8628

achouhan09 commented Dec 26, 2024 •

edited

Loading

romayalon commented Dec 30, 2024

achouhan09 commented Jan 3, 2025

shirady Jan 6, 2025

achouhan09 Jan 7, 2025

shirady Jan 6, 2025

achouhan09 Jan 7, 2025

shirady Jan 7, 2025

shirady commented Jan 6, 2025

achouhan09 commented Jan 7, 2025

shirady commented Jan 7, 2025

achouhan09 commented Jan 7, 2025 •

edited

Loading

achouhan09 commented Jan 7, 2025

shirady Jan 7, 2025

	const max_length_rule_id = 255;
	const MAX_LENGTH_RULE_ID = 255;

		@@ -3,6 +3,7 @@

		const assert = require('assert');

		const max_length_rule_id = 255;

	throw new S3Error({ ...S3Error.InvalidArgument, message: 'ID length should not exceed allowed limit of 255' });
	throw new S3Error({ ...S3Error.InvalidArgument, message: `ID length should not exceed allowed limit of ${config.MAX_RULE_ID_LENGTH}` });

Added restriction to limit the ID length to 255 characters in bucket lifecycle rules #8628

Are you sure you want to change the base?

Added restriction to limit the ID length to 255 characters in bucket lifecycle rules #8628

Conversation

achouhan09 commented Dec 26, 2024 • edited Loading

Explain the changes

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

romayalon commented Dec 30, 2024

achouhan09 commented Jan 3, 2025

shirady Jan 6, 2025

Choose a reason for hiding this comment

achouhan09 Jan 7, 2025

Choose a reason for hiding this comment

shirady Jan 6, 2025

Choose a reason for hiding this comment

achouhan09 Jan 7, 2025

Choose a reason for hiding this comment

shirady Jan 7, 2025

Choose a reason for hiding this comment

shirady commented Jan 6, 2025

achouhan09 commented Jan 7, 2025

shirady commented Jan 7, 2025

achouhan09 commented Jan 7, 2025 • edited Loading

achouhan09 commented Jan 7, 2025

shirady Jan 7, 2025

Choose a reason for hiding this comment

achouhan09 commented Dec 26, 2024 •

edited

Loading

achouhan09 commented Jan 7, 2025 •

edited

Loading