[Service Mesh] General Improvements #1959
Conversation
Co-authored-by: christianvogt <[email protected]>
| const getRoutePromise = !enableServiceMesh | ||
| ? getRoute(notebookName, projectName).then((route) => route?.spec.host) | ||
| ? getRoute(notebookName, projectName).then((route) => route.spec.host) | ||
| : getServiceMeshGwHost(projectName); |
There was a problem hiding this comment.
If enableServiceMesh is true, but getServiceMeshGwHost fails to return a route, should it fallback to getRoute ?
There was a problem hiding this comment.
I don't think so. Since if a notebook has been created or restarted with enableServiceMesh=true, the notebook should not have oauth-proxy injected into it, so even if a route was successfully fetched with the getRoute call, I don't think that it would be functional.
Potentially a user could start a notebook, leave it running and then install service mesh + enable the feature flag, and then be in a state where this scenario could apply. But from previous conversations, I don't think the goal is to support this partial Oauth/Istio state, the user should just restart the notebook in this case.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| /** | ||
| * Feature flags are required in the config -- but upgrades can be mixed and omission of the property | ||
| * usually ends up being enabled. This will prevent that as a general utility. | ||
| */ | ||
| export const featureFlagEnabled = (disabledSettingState?: boolean): boolean => |
There was a problem hiding this comment.
This function is going away in #2010 -- what is the OSSM need here? You just work or not work based on the feature flag? Do you have any backend component stack (in the DSC) that we can look at?
Closes #1958
Description
How Has This Been Tested?
Built and used image
quay.io/maistra-dev/odh-dashboard:gen-improvefor testing.Changes here are pretty minimal, just tested that when a notebook is created/started from the jupyter tile + from a DS project with
ODHDashboardConfig.spec.disableServiceMesh=false, the user is routed to the notebook correctly through the mesh.Verified the same when ODHDashboardConfig.spec.disableServiceMesh=true`, that the user is routed to the old route using oauth-proxy.
Credited @christianvogt as a co-author on the commit as these 3 changes came directly from Christian's review + suggestions on #1088 so I didn't want that to be lost to the sands of time.
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main