cumulus icon indicating copy to clipboard operation
cumulus copied to clipboard

CUMULUS-3051: Propagate ACL value in S3ProviderClient for multipartCopyObject call

Open botanical opened this issue 2 years ago • 1 comments

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.

botanical avatar Sep 21 '22 17:09 botanical

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 avatar Sep 30 '22 16:09 botanical

@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 avatar Jan 24 '23 15:01 npauzenga

@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.

botanical avatar Jan 25 '23 00:01 botanical

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

marchuffnagle avatar Feb 06 '23 21:02 marchuffnagle

Closing this since it is no longer valid

botanical avatar May 11 '23 22:05 botanical