fix: prevent cyclic dependencies created by custom resource provider#32404
Closed
fix: prevent cyclic dependencies created by custom resource provider#32404
Conversation
aws-cdk-automation
requested changes
Dec 6, 2024
Collaborator
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
9cf34ea to
763b0f2
Compare
Contributor
Author
|
Clarification Request There seems to exist no integration test for custom resource providers. |
Collaborator
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
Contributor
Author
|
Exemption Request |
GavinZZ
reviewed
Jan 22, 2025
Member
GavinZZ
left a comment
There was a problem hiding this comment.
Hello, thanks for drafting this PR. I'd like to understand what issue were you seeing prior to this change. Is the cyclic dependency causing the deployment to fail?
|
Yes, the deployment fails with a cyclic dependency error. |
Collaborator
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Contributor
|
Comments on closed issues and PRs are hard for our team to see. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32404 +/- ##
=======================================
Coverage 82.17% 82.17%
=======================================
Files 119 119
Lines 6862 6862
Branches 1158 1158
=======================================
Hits 5639 5639
Misses 1120 1120
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Member
|
Thanks for the quick response. I think this is a great place to add an integ test to make sure now it's deployable if we supply a custom |
|
Unfortunately, I have no clue how an integration test for a custom resource provider could be set up. I would appreciate your help a lot. |
Collaborator
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Collaborator
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Contributor
|
Comments on closed issues and PRs are hard for our team to see. |
1 task
Collaborator
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Contributor
|
Comments on closed issues and PRs are hard for our team to see. |
Member
|
@swachter I can't push to your branch for some reason. Here's the git diff patch file. Feel free to copy this change & use git apply to apply the diff change. Make sure you pull latest remote
|
|
Many thanks for working on the issue, @GavinZZ ! I am sorry that you can not push into the PR branch. The "Allow edits and access to secrets by maintainers" option is checked. I tried to apply the patch this morning. I synced my fork and rebased the branch. Then I tried to apply the patch. First on the PR branch and after this did not work on the main branch. Unfortunately, both times I get the same error: Maybe it is easiest, if you just open a new PR. |
Collaborator
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Member
|
Thanks for getting back to me. I'll create a new PR and reference this old PR. |
Contributor
|
Comments on closed issues and PRs are hard for our team to see. |
Member
|
Closing this in favour of #33600. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for this change
If both, the
isCompleteHandlerand theroleis set in custom resourceProviderPropsthen a resource dependencies cycle results.If
roleis specified then that role is used for all 3 framework lambdas (i.e.onEventFunction,isCompleteFunction, andtimeoutFunction). In order to grant additional required permissions to these functions, a policy is created under the hood. That policy is calledDefaultPolicy.The
DefaultPolicyis attached to the givenroleand includes thelambda:InvokeFunctionpermissions for the two handler functions (onEventHandlerandisCompleteHandler). In addition, theDefaultPolicyincludes thestates:StartExecutionpermission that is granted to theonEventFunctionby the call `waiterStateMachine.grantStartExecution(onEventFunction);.This results in the following dependencies cycle:
waiter state machine --(via its description)--> isCompleteFunction / timeoutFunction --(via explicits dependsOn)--> DefaultPolicy --(via resource constraint)--> waiter state machine
If the resource constraint is dropped from the
states:StartExecutiongrant then the cyclic dependency is cut.Description of changes
Drop the resource constraint from the
states:StartExecutiongrant if an execution role for the framework lambdas is specified.Description of how you validated changes
I tried the following workaround locally:
This workaround applies the proposed modification to the created
DefaultPolicy.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license