Local uri input monitor support #369

AWSHurneyt · 2021-04-02T23:26:45Z

*Issue #, if available: #347

*Description of changes:

Renamed assets from #347 from HttpInput to LocalUriInput
Implemented data class, LocalUriInput, to support creation of local cluster monitors using URI input.
Implemented logic for a map of supported API and their supported JSON payloads. Local cluster monitors for unsupported API will fail to execute.
Implemented logic to support creation of local cluster monitors for cluster health, and cluster stats using a LocalUriInput.

Implementing unit tests as a separate task.

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

…r creating ClusterHealth and ClusterStats monitors.

…yt/alerting into local-uri-input-monitor-support � Conflicts: � alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/settings/SupportedApiSettings.kt

thalurur · 2021-04-13T19:09:39Z

.../main/kotlin/com/amazon/opendistroforelasticsearch/alerting/settings/SupportedApiSettings.kt

+         */
+        // TODO: Currently set to TRUE for testing purposes.
+        //  Should likely be set to FALSE by default.
+        private var supportedApiListEnabled = true


just curious: why do we need this?

This is essentially an on/off switch that I implemented at the beginning of the project. If a user did not want to check the API called by the monitor against the allow list, this value could be set to FALSE. That would allow users to create monitors for any API that this feature support. At the moment, it doesn't serve much of a purpose; but it could be more valuable as more API become supported.

We could remove this value if we feel it's extraneous.

If I understand this correctly this is something intended to be specified by the user and if this is set to false then they cannot use the LocalUrlInput feature.

I am not sure I understand how this is configurable by the end user at the moment. Do you think this can be defined as a cluster setting so user can turn it on off as it seems fit for them? or if this is not configurable at the moment suggest going with removing this as it serves no purpose and in future when needed to add the customization we can introduce the cluster setting

If configured to false, users could still use the LocalUriInput feature; but the allow list wouldn't be checked for the requested API before creating and executing the monitor. At the moment, the allow list exposes both of the supported API; but as support for more API is added, a cluster owner could essentially configure this setting to false to allow access to any supported API, regardless of whether they are on the allow list.

Your comment about configuring via settings makes sense though. This will be easy enough to reimplement if there's a need in the future.

Removed this value.

thalurur · 2021-04-13T19:11:01Z

.../main/kotlin/com/amazon/opendistroforelasticsearch/alerting/settings/SupportedApiSettings.kt

+         * @param localUriInput The [LocalUriInput] to validate.
+         * @return The path that was validated.
+         */
+        fun validateLocalUriInput(localUriInput: LocalUriInput): String {


Do you think it makes sense to rename this to something like resolveToUriPath` as this method seems to be doing more than just validation, its also seems to be converting LocalUriInput`` to a path.

Also do we really need to convert to a uri path? Looks the entity being used later on is transport action request do you think it makes sense to just build and return ActionRequest from this method instead, so callers don't need to worry about building the action request with all the correct attributes?

fun resolveToActionRequest(localUriInput: LocalUriInput): ActionRequest? { .... }

That makes sense. I'll refactor this method to resolveToActionRequest and return the ActionRequest. Thank you for the feedback! I've also made the validatePath method public for use in the Monitor class.

thalurur · 2021-04-13T19:13:44Z

...kotlin/com/amazon/opendistroforelasticsearch/alerting/util/SupportedApiSettingsExtensions.kt

+    if (path == SupportedApiSettings.CLUSTER_HEALTH_PATH) {
+        return client.admin().cluster().health(ClusterHealthRequest()).get()
+    }
+    if (path == SupportedApiSettings.CLUSTER_STATS_PATH) {
+        return client.admin().cluster().clusterStats(ClusterStatsRequest()).get()
+    }
+    throw IllegalArgumentException("Unsupported API: $path")


minor:
you can use when statement instead

when (path) { is SupportedApiSettings.CLUSTER_HEALTH_PATH -> return .. is SupportedApiSettings.CLUSTER_STATS_PATH -> return ... else -> throw IllegalArgumentException }

Thank you for the tip! The syntax is a little different from your example due to some compiler warnings, but I refactored this to when.

thalurur · 2021-04-13T19:16:39Z

...kotlin/com/amazon/opendistroforelasticsearch/alerting/util/SupportedApiSettingsExtensions.kt

+    if (this is ClusterHealthResponse) {
+        return this.convertToMap()
+    }
+    if (this is ClusterStatsResponse) {
+        return this.convertToMap()
+    }
+    throw IllegalArgumentException("Unsupported ActionResponse type: ${this.javaClass.name}")


minor: using when

when (this) { is ClusterHealthResponse, is ClusterStatsResponse -> return this.convertToMap() else -> throw IllegalArgumentException }

Thank you for the tip! The syntax is a little different from your example due to some compiler warnings, but I refactored this to when.

…sed on PR feedback

…t and port is always 9200

thalurur · 2021-04-14T00:41:36Z

core/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/core/model/LocalUriInput.kt

+    }
+
+    /**
+     * Helper function to confirm at least [url], or [scheme]+[host]+[port]+[path]+[query_params] is defined.


is it either or alteast?

if its atleast then I'd think we need to check that either url or [scheme]+[host]+[port]+[path]+[query_params] is not empty and if they are both not empty then check they both are creating same URI?

if it either then just check either url or [scheme]+[host]+[port]+[path]+[query_params] is not empty but never both are empty

Looking at the code I am thinking either is what we are trying to do, but it seem to miss the validation for case that both are empty. We are only checking both are not not empty.

Ah, thank you for pointing that out. Yes, we want either the url or the other fields to be provided, not both so we don't need to worry about a user providing two different URLs in one request. I've refactored this method to include that check.

…sed on PR feedback

…yt/alerting into local-uri-input-monitor-support � Conflicts: � alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/MonitorRunnerIT.kt � alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/TestHelpers.kt � core/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/core/model/LocalUriInput.kt � core/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/core/model/LocalUriInputTests.kt

…nd added unit test. Implemented method to redact fields from a response, and added unit tests

…iList using a JSON resource file

lezzago · 2021-04-20T22:36:04Z

.../main/kotlin/com/amazon/opendistroforelasticsearch/alerting/settings/SupportedApiSettings.kt

+            val supportedJsonPayloads = SupportedApiSettings::class.java.getResource(RESOURCE_FILE)
+            @Suppress("UNCHECKED_CAST")
+            if (supportedJsonPayloads != null) supportedApiList =
+                XContentHelper.convertToMap(
+                    JsonXContent.jsonXContent, supportedJsonPayloads.readText(), false) as HashMap<String, Map<String, ArrayList<String>>>


Do we not want to make this a configurable setting for the customers? What is in the supported_json_payloads.json can be the default values.

For P0, the supportedApiList is configured using the JSON; users will need to make changes to the resource file in order to update that list. As the plan was to only support ClusterHealth and ClusterStats for P0, an API to configure the list was not a high priority for P0.

lezzago · 2021-04-20T23:52:55Z

.../main/kotlin/com/amazon/opendistroforelasticsearch/alerting/settings/SupportedApiSettings.kt

+            validatePath(path)
+            return when (path) {
+                CLUSTER_HEALTH_PATH -> ClusterHealthRequest()
+                CLUSTER_STATS_PATH -> ClusterStatsRequest()
+                else -> throw IllegalArgumentException("Unsupported API: $path")
+            }


Is there a need to have validatePath() if we have the when validation?

Ah, that's a really good point. Thank you for pointing that out! I removed the call to validatePath.

…ePath, and javadocs to be more informative.

…xtensions::redactFieldsFromResponse output.

AWSHurneyt added 2 commits March 29, 2021 18:49

Implemented LocalUriInput and extension class

dc0c449

Implemented SupportedApiSettings and extensions. Implemented logic fo…

f33f9a4

…r creating ClusterHealth and ClusterStats monitors.

AWSHurneyt requested review from rishabhmaurya, tlfeng and lezzago April 2, 2021 23:28

AWSHurneyt added 2 commits April 2, 2021 16:54

Implemented SupportedApiSettings and extensions. Implemented logic fo…

546b384

…r creating ClusterHealth and ClusterStats monitors.

Merge branch 'local-uri-input-monitor-support' of github.com:AWSHurne…

ab030dc

…yt/alerting into local-uri-input-monitor-support � Conflicts: � alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/settings/SupportedApiSettings.kt

AWSHurneyt requested review from skkosuri-amzn and qreshi April 6, 2021 18:08

Implemented unit tests and integration tests for LocalUriInput

c9a73fd

thalurur reviewed Apr 13, 2021

View reviewed changes

AWSHurneyt added 3 commits April 13, 2021 17:17

Refactored SupportedApiSettings and SupportedApiSettingsExtensions ba…

708d00b

…sed on PR feedback

Implemented unit tests and integration tests for LocalUriInput

3ec3d71

Refactored LocalUriInput and tests to confirm host is always localhos…

aa789dd

…t and port is always 9200

thalurur reviewed Apr 14, 2021

View reviewed changes

AWSHurneyt added 4 commits April 14, 2021 15:56

Refactored SupportedApiSettings and SupportedApiSettingsExtensions ba…

e819bfb

…sed on PR feedback

Refactored LocalUriInput to support receiving only a path as input, a…

6111184

…nd added unit test. Implemented method to redact fields from a response, and added unit tests

Implemented support for configuring SupportedApiSettings::supportedAp…

28d3fba

…iList using a JSON resource file

AWSHurneyt requested a review from thalurur April 19, 2021 17:04

lezzago reviewed Apr 21, 2021

View reviewed changes

AWSHurneyt added 2 commits April 22, 2021 13:33

Refactored resolveToActionRequest to remove redundant call to validat…

c05884a

…ePath, and javadocs to be more informative.

Refactored an erroneous value assignment in the SupportedApiSettingsE…

1d03c61

…xtensions::redactFieldsFromResponse output.

AWSHurneyt mentioned this pull request May 20, 2021

Added support for LocalUriInput monitors #380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local uri input monitor support #369

Local uri input monitor support #369

AWSHurneyt commented Apr 2, 2021 •

edited

thalurur Apr 13, 2021

AWSHurneyt Apr 13, 2021

thalurur Apr 14, 2021

AWSHurneyt Apr 14, 2021

thalurur Apr 13, 2021 •

edited

AWSHurneyt Apr 13, 2021

thalurur Apr 13, 2021

AWSHurneyt Apr 13, 2021

thalurur Apr 13, 2021 •

edited

AWSHurneyt Apr 13, 2021

thalurur Apr 14, 2021 •

edited

AWSHurneyt Apr 14, 2021

lezzago Apr 20, 2021

AWSHurneyt Apr 22, 2021

lezzago Apr 20, 2021

AWSHurneyt Apr 22, 2021

Local uri input monitor support #369

Are you sure you want to change the base?

Local uri input monitor support #369

Conversation

AWSHurneyt commented Apr 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalurur Apr 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalurur Apr 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalurur Apr 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AWSHurneyt commented Apr 2, 2021 •

edited

thalurur Apr 13, 2021 •

edited

thalurur Apr 13, 2021 •

edited

thalurur Apr 14, 2021 •

edited