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

redshift-alpha: existing integration test actually fails to deploy #31817

Open
1 task
badmintoncryer opened this issue Oct 19, 2024 · 3 comments
Open
1 task
Labels
@aws-cdk/aws-redshift Related to Amazon Redshift bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@badmintoncryer
Copy link
Contributor

Describe the bug

There is an integ.database.ts integ test, but this test cannot actually be deployed to AWS.

~/git/aws-cdk/packages/@aws-cdk/aws-redshift-alpha
❯ yarn integ test/integ.database.js --force           
yarn run v1.22.21
$ integ-runner --language javascript test/integ.database.js --force

Verifying integration test snapshots...

  UNCHANGED  integ.database 1.152s

Snapshot Results: 

Tests:    1 passed, 1 total

Running integration tests for failed tests...

Running in parallel across regions: us-east-1, us-east-2, us-west-2
Running test /Users/kazuhoshinozuka/git/aws-cdk/packages/@aws-cdk/aws-redshift-alpha/test/integ.database.js in us-east-1

✨  Synthesis time: 0.03s

...

aws-cdk-redshift-cluster-database: deploying... [1/1]
aws-cdk-redshift-cluster-database: creating CloudFormation changeset...

...

Failed resources:
aws-cdk-redshift-cluster-database | 12:36:28 AM | CREATE_FAILED        | Custom::RedshiftDatabaseQuery               | User/TablePrivileges/Resource/Resource/Default (UserTablePrivileges3829D614) Received response status [FAILED] from custom resource. Message returned: Statement status was FAILED: ERROR: syntax error at or near "-" in context "SELECT ON clustereb0386a7-", at line 1
  Position: 48

Logs: /aws/lambda/aws-cdk-redshift-cluster--QueryRedshiftDatabase3de-OKD1xpr2NRJ9

  Position: 48
    at waitForStatementComplete (/var/task/redshift-data.js:34:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async executeStatement (/var/task/redshift-data.js:18:5)
    at async Promise.all (index 0)
    at async grantPrivileges (/var/task/privileges.js:35:5)
    at async handler (/var/task/privileges.js:11:9) (RequestId: 0de4c80b-c1c3-4d12-8ea9-45dc9b986515)
❌  aws-cdk-redshift-cluster-database failed: The stack named aws-cdk-redshift-cluster-database failed creation, it may need to be manually deleted from the AWS console: ROLLBACK_COMPLETE: Received response status [FAILED] from custom resource. Message returned: Statement status was FAILED: ERROR: syntax error at or near "-" in context "SELECT ON clustereb0386a7-", at line 1
  Position: 48

Generated template file is here.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

2.160.0

Expected Behavior

The integ test should be able to deploy.

Current Behavior

Failed to deploy.

Reproduction Steps

You can perform this integration test as follows.

cd packages/@aws-cdk/aws-redshift-alpha
yarn 
yarn build
yarn integ test/integ.database.js --force

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.160.0

Framework Version

No response

Node.js Version

v22.9.0

OS

macOS

Language

TypeScript

Language Version

No response

Other information

No response

@badmintoncryer badmintoncryer added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 19, 2024
@github-actions github-actions bot added the @aws-cdk/aws-redshift Related to Amazon Redshift label Oct 19, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Oct 21, 2024

Looks like grantPrivileges() > executeStatement() is failing while executing GRANT ${actions.join(', ')} ON ${tableName} TO ${username}.

Syntax for GRANT allows to specify comma separated actions (SELECT, UPDATE,..., etc.) ON [ TABLE ] table_name where TABLE keyword is optional.

Table name can use:

  • Standard identifiers which can contain ASCII single-byte alphanumeric characters, underscores, or dollar signs, or UTF-8 multibyte characters two to four bytes long. It cannot contain hyphen -.
  • Delimited identifiers which begin and end with double quotation marks ("). The identifier can contain any standard UTF-8 printable characters other than the double quotation mark itself. Therefore, you can create column or table names that include otherwise illegal characters, such as spaces or the percent symbol.

In the issue, the error is syntax error at or near "-" in context "SELECT ON clustereb0386a7-". So looks like table name somehow contains -. Hence, the database-query-provider should either be updated to use delimited identifiers, OR, integration test should make sure not to use illegal characters in table names.

@ashishdhingra ashishdhingra self-assigned this Oct 21, 2024
@ashishdhingra ashishdhingra added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 21, 2024
@ashishdhingra ashishdhingra removed their assignment Oct 21, 2024
@5d
Copy link
Member

5d commented Dec 10, 2024

Hi @badmintoncryer , after some investigation, we determined that this issue is a regression introduced by PR #24308. I am currently working on a workaround.

@badmintoncryer
Copy link
Contributor Author

@5d Thank you for your information!

mergify bot pushed a commit that referenced this issue Dec 13, 2024
…#32452)

### Issue # (if applicable)

Closes 
 - #31817
 - #29231

### Reason for this change



This regression was introduced in PR#24308, which added support for executing the `ALT TABLE` SQL statement when the table name is changed in the CDK construct. The root cause is in the modification of the physical ID of the custom resource Lambda function. Previously, this Lambda function was returning the `tableName` after table is created. However, the physical ID was updated to a combined ID in the format `clusterId:databaseName:tableNamePrefix:stackIdSuffix`.

This change breaks the logic that depends on the return value being used as the `tableName`. For example, in the reported issue, the integration test relies on this value to grant permissions to specific users. The new combined ID format causes SQL statement errors, as described in the issue.

### Description of changes



The fix involves adding logic to detect whether the resource's physical ID is in the new format. If it is, the logic reconstructs the `tableName` from the combined ID. This ensures that Lambda functions referencing the `tableName` receive the correct value.

### Description of how you validated changes



- rerun unit test cases
- deployed integration test cases

### 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-redshift Related to Amazon Redshift bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

3 participants