Add .ltrstore* as system index and configure test suite to add more p…

…ermissions Signed-off-by: rithin-pullela-aws <[email protected]> Resolve spotless issues Signed-off-by: rithin-pullela-aws <[email protected]> Ignore system index access warning Signed-off-by: rithin-pullela-aws <[email protected]>
rithin-pullela-aws · Jan 29, 2025 · 5c05d02 · 5c05d02
1 parent 35b3cab
commit 5c05d02
Show file tree

Hide file tree

Showing 13 changed files with 446 additions and 35 deletions.
diff --git a/build.gradle b/build.gradle
@@ -261,6 +261,18 @@ spotless {
     removeUnusedImports()
     importOrder 'java', 'javax', 'org', 'com'
 
-    eclipse().configFile rootProject.file('.eclipseformat.xml')
+    eclipse().withP2Mirrors(Map.of("https://download.eclipse.org/", "https://mirror.umd.edu/eclipse/")).configFile rootProject.file('.eclipseformat.xml')
   }
+}
+
+run {
+  doFirst {
+    // There seems to be an issue when running multi node run or integ tasks with unicast_hosts
+    // not being written, the waitForAllConditions ensures it's written
+    getClusters().forEach { cluster ->
+      cluster.waitForAllConditions()
+    }
+  }
+
+  useCluster testClusters.integTest
 }
diff --git a/src/javaRestTest/java/com/o19s/es/ltr/LtrQueryClientYamlTestSuiteIT.java b/src/javaRestTest/java/com/o19s/es/ltr/LtrQueryClientYamlTestSuiteIT.java
@@ -15,8 +15,21 @@
  */
 package com.o19s.es.ltr;
 
+import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_SSL_HTTP_ENABLED;
+import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_FILEPATH;
+import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD;
+import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_PASSWORD;
+import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_SSL_HTTP_PEMCERT_FILEPATH;
+
 import java.io.IOException;
-import java.util.*;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Path;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import org.apache.http.Header;
@@ -33,9 +46,11 @@
 import org.opensearch.client.Response;
 import org.opensearch.client.RestClient;
 import org.opensearch.client.RestClientBuilder;
+import org.opensearch.common.io.PathUtils;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.common.util.concurrent.ThreadContext;
+import org.opensearch.commons.rest.SecureRestClientBuilder;
 import org.opensearch.core.xcontent.DeprecationHandler;
 import org.opensearch.core.xcontent.MediaType;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
@@ -59,9 +74,65 @@ public static Iterable<Object[]> parameters() throws Exception {
         return OpenSearchClientYamlSuiteTestCase.createParameters();
     }
 
+    protected boolean isHttps() {
+        boolean isHttps = Optional.ofNullable(System.getProperty("https")).map("true"::equalsIgnoreCase).orElse(false);
+        if (isHttps) {
+            // currently only external cluster is supported for security enabled testing
+            if (!Optional.ofNullable(System.getProperty("tests.rest.cluster")).isPresent()) {
+                throw new RuntimeException("cluster url should be provided for security enabled testing");
+            }
+        }
+
+        return isHttps;
+    }
+
     @Override
-    protected boolean preserveIndicesUponCompletion() {
-        return true;
+    protected String getProtocol() {
+        return isHttps() ? "https" : "http";
+    }
+
+    @Override
+    protected Settings restAdminSettings() {
+        return Settings
+            .builder()
+            // disable the warning exception for admin client since it's only used for cleanup.
+            .put("strictDeprecationMode", false)
+            .put("http.port", 9200)
+            .put(OPENSEARCH_SECURITY_SSL_HTTP_ENABLED, isHttps())
+            .put(OPENSEARCH_SECURITY_SSL_HTTP_PEMCERT_FILEPATH, "sample.pem")
+            .put(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_FILEPATH, "test-kirk.jks")
+            .put(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_PASSWORD, "changeit")
+            .put(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD, "changeit")
+            .build();
+    }
+
+    @Override
+    protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOException {
+        boolean strictDeprecationMode = settings.getAsBoolean("strictDeprecationMode", true);
+        RestClientBuilder builder = RestClient.builder(hosts);
+        if (isHttps()) {
+            String keystore = settings.get(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_FILEPATH);
+            if (Objects.nonNull(keystore)) {
+                URI uri = null;
+                try {
+                    uri = this.getClass().getClassLoader().getResource("security/sample.pem").toURI();
+                } catch (URISyntaxException e) {
+                    throw new RuntimeException(e);
+                }
+                Path configPath = PathUtils.get(uri).getParent().toAbsolutePath();
+                return new SecureRestClientBuilder(settings, configPath).build();
+            } else {
+                configureHttpsClient(builder, settings);
+                builder.setStrictDeprecationMode(strictDeprecationMode);
+                return builder.build();
+            }
+
+        } else {
+            configureClient(builder, settings);
+            builder.setStrictDeprecationMode(strictDeprecationMode);
+            return builder.build();
+        }
+
     }
 
     @SuppressWarnings("unchecked")
@@ -95,35 +166,6 @@ protected void wipeAllODFEIndices() throws IOException {
         }
     }
 
-    protected String getProtocol() {
-        return isHttps() ? "https" : "http";
-    }
-
-    protected boolean isHttps() {
-        boolean isHttps = Optional.ofNullable(System.getProperty("https")).map("true"::equalsIgnoreCase).orElse(false);
-        if (isHttps) {
-            // currently only external cluster is supported for security enabled testing
-            if (!Optional.ofNullable(System.getProperty("tests.rest.cluster")).isPresent()) {
-                throw new RuntimeException("cluster url should be provided for security enabled testing");
-            }
-        }
-
-        return isHttps;
-    }
-
-    @Override
-    protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOException {
-        RestClientBuilder builder = RestClient.builder(hosts);
-        if (isHttps()) {
-            configureHttpsClient(builder, settings);
-        } else {
-            configureClient(builder, settings);
-        }
-
-        builder.setStrictDeprecationMode(false);
-        return builder.build();
-    }
-
     protected static void configureHttpsClient(RestClientBuilder builder, Settings settings) throws IOException {
         Map<String, String> headers = ThreadContext.buildDefaultHeaders(settings);
         Header[] defaultHeaders = new Header[headers.size()];
@@ -160,4 +202,13 @@ protected static void configureHttpsClient(RestClientBuilder builder, Settings s
             builder.setPathPrefix(settings.get(CLIENT_PATH_PREFIX));
         }
     }
+
+    /**
+     * wipeAllIndices won't work since it cannot delete security index. Use wipeAllODFEIndices instead.
+     */
+    @Override
+    protected boolean preserveIndicesUponCompletion() {
+        return true;
+    }
+
 }
diff --git a/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java b/src/main/java/com/o19s/es/ltr/LtrQueryParserPlugin.java
@@ -57,6 +57,7 @@
 import org.opensearch.env.NodeEnvironment;
 import org.opensearch.index.analysis.PreConfiguredTokenFilter;
 import org.opensearch.index.analysis.PreConfiguredTokenizer;
+import org.opensearch.indices.SystemIndexDescriptor;
 import org.opensearch.ltr.breaker.LTRCircuitBreakerService;
 import org.opensearch.ltr.rest.RestStatsLTRAction;
 import org.opensearch.ltr.settings.LTRSettings;
@@ -75,6 +76,7 @@
 import org.opensearch.plugins.Plugin;
 import org.opensearch.plugins.ScriptPlugin;
 import org.opensearch.plugins.SearchPlugin;
+import org.opensearch.plugins.SystemIndexPlugin;
 import org.opensearch.repositories.RepositoriesService;
 import org.opensearch.rest.RestController;
 import org.opensearch.rest.RestHandler;
@@ -127,7 +129,7 @@
 
 import ciir.umass.edu.learning.RankerFactory;
 
-public class LtrQueryParserPlugin extends Plugin implements SearchPlugin, ScriptPlugin, ActionPlugin, AnalysisPlugin {
+public class LtrQueryParserPlugin extends Plugin implements SearchPlugin, ScriptPlugin, ActionPlugin, AnalysisPlugin, SystemIndexPlugin {
     public static final String LTR_BASE_URI = "/_plugins/_ltr";
     public static final String LTR_LEGACY_BASE_URI = "/_opendistro/_ltr";
     private final LtrRankerParserFactory parserFactory;
@@ -366,4 +368,11 @@ public List<PreConfiguredTokenizer> getPreConfiguredTokenizers() {
                 PreConfiguredTokenizer.singleton("ltr_keyword", () -> new KeywordTokenizer(KeywordTokenizer.DEFAULT_BUFFER_SIZE))
             );
     }
+
+    @Override
+    public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings settings) {
+        List<SystemIndexDescriptor> systemIndexDescriptors = new ArrayList<>();
+        systemIndexDescriptors.add(new SystemIndexDescriptor(".ltrstore*", "ML Commons Agent Index"));
+        return systemIndexDescriptors;
+    }
 }
diff --git a/src/test/resources/rest-api-spec/test/fstore/10_manage.yml b/src/test/resources/rest-api-spec/test/fstore/10_manage.yml
@@ -1,52 +1,76 @@
 ---
 "Create and delete the default store":
+  - skip:
+      features: allowed_warnings
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store: {}
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         indices.exists:
             index: .ltrstore
 
   - is_true: ''
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.delete_store: {}
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         indices.exists:
             index: .ltrstore
 
   - is_false: ''
 
 ---
 "Create and delete a custom store":
+  - skip:
+      features: allowed_warnings
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store:
             store: mystore
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         indices.exists:
             index: .ltrstore_mystore
 
   - is_true: ''
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.get_store:
             store: mystore
 
   - match: { exists: true }
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.delete_store:
             store: mystore
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         indices.exists:
             index: .ltrstore_mystore
 
   - is_false: ''
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         catch: missing
         ltr.get_store:
             store: mystore
@@ -55,31 +79,45 @@
 
 ---
 "Get cache stats":
+  - skip:
+      features: allowed_warnings
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store: {}
 
   - is_true: ''
 
   - do:
+        allowed_warnings:
+           - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.cache_stats: {}
 
   - match: { all.total.ram: 0 }
 
 ---
 "List stores":
+  - skip:
+      features: allowed_warnings
   - do:
         ltr.list_stores: {}
 
   - match: { stores: {} }
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store: {}
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store:
             store: mystore
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_mystore], but in a future major version, direct access to system indices will be prevented by default"
         ltr.list_stores: {}
 
   - match: { stores._default_.version: 2 }
@@ -101,11 +139,17 @@
 
 ---
 "Deleting the store should invalidate the cache":
+  - skip:
+      features: allowed_warnings
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_custom], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store:
           store: custom
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_custom], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_model:
            store: custom
            name: my_model
@@ -145,6 +189,8 @@
   - gt: { all.total.ram: 0 }
 
   - do:
+      allowed_warnings:
+        - "this request accesses system indices: [.ltrstore_custom], but in a future major version, direct access to system indices will be prevented by default"
       indices.delete:
           index: .ltrstore_custom
 
@@ -155,11 +201,17 @@
 
 ---
 "Deleting the model should invalidate the cache":
+  - skip:
+        features: allowed_warnings
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_custom], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_store:
           store: custom
 
   - do:
+        allowed_warnings:
+          - "this request accesses system indices: [.ltrstore_custom], but in a future major version, direct access to system indices will be prevented by default"
         ltr.create_model:
            store: custom
            name: my_model
@@ -199,6 +251,8 @@
   - gt: { all.total.ram: 0 }
 
   - do:
+      allowed_warnings:
+        - "this request accesses system indices: [.ltrstore_custom], but in a future major version, direct access to system indices will be prevented by default"
       ltr.delete_model:
          store: custom
          name: my_model