note: I moved this change to a separate commit as I want to hear your thoughts. It's "the least evil" solution I could come up with. Alternatively we can introduce an endpoint that will return isSpacesEnabled only based on the "Spaces Service" that Spaces registers with Security during setup.

I think this is tolerable. I think it'd make sense (in a followup) for us to export a useSpaces hook from the Spaces plugin itself, so that we could at least have the Spaces plugin own this determination.

The disabled check aside, I could see other plugins benefiting from this hook too.

Yeah, I agree

I think we have a gap in error handling here. The legacy implementation had retry logic (with backoff) in the event of a transient failure, which I think we want here as well. If await registerPrivilegesWithCluster() throws an error, but neither this.status.core$ nor this.license.features$ emit a new value, then Kibana will be stuck in an unusable state from an authorization perspective. We will either have to wait for one of those two observables to change (if that ever happens), or Kibana will have to be restarted in order to reattempt privilege registration.

That's a good point, thanks! I naively expected status service to catch intermittent 503s and emit new status updates, but it seems not be the case since you're bringing this up 🙂 I'm not sure how I feel about having this logic (not simple, prone to errors) in every plugin though. Let me think a bit...

That's a good point, thanks! I naively expected status service to catch intermittent 503s and emit new status updates

I think it will definitely catch some of these, but I could also imagine a race condition where core thinks everything is OK, but then we get network blip where we lose our connection for a very short period of time. core might not catch this blip if it wasn't doing anything with ES at the time.

It's also possible that the ES connection would be technically ok, but if the cluster is starved for resources, then it's possible that our specific call to register privileges would fail (disk space watermark, readonly indices, etc.)

I'm not sure how I feel about having this logic (not simple, prone to errors) in every plugin though. Let me think a bit...

Yeah, that's definitely a tradeoff. That's probably one of the reasons we originally had this as a reusable utility, so that each plugin wouldn't have to implement its own.

It's also possible that the ES connection would be technically ok, but if the cluster is starved for resources, then it's possible that our specific call to register privileges would fail (disk space watermark, readonly indices, etc.)

The thing that concerns me most is that Core determines the first time when we try to perform an action, but if that action fails, then responsibility shifts to a consumer that now decides when and if it's appropriate to do a re-try. It seems we don't have a way to force Core to check whether ES is available or not at the time we do a re-try. If ES is struggling for whatever reason we (every plugin that needs to do a re-try) don't want to make things worse via bombarding it with the requests. Ideally Core should be a single source of truth that decides whether it's appropriate to do a re-try or not.

That's a fair point. @elastic/kibana-platform do you have any thoughts around this?

To summarize, we are waiting for Core to tell us when ES is ready via the exposed CoreStatus service. Once all prerequisites are met, we attempt to make a couple of calls to ES. If these checks fail though, we would like to be able to retry them at some point in the future, but it's possible that everything appears "fine" from the perspective of CoreStatus, so we will never get an updated value from the Observable.

Having each plugin implement its own retry logic is feeling a bit complex and error prone. Aleh also makes a good point that plugins shouldn't be exacerbating the problem by retrying whenever they feel it is appropriate, but instead should wait on a signal from Core. The problem is that we don't have a way to alert Core that something went wrong.

@legrego I don't remember we discussed this case. Added a comment to the status service meta-issue #41983 (comment)

note: presumably we'll get rid of this code in 7.10, so I gave up on learning how to properly leverage retryWhen here and went with plain setTimeout approach 🙈