From bc3d154cddc4d9fb454022f57ab5642f9b432d7c Mon Sep 17 00:00:00 2001 From: dlpzx Date: Tue, 18 Apr 2023 08:29:40 +0200 Subject: [PATCH 1/5] Fix issue with duplicate resources - backwards compatibility --- backend/dataall/cdkproxy/stacks/sagemakerstudio.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py index 8eb5fc899..a6017245f 100644 --- a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py +++ b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py @@ -42,7 +42,7 @@ def check_existing_sagemaker_studio_domain(self, environment: models.Environment ) dataall_created_domain = ParameterStoreManager.client( AwsAccountId=environment.AwsAccountId, region=environment.region, role=cdk_look_up_role_arn - ).get_parameter(Name=f'/dataall/{environment.environmentUri}/sagemaker/sagemakerstudio/domain_id') + ).get_parameter(Name=f'/dataall/{environment.environmentUri}/sagemaker/sagemakerstudio/domainId') return False except ClientError as e: logger.info(f'check sagemaker studio domain created outside of data.all. Parameter data.all not found: {e}') @@ -62,7 +62,7 @@ def __init__(self, scope: Construct, construct_id: str, environment: models.Envi self, 'RoleForSagemakerStudioUsers', assumed_by=iam.ServicePrincipal('sagemaker.amazonaws.com'), - role_name='RoleSagemakerStudioUsers', + role_name='RoleForSagemakerStudioUsers', managed_policies=[ iam.ManagedPolicy.from_managed_policy_arn( self, @@ -117,7 +117,7 @@ def __init__(self, scope: Construct, construct_id: str, environment: models.Envi self, 'SagemakerStudioDomainId', string_value=sagemaker_domain.attr_domain_id, - parameter_name=f'/dataall/{self._environment.environmentUri}/sagemaker/sagemakerstudio/domain_id', + parameter_name=f'/dataall/{self._environment.environmentUri}/sagemaker/sagemakerstudio/domainId', ) From 11ddaff223209b8fac28d5dbc7f671e171f2d7ac Mon Sep 17 00:00:00 2001 From: dlpzx Date: Tue, 18 Apr 2023 09:55:01 +0200 Subject: [PATCH 2/5] Revert "Fix issue with duplicate resources - backwards compatibility" This reverts commit bc3d154cddc4d9fb454022f57ab5642f9b432d7c. --- backend/dataall/cdkproxy/stacks/sagemakerstudio.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py index a6017245f..8eb5fc899 100644 --- a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py +++ b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py @@ -42,7 +42,7 @@ def check_existing_sagemaker_studio_domain(self, environment: models.Environment ) dataall_created_domain = ParameterStoreManager.client( AwsAccountId=environment.AwsAccountId, region=environment.region, role=cdk_look_up_role_arn - ).get_parameter(Name=f'/dataall/{environment.environmentUri}/sagemaker/sagemakerstudio/domainId') + ).get_parameter(Name=f'/dataall/{environment.environmentUri}/sagemaker/sagemakerstudio/domain_id') return False except ClientError as e: logger.info(f'check sagemaker studio domain created outside of data.all. Parameter data.all not found: {e}') @@ -62,7 +62,7 @@ def __init__(self, scope: Construct, construct_id: str, environment: models.Envi self, 'RoleForSagemakerStudioUsers', assumed_by=iam.ServicePrincipal('sagemaker.amazonaws.com'), - role_name='RoleForSagemakerStudioUsers', + role_name='RoleSagemakerStudioUsers', managed_policies=[ iam.ManagedPolicy.from_managed_policy_arn( self, @@ -117,7 +117,7 @@ def __init__(self, scope: Construct, construct_id: str, environment: models.Envi self, 'SagemakerStudioDomainId', string_value=sagemaker_domain.attr_domain_id, - parameter_name=f'/dataall/{self._environment.environmentUri}/sagemaker/sagemakerstudio/domainId', + parameter_name=f'/dataall/{self._environment.environmentUri}/sagemaker/sagemakerstudio/domain_id', ) From d87e421d4406b21f1a4df71354c20a6e905bbccb Mon Sep 17 00:00:00 2001 From: dlpzx Date: Tue, 18 Apr 2023 10:15:13 +0200 Subject: [PATCH 3/5] Fix issue with duplicate resources(back to SageMaker in environment stack) - backwards compatibility --- .../dataall/cdkproxy/stacks/environment.py | 105 ++++++++++++++++-- .../cdkproxy/stacks/sagemakerstudio.py | 105 ------------------ 2 files changed, 95 insertions(+), 115 deletions(-) diff --git a/backend/dataall/cdkproxy/stacks/environment.py b/backend/dataall/cdkproxy/stacks/environment.py index 124b80329..7ef965855 100644 --- a/backend/dataall/cdkproxy/stacks/environment.py +++ b/backend/dataall/cdkproxy/stacks/environment.py @@ -7,6 +7,7 @@ from aws_cdk import ( custom_resources as cr, aws_ec2 as ec2, + aws_sagemaker as sagemaker, aws_s3 as s3, aws_s3_deployment, aws_iam as iam, @@ -26,15 +27,16 @@ Tags, ) from constructs import DependencyGroup +from botocore.exceptions import ClientError from .manager import stack from .pivot_role import PivotRole -from .sagemakerstudio import SageMakerDomain from .policies.data_policy import DataPolicy from .policies.service_policy import ServicePolicy from ... import db from ...aws.handlers.parameter_store import ParameterStoreManager from ...aws.handlers.sts import SessionHelper +from ...aws.handlers.sagemaker_studio import SagemakerStudio from ...db import models from ...utils.cdk_nag_utils import CDKNagUtil from ...utils.runtime_stacks_tagging import TagsUtil @@ -131,6 +133,28 @@ def get_all_environment_datasets(engine, environment: models.Environment) -> [mo .all() ) + def check_existing_sagemaker_studio_domain(self, environment: models.Environment): + logger.info('Check if there is an existing sagemaker studio domain in the account') + try: + logger.info('check sagemaker studio domain created as part of data.all environment stack.') + cdk_look_up_role_arn = SessionHelper.get_cdk_look_up_role_arn( + accountid=environment.AwsAccountId, region=environment.region + ) + dataall_created_domain = ParameterStoreManager.client( + AwsAccountId=environment.AwsAccountId, region=environment.region, + role=cdk_look_up_role_arn + ).get_parameter( + Name=f'/dataall/{environment.environmentUri}/sagemaker/sagemakerstudio/domain_id') + return False + except ClientError as e: + logger.info( + f'check sagemaker studio domain created outside of data.all. Parameter data.all not found: {e}') + existing_domain = SagemakerStudio.get_sagemaker_studio_domain( + AwsAccountId=environment.AwsAccountId, region=environment.region, + role=cdk_look_up_role_arn + ) + return existing_domain.get('DomainId', False) + def __init__(self, scope, id, target_uri: str = None, **kwargs): super().__init__( scope, @@ -555,8 +579,9 @@ def __init__(self, scope, id, target_uri: str = None, **kwargs): ) # Create or import SageMaker Studio domain if ML Studio enabled - if self._environment.mlStudiosEnabled: - # Create dependency group - Sagemaker depends on group IAM roles + self.sagemaker_domain_exists = self.check_existing_sagemaker_studio_domain(environment=self._environment) + if self._environment.mlStudiosEnabled and not self.sagemaker_domain_exists: + # Create dependency group - Sagemaker KMS key policy depends on group IAM roles sagemaker_dependency_group = DependencyGroup() sagemaker_dependency_group.add(default_role) for group_role in group_roles: @@ -575,13 +600,73 @@ def __init__(self, scope, id, target_uri: str = None, **kwargs): f"Default VPC not found, Exception: {e}. If you don't own a default VPC, modify the networking configuration, or disable ML Studio upon environment creation." ) - sagemaker_domain_stack = SageMakerDomain(self, 'SageMakerDomain', - environment=self._environment, - sagemaker_principals=[default_role] + group_roles, - vpc_id=vpc_id, - subnet_ids=subnet_ids - ) - sagemaker_domain_stack.node.add_dependency(sagemaker_dependency_group) + sagemaker_domain_role = iam.Role( + self, + 'RoleForSagemakerStudioUsers', + assumed_by=iam.ServicePrincipal('sagemaker.amazonaws.com'), + role_name='RoleSagemakerStudioUsers', + managed_policies=[ + iam.ManagedPolicy.from_managed_policy_arn( + self, + id='SagemakerFullAccess', + managed_policy_arn='arn:aws:iam::aws:policy/AmazonSageMakerFullAccess', + ), + iam.ManagedPolicy.from_managed_policy_arn( + self, id='S3FullAccess', + managed_policy_arn='arn:aws:iam::aws:policy/AmazonS3FullAccess' + ), + ], + ) + + sagemaker_domain_key = kms.Key( + self, + 'SagemakerDomainKmsKey', + alias='SagemakerStudioDomain', + enable_key_rotation=True, + policy=iam.PolicyDocument( + assign_sids=True, + statements=[ + iam.PolicyStatement( + resources=['*'], + effect=iam.Effect.ALLOW, + principals=[ + iam.AccountPrincipal(account_id=self._environment.AwsAccountId), + sagemaker_domain_role, + default_role, + ] + group_roles, + actions=['kms:*'], + ) + ], + ), + ) + sagemaker_domain_key.node.add_dependency(sagemaker_dependency_group) + + sagemaker_domain = sagemaker.CfnDomain( + self, + 'SagemakerStudioDomain', + domain_name=f'SagemakerStudioDomain-{self._environment.region}-{self._environment.AwsAccountId}', + auth_mode='IAM', + default_user_settings=sagemaker.CfnDomain.UserSettingsProperty( + execution_role=sagemaker_domain_role.role_arn, + security_groups=[], + sharing_settings=sagemaker.CfnDomain.SharingSettingsProperty( + notebook_output_option='Allowed', + s3_kms_key_id=sagemaker_domain_key.key_id, + s3_output_path=f's3://sagemaker-{self._environment.region}-{self._environment.AwsAccountId}', + ), + ), + vpc_id=vpc_id, + subnet_ids=subnet_ids, + app_network_access_type='VpcOnly', + kms_key_id=sagemaker_domain_key.key_id, + ) + + ssm.StringParameter( + self, + 'SagemakerStudioDomainId', + string_value=sagemaker_domain.attr_domain_id, + parameter_name=f'/dataall/{self._environment.environmentUri}/sagemaker/sagemakerstudio/domain_id', + ) # print the IAM role arn for this service account CfnOutput( diff --git a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py index 8eb5fc899..5ecf9a0fb 100644 --- a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py +++ b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py @@ -4,123 +4,18 @@ from constructs import Construct from aws_cdk import ( cloudformation_include as cfn_inc, - aws_ec2 as ec2, - aws_iam as iam, - aws_kms as kms, - aws_lambda as _lambda, - aws_sagemaker as sagemaker, - aws_ssm as ssm, - CustomResource, - Duration, - NestedStack, Stack ) -from botocore.exceptions import ClientError from .manager import stack from ... import db from ...db import models from ...db.api import Environment -from ...aws.handlers.parameter_store import ParameterStoreManager -from ...aws.handlers.sts import SessionHelper -from ...aws.handlers.sagemaker_studio import ( - SagemakerStudio, -) from ...utils.cdk_nag_utils import CDKNagUtil from ...utils.runtime_stacks_tagging import TagsUtil logger = logging.getLogger(__name__) -class SageMakerDomain(NestedStack): - - def check_existing_sagemaker_studio_domain(self, environment: models.Environment): - logger.info('Check if there is an existing sagemaker studio domain in the account') - try: - logger.info('check sagemaker studio domain created as part of data.all environment stack.') - cdk_look_up_role_arn = SessionHelper.get_cdk_look_up_role_arn( - accountid=environment.AwsAccountId, region=environment.region - ) - dataall_created_domain = ParameterStoreManager.client( - AwsAccountId=environment.AwsAccountId, region=environment.region, role=cdk_look_up_role_arn - ).get_parameter(Name=f'/dataall/{environment.environmentUri}/sagemaker/sagemakerstudio/domain_id') - return False - except ClientError as e: - logger.info(f'check sagemaker studio domain created outside of data.all. Parameter data.all not found: {e}') - existing_domain = SagemakerStudio.get_sagemaker_studio_domain( - AwsAccountId=environment.AwsAccountId, region=environment.region, role=cdk_look_up_role_arn - ) - return existing_domain.get('DomainId', False) - - def __init__(self, scope: Construct, construct_id: str, environment: models.Environment, sagemaker_principals, vpc_id, subnet_ids, **kwargs) -> None: - super().__init__(scope, construct_id, **kwargs) - - self._environment = environment - self.existing_sagemaker_domain = self.check_existing_sagemaker_studio_domain(environment=self._environment) - - if self._environment.mlStudiosEnabled and not self.existing_sagemaker_domain: - sagemaker_domain_role = iam.Role( - self, - 'RoleForSagemakerStudioUsers', - assumed_by=iam.ServicePrincipal('sagemaker.amazonaws.com'), - role_name='RoleSagemakerStudioUsers', - managed_policies=[ - iam.ManagedPolicy.from_managed_policy_arn( - self, - id='SagemakerFullAccess', - managed_policy_arn='arn:aws:iam::aws:policy/AmazonSageMakerFullAccess', - ), - iam.ManagedPolicy.from_managed_policy_arn( - self, id='S3FullAccess', managed_policy_arn='arn:aws:iam::aws:policy/AmazonS3FullAccess' - ), - ], - ) - - sagemaker_domain_key = kms.Key( - self, - 'SagemakerDomainKmsKey', - alias='SagemakerStudioDomain', - enable_key_rotation=True, - policy=iam.PolicyDocument( - assign_sids=True, - statements=[ - iam.PolicyStatement( - resources=['*'], - effect=iam.Effect.ALLOW, - principals=[iam.AccountPrincipal(account_id=self._environment.AwsAccountId), sagemaker_domain_role] + sagemaker_principals, - actions=['kms:*'], - ) - ], - ), - ) - - sagemaker_domain = sagemaker.CfnDomain( - self, - 'SagemakerStudioDomain', - domain_name=f'SagemakerStudioDomain-{self._environment.region}-{self._environment.AwsAccountId}', - auth_mode='IAM', - default_user_settings=sagemaker.CfnDomain.UserSettingsProperty( - execution_role=sagemaker_domain_role.role_arn, - security_groups=[], - sharing_settings=sagemaker.CfnDomain.SharingSettingsProperty( - notebook_output_option='Allowed', - s3_kms_key_id=sagemaker_domain_key.key_id, - s3_output_path=f's3://sagemaker-{self._environment.region}-{self._environment.AwsAccountId}', - ), - ), - vpc_id=vpc_id, - subnet_ids=subnet_ids, - app_network_access_type='VpcOnly', - kms_key_id=sagemaker_domain_key.key_id, - ) - - ssm.StringParameter( - self, - 'SagemakerStudioDomainId', - string_value=sagemaker_domain.attr_domain_id, - parameter_name=f'/dataall/{self._environment.environmentUri}/sagemaker/sagemakerstudio/domain_id', - ) - - @stack(stack='sagemakerstudiouserprofile') class SagemakerStudioUserProfile(Stack): module_name = __file__ From d928742756de0afe2ffd2c06607f6410f03f70af Mon Sep 17 00:00:00 2001 From: dlpzx Date: Tue, 18 Apr 2023 10:16:54 +0200 Subject: [PATCH 4/5] Remove unnecessary imports --- backend/dataall/cdkproxy/stacks/sagemakerstudio.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py index 5ecf9a0fb..7b66a0218 100644 --- a/backend/dataall/cdkproxy/stacks/sagemakerstudio.py +++ b/backend/dataall/cdkproxy/stacks/sagemakerstudio.py @@ -1,7 +1,5 @@ import logging import os -import pathlib -from constructs import Construct from aws_cdk import ( cloudformation_include as cfn_inc, Stack From 9604333254b84201cbef1ab18cd6937a742f4aab Mon Sep 17 00:00:00 2001 From: dlpzx Date: Tue, 18 Apr 2023 13:47:26 +0200 Subject: [PATCH 5/5] Fix issue with duplicate resources(fix testing) - backwards compatibility --- tests/cdkproxy/test_environment_stack.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cdkproxy/test_environment_stack.py b/tests/cdkproxy/test_environment_stack.py index f5dceccdf..377c6745d 100644 --- a/tests/cdkproxy/test_environment_stack.py +++ b/tests/cdkproxy/test_environment_stack.py @@ -29,7 +29,7 @@ def patch_methods(mocker, db, env, another_group, permissions): return_value=[another_group], ) mocker.patch( - 'dataall.cdkproxy.stacks.sagemakerstudio.SageMakerDomain.check_existing_sagemaker_studio_domain', + 'dataall.cdkproxy.stacks.environment.EnvironmentSetup.check_existing_sagemaker_studio_domain', return_value=True, ) mocker.patch(