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

construct CfnReferences for CfnInclude'ed resources #7375

Closed
2 tasks
anguslees opened this issue Apr 15, 2020 · 4 comments
Closed
2 tasks

construct CfnReferences for CfnInclude'ed resources #7375

anguslees opened this issue Apr 15, 2020 · 4 comments
Assignees
Labels
@aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package @aws-cdk/core Related to core CDK functionality effort/medium Medium work item – several days of effort feature-request A feature should be added or improved.

Comments

@anguslees
Copy link
Contributor

A way to construct a CfnReference for a resource (with known logicalId) that was included via CfnInclude.

Use Case

I have some non-trivial existing cfn. I want to start using cdk. I have a NestedStack that is basically just a CfnInclude of my old cfn. Great.

Now I want to refer to resources within that NestedStack from my parent/sibling stacks. I can manually create CfnOutputs using getAttr in my NestedStack, and reference them appropriately using getAttr again on the NestedStack, but that's boring. Ideally I could create a CfnReference and the usual token/resolve magic would do all that for me.

Except CfnReference is (currently) private, so I can't. I think the only place it is currently exposed is via CfnResource.getAttr, and I don't want the rest of the CfnResource implementation (because I'm not actually creating new cfn output for my resource).

Proposed Solution

Either:

  1. Move CfnResource.getAtt up to CfnRefElement.getAtt. Basically anything that is read-only should maybe go to CfnRefElement/CfnElement.

  2. Create a new dummy "IncludedResource" class intended specifically for use with CfnInclude. I think it should extend CfnRefElement, and output nothing to the synthesized cfn . Give it a fixed logicalId, and (at least) a getAtt method that uses CfnReference.

In addition, the naive Fn.getAtt("S3Bucket", "Arn") example on the end of https://docs.aws.amazon.com/cdk/latest/guide/use_cfn_template.html should be updated to use our new improved solution, and thus be cross-stack safe.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@anguslees anguslees added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 15, 2020
@anguslees
Copy link
Contributor Author

This isn't the way I'd implement it, but the following looks like it works as a PoC:

// About the only useful thing you can do with this is ref and getAtt()
export class DummyResource extends cdk.CfnResource {
    constructor(scope: cdk.Construct, logicalId: string) {
        super(scope, logicalId, {type: "Dummy"});
        this.overrideLogicalId(logicalId);
    }

    public _toCloudFormation(): object {
        return {};
    }
}

@SomayaB SomayaB added the @aws-cdk/core Related to core CDK functionality label Apr 16, 2020
@eladb eladb assigned skinny85 and unassigned eladb Apr 23, 2020
@eladb
Copy link
Contributor

eladb commented Apr 23, 2020

@skinny85 is currently working on a solution for this.

@skinny85
Copy link
Contributor

@skinny85 is currently working on a solution for this.

#7304

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label May 19, 2020
@skinny85 skinny85 added @aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package effort/medium Medium work item – several days of effort labels May 29, 2020
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jun 13, 2020
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jun 23, 2020
…e template

Previously, we created the Tokens needed for references like Ref and Fn::GetAtt
without actually referencing the created L1s in the template,
just by their logical IDs.
That's not strictly correct,
because users of CfnInclude can call overrideLogicalId()
on the CloudFormation elements retrieved from the template,
and we need to make sure to also reflect that in all references to that resource.

Change the code converting from CloudFormation to CDK values to actually retrieve
the appropriate L1 objects when encountering expressions like Ref and Fn::GetAtt.
This also gives us correctly working cross-stack references for free.

Related to aws#7375
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jun 24, 2020
…e template

Previously, we created the Tokens needed for references like Ref and Fn::GetAtt
without actually referencing the created L1s in the template,
just by their logical IDs.
That's not strictly correct,
because users of CfnInclude can call overrideLogicalId()
on the CloudFormation elements retrieved from the template,
and we need to make sure to also reflect that in all references to that resource.

Change the code converting from CloudFormation to CDK values to actually retrieve
the appropriate L1 objects when encountering expressions like Ref and Fn::GetAtt.
This also gives us correctly working cross-stack references for free.

Related to aws#7375
mergify bot pushed a commit that referenced this issue Jun 24, 2020
Previously, we created the Tokens needed for references like `Ref` and `Fn::GetAtt`
without actually referencing the created L1s in the template,
just by their logical IDs.
That's not strictly correct,
because users of `CfnInclude` can call `overrideLogicalId()`
on the CloudFormation elements retrieved from the template,
and we need to make sure to also reflect that in all references to that resource.

Change the code converting from CloudFormation to CDK values to actually retrieve
the appropriate L1 objects when encountering expressions like `Ref` and `Fn::GetAtt`.
This also gives us correctly working cross-stack references for free.

Related to #7375

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@skinny85
Copy link
Contributor

This is now possible with our cfn-include module.

Resolving, let us know if need anything else from our side for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package @aws-cdk/core Related to core CDK functionality effort/medium Medium work item – several days of effort feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

4 participants