chore(mixins-preview): update Vended Logs to use Facades and Traits and add actions methods to BucketGrants#37239
Conversation
|
|
||||||||||||||
|
|
||||||||||||||
| @@ -141,6 +141,19 @@ export class BucketGrants { | |||
| return this.grant(identity, perms.BUCKET_PUT_ACL_ACTIONS, [], this.arnForObjects(objectsKeyPattern)); | |||
There was a problem hiding this comment.
Now you can update the other methods to use the new actions().
| * @param actions The actions to grant (e.g. 's3:GetObject'). | ||
| */ | ||
| public actions(identity: IGrantable, objectsKeyPattern: string = '*', ...actions: string[]) { | ||
| return this.grant(identity, actions, [], |
There was a problem hiding this comment.
Maybe the key actions should also become a parameter, instead of being hard-coded as an empty array.
There was a problem hiding this comment.
I was thinking about having actions be able to take both Key and Bucket actions (and we sort them out inside the actions method) since you can't have 2 variadic parameters
actions methods to BucketGrants
| "Arn" | ||
| ] | ||
| }, | ||
| ":log-stream:*\"}],\"Version\":\"2012-10-17\"}" |
There was a problem hiding this comment.
Is this change correct? It's hard to tell from this diff, but it seems that we are removing the log streams permission and just granting permission to the log group.
There was a problem hiding this comment.
yeah, I think this needs alteration...need to look at the docs again and see if I can set this up with bucketGrants
There was a problem hiding this comment.
looked through my test stacks and took a second look over the documentation, granting log stream permissions was incorrect to begin with, these permissions are the correct ones
| * @param actions The S3 and/or KMS actions to grant. | ||
| */ | ||
| public actionsOnObjectKeys(identity: IGrantable, objectsKeyPattern: string = '*', ...actions: string[]) { | ||
| const keyActions: string[] = []; |
There was a problem hiding this comment.
(minor) we can remove the duplication between the two new methods by delegating to a new private one.
| // when | ||
| bucket.grantDelete(deleter); | ||
|
|
||
| console.log(JSON.stringify(Template.fromStack(stack), null, 2)); |
There was a problem hiding this comment.
Left behind unintentionally?
There was a problem hiding this comment.
not intentional, was using this for debugging
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 30 minutes 9 seconds in the queue, including 29 minutes 54 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
Vended Logs was previously not using the Traits and Facades that have been introduced with the GA of Mixins.
BucketGrantsis a handwritten Grants class and did not benefit from the update a lot of the other Grants classes received which gave them access to the .actions()method.Description of changes
Note: most of these changes involve refreshing the e2e test files mostly because policy names have changed and some implementation details are a bit different because of grants, the permissions themselves have not changed
This changes the implementation but not the behavior of Vended Logs to use the Facades and Traits.
BucketGrantsnow has 2 methods which operate like the.actions()method in other Grants classes.BucketGrantsgets 2 methods while most other classes only get one becauseBucketGrantshas a somewhat unique scenario where it always receives some kind of object arn to be able to apply permissions to accessing objects in a bucket, but does not always receive a bucket arn which controls what can be done to the bucket itself. We split these 2 use cases into 2 different grant methods.Describe any new or updated permissions being added
Corrects Vended Logs S3 Bucket permissions to not grant to log stream, this is inline with Vended Logs documentation: https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/AWS-logs-infrastructure-V2-S3.html
Description of how you validated changes
Updated unit and integration tests in vended logs and added new unit tests to for
BucketGrantsChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license