Skip to content

Commit

Permalink
Support disable cookie for webhook
Browse files Browse the repository at this point in the history 
Signed-off-by: Hailong Cui <[email protected]>
  • Loading branch information
@Hailong-am
Hailong-am committed Dec 7, 2023
1 parent 1163d54 commit a877e18
Show file tree
Hide file tree
Showing 5 changed files with 249 additions and 10 deletions.
30 changes: 22 additions & 8 deletions ...ns/core/src/main/kotlin/org/opensearch/notifications/core/client/DestinationHttpClient.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import org.apache.hc.client5.http.impl.classic.CloseableHttpClient
import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse
import org.apache.hc.client5.http.impl.classic.HttpClientBuilder
import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager
import org.apache.hc.client5.http.io.HttpClientConnectionManager
import org.apache.hc.core5.http.HttpEntity
import org.apache.hc.core5.http.HttpResponse
import org.apache.hc.core5.http.io.entity.EntityUtils
Expand Down Expand Up @@ -45,13 +46,21 @@ import kotlin.collections.HashSet
class DestinationHttpClient {

private val httpClient: CloseableHttpClient
private val httpClientDisableCookie: CloseableHttpClient

constructor() {
this.httpClient = createHttpClient()
val connectionManager = PoolingHttpClientConnectionManager()
connectionManager.maxTotal = PluginSettings.maxConnections
connectionManager.defaultMaxPerRoute = PluginSettings.maxConnectionsPerRoute

// share same connection pool
this.httpClient = createHttpClient(connectionManager, false)
this.httpClientDisableCookie = createHttpClient(connectionManager, true)
}
@OpenForTesting
constructor(httpClient: CloseableHttpClient) {
this.httpClient = httpClient
this.httpClientDisableCookie = httpClient
}

companion object {
Expand All @@ -70,23 +79,24 @@ class DestinationHttpClient {
)
)

private fun createHttpClient(): CloseableHttpClient {
private fun createHttpClient(connectionManager: HttpClientConnectionManager, disableCookie: Boolean): CloseableHttpClient {
val config: RequestConfig = RequestConfig.custom()
.setConnectTimeout(Timeout.ofMilliseconds(PluginSettings.connectionTimeout.toLong()))
.setConnectionRequestTimeout(Timeout.ofMilliseconds(PluginSettings.connectionTimeout.toLong()))
.setResponseTimeout(Timeout.ofMilliseconds(PluginSettings.socketTimeout.toLong()))
.build()
val connectionManager = PoolingHttpClientConnectionManager()
connectionManager.maxTotal = PluginSettings.maxConnections
connectionManager.defaultMaxPerRoute = PluginSettings.maxConnectionsPerRoute

return HttpClientBuilder.create()
val builder = HttpClientBuilder.create()
.setDefaultRequestConfig(config)
.setConnectionManager(connectionManager)
.setRetryStrategy(DefaultHttpRequestRetryStrategy())
.useSystemProperties()
.disableRedirectHandling()
.build()

if (disableCookie) {
builder.disableCookieManagement()
}
return builder.build()
}
}

Expand Down Expand Up @@ -125,7 +135,11 @@ class DestinationHttpClient {
val entity = StringEntity(buildRequestBody(destination, message), StandardCharsets.UTF_8)
httpRequest.entity = entity

return httpClient.execute(httpRequest)
return if (PluginSettings.disableHttpCookie) {
httpClientDisableCookie.execute(httpRequest)
} else {
httpClient.execute(httpRequest)
}
}

private fun constructHttpRequest(method: String, url: String): HttpUriRequestBase {
Expand Down
36 changes: 35 additions & 1 deletion ...ications/core/src/main/kotlin/org/opensearch/notifications/core/setting/PluginSettings.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ internal object PluginSettings {
*/
private const val EMAIL_KEY_PREFIX = "$KEY_PREFIX.email"

private const val WEBHOOK_KEY_PREFIX = "$KEY_PREFIX.webhook"

/**
* Legacy Email Destination setting prefix used by Alerting.
* Defining this here to be used as a fallback for the Notification plugin setting to account for migrated Email Destinations.
Expand Down Expand Up @@ -66,6 +68,11 @@ internal object PluginSettings {
*/
private const val MAX_CONNECTIONS_KEY = "$HTTP_CONNECTION_KEY_PREFIX.max_connections"

/**
* Settings key for disable http cookie for webhook
*/
private const val DISABLE_HTTP_COOKIE_KEY = "$WEBHOOK_KEY_PREFIX.disable_http_cookie"

/**
* Settings Key prefix for max http connection per route.
*/
Expand Down Expand Up @@ -241,6 +248,12 @@ internal object PluginSettings {
@Volatile
var clusterName: String

/**
* flag to enable/disable cookie management for http connections of webhooks
*/
@Volatile
var disableHttpCookie: Boolean

/**
* Destination Settings
*/
Expand Down Expand Up @@ -278,6 +291,7 @@ internal object PluginSettings {
hostDenyList = settings?.getAsList(HOST_DENY_LIST_KEY, null) ?: DEFAULT_HOST_DENY_LIST
clusterName = settings?.get(CLUSTER_NAME, DEFAULT_CLUSTER_NAME) ?: DEFAULT_CLUSTER_NAME
destinationSettings = if (settings != null) loadDestinationSettings(settings) else DEFAULT_DESTINATION_SETTINGS
disableHttpCookie = settings?.getAsBoolean(DISABLE_HTTP_COOKIE_KEY, false) ?: false

defaultSettings = mapOf(
EMAIL_SIZE_LIMIT_KEY to emailSizeLimit.toString(DECIMAL_RADIX),
Expand Down Expand Up @@ -309,6 +323,12 @@ internal object PluginSettings {
NodeScope, Dynamic
)

val DISABLE_HTTP_COOKIE: Setting<Boolean> = Setting.boolSetting(
DISABLE_HTTP_COOKIE_KEY,
false,
NodeScope, Dynamic
)

val MAX_CONNECTIONS_PER_ROUTE: Setting<Int> = Setting.intSetting(
MAX_CONNECTIONS_PER_ROUTE_KEY,
defaultSettings[MAX_CONNECTIONS_PER_ROUTE_KEY]!!.toInt(),
Expand Down Expand Up @@ -412,7 +432,8 @@ internal object PluginSettings {
TOOLTIP_SUPPORT,
HOST_DENY_LIST,
EMAIL_USERNAME,
EMAIL_PASSWORD
EMAIL_PASSWORD,
DISABLE_HTTP_COOKIE,
)
}
/**
Expand All @@ -431,6 +452,7 @@ internal object PluginSettings {
hostDenyList = HOST_DENY_LIST.get(clusterService.settings)
destinationSettings = loadDestinationSettings(clusterService.settings)
clusterName = clusterService.clusterName.value()
disableHttpCookie = DISABLE_HTTP_COOKIE.get(clusterService.settings)
}

/**
Expand All @@ -454,6 +476,13 @@ internal object PluginSettings {
log.debug("$LOG_PREFIX:$MAX_CONNECTIONS_KEY -autoUpdatedTo-> $clusterMaxConnections")
maxConnections = clusterMaxConnections
}

val disableWebhookHttpCookie = clusterService.clusterSettings.get(DISABLE_HTTP_COOKIE)
if (disableWebhookHttpCookie != null) {
log.debug("$LOG_PREFIX:$DISABLE_HTTP_COOKIE -autoUpdatedTo-> $disableWebhookHttpCookie")
this.disableHttpCookie = disableWebhookHttpCookie
}

val clusterMaxConnectionsPerRoute = clusterService.clusterSettings.get(MAX_CONNECTIONS_PER_ROUTE)
if (clusterMaxConnectionsPerRoute != null) {
log.debug("$LOG_PREFIX:$MAX_CONNECTIONS_PER_ROUTE_KEY -autoUpdatedTo-> $clusterMaxConnectionsPerRoute")
Expand Down Expand Up @@ -517,6 +546,10 @@ internal object PluginSettings {
maxConnections = it
log.info("$LOG_PREFIX:$MAX_CONNECTIONS_KEY -updatedTo-> $it")
}
clusterService.clusterSettings.addSettingsUpdateConsumer(DISABLE_HTTP_COOKIE) {
disableHttpCookie = it
log.info("$LOG_PREFIX:${this.DISABLE_HTTP_COOKIE_KEY} -updatedTo-> $it")
}
clusterService.clusterSettings.addSettingsUpdateConsumer(MAX_CONNECTIONS_PER_ROUTE) {
maxConnectionsPerRoute = it
log.info("$LOG_PREFIX:$MAX_CONNECTIONS_PER_ROUTE_KEY -updatedTo-> $it")
Expand Down Expand Up @@ -595,5 +628,6 @@ internal object PluginSettings {
allowedConfigTypes = DEFAULT_ALLOWED_CONFIG_TYPES
tooltipSupport = DEFAULT_TOOLTIP_SUPPORT
hostDenyList = DEFAULT_HOST_DENY_LIST
disableHttpCookie = false
}
}
22 changes: 21 additions & 1 deletion ...ns/core/src/test/kotlin/org/opensearch/notifications/core/settings/PluginSettingsTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ internal class PluginSettingsTests {
private val httpMaxConnectionKey = "$httpKeyPrefix.max_connections"
private val httpMaxConnectionPerRouteKey = "$httpKeyPrefix.max_connection_per_route"
private val httpConnectionTimeoutKey = "$httpKeyPrefix.connection_timeout"
private val disableWebhookCookieKey = "$keyPrefix.webhook.disable_http_cookie"
private val httpSocketTimeoutKey = "$httpKeyPrefix.socket_timeout"
private val legacyAlertingHostDenyListKey = "opendistro.destination.host.deny_list"
private val alertingHostDenyListKey = "plugins.destination.host.deny_list"
Expand Down Expand Up @@ -93,7 +94,8 @@ internal class PluginSettingsTests {
PluginSettings.SOCKET_TIMEOUT_MILLISECONDS,
PluginSettings.ALLOWED_CONFIG_TYPES,
PluginSettings.TOOLTIP_SUPPORT,
PluginSettings.HOST_DENY_LIST
PluginSettings.HOST_DENY_LIST,
PluginSettings.DISABLE_HTTP_COOKIE,
)
)
)
Expand Down Expand Up @@ -134,6 +136,10 @@ internal class PluginSettingsTests {
"opensearch",
PluginSettings.clusterName
)
Assertions.assertEquals(
false,
PluginSettings.disableHttpCookie
)
}

@Test
Expand All @@ -148,6 +154,7 @@ internal class PluginSettingsTests {
.putList(httpHostDenyListKey, listOf("sample"))
.putList(allowedConfigTypeKey, listOf("slack"))
.put(tooltipSupportKey, false)
.put(disableWebhookCookieKey, true)
.put(clusterNameKey, "OpenSearch OsDomainNameUpdate")
.build()

Expand All @@ -166,6 +173,7 @@ internal class PluginSettingsTests {
PluginSettings.ALLOWED_CONFIG_TYPES,
PluginSettings.TOOLTIP_SUPPORT,
PluginSettings.HOST_DENY_LIST,
PluginSettings.DISABLE_HTTP_COOKIE,
ClusterName.CLUSTER_NAME_SETTING
)
)
Expand Down Expand Up @@ -207,6 +215,10 @@ internal class PluginSettingsTests {
"OpenSearch OsDomainNameUpdate",
clusterService.clusterSettings.get(ClusterName.CLUSTER_NAME_SETTING).value()
)
Assertions.assertEquals(
true,
clusterService.clusterSettings.get(PluginSettings.DISABLE_HTTP_COOKIE)
)
}

@Test
Expand All @@ -227,6 +239,7 @@ internal class PluginSettingsTests {
PluginSettings.ALLOWED_CONFIG_TYPES,
PluginSettings.TOOLTIP_SUPPORT,
PluginSettings.HOST_DENY_LIST,
PluginSettings.DISABLE_HTTP_COOKIE,
ClusterName.CLUSTER_NAME_SETTING
)
)
Expand Down Expand Up @@ -268,6 +281,10 @@ internal class PluginSettingsTests {
"Cluster [opensearch]",
clusterService.clusterSettings.get(ClusterName.CLUSTER_NAME_SETTING).toString()
)
Assertions.assertEquals(
false,
clusterService.clusterSettings.get(PluginSettings.DISABLE_HTTP_COOKIE)
)
}

@Test
Expand Down Expand Up @@ -295,6 +312,7 @@ internal class PluginSettingsTests {
PluginSettings.LEGACY_ALERTING_HOST_DENY_LIST,
PluginSettings.ALERTING_HOST_DENY_LIST,
PluginSettings.HOST_DENY_LIST,
PluginSettings.DISABLE_HTTP_COOKIE,
ClusterName.CLUSTER_NAME_SETTING
)
)
Expand Down Expand Up @@ -330,6 +348,7 @@ internal class PluginSettingsTests {
PluginSettings.LEGACY_ALERTING_HOST_DENY_LIST,
PluginSettings.ALERTING_HOST_DENY_LIST,
PluginSettings.HOST_DENY_LIST,
PluginSettings.DISABLE_HTTP_COOKIE,
ClusterName.CLUSTER_NAME_SETTING
)
)
Expand Down Expand Up @@ -364,6 +383,7 @@ internal class PluginSettingsTests {
PluginSettings.LEGACY_ALERTING_HOST_DENY_LIST,
PluginSettings.ALERTING_HOST_DENY_LIST,
PluginSettings.HOST_DENY_LIST,
PluginSettings.DISABLE_HTTP_COOKIE,
ClusterName.CLUSTER_NAME_SETTING
)
)
Expand Down
1 change: 1 addition & 0 deletions notifications/notifications/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ integTest {
if (usingRemoteCluster) {
filter {
excludeTestsMatching "org.opensearch.integtest.send.SendTestMessageWithMockServerIT"
excludeTestsMatching "org.opensearch.integtest.send.SendWithMockServerCookieIT"
}
}
}
Expand Down
Loading

0 comments on commit a877e18

Please sign in to comment.