Skip to content

Conversation

@aayushchouhan09
Copy link
Member

@aayushchouhan09 aayushchouhan09 commented Apr 9, 2025

Describe the Problem

S3-compatible clients expect the x-amz-expiration response header when an object is subject to a lifecycle expiration rule. However, NooBaa was not setting this header in GET, PUT, or HEAD responses, even when valid expiration rules were configured.

Explain the Changes

  1. Added set_expiration_header() function to include the x-amz-expiration header in GET, PUT, and HEAD object responses.
  2. Implemented parse_expiration_header() to convert lifecycle expiration rules (days or date) into proper s3 style header format.

Below is the output for header of putObject, headObject and getObject (after the fix):

Screenshot from 2025-04-30 18-30-45

Issues: Fixed #xxx / Gap #xxx

  1. Partially Fixed: NSFS | S3 | Lifecycle: bunch of expiration header tests failed #8554
  2. Ceph-s3 tests are failing because in ceph tests they are calculating number of days without converting start_time to midnigt UTC (inside check_expiration_header() function) which is causing error and returning wrong no. of expiration days.
    ceph-s3 tests issue has been created for the same.

Testing Instructions:

  1. Ceph-s3 tests are failing.
  2. Added unit tests.
  • Tests added

@aayushchouhan09 aayushchouhan09 marked this pull request as draft April 9, 2025 13:25
@aayushchouhan09 aayushchouhan09 force-pushed the amz-exp-fix branch 2 times, most recently from 4ffea5c to d5690b2 Compare April 22, 2025 13:30
@aayushchouhan09 aayushchouhan09 marked this pull request as ready for review April 22, 2025 13:31
@aayushchouhan09 aayushchouhan09 force-pushed the amz-exp-fix branch 4 times, most recently from 1e0964c to b0aadfc Compare April 28, 2025 17:35
@aayushchouhan09
Copy link
Member Author

PR is ready for review. For testing we can add ceph-s3 tests which are failing for missing x-amz-expiration header.

Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

Comments and questions

@shirady
Copy link
Contributor

shirady commented Apr 29, 2025

Hi @achouhan09,

  • I don't have prior knowledge about this header - if you can share what you tested in AWS it can be useful (I would also attach it in the PR description). I read this part in the blog to understand it quickly (link):

If you make a GET or a HEAD request on an object that has been scheduled for expiration, the response will include an x-amz-expiration header that includes this expiration date and the corresponding rule Id.

  • I saw in the comment above you mentioned the failing Ceph tests - can you add them in this PR?
    You can remove them from the blacklist/pending list so they will run automatically in the CI.

  • In AWS docs GetObject I found:

If the object expiration is configured (see PutBucketLifecycleConfiguration), the response includes this header. It includes the expiry-date and rule-id key-value pairs providing object expiration information. The value of the rule-id is URL-encoded.

Note - same comment in AWS docs HeadObject and also in AWS docs PutObject

Did you test in AWS the url-encoded part so we can understand what they do there?

I also saw this comment:

Note
Object expiration information is not returned in directory buckets and this header returns the value "NotImplemented" in all responses for directory buckets.

@romayalon @nadavMiz just want you to notice, I think it is fine to implement it in NSFS, but I would like you to think about it.

@aayushchouhan09
Copy link
Member Author

Hi @achouhan09,

  • I don't have prior knowledge about this header - if you can share what you tested in AWS it can be useful (I would also attach it in the PR description). I read this part in the blog to understand it quickly (link):

If you make a GET or a HEAD request on an object that has been scheduled for expiration, the response will include an x-amz-expiration header that includes this expiration date and the corresponding rule Id.

  • I saw in the comment above you mentioned the failing Ceph tests - can you add them in this PR?
    You can remove them from the blacklist/pending list so they will run automatically in the CI.
  • In AWS docs GetObject I found:

If the object expiration is configured (see PutBucketLifecycleConfiguration), the response includes this header. It includes the expiry-date and rule-id key-value pairs providing object expiration information. The value of the rule-id is URL-encoded.

Note - same comment in AWS docs HeadObject and also in AWS docs PutObject

Did you test in AWS the url-encoded part so we can understand what they do there?

I also saw this comment:

Note
Object expiration information is not returned in directory buckets and this header returns the value "NotImplemented" in all responses for directory buckets.

@romayalon @nadavMiz just want you to notice, I think it is fine to implement it in NSFS, but I would like you to think about it.

@shirady

  • Updated PR description.
  • Added ceph s3 tests - which are failing before the fix.
  • Here, as per the aws documentation the rule-id should be url-encoded in x-amz-expiration but aws itself is not doing so, please check below screenshot:
    Screenshot from 2025-04-30 16-23-09

cc @romayalon @nadavMiz

@aayushchouhan09 aayushchouhan09 requested a review from shirady April 30, 2025 13:30
@aayushchouhan09 aayushchouhan09 force-pushed the amz-exp-fix branch 4 times, most recently from 872cedd to 60dbae9 Compare May 8, 2025 07:42
@aayushchouhan09 aayushchouhan09 requested a review from shirady May 8, 2025 07:44
@nadavMiz
Copy link
Contributor

nadavMiz commented May 8, 2025

since the documentation is not clear on this flag. I think it would also be good to check against aws if it exist for versioned objects as well. if it does it doesn't have to be in this PR, but at least we will open a gap for it

@aayushchouhan09 aayushchouhan09 requested a review from nadavMiz May 12, 2025 18:08
@aayushchouhan09 aayushchouhan09 force-pushed the amz-exp-fix branch 4 times, most recently from f26854e to ba183be Compare May 19, 2025 15:02
@aayushchouhan09 aayushchouhan09 force-pushed the amz-exp-fix branch 2 times, most recently from 440434d to c0abc5e Compare May 20, 2025 06:58
… GET/PUT/HEAD

Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
@aayushchouhan09 aayushchouhan09 merged commit c28f4ca into noobaa:master May 20, 2025
11 checks passed
@aayushchouhan09 aayushchouhan09 deleted the amz-exp-fix branch May 20, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NSFS | S3 | Lifecycle: bunch of expiration header tests failed

3 participants