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 <[email protected]> Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Andre Kurait <[email protected]> Co-authored-by: Andre Kurait <[email protected]>
opensearch-project · Oct 17, 2024 · 85d5846 · 85d5846
1 parent d8b4d6c
commit 85d5846
Show file tree

Hide file tree

Showing 18 changed files with 74 additions and 33 deletions.
diff --git a/...ntsFromSnapshotMigration/src/main/java/org/opensearch/migrations/RfsMigrateDocuments.java b/...ntsFromSnapshotMigration/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/...pshotMigration/src/test/java/org/opensearch/migrations/bulkload/ProcessLifecycleTest.java b/...pshotMigration/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
@@ -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 [...]
 ```
 
 <details>
@@ -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 [...]
 ```
 
 <details>

diff --git 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
@@ -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/...dataMigration/src/test/java/org/opensearch/migrations/cli/ClusterReaderExtractorTest.java b/...dataMigration/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
@@ -15,6 +15,7 @@
     },
     "sourceCluster": {
       "endpoint": "<SOURCE_CLUSTER_ENDPOINT>",
+      "version": "<SOURCE_CLUSTER_VERSION, ES_7.10 or OS_1.3>",
       "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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"
       }
     }