KIALI-2479 Block console rendering until all configs are retrieved#1043
KIALI-2479 Block console rendering until all configs are retrieved#1043israel-hdez merged 10 commits intokiali:masterfrom
Conversation
So this is removed from redux (lot of changes) and used instead as a standard constant. This constant is written in 'window' object, in env.js, by the server. File is imported in index.html as static js. Valid durations are now computed statically once for all.
While adding functions in HistoryManager to validate URL, taking that opportunity to do some refactoring here: - Graph components were calling functions from ListPageHelper, which it isn't supposed to do, so moving these functions to HistoryManager - Rename enum URLParams => URLParam - More homogene usage of "undefined" instead of "null" in getting URL params
|
@jshaughn @jotak This is adding an AuthenticationController to "orchestrate" the login flow and block the rendering until all configs are retrieved from the server (prometheus, grafana, status). Once all data is fetched, it allows rendering of the console. It's not yet finished: error handling is pending, and probably I'm abusing of the "Initialization" screen (the login screen should probably show the messages to avoid too much screen switching). But first, I want to check if you like this approach so I proceed to finish the details (so, not yet ready for a detailed review). This is not moving away from redux, so the PR is not "big". If you are OK with this approach, we can either stay with redux or I can incorportate @jotak 's changes from #1012 to move the promConfigs out of redux. |
|
Will this only "retrieve" the config from the backend? Or also verify that the backends exist? |
|
@pilhuhn It's only retrieving config. So, if server side returns blank configs, it won't block rendering. But... I'm checking how the server side works and it seems there are some cases where an error is thrown and it should be valid to continue the login. I'll fix that once @jotak and @jshaughn give their 👍 to proceed to finish this work. Thanks for the observation! |
|
@israel-hdez , I have no problem with a more centralized authentication mechanism. I'm for anything that:
I know this is WIP but if you name the file AuthenticationController please also name the class AuthenticationController and the container AuthenticationControllerContainer. |
|
Yes, this seems like a good idea to me. |
This is adding an AuthenticationController component. This component becomes responsible on determining the moment when it's OK to render the console. Code in charge of retrieving server configs is integrated into the AuthenticationController component.
Adding error message if /api/config endpoint fails. Configs from /api/grafana and /api/status are not required for Kiali to start. This tolerates errors from these endpoints and proceeds to rendering of the main console.
* Moving authenticationConfig constant to src/configs/ folder * Rename StatusInfo type to ServerStatus and move to its own file in src/types/
israel-hdez
requested review from
jotak and
jshaughn
and removed request for
jotak
|
This also will fix https://issues.jboss.org/browse/KIALI-2502 |
|
|
||
| this.setState({ stage: 'logged-in' }); | ||
| } catch (err) { | ||
| this.setState({ isPostLoginError: true }); |
There was a problem hiding this comment.
While testing and opening Kiali the first time, I ran in that situation where isPostLoginError was true and it displayed the error message hardcoded above. I'm curious what would have been the caught err here. Maybe it should be logged? (console.error)
The error was gone after a refresh.
jotak
left a comment
jotak
left a comment
There was a problem hiding this comment.
Just had a minor comment, else LGTM. I focused mainly on the serverConfig / duration parts for review and test, as I haven't worked much on the other parts (login/auth) so more reviews are welcome.
There was a problem hiding this comment.
I'm replacing the initial review comment because it was affected by nip.io going down...
@israel-hdez I installed your branch and ran via yarn start. The server-side is an up to date master. I cleared my browser before starting. It started with the login screen. I entered the creds and it properly rendered the Overview page. I did receive a grafana link error. I waited through some refreshes and it was still OK. I hit F5. I again saw the Initialization screen and also again received the grafana error. Is that necessary? In the past we were not presented with an intermediate screen on F5. I did some more moving around and F5 refreshes and things went well until I hit a "Could not fetch namespaces" 500 error. I don't know what caused this, it seemed independent of any activity.
I noticed that the debug info no longer provides the server config, likely because we removed it from redux. It should include this information.
Overall everything ran well and logouts/logins/killed windows/F5s/navigations all worked. The only issue was the unexplained namespace fetch failure. It would be nice if on F5 we didn't see the Initialization screen. We do need to add the serverConfig to debug Info.
As for the code just a few comments:
- I think AuthenticationControllerContainer should be rename to AuthenticationController just because we've moved away from naming the files after the redux controller.
- I'd like to see AuthenticationFlowProps broken into ReduxProps and AuthenticationControllerProps like in most other files using redux.
- I'd like to see AuthenticationFlowState renamed to AuthenticationControllerState
Overall this looks really good to me.
mattmahoneyrh
left a comment
mattmahoneyrh
left a comment
There was a problem hiding this comment.
Testing done:
- Incogneto login
- F5's (check console logs), logged in incogneto
- Delete Grafana deployment, scaled Kiali, Logged in (incogneto)
- Delete Jaeger deployment, scaled Kiali, Logged in (incogneto)
|
@jshaughn Ok, I'll check the renamings and the Debug dialog. About the intermediate page in F5, what I can do is to present a blank page while data is fetched. Perhaps it could be good to have a timeout to show the intermediate page in case any of the endpoints takes too long to reply, but for now I'll only make it show a blank page on F5. |
Maybe that would be better. I guess this isn't a blocker, though. F5 typically does have some sort of flash or load so I guess it'as not too uncommon. If it's easier, or if you don't like the blank page, we can leave it as is. |
|
A blank blank screen does not sound appealing. At least it should say something along "loading stuff" |
+1, I changed my mind. After looking at various other sites I think seeing the initialization screen on F5 is fine. |
… Integration and refactor (#1025) * KIALI-2456 Error Traces shows kiali home when refreshing the page KIALI-2459 Fix traces tab if value is -1 KIALI-2466 Remove visual options in jaeger toolbar (Hide graph, hide summary) and make the search button bigger KIALI-2465 Get Services in jaegerToolbar is not working after Openshift oauth changes KIALI-2450 Change the namespace Selector in Jaeger Distributed tracing view to be like other views KIALI-2507 Distributed tracing link disappear in a new tab KIALI-2515 UI About - Components not persist when the user open a new tab * Rebased after merge #1043
6 participants
This is adding an AuthenticationController component. This component
becomes responsible on determining the moment when it's OK to render the
console.
Code in charge of retrieving server configs is integrated into the
AuthenticationController component.
https://issues.jboss.org/browse/KIALI-2479
https://issues.jboss.org/browse/KIALI-2502