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

feat(core): weak references #8891

Closed
wants to merge 4 commits into from
Closed

feat(core): weak references #8891

wants to merge 4 commits into from

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Jul 3, 2020

Ability to mark resources as a 'weak reference' and handling them at
reference resolution.

Weak references is handled differently when resolving references when
these resources are referred across stacks. Instead of using the
Fn::ImportValue intrinsic, we create an AWS::SSM::Parameter in the
target stack and use SSM Parameter Types in the consuming stack.

closes aws/aws-cdk-rfcs#82

Motivation
This is the first part addressing #1972 .

Lambda layers are referenced via version ARNs. Any change to the layer
will produce a new ARN. The old ARNs are still present and can still be
referenced.

However, since Fn::ImportValue is used, CloudFormation blocks all
updates that can change the imported value1, thereby blocking any
change that produces a new layer version ARN.


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

Ability to mark resources as a 'weak reference' and handling them at
reference resolution.

This is handled differently when resolving references if these resources
are referred across stacks. Instead of using the `Fn::ImportValue`
intrinsic, we create an `AWS::SSM::Parameter` in the target stack and
use SSM Parameter Types in the consuming stack.

Motivation
This is the first part to fix #8742.
Lambda layers are referenced via version ARNs. Any change to the layer
will produce a new ARN. The old ARNs are still present and can still be
referenced.

However, since `Fn::ImportValue` is used, CloudFormation blocks all
updates that can change the imported value[1], thereby blocking any
change that produces a new layer version ARN.

[1]: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-importvalue.html
@nija-at nija-at requested review from rix0rrr, eladb and a team July 3, 2020 12:17
@nija-at nija-at self-assigned this Jul 3, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 3, 2020
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Constructs-compat is about to be removed in v2.0 so this either needs to be implemented elsewhere or added to the constructs library somehow (not sure if this concept makes sense at that layer)

packages/@aws-cdk/core/README.md Outdated Show resolved Hide resolved
@nija-at
Copy link
Contributor Author

nija-at commented Jul 6, 2020

Constructs-compat is about to be removed in v2.0 so this either needs to be implemented elsewhere or added to the constructs library somehow (not sure if this concept makes sense at that layer)

Suggestions?

What's the motivation to removing it? It seems useful for such use cases.

@nija-at nija-at requested a review from eladb July 6, 2020 08:50
@eladb
Copy link
Contributor

eladb commented Jul 6, 2020

Constructs-compat is about to be removed in v2.0 so this either needs to be implemented elsewhere or added to the constructs library somehow (not sure if this concept makes sense at that layer)

Suggestions?

I need to think about it. Maybe we can discuss over a GitHub issue?

What's the motivation to removing it? It seems useful for such use cases.

I am working on an rfc that will provide all the details. The TL;DR is that having a separate Construct base class in the AWS CDK will hinder composability with other CDK domains such as cdk8s because it won’t be possible to pass a CDK construct as the scope of a cdk8s’s construct.

@nija-at
Copy link
Contributor Author

nija-at commented Jul 6, 2020

Maybe we can discuss over a GitHub issue?

@eladb

Let's discuss here (not very different from an issue) or on aws/aws-cdk-rfcs#82 or on the issue for your RFC.

Can we keep this feature, and make this (a generic form of this) a requirement for your RFC?

Enable weak reference for a resource:

```ts
resource.node.enableWeakReferences();
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a tri-state to accomodate existing deployments, and you need to explain people how to migrate from a situation where they are stuck performing an update they are planning to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced this is needed actually or there is RoI for the added complexity.

I have only one use case for this. Users have likely worked around when they discovered this gap by keeping the layer and the function in the same stack or some other mechanism. The issue has been open long enough that I don't think anyone is waiting.
Nevertheless, I've asked if anyone is stuck in this state on a production stack.

Throwing this behind a feature flag that's enabled for new projects should be sufficient. Existing users will have to destroy and re-create their stacks to avail this benefit.

packages/@aws-cdk/core/lib/private/refs.ts Show resolved Hide resolved
packages/@aws-cdk/core/lib/private/refs.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/private/refs.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/private/refs.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/construct-compat.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Can you provide a real example here and not just conceptual?

If this API is on CfnResource it means it will be pretty tedious to use (users would have to dig up the L1 from the L2).

I also feel this might fall more naturally at the stack level. Is there a use case for weak references only to specific resources.

Let’s write a short RFC. I feel this is not baked.

@nija-at
Copy link
Contributor Author

nija-at commented Jul 7, 2020

@eladb -

I see why this was confusing without a real example. Here is how I have been using to deploy and test this with a real example - 02427c7 dea9966. Let me know if I should explain this better in the README.

The only use case I know of is the one in the PR description. Configuring this on the L1 inside the L2's implementation feels like the correct place.

I'm not aware of any other use cases for this or there is not sufficient breadth in solution space to warrant an RFC.
AFAICT, there are no one way doors we're taking here. When we need to accommodate other use cases, we can adjust the API.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8eee872
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@nija-at nija-at requested a review from rix0rrr July 7, 2020 09:17
@jogold
Copy link
Contributor

jogold commented Jul 7, 2020

Configuring this on the L1 inside the L2's implementation feels like the correct place.

How about also adding a enableWeakReference() in the Resource class that acts on the defaultChild?

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

As discussed, since this is currently only based on the Lambda layers use case, we should probably scope this down to the lambda module instead of introducing a core feature without taking a step back and thinking about it as a general mechanism.

One important issue to take into account (both in the lambda case and in the general case) is that we should try to avoid a situation where users are expected to issue an "empty update" to stacks as it conflicts with a core design and operational principle which says that updates must be triggered by a source change. Ideally, if the value of the SSM parameter is immutable (i.e. we change the parameter name when the value changes), then this will be reflected in the consuming template and therefore won't be an empty update any longer.

@nija-at
Copy link
Contributor Author

nija-at commented Jul 7, 2020

we should probably scope this down to the lambda module instead of introducing a core feature

@eladb - do you see a way to scope this down to just the lambda module?

Even when scoped down, this will need to introduce a new SSM parameter type in the consuming stack. Given the centralized nature of prepare and synth, this information (about cross-stack refs) is only available in core during synthesis. This will likely mean some kind of change in core, correct?

Something I've missed?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 8, 2020

As discussed, since this is currently only based on the Lambda layers use case, we should probably scope this down to the lambda module instead of introducing a core feature without taking a step back and thinking about it as a general mechanism.

I feel that this needs to be a core feature as well, and this is how it needs to be implemented. We get very regular complaints about people being stuck because they cannot apply their update (which leads to an export change).

Sometimes this is:

  • An innocuous change (such as this one)
  • For testing, so people generally don't care about service interruptions and just want to get on with it
  • A real change that wouldn't have (major) observable effects.
  • A real change that would impact service availability in a serious way. To have users protect themselves from that, actually Stack Policies would be a better mechanism.

How we can encourage/enforce the right usage at the L2 level by default is another matter, and one that we can always discuss later.

However, it's pretty clear that this mechanism needs to exist, and by making it exist we can at least give users that find themselves in situations 1-3 a way forward without too much hassle.

Therefore, this is purposely introduced at the L1 level. Possible, but not method of first resort.

@eladb
Copy link
Contributor

eladb commented Jul 8, 2020

As discussed, since this is currently only based on the Lambda layers use case, we should probably scope this down to the lambda module instead of introducing a core feature without taking a step back and thinking about it as a general mechanism.

I feel that this needs to be a core feature as well, and this is how it needs to be implemented. We get very regular complaints about people being stuck because they cannot apply their update (which leads to an export change).

Sometimes this is:

  • An innocuous change (such as this one)
  • For testing, so people generally don't care about service interruptions and just want to get on with it
  • A real change that wouldn't have (major) observable effects.
  • A real change that would impact service availability in a serious way. To have users protect themselves from that, actually Stack Policies would be a better mechanism.

How we can encourage/enforce the right usage at the L2 level by default is another matter, and one that we can always discuss later.

However, it's pretty clear that this mechanism needs to exist, and by making it exist we can at least give users that find themselves in situations 1-3 a way forward without too much hassle.

Therefore, this is purposely introduced at the L1 level. Possible, but not method of first resort.

I am all in favor of introducing this feature in code but I'd like to see an RFC for it that takes into account how this is going to be used and the implications on workflows. For example, how do we ensure in CDK Pipelines that when a weak reference changes the consuming stack will be updated?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 9, 2020

I guess we disagree on the riskiness of this particular change, and hence the need for an RFC. I'll agree that the migration story is going to be an interesting one that we have to think about. Flipping from strong references to weak references in one go is unfortunately not possible, there has to be an in-between state which the user needs to control because it depends on how far they got with their particular deployment.

My fear is that doing an RFC process for even the low-level capability pushes it into Big Project territory, which means it's less likely to get done [soon], and so we are serving our customers worse.

At the same time, putting the mechanism in place without actually making it easily accessible to users means it's very likely we're going to suggest workarounds in GitHub issues and leave it in the current state and never bother to implement the nice L2 layer.


(In both cases, it's never going to get done but in one of those cases at least users have a low-level workaround 😛)

@eladb
Copy link
Contributor

eladb commented Jul 9, 2020

My fear is that doing an RFC process for even the low-level capability pushes it into Big Project territory, which means it's less likely to get done [soon], and so we are serving our customers worse.

I understand the concern and the implication on users, but I don't think it's a good reason to give up on an RFC. I was actually thinking lately that we need a "mini-RFC" format just for these situations, and even started sketching what that looks like. My thought was that the mini-RFC should basically just be the "working backwards" section, which in most cases is just the README. This is also what I suggested to @nija-at.

I still think a mini-RFC is worth it in this particular case.

@nija-at
Copy link
Contributor Author

nija-at commented Jul 10, 2020

Unfortunately at this time, I don't have the bandwidth to write an RFC. Closing this PR.

@nija-at nija-at closed this Jul 10, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 15, 2020

I recently realized we need this pretty badly for CDK Pipelines as well, see here: #10172

Reopening so we don't forget.

@rix0rrr rix0rrr reopened this Sep 15, 2020
@eladb
Copy link
Contributor

eladb commented Sep 17, 2020

I recently realized we need this pretty badly for CDK Pipelines as well, see here: #10172

Reopening so we don't forget.

It's not related to pipelines, it's a general problem we have with strong references: #7602

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 21, 2020

It's not related to pipelines, it's a general problem we have with strong references: #7602

Yes but using CLI deployments one can work around it (deploying consumer stack first). Using pipeline deployments that workaround is not even possible.

@eladb
Copy link
Contributor

eladb commented Sep 21, 2020

It's not related to pipelines, it's a general problem we have with strong references: #7602

Yes but using CLI deployments one can work around it (deploying consumer stack first). Using pipeline deployments that workaround is not even possible.

Perhaps but I don't think this means we should ignore the CLI experience when we come up with a solution.

@nija-at
Copy link
Contributor Author

nija-at commented Oct 29, 2020

Closing this back down since we don't have an urgent need for this. (discussed with @rix0rrr )

@nija-at nija-at closed this Oct 29, 2020
@rix0rrr rix0rrr deleted the nija-at/weak-refs branch April 12, 2021 09:59
@ayush987goyal
Copy link
Contributor

@rix0rrr Any further update on re-opening this change? What can be the implications of adding this since it seems like an opt-in non breaking change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weak references
6 participants