fix(core): logicalId is consumed prior to being overridden (#20560)

When using `Stack.exportValue` to manually create a CloudFormation
export, the logicalId of the referenced resource is used to generate the
logicalId of the `CfnExport`. Because `exportValue` creates a
`CfnExport` _and_ returns an `importValue` it needs to _resolve_ the
logicalId at call time. If the user later overrides the logicalId of the
referenced resource, that override is reflected in the export/import
that was created earlier.

There doesn't seem to be a way to solve this without incurring a
breaking change so this PR attempts to smooth a rough edge by "locking"
the `logicalId` when `exportValue` is called. If the user attempts to
override the id _after_ that point, an error message will be thrown

closes #14335


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] 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*

Author: corymhall

packages/@aws-cdk/core/lib/cfn-element.ts

@@ -47,6 +47,14 @@ export abstract class CfnElement extends CoreConstruct {
    */
   private _logicalIdOverride?: string;
 
+  /**
+   * If the logicalId is locked then it can no longer be overridden.
+   * This is needed for cases where the logicalId is consumed prior to synthesis
+   * (i.e. Stack.exportValue).
+   */
+  private _logicalIdLocked?: boolean;
+
+
   /**
    * Creates an entity and binds it to a tree.
    * Note that the root of the tree must be a Stack object (not just any Root).
@@ -75,7 +83,37 @@ export abstract class CfnElement extends CoreConstruct {
    * @param newLogicalId The new logical ID to use for this stack element.
    */
   public overrideLogicalId(newLogicalId: string) {
-    this._logicalIdOverride = newLogicalId;
+    if (this._logicalIdLocked) {
+      throw new Error(`The logicalId for resource at path ${Node.of(this).path} has been locked and cannot be overridden\n` +
+        'Make sure you are calling "overrideLogicalId" before Stack.exportValue');
+    } else {
+      this._logicalIdOverride = newLogicalId;
+    }
+  }
+
+  /**
+   * Lock the logicalId of the element and do not allow
+   * any updates (e.g. via overrideLogicalId)
+   *
+   * This is needed in cases where you are consuming the LogicalID
+   * of an element prior to synthesis and you need to not allow future
+   * changes to the id since doing so would cause the value you just
+   * consumed to differ from the synth time value of the logicalId.
+   *
+   * For example:
+   *
+   * const bucket = new Bucket(stack, 'Bucket');
+   * stack.exportValue(bucket.bucketArn) <--- consuming the logicalId
+   * bucket.overrideLogicalId('NewLogicalId') <--- updating logicalId
+   *
+   * You should most likely never need to use this method, and if
+   * you are implementing a feature that requires this, make sure
+   * you actually require it.
+   *
+   * @internal
+   */
+  public _lockLogicalId(): void {
+    this._logicalIdLocked = true;
   }
 
   /**

packages/@aws-cdk/core/lib/stack.ts

@@ -907,6 +907,14 @@ export class Stack extends CoreConstruct implements ITaggable {
       throw new Error('exportValue: either supply \'name\' or make sure to export a resource attribute (like \'bucket.bucketName\')');
     }
 
+    // if exportValue is being called manually (which is pre onPrepare) then the logicalId
+    // could potentially be changed by a call to overrideLogicalId. This would cause our Export/Import
+    // to have an incorrect id. For a better user experience, lock the logicalId and throw an error
+    // if the user tries to override the id _after_ calling exportValue
+    if (CfnElement.isCfnElement(resolvable.target)) {
+      resolvable.target._lockLogicalId();
+    }
+
     // "teleport" the value here, in case it comes from a nested stack. This will also
     // ensure the value is from our own scope.
     const exportable = referenceNestedStackValueInParent(resolvable, this);

packages/@aws-cdk/core/test/stack.test.ts

@@ -576,6 +576,52 @@ describe('stack', () => {
     expect(templateA).toEqual(templateM);
   });
 
+  test('throw error if overrideLogicalId is used and logicalId is locked', () => {
+    // GIVEN: manual
+    const appM = new App();
+    const producerM = new Stack(appM, 'Producer');
+    const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });
+    producerM.exportValue(resourceM.getAtt('Att'));
+
+    // THEN - producers are the same
+    expect(() => {
+      resourceM.overrideLogicalId('OVERRIDE_LOGICAL_ID');
+    }).toThrow(/The logicalId for resource at path Producer\/ResourceXXX has been locked and cannot be overridden/);
+  });
+
+  test('do not throw error if overrideLogicalId is used and logicalId is not locked', () => {
+    // GIVEN: manual
+    const appM = new App();
+    const producerM = new Stack(appM, 'Producer');
+    const resourceM = new CfnResource(producerM, 'ResourceXXX', { type: 'AWS::Resource' });
+
+    // THEN - producers are the same
+    resourceM.overrideLogicalId('OVERRIDE_LOGICAL_ID');
+    producerM.exportValue(resourceM.getAtt('Att'));
+
+    const template = appM.synth().getStackByName(producerM.stackName).template;
+    expect(template).toEqual({
+      Outputs: {
+        ExportsOutputFnGetAttOVERRIDELOGICALIDAtt2DD28019: {
+          Export: {
+            Name: 'Producer:ExportsOutputFnGetAttOVERRIDELOGICALIDAtt2DD28019',
+          },
+          Value: {
+            'Fn::GetAtt': [
+              'OVERRIDE_LOGICAL_ID',
+              'Att',
+            ],
+          },
+        },
+      },
+      Resources: {
+        OVERRIDE_LOGICAL_ID: {
+          Type: 'AWS::Resource',
+        },
+      },
+    });
+  });
+
   test('automatic cross-stack references and manual exports look the same: nested stack edition', () => {
     // GIVEN: automatic
     const appA = new App();