-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Move credentials settings merging out of RemoteClusterService
#135491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Move credentials settings merging out of RemoteClusterService
#135491
Conversation
Addresses a second settings cleanup request comment from PR 133834, moving the settings merging and creation of a LinkedProjectConfig out of RemoteClusterService and into TransportReloadRemoteClusterCredentialsAction. This is one step closer to eliminating the Settings based code in RemoteClusterService so it operates on LinkedProjectConfig objects only. Resolves: ES-12864
public synchronized void updateRemoteClusterCredentials(Supplier<Settings> settingsSupplier, ActionListener<Void> listener) { | ||
/** | ||
* Rebuilds linked project connections as needed for updated remote cluster credentials. | ||
* @param settingsSupplier A {@link Supplier} for the updated credentials {@link Settings}. | ||
* @param configFunction A function that builds a {@link LinkedProjectConfig} for an alias, static settings, and new settings. | ||
* @param listener An {@link ActionListener} invoked once all connection modifications have been completed. | ||
*/ | ||
public synchronized void updateRemoteClusterCredentials( | ||
Supplier<Settings> settingsSupplier, | ||
TriFunction<String, Settings, Settings, LinkedProjectConfig> configFunction, | ||
ActionListener<Void> listener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fond of the way this PR turned out. I think I would prefer moving the code in updateRemoteClusterCredentials()
into TransportReloadRemoteClusterCredentialsAction
, and have TransportReloadRemoteClusterCredentialsAction
call RemoteClusterService. maybeRebuildConnectionOnCredentialsChange()
with a LinkedProjectConfig
as needed. We could add a getter on RemoteClusterService
for the RemoteClusterCredentialsManager
.
Yang's comment discusses using ClusterSettingsLinkedProjectConfigService
, but we aren't aware of or have access to one, or a LinkedProjectConfigService
here. I may be misunderstanding what he is suggesting to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't wrapped my head around this and what the cleanest approach here would be but in case this is useful:
We can easily expose LinkedProjectConfigService
to TransportReloadRemoteClusterCredentialsAction
by injecting it during node construction:
final var linkedProjectConfigService = pluginsService.loadSingletonServiceProvider(
LinkedProjectConfigService.class,
() -> new ClusterSettingsLinkedProjectConfigService(settings, clusterService.getClusterSettings(), projectResolver)
);
modules.bindToInstance(LinkedProjectConfigService.class, linkedProjectConfigService);
exposes it to all TransportActions:
@Inject
public TransportReloadRemoteClusterCredentialsAction(
TransportService transportService,
ClusterService clusterService,
ActionFilters actionFilters,
LinkedProjectConfigService linkedProjectConfigService
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeremyDahlgren
My previous comment was indeed a bit vague. It took me a while to remember what I was thinking. 🤦 Sorry for the confusion.
My main point is to remove "settings usage for remote cluster config" as much as possible from RemoteClusterService
since we now have the unified LinkedProjectConfig
object. Hence I mostly like to see us removing maybeRebuildConnectionOnCredentialsChange
from RemoteClusterService
or at least it should not take Settings
but instead LinkedProjectConfig
as input.
For concrete changes:
- I think "add a getter on RemoteClusterService for the RemoteClusterCredentialsManager" seems a reasonable near term option. I assume this means we call
RemoteClusterCredentialsManager#updateClusterCredentials
within the transport action. Conceptually this makes sense since with the unified config object,RemoteClusterService
should remove itself from dealing with these lower level updates and in long term,RemoteClusterCredentialsManager
might need to be managed independently and injected to places. One thing to note is that this means the process of "updating credentials and rebuilding connection" will be performed in twosynchronized
blocks instead of one. It feels OK to me sinceclusterCredentials
is updated as a volatile field and should not have any visible intermediate state. - You are right that
LinkedProjectConfigService
is not accessible inRemoteClusterService
. If we go with your suggestion of moving the logic to the transport action, we should be able to keep the code mostly as is exception in a different class.
Addresses a second settings cleanup request comment from #133834, moving the settings merging and creation of a
LinkedProjectConfig
out ofRemoteClusterService
and intoTransportReloadRemoteClusterCredentialsAction
.This is one step closer to eliminating the
Settings
based code inRemoteClusterService
so it operates onLinkedProjectConfig
objects only.Resolves: ES-12864