fix(oauth): HAWNG-557 Hawtio login page shows up before OpenShift login page

[tadayosi]

https://issues.redhat.com/browse/HAWNG-557

Another approach to fix the issue than #364.

[tadayosi]

It's still under massive refactoring to get all plugins aligned with proper async/await flow, but at least the original issue has been already fixed with the pull req. You can check it with the sample deployment: https://hawtio-online-tasato-test.hawtio-cluster-194953-21cb4c1134e0db6479397e6534459c6a-0000.jp-tok.containers.appdomain.cloud/online/

[tadayosi]

@phantomjinx

I now cannot see what is being changed here and for what real reason. Obviously, there are a lot of superficial name changes of vars and functions none of which go to fixing the issue. Maybe it would be helpful to break those out into a different PR so we can understand how the async flow etc is actually being fixed?

Unfortunately for me it's nearly impossible as I see it's not something I can patch it incrementally. Instead I'm seeing I need to overhaul the entire async/await flow to make it work as expected. Renaming is mostly to follow the naming conventions from hawtio-next and hawtio/hawtio.

[tadayosi]

And other typos like k8 -> k8s as I believe k8 is not a correct abbreviation for kubernetes.

[phantomjinx]

Unfortunately for me it's nearly impossible as I see it's not something I can patch it incrementally. Instead I'm seeing I need to overhaul the entire async/await flow to make it work as expected. Renaming is mostly to follow the naming conventions from hawtio-next and hawtio/hawtio.

That's a shame that is the case. I spent a great deal of time debugging and ensuring the login flow worked as written. Is this really necessary and that broken this close to a general release?

[tadayosi]

Is this really necessary and that broken this close to a general release?

I believe so. I see there are ineffective/unnecessary usages of promise and unnecessary status checking based on incorrect assumptions. For instance, we shouldn't use a raw boolean flag for checking if it's initialised but instead should move it inside a wrapped promise so that everything after the initialisation can happen after the initialisation.

[tadayosi]

https://github.com/hawtio/hawtio-online/blob/main/packages/oauth/src/api.ts#L25-L44

let userProfile: UserProfile | null = null
...
export async function getActiveProfile(): Promise<UserProfile> {
  if (!userProfile) {
    log.debug("Finding 'userProfile' from the active OAuth plugins")
    userProfile = await findUserProfile()
  }

  return userProfile
}

Another example. Actually this doesn't block finding of user profile more than once as multiple function calls can enter inside the if (!userProfile) { block when another call is done while the first call is waiting at await findUserProfile() but before it's returned.

[phantomjinx]

https://github.com/hawtio/hawtio-online/blob/main/packages/oauth/src/api.ts#L25-L44

let userProfile: UserProfile | null = null
...
export async function getActiveProfile(): Promise<UserProfile> {
  if (!userProfile) {
    log.debug("Finding 'userProfile' from the active OAuth plugins")
    userProfile = await findUserProfile()
  }

  return userProfile
}

Another example. Actually this doesn't block finding of user profile more than once as multiple function calls can enter inside the if (!userProfile) { block when another call is done while the first call is waiting at await findUserProfile() but before it's returned.

That's correct. Due to the plugin model, both the oauth and k8-api plugins can get initialised in parallel. Therefore, they both need to wait for the user profile. Only when the user has entered a token and established a login can a user profile become available.

[phantomjinx]

Is this really necessary and that broken this close to a general release?

I believe so. I see there are ineffective/unnecessary usages of promise and unnecessary status checking based on incorrect assumptions. For instance, we shouldn't use a raw boolean flag for checking if it's initialised but instead should move it inside a wrapped promise so that everything after the initialisation can happen after the initialisation.

The process has to fundamentally turn a parallel initialisation of plugins into a sequence. There is no concept of one plugin depending on another. Therefore, service construction has to be held up to wait for the necessary dependency to complete. The bottom of the stack is the oauth which is required to login and present a profile. Only when that is available can the k8-api and then mgmt-api proceed to initialise.

I don't agree this is the time to re-architect this entire flow and the examples cited seem to indicate a difference of opinion in solving the problem rather than highlighting a serious flaw.

[tadayosi]

Due to the plugin model, both the oauth and k8-api plugins can get initialised in parallel.

That's by design. And that's how a JS program works in general. Plugins don't need to bother about parallelism if they just rely on promises for their critical paths. That's what I meant by "For instance, we shouldn't use a raw boolean flag for checking if it's initialised but instead should move it inside a wrapped promise so that everything after the initialisation can happen after the initialisation."

We should not just wrap the plugin entrypoint with async, as that would defeat the basic assumption that the user hooks have been registered before Hawtio fires another cycle of process to bootstrap itself. The user hooks registration must be done at the initial cycle of process at the top-level of JS loading. https://github.com/hawtio/hawtio-online/blob/c8b21a45f1a9260d42d45f9f145b4259e07f00d7/packages/oauth/src/index.ts#L10-L22

[tadayosi]

The process has to fundamentally turn a parallel initialisation of plugins into a sequence.

That's not true. JS is always sequence and it's an illusion that there are parallel processes in the app. If you cleverly use promises you can still make use of concurrency at the initialisation of plugins.

There is no concept of one plugin depending on another. Therefore, service construction has to be held up to wait for the necessary dependency to complete. The bottom of the stack is the oauth which is required to login and present a profile. Only when that is available can the k8-api and then mgmt-api proceed to initialise.

That's true. Currently there are only implicit dependencies between plugins based on the promises of resources at the critical paths. But so far it still works. Plugin dependency sounds like a valid concept to have at hawtio/react. I just wonder if you couldn't have proposed it at hawtio/react instead of choosing a workaround hack at hawtio-online. Both hawtio/react and hawtio-online are the components in the same project. We should always seek for global optmisation.

[tadayosi]

I don't agree this is the time to re-architect this entire flow and the examples cited seem to indicate a difference of opinion in solving the problem rather than highlighting a serious flaw.

Please don't take this as a difference in opinions. Clearly the current code lacks some efficiency and effectiveness in the use of promises and async/await. In fact, my original commit here https://github.com/hawtio/hawtio-online/pull/377/commits/2978d861c36f1012aabbea60ff9b44844e86cf07 should've solved HAWNG-557 issue, but the fact that it wasn't solved is one evidence of a design flaw to be fixed. That's why I still believe it's a necessary refactoring to perform.

That said, I understand your concern. It might be a bit risky to change the code significantly at this point in time. So I'll continue my refactoring at a new branch 2.1.x, intending to be the base for 2.1.0 release. In the meantime, I'll try to backport some fixes done at this pull req to the main branch, along with hawtio/react 1.1.1 upgrade.

[phantomjinx]

I just wonder if you couldn't have proposed it at hawtio/react instead of choosing a workaround hack at hawtio-online. Both hawtio/react and hawtio-online are the components in the same project. We should always seek for global optmisation.

:-) Hindsight is a wonderful thing. It probably need occurred to me to propose such a concept at the time as I was more intent on solving the problem directly in front of me. Anyway, we are where we are so lets come up with a dependency architecture for plugin order.

[phantomjinx]

That said, I understand your concern. It might be a bit risky to change the code significantly at this point in time. So I'll continue my refactoring at a new branch 2.1.x, intending to be the base for 2.1.0 release. In the meantime, I'll try to backport some fixes done at this pull req to the main branch, along with hawtio/react 1.1.1 upgrade.

Excellent. Thanks for your understanding and we can take more time to review the new branch / version to ensure its working right.