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.

@n1v0lg @ywangd - thoughts?

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
)

@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:

1. 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 two synchronized blocks instead of one. It feels OK to me since clusterCredentials is updated as a volatile field and should not have any visible intermediate state.
2. You are right that LinkedProjectConfigService is not accessible in RemoteClusterService. 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.

Thanks Yang for the details here. I relocated the code as suggested, and also ended up moving both updateClusterCredentials() and maybeRebuildConnectionOnCredentialsChange() into the action class. To keep the same synchronization behavior I had a synchronized block on the RCS instance, but this may be different than what you were describing in item 1 of your comment. Please let me know what you think here.

The settings can already be retrieved with clusterService.getSettings() so that the change in RemoteClusterCredentialsManager is not necessary.

👍