From 07fbc88a4bbed83cc2f675d37353e6f02120b4ab Mon Sep 17 00:00:00 2001 From: Vijayan Balasubramanian Date: Mon, 2 Dec 2024 12:31:56 -0800 Subject: [PATCH 1/2] Validate method parameters only after version 2_17_0 k-NN introduced validation to prevent index creation api to accept method paramters that are used by approximate k-NN search algorithm. For ex: create index api accepts model_id, or, method params even if knn index setting was not true before 2.17. However, for index that were created before 2.17 and upgraded to 2.17, will prevent cluster to parse those previously accepted index mapping. Hence, k-NN will allow those paramters for indices that were created before 2.17, but doesn't support those parameters starting 2.17. Signed-off-by: Vijayan Balasubramanian --- .../index/mapper/KNNVectorFieldMapper.java | 13 ++++++++--- .../opensearch/knn/index/OpenSearchIT.java | 23 +++++++++++++++++++ .../org/opensearch/knn/KNNRestTestCase.java | 16 +++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java index 6e5138a56..7c9c7ecb1 100644 --- a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java +++ b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java @@ -182,6 +182,8 @@ public static class Builder extends ParametrizedFieldMapper.Builder { @Setter @Getter private OriginalMappingParameters originalParameters; + @Setter + private boolean isKnnIndex; public Builder( String name, @@ -195,6 +197,7 @@ public Builder( this.indexCreatedVersion = indexCreatedVersion; this.knnMethodConfigContext = knnMethodConfigContext; this.originalParameters = originalParameters; + this.isKnnIndex = true; } @Override @@ -250,7 +253,7 @@ public KNNVectorFieldMapper build(BuilderContext context) { ); } - if (originalParameters.getResolvedKnnMethodContext() == null) { + if (originalParameters.getResolvedKnnMethodContext() == null || isKnnIndex == false) { return FlatVectorFieldMapper.createFieldMapper( buildFullName(context), name, @@ -362,10 +365,14 @@ public Mapper.Builder parse(String name, Map node, ParserCont String.format(Locale.ROOT, "Method and model can not be both specified in the mapping: %s", name) ); } - // Check for flat configuration if (isKNNDisabled(parserContext.getSettings())) { - validateFromFlat(builder); + builder.setKnnIndex(false); + if (parserContext.indexVersionCreated().onOrAfter(Version.V_2_17_0)) { + // on and after 2_17_0 we validate to makes sure that mapping doesn't contain parameters that are + // specific to approximate knn search algorithms + validateFromFlat(builder); + } } else if (builder.modelId.get() != null) { validateFromModel(builder); } else { diff --git a/src/test/java/org/opensearch/knn/index/OpenSearchIT.java b/src/test/java/org/opensearch/knn/index/OpenSearchIT.java index c1d3b47c3..1b491da02 100644 --- a/src/test/java/org/opensearch/knn/index/OpenSearchIT.java +++ b/src/test/java/org/opensearch/knn/index/OpenSearchIT.java @@ -483,4 +483,27 @@ public void testIndexingVectorValidation_updateVectorWithNull() throws Exception assertArrayEquals(vectorForDocumentOne, vectorRestoreInitialValue); } + public void testCreateNonKNNIndex_withKNNModelID() throws Exception { + Settings settings = Settings.builder().put(createKNNDefaultScriptScoreSettings()).build(); + ResponseException ex = expectThrows( + ResponseException.class, + () -> createKnnIndex(INDEX_NAME, settings, createKnnIndexMapping(FIELD_NAME, "random-model-id")) + ); + String expMessage = "Cannot set modelId or method parameters when index.knn setting is false"; + assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString(expMessage)); + } + + public void testCreateNonKNNIndex_withKNNMethodParams() throws Exception { + Settings settings = Settings.builder().put(createKNNDefaultScriptScoreSettings()).build(); + ResponseException ex = expectThrows( + ResponseException.class, + () -> createKnnIndex( + INDEX_NAME, + settings, + createKnnIndexMapping(FIELD_NAME, 2, "hnsw", KNNEngine.FAISS.getName(), SpaceType.DEFAULT.getValue(), false) + ) + ); + String expMessage = "Cannot set modelId or method parameters when index.knn setting is false"; + assertThat(EntityUtils.toString(ex.getResponse().getEntity()), containsString(expMessage)); + } } diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index a4659f691..5ef1c7cc2 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -400,6 +400,22 @@ protected String createKnnIndexMapping(final String fieldName, final Integer dim .toString(); } + /** + * Utility to create a Knn Index Mapping with model id + */ + protected String createKnnIndexMapping(String fieldName, String modelId) throws IOException { + return XContentFactory.jsonBuilder() + .startObject() + .startObject("properties") + .startObject(fieldName) + .field("type", "knn_vector") + .field("model_id", modelId) + .endObject() + .endObject() + .endObject() + .toString(); + } + /** * Utility to create a Knn Index Mapping with specific algorithm and engine */ From 4e2ef45a9ed279f3a38d1d98f6ffd5c77e20ee98 Mon Sep 17 00:00:00 2001 From: Vijayan Balasubramanian Date: Mon, 2 Dec 2024 15:30:09 -0800 Subject: [PATCH 2/2] Update ci.yml Signed-off-by: Vijayan Balasubramanian --- .github/workflows/CI.yml | 16 +++++++++------- .github/workflows/test_security.yml | 11 ++++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 932ce8022..e7b4b811d 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -30,8 +30,7 @@ on: - 'jni/**' - 'micro-benchmarks/**' - '.github/workflows/CI.yml' -env: - ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true + jobs: Get-CI-Image-Tag: @@ -52,11 +51,13 @@ jobs: # this image tag is subject to change as more dependencies and updates will arrive over time image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} # need to switch to root so that github actions can install runner binary on container without permission issues. - options: --user root + options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} steps: + - name: Run start commands + run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} - name: Checkout k-NN - uses: actions/checkout@v1 + uses: actions/checkout@v4 # Setup git user so that patches for native libraries can be applied and committed - name: Setup git user @@ -65,8 +66,9 @@ jobs: su `id -un 1000` -c 'git config --global user.email "github-actions[bot]@users.noreply.github.com"' - name: Setup Java ${{ matrix.java }} - uses: actions/setup-java@v1 + uses: actions/setup-java@v4 with: + distribution: "corretto" java-version: ${{ matrix.java }} - name: Run build @@ -84,7 +86,7 @@ jobs: - name: Upload Coverage Report - uses: codecov/codecov-action@v1 + uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} @@ -99,7 +101,7 @@ jobs: steps: - name: Checkout k-NN - uses: actions/checkout@v1 + uses: actions/checkout@v4 # Setup git user so that patches for native libraries can be applied and committed - name: Setup git user diff --git a/.github/workflows/test_security.yml b/.github/workflows/test_security.yml index 2f8df8526..aefb555c4 100644 --- a/.github/workflows/test_security.yml +++ b/.github/workflows/test_security.yml @@ -28,8 +28,6 @@ on: - 'gradle/**' - 'jni/**' - '.github/workflows/test_security.yml' -env: - ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true jobs: Get-CI-Image-Tag: @@ -50,11 +48,13 @@ jobs: # this image tag is subject to change as more dependencies and updates will arrive over time image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }} # need to switch to root so that github actions can install runner binary on container without permission issues. - options: --user root + options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }} steps: + - name: Run start commands + run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }} - name: Checkout k-NN - uses: actions/checkout@v1 + uses: actions/checkout@v4 # Setup git user so that patches for native libraries can be applied and committed - name: Setup git user run: | @@ -62,9 +62,10 @@ jobs: su `id -un 1000` -c 'git config --global user.email "github-actions[bot]@users.noreply.github.com"' - name: Setup Java ${{ matrix.java }} - uses: actions/setup-java@v1 + uses: actions/setup-java@v4 with: java-version: ${{ matrix.java }} + distribution: "corretto" - name: Run build # switching the user, as OpenSearch cluster can only be started as root/Administrator on linux-deb/linux-rpm/windows-zip.