diff --git a/CHANGELOG.md b/CHANGELOG.md index 74da5dee0aa78..2ff3810fdc7dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,7 +105,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Support for returning scores in matched queries ([#11626](https://github.com/opensearch-project/OpenSearch/pull/11626)) - Add shard id property to SearchLookup for use in field types provided by plugins ([#1063](https://github.com/opensearch-project/OpenSearch/pull/1063)) - Add kuromoji_completion analyzer and filter ([#4835](https://github.com/opensearch-project/OpenSearch/issues/4835)) -- [Query insights] Add user info in top queries ([#12529](https://github.com/opensearch-project/OpenSearch/pull/12529)) +- [Query insights] Add remote address info in top queries ([#12529](https://github.com/opensearch-project/OpenSearch/pull/12529)) ### Dependencies - Bump `peter-evans/find-comment` from 2 to 3 ([#12288](https://github.com/opensearch-project/OpenSearch/pull/12288)) diff --git a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java index 69b6adbe1efcf..4d7e0d486068a 100644 --- a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java +++ b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/QueryInsightsPlugin.java @@ -71,7 +71,7 @@ public Collection createComponents( ) { // create top n queries service final QueryInsightsService queryInsightsService = new QueryInsightsService(threadPool); - return List.of(queryInsightsService, new QueryInsightsListener(clusterService, queryInsightsService, threadPool)); + return List.of(queryInsightsService, new QueryInsightsListener(clusterService, queryInsightsService)); } @Override diff --git a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java index 380a1366d77d9..7137da8dd7913 100644 --- a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java +++ b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListener.java @@ -21,8 +21,6 @@ import org.opensearch.plugin.insights.rules.model.Attribute; import org.opensearch.plugin.insights.rules.model.MetricType; import org.opensearch.plugin.insights.rules.model.SearchQueryRecord; -import org.opensearch.plugin.insights.utils.ThreadContextParser; -import org.opensearch.threadpool.ThreadPool; import java.util.Collections; import java.util.HashMap; @@ -47,23 +45,16 @@ public final class QueryInsightsListener extends SearchRequestOperationsListener private static final Logger log = LogManager.getLogger(QueryInsightsListener.class); private final QueryInsightsService queryInsightsService; - private final ThreadPool threadPool; /** * Constructor for QueryInsightsListener * * @param clusterService The Node's cluster service. * @param queryInsightsService The topQueriesByLatencyService associated with this listener - * @param threadPool The OpenSearch thread pool to run async tasks */ @Inject - public QueryInsightsListener( - final ClusterService clusterService, - final QueryInsightsService queryInsightsService, - final ThreadPool threadPool - ) { + public QueryInsightsListener(final ClusterService clusterService, final QueryInsightsService queryInsightsService) { this.queryInsightsService = queryInsightsService; - this.threadPool = threadPool; clusterService.getClusterSettings() .addSettingsUpdateConsumer(TOP_N_LATENCY_QUERIES_ENABLED, v -> this.setEnableTopQueries(MetricType.LATENCY, v)); clusterService.getClusterSettings() @@ -147,8 +138,7 @@ public void onRequestEnd(final SearchPhaseContext context, final SearchRequestCo attributes.put(Attribute.TOTAL_SHARDS, context.getNumShards()); attributes.put(Attribute.INDICES, request.indices()); attributes.put(Attribute.PHASE_LATENCY_MAP, searchRequestContext.phaseTookMap()); - // add user related information - attributes.putAll(ThreadContextParser.getUserInfoFromThreadContext(threadPool.getThreadContext())); + attributes.put(Attribute.REMOTE_ADDRESS, searchRequestContext.getRemoteAddress()); SearchQueryRecord record = new SearchQueryRecord(request.getOrCreateAbsoluteStartMillis(), measurements, attributes); queryInsightsService.addRecord(record); } catch (Exception e) { diff --git a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/rules/model/Attribute.java b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/rules/model/Attribute.java index 9b98f84afc4b4..3a29415618b97 100644 --- a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/rules/model/Attribute.java +++ b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/rules/model/Attribute.java @@ -47,23 +47,7 @@ public enum Attribute { /** * The remote address of this request */ - REMOTE_ADDRESS, - /** - * Username of the user who sent this request - */ - USER_NAME, - /** - * Backend roles of the user who sent this request - */ - USER_BACKEND_ROLES, - /** - * Roles of the user who sent this request - */ - USER_ROLES, - /** - * Tenant info of the user who sent this request - */ - USER_TENANT; + REMOTE_ADDRESS; /** * Read an Attribute from a StreamInput diff --git a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java index 890f99e72dfb5..52cc1fbde790f 100644 --- a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java +++ b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/settings/QueryInsightsSettings.java @@ -23,15 +23,6 @@ * @opensearch.experimental */ public class QueryInsightsSettings { - /** - * Constant setting for user info header key that are injected during authentication - */ - public static final String REQUEST_HEADER_USER_INFO = "_opendistro_security_user_info"; - /** - * Constant setting for remote address info header key that are injected during authentication - */ - public static final String REQUEST_HEADER_REMOTE_ADDRESS = "_opendistro_security_remote_address"; - /** * Executors settings */ diff --git a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/utils/ThreadContextParser.java b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/utils/ThreadContextParser.java deleted file mode 100644 index ca1bba1d07553..0000000000000 --- a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/utils/ThreadContextParser.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.plugin.insights.utils; - -import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.core.common.Strings; -import org.opensearch.plugin.insights.rules.model.Attribute; -import org.opensearch.plugin.insights.settings.QueryInsightsSettings; - -import java.util.Arrays; -import java.util.HashMap; -import java.util.Map; - -/** - * Helper class to parse information from the thread context - */ -public final class ThreadContextParser { - - private ThreadContextParser() {} - - /** - * Get User info from the thread context - * - * @param threadContext context of the thread - * @return Map of {@link Attribute} and the corresponding values - */ - public static Map getUserInfoFromThreadContext(ThreadContext threadContext) { - Map userInfoMap = new HashMap<>(); - if (threadContext == null) { - return userInfoMap; - } - Object userInfoObj = threadContext.getTransient(QueryInsightsSettings.REQUEST_HEADER_USER_INFO); - if (userInfoObj == null) { - return userInfoMap; - } - String userInfoStr = userInfoObj.toString(); - Object remoteAddressObj = threadContext.getTransient(QueryInsightsSettings.REQUEST_HEADER_REMOTE_ADDRESS); - if (remoteAddressObj != null) { - userInfoMap.put(Attribute.REMOTE_ADDRESS, remoteAddressObj.toString()); - } - - String[] userInfo = userInfoStr.split("\\|"); - if ((userInfo.length == 0) || (Strings.isNullOrEmpty(userInfo[0]))) { - return userInfoMap; - } - userInfoMap.put(Attribute.USER_NAME, userInfo[0].trim()); - if ((userInfo.length > 1) && !Strings.isNullOrEmpty(userInfo[1])) { - userInfoMap.put(Attribute.USER_BACKEND_ROLES, Arrays.asList(userInfo[1].split(","))); - } - if ((userInfo.length > 2) && !Strings.isNullOrEmpty(userInfo[2])) { - userInfoMap.put(Attribute.USER_ROLES, Arrays.asList(userInfo[2].split(","))); - } - if ((userInfo.length > 3) && !Strings.isNullOrEmpty(userInfo[3])) { - userInfoMap.put(Attribute.USER_TENANT, userInfo[3].trim()); - } - return userInfoMap; - } -} diff --git a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/utils/package-info.java b/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/utils/package-info.java deleted file mode 100644 index 220329a8d4bf4..0000000000000 --- a/plugins/query-insights/src/main/java/org/opensearch/plugin/insights/utils/package-info.java +++ /dev/null @@ -1,12 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -/** - * Utils for Query Insights Plugin - */ -package org.opensearch.plugin.insights.utils; diff --git a/plugins/query-insights/src/test/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListenerTests.java b/plugins/query-insights/src/test/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListenerTests.java index 74fd959050c68..fa0cd31d28c50 100644 --- a/plugins/query-insights/src/test/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListenerTests.java +++ b/plugins/query-insights/src/test/java/org/opensearch/plugin/insights/core/listener/QueryInsightsListenerTests.java @@ -15,7 +15,6 @@ import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.plugin.insights.core.service.QueryInsightsService; import org.opensearch.plugin.insights.core.service.TopQueriesService; import org.opensearch.plugin.insights.rules.model.Attribute; @@ -26,7 +25,6 @@ import org.opensearch.search.aggregations.support.ValueType; import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.threadpool.ThreadPool; import org.junit.Before; import java.util.ArrayList; @@ -54,14 +52,9 @@ public class QueryInsightsListenerTests extends OpenSearchTestCase { private final SearchRequest searchRequest = mock(SearchRequest.class); private final QueryInsightsService queryInsightsService = mock(QueryInsightsService.class); private final TopQueriesService topQueriesService = mock(TopQueriesService.class); - private final ThreadPool threadPool = mock(ThreadPool.class); private final Settings.Builder settingsBuilder = Settings.builder(); private final Settings settings = settingsBuilder.build(); private final String remoteAddress = "1.2.3.4"; - private final String userName = "user1"; - private final List userBackendRoles = List.of("bk-role1", "bk-role2"); - private final List userRoles = List.of("role1", "role2"); - private final String userTenant = "tenant1"; private ClusterService clusterService; @Before @@ -73,15 +66,7 @@ public void setup() { clusterService = new ClusterService(settings, clusterSettings, null); when(queryInsightsService.isCollectionEnabled(MetricType.LATENCY)).thenReturn(true); when(queryInsightsService.getTopQueriesService(MetricType.LATENCY)).thenReturn(topQueriesService); - - // inject user info - ThreadContext threadContext = new ThreadContext(settings); - threadContext.putTransient( - QueryInsightsSettings.REQUEST_HEADER_USER_INFO, - userName + '|' + String.join(",", userBackendRoles) + "|" + String.join(",", userRoles) + "|" + userTenant - ); - threadContext.putTransient(QueryInsightsSettings.REQUEST_HEADER_REMOTE_ADDRESS, remoteAddress); - when(threadPool.getThreadContext()).thenReturn(threadContext); + when(searchRequestContext.getRemoteAddress()).thenReturn(remoteAddress); } public void testOnRequestEnd() { @@ -101,7 +86,7 @@ public void testOnRequestEnd() { int numberOfShards = 10; - QueryInsightsListener queryInsightsListener = new QueryInsightsListener(clusterService, queryInsightsService, threadPool); + QueryInsightsListener queryInsightsListener = new QueryInsightsListener(clusterService, queryInsightsService); when(searchRequest.getOrCreateAbsoluteStartMillis()).thenReturn(timestamp); when(searchRequest.searchType()).thenReturn(searchType); @@ -122,10 +107,6 @@ public void testOnRequestEnd() { assertEquals(numberOfShards, attrs.get(Attribute.TOTAL_SHARDS)); assertEquals(indices, attrs.get(Attribute.INDICES)); assertEquals(phaseLatencyMap, attrs.get(Attribute.PHASE_LATENCY_MAP)); - assertEquals(userName, attrs.get(Attribute.USER_NAME)); - assertEquals(userBackendRoles, attrs.get(Attribute.USER_BACKEND_ROLES)); - assertEquals(userRoles, attrs.get(Attribute.USER_ROLES)); - assertEquals(userTenant, attrs.get(Attribute.USER_TENANT)); assertEquals(remoteAddress, attrs.get(Attribute.REMOTE_ADDRESS)); } @@ -162,7 +143,7 @@ public void testConcurrentOnRequestEnd() throws InterruptedException { CountDownLatch countDownLatch = new CountDownLatch(numRequests); for (int i = 0; i < numRequests; i++) { - searchListenersList.add(new QueryInsightsListener(clusterService, queryInsightsService, threadPool)); + searchListenersList.add(new QueryInsightsListener(clusterService, queryInsightsService)); } for (int i = 0; i < numRequests; i++) { @@ -183,7 +164,7 @@ public void testConcurrentOnRequestEnd() throws InterruptedException { public void testSetEnabled() { when(queryInsightsService.isCollectionEnabled(MetricType.LATENCY)).thenReturn(true); - QueryInsightsListener queryInsightsListener = new QueryInsightsListener(clusterService, queryInsightsService, threadPool); + QueryInsightsListener queryInsightsListener = new QueryInsightsListener(clusterService, queryInsightsService); queryInsightsListener.setEnableTopQueries(MetricType.LATENCY, true); assertTrue(queryInsightsListener.isEnabled()); diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java index 383d9b5e82fe2..8127e576c0e47 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java @@ -51,6 +51,10 @@ public Map phaseTookMap() { return phaseTookMap; } + public String getRemoteAddress() { + return searchRequest.remoteAddress().toString(); + } + SearchResponse.PhaseTook getPhaseTook() { if (searchRequest != null && searchRequest.isPhaseTook() != null && searchRequest.isPhaseTook()) { return new SearchResponse.PhaseTook(phaseTookMap); diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java index 80dc34c4d5d68..f96fec932694f 100644 --- a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java @@ -42,6 +42,7 @@ import org.opensearch.common.Booleans; import org.opensearch.core.common.Strings; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; +import org.opensearch.core.common.transport.TransportAddress; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.QueryBuilder; import org.opensearch.rest.BaseRestHandler; @@ -223,6 +224,9 @@ public static void parseSearchRequest( } searchRequest.setCancelAfterTimeInterval(request.paramAsTime("cancel_after_time_interval", null)); + + // set remote address for searchRequest + searchRequest.remoteAddress(new TransportAddress(request.getHttpChannel().getRemoteAddress())); } /**