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.

Sign up for GitHub

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

Jump to bottom

‼️ (aws-appsync): invalid service role creation #23035

Closed
mtliendo opened this issue Nov 22, 2022 · 7 comments · Fixed by #23054
Closed

‼️ (aws-appsync): invalid service role creation #23035

mtliendo opened this issue Nov 22, 2022 · 7 comments · Fixed by #23054
Assignees
@otaviomacedo
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@mtliendo
Copy link

mtliendo commented Nov 22, 2022 •
edited by ryparker
Loading

PATCHED - Upgrade aws-cdk-lib/aws-appsync to version 2.52.1-alpha.0 or later.

Describe the bug

When attaching a DynamoDB datasource to an API, the generated service role does not include the correct servicerole name
image

(it's missing the .amazonaws part)

Expected Behavior

I would expect the correct service role to be created.

Current Behavior

On version 2.51.1 I created the following stack and deployed the application but it failed on the deploying the API. After inspecting the output, I found that the datasource was not given the correct datasource.

Reverting back to v2.50.0 had the correct service role
image

Reproduction Steps

  • clone the repo
  • update to the latest version
  • deploy
  • view error
  • remove node_modules directory and package-lock.json file
  • revert to v2.50.0
  • deploy
  • view successful deploy

Possible Solution

Appending the correct principal

Additional Information/Context

No response

CDK CLI Version

2.51.1

Framework Version

CDK

Node.js Version

16

OS

mac

Language

Typescript

Language Version

No response

Other information

No response
@mtliendo mtliendo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 22, 2022
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Nov 22, 2022
@jpignata
Copy link

Is this related to 65d8e3d? Do the references to the service principal need to be updated? The commit indicates this change was behind a feature flag.

@jpignata
Copy link

It appears this reference (https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-appsync/lib/data-source.ts#L116) needs to be updated.

mtliendo added a commit to mtliendo/aws-cdk that referenced this issue Nov 22, 2022
@mtliendo
fix: update service role name aws#23035
1ef02b8
@peterwoodworth
Copy link
Contributor

I'm finding this new incorrect behavior is only being applied with the feature flag enabled. You should be able to disable the feature flag for now while we get this fixed up. Thanks for reporting!

@peterwoodworth peterwoodworth added p1 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 Nov 22, 2022
@mtliendo
Copy link
Author

Ah yea, that seems to be the issue. On new CDK projects, the default state of the flag is set to true.

jpignata added a commit to jpignata/aws-cdk that referenced this issue Nov 23, 2022
@jpignata
fix(appsync): fully qualify service principal
2d2f725 
In aws#22819, a feature flag was added to use standardized service
principal names instead of using a database lookup. This reference
wasn't updated which causes failures in new CDK applications that try
to create AppSync resources. This change passes the fully qualified
service principal when creating a service role for a new data source.

fixes aws#23035
@jpignata jpignata mentioned this issue Nov 23, 2022
Closed
jpignata added a commit to jpignata/aws-cdk that referenced this issue Nov 23, 2022
@jpignata
fix(appsync): fully qualify service principal
b2fb4d0 
In aws#22819, a feature flag was added to use standardized service
principal names instead of using a database lookup. This reference
wasn't updated which causes failures in new CDK applications that try
to create AppSync resources. This change passes the fully qualified
service principal when creating a service role for a new data source.

fixes aws#23035
jpignata added a commit to jpignata/aws-cdk that referenced this issue Nov 23, 2022
@jpignata
fix(appsync): fully qualify service principal
9315dee 
In aws#22819, a feature flag was added to use standardized service
principal names instead of using a database lookup. This reference
wasn't updated which causes failures in new CDK applications that try
to create AppSync resources. This change passes the fully qualified
service principal when creating a service role for a new data source.

fixes aws#23035
@robertd robertd mentioned this issue Nov 23, 2022
Merged
4 tasks
@mergify mergify bot closed this as completed in #23054 Nov 23, 2022
mergify bot pushed a commit that referenced this issue Nov 23, 2022
@robertd
fix(appsync): fully qualify service principal (#23054)
0bfce89 
Builds on top of @jpignata 's PR #23050.

Updated integration tests.

Closes #23050
Fixes #23035

----

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

⚠️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.

@ryparker
Copy link
Contributor

Thanks to @robertd for the fix, it has been merged and will be included in the next release shortly after re:invent.

Here’s a workaround for the interim:

Disable the @aws-cdk/aws-iam:standardizedServicePrincipals feature flag in your cdk.json.

cdk.json:

{
  "context": {
    "@aws-cdk/aws-iam:standardizedServicePrincipals": false
  }
}

Feature flag docs

@ryparker ryparker pinned this issue Nov 23, 2022
@ryparker ryparker changed the title aws-appsync: invalid service role creation !! aws-appsync: invalid service role creation Nov 23, 2022
@ryparker ryparker changed the title !! aws-appsync: invalid service role creation ‼️ aws-appsync: invalid service role creation Nov 23, 2022
@peterwoodworth peterwoodworth mentioned this issue Nov 24, 2022
Closed
@ryparker ryparker added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Nov 25, 2022
mrgrain pushed a commit that referenced this issue Nov 28, 2022
@robertd @mrgrain
fix(appsync): fully qualify service principal (#23054)
d7141dd 
Builds on top of @jpignata 's PR #23050.

Updated integration tests.

Closes #23050
Fixes #23035

----

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

ryparker commented Dec 2, 2022 •
edited
Loading

Patched in aws-cdk-lib/aws-appsync version 2.52.1-alpha.0.

@ryparker ryparker removed the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Dec 2, 2022
@ryparker ryparker changed the title ‼️ aws-appsync: invalid service role creation ‼️ (aws-appsync): invalid service role creation Dec 2, 2022
@iliapolo iliapolo unpinned this issue Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@otaviomacedo otaviomacedo

Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Milestone
No milestone
5 participants
@jpignata @otaviomacedo @mtliendo @ryparker @peterwoodworth