Add source & target details to CDK #949

mikaylathompson · 2024-09-11T06:40:31Z

Description

What it does:
Provides a mechanism for users to specify source & target cluster auth and version. Right now, the version is passed around internally but not added to any ECS commands or put into the services.yaml.

For the auth: it is passed around to all the necessary places and it is added to the services.yaml.

I focused on creating clear usage for the public case -- BYO Clusters -- at the expense of some overlapping fields in the non-public use case (creating a target cluster). I tried to integrate error checks to make it clear when two overlapping fields were being provided, so those cases should be clear to internal users.

Issues Resolved

MIGRATIONS-1909

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

New functionality includes testing
- All tests pass, including unit test, integration test and doctest
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mikayla Thompson <[email protected]>

codecov · 2024-09-11T06:42:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.62%. Comparing base (3396fce) to head (c1ed295).
Report is 14 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #949      +/-   ##
============================================
- Coverage     78.83%   78.62%   -0.21%     
- Complexity     2430     2520      +90     
============================================
  Files           372      387      +15     
  Lines         14489    15012     +523     
  Branches        875      923      +48     
============================================
+ Hits          11422    11803     +381     
- Misses         2531     2644     +113     
- Partials        536      565      +29

Flag	Coverage Δ
gradle-test	`75.14% <ø> (-0.12%)`	⬇️
python-test	`88.63% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

deployment/cdk/opensearch-service-migration/lib/migration-services-yaml.ts

AndreKurait · 2024-09-11T13:26:51Z

deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

        const osContainerServiceEnabled = this.getContextForType('osContainerServiceEnabled', 'boolean', defaultValues, contextJSON)
        const otelCollectorEnabled = this.getContextForType('otelCollectorEnabled', 'boolean', defaultValues, contextJSON)
        const reindexFromSnapshotServiceEnabled = this.getContextForType('reindexFromSnapshotServiceEnabled', 'boolean', defaultValues, contextJSON)
        const reindexFromSnapshotExtraArgs = this.getContextForType('reindexFromSnapshotExtraArgs', 'string', defaultValues, contextJSON)
        const albAcmCertArn = this.getContextForType('albAcmCertArn', 'string', defaultValues, contextJSON);

+        const sourceClusterDisabled = this.getContextForType('sourceClusterDisabled', 'boolean', defaultValues, contextJSON)


These are all top level fields, what would be the level of effort to consolidate it into an object/nested object like we have in sourceClusterAuth.

e.g.

{ "sourceCluster": { "auth": { "type": "sigv4" "region": "us-west-2" "serviceSigningName": "es" }, "endpoint": "https://source-cluster:9200", "version" : "ES_7.10" "disabled": false // optional/inferred }

Maybe something like this for parsing:

interface BasicAuth { type: 'basic'; username: string; password?: string; passwordFromArn?: string; } interface SigV4Auth { type: 'sigv4'; region: string; serviceSigningName: string; } interface NoAuth { type: 'none'; } // Union type for all auth types type Auth = BasicAuth | SigV4Auth | NoAuth; // Function to parse and validate auth object function parseAuth(json: any): Auth | null { if (json.type === 'basic' && typeof json.username === 'string' && (typeof json.password === 'string' || typeof json.passwordFromArn === 'string') && !(typeof json.password === 'string' && typeof json.passwordFromArn === 'string')) { return json as BasicAuth; } else if (json.type === 'sigv4' && typeof json.region === 'string' && typeof json.serviceSigningName === 'string') { return json as SigV4Auth; } else if (json.type === 'none') { return json as NoAuth; } else { return null; // Invalid auth type } }

Mikayla's comments:

I actually went the opposite way of having a single nested object because of your feedback in the previous version of this PR, but I'm happy to flip that. I like the single object more, and I like keeping the format here consistent with the services.yaml (but json).

Down the line, I'd love to pull all of the generate-a-target-cluster fields into a single object as well so it become super clear which approach is being used and which parameters contraindicate each other.

Offline Discussion

Discussed how we can address both concerns with keeping this as a single object while not immediately removing the flattened fields from the cdk.context.json to retain some compatibility with both ways of specifying for a short period.

lewijacn · 2024-09-11T14:35:38Z

deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

+            throw new Error("The `engineVersion` can only be used when a domain is being provisioned by this tooling, which is contraindicated " +
+                "by the `targetClusterEndpoint` being provided.")
+        }
+        const targetVersion = this.getEngineVersion(targetClusterEndpoint ? targetClusterVersion : engineVersion)


Using getEngineVersion will probably be limiting for newer releases of opensearch or unsupported versions of elasticsearch: https://docs.aws.amazon.com/cdk/api/v2/java/software/amazon/awscdk/services/opensearchservice/EngineVersion.html. We may just want to enforce a format here for now

deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

lewijacn · 2024-09-11T14:48:47Z

deployment/cdk/opensearch-service-migration/lib/service-stacks/migration-console-stack.ts

@@ -151,10 +151,6 @@ export class MigrationConsoleStack extends MigrationServiceCore {
            ...props,
            parameter: MigrationSSMParameter.OS_CLUSTER_ENDPOINT,
        });
-        const sourceClusterEndpoint = props.sourceClusterDisabled ? null : getMigrationStringParameterValue(this, {


General observation: Whenever we make the change to use the source cluster details that are provided by the user in constructing the commands for RFS,Fetch,Capture Proxy, etc. it seems like we can remove this SSM Parameter from being needed at all

Yes, I'm hoping so! I'll do a more comprehensive look at whether we can pull it out of all of them.

deployment/cdk/opensearch-service-migration/lib/migration-services-yaml.ts

deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

mikaylathompson · 2024-09-11T15:13:56Z

deployment/cdk/opensearch-service-migration/lib/service-stacks/migration-console-stack.ts

@@ -151,10 +151,6 @@ export class MigrationConsoleStack extends MigrationServiceCore {
            ...props,
            parameter: MigrationSSMParameter.OS_CLUSTER_ENDPOINT,
        });
-        const sourceClusterEndpoint = props.sourceClusterDisabled ? null : getMigrationStringParameterValue(this, {


Yes, I'm hoping so! I'll do a more comprehensive look at whether we can pull it out of all of them.

Signed-off-by: Mikayla Thompson <[email protected]>

deployment/cdk/opensearch-service-migration/lib/migration-services-yaml.ts

deployment/cdk/opensearch-service-migration/lib/service-stacks/reindex-from-snapshot-stack.ts

AndreKurait · 2024-09-12T02:55:15Z

deployment/cdk/opensearch-service-migration/lib/service-stacks/traffic-replayer-stack.ts

-            });
-            replayerCommand = replayerCommand.concat(` --auth-header-user-and-secret ${osUserAndSecret}`)
+        if (props.clusterAuthDetails.basicAuth) {
+            replayerCommand = replayerCommand.concat(` --auth-header-user-and-secret ${props.clusterAuthDetails.basicAuth.username} ${props.clusterAuthDetails.basicAuth.password_from_secret_arn}`)


This happened automatically with the string parameter, but we need to add " around the value here for it to work

Good catch, thanks

AndreKurait · 2024-09-12T03:06:01Z

deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

-        } else if (targetClusterEndpoint || osContainerServiceEnabled) {
-            targetEndpoint = targetClusterEndpoint ? targetClusterEndpoint : "https://opensearch:9200"
+        let preexistingOrContainerTargetEndpoint
+        if (targetCluster && osContainerServiceEnabled) {


nit: reads strange that this block specifies targetCluster, but then uses targetClusterEndpoint instead of targetCluster?.endpoint

deployment/cdk/opensearch-service-migration/lib/stack-composer.ts

Signed-off-by: Mikayla Thompson <[email protected]>

deployment/cdk/opensearch-service-migration/lib/service-stacks/reindex-from-snapshot-stack.ts

deployment/cdk/opensearch-service-migration/options.md

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson added 2 commits September 10, 2024 23:38

Refactor auth handling throughout stack

bfe0510

Signed-off-by: Mikayla Thompson <[email protected]>

Tests & fixes

44a737f

Signed-off-by: Mikayla Thompson <[email protected]>

AndreKurait reviewed Sep 11, 2024

View reviewed changes

lewijacn reviewed Sep 11, 2024

View reviewed changes

mikaylathompson commented Sep 11, 2024

View reviewed changes

mikaylathompson added 4 commits September 11, 2024 14:10

Lots of updates re: review + team discussion

3563325

Signed-off-by: Mikayla Thompson <[email protected]>

Test and bug fixes

db85d36

Signed-off-by: Mikayla Thompson <[email protected]>

Add sigv4 to RFS command

bf7e583

Signed-off-by: Mikayla Thompson <[email protected]>

Add sigv4 to replayer command

66a88f8

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson requested review from lewijacn and AndreKurait September 11, 2024 23:13

AndreKurait reviewed Sep 12, 2024

View reviewed changes

deployment/cdk/opensearch-service-migration/lib/migration-services-yaml.ts Show resolved Hide resolved

AndreKurait reviewed Sep 12, 2024

View reviewed changes

deployment/cdk/opensearch-service-migration/lib/service-stacks/reindex-from-snapshot-stack.ts Outdated Show resolved Hide resolved

AndreKurait requested changes Sep 12, 2024

View reviewed changes

opensearch-project deleted a comment from mikaylathompson Sep 12, 2024

mikaylathompson added 2 commits September 12, 2024 00:45

Review comments

3e72f42

Signed-off-by: Mikayla Thompson <[email protected]>

Add documentation

7f7b686

Signed-off-by: Mikayla Thompson <[email protected]>

mikaylathompson force-pushed the source-target-details-in-cdk branch from a145cb6 to 7f7b686 Compare September 12, 2024 07:21

mikaylathompson marked this pull request as ready for review September 12, 2024 07:22

mikaylathompson requested review from chelma, gregschohn, okhasawn, peternied and sumobrian as code owners September 12, 2024 07:22

AndreKurait reviewed Sep 12, 2024

View reviewed changes

deployment/cdk/opensearch-service-migration/lib/service-stacks/reindex-from-snapshot-stack.ts Show resolved Hide resolved

AndreKurait reviewed Sep 12, 2024

View reviewed changes

Update documentation from review

c1ed295

Signed-off-by: Mikayla Thompson <[email protected]>

AndreKurait approved these changes Sep 12, 2024

View reviewed changes

mikaylathompson merged commit efb9ceb into opensearch-project:main Sep 12, 2024
15 checks passed

mikaylathompson deleted the source-target-details-in-cdk branch September 12, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source & target details to CDK #949

Add source & target details to CDK #949

mikaylathompson commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

AndreKurait Sep 11, 2024

AndreKurait Sep 11, 2024 •

edited

Loading

AndreKurait Sep 11, 2024 •

edited

Loading

lewijacn Sep 11, 2024

lewijacn Sep 11, 2024

mikaylathompson Sep 11, 2024

mikaylathompson Sep 11, 2024

AndreKurait Sep 12, 2024

mikaylathompson Sep 12, 2024

AndreKurait Sep 12, 2024

Add source & target details to CDK #949

Add source & target details to CDK #949

Conversation

mikaylathompson commented Sep 11, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AndreKurait Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

AndreKurait Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaylathompson commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

AndreKurait Sep 11, 2024 •

edited

Loading

AndreKurait Sep 11, 2024 •

edited

Loading