[Custom threshold] Always pass allowLeadingWildcards as true to the KQL validation in the custom threshold rule API param validation

[maryam-saeidi]

Fixes #189072
Related #190029

Summary

This PR updates the KQL validation on the server side by passing the Kibana leadingWildcard setting as true during validation. This means that even if this configuration is disabled in Kibana, we will still allow saving such a filter in the rule, but it will fail during rule execution.
I've created a separate ticket to discuss how to apply the KQL validation correctly during API param validation. (issue)

This fix will solve the following issues:

We also have proper validation on the UI side that considers Kibana setting during validation:

Error	Leading wildcard error

[obltmachine]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
• run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

[elasticmachine]

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

[maryam-saeidi]

@jasonrhodes I am changing the validateKQLStringFilter validation to string to avoid the issue mentioned here. Alternatively, I can always pass the leadingWildcard setting as true, but there are other configs, and I am not sure if we face the same issue in the future. Any thoughts?

[maryam-saeidi]

Discussed offline: I will change the code to set the leadingWildcard to true by default for all the requests.

[jasonrhodes]

For extra clarity for my future self: the leadingWildcard value will be set to true for the validation function that runs on the server, which means the server validation phase will allow leading wildcards always, regardless of the advanced setting value. All other KQL syntax will continue to be checked and invalid syntax would trigger an error as usual.

In the case that a user submits leading wildcard KQL to the API and allows it to be saved, but the advanced setting in Kibana is set to disallow leading wildcards, the query will fail during the rule execution phase and generate an error that can be seen in the rule's event log.

This is a temporary fix meant to address this validation situation as best as we can until @elastic/response-ops is able to provide a way for us to access advanced setting values during the validation phase.

[pmuellr]

In terms of Backwards Compatibility / Zero Down Time (BWC / ZDT) aspects, this is generally a safe direction to move in. Make validation more permissive in a future version. Older versions of the rule will obviously still validate.

There's still the case of someone creating a new rule during an upgrade, and an older Kibana picking it up to run, and then failing the validation. But this would be short-lived (presumably) and the framework should handle these fine anyway (but report a validation error on run).

The other thing to think about is the NEXT version, where you add a KQL-ish validation. Now you've made it more strict, so there's a greater possibility of BWC/ZDT issues, in theory. It would be again the same scenario - the rule would fail to run but note the validation error, and presumably the UX would also show an error on the field when editing. You may well end up with rules in a failing state on the upgrade though ...

So I think we're generally good. But thought I'd think it through first ...

I'm not sure, but it doesn't feel quite right, as an option, to leave the current validator (presumably with a modification to do whatever it needs to do), and then enhance it in the future - it's still leaving some room for breaking changes. As long as the UX "does the right thing", I'm 98% happy as that would leave just API users hanging on this issue, the course of which is already fraught with peril, and presumably they are looking more closely at logs/histories so would notice ...

[maryam-saeidi]

@pmuellr Did I understand correctly that you are in favor of using the schema.string validator and not validating KQL with the loosest configuration? When Jason and I discussed it, the concern was that allowing users to rely on saving any string in this field would make it harder for us to bring back the proper validation.

[pmuellr]

Did I understand correctly that you are in favor of using the schema.string validator and not validating KQL with the loosest configuration?

That's what this PR is doing, right? On server-side anyway? That's what I thought it was doing given the title and top comment; but looking closer I see it's ADDING a validation of some sort.

So, this actually is a forward compatibility issue then! Old rules which for some reason have rule params that do not validate, will cause the rules to stop running.

When Jason and I discussed it, the concern was that allowing users to rely on saving any string in this field would make it harder for us to bring back the proper validation.

But you are currently storing unvalidated strings in here, right? So when a rule runs with a param which does not validate, the rule will exit early, failing the validation.

I may be misunderstanding what's going on here ...

[maryam-saeidi]

That's what this PR is doing, right? On server-side anyway? That's what I thought it was doing given the title and top comment; but looking closer I see it's ADDING a validation of some sort.

Sorry for the confusion; I forgot to update the PR title and description; my bad. Now it is up to date with the latest changes, thanks for the catch!

So, this actually is a forward compatibility issue then! Old rules which for some reason have rule params that do not validate, will cause the rules to stop running.

This is only at the time of rule creation/update. So, the rule should have failed even before this change, and the rule execution is the same after merging this PR. The difference is that if users save rules with invalid parameters and try to update the rule with the same invalid filter, they will receive an error during the update but no difference on execution.

But you are currently storing unvalidated strings in here, right? So when a rule runs with a param which does not validate, the rule will exit early, failing the validation.

I may be misunderstanding what's going on here ...

I changed this part. Now, we validate the KQL filter by passing the allowLeadingWildcards setting as true always, so we made the validation loose but didn't remove it. Also, in the case of the optional filter, the validation was on the wrong field, which I fixed. So, for that one, previously, we didn't have validation, and now we validate it using allowLeadingWildcards as true.

Please let me know if there are any concerns that we need to take into account. (Update: discussed in Slack and there was no major concern)

[dominiqueclarke]

LGTM

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: ebd1a09
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-190031-ebd1a09902b0

Failed CI Steps

• Jest Tests #7

Test Failures

• [job] [logs] Jest Tests #7 / step select agent policy should not select agent policy by default if multiple exists
• [job] [logs] Jest Tests #7 / step select agent policy should select agent policy by default if one exists

Metrics [docs]

✅ unchanged

History

• 💛 Build #226983 was flaky 4473a87
• 💚 Build #226729 succeeded cf3e014
• 💛 Build #226449 was flaky dc8ee2f
• 💔 Build #226227 failed 6e62589
• 💔 Build #226216 failed 9243c48

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @maryam-saeidi

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.15

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[pmuellr]

Wanted to add a note re: potential Backward Compatibility / Zero Down Time issues.

On the face of it, we made one field stricter and one field looser. The field that we made stricter (query) would end up failing in the rule executor, so the only compatibility issue is where it fails (framework now instead of executor). The field that we made looser (language) wasn't supposed to be validated anyway - the value will be like kql, which happens to validate as valid kql.

So, seems like we shouldn't have any forward- or backward- compatibility issues.