-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add Put Query Ruleset API call #96812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1675a93
4fff2a5
bf753b1
7965506
dcf24dc
4843423
56a3be4
51220a3
c261a20
acb8b67
403579e
8b9670a
7b46892
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| { | ||
| "query_ruleset.put": { | ||
| "documentation": { | ||
| "url": "https://www.elastic.co/guide/en/elasticsearch/reference/master/put-query-ruleset.html", | ||
| "description": "Creates or updates a query ruleset." | ||
| }, | ||
| "stability": "experimental", | ||
| "visibility": "feature_flag", | ||
| "feature_flag": "es.query_rules_feature_flag_enabled", | ||
| "headers": { | ||
| "accept": [ | ||
| "application/json" | ||
| ], | ||
| "content_type": [ | ||
| "application/json" | ||
| ] | ||
| }, | ||
| "url": { | ||
| "paths": [ | ||
| { | ||
| "path": "/_query_rules/{ruleset_id}", | ||
| "methods": [ | ||
| "PUT" | ||
| ], | ||
| "parts": { | ||
| "ruleset_id": { | ||
| "type": "string", | ||
| "description": "The unique identifier of the ruleset to be created or updated." | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| "params": { | ||
| "create": { | ||
| "type": "boolean", | ||
| "description": "If true, requires that a query_ruleset with the specified resource_id does not already exist. (default: false)" | ||
| } | ||
| }, | ||
| "body": { | ||
| "description": "The query ruleset configuration, including `rules`", | ||
| "required": true | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
|
|
||
|
|
||
| --- | ||
| 'Create Query Ruleset': | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Since only the |
||
| - do: | ||
| query_ruleset.put: | ||
| ruleset_id: test-ruleset | ||
| body: | ||
| ruleset_id: test-ruleset | ||
| rules: | ||
| - rule_id: query-rule-id1 | ||
| type: pinned | ||
| criteria: | ||
| - type: exact | ||
| metadata: query_string | ||
| value: elastic | ||
| actions: | ||
| ids: | ||
| - 'id1' | ||
| - 'id2' | ||
| - rule_id: query-rule-id2 | ||
| type: pinned | ||
| criteria: | ||
| - type: exact | ||
| metadata: query_string | ||
| value: kibana | ||
| actions: | ||
| docs: | ||
| - '_index': 'test-index1' | ||
| '_id': 'id3' | ||
| - '_index': 'test-index2' | ||
| '_id': 'id4' | ||
|
|
||
| - match: { result: 'created' } | ||
|
|
||
| --- | ||
| 'Create Query Ruleset - Resource already exists': | ||
| - do: | ||
| query_ruleset.put: | ||
| create: true | ||
| ruleset_id: test-query-ruleset-recreating | ||
| body: | ||
| ruleset_id: 'test-query-ruleset-recreating' | ||
| rules: | ||
| rule_id: 'test-rule-1' | ||
| type: 'pinned' | ||
| criteria: | ||
| type: 'exact' | ||
| metadata: 'query_string' | ||
| value: 'elastic' | ||
| actions: | ||
| ids: | ||
| - 'id1' | ||
|
|
||
| - match: { result: 'created' } | ||
|
|
||
| - do: | ||
| catch: conflict | ||
| query_ruleset.put: | ||
| create: true | ||
| ruleset_id: test-query-ruleset-recreating | ||
| body: | ||
| ruleset_id: 'test-query-ruleset-recreating' | ||
| rules: | ||
| rule_id: 'test-rule-1' | ||
| type: 'pinned' | ||
| criteria: | ||
| type: 'exact' | ||
| metadata: 'query_string' | ||
| value: 'elastic' | ||
| actions: | ||
| ids: | ||
| - 'id2' | ||
|
|
||
| - match: { error.type: 'version_conflict_engine_exception' } | ||
|
|
||
| --- | ||
| 'Create Query Ruleset - Insufficient privilege': | ||
| - skip: | ||
| features: headers | ||
|
|
||
| - do: | ||
| catch: forbidden | ||
| headers: { Authorization: "Basic ZW50c2VhcmNoLXVzZXI6ZW50c2VhcmNoLXVzZXItcGFzc3dvcmQ=" } # user | ||
| query_ruleset.put: | ||
| ruleset_id: forbidden-query-ruleset | ||
| create: true | ||
| body: | ||
| ruleset_id: 'forbidden-query-ruleset' | ||
| rules: | ||
| rule_id: 'test-rule-1' | ||
| type: 'pinned' | ||
| criteria: | ||
| type: 'exact' | ||
| metadata: 'query_string' | ||
| value: 'elastic' | ||
| actions: | ||
| ids: | ||
| - 'id1' | ||
| - 'id2' | ||
|
|
||
| - match: { error.type: 'security_exception' } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| import org.elasticsearch.common.settings.Setting; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.settings.SettingsFilter; | ||
| import org.elasticsearch.common.util.FeatureFlag; | ||
| import org.elasticsearch.env.Environment; | ||
| import org.elasticsearch.env.NodeEnvironment; | ||
| import org.elasticsearch.indices.SystemIndexDescriptor; | ||
|
|
@@ -52,6 +53,9 @@ | |
| import org.elasticsearch.xpack.application.analytics.action.TransportPutAnalyticsCollectionAction; | ||
| import org.elasticsearch.xpack.application.analytics.ingest.AnalyticsEventIngestConfig; | ||
| import org.elasticsearch.xpack.application.rules.QueryRulesIndexService; | ||
| import org.elasticsearch.xpack.application.rules.action.PutQueryRulesetAction; | ||
| import org.elasticsearch.xpack.application.rules.action.RestPutQueryRulesetAction; | ||
| import org.elasticsearch.xpack.application.rules.action.TransportPutQueryRulesetAction; | ||
| import org.elasticsearch.xpack.application.search.SearchApplicationIndexService; | ||
| import org.elasticsearch.xpack.application.search.action.DeleteSearchApplicationAction; | ||
| import org.elasticsearch.xpack.application.search.action.GetSearchApplicationAction; | ||
|
|
@@ -76,6 +80,7 @@ | |
| import org.elasticsearch.xpack.core.action.XPackInfoFeatureAction; | ||
| import org.elasticsearch.xpack.core.action.XPackUsageFeatureAction; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
@@ -90,17 +95,18 @@ public class EnterpriseSearch extends Plugin implements ActionPlugin, SystemInde | |
|
|
||
| public static final String BEHAVIORAL_ANALYTICS_API_ENDPOINT = APPLICATION_API_ENDPOINT + "/analytics"; | ||
|
|
||
| public static final String QUERY_RULES_API_ENDPOINT = "_query_rules"; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I purposely kept this external to the |
||
|
|
||
| private static final Logger logger = LogManager.getLogger(EnterpriseSearch.class); | ||
|
|
||
| public static final String FEATURE_NAME = "ent_search"; | ||
|
|
||
| private final boolean enabled; | ||
|
|
||
| private final boolean queryRulesEnabled; | ||
| private static final FeatureFlag QUERY_RULES_FEATURE_FLAG = new FeatureFlag("query_rules"); | ||
|
|
||
| public EnterpriseSearch(Settings settings) { | ||
| this.enabled = XPackSettings.ENTERPRISE_SEARCH_ENABLED.get(settings); | ||
| this.queryRulesEnabled = XPackSettings.ENTERPRISE_SEARCH_QUERY_RULES_ENABLED.get(settings); | ||
| } | ||
|
|
||
| protected XPackLicenseState getLicenseState() { | ||
|
|
@@ -114,20 +120,29 @@ protected XPackLicenseState getLicenseState() { | |
| if (enabled == false) { | ||
| return List.of(usageAction, infoAction); | ||
| } | ||
| return List.of( | ||
| new ActionHandler<>(PutAnalyticsCollectionAction.INSTANCE, TransportPutAnalyticsCollectionAction.class), | ||
| new ActionHandler<>(GetAnalyticsCollectionAction.INSTANCE, TransportGetAnalyticsCollectionAction.class), | ||
| new ActionHandler<>(DeleteAnalyticsCollectionAction.INSTANCE, TransportDeleteAnalyticsCollectionAction.class), | ||
| new ActionHandler<>(PostAnalyticsEventAction.INSTANCE, TransportPostAnalyticsEventAction.class), | ||
| new ActionHandler<>(DeleteSearchApplicationAction.INSTANCE, TransportDeleteSearchApplicationAction.class), | ||
| new ActionHandler<>(GetSearchApplicationAction.INSTANCE, TransportGetSearchApplicationAction.class), | ||
| new ActionHandler<>(ListSearchApplicationAction.INSTANCE, TransportListSearchApplicationAction.class), | ||
| new ActionHandler<>(PutSearchApplicationAction.INSTANCE, TransportPutSearchApplicationAction.class), | ||
| new ActionHandler<>(QuerySearchApplicationAction.INSTANCE, TransportQuerySearchApplicationAction.class), | ||
| new ActionHandler<>(RenderSearchApplicationQueryAction.INSTANCE, TransportRenderSearchApplicationQueryAction.class), | ||
| usageAction, | ||
| infoAction | ||
|
|
||
| final List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> actionHandlers = new ArrayList<>( | ||
| List.of( | ||
| new ActionHandler<>(PutAnalyticsCollectionAction.INSTANCE, TransportPutAnalyticsCollectionAction.class), | ||
| new ActionHandler<>(GetAnalyticsCollectionAction.INSTANCE, TransportGetAnalyticsCollectionAction.class), | ||
| new ActionHandler<>(DeleteAnalyticsCollectionAction.INSTANCE, TransportDeleteAnalyticsCollectionAction.class), | ||
| new ActionHandler<>(PostAnalyticsEventAction.INSTANCE, TransportPostAnalyticsEventAction.class), | ||
| new ActionHandler<>(DeleteSearchApplicationAction.INSTANCE, TransportDeleteSearchApplicationAction.class), | ||
| new ActionHandler<>(GetSearchApplicationAction.INSTANCE, TransportGetSearchApplicationAction.class), | ||
| new ActionHandler<>(ListSearchApplicationAction.INSTANCE, TransportListSearchApplicationAction.class), | ||
| new ActionHandler<>(PutSearchApplicationAction.INSTANCE, TransportPutSearchApplicationAction.class), | ||
| new ActionHandler<>(QuerySearchApplicationAction.INSTANCE, TransportQuerySearchApplicationAction.class), | ||
| new ActionHandler<>(RenderSearchApplicationQueryAction.INSTANCE, TransportRenderSearchApplicationQueryAction.class), | ||
| usageAction, | ||
| infoAction | ||
| ) | ||
| ); | ||
|
|
||
| if (QUERY_RULES_FEATURE_FLAG.isEnabled()) { | ||
| actionHandlers.add(new ActionHandler<>(PutQueryRulesetAction.INSTANCE, TransportPutQueryRulesetAction.class)); | ||
| } | ||
|
|
||
| return Collections.unmodifiableList(actionHandlers); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -144,18 +159,27 @@ public List<RestHandler> getRestHandlers( | |
| if (enabled == false) { | ||
| return Collections.emptyList(); | ||
| } | ||
| return List.of( | ||
| new RestGetSearchApplicationAction(getLicenseState()), | ||
| new RestListSearchApplicationAction(getLicenseState()), | ||
| new RestPutSearchApplicationAction(getLicenseState()), | ||
| new RestDeleteSearchApplicationAction(getLicenseState()), | ||
| new RestQuerySearchApplicationAction(getLicenseState()), | ||
| new RestPutAnalyticsCollectionAction(getLicenseState()), | ||
| new RestGetAnalyticsCollectionAction(getLicenseState()), | ||
| new RestDeleteAnalyticsCollectionAction(getLicenseState()), | ||
| new RestPostAnalyticsEventAction(getLicenseState()), | ||
| new RestRenderSearchApplicationQueryAction(getLicenseState()) | ||
|
|
||
| final List<RestHandler> restHandlers = new ArrayList<>( | ||
| List.of( | ||
| new RestPutAnalyticsCollectionAction(getLicenseState()), | ||
| new RestGetAnalyticsCollectionAction(getLicenseState()), | ||
| new RestDeleteAnalyticsCollectionAction(getLicenseState()), | ||
| new RestPostAnalyticsEventAction(getLicenseState()), | ||
| new RestDeleteSearchApplicationAction(getLicenseState()), | ||
| new RestGetSearchApplicationAction(getLicenseState()), | ||
| new RestListSearchApplicationAction(getLicenseState()), | ||
| new RestPutSearchApplicationAction(getLicenseState()), | ||
| new RestQuerySearchApplicationAction(getLicenseState()), | ||
| new RestRenderSearchApplicationQueryAction(getLicenseState()) | ||
| ) | ||
| ); | ||
|
|
||
| if (QUERY_RULES_FEATURE_FLAG.isEnabled()) { | ||
| restHandlers.add(new RestPutQueryRulesetAction(getLicenseState())); | ||
| } | ||
|
|
||
| return Collections.unmodifiableList(restHandlers); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -192,7 +216,7 @@ public Collection<Object> createComponents( | |
|
|
||
| @Override | ||
| public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings settings) { | ||
| if (queryRulesEnabled) { | ||
| if (QUERY_RULES_FEATURE_FLAG.isEnabled()) { | ||
| return Arrays.asList( | ||
| SearchApplicationIndexService.getSystemIndexDescriptor(), | ||
| QueryRulesIndexService.getSystemIndexDescriptor() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this from the start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, but it's a paradigm we already use with search applications and it wasn't a big lift to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say YAGNI - let's not add anything we're not sure to need, to avoid testing / documenting / maintaining it. It's easy to add later if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the time I'd agree, but there is a chance of accidentally overwriting an entire ruleset, with no history. This could be a guardrail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we added that option to Search Applications for a very specific UI use case, and I don't feel this is something we need as a rule. I don't see this as a deal breaker for the PR, just a warning that we probably should not add it at least yet. It adds code and tests that we are not needing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I don't feel strongly enough that it has to stay in to argue more 😉 - I will plan to remove it, but I'll do so in a follow up PR (I want to get this merged to unblock the next one).