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

Add logged-on User details to the Monitor and Destination #255

Merged
merged 8 commits into from
Sep 25, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class GetMonitorRequest : ActionRequest {
out.writeEnum(method)
if (srcContext != null) {
out.writeBoolean(true)
srcContext?.writeTo(out)
srcContext.writeTo(out)
} else {
out.writeBoolean(false)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe cleaner?

out.writeBoolean(srcContext != null)
srcContext?.writeTo(out)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like it 👍

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ data class Monitor(
override val schedule: Schedule,
override val lastUpdateTime: Instant,
override val enabledTime: Instant?,
val user: User?,
val schemaVersion: Int = NO_SCHEMA_VERSION,
val inputs: List<Input>,
val triggers: List<Trigger>,
Expand Down Expand Up @@ -81,9 +82,12 @@ data class Monitor(
sin.readLong(), // version
sin.readString(), // name
sin.readBoolean(), // enabled
Schedule.readFrom(sin),
Schedule.readFrom(sin), // schedule
sin.readInstant(), // lastUpdateTime
sin.readOptionalInstant(), // enabledTime
if (sin.readBoolean()) {
User.readFrom(sin) // user
Copy link
Contributor

Choose a reason for hiding this comment

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

Why User.readFrom(sin) and not just User(sin) and remove that readFrom function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason. const is more clean, agree.

} else null,
sin.readInt(), // schemaVersion
sin.readList(::SearchInput), // inputs
sin.readList(::Trigger), // triggers
Expand All @@ -104,6 +108,7 @@ data class Monitor(
builder.field(TYPE_FIELD, type)
.field(SCHEMA_VERSION_FIELD, schemaVersion)
.field(NAME_FIELD, name)
.optionalUserField(user)
.field(ENABLED_FIELD, enabled)
.optionalTimeField(ENABLED_TIME_FIELD, enabledTime)
.field(SCHEDULE_FIELD, schedule)
Expand All @@ -117,8 +122,15 @@ data class Monitor(

override fun fromDocument(id: String, version: Long): Monitor = copy(id = id, version = version)

private fun XContentBuilder.optionalUserField(user: User?): XContentBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Move to ElasticExtensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

if (user == null) {
return nullField(USER_FIELD)
}
return this.field(USER_FIELD, user)
}

@Throws(IOException::class)
fun writeTo(out: StreamOutput) {
override fun writeTo(out: StreamOutput) {
out.writeString(id)
out.writeLong(version)
out.writeString(name)
Expand All @@ -131,6 +143,12 @@ data class Monitor(
schedule.writeTo(out)
out.writeInstant(lastUpdateTime)
out.writeOptionalInstant(enabledTime)
if (user != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Same as above

out.writeBoolean(user != null)
user?.writeTo(out)

out.writeBoolean(true)
user.writeTo(out)
} else {
out.writeBoolean(false)
}
out.writeInt(schemaVersion)
out.writeCollection(inputs)
out.writeCollection(triggers)
Expand All @@ -142,6 +160,7 @@ data class Monitor(
const val TYPE_FIELD = "type"
const val SCHEMA_VERSION_FIELD = "schema_version"
const val NAME_FIELD = "name"
const val USER_FIELD = "user"
const val ENABLED_FIELD = "enabled"
const val SCHEDULE_FIELD = "schedule"
const val TRIGGERS_FIELD = "triggers"
Expand All @@ -163,6 +182,7 @@ data class Monitor(
@Throws(IOException::class)
fun parse(xcp: XContentParser, id: String = NO_ID, version: Long = NO_VERSION): Monitor {
lateinit var name: String
var user: User? = null
lateinit var schedule: Schedule
var lastUpdateTime: Instant? = null
var enabledTime: Instant? = null
Expand All @@ -180,6 +200,7 @@ data class Monitor(
when (fieldName) {
SCHEMA_VERSION_FIELD -> schemaVersion = xcp.intValue()
NAME_FIELD -> name = xcp.text()
USER_FIELD -> user = User.parse(xcp)
ENABLED_FIELD -> enabled = xcp.booleanValue()
SCHEDULE_FIELD -> schedule = Schedule.parse(xcp)
INPUTS_FIELD -> {
Expand Down Expand Up @@ -215,6 +236,7 @@ data class Monitor(
requireNotNull(schedule) { "Monitor schedule is null" },
lastUpdateTime ?: Instant.now(),
enabledTime,
user,
schemaVersion,
inputs.toList(),
triggers.toList(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package com.amazon.opendistroforelasticsearch.alerting.model
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing License


import org.elasticsearch.common.io.stream.StreamInput
import org.elasticsearch.common.io.stream.StreamOutput
import org.elasticsearch.common.io.stream.Writeable
import org.elasticsearch.common.xcontent.ToXContent
import org.elasticsearch.common.xcontent.ToXContentObject
import org.elasticsearch.common.xcontent.XContentBuilder
import org.elasticsearch.common.xcontent.XContentParser
import org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken
import java.io.IOException

data class User(
Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression we wanted to decouple the roles from the user when storing this on the Monitor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having name, roles, backend_roles as user (sub) object is much clean. Checks are simpler. monitor.user

val name: String,
val backendRoles: List<String>,
val roles: List<String>,
val customAttNames: List<String>
) : Writeable, ToXContentObject {

@Throws(IOException::class)
constructor(sin: StreamInput): this(
sin.readString(), // name
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I prefer using the named arguments instead of comments
i.e.

@Throws(IOException::class)
constructor(sin: StreamInput): this(
    name = sin.readString(),
    backendRoles = sin.readStringList(),
    roles = sin.readStringList(),
    customAttNames = sin.readStringList() 
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. Still kotlin newbie

sin.readStringList(), // backendRoles
sin.readStringList(), // roles
sin.readStringList() // customAttNames
)

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
builder.startObject()
.field(NAME_FIELD, name)
.field(BACKEND_ROLES_FIELD, backendRoles)
.field(ROLES_FIELD, roles)
.field(CUSTOM_ATTRIBUTE_NAMES_FIELD, customAttNames)
return builder.endObject()
}
@Throws(IOException::class)
override fun writeTo(out: StreamOutput) {
out.writeString(name)
out.writeStringCollection(backendRoles)
out.writeStringCollection(roles)
out.writeStringCollection(customAttNames)
}

companion object {
const val NAME_FIELD = "name"
const val BACKEND_ROLES_FIELD = "backend_roles"
const val ROLES_FIELD = "roles"
const val CUSTOM_ATTRIBUTE_NAMES_FIELD = "custom_attribute_names"

@JvmStatic
@Throws(IOException::class)
fun parse(xcp: XContentParser): User {
var name = ""
val backendRoles: MutableList<String> = mutableListOf()
val roles: MutableList<String> = mutableListOf()
val customAttNames: MutableList<String> = mutableListOf()

while (xcp.nextToken() != XContentParser.Token.END_OBJECT) {
val fieldName = xcp.currentName()
xcp.nextToken()
when (fieldName) {
NAME_FIELD -> name = xcp.text()
ROLES_FIELD -> {
ensureExpectedToken(XContentParser.Token.START_ARRAY, xcp.currentToken(), xcp::getTokenLocation)
while (xcp.nextToken() != XContentParser.Token.END_ARRAY) {
roles.add(xcp.text())
}
}
BACKEND_ROLES_FIELD -> {
ensureExpectedToken(XContentParser.Token.START_ARRAY, xcp.currentToken(), xcp::getTokenLocation)
while (xcp.nextToken() != XContentParser.Token.END_ARRAY) {
backendRoles.add(xcp.text())
}
}
CUSTOM_ATTRIBUTE_NAMES_FIELD -> {
ensureExpectedToken(XContentParser.Token.START_ARRAY, xcp.currentToken(), xcp::getTokenLocation)
while (xcp.nextToken() != XContentParser.Token.END_ARRAY) {
customAttNames.add(xcp.text())
}
}
}
}
return User(name, backendRoles, roles, customAttNames)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want any validation of the values here? Or is a User("", listOf(), listOf(), listOf()) a valid user?

}

@JvmStatic
@Throws(IOException::class)
fun readFrom(sin: StreamInput): User {
Copy link
Contributor

Choose a reason for hiding this comment

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

Function seems redundant if you can just call User(sin) instead.

return User(sin)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.amazon.opendistroforelasticsearch.alerting.destination.response.Desti
import com.amazon.opendistroforelasticsearch.alerting.elasticapi.convertToMap
import com.amazon.opendistroforelasticsearch.alerting.elasticapi.instant
import com.amazon.opendistroforelasticsearch.alerting.elasticapi.optionalTimeField
import com.amazon.opendistroforelasticsearch.alerting.model.User
import com.amazon.opendistroforelasticsearch.alerting.util.DestinationType
import com.amazon.opendistroforelasticsearch.alerting.util.IndexUtils.Companion.NO_SCHEMA_VERSION
import org.apache.logging.log4j.LogManager
Expand All @@ -46,6 +47,7 @@ data class Destination(
val schemaVersion: Int = NO_SCHEMA_VERSION,
val type: DestinationType,
val name: String,
val user: User?,
val lastUpdateTime: Instant,
val chime: Chime?,
val slack: Slack?,
Expand All @@ -57,6 +59,7 @@ data class Destination(
if (params.paramAsBoolean("with_type", false)) builder.startObject(DESTINATION)
builder.field(TYPE_FIELD, type.value)
.field(NAME_FIELD, name)
.optionalUserField(user)
.field(SCHEMA_VERSION, schemaVersion)
.optionalTimeField(LAST_UPDATE_TIME_FIELD, lastUpdateTime)
.field(type.value, constructResponseForDestinationType(type))
Expand All @@ -68,13 +71,26 @@ data class Destination(
return toXContent(builder, ToXContent.EMPTY_PARAMS)
}

private fun XContentBuilder.optionalUserField(user: User?): XContentBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Duplication of one in Monitor, just move to ElasticExtensions and use in both

if (user == null) {
return nullField(USER_FIELD)
}
return this.field(USER_FIELD, user)
}

@Throws(IOException::class)
fun writeTo(out: StreamOutput) {
out.writeString(id)
out.writeLong(version)
out.writeInt(schemaVersion)
out.writeEnum(type)
out.writeString(name)
if (user != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as other comments

out.writeBoolean(true)
user.writeTo(out)
} else {
out.writeBoolean(false)
}
out.writeInstant(lastUpdateTime)
if (chime != null) {
out.writeBoolean(true)
Expand All @@ -100,6 +116,7 @@ data class Destination(
const val DESTINATION = "destination"
const val TYPE_FIELD = "type"
const val NAME_FIELD = "name"
const val USER_FIELD = "user"
const val NO_ID = ""
const val NO_VERSION = 1L
const val SCHEMA_VERSION = "schema_version"
Expand All @@ -117,6 +134,7 @@ data class Destination(
@Throws(IOException::class)
fun parse(xcp: XContentParser, id: String = NO_ID, version: Long = NO_VERSION): Destination {
lateinit var name: String
var user: User? = null
lateinit var type: String
var slack: Slack? = null
var chime: Chime? = null
Expand All @@ -131,6 +149,7 @@ data class Destination(

when (fieldName) {
NAME_FIELD -> name = xcp.text()
USER_FIELD -> user = User.parse(xcp)
TYPE_FIELD -> {
type = xcp.text()
val allowedTypes = DestinationType.values().map { it.value }
Expand Down Expand Up @@ -164,6 +183,7 @@ data class Destination(
schemaVersion,
DestinationType.valueOf(type.toUpperCase(Locale.ROOT)),
requireNotNull(name) { "Destination name is null" },
user,
lastUpdateTime ?: Instant.now(),
chime,
slack,
Expand All @@ -179,6 +199,9 @@ data class Destination(
sin.readInt(), // schemaVersion
sin.readEnum(DestinationType::class.java), // type
sin.readString(), // name
if (sin.readBoolean()) {
User.readFrom(sin) // user
} else null,
sin.readInstant(), // lastUpdateTime
Chime.readFrom(sin), // chime
Slack.readFrom(sin), // slack
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
return Destination(
type = DestinationType.TEST_ACTION,
name = "test",
user = randomUser(),
lastUpdateTime = Instant.now(),
chime = null,
slack = null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.amazon.opendistroforelasticsearch.alerting.model.ActionRunResult
import com.amazon.opendistroforelasticsearch.alerting.model.InputRunResults
import com.amazon.opendistroforelasticsearch.alerting.model.MonitorRunResult
import com.amazon.opendistroforelasticsearch.alerting.model.TriggerRunResult
import com.amazon.opendistroforelasticsearch.alerting.model.User
import com.amazon.opendistroforelasticsearch.alerting.model.action.Throttle
import org.apache.http.Header
import org.apache.http.HttpEntity
Expand Down Expand Up @@ -57,6 +58,7 @@ import java.time.temporal.ChronoUnit

fun randomMonitor(
name: String = ESRestTestCase.randomAlphaOfLength(10),
user: User = randomUser(),
inputs: List<Input> = listOf(SearchInput(emptyList(), SearchSourceBuilder().query(QueryBuilders.matchAllQuery()))),
schedule: Schedule = IntervalSchedule(interval = 5, unit = ChronoUnit.MINUTES),
enabled: Boolean = ESTestCase.randomBoolean(),
Expand All @@ -67,7 +69,23 @@ fun randomMonitor(
): Monitor {
return Monitor(name = name, enabled = enabled, inputs = inputs, schedule = schedule, triggers = triggers,
enabledTime = enabledTime, lastUpdateTime = lastUpdateTime,
uiMetadata = if (withMetadata) mapOf("foo" to "bar") else mapOf())
user = user, uiMetadata = if (withMetadata) mapOf("foo" to "bar") else mapOf())
}

// Monitor of older versions without security.
fun randomMonitorWithoutUser(
name: String = ESRestTestCase.randomAlphaOfLength(10),
inputs: List<Input> = listOf(SearchInput(emptyList(), SearchSourceBuilder().query(QueryBuilders.matchAllQuery()))),
schedule: Schedule = IntervalSchedule(interval = 5, unit = ChronoUnit.MINUTES),
enabled: Boolean = ESTestCase.randomBoolean(),
triggers: List<Trigger> = (1..randomInt(10)).map { randomTrigger() },
enabledTime: Instant? = if (enabled) Instant.now().truncatedTo(ChronoUnit.MILLIS) else null,
lastUpdateTime: Instant = Instant.now().truncatedTo(ChronoUnit.MILLIS),
withMetadata: Boolean = false
): Monitor {
return Monitor(name = name, enabled = enabled, inputs = inputs, schedule = schedule, triggers = triggers,
enabledTime = enabledTime, lastUpdateTime = lastUpdateTime,
user = null, uiMetadata = if (withMetadata) mapOf("foo" to "bar") else mapOf())
}

fun randomTrigger(
Expand Down Expand Up @@ -164,6 +182,14 @@ fun Monitor.toJsonString(): String {
return this.toXContent(builder).string()
}

fun randomUser(): User {
return User("joe", listOf("ops", "backup"), listOf("all_access"), listOf("test_attr=test"))
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't really seem random 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

randomized

}

fun randomUserEmpty(): User {
return User("", listOf(), listOf(), listOf())
}

/**
* Wrapper for [RestClient.performRequest] which was deprecated in ES 6.5 and is used in tests. This provides
* a single place to suppress deprecation warnings. This will probably need further work when the API is removed entirely
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package com.amazon.opendistroforelasticsearch.alerting.action

import com.amazon.opendistroforelasticsearch.alerting.core.model.CronSchedule
import com.amazon.opendistroforelasticsearch.alerting.model.Monitor
import com.amazon.opendistroforelasticsearch.alerting.randomUser
import org.elasticsearch.common.io.stream.BytesStreamOutput
import org.elasticsearch.common.io.stream.StreamInput
import org.elasticsearch.rest.RestStatus
Expand Down Expand Up @@ -47,7 +48,7 @@ class GetMonitorResponseTests : ESTestCase() {
val cronSchedule = CronSchedule(cronExpression, ZoneId.of("Asia/Kolkata"), testInstance)
val req = GetMonitorResponse("1234", 1L, 2L, 0L, RestStatus.OK,
Monitor("123", 0L, "test-monitor", true, cronSchedule, Instant.now(),
Instant.now(), 0, mutableListOf(), mutableListOf(), mutableMapOf()))
Instant.now(), randomUser(), 0, mutableListOf(), mutableListOf(), mutableMapOf()))
assertNotNull(req)

val out = BytesStreamOutput()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package com.amazon.opendistroforelasticsearch.alerting.action

import com.amazon.opendistroforelasticsearch.alerting.model.destination.Chime
import com.amazon.opendistroforelasticsearch.alerting.model.destination.Destination
import com.amazon.opendistroforelasticsearch.alerting.randomUser
import com.amazon.opendistroforelasticsearch.alerting.util.DestinationType
import org.elasticsearch.action.support.WriteRequest
import org.elasticsearch.common.io.stream.BytesStreamOutput
Expand All @@ -41,6 +42,7 @@ class IndexDestinationRequestTests : ESTestCase() {
1,
DestinationType.CHIME,
"TestChimeDest",
randomUser(),
Instant.now(),
Chime("test.com"),
null,
Expand Down Expand Up @@ -76,6 +78,7 @@ class IndexDestinationRequestTests : ESTestCase() {
1,
DestinationType.CHIME,
"TestChimeDest",
randomUser(),
Instant.now(),
Chime("test.com"),
null,
Expand Down
Loading