Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Service Mesh] General Improvements #1959

Merged
merged 1 commit into from
Oct 18, 2023
Merged

[Service Mesh] General Improvements #1959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions backend/src/routes/api/notebooks/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@ export const getNotebookStatus = async (
let host: string;
if (enableServiceMesh) {
host = await getServiceMeshGwHost(fastify, namespace).catch((e) => {
fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);
fastify.log.warn(`Failed getting service mesh route ${notebookName}: ${e.message}`);
return undefined;
});
} else {
}
if (!host) {
const route = await getRoute(fastify, namespace, notebookName).catch((e) => {
fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);
return undefined;
Expand Down
4 changes: 4 additions & 0 deletions backend/src/utils/resourceUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,10 @@ export const getDashboardConfig = (): DashboardConfig => {
return dashboardConfigWatcher.getResources()?.[0];
};

/**
* 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 =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

disabledSettingState === false;

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/pages/projects/notebook/useRouteForNotebook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const useRouteForNotebook = (
if (notebookName && projectName) {
// if not using service mesh fetch openshift route, otherwise get Istio Ingress Gateway route
const getRoutePromise = !enableServiceMesh
? getRoute(notebookName, projectName).then((route) => route?.spec.host)
? getRoute(notebookName, projectName).then((route) => route.spec.host)
: getServiceMeshGwHost(projectName);
Comment on lines 29 to 31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If enableServiceMesh is true, but getServiceMeshGwHost fails to return a route, should it fallback to getRoute ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


getRoutePromise
Expand Down