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

UPSTREAM: <carry>: Update ArtifactView enum to allow pipelines artifacts api to have optional content disposition #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

UPSTREAM: <carry>: Update ArtifactView enum to allow pipelines artifacts api to have optional content disposition #118

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
7 changes: 7 additions & 0 deletions backend/api/v2beta1/artifacts.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ message GetArtifactRequest {

// Server responses include download_url
DOWNLOAD = 2;

// Server response includes a signed URL,
// allowing in-browser rendering or preview of the artifact.
RENDER = 3;
}

// Optional. Set to "DOWNLOAD" to included a signed URL with
Expand Down Expand Up @@ -137,4 +141,7 @@ message Artifact {
// and the error message is returned. Client has the flexibility of choosing
// how to handle the error. This is especially useful when calling ListArtifacts.
google.rpc.Status error = 11;
// Optional Output. Specifies a signed URL that can be used to
// render this Artifact directly from its store.
string render_url = 12;
}
192 changes: 105 additions & 87 deletions backend/api/v2beta1/go_client/artifacts.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 10 additions & 4 deletions backend/api/v2beta1/swagger/artifacts.swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,15 @@
},
{
"name": "view",
"description": "Optional. Set to \"DOWNLOAD\" to included a signed URL with\nan expiry (default 15 seconds, unless configured other wise).\nThis URL can be used to download the Artifact directly from\nthe Artifact's storage provider. Set to \"BASIC\" to exclude\nthe download_url from server responses, thus preventing the\ncreation of any signed url.\nDefaults to BASIC.\n\n - ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url",
"description": "Optional. Set to \"DOWNLOAD\" to included a signed URL with\nan expiry (default 15 seconds, unless configured other wise).\nThis URL can be used to download the Artifact directly from\nthe Artifact's storage provider. Set to \"BASIC\" to exclude\nthe download_url from server responses, thus preventing the\ncreation of any signed url.\nDefaults to BASIC.\n\n - ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url\n - RENDER: Server response includes a signed URL,\nallowing in-browser rendering or preview of the artifact.",
"in": "query",
"required": false,
"type": "string",
"enum": [
"ARTIFACT_VIEW_UNSPECIFIED",
"BASIC",
"DOWNLOAD"
"DOWNLOAD",
"RENDER"
],
"default": "ARTIFACT_VIEW_UNSPECIFIED"
}
Expand All @@ -131,10 +132,11 @@
"enum": [
"ARTIFACT_VIEW_UNSPECIFIED",
"BASIC",
"DOWNLOAD"
"DOWNLOAD",
"RENDER"
],
"default": "ARTIFACT_VIEW_UNSPECIFIED",
"title": "- ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url"
"description": " - ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url\n - RENDER: Server response includes a signed URL,\nallowing in-browser rendering or preview of the artifact."
},
"ListArtifactRequestField": {
"type": "string",
Expand Down Expand Up @@ -253,6 +255,10 @@
"error": {
"$ref": "#/definitions/googlerpcStatus",
"description": "In case any error happens retrieving an artifact field, only artifact ID\nand the error message is returned. Client has the flexibility of choosing\nhow to handle the error. This is especially useful when calling ListArtifacts."
},
"render_url": {
"type": "string",
"description": "Optional Output. Specifies a signed URL that can be used to\nrender this Artifact directly from its store."
}
}
},
Expand Down
14 changes: 10 additions & 4 deletions backend/api/v2beta1/swagger/kfp_api_single_file.swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,15 @@
},
{
"name": "view",
"description": "Optional. Set to \"DOWNLOAD\" to included a signed URL with\nan expiry (default 15 seconds, unless configured other wise).\nThis URL can be used to download the Artifact directly from\nthe Artifact's storage provider. Set to \"BASIC\" to exclude\nthe download_url from server responses, thus preventing the\ncreation of any signed url.\nDefaults to BASIC.\n\n - ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url",
"description": "Optional. Set to \"DOWNLOAD\" to included a signed URL with\nan expiry (default 15 seconds, unless configured other wise).\nThis URL can be used to download the Artifact directly from\nthe Artifact's storage provider. Set to \"BASIC\" to exclude\nthe download_url from server responses, thus preventing the\ncreation of any signed url.\nDefaults to BASIC.\n\n - ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url\n - RENDER: Server response includes a signed URL,\nallowing in-browser rendering or preview of the artifact.",
"in": "query",
"required": false,
"type": "string",
"enum": [
"ARTIFACT_VIEW_UNSPECIFIED",
"BASIC",
"DOWNLOAD"
"DOWNLOAD",
"RENDER"
],
"default": "ARTIFACT_VIEW_UNSPECIFIED"
}
Expand Down Expand Up @@ -1644,10 +1645,11 @@
"enum": [
"ARTIFACT_VIEW_UNSPECIFIED",
"BASIC",
"DOWNLOAD"
"DOWNLOAD",
"RENDER"
],
"default": "ARTIFACT_VIEW_UNSPECIFIED",
"title": "- ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url"
"description": " - ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url\n - RENDER: Server response includes a signed URL,\nallowing in-browser rendering or preview of the artifact."
},
"ListArtifactRequestField": {
"type": "string",
Expand Down Expand Up @@ -1766,6 +1768,10 @@
"error": {
"$ref": "#/definitions/googlerpcStatus",
"description": "In case any error happens retrieving an artifact field, only artifact ID\nand the error message is returned. Client has the flexibility of choosing\nhow to handle the error. This is especially useful when calling ListArtifacts."
},
"render_url": {
"type": "string",
"description": "Optional Output. Specifies a signed URL that can be used to\nrender this Artifact directly from its store."
}
}
},
Expand Down
8 changes: 8 additions & 0 deletions backend/src/apiserver/resource/resource_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2052,6 +2052,14 @@ func (r *ResourceManager) GetSignedUrl(bucketConfig *objectstore.Config, secret
return signedUrl, nil
}

func (r *ResourceManager) GetSignedUrlWithoutContentDisposition(bucketConfig *objectstore.Config, secret *corev1.Secret, expirySeconds time.Duration, artifactURI string) (string, error) {
signedUrl, err := r.objectStore.GetSignedUrlWithoutContentDisposition(bucketConfig, secret, expirySeconds, artifactURI)
if err != nil {
return "", err
}
return signedUrl, nil
}

// GetObjectSize retrieves the size of the Artifact's object in bytes.
func (r *ResourceManager) GetObjectSize(bucketConfig *objectstore.Config, secret *corev1.Secret, artifactURI string) (int64, error) {
size, err := r.objectStore.GetObjectSize(bucketConfig, secret, artifactURI)
Expand Down
4 changes: 4 additions & 0 deletions backend/src/apiserver/resource/resource_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ func (m *FakeBadObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secr
return "", util.NewInternalServerError(errors.New("Error"), "bad object store")
}

func (m *FakeBadObjectStore) GetSignedUrlWithoutContentDisposition(bucketConfig *objectstore.Config, secret *corev1.Secret, expirySeconds time.Duration, artifactURI string) (string, error) {
return "", util.NewInternalServerError(errors.New("Error"), "bad object store")
}

func (m *FakeBadObjectStore) GetObjectSize(bucketConfig *objectstore.Config, secret *corev1.Secret, artifactURI string) (int64, error) {
return 0, util.NewInternalServerError(errors.New("Error"), "bad object store")
}
Expand Down
23 changes: 18 additions & 5 deletions backend/src/apiserver/server/artifact_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (s *ArtifactServer) ListArtifacts(ctx context.Context, r *apiv2beta1.ListAr
if err1 != nil || bucketConfig == nil {
return nil, util.NewInternalServerError(fmt.Errorf("failed to retrieve session info error: %v", err1), artifactId)
}
artifactResp, err1 := s.generateResponseArtifact(ctx, artifact, bucketConfig, namespace, false)
artifactResp, err1 := s.generateResponseArtifact(ctx, artifact, bucketConfig, namespace, false, false)
if err1 != nil {
return nil, util.NewInternalServerError(fmt.Errorf("encountered error parsing artifact: %v", err), artifactId)
}
Expand Down Expand Up @@ -186,12 +186,15 @@ func (s *ArtifactServer) GetArtifact(ctx context.Context, r *apiv2beta1.GetArtif
return nil, util.Wrap(err, "Failed to authorize the request")
}

downloadURL := false
if r.GetView() == apiv2beta1.GetArtifactRequest_DOWNLOAD {
downloadURL, renderURL := false, false

switch r.GetView() {
case apiv2beta1.GetArtifactRequest_DOWNLOAD:
downloadURL = true
case apiv2beta1.GetArtifactRequest_RENDER:
renderURL = true
}

artifactResp, err := s.generateResponseArtifact(ctx, artifact, sessionInfo, namespace, downloadURL)
artifactResp, err := s.generateResponseArtifact(ctx, artifact, sessionInfo, namespace, downloadURL, renderURL)
if err != nil {
return nil, util.NewInternalServerError(fmt.Errorf("encountered error parsing artifact: %v", err), r.ArtifactId)
}
Expand All @@ -212,6 +215,7 @@ func (s *ArtifactServer) generateResponseArtifact(
bucketConfig *objectstore.Config,
namespace string,
includeShareUrl bool,
includeRenderUrl bool,
Copy link

Choose a reason for hiding this comment

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

Optional, but rather than a bool, you could replace includeShareUrl and includeRenderUrl with a single argument of type GetArtifactRequest_ArtifactView since it doesn't seem like this will ever need to generate more than one. This would allow for additional options in the future.

) (*apiv2beta1.Artifact, error) {
params, err := objectstore.StructuredS3Params(bucketConfig.SessionInfo.Params)
if err != nil {
Expand Down Expand Up @@ -251,6 +255,15 @@ func (s *ArtifactServer) generateResponseArtifact(
artifactResp.DownloadUrl = shareUrl
}

if includeRenderUrl {
expiry := time.Second * time.Duration(common.GetSignedURLExpiryTimeSeconds())
renderUrl, err := s.resourceManager.GetSignedUrlWithoutContentDisposition(bucketConfig, secret, expiry, *artifact.Uri)
if err != nil {
return nil, err
}
artifactResp.RenderUrl = renderUrl
}

return artifactResp, nil
}

Expand Down
28 changes: 25 additions & 3 deletions backend/src/apiserver/server/artifact_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ package server

import (
"context"
apiv2beta1 "github.com/kubeflow/pipelines/backend/api/v2beta1/go_client"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/types/known/timestamppb"
"strings"
"testing"
"time"

apiv2beta1 "github.com/kubeflow/pipelines/backend/api/v2beta1/go_client"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/types/known/timestamppb"
)

func TestGetArtifacts(t *testing.T) {
Expand Down Expand Up @@ -76,6 +77,27 @@ func TestGetArtifacts(t *testing.T) {
LastUpdatedAt: timestamppb.New(time.Unix(2, 0)),
},
},
{
name: "Fetch artifact 2",
artifactRequest: &apiv2beta1.GetArtifactRequest{
ArtifactId: "0",
View: apiv2beta1.GetArtifactRequest_RENDER,
},
expectedArtifact: &apiv2beta1.Artifact{
ArtifactId: "0",
StorageProvider: "s3",
StoragePath: "pipeline/some-pipeline-id/task/key0",
Uri: "s3://test-bucket/pipeline/some-pipeline-id/task/key0",
DownloadUrl: "",
RenderUrl: "dummy-render-url", // defined in object_store_fake
Namespace: "test-namespace",
ArtifactType: "1",
ArtifactSize: 123,

CreatedAt: timestamppb.New(time.Unix(1, 0)),
LastUpdatedAt: timestamppb.New(time.Unix(1, 0)),
},
},
{
name: "Fetch artifact 1 - View unspecified",
artifactRequest: &apiv2beta1.GetArtifactRequest{
Expand Down
22 changes: 22 additions & 0 deletions backend/src/apiserver/storage/object_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type ObjectStoreInterface interface {
GetFromYamlFile(o interface{}, filePath string) error
GetPipelineKey(pipelineId string) string
GetSignedUrl(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string) (string, error)
GetSignedUrlWithoutContentDisposition(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string) (string, error)
Copy link

Choose a reason for hiding this comment

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

Could we make this more configurable? For example, the following would allow further customizations without having to add a new method for each variation.

Suggested change
GetSignedUrlWithoutContentDisposition(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string) (string, error)
GetSignedUrlWithQueryParams(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string, queryParams url.Values)

GetObjectSize(bucketConfig *objectstore.Config, secret *v1.Secret, artifactURI string) (int64, error)
}

Expand Down Expand Up @@ -143,6 +144,27 @@ func (m *MinioObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secret
return "", err
}
reqParams := make(url.Values)
reqParams.Set("response-content-disposition", "attachment; filename=\""+key+"\"")
Copy link

Choose a reason for hiding this comment

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

If you go with my comment of having the method GetSignedUrlWithQueryParams(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string, queryParams url.Values) , then you can just have GetSignedUrl wrap GetSignedUrlWithQueryParams and pass the content disposition to queryParams.

This will reduce code duplication.

signedUrl, err := s3Client.Presign("GET", bucketConfig.BucketName, key, expirySeconds, reqParams)
if err != nil {
return "", util.Wrap(err, "Failed to generate signed url")
}

return signedUrl.String(), nil
}

func (m *MinioObjectStore) GetSignedUrlWithoutContentDisposition(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string) (string, error) {
Copy link

Choose a reason for hiding this comment

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

What's the plan for test coverage of this method since the tests I see are mocking this out? Is this a separate task for an integration test?

s3Client, err := buildClientFromConfig(bucketConfig, secret)
if err != nil {
return "", err
}

key, err := objectstore.ArtifactKeyFromURI(artifactURI)
if err != nil {
return "", err
}
reqParams := make(url.Values)
reqParams.Set("response-content-disposition", "inline")
signedUrl, err := s3Client.Presign("GET", bucketConfig.BucketName, key, expirySeconds, reqParams)
if err != nil {
return "", util.Wrap(err, "Failed to generate signed url")
Expand Down
9 changes: 7 additions & 2 deletions backend/src/apiserver/storage/object_store_fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
package storage

import (
"github.com/kubeflow/pipelines/backend/src/v2/objectstore"
"k8s.io/api/core/v1"
"time"

"github.com/kubeflow/pipelines/backend/src/v2/objectstore"
v1 "k8s.io/api/core/v1"
)

type fakeMinioObjectStore struct {
Expand Down Expand Up @@ -52,6 +53,10 @@ func (m *fakeMinioObjectStore) GetSignedUrl(*objectstore.Config, *v1.Secret, tim
return "dummy-signed-url", nil
}

func (m *fakeMinioObjectStore) GetSignedUrlWithoutContentDisposition(*objectstore.Config, *v1.Secret, time.Duration, string) (string, error) {
return "dummy-render-url", nil
}

func (m *fakeMinioObjectStore) GetObjectSize(*objectstore.Config, *v1.Secret, string) (int64, error) {
return 123, nil
}
Expand Down
Loading