(aws-ecs): Unable to use multiple EnvironmentFile

i906 commented 2 years ago

Describe the bug

The use of multiple ecs.EnvironmentFile.fromAsset() in taskDefinition.addContainer() resulted to an error.

Expected Behavior

Able to use ecs.EnvironmentFile.fromAsset() multiple times in a stack.

Current Behavior

Unable to synth the stack.

~/cdk/node_modules/constructs/src/construct.ts:403
      throw new Error(`There is already a Construct with name '${childName}' in ${typeName}${name.length > 0 ? ' [' + name + ']' : ''}`);
            ^
Error: There is already a Construct with name 'EnvironmentFile' in ContainerDefinition [DefaultContainer]
    at Node.addChild (~/cdk/node_modules/constructs/src/construct.ts:403:13)
    at new Node (~/cdk/node_modules/constructs/src/construct.ts:71:17)
    at new Construct (~/cdk/node_modules/constructs/src/construct.ts:464:17)
    at new Asset (~/cdk/node_modules/aws-cdk-lib/aws-s3-assets/lib/asset.js:1:473)
    at AssetEnvironmentFile.bind (~/cdk/node_modules/aws-cdk-lib/aws-ecs/lib/environment-file.js:1:1481)
    at new ContainerDefinition (~/cdk/node_modules/aws-cdk-lib/aws-ecs/lib/container-definition.js:1:3421)
    at Ec2TaskDefinition.addContainer (~/cdk/node_modules/aws-cdk-lib/aws-ecs/lib/base/task-definition.js:1:8925)
    at new SampleStack (~/cdk/lib/sample-stack.ts:19:20)
    at createApp (~/cdk/bin/cloud-cdk.ts:336:3)
    at Object.<anonymous> (~/cdk/bin/cloud-cdk.ts:384:1)

Reproduction Steps

import * as cdk from "aws-cdk-lib";
import * as ecs from "aws-cdk-lib/aws-ecs";
import { Construct } from "constructs";

export class SampleStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const taskDefinition = new ecs.Ec2TaskDefinition(this, 'TaskDef');

    taskDefinition.addContainer('DefaultContainer', {
      image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
      memoryLimitMiB: 512,
      environmentFiles: [
        ecs.EnvironmentFile.fromAsset('demo-1.env'),
        ecs.EnvironmentFile.fromAsset('demo-2.env'),
      ]
    });
  }
}

Possible Solution

Modify the Asset ID from EnvironmentFile to something that is derived from the asset path.

import { Construct } from "constructs";
import { EnvironmentFile, EnvironmentFileConfig, EnvironmentFileType } from "aws-cdk-lib/aws-ecs";
import { Asset, AssetOptions } from "aws-cdk-lib/aws-s3-assets";
import * as iam from "aws-cdk-lib/aws-iam";
import * as crypto from "crypto";

export class FixedAssetEnvironmentFile extends EnvironmentFile {
  private asset?: Asset;
  private grantees: iam.IGrantable[] = [];

  /**
   * @param path The path to the asset file or directory.
   * @param options
   */
  constructor(
    public readonly path: string,
    private readonly options: AssetOptions = {},
  ) {
    super();
  }

  public grantRead(grantee: iam.IGrantable) {
    this.grantees.push(grantee);
  }

  public bind(scope: Construct): EnvironmentFileConfig {
    const hash = crypto.createHash('sha256')
      .update(this.path)
      .digest('hex');

    // If the same AssetCode is used multiple times, retain only the first instantiation.
    if (!this.asset) {
      this.asset = new Asset(scope, hash, {
        path: this.path,
        ...this.options,
      });
    }

    let grantee = this.grantees.pop();

    while (grantee) {
      this.asset.grantRead(grantee);

      grantee = this.grantees.pop();
    }

    if (!this.asset.isFile) {
      throw new Error(`Asset must be a single file (${this.path})`);
    }

    return {
      fileType: EnvironmentFileType.S3,
      s3Location: {
        bucketName: this.asset.s3BucketName,
        objectKey: this.asset.s3ObjectKey,
      },
    };
  }
}

Additional Information/Context

No response

CDK CLI Version

2.33.0 (build 859272d)

Framework Version

No response

Node.js Version

v16.16.0

OS

MacOS

Language

Typescript

Language Version

3.9.10

Other information

pahud commented 1 year ago

This makes great sense to me. I think we probably should fix this place and allow AssetEnvironmentFile to pass id to the Asset class internally. Feel free to suggest any solution any any PR submission would be highly welcome here.

https://github.com/aws/aws-cdk/blob/cea1039e3664fdfa89c6f00cdaeb1a0185a12678/packages/%40aws-cdk/aws-ecs/lib/environment-file.ts#L15-L17

ryanolee commented 1 year ago

Opened A PR that could possibly resolve this. Not 100% sure if the implementation fits to what you were thinking @pahud but would love to hear back to see if it seems like a reasonable fix. The PR has some more information as to the rationale behind the the proposed changes. If it looks good happy rejigging all the tests!

Thanks in advance for any input :raised_hands:

aws / aws-cdk