feat(ossm): adds OSSM annotations to the relevant cluster resources #1088

cam-garrison · 2023-04-04T20:40:42Z

Closes: #1087

Description

This change takes care of populating the following annotations to resources created through ODH Dashboard

opendatahub.io/service-mesh can be set to true or false and will be used to adjust ODH components and make them part of the Service Mesh. This is applied on both Project and Notebook resources
The dashboard will now link to notebooks through the mesh if feature flag disableServiceMesh=false

In addition, the code hides service-mesh-specific changes to annotations behind disableServiceMesh flag which is added in opendatahub-io/opendatahub-operator#217

How Has This Been Tested?

We have tested this manually to ensure that when disableServiceMesh=false the resources mentioned above contain the corresponding annotations. Also have tested manually to ensure that when disableServiceMesh=false notebook links send the user through the mesh ingress-gateway, and when disableServiceMesh=true the user is sent through the OpenShift route as before.

The cluster adjustments to enable Service Mesh with ODH are still in progress (PR in manifests here) .

One visible change even without the Service Mesh deployed is that when disableServiceMesh=false the oauth-proxy sidecar is not injected anymore. This is intended.

Until this PR is merged and a new bundle is released you can use my build of the ODH Operator that includes the disableServiceMesh flag as part of the ODHDashboardConfig. Do so by running operator-sdk run bundle quay.io/cgarriso/opendatahub-operator-bundle:dev-0.0.2 --namespace $OPERATOR_NAMESPACE (req operator-sdk v1.24.1 )

Either build and push this dashboard image or use quay.io/maistra-dev/odh-dashboard:ossm_annotations image.

Test Impact

If you can offer us some hints on how to test feature flags and resource creation through Jest tests we are happy to extend this PR with it.

Request review criteria:

The commits have meaningful messages (squashes happen on merge by the bot).
Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

openshift-ci · 2023-04-04T20:40:54Z

Hi @cam-garrison. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2023-04-05T13:12:01Z

/ok-to-test

cam-garrison · 2023-04-05T13:32:28Z

I also have this recorded demo showing the changes which may be useful for review.

lucferbux

I'm requesting changes, this could be solved just with some clarification about the code, I'm lacking of the context about how it will be handled by the controller, but so far I'm worried about what happens when you enable/disable the feature flag with existing notebooks, which is the most common scenario we'll get.

backend/src/routes/api/namespaces/namespaceUtils.ts

backend/src/routes/api/notebooks/utils.ts

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

lucferbux · 2023-05-08T09:39:49Z

@bartoszmajsak @cam-garrison Hi! We've added new features that might conflict with the current PR, if you have any questions let me know and we can get through them.

This change takes care of populating the following annotations to resources created through ODH Dashboard - `opendatahub.io/service-mesh` can be set to `true` or `false` and will be used to alternate ODH components and make them part of the Service Mesh. This is applied on both `Project` and `Notebook` resources - `opendatahub.io/hub-url` is added to the `Notebook` resources at this point and is inteded for use with Authorino's `AuthConfig` host - The dashboard will now link to notebooks through the mesh if feature flag `disableServiceMesh=false` In addition, the code hides service-mesh-specific changes to annotations behind `disableServiceMesh` flag which is added in opendatahub-io/opendatahub-operator#217 We have tested this manually. If you can offer us some hints on how to test feature flags and resource creation through Jest tests we are happy to extend this PR with it. Until this [PR](opendatahub-io/opendatahub-operator#217) is merged and a new bundle is released you can use my build of the ODH Operator that includes the `disableServiceMesh` flag as part of the `ODHDashboardConfig`. Do so by running `operator-sdk run bundle quay.io/cgarriso/opendatahub-operator-bundle:dev-0.0.1 --namespace $OPERATOR_NAMESPACE` (req operator-sdk v1.24.1 ) Either build and push this dashboard image or use `quay.io/maistra-dev/odh-dashboard:ossm_annotations` image. Co-authored-by: bartoszmajsak <[email protected]>

* use project annotation for ds project nb routing * chore: passes host instead of the entire route and removes hub-url annotation as it is not needed * remove reference to hub-host --------- Co-authored-by: bartoszmajsak <[email protected]>

* update existing annotation reference * use ns annotation in backend route fetching

frontend/src/k8sTypes.ts

cam-garrison · 2023-06-28T14:39:52Z

Hi @lucferbux,

This PR is now ready for your re-review.

Changes since you last reviewed:

Added a service-mesh annotation to the created data science projects. This annotation is utilized in our odh-project-controller to add service-mesh resources and annotations to data science projects).
Modified the notebook linking logic. Now, instead of fetching the ingress-gateway route from the istio-system namespace, we are fetching the host from an annotation created by the odh-project-controller. This change simplifies the logic when linking to notebooks and prevents permissions issues when trying to fetch a route from the istio-system namespace.
Regarding migration - notebooks that were initially created with disableServiceMesh=true and later had disableServiceMesh set to false will have their annotations (inject-oauth and service-mesh) "migrated" upon restart. The commit adding this change can be found here. Please note, this PR does not touch the data science project annotation; this task will be handled elsewhere (currently in our init-job in manifests we will switch all previously created dsp to be a part of the service mesh).

I believe these modifications, particularly concerning the migration of old notebooks' annotations, should address your prior concerns. Please take a look and don't hesitate to reach out if you have any questions!

lucferbux · 2023-06-29T13:53:32Z

Hi @lucferbux,

This PR is now ready for your re-review.

Changes since you last reviewed:

Added a service-mesh annotation to the created data science projects. This annotation is utilized in our odh-project-controller to add service-mesh resources and annotations to data science projects).

Modified the notebook linking logic. Now, instead of fetching the ingress-gateway route from the istio-system namespace, we are fetching the host from an annotation created by the odh-project-controller. This change simplifies the logic when linking to notebooks and prevents permissions issues when trying to fetch a route from the istio-system namespace.

Regarding migration - notebooks that were initially created with disableServiceMesh=true and later had disableServiceMesh set to false will have their annotations (inject-oauth and service-mesh) "migrated" upon restart. The commit adding this change can be found here. Please note, this PR does not touch the data science project annotation; this task will be handled elsewhere (currently in our init-job in manifests we will switch all previously created dsp to be a part of the service mesh).

I believe these modifications, particularly concerning the migration of old notebooks' annotations, should address your prior concerns. Please take a look and don't hesitate to reach out if you have any questions!

Ok, I think I'll try to clear my morning tomorrow to give a deep look at the changes, thank you very much!

lucferbux · 2023-06-29T13:55:13Z

@cam-garrison In the meantime, take a look at this warnings that you need to address before merging the PR.
Runing locally npm test should tell you the linting warnings the CI is raising.

lucferbux · 2023-07-07T14:41:26Z

/hold

cam-garrison · 2023-09-22T15:28:15Z

Thx for the review! I think I've addressed all points now - left one followup question where I wasn't sure.

Re:

I've added #1838 as a tracker, this should go to incubation first, changed targeted branch to f/ossm

What does "moving to incubation" mean exactly? testing by QE? Will there be some publicly available build available? Just want to know what the incubation phase entails.

Thanks!

cam-garrison · 2023-10-04T13:15:13Z

@lucferbux can we re-target this to incubation branch? - going to be adding one more commit as well with manifests updates.

cam-garrison · 2023-10-04T14:08:36Z

Added manifests changes from #1900 into this PR:
See closed PR desc for full reasoning, TLDR: since v2 operator pulls manifests from dashboard, need to add requisite service mesh overlay.

andrewballantyne · 2023-10-04T14:49:08Z

As I noted in Slack: https://github.com/opendatahub-io/odh-dashboard/blob/main/docs/process-definition/branches.md our branching strategy uses feature branches first. Going directly to incubation removes the layer of indirection that allows it to ever merge back to main & onto downstream products like RHODS.

lucferbux

Just small nits, I'm not 100% sure about all the config files, i lack a little bit more context for that, and I haven't tested that yet, but so far code looks great.

Please squash before we can merge, thanks a lot for all the work @cam-garrison

lucferbux · 2023-10-04T15:18:03Z

frontend/src/api/k8s/notebooks.ts

  enablePipelines?: boolean,
 ): Promise<NotebookKind> => {
-  const patches: Patch[] = [];
-  patches.push(startPatch);
+  const enableServiceMesh = featureFlagEnabled(


Yes, I'm talking about something like this: https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/projectSharing/utils.ts#L7 is not mandatory but it might be nice to have

lucferbux · 2023-10-04T15:20:06Z

manifests/overlays/service-mesh/OWNERS

@@ -0,0 +1,10 @@
+# Each list is sorted alphabetically, additions should maintain that order


why do we have this OWNERS files in the manifest, I don't understand

OWNERS files can be placed at lower levels to support "just these files" changing. So if something under the overlays/service-mesh folder is the only thing to change, they would need need the repo ownership to step in to review it. It's additive on

I think this is fine for this use-case... We should definitely avoid OWNERS files (with approve access) over code so that we can still approve the holistic nature of that.

why do we have this OWNERS files in the manifest, I don't understand

Our line of thinking is that we offer help and knowledge to keep this thing up-to-date and evolving. So if anything will be proposed to change here we will be pinged

lucferbux · 2023-10-04T15:22:06Z

manifests/overlays/minimal/README.md

@@ -0,0 +1,4 @@
+## Dashboard Minimal overlay


I don't see the harm on having a minimal overlay, but maybe we would want to have this in other PR?

cc @andrewballantyne @christianvogt what do you think? Not sure if we should stick this, which is related to a deployment of our repo with this same PR.

This is a neat idea... I'm not against new overlays like this. @cam-garrison was this something you used in your testing/coding?

Yeah, this can be removed. It is anyway not possible to combine overlays like it was with KfDef magic, so better to make it separate and shrink this PR.

Yes, we used this for testing/running in CRC specifically. +1 on bartosz's point though, can be removed especially since in v2 operator it is not as easy to apply multiple overlays.

manifests/base/_mesh_overlay-delete-oauth-config-patch.yaml

christianvogt · 2023-10-04T15:41:33Z

backend/src/routes/api/notebooks/utils.ts

+    if (enableServiceMesh) {
+      host = await getServiceMeshGwHost(fastify, namespace).catch((e) => {
+        fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);
+        return undefined;
+      });
+    } else {
+      const route = await getRoute(fastify, namespace, notebookName).catch((e) => {
+        fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);
+        return undefined;
+      });
+      host = route?.spec.host;
+    }


Should we fall back to retrieving the host through the route?
By enabling the flag without service mesh, we'll get an invalid url.

Suggested change

if (enableServiceMesh) {

host = await getServiceMeshGwHost(fastify, namespace).catch((e) => {

fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);

return undefined;

});

} else {

const route = await getRoute(fastify, namespace, notebookName).catch((e) => {

fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);

return undefined;

});

host = route?.spec.host;

}

if (enableServiceMesh) {

host = await getServiceMeshGwHost(fastify, namespace).catch((e) => {

fastify.log.warn(`Failed getting service mesh route ${notebookName}: ${e.message}`);

return undefined;

});

}

if (!host) {

const route = await getRoute(fastify, namespace, notebookName).catch((e) => {

fastify.log.warn(`Failed getting route ${notebookName}: ${e.message}`);

return undefined;

});

host = route?.spec.host;

}

christianvogt · 2023-10-04T16:01:56Z

backend/src/utils/resourceUtils.ts

+export const featureFlagEnabled = (disabledSettingState?: boolean): boolean =>
+  disabledSettingState === false;
+


I suggest adding the same comment as we have present in the frontend code.

Suggested change

export const featureFlagEnabled = (disabledSettingState?: boolean): boolean =>

disabledSettingState === false;

/**

* 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 =>

disabledSettingState === false;

christianvogt · 2023-10-04T16:05:27Z

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

+        const getRoutePromise = !enableServiceMesh
+          ? getRoute(notebookName, projectName).then((route) => route?.spec.host)
+          : getServiceMeshGwHost(projectName);


Should getRoute become a fallback instead of a conditional?

Interesting idea... using the getRoute when getServiceMeshGwHost returns an invalid value?

christianvogt · 2023-10-04T16:07:13Z

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

+        );
+        // 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)


Suggested change

? getRoute(notebookName, projectName).then((route) => route?.spec.host)

? getRoute(notebookName, projectName).then((route) => route.spec.host)

christianvogt · 2023-10-04T16:07:38Z

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

            if (cancelled) {
              return;
            }
-            setRoute(`https://${route.spec.host}/notebook/${projectName}/${notebookName}`);
+            setRoute(`https://${host}/notebook/${projectName}/${notebookName}/`);


getServiceMeshGwHost can return null creating an invalid url.

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

andrewballantyne · 2023-10-04T16:39:51Z

frontend/src/api/k8s/notebooks.ts

+    getInjectOAuthPatch(enableServiceMesh),
+    getServiceMeshPatch(enableServiceMesh),
+    getProxyInjectPatch(enableServiceMesh),


These seem all very related, we may want to look at cleaning that up in the future to make it one "set of patches" rather than 3 separate ones. Just for code cleanness.

andrewballantyne · 2023-10-04T16:41:42Z

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

+        const getRoutePromise = !enableServiceMesh
+          ? getRoute(notebookName, projectName).then((route) => route?.spec.host)
+          : getServiceMeshGwHost(projectName);


Interesting idea... using the getRoute when getServiceMeshGwHost returns an invalid value?

frontend/src/pages/projects/notebook/useRouteForNotebook.ts

frontend/src/utilities/notebookControllerUtils.ts

manifests/base/_mesh_overlay-delete-oauth-config-patch.yaml

manifests/base/_mesh_overlay_delete-oauth-client-patch.yaml

manifests/overlays/service-mesh/patches/oauth-client-patch.yaml

openshift-ci · 2023-10-04T21:02:24Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewballantyne · 2023-10-04T21:10:57Z

For the record -- I asked Cameron to merge it with a squash to save the time working around the merge commits from main.

He had powers because of the org-management permissions -- I'll look to be removing this shortly.

openshift-ci bot requested review from Gkrumbach07 and lucferbux April 4, 2023 20:40

openshift-ci bot added the needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI label Apr 4, 2023

andrewballantyne assigned lucferbux Apr 5, 2023

openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Apr 5, 2023

bartoszmajsak force-pushed the ossm_annotations branch from 8c638b3 to a3442eb Compare April 12, 2023 13:52

lucferbux requested changes Apr 17, 2023

View reviewed changes

backend/src/routes/api/namespaces/namespaceUtils.ts Outdated Show resolved Hide resolved

backend/src/routes/api/notebooks/utils.ts Outdated Show resolved Hide resolved

frontend/src/pages/projects/notebook/useRouteForNotebook.ts Outdated Show resolved Hide resolved

openshift-ci bot requested a review from cfchase April 17, 2023 18:19

bartoszmajsak force-pushed the ossm_annotations branch from a3442eb to 6c2c436 Compare April 18, 2023 17:16

openshift-merge-robot added the needs-rebase PR needs to be rebased label May 7, 2023

openshift-merge-robot removed the needs-rebase PR needs to be rebased label May 16, 2023

cam-garrison and others added 5 commits June 28, 2023 07:33

fix: adds mesh annotation to the ns

1408c29

feat: use annotation to fetch host for notebook routing (#5)

47e43ac

* update existing annotation reference * use ns annotation in backend route fetching

fix: patch notebook annotations (#6)

dd22cd7

bartoszmajsak force-pushed the ossm_annotations branch from 0e9952f to dd22cd7 Compare June 28, 2023 05:46

bartoszmajsak reviewed Jun 28, 2023

View reviewed changes

frontend/src/k8sTypes.ts Outdated Show resolved Hide resolved

bartoszmajsak requested a review from lucferbux June 28, 2023 17:42

Linting fixes

c917d76

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Jul 7, 2023

cam-garrison added 3 commits September 21, 2023 16:20

move getDashboardConfig call, remove unnec constant

6b12f82

simplify annotations fetch

ca51ef3

pass parsed SM flag instead ofdash config

74b4c79

cam-garrison requested a review from lucferbux October 3, 2023 14:43

add sm manifests changes

b55490f

cam-garrison mentioned this pull request Oct 4, 2023

OSSM: Add service mesh manifests overlay #1900

Closed

7 tasks

remove image patch

2d2244c

lucferbux requested changes Oct 4, 2023

View reviewed changes

christianvogt reviewed Oct 4, 2023

View reviewed changes

andrewballantyne requested changes Oct 4, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Oct 4, 2023

cam-garrison added 2 commits October 4, 2023 15:15

no longer poll on dashboardconfig, remove trailing slash

104c88b

remove unnecessary patches and rename patches in kustomization

df9f285

andrewballantyne added do-not-merge/hold This PR is hold for some reason lgtm approved labels Oct 4, 2023

cam-garrison merged commit e94a848 into opendatahub-io:f/ossm Oct 4, 2023
3 of 4 checks passed

andrewballantyne removed the do-not-merge/hold This PR is hold for some reason label Oct 4, 2023

christianvogt mentioned this pull request Oct 5, 2023

[Tracker]: Service Mesh #1838

Closed

bartoszmajsak mentioned this pull request Oct 5, 2023

feat(service-mesh): adds service mesh authz support for Dashboard and Workbenches opendatahub-io/opendatahub-operator#605

Closed

6 tasks

This was referenced Oct 11, 2023

OSSM: Consolidate Service Mesh related notebook patches #1954

Closed

[Service Mesh] include SM notebook patches in one function #1955

Merged

OSSM: General Improvements #1958

Closed

[Service Mesh] General Improvements #1959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ossm): adds OSSM annotations to the relevant cluster resources #1088

feat(ossm): adds OSSM annotations to the relevant cluster resources #1088

cam-garrison commented Apr 4, 2023 •

edited by lucferbux

Loading

openshift-ci bot commented Apr 4, 2023

andrewballantyne commented Apr 5, 2023

cam-garrison commented Apr 5, 2023

lucferbux left a comment

lucferbux commented May 8, 2023

cam-garrison commented Jun 28, 2023

lucferbux commented Jun 29, 2023

lucferbux commented Jun 29, 2023

lucferbux commented Jul 7, 2023

cam-garrison commented Sep 22, 2023

cam-garrison commented Oct 4, 2023 •

edited

Loading

cam-garrison commented Oct 4, 2023

andrewballantyne commented Oct 4, 2023

lucferbux left a comment

lucferbux Oct 4, 2023

lucferbux Oct 4, 2023

andrewballantyne Oct 4, 2023 •

edited

Loading

bartoszmajsak Oct 4, 2023

lucferbux Oct 4, 2023

andrewballantyne Oct 4, 2023

bartoszmajsak Oct 4, 2023

cam-garrison Oct 4, 2023

christianvogt Oct 4, 2023

christianvogt Oct 4, 2023

christianvogt Oct 4, 2023

andrewballantyne Oct 4, 2023

christianvogt Oct 4, 2023

christianvogt Oct 4, 2023

andrewballantyne Oct 4, 2023

andrewballantyne Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

andrewballantyne commented Oct 4, 2023 •

edited

Loading

		@@ -0,0 +1,10 @@
		# Each list is sorted alphabetically, additions should maintain that order

		export const featureFlagEnabled = (disabledSettingState?: boolean): boolean =>
		disabledSettingState === false;

	? getRoute(notebookName, projectName).then((route) => route?.spec.host)
	? getRoute(notebookName, projectName).then((route) => route.spec.host)

feat(ossm): adds OSSM annotations to the relevant cluster resources #1088

feat(ossm): adds OSSM annotations to the relevant cluster resources #1088

Conversation

cam-garrison commented Apr 4, 2023 • edited by lucferbux Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Apr 4, 2023

andrewballantyne commented Apr 5, 2023

cam-garrison commented Apr 5, 2023

lucferbux left a comment

Choose a reason for hiding this comment

lucferbux commented May 8, 2023

cam-garrison commented Jun 28, 2023

lucferbux commented Jun 29, 2023

lucferbux commented Jun 29, 2023

lucferbux commented Jul 7, 2023

cam-garrison commented Sep 22, 2023

cam-garrison commented Oct 4, 2023 • edited Loading

cam-garrison commented Oct 4, 2023

andrewballantyne commented Oct 4, 2023

lucferbux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 4, 2023

andrewballantyne commented Oct 4, 2023 • edited Loading

cam-garrison commented Apr 4, 2023 •

edited by lucferbux

Loading

cam-garrison commented Oct 4, 2023 •

edited

Loading

andrewballantyne Oct 4, 2023 •

edited

Loading

andrewballantyne commented Oct 4, 2023 •

edited

Loading