Skip to content

Commit

Permalink
fix(ecs): canContainersAccessInstanceRole is ignored when passed in A…
Browse files Browse the repository at this point in the history
…sgCapacityProvider constructor (#20522)

Fixes #20293 

When adding an AsgCapacityProvider the property `canContainersAccessInstanceRole` is only checked when passed in via the method `addAsgCapacityProvider`. It is ignored when passing the property via the instantiation of an AsgCapacityProvider. In this PR I added, that if either one way (method or constructor) has got the property set - it is respected in the outcome. For more details please see the issue #20293 

I decided **not** to omit the property on the class level because it would bring in breaking changes.

----

### 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

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [x] 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*
  • Loading branch information
daschaa authored May 27, 2022
1 parent b7bc10c commit dacefd6
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 0 deletions.
10 changes: 10 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ export class Cluster extends Resource implements ICluster {
machineImageType: provider.machineImageType,
// Don't enable the instance-draining lifecycle hook if managed termination protection is enabled
taskDrainTime: provider.enableManagedTerminationProtection ? Duration.seconds(0) : options.taskDrainTime,
canContainersAccessInstanceRole: options.canContainersAccessInstanceRole ?? provider.canContainersAccessInstanceRole,
});

this._capacityProviderNames.push(provider.capacityProviderName);
Expand Down Expand Up @@ -1109,13 +1110,22 @@ export class AsgCapacityProvider extends CoreConstruct {
*/
readonly enableManagedTerminationProtection?: boolean;

/**
* Specifies whether the containers can access the container instance role.
*
* @default false
*/
readonly canContainersAccessInstanceRole?: boolean;

constructor(scope: Construct, id: string, props: AsgCapacityProviderProps) {
super(scope, id);

this.autoScalingGroup = props.autoScalingGroup as autoscaling.AutoScalingGroup;

this.machineImageType = props.machineImageType ?? MachineImageType.AMAZON_LINUX_2;

this.canContainersAccessInstanceRole = props.canContainersAccessInstanceRole;

this.enableManagedTerminationProtection =
props.enableManagedTerminationProtection === undefined ? true : props.enableManagedTerminationProtection;

Expand Down
142 changes: 142 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/cluster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2306,3 +2306,145 @@ test('throws when ASG Capacity Provider with capacityProviderName starting with
cluster.addAsgCapacityProvider(capacityProviderAl2);
}).toThrow(/Invalid Capacity Provider Name: ecscp, If a name is specified, it cannot start with aws, ecs, or fargate./);
});

describe('Accessing container instance role', function () {

const addUserDataMock = jest.fn();
const autoScalingGroup: autoscaling.AutoScalingGroup = {
addUserData: addUserDataMock,
addToRolePolicy: jest.fn(),
protectNewInstancesFromScaleIn: jest.fn(),
} as unknown as autoscaling.AutoScalingGroup;

afterEach(() => {
addUserDataMock.mockClear();
});

test('block ecs from accessing metadata service when canContainersAccessInstanceRole not set', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

// WHEN

const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', {
autoScalingGroup: autoScalingGroup,
});

cluster.addAsgCapacityProvider(capacityProvider);

// THEN
expect(autoScalingGroup.addUserData).toHaveBeenCalledWith('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');
expect(autoScalingGroup.addUserData).toHaveBeenCalledWith('sudo service iptables save');
expect(autoScalingGroup.addUserData).toHaveBeenCalledWith('echo ECS_AWSVPC_BLOCK_IMDS=true >> /etc/ecs/ecs.config');
});

test('allow ecs accessing metadata service when canContainersAccessInstanceRole is set on addAsgCapacityProvider', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

// WHEN
const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', {
autoScalingGroup: autoScalingGroup,
});

cluster.addAsgCapacityProvider(capacityProvider, {
canContainersAccessInstanceRole: true,
});

// THEN
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo service iptables save');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('echo ECS_AWSVPC_BLOCK_IMDS=true >> /etc/ecs/ecs.config');
});

test('allow ecs accessing metadata service when canContainersAccessInstanceRole is set on AsgCapacityProvider instantiation', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

// WHEN
const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', {
autoScalingGroup: autoScalingGroup,
canContainersAccessInstanceRole: true,
});

cluster.addAsgCapacityProvider(capacityProvider);

// THEN
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo service iptables save');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('echo ECS_AWSVPC_BLOCK_IMDS=true >> /etc/ecs/ecs.config');
});

test('allow ecs accessing metadata service when canContainersAccessInstanceRole is set on constructor and method', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

// WHEN
const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', {
autoScalingGroup: autoScalingGroup,
canContainersAccessInstanceRole: true,
});

cluster.addAsgCapacityProvider(capacityProvider, {
canContainersAccessInstanceRole: true,
});

// THEN
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo service iptables save');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('echo ECS_AWSVPC_BLOCK_IMDS=true >> /etc/ecs/ecs.config');
});

test('block ecs from accessing metadata service when canContainersAccessInstanceRole set on constructor and not set on method', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

// WHEN
const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', {
autoScalingGroup: autoScalingGroup,
canContainersAccessInstanceRole: true,
});

cluster.addAsgCapacityProvider(capacityProvider, {
canContainersAccessInstanceRole: false,
});

// THEN
expect(autoScalingGroup.addUserData).toHaveBeenCalledWith('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');
expect(autoScalingGroup.addUserData).toHaveBeenCalledWith('sudo service iptables save');
expect(autoScalingGroup.addUserData).toHaveBeenCalledWith('echo ECS_AWSVPC_BLOCK_IMDS=true >> /etc/ecs/ecs.config');
});

test('allow ecs accessing metadata service when canContainersAccessInstanceRole is not set on constructor and set on method', () => {
// GIVEN
const app = new cdk.App();
const stack = new cdk.Stack(app, 'test');
const cluster = new ecs.Cluster(stack, 'EcsCluster');

// WHEN
const capacityProvider = new ecs.AsgCapacityProvider(stack, 'Provider', {
autoScalingGroup: autoScalingGroup,
canContainersAccessInstanceRole: false,
});

cluster.addAsgCapacityProvider(capacityProvider, {
canContainersAccessInstanceRole: true,
});

// THEN
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo iptables --insert FORWARD 1 --in-interface docker+ --destination 169.254.169.254/32 --jump DROP');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('sudo service iptables save');
expect(autoScalingGroup.addUserData).not.toHaveBeenCalledWith('echo ECS_AWSVPC_BLOCK_IMDS=true >> /etc/ecs/ecs.config');
});
});

0 comments on commit dacefd6

Please sign in to comment.