Watcher deprecate notification service settings

[albertzaharovits]

Deprecates all the settings for watcher notifications account that are dynamic cluster filtered and have a corresponding secure sibling.
They are deprecated in favor of the sibling that is stored in the keystore.

Follow-up on #35610

[elasticmachine]

Pinging @elastic/es-core-features

[albertzaharovits]

This deprecation work got me wondering on an issue:
Generally speaking there are "accounts" that require both secure settings and cluster dynamic settings (although PagerDutyService and SlackService accounts can be set up using secure settings exclusively) when built. In other words you cannot add a new account until you add both the cluster and the secure settings.
This causes problems because these settings are added using distinct APIs. For example, consider adding a new account for the email notification service. The "secure" way to go about it right now is to:

1. Add xpack.notification.email.account.<new_acount>.smtp.secure_password to the keystore with
   bin/elasticsearch-keystore add xpack.notification.email.account.<new_acount>.smtp.secure_password
2. Trigger the keystore reread with POST _nodes/reload_secure_settings
3. Add the rest of the cluster settings with

curl -X PUT "localhost:9200/_cluster/settings" -H 'Content-Type: application/json' -d'
{
    "persistent" : {
        "xpack.notification.email.account.<new_account>.smtp.user" : "new_user",
        "xpack.notification.email.account.<new_account>.smtp.host" : "smtp.google.com",
        "xpack.notification.email.account.<new_account>.smtp.port" : 587
    }
}
'

After step 3 is completed the new account can be used.
However, swapping steps 1 and 3 won't cut it. Adding the cluster settings before a required secure setting is available will trigger a SettingsException when trying to build the account. And the way it's implemented now, adding accounts with only secure settings won't work.

We need to find a way of updating partial settings and "separately" triggering the clients rebuilding. This sounds complicated to me.
I think a simpler way is to just tolerate partial settings and build accounts for complete setting sets only, i.e., don't bubble up the SettingsException and log the client names that have been built without errors (and log missing settings exceptions).

What do you think Alex?

[spinscale]

This sounds good to me, as long as we return a helpful error message (or even have an API that properly returns the valid accounts). Even though it might be hard to define when a account is 'complete' (add a slack secret, then add message defaults), I think this approach is fine.

[hub-cap]

Would it make sense to just have the user put all of the account info in secure settings instead? All-or-nothing type thing, so that they only have 1 place to build a complete account settings for a given account?

[albertzaharovits]

Would it make sense to just have the user put all of the account info in secure settings instead?

I think some of the non-secret settings might change rather frequent and right now the user experience with the keystore is poor-ish (you have to write a script to propagate the change over all keystores on all the nodes). I think it would look like a feature designed by developers (in the wrong sense). I also fear that if we start picking dependent settings one of which is a cluster setting and another which is in the keystore, and move them both in the keystore, soon we might end up with all the settings in the keystore (not true right now, but it's conceivable).

I am not sure how we're going to get the password on the keystore, but it would be wonderful if we can reread (and decrypt) the keystore during the cluster settings update....

[hub-cap]

these are indeed fair points. i withdraw my point/question :P

Its not feasible to read/reread the keystore during cluster settings update? Surely watcher cannot be the only feature that has a 1/2 configured component with some things in the keystore and some not?

[albertzaharovits]

Its not feasible to read/reread the keystore during cluster settings update?

Since right now there is no password required to reread the keystore it is possible to do it in a cluster update handler. Originally the reload API had a "password" parameter, but it was later removed.

[hub-cap]

Big +1 on deprecation. Is it possible to also add checks for these when setting them a-la #36024 , so the customer can see them in the get deprecations info API?

[gwbrown]

Thank you @hub-cap! I came here to say exactly that. Could you please add a deprecation check to this PR? If not, could you add an item to #36024?

[albertzaharovits]

@hub-cap @gwbrown I have added an item to #36024 . The reason that I have not included a deprecation check in this PR is that, from what I can tell, the settings should first be removed in 7.0 (#36736) and only then we're able to have this check point to the removal docs.

From what I can tell, we should merge this, then #36736, and only then raise a PR for the deprecation check.

Can someone please confirm and review this PR?

[gwbrown]

@albertzaharovits Thank you for adding it to the list! That's probably the best approach, yes, although if #36736 gets merged before this one, a deprecation check could be added to this PR.

(aside: the docs links just use anchors from the asciidoc, so they are very predictable, and are not automatically checked, so there's nothing preventing the check from being merged before the docs really, just best practice is to have the docs in place first.)

[gwbrown]

LGTM in terms of the changes to deprecate these settings. Approving since it looks like @spinscale and @hub-cap are on board with the direction.