Handle External ID SSM v1.6.1> #630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nikpodsh
reviewed
Aug 4, 2023
| ssm_client = session.client('ssm', region_name=region) | ||
| parameter_value = ssm_client.get_parameter(Name=parameter_path)['Parameter']['Value'] | ||
| return parameter_value | ||
| except: | ||
| try: |
There was a problem hiding this comment.
The comment to this method should be updated accordingly
There was a problem hiding this comment.
good call out just updated
nikpodsh
reviewed
Aug 4, 2023
| raise Exception | ||
| return secret_value | ||
| except: | ||
| return False |
There was a problem hiding this comment.
Are we expecting a new minor version release for this change?
Since we break backward compatibility by removing this block, I think we should add Run the upgrade to v1.6.1 to the release notes.
Alternatively, I can propose that we keep this block for main and remove it in v2.0.0. We are anyway going to fetch this change for modularization-main and v2.0.0 requires upgrade to v1.6 for old deployments (can change to v1.6.1)
nikpodsh
approved these changes
Aug 7, 2023
|
Hi @noah-paige I agree with the above. Keeping the secret has proved to be as cumbersome as manually storing the secret in SSM. We will need to update the docs accordingly. Just to confirm, this is an issue that only affects those customers with manual created pivot roles right? |
Customers with auto created pivot roles will have the trust policies of the pivotRole updated with the new externalID when the stacks get updated on scheduled basis or via CodePipeline execution so this issue will resolve for them without any manual intervention Your are correct - this is mostly an issue that affects those customers with manual created pivot roles |
nikpodsh
added a commit
that referenced
this pull request
Aug 16, 2023
Merge latest changes from main into modularization-main It includes changes from #626, #630, #648, #649, and #651 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dlpzx <[email protected]> Co-authored-by: wolanlu <[email protected]> Co-authored-by: Amr Saber <[email protected]> Co-authored-by: Noah Paige <[email protected]> Co-authored-by: kukushking <[email protected]> Co-authored-by: Dariusz Osiennik <[email protected]> Co-authored-by: Dennis Goldner <[email protected]> Co-authored-by: Abdulrahman Kaitoua <[email protected]> Co-authored-by: akaitoua-sa <[email protected]> Co-authored-by: Gezim Musliaj <[email protected]> Co-authored-by: Rick Bernotas <[email protected]> Co-authored-by: David Mutune Kimengu <[email protected]> Co-authored-by: chamcca <[email protected]> Co-authored-by: Dhruba <[email protected]> Co-authored-by: dbalintx <[email protected]> Co-authored-by: Srinivas Reddy <[email protected]> Co-authored-by: mourya-33 <[email protected]> Co-authored-by: Noah Paige <[email protected]> Co-authored-by: dlpzx <[email protected]>
noah-paige
added a commit
that referenced
this pull request
Jun 25, 2024
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature or 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.
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
synthbecause IAM permissions out of the box do not allow said actionsThis 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
dataall-externalId-{envname}) from version <= 1.5X/dataall/{envname}/pivotRole/externalId") with the original value for external IDBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.