Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📊Tracking: Stabilize aws-scheduler-alpha #31785

Open
samson-keung opened this issue Oct 16, 2024 · 1 comment
Open

📊Tracking: Stabilize aws-scheduler-alpha #31785

samson-keung opened this issue Oct 16, 2024 · 1 comment
Assignees
Labels
@aws-cdk/aws-cloudformation Related to AWS CloudFormation management/tracking Issues that track a subject or multiple issues p2

Comments

@samson-keung
Copy link
Contributor

samson-keung commented Oct 16, 2024

Overview

Tracking issue for stabilizing aws-scheduler-alpha module.

Link to the service’s CDK Construct Library API reference page.

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-scheduler-alpha-readme.html

Maturity: CloudFormation Resources Only

Developer Preview

Implementation

Constructs to stablize:

  • Schedule
  • Group
  • scheduler-targets module

Issue list

Related feature request

@samson-keung samson-keung added management/tracking Issues that track a subject or multiple issues needs-triage This issue or PR still needs to be triaged. labels Oct 16, 2024
@github-actions github-actions bot added the @aws-cdk/aws-cloudformation Related to AWS CloudFormation label Oct 16, 2024
@khushail khushail added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 16, 2024
@khushail
Copy link
Contributor

created by core team to track the related issues.

mergify bot pushed a commit that referenced this issue Oct 28, 2024
### Issue # (if applicable)

Tracking #31785.

### Reason for this change

When customer use a KMS Customer Managed Key (CMK) with the `Schedule` construct, the following permissions are added to the scheduler execution role:
```
'kms:Decrypt', 'kms:Encrypt', 'kms:ReEncrypt*', 'kms:GenerateDataKey*'
```

However, upon testing, having only the `kms:Decrypt` permission is enough for the Schedule to invoke the target (Lambda Function as a target was used in the test.).

### Description of changes

This PR removes the unneeded KMS permissions and updated integ test to verify that the schedule is still able to invoke the target. 

### Description of how you validated changes

Unit test and integ test.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

----

BREAKING CHANGE: Extra KMS permissions are removed from Schedule execution role when KMS key is passed to Schedule.
@samson-keung samson-keung self-assigned this Oct 29, 2024
mergify bot pushed a commit that referenced this issue Oct 30, 2024
### Issue # (if applicable)

Tracking #31785.

### Reason for this change

Updating doc for better clarity.

### Description of changes

Doc updates

### Description of how you validated changes

N/A

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 1, 2024
…gleton schedule target role (#31895)

### Issue # (if applicable)

Tracking #31785.

### Reason for this change

The current logic for creating a schedule target execution role uses a hash on the `targetArn` to determine if there is an existing role in the stack. Currently if the `targetArn` contains token values (e.g. intrinsic functions), `stack.resolve(targetArn).toString()` is used to convert the tokenized ARN into a string. However this always results in `[object Object]` which then gets hashed, meaning the same role is used for any target where the ARN passed in is not a pure string. This does not follow principle of least privilege, and a singleton role used across multiple different targets/target types can be confusing for the customer to manage. 

### Description of changes

- Use `JSON.stringify()` instead of `.toString()` to produce unique hash (and thus create new role) per target.

### Description of how you validated changes

Updated unit tests and integration tests

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

BREAKING CHANGE: Schedule Target will reuse role if target is re-used across schedules. This change triggered replacement of existing roles for Schedule as logical ID of the roles are changed.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 6, 2024
### Issue # (if applicable)

tracking issue: #31785 

### Reason for this change

Satisfy 90% unit test coverage required for developer preview (see https://github.com/cdklabs/team-internal/blob/main/docs/construct-library-lifecycle.md#exit-criteria-1)

### Description of changes

Added parameterized unit tests for metric methods

Previous coverage summary:
```
=============================== Coverage summary ===============================
Statements   : 83.06% ( 103/124 )
Branches     : 76.19% ( 32/42 )
Functions    : 70.9% ( 39/55 )
Lines        : 83.47% ( 101/121 )
================================================================================
```
Current coverage summary:

```
=============================== Coverage summary ===============================
Statements   : 95.16% ( 118/124 )
Branches     : 83.33% ( 35/42 )
Functions    : 96.36% ( 53/55 )
Lines        : 95.86% ( 116/121 )
================================================================================
```

### Description of how you validated changes

Unit tests pass

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 7, 2024
…d of queue policy (#32032)

### Issue # (if applicable)

Tracking #31785.

### Reason for this change

Currently if a dead letter queue (DLQ) is specified then a queue policy is created for the DLQ which allows the schedule to send messages. This is incorrect and the permissions should be added to the schedule's execution role instead.  

### Description of changes

Add `sqs:SendMessage` permission to execution role's policy statement if dead letter queue is specified. This follows the [service docs](https://docs.aws.amazon.com/scheduler/latest/UserGuide/configuring-schedule-dlq.html#configuring-schedule-dlq-permissions) for configuring a schedule DLQ.

Also removed cross-region validation as the deployment will fail fast for this case so the validation is unnecessary.

### Description of how you validated changes

Updated unit tests and added a new integration test with dead letter queue setup on the schedule

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Leo10Gama pushed a commit to Leo10Gama/aws-cdk that referenced this issue Nov 13, 2024
### Issue # (if applicable)

tracking issue: aws#31785 

### Reason for this change

Satisfy 90% unit test coverage required for developer preview (see https://github.com/cdklabs/team-internal/blob/main/docs/construct-library-lifecycle.md#exit-criteria-1)

### Description of changes

Added parameterized unit tests for metric methods

Previous coverage summary:
```
=============================== Coverage summary ===============================
Statements   : 83.06% ( 103/124 )
Branches     : 76.19% ( 32/42 )
Functions    : 70.9% ( 39/55 )
Lines        : 83.47% ( 101/121 )
================================================================================
```
Current coverage summary:

```
=============================== Coverage summary ===============================
Statements   : 95.16% ( 118/124 )
Branches     : 83.33% ( 35/42 )
Functions    : 96.36% ( 53/55 )
Lines        : 95.86% ( 116/121 )
================================================================================
```

### Description of how you validated changes

Unit tests pass

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Leo10Gama pushed a commit to Leo10Gama/aws-cdk that referenced this issue Nov 13, 2024
…d of queue policy (aws#32032)

### Issue # (if applicable)

Tracking aws#31785.

### Reason for this change

Currently if a dead letter queue (DLQ) is specified then a queue policy is created for the DLQ which allows the schedule to send messages. This is incorrect and the permissions should be added to the schedule's execution role instead.  

### Description of changes

Add `sqs:SendMessage` permission to execution role's policy statement if dead letter queue is specified. This follows the [service docs](https://docs.aws.amazon.com/scheduler/latest/UserGuide/configuring-schedule-dlq.html#configuring-schedule-dlq-permissions) for configuring a schedule DLQ.

Also removed cross-region validation as the deployment will fail fast for this case so the validation is unnecessary.

### Description of how you validated changes

Updated unit tests and added a new integration test with dead letter queue setup on the schedule

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 14, 2024
### Issue # (if applicable)

Tracking: #31785 

### Reason for this change

Improve README quality.

### Description of changes

Mostly grammar and formatting fixes.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 14, 2024
…arget throws synth error (#32105)

### Issue # (if applicable)

Tracking #31785 

### Reason for this change

Similar to what was done in this PR for the Lambda Invoke target: #31837. 
Reasoning is explained in this [comment](#29615 (comment)):
> At synth time, there are cases where CDK does not know about the actual environment (e.g. when the environment is using Tokens). In other words, the environment check works in some scenarios and not others. This creates more confusion than benefit.
IF cross env target support is added in the future, this check will become a blocker for CDK customers to set up cross env target.


### Description of changes

Remove synth-time error that checks for same account and region for all targets to allow use of imported resources as target.

### Description of how you validated changes

All tests are passing.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 15, 2024
…sis stream targets (#32122)

### Issue # (if applicable)

Tracking #31785 

### Reason for this change

We want to enforce principle of least privilege when granting target actions to the scheduler execution role. From the Scheduler docs, only `kinesis:PutRecord` and `sqs:SendMessage` are required. Previously we were using built-in grant methods for these targets that granted additional permissions. If wider permissions are needed the user can always provide their own IAM role for the scheduler to use.

KMS permissions references from service docs:
- Kinesis stream: https://docs.aws.amazon.com/streams/latest/dev/permissions-user-key-KMS.html#example-producer-permissions
- SQS Queue: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-key-management.html#sqs-what-permissions-for-sse

### Description of changes

- Explicitly grant only the necessary target permissions
- If target uses customer-managed key for SSEKMS, grant key permissions to role as well

### Description of how you validated changes

- updated unit tests
- deployed stacks with SSEKMS targets and verified the scheduled actions succeed

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Nov 17, 2024
…stead of l2 (#32150)

### Issue # (if applicable)

Tracking #31785 

### Reason for this change

Since the Kinesis Data Firehose Alpha module is in developer preview and contains the L2 construct for a Firehose Delivery Stream, we should make this upgrade from L1 to L2 now while the module is experimental instead of in the future, where we would need to add a V2 for this target (like for event targets, see #30189) to avoid breaking customers. 

### Description of changes

Replace CfnDeliveryStream with IDeliveryStream. The L1 uses `S3DestinationConfiguration` property whereas the L2 uses `ExtendedS3DestinationConfiguration` property  (includes additional fields on top of S3DestinationConfiguration fields). According to 
 https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-kinesisfirehose-deliverystream.html, "If you change the delivery stream destination from an Amazon S3 destination to an Amazon ES destination, update requires [some interruptions](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html#update-some-interrupt)."

### Description of how you validated changes

- Updated unit tests and integration test
- Deployed locally using CfnDeliveryStream then IDeliveryStream and verified the objects are written to the S3 bucket.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

BREAKING CHANGE: KinesisDataFirehosePutRecord scheduler target now accepts IDeliveryStream instead of CfnDeliveryStream.
mergify bot pushed a commit that referenced this issue Nov 20, 2024
…in Developer Preview (#32207)

### Issue # (if applicable)

Tracking #31785 

### Description of changes

Set maturity to `developer-preview` and update the README stability banner for `scheduler-alpha` and `scheduler-targets-alpha` modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudformation Related to AWS CloudFormation management/tracking Issues that track a subject or multiple issues p2
Projects
None yet
Development

No branches or pull requests

2 participants