feat: fine-grained nacls for backend vpc creation #543
Conversation
### Feature or Bugfix - Bugfix ### Detail - fix how dynamic SQL with varying table names is generated ### Relates - <URL or Ticket> By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
…ride react-redux to non-vulnerable version (#521) ### Feature or Bugfix - Bugfix ### Detail - Upgraded `fast-xml-parser` - In the process I also found that other dependency libraries included vulnerabilities. In particular `react-redux` and `nth-check`, the parent packages `aws-amplify`, `react-scripts` and `appbaseio/reactivesearch` have been updated. For this last one, the latest version still uses a vulnerable version of `react-redux` so I added a ovverride clause in the package.json ### Relates - Related to https://github.com/NaturalIntelligence/fast-xml-parser/releases/tag/v4.2.4 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
### Feature or Bugfix - Bugfix ### Detail - Resolve nth-check in sub-dependencies to version 2.0.1 ### Relates By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
| @@ -12,7 +12,7 @@ | |||
| from ....db import permissions, models | |||
| from ....db.api import ResourcePolicy, Glossary | |||
| from ....searchproxy import indexers | |||
| from ....utils import json_utils | |||
| from ....utils import json_utils, sql_utils | |||
There was a problem hiding this comment.
I see that this feature branch includes changes from main to v1.6
| def create_ecs_security_groups(self, envname, resource_prefix, vpc, vpce_connection, s3_prefix_list, lambdas): | ||
| scheduled_tasks_sg = ec2.SecurityGroup( | ||
| self, | ||
| f'ScheduledTasksSG{envname}', |
There was a problem hiding this comment.
It is for scheduled tasks and for cdkproxy tasks right? But I see why you left the name for simplicity
There was a problem hiding this comment.
Yes - originally I had cdkproxy with it's own SG but ultimately I found a way to make cdkproxy ECS tasks work with no traffic over the internet so I removed the additional cdkproxy_security_group and now use scheduled_tasks_sg for all scheduled tasks and cdkproxy while only shares are handled separately with their own SG
I am open to changing the name to something more appropriate but for simplicity I kept it as scheduled_tasks_sg for now
| @@ -1,6 +1,6 @@ | |||
| import { createContext, useEffect, useReducer } from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import Amplify, { Auth } from 'aws-amplify'; | |||
| import { Amplify, Auth } from 'aws-amplify'; | |||
deploy/stacks/pipeline.py
Outdated
| 'pip install -r deploy/requirements.txt', | ||
| 'cdk synth', | ||
| 'echo ${CODEBUILD_SOURCE_VERSION}' | ||
| ], | ||
| role_policy_statements=self.codebuild_policy, | ||
| role=self.pipeline_iam_role, |
There was a problem hiding this comment.
if we are replacing the codebuild policy by a role, shouldn't we define the codebuild_role here?
There was a problem hiding this comment.
This goes back to the previous code where we had the following configuration:
Baseline CodeBuild Policies were assigned to the following CodeBuild Steps:
- Synth
- ALL Quality Gate CodeBuild Steps
- Lambda Image
- ECS Image
Baseline CodeBuild Policies + select sts/cloudfront permissions were assigned to:
- DB Migration
- Stack Updater
- Deploy FrontEnd
- Update Documentation
- Configure RUM
- Configure Cognito
- ALBFrontEndImage
- UserGuideImage
Baseline CodeBuild Policies + sts/cloudfront + select codecommit permissions were assigned to:
- GitRelease
I created 3 roles to align with the above 3 sets of permissions, pipeline_iam_role, codebuild_role, and git_project_role keeping the permissions the same and refactoring to focus on improving management of these permission sets.
I will rename the roles to be baseline_codebuild_role, expanded_codebuild_role, and git_project_role to avoid confusion with naming.
I think an optimization here would be to define individual permission sets for each CodeBuild Role (or each logical grouping of CodeBuild Steps) but would opt to leave that as a separate PR unless others think differently?
There was a problem hiding this comment.
My bad for not looking into each role policies! I see now. And yes and additional improvement would be to define each role/set of roles separatedly. But I agree, this can be part of a different PR
|
Yes, I was thinking of having some alternative to VPC endpoints. The ones introduced in this PR are also needed only for particular features which means that can be avoided if a module is disabled in modularization. So I would wait for this change and implement it directly in the modularization architecture |
|
Hi @noah-paige! I have tested the backwards compatibility of this feature and I run into the following issue when merging this feature branch in a pre-existing deployment. What I did is adding the permission manually to the role that is created for CodeBuild based on the IAM policy(a role which will be deleted in CDK Synth and replaced by a role instead of a policy). After this it succeeds and does not throw any other error. We have 2 options:
|
| @@ -55,7 +55,12 @@ def update_stack_output(session, stack): | |||
|
|
|||
| def deploy_cdk_stack(engine: Engine, stackid: str, app_path: str = None, path: str = None): | |||
| logger.warning(f'Starting new stack from stackid {stackid}') | |||
| sts = boto3.client('sts') | |||
| region = os.getenv('AWS_REGION', 'eu-west-1') | |||
There was a problem hiding this comment.
I wonder if we can create a factory for this client to avoid copy-paste e.g.
@staticmethod
def client(additional_params: dict = None):
if additional_params is None:
additional_params = {}
region = os.getenv('AWS_REGION', 'eu-west-1')
return boto3.client(
'sts',
region_name=region,
endpoint_url=f"https://sts.{region}.amazonaws.com",
**additional_params
)
There was a problem hiding this comment.
Added a new Issue for the above #561
| @@ -56,7 +56,12 @@ def up(response: Response): | |||
| def check_creds(response: Response): | |||
| logger.info('GET /awscreds') | |||
| try: | |||
| sts = boto3.client('sts', region_name=os.getenv('AWS_REGION', 'eu-west-1')) | |||
| region = os.getenv('AWS_REGION', 'eu-west-1') | |||
There was a problem hiding this comment.
To consider: Looks like we reach the same behavior if we specify AWS_STS_REGIONAL_ENDPOINTS=regional. boto3 sts client will make a call to a regional sts endpoint. This way we don't have to worry about forgetting to put region to sts client
### 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>
Feature or Bugfix
Detail
Relates
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.