cumulus
cumulus copied to clipboard
CUMULUS-3051: Propagate ACL value in S3ProviderClient for multipartCopyObject call
Summary: Summary of changes
Addresses CUMULUS-3051: Propagate ACL value in S3ProviderClient for multipartCopyObject call
Changes
- Update S3ProviderClient.sync() to take ACL parameter
PR Checklist
- [x] Update CHANGELOG
- [x] Unit tests
- [x] Ad-hoc testing - Deploy changes and test manually
- [ ] Integration tests
Ad-hoc testing - Deploy changes and test manually
It was necessary to create an S3 bucket in a different account since ACL errors don't show when you write objects to from one account to an S3 bucket owned by that same AWS account no matter the configuration of the bucket object ownership (ACLs disabled or enabled).
I tested by creating a bucket in our SIT account and then redeployed my dev Cumulus stack to reference the new SIT bucket. Updated the SIT bucket to have ACLs disabled (necessary to do after deploying rather than before because the deployment will fail since we use aws_s3_bucket_object
which defaults the ACL to private
). Then I ran SyncGranuleSuccessSpec.js to ensure that the error AccessControlListNotSupported
appears when the SyncGranule workflow isn't configured to disable ACLs in order to verify that the multipartCopyObject call fails since it originally defaults the ACL to private
.
Then I implemented my parameter changes and redeployed, and ran the tests using my SIT bucket as the system bucket, and verified that the SyncGranuleSuccessSpec test succeeds.
Suggestion from @kkelly51 - Update docs to list out the canned ACLs that AWS accepts (export declare type ObjectCannedACL = "authenticated-read" | "aws-exec-read" | "bucket-owner-full-control" | "bucket-owner-read" | "private" | "public-read" | "public-read-write";)
@botanical just going through old PRs. Does this need a fresh set of eyes? It looks like there were comments that may need addressing?
@botanical just going through old PRs. Does this need a fresh set of eyes? It looks like there were comments that may need addressing?
@npauzenga Hey Nate! Thanks for following up. I think this does need a fresh set of eyes. I believe I did address all the comments as well. The main thing holding up this review, I believe, is that testing it manually isn't trivial. It would be great to get this reviewed though so we can fix the sync-granule ACL configuration bug.
Should we be applying an ACL in this case as well?
https://github.com/nasa/cumulus/blob/CUMULUS-3051/packages/ingest/src/S3ProviderClient.ts#L111
Closing this since it is no longer valid