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

add should_create_single_alert_for_findings field to security-analytics #757

Merged
merged 5 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ data class IndexExecutionContext(
val updatedIndexNames: List<String>,
val concreteIndexNames: List<String>,
val conflictingFields: List<String>,
val docIds: List<String>? = emptyList()
val docIds: List<String>? = emptyList(),
val findingIds: List<String>? = emptyList()
Copy link
Member

Choose a reason for hiding this comment

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

why are we adding this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just to add findingIds to IndexExecutionContext.

) : Writeable, ToXContent {

@Throws(IOException::class)
Expand All @@ -34,7 +35,8 @@ data class IndexExecutionContext(
updatedIndexNames = sin.readStringList(),
concreteIndexNames = sin.readStringList(),
conflictingFields = sin.readStringList(),
docIds = sin.readOptionalStringList()
docIds = sin.readOptionalStringList(),
findingIds = sin.readOptionalStringList()
)

override fun writeTo(out: StreamOutput?) {
Expand All @@ -47,6 +49,7 @@ data class IndexExecutionContext(
out.writeStringCollection(concreteIndexNames)
out.writeStringCollection(conflictingFields)
out.writeOptionalStringCollection(docIds)
out.writeOptionalStringCollection(findingIds)
}

override fun toXContent(builder: XContentBuilder?, params: ToXContent.Params?): XContentBuilder {
Expand All @@ -60,6 +63,7 @@ data class IndexExecutionContext(
.field("concrete_index_names", concreteIndexNames)
.field("conflicting_fields", conflictingFields)
.field("doc_ids", docIds)
.field("finding_ids", findingIds)
.endObject()
return builder
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ data class Monitor(
val uiMetadata: Map<String, Any>,
val dataSources: DataSources = DataSources(),
val deleteQueryIndexInEveryRun: Boolean? = false,
val shouldPersistFindingsAndAlerts: Boolean? = false,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be true by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the field is named now as shouldCreateSingleAlertForFindings. & its set to False by default.

val owner: String? = "alerting"
) : ScheduledJob {

Expand Down Expand Up @@ -112,6 +113,7 @@ data class Monitor(
DataSources()
},
deleteQueryIndexInEveryRun = sin.readOptionalBoolean(),
shouldPersistFindingsAndAlerts = sin.readOptionalBoolean(),
owner = sin.readOptionalString()
)

Expand Down Expand Up @@ -172,6 +174,7 @@ data class Monitor(
if (uiMetadata.isNotEmpty()) builder.field(UI_METADATA_FIELD, uiMetadata)
builder.field(DATA_SOURCES_FIELD, dataSources)
builder.field(DELETE_QUERY_INDEX_IN_EVERY_RUN_FIELD, deleteQueryIndexInEveryRun)
builder.field(SHOULD_PERSIST_FINDINGS_AND_ALERTS_FIELD, shouldPersistFindingsAndAlerts)
builder.field(OWNER_FIELD, owner)
if (params.paramAsBoolean("with_type", false)) builder.endObject()
return builder.endObject()
Expand Down Expand Up @@ -224,6 +227,7 @@ data class Monitor(
out.writeBoolean(dataSources != null) // for backward compatibility with pre-existing monitors which don't have datasources field
dataSources.writeTo(out)
out.writeOptionalBoolean(deleteQueryIndexInEveryRun)
out.writeOptionalBoolean(shouldPersistFindingsAndAlerts)
out.writeOptionalString(owner)
}

Expand All @@ -245,6 +249,7 @@ data class Monitor(
const val DATA_SOURCES_FIELD = "data_sources"
const val ENABLED_TIME_FIELD = "enabled_time"
const val DELETE_QUERY_INDEX_IN_EVERY_RUN_FIELD = "delete_query_index_in_every_run"
const val SHOULD_PERSIST_FINDINGS_AND_ALERTS_FIELD = "should_persist_findings_and_alerts"
const val OWNER_FIELD = "owner"
val MONITOR_TYPE_PATTERN = Pattern.compile("[a-zA-Z0-9_]{5,25}")

Expand Down Expand Up @@ -274,6 +279,7 @@ data class Monitor(
val inputs: MutableList<Input> = mutableListOf()
var dataSources = DataSources()
var deleteQueryIndexInEveryRun = false
var delegateMonitor = false
var owner = "alerting"

XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, xcp.currentToken(), xcp)
Expand Down Expand Up @@ -332,6 +338,11 @@ data class Monitor(
} else {
xcp.booleanValue()
}
SHOULD_PERSIST_FINDINGS_AND_ALERTS_FIELD -> delegateMonitor = if (xcp.currentToken() == XContentParser.Token.VALUE_NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

thinking of a scenario where this field is null
shouldn't we persist alerts and findings by default but we have initilalized var delegateMonitor = false and wont end up persisting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as #757 (comment)

delegateMonitor
} else {
xcp.booleanValue()
}
OWNER_FIELD -> owner = if (xcp.currentToken() == XContentParser.Token.VALUE_NULL) owner else xcp.text()
else -> {
xcp.skipChildren()
Expand Down Expand Up @@ -360,6 +371,7 @@ data class Monitor(
uiMetadata,
dataSources,
deleteQueryIndexInEveryRun,
delegateMonitor,
owner
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ data class WorkflowRunContext(
val workflowId: String,
val workflowMetadataId: String,
val chainedMonitorId: String?,
val matchingDocIdsPerIndex: Map<String, List<String>>,
val matchingDocIdsPerIndex: Pair<Map<String, List<String>>, List<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

is this change backward compatible?

can we revert this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed this.

val auditDelegateMonitorAlerts: Boolean
) : Writeable, ToXContentObject {
companion object {
Expand All @@ -30,15 +30,16 @@ data class WorkflowRunContext(
sin.readString(),
sin.readString(),
sin.readOptionalString(),
sin.readMap() as Map<String, List<String>>,
Pair(sin.readMap() as Map<String, List<String>>, sin.readStringList()),
sin.readBoolean()
)

override fun writeTo(out: StreamOutput) {
out.writeString(workflowId)
out.writeString(workflowMetadataId)
out.writeOptionalString(chainedMonitorId)
out.writeMap(matchingDocIdsPerIndex)
out.writeMap(matchingDocIdsPerIndex.first)
out.writeStringCollection(matchingDocIdsPerIndex.second)
out.writeBoolean(auditDelegateMonitorAlerts)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class DocLevelMonitorFanOutRequestTests {
Workflow.NO_ID,
Workflow.NO_ID,
Monitor.NO_ID,
mutableMapOf("index" to listOf("1")),
Pair(mutableMapOf("index" to listOf("1")), listOf("finding1")),
true
)
val docLevelMonitorFanOutRequest = DocLevelMonitorFanOutRequest(
Expand Down
Loading