Added x-amz-expiration missing HTTP header in the response of object operations
#8958
Conversation
achouhan09
force-pushed
the
amz-exp-fix
branch
2 times, most recently
from
4ffea5c to
d5690b2
Compare
achouhan09
force-pushed
the
amz-exp-fix
branch
4 times, most recently
from
1e0964c to
b0aadfc
Compare
|
PR is ready for review. For testing we can add ceph-s3 tests which are failing for missing |
|
Hi @achouhan09,
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:
@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. |
|
… GET/PUT/HEAD Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
achouhan09
force-pushed
the
amz-exp-fix
branch
3 times, most recently
from
1c4c44b to
872cedd
Compare
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
| * @param {Object} object_md | ||
| * @returns {Object|undefined} | ||
| */ | ||
| function get_lifecycle_rule_for_object(rules, object_md) { |
There was a problem hiding this comment.
I don't think http_utils id a good place to put this function. there is a file lifecycle_utils. its currently mostly by nc lifecycle. but it really should be for lifecycle in general
There was a problem hiding this comment.
also there is also a function in nc called _build_lifecycle_filter that creates a function that validates the filter. there is probably something similar in containerized lifecycle, that you might be able to use. this will make it easier if we will ever want to change the filter validation to change it only in one spot.
| if (!matches_all_tags) continue; | ||
| } | ||
|
|
||
| const priority = { |
There was a problem hiding this comment.
what is this priority? why do you need it?
| */ | ||
| async function set_expiration_header(req, res, object_md) { | ||
| const rules = req.params.bucket && await req.object_sdk.get_bucket_lifecycle_configuration_rules({ name: req.params.bucket }); | ||
| if (!object_md) { // calculating object_md for putObject |
There was a problem hiding this comment.
from what I understand this only happens for put commands. its better that you construct this outside this function and pass the object_md as a parameter. also you should get as much information as you can from the response and not from the request
| * Example output: | ||
| * expiry-date="Thu, 10 Apr 2025 00:00:00 GMT", rule-id="rule_id" | ||
| */ | ||
| function parse_expiration_header(rule, create_time) { |
There was a problem hiding this comment.
I don't think its a good name. you don't parse the header, you write it
| @@ -363,9 +363,6 @@ s3tests_boto3/functional/test_s3.py::test_lifecycle_expiration_tags2 | |||
| s3tests_boto3/functional/test_s3.py::test_lifecycle_expiration_versioned_tags2 | |||
There was a problem hiding this comment.
I think it would be good if we will add our own tests as well. check that it works on both containerized and non-containerized environments
|
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 |
Describe the Problem
S3-compatible clients expect the
x-amz-expirationresponse 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
set_expiration_header()function to include thex-amz-expirationheader in GET, PUT, and HEAD object responses.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):
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Note: Need to fix for nsfs.