Byo vpc mlstudio #894

noah-paige · 2023-11-28T22:36:05Z

Feature or Bugfix

Feature

Detail

Enable SageMaker Studio Domain to be deployed in a already provisioned VPC

Relates

Allow for bring-your-own VPC for SageMaker domains #795

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume?
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features?
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users?
- Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

noah-paige · 2023-12-04T20:48:53Z

noah-paige · 2023-12-05T17:13:45Z

noah-paige · 2023-12-05T17:18:55Z

Opening up this PR for review with the following questions/enhancements in mind:

Questions:

Should only the Env Admin be able to see ML Studio Tab in Environment or should any group in environment be able to see it? (Currently, only Env Admin can see details on ML Studio Tab since they are the only group who can enable/disable mlstudio env feature)
What is the best location to store ML Studio Domain APIs (create, get, delete)? Should this be a new MLStudio folder in services/graphql/...? (Currently ML Studio Domain APIs are in Environment folder since domains only created/deleted by enabling env feature flag and domain information only shown on env tab)

Enhancements:

How best to update the sagemaker studio domain record for the environment with the domain id that is auto-generated by CDK/Cloudformation after the env stack is done creating/updating for the domain?

I think the enhancement could be implemented at a later date so can think on the best approach but open to ideas

backend/dataall/core/environment/api/input_types.py

backend/dataall/core/environment/api/resolvers.py

backend/dataall/core/environment/aws/ec2_client.py

frontend/src/modules/Environments/services/createMLStudioDomain.js

…te Domain on delete env

…ronment create / delete / update

noah-paige · 2023-12-07T00:18:25Z

dlpzx · 2023-12-07T08:00:11Z

backend/dataall/modules/mlstudio/db/mlstudio_repositories.py

Love this change, it was my main concern with the PR

The final implementation is way cleaner and more elegant :) :)

dlpzx · 2023-12-07T09:30:30Z

dlpzx · 2023-12-07T09:36:43Z

frontend/src/modules/Environments/components/EnvironmentMLStudio.js

+                created={mlStudioDomain.created}
+              />
+            </Grid>
+          </Grid>


The ObjectMetadata component looks a bit weird in the frontend, was it intentional?

I was just wanting to add all relevant information we could about domain - however the ObjectMetadata is esentially the same as the ObjectMetadata of the Environment in the Overview Tab - so I can remove and just keep ML Studio Information if you think that is cleaner from UX side?

I would remove it, first because it might lead to errors on users thinking that the domain was created at the environment creation time. And also because we need to do some tweaking for it to look good on all screen sizes

dlpzx · 2023-12-07T09:43:24Z

…LStudio View

noah-paige · 2023-12-07T16:14:35Z

Finishing up these final 3 points and additional testing:

Add SamlGroupName column to Studio Domain
Remove ObjectMetaData from EnvironmentMLStudioView
Edit update_env() in sagemaker studio domain to set vpcType, vpcId (if possible) and subnetIds (if possible)
- I edited update_env() in sagemaker studio domain to set vpc information but I kept the extension stack as is because of the chance where a user calls update_stack() without first editing the environment... update_env() only runs on edits to the environment
- Then in the case above an environment with mlstudio domain vpcType of unknown would try to create a new VPC and new Domain and would fail unless they first edit the stack to have the domain vpc information populated (if imported or default)
- Also after far too much trial and error I discovered that specifying the subnets from the RDS table (i.e. domain.subnetIds) verse specifying the subnets from cdk VPC lookup (i.e. subnet.subnet_id for subnet in vpc.private_subnets) will cause Domain to recreate EVEN IF the subnets are the exact same 😞
- Meaning we will likely still have to make a VPC boto3 call in extension stack to check if default vpc exists and go from there

dlpzx · 2023-12-08T07:03:56Z

Finishing up these final 3 points and additional testing:

* [x]  Add `SamlGroupName` column to Studio Domain

* [x]  Remove `ObjectMetaData` from `EnvironmentMLStudioView`

* [x]  Edit `update_env()` in sagemaker studio domain to set `vpcType`, `vpcId` (if possible) and `subnetIds` (if possible)
  
  * I edited  `update_env()` in sagemaker studio domain to set vpc information but I kept the extension stack as is because of the chance where a user calls `update_stack()` without first editing the environment... `update_env()` only runs on edits to the environment
  * Then in the case above an environment with mlstudio domain vpcType of `unknown` would try to create a new VPC and new Domain and would fail unless they first edit the stack to have the domain vpc information populated (if imported or default)
  * Also after far too much trial and error I discovered that specifying the subnets from the RDS table (i.e. `domain.subnetIds`) verse specifying the subnets from cdk VPC lookup (i.e. `subnet.subnet_id for subnet in vpc.private_subnets`) will cause Domain to recreate EVEN IF the subnets are the exact same 😞
  * Meaning we will likely still have to make a VPC boto3 call in extension stack to check if default vpc exists and go from there

Thank you! I will do a final testing but I think we are good! And regarding the last point, it might be related with some id that we are not aware of, strange. Great testing though, this is something that we would have not spotted by reading the documentation. So it seems like we will keep the tests in all places :) but at least we will update metadata as users use the MLStudio feature more (when they do update_env)

dlpzx · 2023-12-08T08:28:31Z

Tested the latest features:

CICD pipeline runs successfully
downgraded and re-upgraded migration scripts
frontend changes
update environment updates ML Studio info
enable/disable ML Studio works as expected

Approving!

dlpzx

looks great!

### Feature or Bugfix  - Bugfix ### Detail - When deploying from scratch - the migration script for byo vpc for mlstudio had a line of code `if has_table('sagemaker_studio_domain')` which was evaluating to `False` and skipping necessary migration code - Removed the `if has_table()` and just throw Exception in try/catch block for upgrade migration script - Also as part of this PR I made sure the security group gets created in the mlstudio_extension stack no matter what type of VPC we use for ML Studio Domain - Without the Security Group VpcOnly Deployments of ML Studio Domain with `security_group=[]` will never be able to connect to internet without manual changes - Tested with Imported VPC and provisioned Studio User Profile - can access internet via NAT Gateway as prescribed with VPCOnly deployments if internet access is required - Tested changes to ML Studio Domain Security Group does NOT cause new creation of Domain (only UpdateDomain no replacement) --> Environment Stack Updates Successfully ### Relates - #894 ### Security N/A Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

noah-paige added 4 commits November 21, 2023 11:54

Start work on byo vpc ml studio

045f701

Add Payload

86de123

Add backend logic deploy mlstudio custom vpc

866548c

lint

b717f25

noah-paige linked an issue Nov 29, 2023 that may be closed by this pull request

Allow for bring-your-own VPC for SageMaker domains #795

Closed

noah-paige added 13 commits November 29, 2023 09:53

Merge branch 'os-main' into byo-vpc-mlstudio

f110930

Test new ML Studio Domain Views

0b2d6a7

Test new ML Studio Domain Views - add missing files

dc8a412

Test new ML Studio Domain Views - add missing files

973386a

Write backend resolvers for ml studio domain apis

2de32a4

Fixes to API params and permissions checks

9212012

Add Stack Deploy on create/delete studio domain

56099d6

Fix Migration script and clean up naming and lint checks

ecb9be0

Create Studio Domain on Env Create/Update, rework Frontend Views

3280a05

Remove unused API list domains

6909b9d

add tests mlstudio domain apis

50a90b3

Clean Up

4eb4858

Clean up get studio domain and update migration script

2acfdfe

noah-paige added 4 commits December 4, 2023 15:52

Edit text when ML Studio disabled

4215ff4

Fix coverage tests

95db11b

Revert migration script

2dc0e1a

Handle null values Edit form

5cb1d8b

noah-paige marked this pull request as ready for review December 5, 2023 17:18

dlpzx reviewed Dec 6, 2023

View reviewed changes

backend/dataall/core/environment/api/input_types.py Outdated Show resolved Hide resolved

dlpzx reviewed Dec 6, 2023

View reviewed changes

backend/dataall/core/environment/api/resolvers.py Show resolved Hide resolved

dlpzx reviewed Dec 6, 2023

View reviewed changes

backend/dataall/core/environment/aws/ec2_client.py Outdated Show resolved Hide resolved

dlpzx reviewed Dec 6, 2023

View reviewed changes

frontend/src/modules/Environments/services/createMLStudioDomain.js Outdated Show resolved Hide resolved

noah-paige added 8 commits December 6, 2023 10:32

Move EC2 to base, clean up unused code, move APIs to shared, add dele…

f9e522d

…te Domain on delete env

Move EC2 to base, clean up unused code, move APIs to shared, add dele…

cec2afe

…te Domain on delete env

fix import paths

47241c6

Fix tests patch

c50e345

delete domain before deleting env

f2a8350

Add delete ml studio domain as part of delete env backend env resource

e5cc43f

Fix delete_env

3aa03b3

Change method of create / delete ml studio to not call 2 APIs on envi…

57f0e8b

…ronment create / delete / update

Clean up tests

9287d16

dlpzx reviewed Dec 7, 2023

View reviewed changes

Add default vpc info, SAML Group Name to domain, and Fix EnvironmentM…

513e691

…LStudio View

noah-paige added 7 commits December 7, 2023 11:40

Fix downgrade migration and add saml group name to models

cc77b4e

Fix tests to add samlgroupname

4bbba28

fix migration script downgrade then upgrade

cd4fe7f

Final fixes

9417ab9

Fix unknown vpc type integration tests

33b035f

lint

19c194e

only use domain RDS record on imported vpc for mlstudio extension stack

31f250a

dlpzx approved these changes Dec 8, 2023

View reviewed changes

noah-paige merged commit 94c93d9 into main Dec 8, 2023
8 checks passed

noah-paige deleted the byo-vpc-mlstudio branch December 8, 2023 13:43

noah-paige mentioned this pull request Dec 12, 2023

Fix Migration Script for New Deployment #908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Byo vpc mlstudio #894

Byo vpc mlstudio #894

noah-paige commented Nov 28, 2023

noah-paige commented Dec 4, 2023

noah-paige commented Dec 5, 2023

noah-paige commented Dec 5, 2023

noah-paige commented Dec 7, 2023

dlpzx Dec 7, 2023

dlpzx Dec 7, 2023

dlpzx commented Dec 7, 2023 •

edited

Loading

dlpzx Dec 7, 2023

noah-paige Dec 7, 2023

dlpzx Dec 7, 2023

dlpzx commented Dec 7, 2023 •

edited

Loading

noah-paige commented Dec 7, 2023 •

edited

Loading

dlpzx commented Dec 8, 2023

dlpzx commented Dec 8, 2023

dlpzx left a comment

Byo vpc mlstudio #894

Byo vpc mlstudio #894

Conversation

noah-paige commented Nov 28, 2023

Feature or Bugfix

Detail

Relates

Security

noah-paige commented Dec 4, 2023

noah-paige commented Dec 5, 2023

noah-paige commented Dec 5, 2023

noah-paige commented Dec 7, 2023

dlpzx Dec 7, 2023

Choose a reason for hiding this comment

dlpzx Dec 7, 2023

Choose a reason for hiding this comment

dlpzx commented Dec 7, 2023 • edited Loading

dlpzx Dec 7, 2023

Choose a reason for hiding this comment

noah-paige Dec 7, 2023

Choose a reason for hiding this comment

dlpzx Dec 7, 2023

Choose a reason for hiding this comment

dlpzx commented Dec 7, 2023 • edited Loading

noah-paige commented Dec 7, 2023 • edited Loading

dlpzx commented Dec 8, 2023

dlpzx commented Dec 8, 2023

dlpzx left a comment

Choose a reason for hiding this comment

dlpzx commented Dec 7, 2023 •

edited

Loading

dlpzx commented Dec 7, 2023 •

edited

Loading

noah-paige commented Dec 7, 2023 •

edited

Loading