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. 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 */