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

get prefix list ids for dbmigration for infra region #624

Merged
merged 3 commits into from
Aug 8, 2023
Merged

get prefix list ids for dbmigration for infra region #624

merged 3 commits into from
Aug 8, 2023

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Aug 3, 2023

Feature or Bugfix

  • Bugfix

Detail

  • get the prefix id list for S3 from the infra region. We need the prefix id to connect the dbmigration stage with the S3 bucket containing the migration scripts (add it in the security groups)

Relates

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

@dlpzx
Copy link
Contributor Author

dlpzx commented Aug 3, 2023 •
edited
Loading

When testing the fix, the dbmigration nested stack gets created successfully, but another error arises:
image

From CloudTrail logs:

"User: arn:aws:sts::<INFRA-ACCOUNT>:assumed-role/cdk-hnb659fds-cfn-exec-role-<INFRA-ACCOUNT>-eu-central-1/AWSCloudFormation is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:eu-central-1:<TOOLING-ACCOUNT>:repository/pref-dep2-repository because no resource-based policy allows the ecr:BatchGetImage action",

Note that the ECR repository will be in us-east-1 not in eu-central-1, that is causing the issue. When reading the ECR repo we get another error:

"Source image xxxxxxxx.dkr.ecr.us-east-1.amazonaws.com/pref-dep2-repository:lambdas-dep2-4eeba1d1 is not valid. Provide a valid source image.",

This is because we are trying to create a Lambda in eu-central-1 from an image in us-east-1. At the moment it is not possible to deploy with the tooling in one region and the infra in another region. We can work on a fix (deploying the ECR repo in the infra region) or update the documentation

@dlpzx
Copy link
Contributor Author

dlpzx commented Aug 3, 2023

I will open a new GitHub issue to tackle the cross-region deployment issue as a whole. This PR will only fix the part of creating the CodeBuild migration stage, other PRs will be needed to allow the cross-region deployment

noah-paige
noah-paige approved these changes Aug 3, 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.

DB Migration changes look good

@dlpzx dlpzx merged commit f0a932f into data-dot-all:main Aug 8, 2023
noah-paige added a commit that referenced this pull request Jun 25, 2024
@noah-paige
Squashed commit of the following
53fc84f 
commit df87bb5a 
Author: Noah Paige <[email protected]> 
Date: Wed Aug 09 2023 13:50:41 GMT-0400 (Eastern Daylight Time) 

    Merge branch 'test2' into origin/open-source


commit 554d74e 
Author: Noah Paige <[email protected]> 
Date: Wed Aug 09 2023 12:42:19 GMT-0400 (Eastern Daylight Time) 

    Cosmetic Changes to Linking Env Frontend Steps


commit b91b157 
Author: Noah Paige <[email protected]> 
Date: Wed Aug 09 2023 13:40:45 GMT-0400 (Eastern Daylight Time) 

    Linting


commit 9b2a85b 
Author: Noah Paige <[email protected]> 
Date: Wed Aug 09 2023 11:10:12 GMT-0400 (Eastern Daylight Time) 

    Resolve S3 Permissions Nested Stack CDK Exec Role


commit e567eab 
Author: Noah Paige <[email protected]> 
Date: Wed Aug 09 2023 13:37:05 GMT-0400 (Eastern Daylight Time) 

    Glue Profiling Job Fixes


commit c678e67 
Author: Noah Paige <[email protected]> 
Date: Fri Aug 04 2023 13:27:53 GMT-0400 (Eastern Daylight Time) 

    Allow restricted nacls backend VPC (#626)

### Feature or Bugfix
- Feature


### Detail
- Extend the restricted NACLs parameter to allow for both the tooling
VPC and the backend VPC


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

commit f235c19 
Author: Noah Paige <[email protected]> 
Date: Tue Aug 08 2023 11:04:05 GMT-0400 (Eastern Daylight Time) 

    Handle External ID SSM v1.6.1> (#630)

### Feature or Bugfix
<!-- please choose -->
- Bugfix


### Detail
- As part of v1.6 Data.All moved away from storing the externalID as a
rotated secret in Secret Manager and instead placed the external ID in
SSM Parameter Store.
- In the current implementation in v1.6.1 we check if the secret exists
and the ssm parameter does not and if these conditions are met the
secret value is retrieved and a new ssm parameter is set with the same
externalID
- The problem with the above is CDK uses dynamic references to resolve
the secret value (meaning in the first upgrade deployment we set ssm
parameter as ref to secret value and delete secret, in 2nd and so one
deployments it will fail with `Secrets Manager can't find the specified
secret.`)

- Alternatively we can not use the CDK bootstrap role, such as the look
up role, and boto3 SDK commands to retrieve the secret value during
`synth` because IAM permissions out of the box do not allow said actions
- This would theoretically be a way to overcome the dynamic reference
issue mentioned above

- This PR reverts to a more straightforward approach where we create a
new SSM Parameter if one does not exist already for the external ID and
does not reference the previously created secret externalID
- NOTE: In order to keep the same externalID and prevent additional
manual work to update the pivotRole's using this value one would have to
- retain the current externalID in Secret Manager (named
`dataall-externalId-{envname}`) from version <= 1.5X
    - Run the upgrade to v1.6.1
- Replace the newly created SSM (parameter named
`/dataall/{envname}/pivotRole/externalId"`) with the original value for
external ID


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

commit f0a932f 
Author: dlpzx <[email protected]> 
Date: Tue Aug 08 2023 03:30:40 GMT-0400 (Eastern Daylight Time) 

    get prefix list ids for dbmigration for infra region (#624)

### Feature or Bugfix
- Bugfix

### Detail
- get the prefix id list for S3 from the infra region. We need the
prefix id to connect the dbmigration stage with the S3 bucket containing
the migration scripts (add it in the security groups)

### Relates
- #618 

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

commit 8900ebf 
Author: dlpzx <[email protected]> 
Date: Tue Aug 08 2023 03:30:06 GMT-0400 (Eastern Daylight Time) 

    resolve unnecessary dependency in git_release role (#623)

### Feature or Bugfix
- Bugfix

### Detail
- Remove small bug on the way we define the git release role - managed
policies are attached after role creation
- NOTE: The fix is already included in the `modularization-main` branch

### Relates
-  #617 

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Assignees

@noah-paige noah-paige

@nikpodsh nikpodsh

@dbalintx dbalintx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlpzx @noah-paige @nikpodsh @dbalintx