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

fix: Delete egress role in workflow instead of API #740

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -554,38 +554,9 @@ describe('DataEgressService', () => {
lockService.tryWriteLockAndRun = jest.fn((_params, callback) => callback());
AWSMock.mock('S3', 'putBucketPolicy', putBucketPolicyMock);

mockDeleteEgressStoreRole(egressStoreId);

await dataEgressService.terminateEgressStore(requestContext, envId);
});

function mockDeleteEgressStoreRole(egressStoreId) {
const policyArn = 'test-PermissionBoundaryArn';
AWSMock.mock('IAM', 'listAttachedRolePolicies', (params, callback) => {
expect(params.RoleName).toEqual(`swb-study-${egressStoreId}`);
callback(null, {
AttachedPolicies: [
{
PolicyName: 'test-PermissionBoundaryName',
PolicyArn: policyArn,
},
],
});
});
AWSMock.mock('IAM', 'detachRolePolicy', (params, callback) => {
expect(params).toMatchObject({ RoleName: `swb-study-${egressStoreId}`, PolicyArn: policyArn });
callback();
});
AWSMock.mock('IAM', 'deleteRole', (params, callback) => {
expect(params).toMatchObject({ RoleName: `swb-study-${egressStoreId}` });
callback();
});
AWSMock.mock('IAM', 'deletePolicy', (params, callback) => {
expect(params).toMatchObject({ PolicyArn: policyArn });
callback();
});
}

it('should successfully delete egress store that is in CREATED state: deleteEgressStoreInCreatedStateTest = true', async () => {
await deleteEgressStoreInCreatedStateTest(true);
});
Expand Down Expand Up @@ -661,7 +632,6 @@ describe('DataEgressService', () => {
// Mock locking so that the putBucketPolicy actually gets called
lockService.tryWriteLockAndRun = jest.fn((_params, callback) => callback());
AWSMock.mock('S3', 'putBucketPolicy', putBucketPolicyMock);
mockDeleteEgressStoreRole(egressStoreId);

await dataEgressService.terminateEgressStore(requestContext, envId);
}
Expand Down Expand Up @@ -1236,4 +1206,37 @@ describe('DataEgressService', () => {
);
});
});

it('should successfully delete egress store role', async () => {
// BUILD
const policyArn = 'test-PermissionBoundaryArn';
AWSMock.mock('IAM', 'listAttachedRolePolicies', (params, callback) => {
expect(params.RoleName).toEqual(`swb-study-${egressStoreId}`);
callback(null, {
AttachedPolicies: [
{
PolicyName: 'test-PermissionBoundaryName',
PolicyArn: policyArn,
},
],
});
});
AWSMock.mock('IAM', 'detachRolePolicy', (params, callback) => {
expect(params).toMatchObject({ RoleName: `swb-study-${egressStoreId}`, PolicyArn: policyArn });
callback();
});
AWSMock.mock('IAM', 'deleteRole', (params, callback) => {
expect(params).toMatchObject({ RoleName: `swb-study-${egressStoreId}` });
callback();
});
AWSMock.mock('IAM', 'deletePolicy', (params, callback) => {
expect(params).toMatchObject({ PolicyArn: policyArn });
callback();
});

const egressStoreId = 'abc';

// OPERATE and CHECK
await expect(dataEgressService.deleteMainAccountEgressStoreRole(egressStoreId)).resolves.toBeUndefined();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ class DataEgressService extends Service {
true,
);
}
await this.deleteMainAccountEgressStoreRole(egressStoreInfo.id);

const egressStoreDdbLockId = `egress-store-ddb-access-${egressStoreInfo.id}`;
egressStoreInfo.status = TERMINATED_STATUS_CODE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,4 @@ describe('LaunchProduct', () => {
expect(namespaceIndex).toEqual(-1);
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

const WorkflowPayload = require('@aws-ee/workflow-engine/lib/workflow-payload');
const TerminateProduct = require('../terminate-product/terminate-product');

describe('TerminateProduct', () => {
const meta = {};
const input = {};

const tp = new TerminateProduct({
step: { config: {} },
workflowPayload: new WorkflowPayload({ meta, input, workflowInstance: {} }),
});

beforeAll(async () => {
tp.payload = {
getValue: value => {
if (value === 'envId') {
return 'abc';
}
return '';
},
};
});

describe('start', () => {
it('deleteMainAccountEgressStoreRole should be called once if Egress Store is enabled', async () => {
// BUILD
const deleteMainAccountEgressStoreRole = jest.fn();
mockDataEgressService(deleteMainAccountEgressStoreRole);
mockEnableEgressStoreSettings(true);

// OPERATE
await tp.start();

// CHECK
expect(deleteMainAccountEgressStoreRole).toHaveBeenCalledTimes(1);
});

it('deleteMainAccountEgressStoreRole should NOT be called if Egress Store is disabled', async () => {
// BUILD
const deleteMainAccountEgressStoreRole = jest.fn();
mockDataEgressService(deleteMainAccountEgressStoreRole);
mockEnableEgressStoreSettings(false);

// OPERATE
await tp.start();

// CHECK
expect(deleteMainAccountEgressStoreRole).toHaveBeenCalledTimes(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use .not.toHaveBeenCalled() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

});

function mockDataEgressService(deleteMainAccountEgressStoreRole) {
const dataEgressService = {};
dataEgressService.deleteMainAccountEgressStoreRole = deleteMainAccountEgressStoreRole;
tp.mustFindServices = jest.fn().mockImplementation(param => {
if (param === 'dataEgressService') {
return dataEgressService;
}
throw new Error('Unexpected service');
});
}

function mockEnableEgressStoreSettings(isEnabled) {
tp.settings = {
getBoolean: key => {
if (key === 'enableEgressStore') {
return isEnabled;
}
throw new Error('Unexpected key');
},
};
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const inPayloadKeys = {
};
const settingKeys = {
envMgmtRoleArn: 'envMgmtRoleArn',
enableEgressStore: 'enableEgressStore',
};

const pluginConstants = {
Expand All @@ -51,8 +52,9 @@ class TerminateProduct extends StepBase {
}

async start() {
const [provisionedProductId] = await Promise.all([
const [provisionedProductId, envId] = await Promise.all([
this.payloadOrConfig.optionalString(inPayloadKeys.provisionedProductId, ''),
this.payloadOrConfig.string(inPayloadKeys.envId),
]);

if (provisionedProductId) {
Expand All @@ -68,6 +70,12 @@ class TerminateProduct extends StepBase {
this.state.setKey('RECORD_ID', recordDetail.RecordId);
}

const enableEgressStore = this.settings.getBoolean(settingKeys.enableEgressStore);
if (enableEgressStore) {
const dataEgressService = await this.mustFindServices('dataEgressService');
await dataEgressService.deleteMainAccountEgressStoreRole(envId);
}

return (
this.wait(5) // check every 5 seconds
// keep doing it for 1*1296000 seconds = 15 days
Expand Down
17 changes: 4 additions & 13 deletions main/solution/backend/config/infra/cloudformation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -589,19 +589,6 @@ Resources:
- appstream:UpdateImagePermissions
Resource:
- !Sub 'arn:aws:appstream:${AWS::Region}:${AWS::AccountId}:image/ServiceWorkbench*'
- Effect: Allow
Action:
- iam:GetRole
- iam:DeleteRole
- iam:DetachRolePolicy
- iam:ListAttachedRolePolicies
Resource:
- !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-study-*'
- Effect: Allow
Action:
- iam:DeletePolicy
Resource:
- !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/swb-study-*'
# IAM Role for the apiHandler Function
RoleApiHandler:
Type: 'AWS::IAM::Role'
Expand Down Expand Up @@ -754,11 +741,15 @@ Resources:
- Effect: Allow
Action:
- iam:AttachRolePolicy
- iam:DeleteRole
- iam:DetachRolePolicy
- iam:ListAttachedRolePolicies
Resource:
- !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/swb-study-*'
- Effect: Allow
Action:
- iam:CreatePolicy
- iam:DeletePolicy
Resource:
- !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/swb-study-*'

Expand Down