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

(ecs): Allow specifying that an imported TaskDefinition does not have a revision specified #32485

Open
2 tasks
blimmer opened this issue Dec 11, 2024 · 2 comments
Open
2 tasks
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@blimmer
Copy link
Contributor

blimmer commented Dec 11, 2024

Describe the feature

Today, we cannot specify whether or not an imported TaskDefinition is "fully qualified" (e.g., MyTaskDef:123) or not (e.g., MyTaskDef).

There are certain cases, like granting IAM permissions - see #30390 / #31615, where different behaviors need to happen depending on if the task def is fully qualified with a revision or not.

Use Case

I expose various pgdump containers to assist with dumping Aurora Postgres databases to S3. I create a single, shared Task Definition that I expose via CloudFormation outputs. Then, I use Fn.importValue to import these in shared logic.

Here's the some sample code to show the issue

import { Fn, Stack, type StackProps } from 'aws-cdk-lib';
import { SubnetType } from 'aws-cdk-lib/aws-ec2';
import { Cluster, NetworkMode, type ICluster } from 'aws-cdk-lib/aws-ecs';
import { FargateTaskDefinition } from 'aws-cdk-lib/aws-ecs';
import { Rule, Schedule } from 'aws-cdk-lib/aws-events';
import { Role } from 'aws-cdk-lib/aws-iam';
import { Construct } from 'constructs';
import { EcsTask as EcsTaskTarget, EcsTaskProps } from "aws-cdk-lib/aws-events-targets";

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

    const ecsCluster = new Cluster(this, "EcsCluster", {});
    const clusterMajorVersion = "16";

    // IMPORTANT - this is a task definition _without_ a revision number (e.g., arn:aws:ecs:us-west-2:12345678910:task-definition/PgDump-16)
    const pgDumpTaskDefArn = Fn.importValue(`PgDump-${clusterMajorVersion}-TaskDefArn`);
    const pgDumpContainerName = Fn.importValue(`PgDump-${clusterMajorVersion}-ContainerName`);
    const pgDumpTaskRole = Role.fromRoleArn(
      this,
      "PgDumpTaskRoleImport",
      Fn.importValue(`PgDump-${clusterMajorVersion}-TaskDefRoleArn`)
    );
    const pgDumpExecutionRole = Role.fromRoleArn(
      this,
      "PgDumpExecutionRoleImport",
      Fn.importValue(`PgDump-${clusterMajorVersion}-TaskDefExecutionRoleArn`)
    );

    const pgDumpEcsTask = FargateTaskDefinition.fromFargateTaskDefinitionAttributes(this, "PgDumpTaskDefImport", {
      taskDefinitionArn: pgDumpTaskDefArn,
      executionRole: pgDumpExecutionRole,
      taskRole: pgDumpTaskRole,
      networkMode: NetworkMode.AWS_VPC,
    });

    const commonEcsTaskTargetProps: EcsTaskProps = {
      cluster: ecsCluster,
      taskDefinition: pgDumpEcsTask,
      taskCount: 1,
      subnetSelection: { subnetType: SubnetType.PRIVATE_WITH_EGRESS },
      // Other values are not important
    };

    const scheduleTarget = new EcsTaskTarget(commonEcsTaskTargetProps);
    new Rule(this, "PgDumpScheduleRule", {
      targets: [scheduleTarget],
      schedule: Schedule.cron({
        minute: "0",
        hour: "23",
      }),
    });
  }
}

If you synth this code, you'll see that ecs:RunTask specifies the Task Definition exactly as imported. This assumes that it has a revision attached:

  PgDumpTaskDefImportEventsRoleDefaultPolicy3690671F:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action: ecs:RunTask
            Condition:
              ArnEquals:
                ecs:cluster:
                  Fn::GetAtt:
                    - EcsCluster97242B84
                    - Arn
            Effect: Allow
            Resource:
              Fn::ImportValue: PgDump-16-TaskDefArn
          - Action: ecs:TagResource
            Effect: Allow
            Resource:
              Fn::Join:
                - ""
                - - "arn:"
                  - Ref: AWS::Partition
                  - ":ecs:"
                  - Ref: AWS::Region
                  - :*:task/
                  - Ref: EcsCluster97242B84
                  - /*
          - Action: iam:PassRole
            Effect: Allow
            Resource:
              - Fn::ImportValue: PgDump-16-TaskDefExecutionRoleArn
              - Fn::ImportValue: PgDump-16-TaskDefRoleArn
        Version: "2012-10-17"
      PolicyName: PgDumpTaskDefImportEventsRoleDefaultPolicy3690671F
      Roles:
        - Ref: PgDumpTaskDefImportEventsRoleF7E90D4B
    Metadata:
      aws:cdk:path: ExampleStack/PgDumpTaskDefImport/EventsRole/DefaultPolicy/Resource

Specifically, this action:

- Action: ecs:RunTask
  Condition:
    ArnEquals:
      ecs:cluster:
        Fn::GetAtt:
          - EcsCluster97242B84
          - Arn
  Effect: Allow
  Resource:
    Fn::ImportValue: PgDump-16-TaskDefArn

Instead, I want to grant access to the imported value + :* (see #30390 for why). If I could specify that in my FargateTaskDefinition.fromFargateTaskDefinitionAttributes call, then the downstream logic (implemented here: #31615) could check that.

Proposed Solution

I suggest adding a field on IFargateTaskDefinition like arnIncludesRevision or similar. Then, when we need to know about this (e.g., in #31615), we can use that value instead of checking the string, like @samson-keung added in that PR.

Other Information

For a workaround to the sample code posted above, you can do something like this:

const role = (scheduleTarget as any).role as Role;
const statements = (role as any).defaultPolicy.document.statements as PolicyStatement[];
const withoutBadStatements = statements.filter((s) => {
  const isBadStatement = s.actions.length === 1 && s.actions[0] === "ecs:RunTask";
  return !isBadStatement;
});
(role as any).defaultPolicy.document.statements = withoutBadStatements;

role.addToPrincipalPolicy(
  new PolicyStatement({
    actions: ["ecs:RunTask"],
    resources: [pgDumpTaskDefArn + ":*"], // this is the fix for the bug
    conditions: {
      ArnEquals: {
        "ecs:cluster": ecsCluster.clusterArn,
      },
    },
  })
);

This forces the :* on the imported task def, as you can see in this synthed template:

- Action: ecs:RunTask
  Condition:
    ArnEquals:
      ecs:cluster:
        Fn::GetAtt:
          - EcsCluster97242B84
          - Arn
  Effect: Allow
  Resource:
    Fn::Join:
      - ""
      - - Fn::ImportValue: PgDump-16-TaskDefArn
        - :*

Acknowledgements

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

CDK version used

2.172.0

Environment details (OS name and version, etc.)

macOS

@blimmer blimmer added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 11, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Dec 11, 2024
@ashishdhingra ashishdhingra self-assigned this Dec 12, 2024
@ashishdhingra ashishdhingra added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 12, 2024
@ashishdhingra
Copy link
Contributor

@blimmer Good afternoon. Thanks for reporting the issue. Please confirm if this is the extension of #30390 where for imported value of TaskDefinitionArn, :* is not included for ecs:RunTask action.

Issue is reproducible using your example code. After replacing const pgDumpTaskDefArn = Fn.importValue(PgDump-${clusterMajorVersion}-TaskDefArn); in your code to const pgDumpTaskDefArn = 'arn:aws:ecs:us-west-2:12345678910:task-definition/PgDump-16', it produced the correct Resource: arn:aws:ecs:us-west-2:12345678910:task-definition/PgDump-16:* for ecs:runTask action.

In PR #31615 which fixed the other issue #30390, there is a check here in createEventRolePolicyStatements() which only appends revision if taskDefinitionArn token is resolved.

@ashishdhingra ashishdhingra added effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Dec 12, 2024
@ashishdhingra ashishdhingra removed their assignment Dec 12, 2024
@blimmer
Copy link
Contributor Author

blimmer commented Dec 12, 2024

Correct - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

2 participants