From 6ca9daccff53932841e7dc53056b0f96163e7fc5 Mon Sep 17 00:00:00 2001 From: Josh Usiskin Date: Mon, 15 Nov 2021 18:21:42 +0000 Subject: [PATCH] fix(deadline): configure identity registration settings using RenderQueue backend security group Fixes #632 --- .../lib/core/lib/deployment-instance.ts | 20 ++- .../lib/core/test/deployment-instance.test.ts | 23 ++- .../aws-rfdk/lib/deadline/lib/render-queue.ts | 3 +- .../lib/deadline/lib/secrets-management.ts | 9 -- ...onfigure_identity_registration_settings.py | 78 ++++++++-- ...onfigure_identity_registration_settings.py | 146 +++++++++++++++++- .../lib/deadline/test/render-queue.test.ts | 30 ++++ .../deadline/test/secrets-management.test.ts | 11 -- 8 files changed, 278 insertions(+), 42 deletions(-) diff --git a/packages/aws-rfdk/lib/core/lib/deployment-instance.ts b/packages/aws-rfdk/lib/core/lib/deployment-instance.ts index 98103b610..69c92ffce 100644 --- a/packages/aws-rfdk/lib/core/lib/deployment-instance.ts +++ b/packages/aws-rfdk/lib/core/lib/deployment-instance.ts @@ -9,12 +9,14 @@ import { UpdatePolicy, } from '@aws-cdk/aws-autoscaling'; import { + AmazonLinuxGeneration, Connections, IConnectable, IMachineImage, InstanceClass, InstanceSize, InstanceType, + ISecurityGroup, IVpc, MachineImage, SubnetSelection, @@ -92,6 +94,13 @@ export interface DeploymentInstanceProps { */ readonly machineImage?: IMachineImage; + /** + * A security group to associate with the DeploymentInstance + * + * @default A new security group is created for the DeploymentInstance + */ + readonly securityGroup?: ISecurityGroup; + /** * Whether the instance should self-terminate after the deployment succeeds * @@ -164,17 +173,18 @@ export class DeploymentInstance extends Construct implements IScriptHost, IConne this.asg = new AutoScalingGroup(this, 'ASG', { instanceType: props.instanceType ?? InstanceType.of(InstanceClass.T3, InstanceSize.SMALL), keyName: props.keyName, - machineImage: props.machineImage ?? MachineImage.latestAmazonLinux(), - vpc: props.vpc, - vpcSubnets: props.vpcSubnets ?? { - subnetType: SubnetType.PRIVATE, - }, + machineImage: props.machineImage ?? MachineImage.latestAmazonLinux({ generation: AmazonLinuxGeneration.AMAZON_LINUX_2 }), minCapacity: 1, maxCapacity: 1, + securityGroup: props.securityGroup, signals: Signals.waitForAll({ timeout: props.executionTimeout ?? DeploymentInstance.DEFAULT_EXECUTION_TIMEOUT, }), updatePolicy: UpdatePolicy.replacingUpdate(), + vpc: props.vpc, + vpcSubnets: props.vpcSubnets ?? { + subnetType: SubnetType.PRIVATE, + }, }); this.node.defaultChild = this.asg; diff --git a/packages/aws-rfdk/lib/core/test/deployment-instance.test.ts b/packages/aws-rfdk/lib/core/test/deployment-instance.test.ts index 5e9c3bf15..803dbc817 100644 --- a/packages/aws-rfdk/lib/core/test/deployment-instance.test.ts +++ b/packages/aws-rfdk/lib/core/test/deployment-instance.test.ts @@ -13,6 +13,7 @@ import { } from '@aws-cdk/assert'; import { CfnLaunchConfiguration } from '@aws-cdk/aws-autoscaling'; import { + AmazonLinuxGeneration, AmazonLinuxImage, ExecuteFileOptions, InstanceType, @@ -167,7 +168,7 @@ describe('DeploymentInstance', () => { test('uses latest Amazon Linux machine image', () => { // GIVEN - const amazonLinux = MachineImage.latestAmazonLinux(); + const amazonLinux = MachineImage.latestAmazonLinux({ generation: AmazonLinuxGeneration.AMAZON_LINUX_2 }); const imageId: { Ref: string } = stack.resolve(amazonLinux.getImage(stack)).imageId; // THEN @@ -659,6 +660,26 @@ describe('DeploymentInstance', () => { })); }); + test('uses specified security group', () => { + // GIVEN + const securityGroupId = 'securityGroupId'; + const securityGroup = SecurityGroup.fromSecurityGroupId(depStack, 'SecurityGroup', securityGroupId); + stack = new cdk.Stack(app, 'SecurityGroupStack'); + + // WHEN + new DeploymentInstance(stack, DEFAULT_CONSTRUCT_ID, { + vpc, + securityGroup, + }); + + // THEN + expectCDK(stack).to(haveResourceLike('AWS::AutoScaling::LaunchConfiguration', { + SecurityGroups: arrayWith( + securityGroupId, + ), + })); + }); + describe('.selfTermination = false', () => { beforeAll(() => { // GIVEN diff --git a/packages/aws-rfdk/lib/deadline/lib/render-queue.ts b/packages/aws-rfdk/lib/deadline/lib/render-queue.ts index b6441c361..edcca3a3c 100644 --- a/packages/aws-rfdk/lib/deadline/lib/render-queue.ts +++ b/packages/aws-rfdk/lib/deadline/lib/render-queue.ts @@ -378,7 +378,7 @@ export class RenderQueue extends RenderQueueBase implements IGrantable { /** * Depend on this to ensure that ECS Service is stable. */ - private ecsServiceStabilized: WaitForStableService; + private readonly ecsServiceStabilized: WaitForStableService; constructor(scope: Construct, id: string, private readonly props: RenderQueueProps) { super(scope, id); @@ -973,6 +973,7 @@ export class RenderQueue extends RenderQueueBase implements IGrantable { const deploymentInstanceNode = this.node.tryFindChild(CONFIGURE_REPOSITORY_CONSTRUCT_ID); if (deploymentInstanceNode === undefined) { return new DeploymentInstance(this, CONFIGURE_REPOSITORY_CONSTRUCT_ID, { + securityGroup: this.backendConnections.securityGroups[0], vpc: this.props.vpc, vpcSubnets: this.props.vpcSubnets ?? RenderQueue.DEFAULT_VPC_SUBNETS_OTHER, }); diff --git a/packages/aws-rfdk/lib/deadline/lib/secrets-management.ts b/packages/aws-rfdk/lib/deadline/lib/secrets-management.ts index 6771d59f0..ab3a774c3 100644 --- a/packages/aws-rfdk/lib/deadline/lib/secrets-management.ts +++ b/packages/aws-rfdk/lib/deadline/lib/secrets-management.ts @@ -134,7 +134,6 @@ export class SecretsManagementIdentityRegistration extends Construct { }); // Install python dependencies - this.preparePythonEnvironment(props); const localScriptFile = this.preparePythonScript(props); this.runPythonScript(props, localScriptFile); @@ -286,12 +285,4 @@ export class SecretsManagementIdentityRegistration extends Construct { ].join(' '), ); } - - private preparePythonEnvironment(props: SecretsManagementIdentityRegistrationProps) { - // The script must run as ec2-user because Repository.configureClientInstance(...) used above will store the - // credentials in a per-user credential store that is only available to "ec2-user". - props.deploymentInstance.userData.addCommands( - 'sudo -u ec2-user python3 -m pip install --user boto3', - ); - } } diff --git a/packages/aws-rfdk/lib/deadline/scripts/python/configure_identity_registration_settings.py b/packages/aws-rfdk/lib/deadline/scripts/python/configure_identity_registration_settings.py index 567340241..bea704514 100644 --- a/packages/aws-rfdk/lib/deadline/scripts/python/configure_identity_registration_settings.py +++ b/packages/aws-rfdk/lib/deadline/scripts/python/configure_identity_registration_settings.py @@ -7,7 +7,6 @@ """ import argparse -import base64 import io import ipaddress import json @@ -19,8 +18,6 @@ from typing import Dict, Iterable, List, NamedTuple, Match -import boto3 - # Regex's for validating and splitting arguments SECRET_ARN_RE = re.compile(r''' ^ @@ -60,7 +57,7 @@ RE_CAMEL_HUMP_SUB = re.compile(r'[A-Z]?[a-z]+|[A-Z]{2,}(?=[A-Z][a-z]|\d|\W|$)|\d+|[A-Z]{2,}|[A-Z]$') # Constants for the naming convention of RFDK-managed identity registration settings -RFDK_IDENTITY_REGISTRATION_SETTING_NAME_PREFIX = f'RfdkSubnet' +RFDK_IDENTITY_REGISTRATION_SETTING_NAME_PREFIX = 'RfdkSubnet' RFDK_IDENTITY_REGISTRATION_SETTING_NAME_SEP = '|' RFDK_IDENTITY_REGISTRATION_SETTING_NAME_RE = re.compile(rf""" ^ @@ -223,29 +220,82 @@ def validate_config(config): #################################### -def fetch_secret(secret, binary=False): +def aws_cli(args: List[str]): + """ + Run an AWS CLI command and return JSON parsed output + """ + + try: + text_output = subprocess.check_output(['aws'] + args, text=True) + except subprocess.CalledProcessError as e: + raise Exception(f"failed to call AWS CLI ({e.returncode}): \n{e.stdout}\n\n{e.stderr}") from e + + try: + json_obj = json.loads(text_output) + except json.JSONDecodeError as e: + raise Exception(f"AWS CLI did not output JSON as expected ({e.msg}). Output was:\n{text_output}") from e + + return json_obj + + +def fetch_secret(secret): """ Fetch a secret from AWS :return: returns the contents of the secret """ if isinstance(secret, AwsSecret): - secrets_client = boto3.client('secretsmanager', region_name=secret.region) - secret_value = secrets_client.get_secret_value(SecretId=secret.arn) - if binary: - return base64.b64decode(secret_value.get('SecretBinary')) - else: - return secret_value.get('SecretString') + result = aws_cli([ + 'secretsmanager', + '--region', secret.region, + 'get-secret-value', + '--secret-id', secret.arn + ]) + + """ + Result will be of the form: + { + ... + "SecretString": "{\n \"username\":\"david\",\n \"password\":\"BnQw&XDWgaEeT9XGTT29\"\n}\n", + ... + } + + See https://docs.aws.amazon.com/cli/latest/reference/secretsmanager/get-secret-value.html + """ + return result['SecretString'] else: raise TypeError('Unknown Secret type.') def get_subnet_cidrs(region: str, subnet_ids: Iterable[str]) -> Dict[str, str]: - ec2 = boto3.resource('ec2', region_name=region) + filters = f'Name=subnet-id,Values={",".join(subnet_ids)}' + result = aws_cli([ + '--region', region, + 'ec2', + 'describe-subnets', + '--filters', filters + ]) + + """ + Result will be of the form: + { + "Subnets": [ + { + ..., + "CidrBlock": "172.31.80.0/20", + "SubnetId": "subnet-0bb1c79de3EXAMPLE", + ... + }, + ... + ] + } + + See https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-subnets.html + """ return { - subnet.subnet_id: subnet.cidr_block - for subnet in ec2.subnets.filter(SubnetIds=list(subnet_ids)) + subnet["SubnetId"]: subnet["CidrBlock"] + for subnet in result["Subnets"] } diff --git a/packages/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py b/packages/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py index c94f31cc8..b8873c1fb 100644 --- a/packages/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py +++ b/packages/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py @@ -6,9 +6,12 @@ Tests for configure_identity_registration_settings.py """ +import json +import re +import subprocess import sys import unittest -from unittest.mock import MagicMock, Mock +from unittest.mock import MagicMock, Mock, patch sys.modules['boto3'] = Mock() @@ -491,5 +494,146 @@ def test_delete_no_removed_settings(self): self.dl_secrets.run_str.assert_not_called() +class TestAwsInteraction(unittest.TestCase): + def test_aws_cli_success(self): + """ + Tests that the aws_cli function creates a subprocess using the supplied arguments, decodes the JSON output and + returns the result. + """ + # GIVEN + args = ['foo', 'bar'] + expected = { "foo": 1 } + return_val = json.dumps(expected) + + with patch.object(subject.subprocess, 'check_output', return_value=return_val) as check_output_mock: + # WHEN + result = subject.aws_cli(args) + + # THEN + check_output_mock.assert_called_once_with(['aws'] + args, text=True) + self.assertDictEqual(expected, result) + + def test_aws_cli_error(self): + """ + Tests that when the subprocess fails with a non-zero exit code, that the aws_cli function raises a user-friendly + error message that includes the exit code, standard output contents, and standard error contents. + """ + # GIVEN + args = ['foo', 'bar'] + exception = subprocess.CalledProcessError( + cmd=['aws'] + args, + output='output', + stderr='stderr', + returncode=-1 + ) + + with patch.object(subject.subprocess, 'check_output', side_effect=exception) as check_output_mock: + # THEN + with self.assertRaises(Exception, msg=f'failed to call AWS CLI ({exception.returncode}): \n{exception.output}\n\n{exception.stderr}') as raise_assertion: + # WHEN + subject.aws_cli(args) + + # Assert that the exception thrown from subprocess.check_output is chained to the thrown exception + chained_exception = raise_assertion.exception.__cause__ + self.assertIs(chained_exception, exception) + + # Assert that subprocess.check_output was called with the supplied arguments + check_output_mock.assert_called_once_with(['aws'] + args, text=True) + + def test_aws_cli_non_json(self): + """ + Tests that when the AWS CLI subprocess returns output that is not valid JSON, that aws_cli raises a new + exception with a more user-friendly error message. + """ + # GIVEN + args = ['foo', 'bar'] + aws_cli_output = 'not valid JSON' + + with patch.object(subject.subprocess, 'check_output', return_value=aws_cli_output): + # THEN + with self.assertRaises(Exception) as raises_assertion: + # WHEN + subject.aws_cli(args) + + msg = raises_assertion.exception.args[0] + + # Assert that the exception thrown has a chained JSONDecodeError exception + chained_exception = raises_assertion.exception.__cause__ + self.assertIsInstance(chained_exception, json.JSONDecodeError) + json_exception_msg = chained_exception.msg + + self.assertRegexpMatches(msg, f'^AWS CLI did not output JSON as expected \\({re.escape(json_exception_msg)}\\). Output was:\n{re.escape(aws_cli_output)}') + + def test_fetch_secret(self): + """ + Tests that the fetch_secret function calls the aws_cli function with expected arguments, and returns the + contents of the "SecretString" field of the returned JSON object. + """ + # GIVEN + arn = 'arn' + region = 'region' + secret_string = 'abc' + secret = { + "SecretString": secret_string + } + with patch.object(subject, 'aws_cli', return_value=secret) as mock_aws_cli: + # WHEN + result = subject.fetch_secret(subject.AwsSecret( + arn='arn', + region='region' + )) + + # THEN + self.assertEqual(secret_string, result) + mock_aws_cli.assert_called_with([ + 'secretsmanager', + '--region', region, + 'get-secret-value', + '--secret-id', arn + ]) + + def test_get_subnet_cidrs(self): + """ + Tests that the get_subnet_cidrs function calls the aws_cli function with expected arguments, and transforms the + JSON output into the expected subnet ID -> subnet CIDR mapping + """ + + # GIVEN + subnet_id_1 = 'aaa' + subnet_id_2 = 'bbb' + subnet_ids = [subnet_id_1, subnet_id_2] + subnets = [ + { + 'SubnetId': subnet_id_1, + 'CidrBlock': '1.2.3.4' + }, + { + 'SubnetId': subnet_id_2, + 'CidrBlock': '2.3.4.5' + } + ] + aws_cli_result = { + 'Subnets': subnets + } + expected_mapping = { + entry['SubnetId']: entry['CidrBlock'] + for entry in subnets + } + region = 'region' + with patch.object(subject, 'aws_cli', return_value=aws_cli_result) as mock_aws_cli: + # WHEN + result = subject.get_subnet_cidrs(region, subnet_ids) + + # THEN + self.assertEqual(result, expected_mapping) + mock_aws_cli.assert_called_with([ + '--region', region, + 'ec2', + 'describe-subnets', + '--filters', + f'Name=subnet-id,Values={",".join(subnet_ids)}' + ]) + + if __name__ == '__main__': unittest.main() diff --git a/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts b/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts index d17487374..a61abae0f 100644 --- a/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts +++ b/packages/aws-rfdk/lib/deadline/test/render-queue.test.ts @@ -2904,6 +2904,36 @@ describe('RenderQueue', () => { })); }); + test('DeploymentInstance uses specified backend security group', () => { + // GIVEN + const backendSecurityGroupId = 'backend-sg-id'; + const backendSecurityGroup = SecurityGroup.fromSecurityGroupId(stack, 'BackendSG', backendSecurityGroupId); + rqSecretsManagementProps = { + ...rqSecretsManagementProps, + securityGroups: { + backend: backendSecurityGroup, + }, + }; + + // WHEN + const renderQueue = new RenderQueue(stack, 'SecretsManagementRenderQueue', rqSecretsManagementProps); + + // THEN + // eslint-disable-next-line dot-notation + expect(renderQueue['deploymentInstance'].connections.securityGroups[0].securityGroupId).toEqual(backendSecurityGroupId); + }); + + test('DeploymentInstance uses implicitly created backend security group', () => { + // WHEN + const renderQueue = new RenderQueue(stack, 'SecretsManagementRenderQueue', rqSecretsManagementProps); + + // THEN + // eslint-disable-next-line dot-notation + expect(renderQueue['deploymentInstance'].connections.securityGroups[0]).toBe(renderQueue.backendConnections.securityGroups[0]); + // eslint-disable-next-line dot-notation + expect(renderQueue['deploymentInstance'].connections.securityGroups[0]).toBe(renderQueue.asg.connections.securityGroups[0]); + }); + describe('client calls .configureSecretsManagementAutoRegistration()', () => { let callParams: any; let clientInstance: Instance; diff --git a/packages/aws-rfdk/lib/deadline/test/secrets-management.test.ts b/packages/aws-rfdk/lib/deadline/test/secrets-management.test.ts index c6f0c3eb2..70c8774b5 100644 --- a/packages/aws-rfdk/lib/deadline/test/secrets-management.test.ts +++ b/packages/aws-rfdk/lib/deadline/test/secrets-management.test.ts @@ -249,17 +249,6 @@ describe('SecretsManagementIdentityRegistration', () => { })); }); - test('sets up Python environment', () => { - // WHEN - createTarget(); - - // THEN - // The script requires boto3 to query the subnets CIDR. This script runs - // as the ec2-user so we install this into the user's package directory - expect(deploymentInstance.userData.addCommands) - .toHaveBeenCalledWith('sudo -u ec2-user python3 -m pip install --user boto3'); - }); - test('configures direct repository connection', () => { // WHEN createTarget();