Skip to content

Commit 7214c8c

Browse files
VaniHaripriyaVaniHaripriya
committed
UPSTREAM: <carry>:Update ArifactView enum to allow pipelines artifacts api to have optional content disposition
Signed-off-by: VaniHaripriya <[email protected]>
1 parent 0487d73 commit 7214c8c

File tree

10 files changed

+201
-120
lines changed

10 files changed

+201
-120
lines changed

‎backend/api/v2beta1/artifacts.proto

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,10 @@ message GetArtifactRequest {
5555

5656
// Server responses include download_url
5757
DOWNLOAD = 2;
58+
59+
// Server response includes a signed URL,
60+
// allowing in-browser rendering or preview of the artifact.
61+
RENDER = 3;
5862
}
5963

6064
// Optional. Set to "DOWNLOAD" to included a signed URL with
@@ -137,4 +141,7 @@ message Artifact {
137141
// and the error message is returned. Client has the flexibility of choosing
138142
// how to handle the error. This is especially useful when calling ListArtifacts.
139143
google.rpc.Status error = 11;
144+
// Optional Output. Specifies a signed URL that can be used to
145+
// render this Artifact directly from its store.
146+
string render_url = 12;
140147
}

‎backend/api/v2beta1/go_client/artifacts.pb.go

Lines changed: 105 additions & 87 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎backend/api/v2beta1/swagger/artifacts.swagger.json

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -107,14 +107,15 @@
107107
},
108108
{
109109
"name": "view",
110-
"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",
110+
"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.",
111111
"in": "query",
112112
"required": false,
113113
"type": "string",
114114
"enum": [
115115
"ARTIFACT_VIEW_UNSPECIFIED",
116116
"BASIC",
117-
"DOWNLOAD"
117+
"DOWNLOAD",
118+
"RENDER"
118119
],
119120
"default": "ARTIFACT_VIEW_UNSPECIFIED"
120121
}
@@ -131,10 +132,11 @@
131132
"enum": [
132133
"ARTIFACT_VIEW_UNSPECIFIED",
133134
"BASIC",
134-
"DOWNLOAD"
135+
"DOWNLOAD",
136+
"RENDER"
135137
],
136138
"default": "ARTIFACT_VIEW_UNSPECIFIED",
137-
"title": "- ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url"
139+
"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."
138140
},
139141
"ListArtifactRequestField": {
140142
"type": "string",
@@ -253,6 +255,10 @@
253255
"error": {
254256
"$ref": "#/definitions/googlerpcStatus",
255257
"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."
258+
},
259+
"render_url": {
260+
"type": "string",
261+
"description": "Optional Output. Specifies a signed URL that can be used to\nrender this Artifact directly from its store."
256262
}
257263
}
258264
},

‎backend/api/v2beta1/swagger/kfp_api_single_file.swagger.json

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,15 @@
117117
},
118118
{
119119
"name": "view",
120-
"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",
120+
"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.",
121121
"in": "query",
122122
"required": false,
123123
"type": "string",
124124
"enum": [
125125
"ARTIFACT_VIEW_UNSPECIFIED",
126126
"BASIC",
127-
"DOWNLOAD"
127+
"DOWNLOAD",
128+
"RENDER"
128129
],
129130
"default": "ARTIFACT_VIEW_UNSPECIFIED"
130131
}
@@ -1644,10 +1645,11 @@
16441645
"enum": [
16451646
"ARTIFACT_VIEW_UNSPECIFIED",
16461647
"BASIC",
1647-
"DOWNLOAD"
1648+
"DOWNLOAD",
1649+
"RENDER"
16481650
],
16491651
"default": "ARTIFACT_VIEW_UNSPECIFIED",
1650-
"title": "- ARTIFACT_VIEW_UNSPECIFIED: Not specified, equivalent to BASIC.\n - BASIC: Server responses excludes download_url\n - DOWNLOAD: Server responses include download_url"
1652+
"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."
16511653
},
16521654
"ListArtifactRequestField": {
16531655
"type": "string",
@@ -1766,6 +1768,10 @@
17661768
"error": {
17671769
"$ref": "#/definitions/googlerpcStatus",
17681770
"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."
1771+
},
1772+
"render_url": {
1773+
"type": "string",
1774+
"description": "Optional Output. Specifies a signed URL that can be used to\nrender this Artifact directly from its store."
17691775
}
17701776
}
17711777
},

‎backend/src/apiserver/resource/resource_manager.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@ import (
1818
"context"
1919
"encoding/json"
2020
"fmt"
21-
scheduledworkflow "github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow/v1beta1"
2221
"io"
2322
"net"
23+
"net/url"
2424
"reflect"
2525
"strconv"
2626
"time"
2727

28+
scheduledworkflow "github.com/kubeflow/pipelines/backend/src/crd/pkg/apis/scheduledworkflow/v1beta1"
29+
2830
"github.com/kubeflow/pipelines/backend/src/v2/metadata"
2931
"github.com/kubeflow/pipelines/backend/src/v2/objectstore"
3032
"github.com/kubeflow/pipelines/third_party/ml-metadata/go/ml_metadata"
@@ -2129,8 +2131,8 @@ func (r *ResourceManager) GetSecret(ctx context.Context, namespace, name string)
21292131
}
21302132

21312133
// GetSignedUrl retrieves a signed url for the associated artifact.
2132-
func (r *ResourceManager) GetSignedUrl(bucketConfig *objectstore.Config, secret *corev1.Secret, expirySeconds time.Duration, artifactURI string) (string, error) {
2133-
signedUrl, err := r.objectStore.GetSignedUrl(bucketConfig, secret, expirySeconds, artifactURI)
2134+
func (r *ResourceManager) GetSignedUrl(bucketConfig *objectstore.Config, secret *corev1.Secret, expirySeconds time.Duration, artifactURI string, queryParams url.Values) (string, error) {
2135+
signedUrl, err := r.objectStore.GetSignedUrl(bucketConfig, secret, expirySeconds, artifactURI, queryParams)
21342136
if err != nil {
21352137
return "", err
21362138
}

‎backend/src/apiserver/resource/resource_manager_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"encoding/json"
2020
"fmt"
21+
"net/url"
2122
"strings"
2223
"testing"
2324
"time"
@@ -83,7 +84,7 @@ func (m *FakeBadObjectStore) GetFromYamlFile(o interface{}, filePath string) err
8384
return util.NewInternalServerError(errors.New("Error"), "bad object store")
8485
}
8586

86-
func (m *FakeBadObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secret *corev1.Secret, expirySeconds time.Duration, artifactURI string) (string, error) {
87+
func (m *FakeBadObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secret *corev1.Secret, expirySeconds time.Duration, artifactURI string, queryParams url.Values) (string, error) {
8788
return "", util.NewInternalServerError(errors.New("Error"), "bad object store")
8889
}
8990

‎backend/src/apiserver/server/artifact_server.go

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package server
1717
import (
1818
"context"
1919
"fmt"
20+
"net/url"
2021
"strconv"
2122
"strings"
2223
"time"
@@ -141,7 +142,7 @@ func (s *ArtifactServer) ListArtifacts(ctx context.Context, r *apiv2beta1.ListAr
141142
if err1 != nil || bucketConfig == nil {
142143
return nil, util.NewInternalServerError(fmt.Errorf("failed to retrieve session info error: %v", err1), artifactId)
143144
}
144-
artifactResp, err1 := s.generateResponseArtifact(ctx, artifact, bucketConfig, namespace, false)
145+
artifactResp, err1 := s.generateResponseArtifact(ctx, artifact, bucketConfig, namespace, apiv2beta1.GetArtifactRequest_ARTIFACT_VIEW_UNSPECIFIED)
145146
if err1 != nil {
146147
return nil, util.NewInternalServerError(fmt.Errorf("encountered error parsing artifact: %v", err), artifactId)
147148
}
@@ -186,12 +187,7 @@ func (s *ArtifactServer) GetArtifact(ctx context.Context, r *apiv2beta1.GetArtif
186187
return nil, util.Wrap(err, "Failed to authorize the request")
187188
}
188189

189-
downloadURL := false
190-
if r.GetView() == apiv2beta1.GetArtifactRequest_DOWNLOAD {
191-
downloadURL = true
192-
}
193-
194-
artifactResp, err := s.generateResponseArtifact(ctx, artifact, sessionInfo, namespace, downloadURL)
190+
artifactResp, err := s.generateResponseArtifact(ctx, artifact, sessionInfo, namespace, r.GetView())
195191
if err != nil {
196192
return nil, util.NewInternalServerError(fmt.Errorf("encountered error parsing artifact: %v", err), r.ArtifactId)
197193
}
@@ -211,7 +207,7 @@ func (s *ArtifactServer) generateResponseArtifact(
211207
artifact *ml_metadata.Artifact,
212208
bucketConfig *objectstore.Config,
213209
namespace string,
214-
includeShareUrl bool,
210+
view apiv2beta1.GetArtifactRequest_ArtifactView,
215211
) (*apiv2beta1.Artifact, error) {
216212
params, err := objectstore.StructuredS3Params(bucketConfig.SessionInfo.Params)
217213
if err != nil {
@@ -242,13 +238,25 @@ func (s *ArtifactServer) generateResponseArtifact(
242238
LastUpdatedAt: timestamppb.New(time.UnixMilli(*artifact.LastUpdateTimeSinceEpoch)),
243239
}
244240

245-
if includeShareUrl {
246-
expiry := time.Second * time.Duration(common.GetSignedURLExpiryTimeSeconds())
247-
shareUrl, err := s.resourceManager.GetSignedUrl(bucketConfig, secret, expiry, *artifact.Uri)
241+
expiry := time.Second * time.Duration(common.GetSignedURLExpiryTimeSeconds())
242+
switch view {
243+
case apiv2beta1.GetArtifactRequest_DOWNLOAD:
244+
queryParams := make(url.Values)
245+
queryParams.Set("response-content-disposition", "attachment")
246+
shareUrl, err := s.resourceManager.GetSignedUrl(bucketConfig, secret, expiry, *artifact.Uri, queryParams)
248247
if err != nil {
249248
return nil, err
250249
}
251250
artifactResp.DownloadUrl = shareUrl
251+
252+
case apiv2beta1.GetArtifactRequest_RENDER:
253+
queryParams := make(url.Values)
254+
queryParams.Set("response-content-disposition", "inline")
255+
renderUrl, err := s.resourceManager.GetSignedUrl(bucketConfig, secret, expiry, *artifact.Uri, queryParams)
256+
if err != nil {
257+
return nil, err
258+
}
259+
artifactResp.RenderUrl = renderUrl
252260
}
253261

254262
return artifactResp, nil

‎backend/src/apiserver/server/artifact_server_test.go

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ package server
1616

1717
import (
1818
"context"
19-
apiv2beta1 "github.com/kubeflow/pipelines/backend/api/v2beta1/go_client"
20-
"github.com/stretchr/testify/require"
21-
"google.golang.org/protobuf/types/known/timestamppb"
2219
"strings"
2320
"testing"
2421
"time"
22+
23+
apiv2beta1 "github.com/kubeflow/pipelines/backend/api/v2beta1/go_client"
24+
"github.com/stretchr/testify/require"
25+
"google.golang.org/protobuf/types/known/timestamppb"
2526
)
2627

2728
func TestGetArtifacts(t *testing.T) {
@@ -76,6 +77,27 @@ func TestGetArtifacts(t *testing.T) {
7677
LastUpdatedAt: timestamppb.New(time.Unix(2, 0)),
7778
},
7879
},
80+
{
81+
name: "Fetch artifact 2",
82+
artifactRequest: &apiv2beta1.GetArtifactRequest{
83+
ArtifactId: "0",
84+
View: apiv2beta1.GetArtifactRequest_RENDER,
85+
},
86+
expectedArtifact: &apiv2beta1.Artifact{
87+
ArtifactId: "0",
88+
StorageProvider: "s3",
89+
StoragePath: "pipeline/some-pipeline-id/task/key0",
90+
Uri: "s3://test-bucket/pipeline/some-pipeline-id/task/key0",
91+
DownloadUrl: "",
92+
RenderUrl: "dummy-render-url", // defined in object_store_fake
93+
Namespace: "test-namespace",
94+
ArtifactType: "1",
95+
ArtifactSize: 123,
96+
97+
CreatedAt: timestamppb.New(time.Unix(1, 0)),
98+
LastUpdatedAt: timestamppb.New(time.Unix(1, 0)),
99+
},
100+
},
79101
{
80102
name: "Fetch artifact 1 - View unspecified",
81103
artifactRequest: &apiv2beta1.GetArtifactRequest{

‎backend/src/apiserver/storage/object_store.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ type ObjectStoreInterface interface {
4040
AddAsYamlFile(o interface{}, filePath string) error
4141
GetFromYamlFile(o interface{}, filePath string) error
4242
GetPipelineKey(pipelineId string) string
43-
GetSignedUrl(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string) (string, error)
43+
GetSignedUrl(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string, queryParams url.Values) (string, error)
4444
GetObjectSize(bucketConfig *objectstore.Config, secret *v1.Secret, artifactURI string) (int64, error)
4545
}
4646

@@ -132,7 +132,7 @@ func (m *MinioObjectStore) GetFromYamlFile(o interface{}, filePath string) error
132132
// store for this artifact. Signed URLs are built using the "GET" method, and are only intended for
133133
// Artifact downloads.
134134
// TODO: Add support for irsa and gcs app credentials pulled from environment
135-
func (m *MinioObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string) (string, error) {
135+
func (m *MinioObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secret *v1.Secret, expirySeconds time.Duration, artifactURI string, queryParams url.Values) (string, error) {
136136
s3Client, err := buildClientFromConfig(bucketConfig, secret)
137137
if err != nil {
138138
return "", err
@@ -142,8 +142,11 @@ func (m *MinioObjectStore) GetSignedUrl(bucketConfig *objectstore.Config, secret
142142
if err != nil {
143143
return "", err
144144
}
145-
reqParams := make(url.Values)
146-
signedUrl, err := s3Client.Presign("GET", bucketConfig.BucketName, key, expirySeconds, reqParams)
145+
if queryParams == nil {
146+
queryParams = make(url.Values)
147+
}
148+
149+
signedUrl, err := s3Client.Presign("GET", bucketConfig.BucketName, key, expirySeconds, queryParams)
147150
if err != nil {
148151
return "", util.Wrap(err, "Failed to generate signed url")
149152
}

‎backend/src/apiserver/storage/object_store_fake.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,11 @@
1515
package storage
1616

1717
import (
18-
"github.com/kubeflow/pipelines/backend/src/v2/objectstore"
19-
"k8s.io/api/core/v1"
18+
"net/url"
2019
"time"
20+
21+
"github.com/kubeflow/pipelines/backend/src/v2/objectstore"
22+
v1 "k8s.io/api/core/v1"
2123
)
2224

2325
type fakeMinioObjectStore struct {
@@ -48,7 +50,13 @@ func (m *fakeMinioObjectStore) GetFromYamlFile(o interface{}, filePath string) e
4850
return m.minioObjectStore.GetFromYamlFile(o, filePath)
4951
}
5052

51-
func (m *fakeMinioObjectStore) GetSignedUrl(*objectstore.Config, *v1.Secret, time.Duration, string) (string, error) {
53+
func (m *fakeMinioObjectStore) GetSignedUrl(
54+
bucketConfig *objectstore.Config, secret *v1.Secret, expiry time.Duration,
55+
uri string, queryParams url.Values) (string, error) {
56+
57+
if queryParams.Get("response-content-disposition") == "inline" {
58+
return "dummy-render-url", nil
59+
}
5260
return "dummy-signed-url", nil
5361
}
5462

0 commit comments

Comments
 (0)