From 85d5846216002abfc8a437dc41356347d82b5a18 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 17 Oct 2024 06:41:05 -0500 Subject: [PATCH] Make `--source-version` required (#1058) * Make --source-version required By defaulting to ES 7.10 this caused confusion with the version parameter was not passed into RFS for different snapshot sources. While this does require all customers to follow the same path it will prevent strange errors when the snapshot reader is unable to parse the snapshot. Signed-off-by: Peter Nied Signed-off-by: Peter Nied Signed-off-by: Andre Kurait Co-authored-by: Andre Kurait --- .../migrations/RfsMigrateDocuments.java | 4 ++-- .../bulkload/ProcessLifecycleTest.java | 2 ++ MetadataMigration/README.md | 4 ++-- .../migrations/MigrateOrEvaluateArgs.java | 4 ++-- .../cli/ClusterReaderExtractor.java | 3 +++ .../cli/ClusterReaderExtractorTest.java | 10 +++++++++ .../cdk.context.json | 1 + .../lib/common-utilities.ts | 3 ++- .../lib/stack-composer.ts | 9 +++++++- .../opensearch-service-migration/options.md | 5 +---- .../test/common-utilities.test.ts | 2 ++ .../test/migration-services-yaml.test.ts | 9 +++++--- .../test/network-stack.test.ts | 9 +++++--- .../test/opensearch-domain-stack.test.ts | 12 +++++++---- .../test/reindex-from-snapshot-stack.test.ts | 21 ++++++++++++------- .../test/resources/sample-context-file.json | 1 + .../test/stack-composer-ordering.test.ts | 3 ++- .../test/stack-composer.test.ts | 5 +++-- 18 files changed, 74 insertions(+), 33 deletions(-) diff --git a/DocumentsFromSnapshotMigration/src/main/java/org/opensearch/migrations/RfsMigrateDocuments.java b/DocumentsFromSnapshotMigration/src/main/java/org/opensearch/migrations/RfsMigrateDocuments.java index 14039232e..c4ddac955 100644 --- a/DocumentsFromSnapshotMigration/src/main/java/org/opensearch/migrations/RfsMigrateDocuments.java +++ b/DocumentsFromSnapshotMigration/src/main/java/org/opensearch/migrations/RfsMigrateDocuments.java @@ -150,10 +150,10 @@ public static class Args { "used to communicate to the target, default 10") int maxConnections = 10; - @Parameter(required = false, + @Parameter(required = true, names = { "--source-version" }, converter = VersionConverter.class, - description = ("Optional. Version of the source cluster. Default: ES_7.10")) + description = ("Version of the source cluster.")) public Version sourceVersion = Version.fromString("ES 7.10"); } diff --git a/DocumentsFromSnapshotMigration/src/test/java/org/opensearch/migrations/bulkload/ProcessLifecycleTest.java b/DocumentsFromSnapshotMigration/src/test/java/org/opensearch/migrations/bulkload/ProcessLifecycleTest.java index 92e18aa6e..1cad68a12 100644 --- a/DocumentsFromSnapshotMigration/src/test/java/org/opensearch/migrations/bulkload/ProcessLifecycleTest.java +++ b/DocumentsFromSnapshotMigration/src/test/java/org/opensearch/migrations/bulkload/ProcessLifecycleTest.java @@ -224,6 +224,8 @@ private static ProcessBuilder setupProcess( "10", "--max-connections", "1", + "--source-version", + "ES_7_10", "--initial-lease-duration", failHow == FailHow.NEVER ? "PT10M" : "PT1S" }; diff --git a/MetadataMigration/README.md b/MetadataMigration/README.md index 1d4e4b61f..849dc0828 100644 --- a/MetadataMigration/README.md +++ b/MetadataMigration/README.md @@ -22,7 +22,7 @@ Metadata migration is a relatively fast process to execute so we recommend attem By scanning the contents of the source cluster, applying filtering, and applying modifications a list of all items that will be migrated will be created. Any items not seen in this output will not be migrated onto the target cluster if the migrate command was to be run. This is a safety check before making modifications on the target cluster. ```shell -console metadata evaluate +console metadata evaluate [...] ```
@@ -64,7 +64,7 @@ Results: Running through the same data as the evaluate command all of the migrated items will be applied onto the target cluster. If re-run multiple times items that were previously migrated will not be recreated. If any items do need to be remigrated, please delete them from the target cluster and then rerun the evaluate then migrate commands to ensure the desired changes are made. ```shell -console metadata migrate +console metadata migrate [...] ```
diff --git a/MetadataMigration/src/main/java/org/opensearch/migrations/MigrateOrEvaluateArgs.java b/MetadataMigration/src/main/java/org/opensearch/migrations/MigrateOrEvaluateArgs.java index 037a6f881..01942be40 100644 --- a/MetadataMigration/src/main/java/org/opensearch/migrations/MigrateOrEvaluateArgs.java +++ b/MetadataMigration/src/main/java/org/opensearch/migrations/MigrateOrEvaluateArgs.java @@ -50,6 +50,6 @@ public class MigrateOrEvaluateArgs { + "forwarded. If no value is provided, metrics will not be forwarded.") String otelCollectorEndpoint; - @Parameter(names = {"--source-version" }, description = "Version of the source cluster, for example: Elasticsearch 7.10 or OS 1.3. Defaults to: ES_7.10", converter = VersionConverter.class) - public Version sourceVersion = Version.fromString("ES 7.10"); + @Parameter(names = {"--source-version" }, description = "Version of the source cluster, for example: Elasticsearch 7.10 or OS 1.3.", converter = VersionConverter.class) + public Version sourceVersion = null; } diff --git a/MetadataMigration/src/main/java/org/opensearch/migrations/cli/ClusterReaderExtractor.java b/MetadataMigration/src/main/java/org/opensearch/migrations/cli/ClusterReaderExtractor.java index f389a5968..ca281036d 100644 --- a/MetadataMigration/src/main/java/org/opensearch/migrations/cli/ClusterReaderExtractor.java +++ b/MetadataMigration/src/main/java/org/opensearch/migrations/cli/ClusterReaderExtractor.java @@ -40,6 +40,9 @@ public ClusterReader extractClusterReader() { throw new ParameterException("Unable to find valid resource provider"); } + if (arguments.sourceVersion == null) { + throw new ParameterException("Unable to read from snapshot without --source-version parameter"); + } return getSnapshotReader(arguments.sourceVersion, repo); } diff --git a/MetadataMigration/src/test/java/org/opensearch/migrations/cli/ClusterReaderExtractorTest.java b/MetadataMigration/src/test/java/org/opensearch/migrations/cli/ClusterReaderExtractorTest.java index 9174cf6f6..1b22ebfb0 100644 --- a/MetadataMigration/src/test/java/org/opensearch/migrations/cli/ClusterReaderExtractorTest.java +++ b/MetadataMigration/src/test/java/org/opensearch/migrations/cli/ClusterReaderExtractorTest.java @@ -54,6 +54,16 @@ void testExtractClusterReader_invalidS3Snapshot_missingLocalDirPath() { assertThat(exception.getMessage(), equalTo("If an s3 repo is being used, s3-region and s3-local-dir-path must be set")); } + @Test + void testExtractClusterReader_validLocalSnapshot_missingVersion() { + var args = new MigrateOrEvaluateArgs(); + args.fileSystemRepoPath = "foo.bar"; + var extractor = spy(new ClusterReaderExtractor(args)); + + var exception = assertThrows(ParameterException.class, () -> extractor.extractClusterReader()); + assertThat(exception.getMessage(), equalTo("Unable to read from snapshot without --source-version parameter")); + } + @Test void testExtractClusterReader_validLocalSnapshot() { var args = new MigrateOrEvaluateArgs(); diff --git a/deployment/cdk/opensearch-service-migration/cdk.context.json b/deployment/cdk/opensearch-service-migration/cdk.context.json index 85f305f3f..e07c5aea1 100644 --- a/deployment/cdk/opensearch-service-migration/cdk.context.json +++ b/deployment/cdk/opensearch-service-migration/cdk.context.json @@ -15,6 +15,7 @@ }, "sourceCluster": { "endpoint": "", + "version": "", "auth": { "type": "none | basic | sigv4", "// basic auth documentation": "The next two lines are releavant for basic auth only", diff --git a/deployment/cdk/opensearch-service-migration/lib/common-utilities.ts b/deployment/cdk/opensearch-service-migration/lib/common-utilities.ts index 193e4e0de..5d9b7d1ed 100644 --- a/deployment/cdk/opensearch-service-migration/lib/common-utilities.ts +++ b/deployment/cdk/opensearch-service-migration/lib/common-utilities.ts @@ -5,6 +5,7 @@ import {RemovalPolicy, Stack} from "aws-cdk-lib"; import { IStringParameter, StringParameter } from "aws-cdk-lib/aws-ssm"; import * as forge from 'node-forge'; import { ClusterYaml } from "./migration-services-yaml"; +import { CdkLogger } from "./cdk-logger"; export function getSecretAccessPolicy(secretArn: string): PolicyStatement { return new PolicyStatement({ @@ -421,10 +422,10 @@ function parseAuth(json: any): ClusterAuth | null { export function parseClusterDefinition(json: any): ClusterYaml { const endpoint = json.endpoint - const version = json.version if (!endpoint) { throw new Error('Missing required field in cluster definition: endpoint') } + const version = json.version; const auth = parseAuth(json.auth) if (!auth) { throw new Error(`Invalid auth type when parsing cluster definition: ${json.auth.type}`) diff --git a/deployment/cdk/opensearch-service-migration/lib/stack-composer.ts b/deployment/cdk/opensearch-service-migration/lib/stack-composer.ts index b3902350b..806002c93 100644 --- a/deployment/cdk/opensearch-service-migration/lib/stack-composer.ts +++ b/deployment/cdk/opensearch-service-migration/lib/stack-composer.ts @@ -222,14 +222,21 @@ export class StackComposer { if (!sourceClusterDefinition && (sourceClusterEndpointField || sourceClusterDisabledField)) { CdkLogger.warn("`sourceClusterDisabled` and `sourceClusterEndpoint` are being deprecated in favor of a `sourceCluster` object.") CdkLogger.warn("Please update your CDK context block to use the `sourceCluster` object.") + CdkLogger.warn("Defaulting to source cluster version: ES_7.10") sourceClusterDefinition = { "disabled": sourceClusterDisabledField, "endpoint": sourceClusterEndpointField, - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } const sourceClusterDisabled = !!sourceClusterDefinition?.disabled const sourceCluster = (sourceClusterDefinition && !sourceClusterDisabled) ? parseClusterDefinition(sourceClusterDefinition) : undefined + if (sourceCluster) { + if (!sourceCluster.version) { + throw new Error("The `sourceCluster` object requires a `version` field.") + } + } const sourceClusterEndpoint = sourceCluster?.endpoint if (managedServiceSourceSnapshotEnabled && !sourceCluster?.auth.sigv4) { diff --git a/deployment/cdk/opensearch-service-migration/options.md b/deployment/cdk/opensearch-service-migration/options.md index 34a3e8949..b91864f2a 100644 --- a/deployment/cdk/opensearch-service-migration/options.md +++ b/deployment/cdk/opensearch-service-migration/options.md @@ -42,16 +42,13 @@ If no source cluster is being configured, the source cluster object should be `{ In all other cases, the required components of each cluster object are: - `endpoint` -- the fully specified endpoint for the cluster +- `version` -- the Elasticsearch or OpenSearch version of the cluster, in the format of `OS_x.y` or `ES_x.y` This will be passed to the ReindexFromSnapshot service, if enabled, and provided for the metadata migration on the Migration Console. - `auth` -- what authorization strategy the cluster has. The supported options are: 1. No auth: `{"type": "none"}` 2. Sigv4 Signing: `{"type": "sigv4", "region": "us-east-1", "serviceSigningName": "es"}` The serviceSigningName is `es` for Elasticsearch and OpenSearch managed service domains, and `aoss` for Amazon OpenSearch Serverless 3. Basic auth with plaintext password (only supported for the source cluster and not recommended): `{"type": "basic", "username": "admin", "password": "admin123"}` 4. Basic auth with password in secrets manager (recommended): `{"type": "basic", "username": "admin", "passwordFromSecretArn": "arn:aws:secretsmanager:us-east-1:12345678912:secret:master-user-os-pass-123abc"}` -The optional component is: - -- `version` -- the Elasticsearch or OpenSearch version of the cluster, in the format of `OS_x.y` or `ES_x.y` This will be passed to the ReindexFromSnapshot service, if enabled, and provided for the metadata migration on the Migration Console. It defaults to `ES_7.10.2`. - ### Reindex from Snapshot (RFS) Service Options | Name | Type | Example | Description | diff --git a/deployment/cdk/opensearch-service-migration/test/common-utilities.test.ts b/deployment/cdk/opensearch-service-migration/test/common-utilities.test.ts index 1ce27c069..490fdfc31 100644 --- a/deployment/cdk/opensearch-service-migration/test/common-utilities.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/common-utilities.test.ts @@ -245,6 +245,7 @@ describe('validateFargateCpuArch', () => { test('parseClusterDefinition with basic auth parameters', () => { const clusterDefinition = { endpoint: 'https://target-cluster', + version: 'ES_7.10', auth: { type: 'basic', username: 'admin', @@ -254,6 +255,7 @@ describe('validateFargateCpuArch', () => { const parsed = parseClusterDefinition(clusterDefinition); expect(parsed).toBeDefined(); expect(parsed.endpoint).toBe(clusterDefinition.endpoint); + expect(parsed.version).toBe(clusterDefinition.version); expect(parsed.auth.basicAuth).toBeDefined(); expect(parsed.auth.basicAuth?.username).toBe(clusterDefinition.auth.username); expect(parsed.auth.basicAuth?.password_from_secret_arn).toBe(clusterDefinition.auth.passwordFromSecretArn); diff --git a/deployment/cdk/opensearch-service-migration/test/migration-services-yaml.test.ts b/deployment/cdk/opensearch-service-migration/test/migration-services-yaml.test.ts index d1c37d328..10b15abbb 100644 --- a/deployment/cdk/opensearch-service-migration/test/migration-services-yaml.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/migration-services-yaml.test.ts @@ -125,7 +125,8 @@ describe('Migration Services YAML Tests', () => { migrationConsoleServiceEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, reindexFromSnapshotServiceEnabled: true, trafficReplayerServiceEnabled: true, @@ -177,7 +178,8 @@ describe('Migration Services YAML Tests', () => { migrationConsoleServiceEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, targetCluster: { "endpoint": "https://target-cluster", @@ -230,7 +232,8 @@ describe('Migration Services YAML Tests', () => { migrationConsoleServiceEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, targetCluster: { "endpoint": "https://target-cluster", diff --git a/deployment/cdk/opensearch-service-migration/test/network-stack.test.ts b/deployment/cdk/opensearch-service-migration/test/network-stack.test.ts index e6c75d710..681559447 100644 --- a/deployment/cdk/opensearch-service-migration/test/network-stack.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/network-stack.test.ts @@ -62,7 +62,8 @@ describe('NetworkStack Tests', () => { vpcAZCount: 2, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } @@ -88,7 +89,8 @@ describe('NetworkStack Tests', () => { vpcAZCount: 2, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } @@ -107,7 +109,8 @@ describe('NetworkStack Tests', () => { vpcAZCount: 2, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } diff --git a/deployment/cdk/opensearch-service-migration/test/opensearch-domain-stack.test.ts b/deployment/cdk/opensearch-service-migration/test/opensearch-domain-stack.test.ts index 85070b651..e309b26f6 100644 --- a/deployment/cdk/opensearch-service-migration/test/opensearch-domain-stack.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/opensearch-domain-stack.test.ts @@ -58,7 +58,8 @@ describe('OpenSearch Domain Stack Tests', () => { domainRemovalPolicy: "DESTROY", sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } @@ -114,7 +115,8 @@ describe('OpenSearch Domain Stack Tests', () => { domainRemovalPolicy: "DESTROY", sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } @@ -150,7 +152,8 @@ describe('OpenSearch Domain Stack Tests', () => { nodeToNodeEncryptionEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } @@ -173,7 +176,8 @@ describe('OpenSearch Domain Stack Tests', () => { nodeToNodeEncryptionEnabled: "true", sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } diff --git a/deployment/cdk/opensearch-service-migration/test/reindex-from-snapshot-stack.test.ts b/deployment/cdk/opensearch-service-migration/test/reindex-from-snapshot-stack.test.ts index 68c645993..da75e12e7 100644 --- a/deployment/cdk/opensearch-service-migration/test/reindex-from-snapshot-stack.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/reindex-from-snapshot-stack.test.ts @@ -39,7 +39,8 @@ describe('ReindexFromSnapshotStack Tests', () => { vpcEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, reindexFromSnapshotServiceEnabled: true, stage: 'unit-test', @@ -79,7 +80,8 @@ describe('ReindexFromSnapshotStack Tests', () => { vpcEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, reindexFromSnapshotServiceEnabled: true, stage: 'unit-test', @@ -118,7 +120,7 @@ describe('ReindexFromSnapshotStack Tests', () => { { "Ref": "SsmParameterValuemigrationunittestdefaultosClusterEndpointC96584B6F00A464EAD1953AFF4B05118Parameter", }, - " --max-shard-size-bytes 85899345920" + " --max-shard-size-bytes 85899345920 --source-version \"ES_7.10\"" ], ], } @@ -149,7 +151,8 @@ describe('ReindexFromSnapshotStack Tests', () => { stage: 'unit-test', sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, targetCluster: { "endpoint": "https://target-cluster", @@ -186,7 +189,7 @@ describe('ReindexFromSnapshotStack Tests', () => { { "Ref": "SsmParameterValuemigrationunittestdefaultosClusterEndpointC96584B6F00A464EAD1953AFF4B05118Parameter", }, - " --max-shard-size-bytes 85899345920 --target-aws-service-signing-name aoss --target-aws-region eu-west-1", + " --max-shard-size-bytes 85899345920 --target-aws-service-signing-name aoss --target-aws-region eu-west-1 --source-version \"ES_7.10\"", ], ], } @@ -217,7 +220,8 @@ describe('ReindexFromSnapshotStack Tests', () => { stage: 'unit-test', sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, migrationAssistanceEnabled: true, }; @@ -242,7 +246,8 @@ describe('ReindexFromSnapshotStack Tests', () => { stage: 'unit-test', sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" }, reindexFromSnapshotExtraArgs: '--custom-arg value --flag --snapshot-name \"custom-snapshot\"', migrationAssistanceEnabled: true, @@ -275,7 +280,7 @@ describe('ReindexFromSnapshotStack Tests', () => { { "Ref": "SsmParameterValuemigrationunittestdefaultosClusterEndpointC96584B6F00A464EAD1953AFF4B05118Parameter", }, - " --max-shard-size-bytes 85899345920 --custom-arg value --flag --snapshot-name \"custom-snapshot\"" + " --max-shard-size-bytes 85899345920 --source-version \"ES_7.10\" --custom-arg value --flag --snapshot-name \"custom-snapshot\"" ] ] } diff --git a/deployment/cdk/opensearch-service-migration/test/resources/sample-context-file.json b/deployment/cdk/opensearch-service-migration/test/resources/sample-context-file.json index 274ecb476..3bb809cab 100644 --- a/deployment/cdk/opensearch-service-migration/test/resources/sample-context-file.json +++ b/deployment/cdk/opensearch-service-migration/test/resources/sample-context-file.json @@ -7,6 +7,7 @@ "otelCollectorEnabled": false, "sourceCluster": { "endpoint": "https://test-cluster", + "version": "ES_7.10", "auth": { "type": "none" } } } diff --git a/deployment/cdk/opensearch-service-migration/test/stack-composer-ordering.test.ts b/deployment/cdk/opensearch-service-migration/test/stack-composer-ordering.test.ts index db0424492..ad5ab2a36 100644 --- a/deployment/cdk/opensearch-service-migration/test/stack-composer-ordering.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/stack-composer-ordering.test.ts @@ -117,7 +117,8 @@ describe('Stack Composer Ordering Tests', () => { "reindexFromSnapshotServiceEnabled": false, "sourceCluster": { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } } diff --git a/deployment/cdk/opensearch-service-migration/test/stack-composer.test.ts b/deployment/cdk/opensearch-service-migration/test/stack-composer.test.ts index 65002d981..b1cf706d5 100644 --- a/deployment/cdk/opensearch-service-migration/test/stack-composer.test.ts +++ b/deployment/cdk/opensearch-service-migration/test/stack-composer.test.ts @@ -42,7 +42,7 @@ describe('Stack Composer Tests', () => { expect(createStackFunc).toThrow() }) - test('Test ES 7.10 engine version format is parsed', () => { + test('Test ES_7.10 engine version format is parsed', () => { const contextOptions = { engineVersion: "ES_7.10" } @@ -218,7 +218,8 @@ describe('Stack Composer Tests', () => { migrationConsoleServiceEnabled: true, sourceCluster: { "endpoint": "https://test-cluster", - "auth": {"type": "none"} + "auth": {"type": "none"}, + "version": "ES_7.10" } }