Change Elyra public API endpoint to ensure a correct URL for redirect #1461

mlassak · 2023-07-04T12:59:54Z

Complementary PR from notebooks: opendatahub-io/notebooks#137

Description

Change the Elyra public API endpoint URL so that it correctly references to the desired data science pipeline run details.

How Has This Been Tested?

Not fully tested yet, testing in progress

Test Impact

Request review criteria:

The commits have meaningful messages (squashes happen on merge by the bot).
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)

andrewballantyne · 2023-07-04T14:15:29Z

Interesting -- not sure this solves the problem -- it'll just shift it to another problem.

Although I agree, the internal routing structure of Dashboard should be maintained on our end... but we should probably change that property to be something else or have two properties or something. You're going to not be able to handle upgrades this way, since that route will just fail on the Dashboard as-is on upgrade.

@harshad16 we should be careful about how things are handled here... can we use two params? One for "this is where runs exists" and one for "this is the path to the run, insert ID at the end" so that it always works and doesn't break. It'll still suck for the old notebooks with an out of date secret, but at least it won't go from working badly to a 404 page.

EDIT: Also we need to handle upgrades / updating the secret when it lacks the suffix value in question.

harshad16 · 2023-07-06T22:51:03Z

Interesting -- not sure this solves the problem -- it'll just shift it to another problem.

@andrewballantyne , i m not sure, which another problem will this cause?

Although I agree, the internal routing structure of Dashboard should be maintained on our end... but we should probably change that property to be something else or have two properties or something. You're going to not be able to handle upgrades this way, since that route will just fail on the Dashboard as-is on upgrade.

Is this comment, in regards to dashboard-and-components would be agnostic each other in future.
Currently, the requirement was to redirect users to pipelinerun of the execution, this would help with that.

we should be careful about how things are handled here... can we use two params? One for "this is where runs exists" and one for "this is the path to the run, insert ID at the end" so that it always works and doesn't break. It'll still suck for the old notebooks with an out of date secret, but at least it won't go from working badly to a 404 page.

Can please share some more information here,
as with testing, this would take users to the runs they executed.

EDIT: Also we need to handle upgrades / updating the secret when it lacks the suffix value in question.

Agreed, in long run we should focus on this.

harshad16 · 2023-07-06T23:05:29Z

/lgtm

thanks 💯

Tested on cluster, the redirect takes directly to the pipelineruns

lucferbux · 2023-07-10T07:00:47Z

/lgtm

thanks 💯

Tested on cluster, the redirect takes directly to the pipelineruns

Hi @harshad16 can you share the testing details here please? I would like to replicate it to review this.
Thanks in advance!

harshad16 · 2023-07-10T13:48:58Z

Testing procedure:

Install RHODS/ODH
Stop the operator, and switch the pr image in odh-dashboard deployment.
Head to data science project, create project.
create a pipeline server
create a workbench
the result can be checked either in secret/ds-pipeline-config or in workbench, on left panel called
runtime configurations > data science pipeline for the UI view.

@lucferbux

lucferbux · 2023-07-11T11:15:48Z

Testing procedure:

Install RHODS/ODH

Stop the operator, and switch the pr image in odh-dashboard deployment.

Head to data science project, create project.

create a pipeline server

create a workbench

the result can be checked either in secret/ds-pipeline-config or in workbench, on left panel called
runtime configurations > data science pipeline for the UI view.

@lucferbux

thanks! ill try to review it today, if not ill get to that next Tuesday (I'm on to the rest of the week)

harshad16 · 2023-07-11T13:33:03Z

@lucferbux @andrewballantyne please try to review in this week
this is bounded by an issue we want in this sprint.
opendatahub-io/notebooks#130

andrewballantyne · 2023-07-11T13:55:59Z

Will discuss with Harshad later today... holding until we cover my concerns with this PR. Apologizes for the delay, behind on emails.

andrewballantyne

Could you do these changes (patch to apply below):

diff --git a/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts b/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts
index c703e519..317fec25 100644
--- a/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts
+++ b/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts
@@ -4,7 +4,11 @@ import { createSecret, replaceSecret } from '~/api';
 import { DSPipelineKind } from '~/k8sTypes';
 import { generateElyraSecret } from '~/concepts/pipelines/elyra/utils';
 import useAWSSecret from '~/concepts/secrets/apiHooks/useAWSSecret';
-import { ELYRA_SECRET_DATA_KEY, ELYRA_SECRET_DATA_TYPE } from '~/concepts/pipelines/elyra/const';
+import {
+  ELYRA_SECRET_DATA_ENDPOINT,
+  ELYRA_SECRET_DATA_KEY,
+  ELYRA_SECRET_DATA_TYPE,
+} from '~/concepts/pipelines/elyra/const';

 const useManageElyraSecret = (
   namespace: string,
@@ -41,9 +45,14 @@ const useManageElyraSecret = (
         return;
       }
       try {
-        const secretValue = JSON.parse(atob(elyraSecret.data?.[ELYRA_SECRET_DATA_KEY] || '{}'));
-        if (secretValue.metadata[ELYRA_SECRET_DATA_TYPE] === 'USER_CREDENTIALS') {
-          // Secret is invalid -- update it
+        const secretValue = JSON.parse(
+          atob(elyraSecret.data?.[ELYRA_SECRET_DATA_KEY] || '{ metadata: {} }'),
+        );
+        const usingOldDataType =
+          secretValue.metadata[ELYRA_SECRET_DATA_TYPE] === 'USER_CREDENTIALS';
+        const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');
+        if (usingOldDataType || usingOldUrl) {
+          // Secret is out of date, update it
           replaceSecret(
             generateElyraSecret(
               dataConnection.data,
diff --git a/frontend/src/concepts/pipelines/elyra/const.ts b/frontend/src/concepts/pipelines/elyra/const.ts
index 24c250aa..7db53078 100644
--- a/frontend/src/concepts/pipelines/elyra/const.ts
+++ b/frontend/src/concepts/pipelines/elyra/const.ts
@@ -3,4 +3,5 @@ import { PIPELINE_DEFINITION_NAME } from '~/concepts/pipelines/const';
 export const ELYRA_SECRET_NAME = 'ds-pipeline-config';
 export const ELYRA_SECRET_DATA_KEY = 'odh_dsp.json';
 export const ELYRA_SECRET_DATA_TYPE = 'cos_auth_type';
+export const ELYRA_SECRET_DATA_ENDPOINT = 'public_api_endpoint';
 export const ELYRA_ROLE_NAME = `ds-pipeline-user-access-${PIPELINE_DEFINITION_NAME}`;
diff --git a/frontend/src/concepts/pipelines/elyra/utils.ts b/frontend/src/concepts/pipelines/elyra/utils.ts
index 2888c525..66f761dc 100644
--- a/frontend/src/concepts/pipelines/elyra/utils.ts
+++ b/frontend/src/concepts/pipelines/elyra/utils.ts
@@ -2,6 +2,7 @@ import { Patch } from '@openshift/dynamic-plugin-sdk-utils';
 import { AWSSecretKind, KnownLabels, NotebookKind, RoleBindingKind, SecretKind } from '~/k8sTypes';
 import {
   ELYRA_ROLE_NAME,
+  ELYRA_SECRET_DATA_ENDPOINT,
   ELYRA_SECRET_DATA_KEY,
   ELYRA_SECRET_DATA_TYPE,
   ELYRA_SECRET_NAME,
@@ -63,7 +64,8 @@ export const generateElyraSecret = (
         engine: 'Tekton',
         auth_type: 'KUBERNETES_SERVICE_ACCOUNT_TOKEN',
         api_endpoint: route,
-        public_api_endpoint: `${location.origin}/pipelineRuns/${namespace}`,
+        // Append the id on the end to navigate to the details page for that PipelineRun
+        [ELYRA_SECRET_DATA_ENDPOINT]: `${location.origin}/pipelineRuns/${namespace}/pipelineRun/view/`,
         [ELYRA_SECRET_DATA_TYPE]: 'KUBERNETES_SECRET',
         cos_secret: dataConnectionName,
         cos_endpoint: atob(dataConnectionData[AWS_KEYS.S3_ENDPOINT]),

That'll allow us to update the secret for existing items.

andrewballantyne

Code looks good... @harshad16 have you retested this to make sure it works?

harshad16 · 2023-07-13T15:10:27Z

frontend/src/concepts/pipelines/context/useManageElyraSecret.ts

+        );
+        const usingOldDataType =
+          secretValue.metadata[ELYRA_SECRET_DATA_TYPE] === 'USER_CREDENTIALS';
+        const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');


Code looks good... have you retested this to make sure it works?

I have tested the 2 scenario ,

in a new cluster setup, it works as we want.

In a old cluster setup, the value doesn't get updated.
i feel the reason is because are not checking the right condition:

Suggested change

const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');

const usingOldUrl = !secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');

ooooh, good catch. That was supposed to be a not. @mlassak please make that change.

harshad16 · 2023-07-14T14:59:30Z

Thanks for the updated.
Retested the 2nd scenario of old notebook run,
the changes gets updated.

Pasting only the secret, the manage field show update operation and correctly updates the field

kind: Secret
apiVersion: v1
metadata:
  name: ds-pipeline-config
  namespace: test
  uid: ddf1a7ed-4e63-40de-b102-6b2f38a85133
  resourceVersion: '1156151'
  creationTimestamp: '2023-07-13T15:00:53Z'
  managedFields:
    - manager: unknown
      operation: Update
      apiVersion: v1
      time: '2023-07-14T14:56:52Z'
      fieldsType: FieldsV1
      fieldsV1:
        'f:data':
          .: {}
          'f:odh_dsp.json': {}
        'f:type': {}

/lgtm

openshift-ci · 2023-07-14T15:34:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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~~ [andrewballantyne]

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

…rect-elyra-redirect" This reverts commit 672b772, reversing changes made to 4af4471.

openshift-ci bot requested review from alexcreasy and lucferbux July 4, 2023 13:00

mlassak mentioned this pull request Jul 4, 2023

Update datascience pipeline run URL in Elyra opendatahub-io/notebooks#137

Merged

3 tasks

lucferbux requested a review from Gkrumbach07 July 6, 2023 11:25

openshift-ci bot assigned harshad16 Jul 6, 2023

openshift-ci bot added the lgtm label Jul 6, 2023

andrewballantyne added the do-not-merge/hold This PR is hold for some reason label Jul 11, 2023

andrewballantyne requested changes Jul 11, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Jul 11, 2023

openshift-ci bot removed the lgtm label Jul 11, 2023

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

mlassak force-pushed the fix/incorrect-elyra-redirect branch 2 times, most recently from efbaa95 to becccf7 Compare July 12, 2023 08:58

andrewballantyne reviewed Jul 12, 2023

View reviewed changes

harshad16 reviewed Jul 13, 2023

View reviewed changes

Update elyra public API endpoint to ensure a correct URL for redirect

0ac1dbf

mlassak force-pushed the fix/incorrect-elyra-redirect branch from becccf7 to 0ac1dbf Compare July 14, 2023 09:24

openshift-ci bot added the lgtm label Jul 14, 2023

andrewballantyne approved these changes Jul 14, 2023

View reviewed changes

openshift-ci bot added the approved label Jul 14, 2023

openshift-merge-robot merged commit 672b772 into opendatahub-io:main Jul 14, 2023
4 checks passed

harshad16 mentioned this pull request Jul 14, 2023

Elyra does not redirect to the correct URL after submitting a pipeline run opendatahub-io/notebooks#130

Closed

alexcreasy added a commit to alexcreasy/odh-dashboard that referenced this pull request Jul 18, 2023

Revert "Merge pull request opendatahub-io#1461 from mlassak/fix/incor…

310544f

…rect-elyra-redirect" This reverts commit 672b772, reversing changes made to 4af4471.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Elyra public API endpoint to ensure a correct URL for redirect #1461

Change Elyra public API endpoint to ensure a correct URL for redirect #1461

mlassak commented Jul 4, 2023

andrewballantyne commented Jul 4, 2023 •

edited

Loading

harshad16 commented Jul 6, 2023

harshad16 commented Jul 6, 2023

lucferbux commented Jul 10, 2023

harshad16 commented Jul 10, 2023

lucferbux commented Jul 11, 2023

harshad16 commented Jul 11, 2023

andrewballantyne commented Jul 11, 2023

andrewballantyne left a comment

andrewballantyne left a comment

harshad16 Jul 13, 2023

andrewballantyne Jul 13, 2023

harshad16 commented Jul 14, 2023

openshift-ci bot commented Jul 14, 2023

	const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');
	const usingOldUrl = !secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');

Change Elyra public API endpoint to ensure a correct URL for redirect #1461

Change Elyra public API endpoint to ensure a correct URL for redirect #1461

Conversation

mlassak commented Jul 4, 2023

Description

How Has This Been Tested?

Test Impact

Request review criteria:

andrewballantyne commented Jul 4, 2023 • edited Loading

harshad16 commented Jul 6, 2023

harshad16 commented Jul 6, 2023

lucferbux commented Jul 10, 2023

harshad16 commented Jul 10, 2023

lucferbux commented Jul 11, 2023

harshad16 commented Jul 11, 2023

andrewballantyne commented Jul 11, 2023

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

harshad16 Jul 13, 2023

Choose a reason for hiding this comment

andrewballantyne Jul 13, 2023

Choose a reason for hiding this comment

harshad16 commented Jul 14, 2023

openshift-ci bot commented Jul 14, 2023

andrewballantyne commented Jul 4, 2023 •

edited

Loading