search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.38k stars 3.78k forks source link

ec2: `grantAttachVolume` results in circular dependency #29298

Open cheruvian opened 4 months ago

cheruvian commented 4 months ago

Describe the bug

When creating an ec2 instance and then granting attach volume, it results in a circular dependency because it tries to add to the role's default policy which the instance depends on, and then the grant depends on the instance to include the instance id in the policy.

Expected Behavior

It should not result in a circular dependency

Current Behavior

 ❌  cdk-tst failed: Error [ValidationError]: Circular dependency between resources: [CDKTstInstanceInstanceRoleDefaultPolicyD5E89317, CDKTstInstance193C36F8]
    at Request.extractError (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:46692)
    at Request.callListeners (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:91437)
    at Request.emit (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:90885)
    at Request.emit (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:199281)
    at Request.transition (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:192833)
    at AcceptorStateMachine.runTo (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:157705)
    at /workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:158035
    at Request.<anonymous> (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:193125)
    at Request.<anonymous> (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:199356)
    at Request.callListeners (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:91605) {
  code: 'ValidationError',
  time: 2024-02-28T15:55:36.619Z,
  requestId: 'eaee1eeb-528e-4f65-a48d-e5eb3620400c',
  statusCode: 400,
  retryable: false,
  retryDelay: 351.6214032309648
}

 ❌ Deployment failed: Error [ValidationError]: Circular dependency between resources: [CDKTstInstanceInstanceRoleDefaultPolicyD5E89317, CDKTstInstance193C36F8]
    at Request.extractError (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:46692)
    at Request.callListeners (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:91437)
    at Request.emit (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:90885)
    at Request.emit (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:199281)
    at Request.transition (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:192833)
    at AcceptorStateMachine.runTo (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:157705)
    at /workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:158035
    at Request.<anonymous> (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:193125)
    at Request.<anonymous> (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:199356)
    at Request.callListeners (/workplace/cdk-tst/node_modules/aws-cdk/lib/index.js:376:91605) {
  code: 'ValidationError',
  time: 2024-02-28T15:55:36.619Z,
  requestId: 'eaee1eeb-528e-4f65-a48d-e5eb3620400c',
  statusCode: 400,
  retryable: false,
  retryDelay: 351.6214032309648
}

Circular dependency between resources: [CDKTstInstanceInstanceRoleDefaultPolicyD5E89317, CDKTstInstance193C36F8]```

### Reproduction Steps

const vpc = new ec2.Vpc(this, 'VPC');
const ec2Instance = new ec2.Instance(
  this,
  'Instance',
  {
    vpc,
    instanceType: ec2.InstanceType.of(
      ec2.InstanceClass.T3,
      ec2.InstanceSize.MICRO
    ),
    machineImage: new ec2.AmazonLinuxImage({
      generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
    }),
  }
);
const volume = new ec2.Volume(this, `volume`, {
  size: Size.gibibytes(8),
  availabilityZone: vpc.selectSubnets().subnets[0].availabilityZone
});

volume.grantAttachVolume(ec2Instance.role, [ec2Instance]);
volume.applyRemovalPolicy(RemovalPolicy.DESTROY);


### Possible Solution

Creating a new policy and attaching it to the role should remove the circular dependency since the new policy will depend on both the role and the instance.

### Additional Information/Context

_No response_

### CDK CLI Version

2.126.0

### Framework Version

_No response_

### Node.js Version

v20.10.0

### OS

OSX

### Language

TypeScript

### Language Version

_No response_

### Other information

_No response_
cheruvian commented 4 months ago

I should also mention (let me know if this should be a separate issue) using the instance.availabilityZone results in another circular dependency since the availabilityZone uses a CF attribute.

Change above to repro:

    const volume = new ec2.Volume(this, `volume`, {
      size: Size.gibibytes(8),
      availabilityZone: ec2Instance.instanceAvailabilityZone
    });
 ❌ Deployment failed: Error [ValidationError]: Circular dependency between resources: [InstanceInstanceRoleDefaultPolicy4ACE9290, InstanceC1063A87, volume438BCE83]

This is a little bit less obvious of a solution, but AWS should consider setting this to the resolved AZ (and other props) rather than the instance attribute:

https://github.com/aws/aws-cdk/blob/v2.130.0/packages/aws-cdk-lib/aws-ec2/lib/instance.ts#L479

    // current
    this.instanceAvailabilityZone = this.instance.attrAvailabilityZone;
    /// tentative proposal
    this.instanceAvailabilityZone = subnet.availabilityZone; // comes from props or first subnet in vpc and what is used for the underlying CfnInstance constructor

The underlying attribute would still be accessible from this.instance.attrAvailabilityZone but would not cause any circular dependencies. Unclear if this would be considered a breaking change or not.

pahud commented 4 months ago

Looks like we have to break this circular dependency but I have so solution off the top of my head:

instance -> role -> policy -> instance

According to the document, we probably should not specify the instance ID but the condition instead.

Being said, I guess you will need to create the policy like this and optionally apply your own condition like this:

ec2Instance.addToRolePolicy(new iam.PolicyStatement({
    actions: ['ec2:AttachVolume'],
    resources: [
        `arn:${stack.partition}:ec2:${stack.region}:${stack.account}:volume/${volume.volumeId}`,
            ],
}));

ec2Instance.addToRolePolicy(new iam.PolicyStatement({
    actions: ['ec2:AttachVolume'],
    resources: [
        `arn:${stack.partition}:ec2:${stack.region}:${stack.account}:instance/*`,
    ],
    conditions: {
        "StringEquals": {"aws:ResourceTag/Department": "Development"}
    },
}));
cheruvian commented 4 months ago

Sorry I might be missing something but I think creating a separate policy would break the dependency loop

                ->  instance -> role
             /               /
policy ----------------------

The role doesn't actually need to depend on the policy it just happens in the way this is implemented (default inline policy on role) if we create a separate Policy then it can reference both the role and the instance

This is to make it super obvious but the instanceRole can also get create in the instance code and referenced as a property since it's not actually a CF Attr of the underlying CF resource


    const role = new iam.Role(this, 'aws-storage-performance-role', {
      assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
    });
    const ec2Instance = new ec2.Instance(
      this,
      'CDKRepro',
      {
        vpc,
        role,
        securityGroup,
        instanceType: ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MICRO),
        machineImage: new ec2.AmazonLinuxImage({
          generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2,
        }),
      }
    );
    new Policy(this, 'policy', {
      statements: [
        new iam.PolicyStatement({
          actions: ['thing:*'],
          resources: [`PREFIX/${ec2Instance.instanceId}`],
        }),
      ],
      roles: [role],
    })