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

ec2: grantAttachVolume results in circular dependency #29298

Open
cheruvian opened this issue Feb 28, 2024 · 3 comments
Open

ec2: grantAttachVolume results in circular dependency #29298

cheruvian opened this issue Feb 28, 2024 · 3 comments
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@cheruvian
Copy link
Contributor

cheruvian commented Feb 28, 2024

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 cheruvian added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Feb 28, 2024
@cheruvian
Copy link
Contributor Author

cheruvian commented Feb 28, 2024

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 pahud changed the title aws-ec2: grantAttachVolume results in circular dependency ec2: grantAttachVolume results in circular dependency Feb 28, 2024
@pahud
Copy link
Contributor

pahud commented Feb 28, 2024

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"}
	},
}));

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@cheruvian
Copy link
Contributor Author

cheruvian commented Feb 29, 2024

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],
    })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

2 participants