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

Add User to Alerts, add filterby backendroles #264

Merged
merged 16 commits into from
Oct 6, 2020
Merged

Conversation

skkosuri-amzn
Copy link
Contributor

@skkosuri-amzn skkosuri-amzn commented Oct 5, 2020

Issue #, if available:
#6
Description of changes:

  • Add User to Alert.
  • Added option to enable filterby backend roles. This is useful when we want to show only the monitors/destinations/alerts created by users who belong same backend role.
  • By default this filterby is disabled. To enable this
curl -X PUT "https://localhost:9200/_cluster/settings?pretty" -H 'Content-Type: application/json' -d'
{
    "persistent" : {
	"opendistro.alerting.filter_by_backend_roles" : true
    }
}
'

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@8dbefaf). Click here to learn what that means.
The diff coverage is 71.69%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #264   +/-   ##
=========================================
  Coverage          ?   80.64%           
  Complexity        ?      199           
=========================================
  Files             ?      150           
  Lines             ?     5099           
  Branches          ?      670           
=========================================
  Hits              ?     4112           
  Misses            ?      627           
  Partials          ?      360           
Impacted Files Coverage Δ Complexity Δ
...endistroforelasticsearch/alerting/MonitorRunner.kt 76.31% <0.00%> (ø) 0.00 <0.00> (?)
...erting/transport/TransportGetDestinationsAction.kt 81.81% <0.00%> (ø) 0.00 <0.00> (?)
...ch/alerting/resthandler/RestSearchMonitorAction.kt 80.48% <57.14%> (ø) 0.00 <0.00> (?)
...search/alerting/resthandler/RestGetAlertsAction.kt 85.71% <58.33%> (ø) 0.00 <0.00> (?)
.../alerting/resthandler/RestGetDestinationsAction.kt 84.09% <61.53%> (ø) 0.00 <0.00> (?)
...rch/alerting/transport/TransportGetAlertsAction.kt 81.25% <70.00%> (ø) 0.00 <0.00> (?)
...opendistroforelasticsearch/alerting/model/Alert.kt 87.50% <75.00%> (ø) 0.00 <0.00> (?)
...ndistroforelasticsearch/alerting/AlertingPlugin.kt 90.81% <100.00%> (ø) 0.00 <0.00> (?)
...relasticsearch/alerting/action/GetAlertsRequest.kt 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...icsearch/alerting/action/GetDestinationsRequest.kt 100.00% <100.00%> (ø) 0.00 <0.00> (?)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dbefaf...8af980f. Read the comment docs.

.put(OPENDISTRO_SECURITY_SSL_HTTP_PEMCERT_FILEPATH, "sample.pem")
.put(OPENDISTRO_SECURITY_SSL_HTTP_KEYSTORE_FILEPATH, "test-kirk.jks")
.put(OPENDISTRO_SECURITY_SSL_HTTP_KEYSTORE_PASSWORD, "changeit")
.put(OPENDISTRO_SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD, "changeit")
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what's the difference between the KEYSTORE_PASSWORD and KEYSTORE_KEYPASSWORD?

lezzago
lezzago previously approved these changes Oct 5, 2020
Copy link
Contributor

@lezzago lezzago left a comment

Choose a reason for hiding this comment

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

Minor refactor comments, but they not related to the current PR, so I am fine with that refactor being done in a later PR.

Comment on lines 28 to 30
val severityLevel: String
val alertState: String
val monitorId: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove these attributes since they are used for filtering the query. These query filters can be added to the attribute filter and we can change filter to be type: List<TermsQueryBuilder>

Comment on lines 36 to -38
destinationId: String?,
version: Long,
srcContext: FetchSourceContext?,
table: Table,
destinationType: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove destinationId and destinationType since they are used for filtering the query. These query filters can be added to the attribute filter and we can change filter to be type: List<TermsQueryBuilder>

Setting.Property.NodeScope, Setting.Property.Dynamic)

val FILTER_BY_BACKEND_ROLES = Setting.boolSetting(
"opendistro.alerting.filterby_backendroles",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the actual setting name match the naming change as well?

I.e

opendistro.alerting.filter_by_backend_roles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, fixed.

qreshi
qreshi previously approved these changes Oct 6, 2020
lezzago
lezzago previously approved these changes Oct 6, 2020
@skkosuri-amzn skkosuri-amzn dismissed stale reviews from lezzago and qreshi via 8af980f October 6, 2020 21:26
@skkosuri-amzn skkosuri-amzn merged commit 44abca1 into master Oct 6, 2020
tlfeng pushed a commit that referenced this pull request Feb 6, 2021
* add User to Alerts, add filterby bckroles
* Added unit tests
* rename setting option and reverted one unit test
* Fixed a typo
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants