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

aws-sqs: redriveAllowPolicy.redrivePermission is not working correctly #29129

Closed
yoshi-d-24 opened this issue Feb 16, 2024 · 2 comments · Fixed by #29130
Closed

aws-sqs: redriveAllowPolicy.redrivePermission is not working correctly #29129

yoshi-d-24 opened this issue Feb 16, 2024 · 2 comments · Fixed by #29130
Labels
@aws-cdk/aws-sqs Related to Amazon Simple Queue Service bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@yoshi-d-24
Copy link
Contributor

yoshi-d-24 commented Feb 16, 2024

Describe the bug

Cfn template value is always byQueue, regardless of the value of redriveAllowPolicy.redrivePermission set in the CDK

const myQueueAllowAll = new sqs.Queue(this, 'MyQueueAllowAll', {
  queueName: 'MyQueueAllowAll',
  redriveAllowPolicy: {
      redrivePermission: sqs.RedrivePermission.ALLOW_ALL,
  },
});

const myQueueDenyAll = new sqs.Queue(this, 'MyQueueDenyAll', {
  queueName: 'MyQueueDenyAll',
  redriveAllowPolicy: {
      redrivePermission: sqs.RedrivePermission.DENY_ALL,
  },
});

Expected Behavior

The configured values are output to the Cfn template.

Current Behavior

The value of the Cfn template is always byQueue

{
    "Resources": {
        "MyQueueAllowAllAF132E44": {
            "Type": "AWS::SQS::Queue",
            "Properties": {
                "QueueName": "MyQueueAllowAll",
                "RedriveAllowPolicy": {
                    "redrivePermission": "byQueue"
                }
            }
        },
        "MyQueueDenyAll6B4D4BA9": {
            "Type": "AWS::SQS::Queue",
            "Properties": {
                "QueueName": "MyQueueDenyAll",
                "RedriveAllowPolicy": {
                    "redrivePermission": "byQueue"
                }
            }
        }
    }
}

Reproduction Steps

Generate cfn template with code written in Describe the bug

Possible Solution

Change the expression of redrivePermission.

queue.ts

      redrivePermission: props.redriveAllowPolicy.redrivePermission
      // When `sourceQueues` is provided in `redriveAllowPolicy`, `redrivePermission` defaults to allow specified queues (`BY_QUEUE`);
      // otherwise, it defaults to allow all queues (`ALLOW_ALL`).
        ?? props.redriveAllowPolicy.sourceQueues ? RedrivePermission.BY_QUEUE : RedrivePermission.ALLOW_ALL,

to

      redrivePermission: props.redriveAllowPolicy.redrivePermission
      // When `sourceQueues` is provided in `redriveAllowPolicy`, `redrivePermission` defaults to allow specified queues (`BY_QUEUE`);
      // otherwise, it defaults to allow all queues (`ALLOW_ALL`).
        ?? (props.redriveAllowPolicy.sourceQueues ? RedrivePermission.BY_QUEUE : RedrivePermission.ALLOW_ALL),

Additional Information/Context

No response

CDK CLI Version

2.128.0

Framework Version

No response

Node.js Version

v20.11.0

OS

Windows 11

Language

TypeScript

Language Version

No response

Other information

No response

@yoshi-d-24 yoshi-d-24 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 16, 2024
@github-actions github-actions bot added the @aws-cdk/aws-sqs Related to Amazon Simple Queue Service label Feb 16, 2024
@pahud
Copy link
Contributor

pahud commented Feb 16, 2024

Yes this should be a tiny fix. Thank you for the PR.

@pahud pahud added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 16, 2024
@mergify mergify bot closed this as completed in #29130 Mar 1, 2024
mergify bot pushed a commit that referenced this issue Mar 1, 2024
…e is specified (#29130)

### Issue #29129

Closes #29129.

### Reason for this change

When `redriveAllowPolicy.redrivePermission` is specified, any value will be output to template as `byQueue`

### Description of changes

1. Fix the evaluation order by enclosing the ternary operators in parentheses
   ```typescript
        ?? (props.redriveAllowPolicy.sourceQueues ? RedrivePermission.BY_QUEUE : RedrivePermission.ALLOW_ALL),
   ```
2.  Added a test case in `packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts` when redrivePermission is specified other than `BY_QUEUE`.
3. Added an integ test case in `packages/@aws-cdk-testing/framework-integ/test/aws-sqs/test/integ.sqs-source-queue-permission.ts`

### Description of how you validated changes

Added a test case in `packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts` when redrivePermission is specified other than `BY_QUEUE`.
Added an integ test case in `packages/@aws-cdk-testing/framework-integ/test/aws-sqs/test/integ.sqs-source-queue-permission.ts`
And ran the test case.

### 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*
Copy link

github-actions bot commented Mar 1, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

godwingrs22 pushed a commit to godwingrs22/aws-cdk that referenced this issue Mar 1, 2024
…e is specified (aws#29130)

### Issue aws#29129

Closes aws#29129.

### Reason for this change

When `redriveAllowPolicy.redrivePermission` is specified, any value will be output to template as `byQueue`

### Description of changes

1. Fix the evaluation order by enclosing the ternary operators in parentheses
   ```typescript
        ?? (props.redriveAllowPolicy.sourceQueues ? RedrivePermission.BY_QUEUE : RedrivePermission.ALLOW_ALL),
   ```
2.  Added a test case in `packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts` when redrivePermission is specified other than `BY_QUEUE`.
3. Added an integ test case in `packages/@aws-cdk-testing/framework-integ/test/aws-sqs/test/integ.sqs-source-queue-permission.ts`

### Description of how you validated changes

Added a test case in `packages/aws-cdk-lib/aws-sqs/test/sqs.test.ts` when redrivePermission is specified other than `BY_QUEUE`.
Added an integ test case in `packages/@aws-cdk-testing/framework-integ/test/aws-sqs/test/integ.sqs-source-queue-permission.ts`
And ran the test case.

### 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-sqs Related to Amazon Simple Queue Service bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants