fix(backend): remove content disposition http header from artifact download #70

gregsheremeta · 2024-08-09T12:37:36Z

Description of your changes:
Remove content disposition http header from artifact download. This header prevents an artifact from being previewed/rendered in an iframe.

Fixes: https://issues.redhat.com/browse/RHOAIENG-11136

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

dsp-developers · 2024-08-09T12:38:36Z

Commit Checker results:

**NOTE**: These are the results of the commit checker scans. 
If these are not commits from upstream kfp, then please ensure
you adhere to the commit checker formatting

commitchecker verson unknown
Validating 1 commits between bbfb5897533403b38ad8956424931868c103b0b0...0494b12a68e70bf3e3d9935b16d41241d9dcebde

UPSTREAM commit 0494b12 has invalid summary DO NOT MERGE - testing removal of content disposition.

UPSTREAM commits are validated against the following regular expression:
  ^UPSTREAM: (revert: )?(([\w.-]+/[\w-.-]+)?: )?(\d+:|<carry>:|<drop>:)

UPSTREAM commit summaries should look like:

  UPSTREAM: <PR number|carry|drop>: description

UPSTREAM commits which revert previous UPSTREAM commits should look like:

  UPSTREAM: revert: <normal upstream format>

Examples of valid summaries:

  UPSTREAM: 12345: A kube fix
  UPSTREAM: <carry>: A carried kube change
  UPSTREAM: <drop>: A dropped kube change
  UPSTREAM: revert: 12345: A kube revert

dsp-developers · 2024-08-09T12:47:40Z

A set of new images have been built to help with testing out this PR:
API Server: quay.io/opendatahub/ds-pipelines-api-server:pr-70
DSP DRIVER: quay.io/opendatahub/ds-pipelines-driver:pr-70
DSP LAUNCHER: quay.io/opendatahub/ds-pipelines-launcher:pr-70
Persistence Agent: quay.io/opendatahub/ds-pipelines-persistenceagent:pr-70
Scheduled Workflow Manager: quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-70
MLMD Server: quay.io/opendatahub/mlmd-grpc-server:latest
MLMD Envoy Proxy: registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2
UI: quay.io/opendatahub/ds-pipelines-frontend:pr-70

dsp-developers · 2024-08-09T12:47:41Z

An OCP cluster where you are logged in as cluster admin is required.

The Data Science Pipelines team recommends testing this using the Data Science Pipelines Operator. Check here for more information on using the DSPO.

To use and deploy a DSP stack with these images (assuming the DSPO is deployed), first save the following YAML to a file named dspa.pr-70.yaml:

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: pr-70
spec:
  dspVersion: v2
  apiServer:
    image: "quay.io/opendatahub/ds-pipelines-api-server:pr-70"
    argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-70"
    argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-70"
  persistenceAgent:
    image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-70"
  scheduledWorkflow:
    image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-70"
  mlmd:  
    deploy: true  # Optional component
    grpc:
      image: "quay.io/opendatahub/mlmd-grpc-server:latest"
    envoy:
      image: "registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2"
  mlpipelineUI:
    deploy: true  # Optional component 
    image: "quay.io/opendatahub/ds-pipelines-frontend:pr-70"
  objectStorage:
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'

Then run the following:

cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines.git
cd data-science-pipelines/
git fetch origin pull/70/head
git checkout -b pullrequest 0494b12a68e70bf3e3d9935b16d41241d9dcebde
oc apply -f dspa.pr-70.yaml

More instructions here on how to deploy and test a Data Science Pipelines Application.

…ct download This header prevents an artifact from being previewed/rendered in an iframe. Remove it.

dsp-developers · 2024-08-09T15:52:19Z

Commit Checker results:

**NOTE**: These are the results of the commit checker scans. 
If these are not commits from upstream kfp, then please ensure
you adhere to the commit checker formatting

commitchecker verson unknown
Validating 1 commits between bbfb5897533403b38ad8956424931868c103b0b0...2829d6179ab680edea5091ece13f13b4c01a7dc9

diegolovison · 2024-08-09T16:00:01Z

What is going to happen to those who wish to download the file?
IMHO, the info should come from the dashboard, if they wish to display or download.
By default, if the parameter is not presented, the attachment; filename will be presented in the response.

dsp-developers · 2024-08-09T16:01:08Z

Change to PR detected. A new PR build was completed.
A set of new images have been built to help with testing out this PR:
API Server: quay.io/opendatahub/ds-pipelines-api-server:pr-70
DSP DRIVER: quay.io/opendatahub/ds-pipelines-driver:pr-70
DSP LAUNCHER: quay.io/opendatahub/ds-pipelines-launcher:pr-70
Persistence Agent: quay.io/opendatahub/ds-pipelines-persistenceagent:pr-70
Scheduled Workflow Manager: quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-70
MLMD Server: quay.io/opendatahub/mlmd-grpc-server:latest
MLMD Envoy Proxy: registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2
UI: quay.io/opendatahub/ds-pipelines-frontend:pr-70

andrewballantyne · 2024-08-09T16:12:58Z

What is going to happen to those who wish to download the file? IMHO, the info should come from the dashboard, if they wish to display or download. By default, if the parameter is not presented, the attachment; filename will be presented in the response.

The lack of the direct content disposition property doesn't change what you can do it changes what it will do by default.

Our goal in the UI will be to show content, not download it (when applicable). But there is some content that won't be showable in a "nice way" -- such as say Model Artifacts. Those will download. Basically the removal here is leaving it up the source to decide what to do.

When we display content in a new tab, and you wanted it downloaded (ie HTML page) you can right click and say "save as..." (or equiv wording depending on your browser).

diegolovison · 2024-08-09T16:16:46Z

When we display content in a new tab, and you want it downloaded

How about the name of the file? Is it going to match what was saved?

DharmitD

/lgtm

andrewballantyne · 2024-08-09T16:43:44Z

When we display content in a new tab, and you want it downloaded

How about the name of the file? Is it going to match what was saved?

This is defined by your browser implementation. The content will be whatever you decide and is prefilled with whatever the browser decides.

Example for the STRAT behind this feature:

document.title
'[RHOAISTRAT-247] Implement artifacts API in Data Science Pipelines Backend - Red Hat Issue Tracker'

Milage may vary -- we'd need to test specific named resources to see. Your HTML resource can set this value, not sure about other resources. Not sure there is much we can do here ... we'll have to see once we get some use-cases going and determining the expectation / desires. This change is needed to show anything in the Dashboard though, so it may be a loss for the larger gain. Again, we'll need to test this out from the Dashboard perspective.

diegolovison · 2024-08-09T16:58:11Z

I am creating a cluster to test this PR. Give me a few minutes.

diegolovison · 2024-08-09T18:39:37Z

If we merge this PR, the user won't know the file name that was stored.
For each pipeline step, they will need to remember and set a name for the file. Each step can have multiple outputs as well.

Here is a pipeline example:

@dsl.component....
def fn1...
    with open(iris_dataset.path, "w") as f:
        df.to_csv(f)

@dsl.component....
def fn2....
    with open(normalized_iris_dataset.path, "w") as f:
        df.to_csv(f)

@dsl.component....
def fn3....
    with open(model.path, "wb") as f:
        pickle.dump(clf, f)

gregsheremeta · 2024-08-09T23:26:35Z

Apparently the content disposition inline could be used to do that. Ref: https://stackoverflow.com/questions/1741353/how-to-set-response-filename-without-forcing-save-as-dialog

gregsheremeta · 2024-08-09T23:27:03Z

/hold

@andrewballantyne how do you want to proceed?

HumairAK · 2024-08-12T14:17:27Z

/unhold
/lgtm
/approve

openshift-ci · 2024-08-12T14:17:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK

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

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

dsp-developers · 2024-08-12T14:24:14Z

Commit Checker results:

**NOTE**: These are the results of the commit checker scans. 
If these are not commits from upstream kfp, then please ensure
you adhere to the commit checker formatting

commitchecker verson unknown
Validating 0 commits between d0ee4ca4524ece9f201581345ef791a1d4ee8e68...2829d6179ab680edea5091ece13f13b4c01a7dc9

andrewballantyne · 2024-08-12T14:36:25Z

@andrewballantyne how do you want to proceed?

For posterity, we discussed it and want to move forward with this API change and see about solving this purely from the UI.

openshift-ci bot requested review from gmfrasca and rimolive August 9, 2024 12:37

UPSTREAM: <carry>: remove content disposition http header from artifa…

2829d61

…ct download This header prevents an artifact from being previewed/rendered in an iframe. Remove it.

gregsheremeta force-pushed the remove-content-disposition-header branch from 0494b12 to 2829d61 Compare August 9, 2024 15:51

gregsheremeta changed the title ~~DO NOT MERGE - testing removal of content disposition~~ remove content disposition http header from artifact download Aug 9, 2024

gregsheremeta changed the title ~~remove content disposition http header from artifact download~~ fix(backend): remove content disposition http header from artifact download Aug 9, 2024

DharmitD reviewed Aug 9, 2024

View reviewed changes

openshift-ci bot assigned DharmitD Aug 9, 2024

openshift-ci bot added the lgtm label Aug 9, 2024

openshift-ci bot added the do-not-merge/hold label Aug 9, 2024

openshift-ci bot removed the do-not-merge/hold label Aug 12, 2024

openshift-ci bot assigned HumairAK Aug 12, 2024

openshift-ci bot added the approved label Aug 12, 2024

openshift-merge-bot bot merged commit d0ee4ca into opendatahub-io:master Aug 12, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(backend): remove content disposition http header from artifact download #70

fix(backend): remove content disposition http header from artifact download #70

gregsheremeta commented Aug 9, 2024 •

edited

Loading

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

diegolovison commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

andrewballantyne commented Aug 9, 2024 •

edited

Loading

diegolovison commented Aug 9, 2024

DharmitD left a comment

andrewballantyne commented Aug 9, 2024

diegolovison commented Aug 9, 2024 •

edited

Loading

diegolovison commented Aug 9, 2024

gregsheremeta commented Aug 9, 2024

gregsheremeta commented Aug 9, 2024

HumairAK commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

dsp-developers commented Aug 12, 2024

andrewballantyne commented Aug 12, 2024

fix(backend): remove content disposition http header from artifact download #70

fix(backend): remove content disposition http header from artifact download #70

Conversation

gregsheremeta commented Aug 9, 2024 • edited Loading

dsp-developers commented Aug 9, 2024

Commit Checker results:

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

Commit Checker results:

diegolovison commented Aug 9, 2024

dsp-developers commented Aug 9, 2024

andrewballantyne commented Aug 9, 2024 • edited Loading

diegolovison commented Aug 9, 2024

DharmitD left a comment

Choose a reason for hiding this comment

andrewballantyne commented Aug 9, 2024

diegolovison commented Aug 9, 2024 • edited Loading

diegolovison commented Aug 9, 2024

gregsheremeta commented Aug 9, 2024

gregsheremeta commented Aug 9, 2024

HumairAK commented Aug 12, 2024

openshift-ci bot commented Aug 12, 2024

dsp-developers commented Aug 12, 2024

Commit Checker results:

andrewballantyne commented Aug 12, 2024

gregsheremeta commented Aug 9, 2024 •

edited

Loading

andrewballantyne commented Aug 9, 2024 •

edited

Loading

diegolovison commented Aug 9, 2024 •

edited

Loading