Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Added User support for background job and API Transport Actions #272

Merged
merged 5 commits into from
Oct 20, 2020
Merged

Added User support for background job and API Transport Actions #272

merged 5 commits into from
Oct 20, 2020

Conversation

saratvemulapalli
Copy link
Contributor

*Issue #195 *

Description of changes:
Added User support for background job and API Transport Actions

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Oct 17, 2020 •
edited

Codecov Report

Merging #272 into master will decrease coverage by 0.53%.
The diff coverage is 48.84%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #272      +/-   ##
============================================
- Coverage     71.23%   70.69%   -0.54%     
- Complexity     1761     1776      +15     
============================================
  Files           185      186       +1     
  Lines          8436     8597     +161     
  Branches        722      731       +9     
============================================
+ Hits           6009     6078      +69     
- Misses         2089     2173      +84     
- Partials        338      346       +8
Flag Coverage Δ Complexity Δ
#cli 79.27% <ø> (ø) 0.00 <ø> (ø)
#plugin 69.91% <48.84%> (-0.57%) 1776.00 <22.00> (+15.00) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...stroforelasticsearch/ad/AnomalyDetectorRunner.java 10.90% <0.00%> (-0.42%) 1.00 <0.00> (ø)
...stroforelasticsearch/ad/model/AnomalyDetector.java 65.02% <0.00%> (-0.15%) 56.00 <0.00> (+1.00) ⬇️
...forelasticsearch/ad/rest/AbstractSearchAction.java 23.52% <0.00%> (-1.48%) 2.00 <0.00> (ø)
...search/ad/rest/RestIndexAnomalyDetectorAction.java 46.00% <0.00%> (-0.94%) 3.00 <0.00> (ø)
...stroforelasticsearch/ad/util/RestHandlerUtils.java 84.21% <0.00%> (-12.76%) 13.00 <0.00> (ø)
.../transport/SearchAnomalyResultTransportAction.java 34.09% <30.55%> (-43.69%) 4.00 <3.00> (+2.00) ⬇️
...ransport/SearchAnomalyDetectorTransportAction.java 35.71% <32.35%> (-42.07%) 4.00 <3.00> (+2.00) ⬇️
...oforelasticsearch/ad/AnomalyDetectorJobRunner.java 75.52% <33.33%> (-0.27%) 35.00 <0.00> (ø)
...d/transport/GetAnomalyDetectorTransportAction.java 46.87% <35.00%> (-0.38%) 9.00 <0.00> (ø)
...est/handler/IndexAnomalyDetectorActionHandler.java 48.70% <43.75%> (-2.69%) 28.00 <2.00> (+1.00) ⬇️
... and 16 more

ohltyler
ohltyler approved these changes Oct 18, 2020
View reviewed changes
Copy link
Contributor

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

ylwu-amzn
ylwu-amzn reviewed Oct 18, 2020
View reviewed changes
src/main/java/com/amazon/opendistroforelasticsearch/ad/AnomalyDetectorPlugin.java Outdated
try {
this.restClient = new SecureRestClientBuilder(settings, environment.configFile()).build();
} catch (IOException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to log.error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point sure.

ylwu-amzn
ylwu-amzn reviewed Oct 18, 2020
View reviewed changes
src/main/java/com/amazon/opendistroforelasticsearch/ad/util/RestHandlerUtils.java
@@ -101,6 +106,14 @@ public static String validateAnomalyDetector(AnomalyDetector anomalyDetector, in
return null;
}

public static void addFilter(User user, SearchSourceBuilder searchSourceBuilder, String fieldName) {
TermsQueryBuilder filterBackendRoles = QueryBuilders.termsQuery(fieldName, user.getBackendRoles());
if (searchSourceBuilder.query() instanceof BoolQueryBuilder) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the searchSourceBuilder.query() is not a BoolQueryBuilder ?
For example, one user want to count all anomaly result. That will be just an aggregation query. For this case we will not add this filterBackendRoles term query to the original query, then user can get total anomaly result count for all users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any updates? If it's not easy to build a flexible solution. How about we start from supporting BoolQueryBuilder like alerting ? We can support more query types in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, its getting tricky support other queries, I spent some time to figure out if there is a cleaner way of doing it.
Like you said, lets start supporting the bool query and we can add support to the other queries if needed later.

I will handle queries with an error for now.

@ohltyler ohltyler added the enhancement New feature or request label Oct 19, 2020
@saratvemulapalli saratvemulapalli added the feature new feature label Oct 19, 2020
ylwu-amzn
ylwu-amzn reviewed Oct 19, 2020
View reviewed changes
...com/amazon/opendistroforelasticsearch/ad/transport/SearchAnomalyDetectorTransportAction.java

private void addFilter(User user, SearchSourceBuilder searchSourceBuilder, String fieldName) {
TermsQueryBuilder filterBackendRoles = QueryBuilders.termsQuery(fieldName, user.getBackendRoles());
// For search detector queries, non BoolQuery is only used to find if the new detector name being created is
Copy link
Contributor

@ylwu-amzn ylwu-amzn Oct 19, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user use curl to search detector, and they don't use bool query, they can search other user's detector data. So we may have security leak.

How about we build a dedicated API to check if detector name is unique or not. The input is simple a detector name string, the output is true or false. And we control access in java code by ourselves. This needs to change AD Kibana code to switch to this new API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure that sounds good. I will work with Tyler and work on these changes.

Copy link
Contributor

@ylwu-amzn ylwu-amzn Oct 19, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Will approve this one. Put the new API in another PR.
BTW, we should throw error for search detectors if not bool query.

@saratvemulapalli saratvemulapalli merged commit 48e7f74 into master Oct 20, 2020
@saratvemulapalli saratvemulapalli deleted the fgac-background-job branch October 20, 2020 01:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@ohltyler ohltyler ohltyler approved these changes

@skkosuri-amzn skkosuri-amzn Awaiting requested review from skkosuri-amzn

Assignees
No one assigned
Labels
enhancement New feature or request feature new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@saratvemulapalli @ylwu-amzn @skkosuri-amzn @ohltyler