Skip to content

fix(stepfunctions): proper s3 arn naming#22694

Closed
aaronatbissell wants to merge 1 commit intoaws:mainfrom
aaronatbissell:fix-step-functions-athena-start-query-execution-task-s3-arn-generation
Closed

fix(stepfunctions): proper s3 arn naming#22694
aaronatbissell wants to merge 1 commit intoaws:mainfrom
aaronatbissell:fix-step-functions-athena-start-query-execution-task-s3-arn-generation

Conversation

@aaronatbissell
Copy link
Copy Markdown
Contributor

When using an athena start-query-execution task, the CDK generates a default policy including some permissions for the S3 buckets used as an output location. This S3 bucket policy includes the S3 bucket ARN as a resource, but the auto-generated ARN includes region and account ID, and shouldn't. When trying to deploy, you end up with an error that looks like this:

Error: The stack failed to deploy: UPDATE_ROLLBACK_COMPLETE: Resource arn:aws:s3:us-east-1:accountId:bucket/export can not contain region information.

I believe this bug was introduced in PR #22314

fixes #22650


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Oct 28, 2022

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Oct 28, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team October 28, 2022 20:03
Copy link
Copy Markdown
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ebbdf5c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Copy Markdown
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch, thanks for submitting this! I'm concerned, however, that the update wasn't caught by any integration tests. That means that either the fix didn't make a change to the template as we'd expect or that we are missing integration test coverage. I think this is a coverage issue. Please add an integration test that covers this fix.

Additionally, please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests).

@TheRealAmazonKendra
Copy link
Copy Markdown
Contributor

Looks like this fix has actually already been merged. Closing this one.

mergify bot pushed a commit that referenced this pull request Oct 31, 2022
…egration tests for better coverage (#22699)

Based on the recommendation from @TheRealAmazonKendra in #22694, here is a modification to the integration test for the fix to #22650 . This is in addition to PR #22692 which fixed the original problem, thanks to @kaizencc.

I know this bug has already been closed, but I already had this written, so I just figured I would submit it anyways.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*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

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(stepfunctions): athena-start-query-execution tasks generate invalid S3 ARNs

3 participants