nice ! TIL

I'm not sure we want a timeout here. If the timeout is hit it will not cancel the underlying work and other than returning sooner not sure what that buys us. It only encourages calling the API again which can compound the already unhealthy scenerio (and we will never be able to guess a correct upper bound unless it is huge). If anything the per-connection could have a timeout/retry ...but if does not already i don't see a need to add one now. i.e. use the explicit listener variant instead of the implicit listener used in the future to make the call to the action.

Can we short circuit this entirely if the credentials did not change ?

It's tricky: just the absence of credentials in the settings doesn't mean a noop (it could be that we removed credentials) -- anything else would require access to the remote cluster service which is not available here. I wanted to add a short-circuit here as well originally but didn't come up with a good way to do it.

does this handle tearing down a connection after removing a credential ?

If a connection exists, and we remove a credential, updateRemoteCluster handles re-building it. If the connection does not exist, that means cluster settings for it were removed, and updateRemoteCluster will tear it down on the regular settings update call that removed the cluster settings. So if a connection does not exist anymore, and but a credential does still and is removed, there is nothing left to tear down. Let me know if I misinterpreted the question!

Cool! I like the GroupedActionListener.

This is only called from a single-threaded context. Might consider keeping it synchronized for future proofing, but it adds some overhead to acquire an extra lock.

Yes great point. I meant to take a closer look at being less heavy handed with synchronization but didn't get to it. Thanks for prompting me to think about this again. I agree that with the current structure, we can remove synchronization here. I'll think on it and tweak some things 👍

Discussed this on Slack also:

• I prefer to keep this one synchronized for future proofing
• I removed sorting the cluster aliases as that adds unnecessary overhead without utility -- originally, I used sorting with the notion to prevent deadlocks in RemoteClusterService::updateRemoteClusterCredentials (since we iterate over aliases and call a synchronized method for each, we have a potential deadlock). However updateRemoteClusterCredentials is itself syncronized at the top level, so we're safe.

Thanks for raising and discussing this 👍

nit: Since we know that this will always be true when we reload credentials, it might make sense to break this in to its own method to avoid the boolean in the method signature.

agreed that the flag is not ideal but having stared at the method for a bit I don't see a good way to refactor it without basically duplicating it or using some sort of boolean flag in the end -- did you have a specific tweak in mind? partly, I wanted to avoid touching too much of the connection rebuilding code since it's fairly complex and outside of the security team's scope.

Makes sense! It's fairly complex so I think it's out of scope to refactor any of that. 👍

If the CredentialsManager is expected to be thread safe, a concurrency test could be useful in this class.

I didn't get around to this yet -- I think a concurrency test for RemoteClusterService::updateRemoteClusterCredentials would give us the most useful coverage. I think it's a really nice-to-add test but don't want to defer another round of review for it. My plan is to add it still.

I'll log a follow up item to address this one in Jira to not block getting this out the door.

Would be interesting with a test case that blasts the API with concurrent reload requests combined with config updates.

agreed. it wouldn't work with a REST level test because of the keystore file rewrite but I'll see if there is something lower-level we can do

I do not like stacking the timeouts one after the other like this. If we have, say, 150 remotes, and they all hit the 10s timeout, then with this logic we'll be waiting for 1500s = 25 minutes. If we need to time out at all then we should have one top-level timeout, and TBH I'd rather it was in the caller seeing as how we don't do anything (except logging) when we hit the timeout anyway.

I know we have this problem when applying cluster states too, and IMO that's a bug. It's tricky to solve in that case because of the mechanics of how settings are applied, but I would rather avoid making the same mistake here too.

Fair point. I don't feel strongly about the per-connection timeout (nor the top-level timeout for that matter).

If we have 150 remotes, and each connection takes >10s to rebuild the overall request will still wait for 25m. A top level timeout could prevent this but it's difficult to pick a reasonable default since I expect timeouts to vary greatly based on the network situation for a given connection (@jakelandis points this out as here which prompted me to consider a per-connection timeout) -- e.g., a healthy connection will rebuild in >1s but a slow/unstable network could result in long waits.

It could be nice to add a timeout parameter to the initial, generic reload-secure-settings request but we have no way of passing this down to the individual plugin reload call, since the reload() call only takes settings as input and I would prefer not to change the ReloadablePlugin interface here since that's a bigger change that requires separate input and deliberation.

The only other alternative that comes to mind is adding a dedicated setting where end-users can set a connection rebuild timeout but that feels hacky and like an overkill since, as you point out, we don't do anything (except logging) when we hit the timeout anyway.

Given that it's hard to come up with a generic top-level timeout, and that individual connection timeouts can give us very high (and pretty arbitrary) wait times, I prefer to remove the timeout altogether for now and possibly revisit this in the future. @DaveCTurner @jakelandis let me know if you have objections.

I would be happy with no timeout here I think, especially if we trigger all the connection attempts in parallel since there's other timeouts in play lower down the stack, preventing this from really waiting forever.

I do think it's worth reconsidering the synchronous nature of the reload() API, which forces us to call each plugin in turn (or do something worse like spawning a thread per plugin). Stuff like this should be async all the way down the stack IMO and then these concerns wouldn't really arise.

nit: would it be possible to assert no API key configured for my_remote_cluster ? Just a bit paraniod about ensuring this is really RCS 1.0. If it is anything but trivial, feel free to ignore this.

configureRemoteCluster calls checkRemoteConnection under the hood which ensures that no credential is configured when basicSecurity is true here

Checking the keystore would be a bit tricky, off the top of my head. Hope this is sufficient!

requires TLS to be configured for the transport layer

where is this enforced ? Also, isn't TLS enabled for the tranport level in these tests ?

Also, could you log an issue for follow up. Ideally we wouldn't skip these in FIPS mode.

Ah good catch! The message is a copy-paste hoopla. FIPS is still a problem though, around insufficient password length for the keystore password. I'll tweak the message and log a follow up to address this.