feat(core): CfnResource depends on methods

[jusdino]

Add some new methods to allow a minimal interface for viewing and editing resource-to-resource dependencies that mirrors the behavior of CfnResource.addDependsOn().

Related to #20418 - details for justification are there

---

All Submissions:

• [x] Have you followed the guidelines in our Contributing guide?
• [ ] This PR adds new unconventional dependencies following the process described here
• [x] Have you added the new feature to an integration test?
  • [x] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

Adding new Unconventional Dependencies:

New Features

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

[gitpod-io[bot]]

[jusdino]

@comcalvi, based on what I learned in #20281, I think we'll eventually have to reopen this pull request from a personal fork of aws-cdk instead of this organization-owned fork, since your mergify bot cannot push changes back to this fork branch. If you're close to satisfied with this PR, let me know and I'll do that. I didn't want to do that before we were nearly there, since I didn't want to break up our workflow while we still had revisions to work out.

[TheRealAmazonKendra]

@jusdino Go into the aws-cdk-lib package and run yarn pkglint. Commit the change to the README file for aws-cdk-lib.

[jusdino]

Ok, I detected the Names.uniqueId bug in an existing unit test and added a two-part integration test. I can look into adding some more unit tests for further coverage next.

[jusdino]

Had to roll back an update merge since it seems to have introduced a build error - something about cxapi not having an expected attribute.

[aws-cdk-automation]

This PR has been in the BUILD FAILING 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.

[jusdino]

This PR has been in the BUILD FAILING 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.

@TheRealAmazonKendra any chance you can turn off this auto-close? Still waiting for feedback on this update before I go trolling through the whole codebase to update references for this proposed method name change.

[Naumel]

@Mergifyio update

With regard to feedback: the build is failing. That needs to be fixed before all other steps.

[mergify[bot]]

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request. @Mxr-RAPID-DevOps needs to authorize modification on its head branch. err-code: 57B81

[jusdino]

Hello, for better visibility, I will restate here what I added in the comments:

Feedback has been provided already and the ask is to fix the failing build before further feedback is to be added.

Understood - just buying myself some time to work on this

[aws-cdk-automation]

This PR has been in the MERGE CONFLICTS 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.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[jusdino]

@Naumel, @TheRealAmazonKendra , is this still on your radar? I hope somebody can get back to this before I have to do another round of fussing with merge conflicts.

[comcalvi]

Overall I don't understand why this can't be implemented by adding only

• _removeAssemblyDependency()
• removeDependency()

and have those methods mirror the implementation of their add counterparts + some helper methods to pull out the common code.

[jusdino]

Thanks for the reviews, @comcalvi and @MrArnoldPalmer . I'll try to address the new comments not part of a PR review change requested here:

This PR makes a number of breaking changes and adds a ton of machinery to our dependency logic, which is already fairly complex, and I'm not sure that we need all of this.

I don't see any breaking changes here - what are you referring to? The closest thing I see is the addDependsOn -> addDependency deprecation, which I did specifically as a change requested by Amazon. I'm happy to roll that back, if that is ultimately what you want.

The main benefit of this new logic is the ability to remove dependencies. I don't understand why we can't just traverse the construct tree to remove the dependencies.

Traversing the construct tree to remove dependencies is exactly what this enables. With the existing code, there is no mechanism to do that. I'm not sure what your point is here. Can you help me understand?

Overall I don't understand why this can't be implemented by adding only

_removeAssemblyDependency() removeDependency()

and have those methods mirror the implementation of their add counterparts + some helper methods to pull out the common code.

Ok, I'm trying to boil down where my approach differs from what you're suggesting, since I don't think it's far from what I did. Aside from adding _removeAssemblyDependency() and removeDependency() and adding the helper methods to pull out the common code, I also:

• Added an obtainDependencies() method discusion about that piece here
• Added a replaceDependency() method discussion about that piece here
• Deprecated addDependsOn() in favor of addDependency() disscussion about that piece here

If able I would love more info on the use case for the feature as #20418 provides some theoretical stuff but have you run into this limitation using specific constructs in aws-cdk-lib.

Sure - as it happens, the integration test TestStack at aws-cloudformation/test/integ.core-deps.ts is actually based on a real use case I've dealt with. Let's say we have a security rule that prohibits the use of in-line IAM policies in favor of managed policies (the the security rule is what it is, so let's not get into whether that's actually a good idea). In order to alleviate the pain experienced by our developers using AWS-CDK, we implemented an Aspect pattern that will find and replace all in-line policies with a managed policy resource. The problem we ran into is that, if any existing resources (like LogRetention) have added a dependency on the inline policy that we removed, we have no graceful way to salvage the resulting Cfn template.

Did I miss anything? I'm happy to work the smaller changes each of you pointed out, though I hesitate to invest in the details when there seem to be more fundamental questions about this work that still need sorting. Thoughts?

[MrArnoldPalmer]

Sure - as it happens, the integration test TestStack at aws-cloudformation/test/integ.core-deps.ts is actually based on a real use case I've dealt with. Let's say we have a security rule that prohibits the use of in-line IAM policies in favor of managed policies (the the security rule is what it is, so let's not get into whether that's actually a good idea). In order to alleviate the pain experienced by our developers using AWS-CDK, we implemented an Aspect pattern that will find and replace all in-line policies with a managed policy resource. The problem we ran into is that, if any existing resources (like LogRetention) have added a dependency on the inline policy that we removed, we have no graceful way to salvage the resulting Cfn template.

Thank you that definitely helps!

[jusdino]

@comcalvi ?

[comcalvi]

I don't see any breaking changes here - what are you referring to? The closest thing I see is the addDependsOn -> addDependency deprecation, which I did specifically as a change requested by Amazon. I'm happy to roll that back, if that is ultimately what you want.

I missed that the method whose signature changed was internal, that's my mistake.

Traversing the construct tree to remove dependencies is exactly what this enables. With the existing code, there is no mechanism to do that. I'm not sure what your point is here. Can you help me understand?

Sorry I wasn't more clear; I do not understand this mechanism. The main thing I'm missing the usage of filterReasons(). What does this function do? Why do we need to filter any reasons at all? I see filterReasons() used in a few spots, and I don't see what this is doing.

Ok, I'm trying to boil down where my approach differs from what you're suggesting, since I don't think it's far from what I did. Aside from adding _removeAssemblyDependency() and removeDependency() and adding the helper methods to pull out the common code, I also:

The method additions and name changes are good, this comment was about the new mechanism. A comment block detailing this mechanism would be helpful. I'm not understanding what precise capability this is adding.

[jusdino]

Sorry I wasn't more clear; I do not understand this mechanism. The main thing I'm missing the usage of filterReasons(). What does this function do? Why do we need to filter any reasons at all? I see filterReasons() used in a few spots, and I don't see what this is doing.

Ah ok. So the filterReasons function is the core of what I'm explaining here: https://github.com/aws/aws-cdk/pull/20419#discussion_r963923393

Would adding a comment block with something along those lines be helpful?

[comcalvi]

the method will resolve it to a stack-level dependency and, rather than remove the dependency, it will find the matching source and target in the StackDependencyReason, remove that reason, but leave the stack dependency in place, since it will still have the source: ResourceB, target: ResourceD dependency reason. If the user code also then does ResourceB.removeDependsOn(ResourceD), the method will again resolve to the stack level, remove the corresponding StackDependencyReason then, seeing that there is no longer a reason for the dependency, remove the Stack1 on Stack2 dependency as well.

I don't see the purpose of filtering reasons in this process though. From this, I understand that we need to track resource dependencies across stacks. I think today we don't do this, and instead only track the stack itself as a dependency; this is a problem. As you explain, if we only track the stack as a dependency and a user removes one dependency but not the other, we can't determine that the stack dependency should actually be left in place; therefore, we need a mechanism to track the resources themselves as dependencies and not just the stack.

With that in mind, I would expect the fundamental change to add the construct path to the dependency list, eg Stack2/Resource (where today, it's just Stack2). To achieve that, you're tracking source and target for each dependency. That makes sense. What doesn't make sense to me is why we need to filter the dependency reasons. What problem does filtering dependencies sovle?

[jusdino]

What doesn't make sense to me is why we need to filter the dependency reasons. What problem does filtering dependencies sovle?

Sorry for the slow reply - I've been sick and wanted to think this through. My short answer is, looking closer, the filterReasons may be a bit overkill.

What we really need is to be able to match a StackDependencyReason reliably when removing resource-to-resource dependencies that ended up resolving as stack-to-stack dependencies. The filterReasons function is doing this for us but it may be a bit more complex than necessary specifically because I was trying to be a bit defensive in my approach. I was specifically thinking about the situation where, somehow, a source or target of null or undefined was added to the stackDependency.reasons. I don't think the public interface allows such a thing to happen, but if it did, I wanted a smarter approach to pulling out a match than specifically looking for source == reason.source && target == reason.target. Otherwise, that reason would never match and so would prevent the StackDependency from ever being completely removed.

It sounds like your preference would be to simplify. I could simplify/remove the filterReasons function if you would rather leave that null/undefined unaddressed, which I think is probably fine since it shouldn't happen, anyway.

[comcalvi]

Thanks for the explanation, this makes sense. It's good to think of edge cases like this, and we should definitely guard against it. However I'd prefer to fail in this case, since the API shouldn't allow this to happen. Even it happens to be impossible for this case to occur with today's code, future changes may have bugs that create a null or undefined reason that could be silently ignored if we just filter out those reasons. I'd prefer we throw an error if we encounter a null or undefined reason and remove the filterReasons() method entirely, since such a reason existing means that something has gone wrong and the deployment may fail (or worse yet, deploy successfully by chance but with missing dependency logic which may cause it to fail for seemingly no reason in the future).

[jusdino]

Thanks for the explanation, this makes sense. It's good to think of edge cases like this, and we should definitely guard against it. However I'd prefer to fail in this case, since the API shouldn't allow this to happen. Even it happens to be impossible for this case to occur with today's code, future changes may have bugs that create a null or undefined reason that could be silently ignored if we just filter out those reasons. I'd prefer we throw an error if we encounter a null or undefined reason and remove the filterReasons() method entirely, since such a reason existing means that something has gone wrong and the deployment may fail (or worse yet, deploy successfully by chance but with missing dependency logic which may cause it to fail for seemingly no reason in the future).

Sounds good. I'll work on that next along with the smaller changes from your last review.

[jusdino]

@comcalvi, @MrArnoldPalmer, I think this covers everything except for what to do about IElement - just need some direction on which way to go with that.

[comcalvi]

for IElement, please leave the original union type Element as it was.

[comcalvi]

@jusdino Can you please enable contributions by maintainers to your fork? I have a commit I'd like to push to this PR, and then I'll be happy to approve it. Thanks for all your hard work on this PR, this is in a really good spot now!

[jusdino]

@jusdino Can you please enable contributions by maintainers to your fork? I have a commit I'd like to push to this PR, and then I'll be happy to approve it. Thanks for all your hard work on this PR, this is in a really good spot now!

Glad to be close to the finish line!

So this PR is from a fork on a GitHub organization and I wasn’t able to find a way to do that with my last PR from there. Last time, I ended up reopening a new PR from a personal fork. I can do that here as well though it will take me a bit - got some personal stuff going on atm.

For reference: https://github.com/aws/aws-cdk/pull/20419#issuecomment-1195662725

[comcalvi]

Thanks! Just link it here and I'll approve once I push a commit to it.

[jusdino]

@comcalvi , #23383 is ready :+1:

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[comcalvi]

Closing this one in favor of #23383