Skip to content
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

Remove user from the responses #201

Merged
merged 3 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions DEVELOPER_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ When launching a cluster using one of the above commands, logs are placed in `al

1. Setup a local opensearch cluster with security plugin.

- `./gradlew :alerting:integTestRunner -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername=es-integrationtest -Dhttps=true -Dsecurity=true -Duser=admin -Dpassword=admin`
- `./gradlew :alerting:integTest -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername=opensearch -Dhttps=true -Dsecurity=true -Duser=admin -Dpassword=admin`

- `./gradlew :alerting:integTestRunner -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername=es-integrationtest -Dhttps=true -Dsecurity=true -Duser=admin -Dpassword=admin --tests "org.opensearch.alerting.MonitorRunnerIT.test execute monitor returns search result"`
- `./gradlew :alerting:integTest -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername=opensearch -Dhttps=true -Dsecurity=true -Duser=admin -Dpassword=admin --tests "org.opensearch.alerting.MonitorRunnerIT.test execute monitor returns search result"`


#### Building from the IDE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import org.opensearch.client.Client
import org.opensearch.common.bytes.BytesReference
import org.opensearch.common.xcontent.LoggingDeprecationHandler
import org.opensearch.common.xcontent.NamedXContentRegistry
import org.opensearch.common.xcontent.ToXContent
import org.opensearch.common.xcontent.XContentFactory
import org.opensearch.common.xcontent.XContentHelper
import org.opensearch.common.xcontent.XContentParser
Expand Down Expand Up @@ -276,7 +275,7 @@ class AlertService(
listOf<DocWriteRequest<*>>(
IndexRequest(AlertIndices.ALERT_INDEX)
.routing(alert.monitorId)
.source(alert.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS))
.source(alert.toXContentWithUser(XContentFactory.jsonBuilder()))
.id(if (alert.id != Alert.NO_ID) alert.id else null)
)
}
Expand All @@ -287,7 +286,7 @@ class AlertService(
listOf<DocWriteRequest<*>>(
IndexRequest(AlertIndices.ALERT_INDEX)
.routing(alert.monitorId)
.source(alert.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS))
.source(alert.toXContentWithUser(XContentFactory.jsonBuilder()))
.id(if (alert.id != Alert.NO_ID) alert.id else null)
)
} else {
Expand All @@ -305,7 +304,7 @@ class AlertService(
if (alertIndices.isHistoryEnabled()) {
IndexRequest(AlertIndices.HISTORY_WRITE_INDEX)
.routing(alert.monitorId)
.source(alert.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS))
.source(alert.toXContentWithUser(XContentFactory.jsonBuilder()))
.id(alert.id)
} else null
)
Expand Down Expand Up @@ -349,7 +348,7 @@ class AlertService(
}
IndexRequest(AlertIndices.ALERT_INDEX)
.routing(alert.monitorId)
.source(alert.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS))
.source(alert.toXContentWithUser(XContentFactory.jsonBuilder()))
}.toMutableList()

if (requestsToRetry.isEmpty()) return listOf()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import org.opensearch.client.Client
import org.opensearch.common.bytes.BytesReference
import org.opensearch.common.xcontent.LoggingDeprecationHandler
import org.opensearch.common.xcontent.NamedXContentRegistry
import org.opensearch.common.xcontent.ToXContent
import org.opensearch.common.xcontent.XContentFactory
import org.opensearch.common.xcontent.XContentHelper
import org.opensearch.common.xcontent.XContentParser
Expand Down Expand Up @@ -88,7 +87,7 @@ suspend fun moveAlerts(client: Client, monitorId: String, monitor: Monitor? = nu
.source(
Alert.parse(alertContentParser(hit.sourceRef), hit.id, hit.version)
.copy(state = Alert.State.DELETED)
.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS)
.toXContentWithUser(XContentFactory.jsonBuilder())
)
.version(hit.version)
.versionType(VersionType.EXTERNAL_GTE)
Expand Down
15 changes: 13 additions & 2 deletions alerting/src/main/kotlin/org/opensearch/alerting/model/Alert.kt
Original file line number Diff line number Diff line change
Expand Up @@ -297,15 +297,26 @@ data class Alert(
}

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
return createXContentBuilder(builder, true)
}

fun toXContentWithUser(builder: XContentBuilder): XContentBuilder {
return createXContentBuilder(builder, false)
}
private fun createXContentBuilder(builder: XContentBuilder, secure: Boolean): XContentBuilder {
builder.startObject()
.field(ALERT_ID_FIELD, id)
.field(ALERT_VERSION_FIELD, version)
.field(MONITOR_ID_FIELD, monitorId)
.field(SCHEMA_VERSION_FIELD, schemaVersion)
.field(MONITOR_VERSION_FIELD, monitorVersion)
.field(MONITOR_NAME_FIELD, monitorName)
.optionalUserField(MONITOR_USER_FIELD, monitorUser)
.field(TRIGGER_ID_FIELD, triggerId)

if (!secure) {
builder.optionalUserField(MONITOR_USER_FIELD, monitorUser)
}

builder.field(TRIGGER_ID_FIELD, triggerId)
.field(TRIGGER_NAME_FIELD, triggerName)
.field(STATE_FIELD, state)
.field(ERROR_MESSAGE_FIELD, errorMessage)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,24 +141,32 @@ data class Monitor(
}
}

fun toXContent(builder: XContentBuilder): XContentBuilder {
return toXContent(builder, ToXContent.EMPTY_PARAMS)
}

/** Returns a representation of the monitor suitable for passing into painless and mustache scripts. */
fun asTemplateArg(): Map<String, Any> {
return mapOf(_ID to id, _VERSION to version, NAME_FIELD to name, ENABLED_FIELD to enabled)
}

fun toXContentWithUser(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
return createXContentBuilder(builder, params, false)
}

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
return createXContentBuilder(builder, params, true)
}

private fun createXContentBuilder(builder: XContentBuilder, params: ToXContent.Params, secure: Boolean): XContentBuilder {
builder.startObject()
if (params.paramAsBoolean("with_type", false)) builder.startObject(type)
builder.field(TYPE_FIELD, type)
.field(SCHEMA_VERSION_FIELD, schemaVersion)
.field(NAME_FIELD, name)
.field(MONITOR_TYPE_FIELD, monitorType)
.optionalUserField(USER_FIELD, user)
.field(ENABLED_FIELD, enabled)

if (!secure) {
builder.optionalUserField(USER_FIELD, user)
}

builder.field(ENABLED_FIELD, enabled)
.optionalTimeField(ENABLED_TIME_FIELD, enabledTime)
.field(SCHEDULE_FIELD, schedule)
.field(INPUTS_FIELD, inputs.toTypedArray())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import org.opensearch.alerting.destination.message.ChimeMessage
import org.opensearch.alerting.destination.message.CustomWebhookMessage
import org.opensearch.alerting.destination.message.EmailMessage
import org.opensearch.alerting.destination.message.SlackMessage
import org.opensearch.alerting.destination.response.DestinationResponse
import org.opensearch.alerting.elasticapi.convertToMap
import org.opensearch.alerting.elasticapi.instant
import org.opensearch.alerting.elasticapi.optionalTimeField
Expand Down Expand Up @@ -74,21 +73,31 @@ data class Destination(
) : ToXContent {

override fun toXContent(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
return createXContentBuilder(builder, params, true)
}

fun toXContentWithUser(builder: XContentBuilder, params: ToXContent.Params): XContentBuilder {
return createXContentBuilder(builder, params, false)
}
private fun createXContentBuilder(builder: XContentBuilder, params: ToXContent.Params, secure: Boolean): XContentBuilder {
builder.startObject()
if (params.paramAsBoolean("with_type", false)) builder.startObject(DESTINATION)
builder.field(ID_FIELD, id)
.field(TYPE_FIELD, type.value)
.field(NAME_FIELD, name)
.optionalUserField(USER_FIELD, user)
.field(SCHEMA_VERSION, schemaVersion)

if (!secure) {
builder.optionalUserField(USER_FIELD, user)
}

builder.field(SCHEMA_VERSION, schemaVersion)
.field(SEQ_NO_FIELD, seqNo)
.field(PRIMARY_TERM_FIELD, primaryTerm)
.optionalTimeField(LAST_UPDATE_TIME_FIELD, lastUpdateTime)
.field(type.value, constructResponseForDestinationType(type))
if (params.paramAsBoolean("with_type", false)) builder.endObject()
return builder.endObject()
}

fun toXContent(builder: XContentBuilder): XContentBuilder {
return toXContent(builder, ToXContent.EMPTY_PARAMS)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ class TransportIndexDestinationAction @Inject constructor(
val destination = request.destination.copy(schemaVersion = IndexUtils.scheduledJobIndexSchemaVersion)
val indexRequest = IndexRequest(ScheduledJob.SCHEDULED_JOBS_INDEX)
.setRefreshPolicy(request.refreshPolicy)
.source(destination.toXContent(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.source(destination.toXContentWithUser(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.setIfSeqNo(request.seqNo)
.setIfPrimaryTerm(request.primaryTerm)
.timeout(indexTimeout)
Expand Down Expand Up @@ -282,7 +282,7 @@ class TransportIndexDestinationAction @Inject constructor(
val indexDestination = request.destination.copy(schemaVersion = IndexUtils.scheduledJobIndexSchemaVersion)
val indexRequest = IndexRequest(ScheduledJob.SCHEDULED_JOBS_INDEX)
.setRefreshPolicy(request.refreshPolicy)
.source(indexDestination.toXContent(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.source(indexDestination.toXContentWithUser(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.id(request.destinationId)
.setIfSeqNo(request.seqNo)
.setIfPrimaryTerm(request.primaryTerm)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ class TransportIndexMonitorAction @Inject constructor(
request.monitor = request.monitor.copy(schemaVersion = IndexUtils.scheduledJobIndexSchemaVersion)
val indexRequest = IndexRequest(SCHEDULED_JOBS_INDEX)
.setRefreshPolicy(request.refreshPolicy)
.source(request.monitor.toXContent(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.source(request.monitor.toXContentWithUser(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.setIfSeqNo(request.seqNo)
.setIfPrimaryTerm(request.primaryTerm)
.timeout(indexTimeout)
Expand Down Expand Up @@ -470,7 +470,7 @@ class TransportIndexMonitorAction @Inject constructor(
request.monitor = request.monitor.copy(schemaVersion = IndexUtils.scheduledJobIndexSchemaVersion)
val indexRequest = IndexRequest(SCHEDULED_JOBS_INDEX)
.setRefreshPolicy(request.refreshPolicy)
.source(request.monitor.toXContent(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.source(request.monitor.toXContentWithUser(jsonBuilder(), ToXContent.MapParams(mapOf("with_type" to "true"))))
.id(request.monitorId)
.setIfSeqNo(request.seqNo)
.setIfPrimaryTerm(request.primaryTerm)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
response.entity.content
).map()
assertUserNull(monitorJson as HashMap<String, Any>)
return monitor.copy(id = monitorJson["_id"] as String, version = (monitorJson["_version"] as Int).toLong())
}

Expand All @@ -140,6 +141,8 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
response.entity.content
).map()
assertUserNull(destinationJson as HashMap<String, Any>)

return destination.copy(
id = destinationJson["_id"] as String,
version = (destinationJson["_version"] as Int).toLong(),
Expand Down Expand Up @@ -171,6 +174,8 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
response.entity.content
).map()
assertUserNull(destinationJson as HashMap<String, Any>)

return destination.copy(id = destinationJson["_id"] as String, version = (destinationJson["_version"] as Int).toLong())
}

Expand Down Expand Up @@ -311,6 +316,7 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
response.entity.content
).map()
assertUserNull(destinationJson as HashMap<String, Any>)
return (destinationJson["destinations"] as List<Any?>)[0] as Map<String, Any>
}

Expand Down Expand Up @@ -392,14 +398,16 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
protected fun createAlert(alert: Alert): Alert {
val response = adminClient().makeRequest(
"POST", "/${AlertIndices.ALERT_INDEX}/_doc?refresh=true&routing=${alert.monitorId}",
emptyMap(), alert.toHttpEntity()
emptyMap(), alert.toHttpEntityWithUser()
)
assertEquals("Unable to create a new alert", RestStatus.CREATED, response.restStatus())

val alertJson = jsonXContent.createParser(
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
response.entity.content
).map()

assertNull(alertJson["monitor_user"])
return alert.copy(id = alertJson["_id"] as String, version = (alertJson["_version"] as Int).toLong())
}

Expand All @@ -418,6 +426,7 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
emptyMap(), monitor.toHttpEntity()
)
assertEquals("Unable to update a monitor", RestStatus.OK, response.restStatus())
assertUserNull(response.asMap()["monitor"] as Map<String, Any>)
return getMonitor(monitorId = monitor.id)
}

Expand All @@ -441,6 +450,8 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
"monitor" -> monitor = Monitor.parse(parser)
}
}

assertUserNull(monitor)
return monitor.copy(id = id, version = version)
}

Expand Down Expand Up @@ -526,7 +537,7 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
}

protected fun executeMonitor(client: RestClient, monitor: Monitor, params: Map<String, String> = mapOf()): Response =
client.makeRequest("POST", "$ALERTING_BASE_URI/_execute", params, monitor.toHttpEntity())
client.makeRequest("POST", "$ALERTING_BASE_URI/_execute", params, monitor.toHttpEntityWithUser())

protected fun indexDoc(index: String, id: String, doc: String, refresh: Boolean = true): Response {
val requestBody = StringEntity(doc, APPLICATION_JSON)
Expand Down Expand Up @@ -630,7 +641,16 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {

private fun Monitor.toJsonString(): String {
val builder = XContentFactory.jsonBuilder()
return shuffleXContent(toXContent(builder)).string()
return shuffleXContent(toXContent(builder, ToXContent.EMPTY_PARAMS)).string()
}

protected fun Monitor.toHttpEntityWithUser(): HttpEntity {
return StringEntity(toJsonStringWithUser(), APPLICATION_JSON)
}

private fun Monitor.toJsonStringWithUser(): String {
val builder = XContentFactory.jsonBuilder()
return shuffleXContent(toXContentWithUser(builder, ToXContent.EMPTY_PARAMS)).string()
}

protected fun Destination.toHttpEntity(): HttpEntity {
Expand Down Expand Up @@ -660,13 +680,13 @@ abstract class AlertingRestTestCase : ODFERestTestCase() {
return shuffleXContent(toXContent(builder)).string()
}

private fun Alert.toHttpEntity(): HttpEntity {
return StringEntity(toJsonString(), APPLICATION_JSON)
private fun Alert.toHttpEntityWithUser(): HttpEntity {
return StringEntity(toJsonStringWithUser(), APPLICATION_JSON)
}

private fun Alert.toJsonString(): String {
private fun Alert.toJsonStringWithUser(): String {
val builder = XContentFactory.jsonBuilder()
return shuffleXContent(toXContent(builder, ToXContent.EMPTY_PARAMS)).string()
return shuffleXContent(toXContentWithUser(builder)).string()
}

protected fun Monitor.relativeUrl() = "$ALERTING_BASE_URI/$id"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ class MonitorRunnerIT : AlertingRestTestCase() {
assertEquals("Alert not saved", 1, alerts.size)
verifyAlert(alerts.single(), monitor, ERROR)

Assert.assertTrue(alerts.single().errorMessage?.contains("The destination address is invalid") as Boolean)
Assert.assertNotNull(alerts.single().errorMessage)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check being modified? It doesn't seem like the error message should have changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the flaky test, not really part of user object changes. I have this test failing sometimes when security is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok that makes sense. Thanks.

Copy link
Contributor

@qreshi qreshi Oct 13, 2021

Choose a reason for hiding this comment

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

If this fails sometimes but your assertNotNull check doesn't, it means that there is an error message but not the one we expect. That would mean the Alert is failing for another reason. Did you get a chance to see what the error was in these scenarios?

Copy link
Contributor

Choose a reason for hiding this comment

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

@skkosuri-amzn By the way, I realized even as far back as our plugin version being 1.0.0, the Pull and Run Docker step in our Multi-node test workflow has been failing with Error response from daemon: manifest for opensearchstaging/opensearch:<version> not found: manifest unknown: manifest unknown. Is that a known issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skkosuri-amzn By the way, I realized even as far back as our plugin version being 1.0.0, the Pull and Run Docker step in our Multi-node test workflow has been failing with Error response from daemon: manifest for opensearchstaging/opensearch:<version> not found: manifest unknown: manifest unknown. Is that a known issue?

I am not aware of that. Looks like its a different one based on above error msg.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
*/
package org.opensearch.alerting

import junit.framework.TestCase.assertNull
import org.apache.http.Header
import org.apache.http.HttpEntity
import org.opensearch.alerting.aggregation.bucketselectorext.BucketSelectorExtAggregationBuilder
Expand Down Expand Up @@ -67,6 +68,7 @@ import org.opensearch.common.settings.SecureString
import org.opensearch.common.settings.Settings
import org.opensearch.common.xcontent.LoggingDeprecationHandler
import org.opensearch.common.xcontent.NamedXContentRegistry
import org.opensearch.common.xcontent.ToXContent
import org.opensearch.common.xcontent.XContentBuilder
import org.opensearch.common.xcontent.XContentFactory
import org.opensearch.common.xcontent.XContentParser
Expand Down Expand Up @@ -398,7 +400,17 @@ fun randomActionRunResult(): ActionRunResult {

fun Monitor.toJsonString(): String {
val builder = XContentFactory.jsonBuilder()
return this.toXContent(builder).string()
return this.toXContent(builder, ToXContent.EMPTY_PARAMS).string()
}

fun Monitor.toJsonStringWithUser(): String {
val builder = XContentFactory.jsonBuilder()
return this.toXContentWithUser(builder, ToXContent.EMPTY_PARAMS).string()
}

fun Alert.toJsonString(): String {
val builder = XContentFactory.jsonBuilder()
return this.toXContent(builder, ToXContent.EMPTY_PARAMS).string()
}

fun randomUser(): User {
Expand Down Expand Up @@ -494,3 +506,12 @@ fun xContentRegistry(): NamedXContentRegistry {
) + SearchModule(Settings.EMPTY, false, emptyList()).namedXContents
)
}

fun assertUserNull(map: Map<String, Any?>) {
val user = map["user"]
assertNull("User is not null", user)
}

fun assertUserNull(monitor: Monitor) {
assertNull("User is not null", monitor.user)
}
Loading
Loading