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.

Sign up for GitHub

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

Jump to bottom

Cdk exec policy linked envs #562

Merged
merged 10 commits into from
Jul 19, 2023
Merged

Cdk exec policy linked envs #562

merged 10 commits into from
Jul 19, 2023

Conversation

mourya-33
Copy link
Contributor

Feature

  • Feature

Detail

  • UI changes to provide CloudFormation stack Yaml file to create custom IAM policy with least privileges for bootstrapping CDK in linked environments.

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

dlpzx
dlpzx reviewed Jul 11, 2023
View reviewed changes
deploy/cdk_exec_policy/cdkExecPolicy.yaml
Action:
- 'kms:CreateAlias'
- 'kms:CreateGrant'
- 'kms:Decrypt'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need Encrypt/Decrypt permissions for this role. It is a role to create resources right?

dlpzx
dlpzx reviewed Jul 11, 2023
View reviewed changes
deploy/cdk_exec_policy/cdkExecPolicy.yaml
Effect: Allow
Resource:
- !Sub 'arn:aws:kms:${AWS::Region}:${AWS::AccountId}:alias/*'
- Sid: KMSKey
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this statement, what is it for?

dlpzx
dlpzx requested changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

It needs some changes. Plus I see that it is based out of main so it introduces conflicts in v1.6.0 branch. We either rebase to v1m6m0 or we wait to do the PR to main. What do you think?

@noah-paige
Copy link
Contributor

noah-paige commented Jul 11, 2023 •
edited
Loading

I have tested the following with the restricted CDK Execution Role:

  • Create Environment

  • Update Environment (All Play Features Enabled)

  • Delete Environment

  • Create Notebook

  • Delete Notebook

  • Create ML Studio User Profile

  • Delete ML Studio User Profile

  • Create Dataset

  • Update Dataset

  • Delete Dataset

  • Create All Pipeline Types w/ CDK Exec Policy Passed on DDK Bootstrap

  • Delete All Pipeline Types w/ CDK Exec Policy Passed on DDK Bootstrap

Comments:
For the CloudfromationTwo Statement - if my understanding is correct it appears that this statement giving the execution role access to ALL actions as long as the role is being used by CloudFormation. If so is this not very similar to adding full access to the CDK Execution role? Can we possible restrict this further?

Additionally, in my testing I had to add the following permissions to the CDK Execution Role to get all create/delete actions to work as expected:

Create Dataset
iam:TagRole
Iam:UnTagRole

Delete Dataset
Iam:DeleteRole
Iam:DetachRolePolicy
iam:DeleteRolePolicy
lambda:RemovePermission
S3:DeleteBucketPolicy

Create Pipelines
codepipeline:TagResource
codepipeline:UntagResource
codepipeline:CreatePipeline
codepipeline:UpdatePipeline
codepipeline:StartPipelineExecution
codepipeline:GetPipeline
iam:PassRole
iam:UpdateAssumeRolePolicy
codepipeline:GetPipelineState
events:PutRule
events:DescribeRule
eventsPutTargets

Delete Pipelines
events:RemoveTargets
events.DeleteRule
codepipeline:DeletePipeline
s3:DeleteBucket

Delete Environment
iam:ListPolicyVersions
iam:DeletePolicy

NOTE: For Data All Pipelines this is a little tricky since the DDK execution role permissions will be used to both set up the foundational CICD Pipeline (this is managed by data all and known) AND deploy the infra to the respective development environments (this is customized by the customer and can be extended however they see fit per their use case).

Because we will not know what a person would want to create as pipeline of their stack deployed in the pipeline should we opt to leave the default ddk bootstrap command with broader permissions? Curious on thoughts here

@dlpzx dlpzx mentioned this pull request Jul 11, 2023
Merged
@mourya-33
Copy link
Contributor Author

mourya-33 commented Jul 13, 2023 •
edited
Loading

Updated the stack to add additional required permissions and tested the following.

Create Environment
Update Environment (All Play Features Enabled)
Delete Environment
Create Notebook
Delete Notebook
Create ML Studio User Profile
Delete ML Studio User Profile
Create Dataset
Update Dataset
Delete Dataset
Dashboards
CreatePipelines

@noah-paige noah-paige self-requested a review July 17, 2023 13:40
noah-paige
noah-paige approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@noah-paige noah-paige left a comment

Choose a reason for hiding this comment

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

Re-tested creating and deleting of all data all stacks - good to merge from me

@dlpzx dlpzx self-requested a review July 19, 2023 06:21
dlpzx
dlpzx approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

Looks good. It can serve as starting point but there are a couple of possible enhacements

@dlpzx dlpzx merged commit 90dd0e5 into data-dot-all:v1m6m0 Jul 19, 2023
@dlpzx
Copy link
Contributor

dlpzx commented Jul 19, 2023

Found issues during deployment of branch V1.6.0 introduced by this PR. I am opening a separate PR for this.

@dlpzx dlpzx mentioned this pull request Jul 19, 2023
Merged
dlpzx added a commit that referenced this pull request Jul 19, 2023
@dlpzx
fix: cdk exec role deployment (#583)
67d7fb2 
### Feature or Bugfix
- Bugfix

### Detail
- Renamed resource to avoid duplicated resource names in CloudFormation

### Relates
- #562 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
dlpzx added a commit that referenced this pull request Jul 19, 2023
@dlpzx @gmuslia
@noah-paige @nikpodsh @chamcca @dbalintx @mourya-33
V1.6.0 features (#565)
84c555e 
### Feature or Bugfix
Release PR with the following list of features. Refer to each PR for the
details

### Detail
- #498 
- #482 
- #543
- #524 (which also solves #531)
- #532 
- #535 
- #497 
- #515
- #529 
- #562 
- #455 
- #572 
- #567 
- #573 
- #579 
- #578 
- #582 

### Breaking changes - release notes
- ⚠️ IMPORTANT: upgrade to a version >V1.5.0 before upgrading to V1.6 to
avoid deletion of resources in custom resource deletion
- ⚠️ IMPORTANT: requires an update of environments and then datasets
after upgrading. Either using cdk.json parameter
`enable_update_dataall_stacks_in_cicd_pipeline`, waiting for overnight
update stack task, or manually updating first environments and then
datasets
- CloudFront distribution replace for #529 
- Additional EC2 permissions in CDK Synth CodeBuild stage for #543 -->
this can be avoided by upgrading to v1.5.6 before upgrading to v1.6.0
- local development affected by more restrictive pivotRole trust policy


### Relates
V1.6.0 release

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

---------

Co-authored-by: Gezim Musliaj <102723839+gmuslia@users.noreply.github.com>
Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com>
Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com>
Co-authored-by: chamcca <40579012+chamcca@users.noreply.github.com>
Co-authored-by: Nikita Podshivalov <nikpodsh@amazon.com>
Co-authored-by: dbalintx <132444646+dbalintx@users.noreply.github.com>
Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlpzx dlpzx dlpzx approved these changes

@noah-paige noah-paige noah-paige approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mourya-33 @noah-paige @dlpzx