Fix Watcher NotificationService's secure settings #35610
Conversation
|
Pinging @elastic/es-core-infra |
spinscale
left a comment
spinscale
left a comment
There was a problem hiding this comment.
I left some comments, mainly cosmetic though
| this.defaultAccount = selectDefaultAccount(completeSettings, this.accounts); | ||
| } | ||
|
|
||
| // Used for testing only |
There was a problem hiding this comment.
can you move this ctor up again so it does not get lost?
|
|
||
| final String defaultAccountName = settings.get("xpack.notification." + type + ".default_account"); | ||
| A defaultAccount; | ||
| private @Nullable Account selectDefaultAccount(Settings settings, Map<String, Account> accounts) { |
There was a problem hiding this comment.
how about naming this findDefaultAccount or findDefaultAccountOrNull?
There was a problem hiding this comment.
I have went with findDefaultAccountOrNull
|
|
||
| public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) { | ||
| super("email", clusterSettings, EmailService.getSettings()); | ||
| super("email", clusterSettings, Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_EMAIL_DEFAULTS, SETTING_SMTP_AUTH, |
There was a problem hiding this comment.
this is a lot of duplication from the setting. If a new settings gets added it needs to be added here as well. Would it make sense to have EmailService.getSettings() and EmailService.getSecureSettings() for each of the service? This also applies for the next changes
| } | ||
| } | ||
|
|
||
| private SecureSettings cacheSecureSettings(Settings source) throws GeneralSecurityException { |
There was a problem hiding this comment.
I think some more javadoc/explanation why this is done would be super useful for the next person reading this :-)
|
@spinscale I have addressed all your suggestions. Mind you reappraise this, please? |
spinscale
left a comment
spinscale
left a comment
There was a problem hiding this comment.
One last naming nit, but LGTM otherwise
There was a problem hiding this comment.
the naming might be a bit confusing: clusterSettings and then getClusterSettings() as variable names
There was a problem hiding this comment.
I have renamed getClusterSettings to getDynamicSettings . The fact that they are cluster settings is not relevant in this context (they are all node scope) what is relevant is that they are dynamic (Property.Dynamic).
|
run gradle build test 1 |
|
run gradle build tests 1 |
|
test this please |
|
run gradle build tests 1 |
|
run gradle build tests 2 |
The NotificationService (base class for SlackService, HipchatService ...) has both dynamic cluster settings and SecureSettings and builds the clients (Account) that are used to comm with external services. This commit fixes an important bug about updating/reloading any of these settings (both Secure and dynamic cluster). Briefly the bug is due to the fact that both the secure settings as well as the dynamic node scoped ones can be updated independently, but when constructing the clients some of the settings might not be visible.
The NotificationService (base class for SlackService, HipchatService ...) has both dynamic cluster settings and SecureSettings and builds the clients (Account) that are used to comm with external services. This commit fixes an important bug about updating/reloading any of these settings (both Secure and dynamic cluster). Briefly the bug is due to the fact that both the secure settings as well as the dynamic node scoped ones can be updated independently, but when constructing the clients some of the settings might not be visible.
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
4 participants
The
NotificationService(base class forSlackService,HipchatService...) has both dynamic cluster settings andSecureSettings and builds the clients (Account) that are used to comm with external services. This commit fixes an important bug about updating/reloading any of these settings (both "Secure" or not).This use site for reloading secure settings is unlike the others because here there are also dynamic cluster settings at play - interacting to build an object. That means that, the last update to cluster settings has to pair up with the last reload of the secure settings. This commit tries to do exactly that: when calling the cluster update settings consumer it looks for the last secure settings, and rebuilds the clients. Likewise, when reloading the secure settings it looks for the last updated cluster settings, and rebuild the clients.
It's easy enough to cache the last cluster settings update but it's messy to cache the
SecureSettings, seeNotificationService#cacheSecureSettings. This is however the only alternative that I could come up with, without introducing any limitations (or refactoring all clients): for example we could say that dynamic cluster settings don't take effect until "reload"-ing, because there are dependedSecureSettingthat are not available at the time of the cluster update call. But this is a too big of a change...Another implementation detail is how to tolerate incomplete clients, because if there are two API calls required to add/remove a client (one for the cluster settings, the other for the secure settings). I this implementation, the client builder will complain if it does not find the secure settings to pair of some client cluster seettings, but will NOT complain if there are extra secure settings that cannot be paired with cluster settings. I have chosen this approach because in 6.x there could be clients requiring only cluster seettings (using deprecated ones).
Lastly, I have tried to contain the changes, and I've managed to mostly narrow them to the
NotificationService, without touching the clients.Closes #35378
CC @romseygeek