Support .opensearch-knn-model index as system index with security enabled #827

martin-gaievski · 2023-03-29T00:28:52Z

Description

With this change plugin's integ tests can be execute in remote cluster with enabled security plugin. It will work with latest security plugin code, we don't need to repeat opensearch-project/security#2480 similarly to 2.6 branch/release.

Main implemented ideas are:

setup https client using pre-generated certificates (certificates and https setup and settings are copied from AD)
do not delete our model system index, only cleanup all docs and models
user permissions are elevated temporarily on backend side by stashing thread context in model related operations
test logic has been changed to use model APIs instead of direct upload to training index

Issues Resolved

opensearch-project/security#2265

Check List

New functionality includes testing.
- All tests pass
Commits are signed as 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.

codecov-commenter · 2023-03-29T00:48:03Z

Codecov Report

Merging #827 (47c667c) into main (59cc733) will increase coverage by 0.03%.
The diff coverage is 96.61%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #827      +/-   ##
============================================
+ Coverage     85.14%   85.18%   +0.03%     
- Complexity     1079     1081       +2     
============================================
  Files           151      151              
  Lines          4370     4394      +24     
  Branches        390      390              
============================================
+ Hits           3721     3743      +22     
- Misses          472      474       +2     
  Partials        177      177

Impacted Files	Coverage Δ
...main/java/org/opensearch/knn/indices/ModelDao.java	`82.67% <96.61%> (+0.93%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

martin-gaievski · 2023-03-30T16:32:50Z

checks for this PR will fail until we get fresh distribution build, current blocker is security plugin, PR is opensearch-project/security#2608

jmazanec15 · 2023-03-30T16:42:14Z

we don't need to repeat opensearch-project/security#2480 similarly to 2.6 branch/release.

What do you mean by this? I think we need to revert this for 2.x until we add special access to system index in code.

jmazanec15 · 2023-03-30T16:39:05Z

src/test/java/org/opensearch/knn/index/FaissIT.java

+
+    @Override
+    protected RestClient getClient() {
+        return adminClient();


Not sure if we should use admin client for all requests.

let me check, if there will be more case where normal client is enough I'll update the logic and use admin client only where needed

jmazanec15 · 2023-03-30T16:40:19Z

src/testFixtures/java/org/opensearch/knn/ODFERestTestCase.java

-                String indexName = (String) index.get("index");
+                final String indexName = (String) index.get("index");
+                if (isIndexCleanupRequired(indexName)) {
+                    wipeIndexContent(MODEL_INDEX_NAME);


Why is MODEL_INDEX_NAME used here and not indexName?

naveentatikonda · 2023-03-30T16:49:23Z

src/test/java/org/opensearch/knn/plugin/action/RestKNNStatsHandlerIT.java

-        assertNull(statsMap.get(StatNames.MODEL_INDEX_STATUS.getName()));
+            // Check that model health status is null since model index is not created to system yet
+            assertNull(statsMap.get(StatNames.MODEL_INDEX_STATUS.getName()));
+        }

        createModelSystemIndex();


May be we should move this line of code into the if condition as we want to create it when it doesn't exist

martin-gaievski · 2023-03-30T17:35:07Z

opensearch-project/security#2480

I did development/testing on main of security plugin, that does have opensearch-project/security#2274. That was my understanding, that we need to keep model index as system and make tests work.

If we revert the security plugin change then I think it will just work, model index became normal index

navneet1v · 2023-04-01T18:42:14Z

build.gradle

@@ -178,6 +178,7 @@ dependencies {
    testImplementation group: 'net.bytebuddy', name: 'byte-buddy', version: '1.14.2'
    testImplementation group: 'org.objenesis', name: 'objenesis', version: '3.2'
    testImplementation group: 'net.bytebuddy', name: 'byte-buddy-agent', version: '1.14.2'
+    api "org.opensearch:common-utils:${version}"


Why we need this?

we're importing some constants from common-utils for https client setup https://github.com/martin-gaievski/k-NN/blob/support-integ-tests-on-secured-cluster/src/testFixtures/java/org/opensearch/knn/ODFERestTestCase.java#L289-L300

navneet1v · 2023-04-01T18:48:27Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

-            GetResponse getResponse = getRequestBuilder.execute().get();
-            Map<String, Object> responseMap = getResponse.getSourceAsMap();
-            return Model.getModelFromSourceMap(responseMap);
+            try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {


Can you please add some comment which can explain what we want to achieve here? I also don't see the usage of context variable. May be I am missing something here.

ack, also variable assignment is required by Java

navneet1v · 2023-04-01T18:48:42Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

@@ -404,8 +407,10 @@ public void get(String modelId, ActionListener<GetModelResponse> actionListener)
         */
        @Override
        public void search(SearchRequest request, ActionListener<SearchResponse> actionListener) {
-            request.indices(MODEL_INDEX_NAME);
-            client.search(request, actionListener);
+            try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {


Same as above

src/main/java/org/opensearch/knn/plugin/transport/DeleteModelTransportAction.java

navneet1v · 2023-04-01T18:51:36Z

src/main/java/org/opensearch/knn/plugin/transport/DeleteModelTransportAction.java

+            modelDao.delete(modelID, listener);
+        } catch (Exception e) {
+            logger.error(e);
+            listener.onFailure(e);


If we do this then do we need above line where we are logging the error?

probably not needed, checked k-NN implementations, we log exception with the proper error message in there https://github.com/martin-gaievski/k-NN/blob/support-integ-tests-on-secured-cluster/src/main/java/org/opensearch/knn/index/KNNSettings.java#L412. Will remove in revision

src/main/java/org/opensearch/knn/plugin/transport/GetModelTransportAction.java

src/main/java/org/opensearch/knn/indices/ModelDao.java

src/main/java/org/opensearch/knn/plugin/transport/DeleteModelTransportAction.java

src/main/java/org/opensearch/knn/plugin/transport/GetModelTransportAction.java

jmazanec15 · 2023-04-03T20:57:16Z

CHANGELOG.md

@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 * Add CHANGELOG ([#800](https://github.com/opensearch-project/k-NN/pull/800))
 * Bump byte-buddy version from 1.12.22 to 1.14.2 ([#804](https://github.com/opensearch-project/k-NN/pull/804))
 * Bump numpy version from 1.22.x to 1.24.2 ([#811](https://github.com/opensearch-project/k-NN/pull/811))
+* Add support for integ tests on secured cluster ([#827](https://github.com/opensearch-project/k-NN/pull/827))


Change this to "Support .opensearch-knn-model index as system index with security enabled"

jmazanec15 · 2023-04-03T21:01:57Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

+            // temporary setting thread context to default, this is needed to allow actions on model system index
+            // when security plugin is enabled
+            try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
+                client.execute(


This doesnt touch the model system index. Is this needed?

jmazanec15 · 2023-04-03T21:02:19Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

-            );
+            // temporary setting thread context to default, this is needed to allow actions on model system index
+            // when security plugin is enabled
+            try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {


This doesnt touch the model system index. Is this still needed?

jmazanec15 · 2023-04-03T21:03:22Z

src/main/java/org/opensearch/knn/indices/ModelDao.java

@@ -583,59 +624,76 @@ private void updateModelGraveyardToDelete(
            StepListener<AcknowledgedResponse> step,
            Optional<Exception> exception
        ) {
+            // temporary setting thread context to default, this is needed to allow actions on model system index
+            // when security plugin is enabled
+            try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {


This doesnt touch the model system index. Is this still needed?

updated modelDao, removed context stashes where we access metadata

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

…ng calls Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Signed-off-by: Martin Gaievski <gaievski@amazon.com> Fixing issues related to exception handling Signed-off-by: Martin Gaievski <gaievski@amazon.com>

jmazanec15

Looks good to me thanks!

opensearch-trigger-bot · 2023-04-05T01:30:39Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-827-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b94b030afc74efc1fae5c5c7528edc25974a3fec
# Push it to GitHub
git push --set-upstream origin backport/backport-827-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-827-to-2.x.

opensearch-trigger-bot · 2023-04-05T01:30:40Z

The backport to 2.7 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.7 2.7
# Navigate to the new working tree
cd .worktrees/backport-2.7
# Create a new branch
git switch --create backport/backport-827-to-2.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b94b030afc74efc1fae5c5c7528edc25974a3fec
# Push it to GitHub
git push --set-upstream origin backport/backport-827-to-2.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.7

Then, create a pull request where the base branch is 2.7 and the compare/head branch is backport/backport-827-to-2.7.

…bled (opensearch-project#827) * Add support for integ tests on secured cluster Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit b94b030)

…bled (opensearch-project#827) * Add support for integ tests on secured cluster Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit b94b030) Signed-off-by: Martin Gaievski <gaievski@amazon.com>

…bled (opensearch-project#827) * Add support for integ tests on secured cluster Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit b94b030) Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit 99300b1)

…bled (#827) (#842) * Add support for integ tests on secured cluster (cherry picked from commit b94b030) Signed-off-by: Martin Gaievski <gaievski@amazon.com>

…bled (#827) (#843) * Add support for integ tests on secured cluster Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit b94b030) Signed-off-by: Martin Gaievski <gaievski@amazon.com> (cherry picked from commit 99300b1)

martin-gaievski added Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x v2.7.0 labels Mar 29, 2023

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch from 871e286 to ed83707 Compare March 29, 2023 00:36

martin-gaievski marked this pull request as ready for review March 30, 2023 16:34

martin-gaievski requested a review from a team March 30, 2023 16:34

jmazanec15 reviewed Mar 30, 2023

View reviewed changes

naveentatikonda reviewed Mar 30, 2023

View reviewed changes

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch from ce86540 to 75a772c Compare April 1, 2023 00:29

martin-gaievski requested review from jmazanec15 and naveentatikonda April 1, 2023 00:29

navneet1v reviewed Apr 1, 2023

View reviewed changes

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch from 7095ec8 to 60681e6 Compare April 3, 2023 18:07

jmazanec15 reviewed Apr 3, 2023

View reviewed changes

martin-gaievski requested review from jmazanec15 and navneet1v April 3, 2023 20:14

jmazanec15 reviewed Apr 3, 2023

View reviewed changes

martin-gaievski requested a review from jmazanec15 April 3, 2023 22:57

martin-gaievski changed the title ~~Add support for integ tests on secured cluster~~ Support .opensearch-knn-model index as system index with security enabled Apr 3, 2023

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch from 27e70ad to d426aea Compare April 3, 2023 23:02

martin-gaievski mentioned this pull request Apr 3, 2023

Add github action for secure integ tests #836

Merged

3 tasks

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch 2 times, most recently from de3e487 to 0fc30b9 Compare April 4, 2023 00:46

martin-gaievski added the backport 2.7 label Apr 4, 2023

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch 2 times, most recently from a5100ef to fb1111d Compare April 4, 2023 21:01

martin-gaievski added 8 commits April 4, 2023 15:28

Add support for integ tests on secured cluster

0024063

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Remove adminClient, switch from direct index put/post to model traini…

9bcfd82

…ng calls Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Stashing thread context to allow model operations to regular client

32e6f27

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Adding code comments and removing redundant logging

ba23f38

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Fixed after test cleanup logic

7e28a86

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Move thread context stashing from transport actions to model dao

2a81605

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Stash context only in methods that work with system index

9920fef

Signed-off-by: Martin Gaievski <gaievski@amazon.com>

Abstract thread context stashing into a method

47c667c

Signed-off-by: Martin Gaievski <gaievski@amazon.com> Fixing issues related to exception handling Signed-off-by: Martin Gaievski <gaievski@amazon.com>

martin-gaievski force-pushed the support-integ-tests-on-secured-cluster branch from fb1111d to 47c667c Compare April 4, 2023 22:32

jmazanec15 approved these changes Apr 5, 2023

View reviewed changes

naveentatikonda approved these changes Apr 5, 2023

View reviewed changes

martin-gaievski merged commit b94b030 into opensearch-project:main Apr 5, 2023

martin-gaievski mentioned this pull request Apr 5, 2023

[Backport 2.x] Support .opensearch-knn-model index as system index with security enabled (#827) #842

Merged

martin-gaievski mentioned this pull request Apr 6, 2023

[Backport 2.7] Support .opensearch-knn-model index as system index with security enabled (#827) #843

Merged

martin-gaievski mentioned this pull request Apr 7, 2023

Add support of .opensearch-knn-model as system index to transport actions #847

Merged

3 tasks

jmazanec15 mentioned this pull request Apr 14, 2023

Switch stashing thread context back to only around system index calls #859

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support .opensearch-knn-model index as system index with security enabled #827

Support .opensearch-knn-model index as system index with security enabled #827

martin-gaievski commented Mar 29, 2023 •

edited

Loading

codecov-commenter commented Mar 29, 2023 •

edited

Loading

martin-gaievski commented Mar 30, 2023 •

edited

Loading

jmazanec15 commented Mar 30, 2023

jmazanec15 Mar 30, 2023

martin-gaievski Mar 30, 2023

jmazanec15 Mar 30, 2023

martin-gaievski Mar 30, 2023

naveentatikonda Mar 30, 2023

martin-gaievski Mar 30, 2023

martin-gaievski commented Mar 30, 2023

navneet1v Apr 1, 2023

martin-gaievski Apr 3, 2023

navneet1v Apr 1, 2023

martin-gaievski Apr 3, 2023 •

edited

Loading

navneet1v Apr 1, 2023

navneet1v Apr 1, 2023

martin-gaievski Apr 3, 2023

jmazanec15 Apr 3, 2023

martin-gaievski Apr 3, 2023

jmazanec15 Apr 3, 2023

martin-gaievski Apr 3, 2023

jmazanec15 Apr 3, 2023

jmazanec15 Apr 3, 2023

martin-gaievski Apr 3, 2023

jmazanec15 left a comment

opensearch-trigger-bot bot commented Apr 5, 2023

opensearch-trigger-bot bot commented Apr 5, 2023

Support .opensearch-knn-model index as system index with security enabled #827

Support .opensearch-knn-model index as system index with security enabled #827

Conversation

martin-gaievski commented Mar 29, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Mar 29, 2023 • edited Loading

Codecov Report

martin-gaievski commented Mar 30, 2023 • edited Loading

jmazanec15 commented Mar 30, 2023

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

martin-gaievski commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-gaievski Apr 3, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Apr 5, 2023

opensearch-trigger-bot bot commented Apr 5, 2023

martin-gaievski commented Mar 29, 2023 •

edited

Loading

codecov-commenter commented Mar 29, 2023 •

edited

Loading

martin-gaievski commented Mar 30, 2023 •

edited

Loading

martin-gaievski Apr 3, 2023 •

edited

Loading