Add Put Query Ruleset API call

[kderusso]

Adds a PUT Query Ruleset API request. Also changes this from a system property to a feature flag.

Example API request:

PUT /_query_rules/test-ruleset
{
  "ruleset_id": "test-ruleset",
  "rules": [
    {
      "rule_id": "query-rule-id1",
      "type": "pinned",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query_string",
          "value": "puggles"
        }
      ],
      "actions":
        {
          "ids": [
            "abc",
            "def"
          ]
        }
    },
    {
      "rule_id": "query-rule-id1",
      "type": "pinned",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query_string",
          "value": "puggles"
        }
      ],
      "actions":
        {
          "docs": [
            { "_index": "test-index-1", "_id": "ghi"},
            { "_index": "test-index-1", "_id": "jkl"}
          ]
        }
    }
  ]
}

[kderusso]

Note: I purposely kept this external to the _application endpoint. (If we like this and think the classes need to move packages, I'd prefer to do that refactor/move in a single PR to limit noise)

[kderusso]

Note: Since only the PUT API call is implemented and not GET or DELETE, this test is pretty lightweight. It will be extended as additional CRUD API calls are added.

[kderusso]

I'm not quite sure what validation we'd want here that we don't already do in constructors. Ideas welcome?

[carlosdelest]

I can't think of any! Maybe we can change the TODO to a comment indicating that they are already validated in constructors

[ioanatia]

So I would actually try to move all the validations here if possible.
Because right now, what happens is that we validate the request payload before we validate the license.
We want the license check to be the first one that happens, not the validation of the payload.
It's a problem we have with the rest of the APIs in this module, that we did not get to fix (we have an issue for it).
But since this is a new API, maybe we can make this right from the beginning?

[kderusso]

Updated with validations here. Is that enough, @ioanatia? I don't like the idea of removing constructor validation too, but we can discuss for a followup PR

[kderusso]

I'd especially like feedback from @ioanatia here - Is a "manage query rules" cluster privilege sufficient?

[ioanatia]

If it was me, I would actually just use the manage cluster privilege for now because this feature is behind a feature flag and it's disabled by default. So there's no immediate need to add a cluster privilege.
The manage cluster privilege includes all the cluster privileges except manage_security, so it is perfectly fine.
We can then add this as a separate cluster privilege and have a review from the Elasticsearch security team.
The manage cluster privilege would most likely include this new cluster privilege.

One unknown that I have would be what would happen if at some point we decide to remove or rename this manage_query_rules privilege - if a cluster has an API key that uses it, when it gets upgraded to a new version where this privilege does not exist, what would the behaviour of the API key be? Would the upgrade even fail if API keys or roles refer to a cluster privilege that does not exist? We had a similar conversation when we thought about removing the cluster privilege we have for behavioral analytics and rely on the new workflow restrictions.

So instead of trying to answer questions like these right away, I would just not focus on it at the moment and use the manage cluster privilege for now.

[kderusso]

Sounds reasonable, thank you. I'll update this 👍

[elasticsearchmachine]

Pinging @elastic/ent-search-eng (Team:Enterprise Search)

[carlosdelest]

Some minor comments, overall 💯

[carlosdelest]

Do we need this from the start?

[kderusso]

Probably not, but it's a paradigm we already use with search applications and it wasn't a big lift to support.

[carlosdelest]

I'd say YAGNI - let's not add anything we're not sure to need, to avoid testing / documenting / maintaining it. It's easy to add later if needed

[kderusso]

Most of the time I'd agree, but there is a chance of accidentally overwriting an entire ruleset, with no history. This could be a guardrail.

[carlosdelest]

I think we added that option to Search Applications for a very specific UI use case, and I don't feel this is something we need as a rule. I don't see this as a deal breaker for the PR, just a warning that we probably should not add it at least yet. It adds code and tests that we are not needing.

[kderusso]

OK, I don't feel strongly enough that it has to stay in to argue more 😉 - I will plan to remove it, but I'll do so in a follow up PR (I want to get this merged to unblock the next one).

[carlosdelest]

I can't think of any! Maybe we can change the TODO to a comment indicating that they are already validated in constructors