feat(frontend): Implement aws-js-sdk crendentials to support IRSA for s3 #8651

ryansteakley · 2023-01-04T21:08:57Z

Description of your changes:

#8502 details most of the changes and reasoning in a design document.

Removing most of the aws-helper code that pertains to grabbing the credentials from the ec2 instance metadata as this code is already handled as part of the credentialProviderChain that is being imported here.

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.

ryansteakley · 2023-01-04T21:09:32Z

@james-jwu @chensun @zijianjoy

ryansteakley · 2023-01-05T00:46:53Z

Looks like the prow job is running with node 12 installed, when it should move to using node 14? The front-end unit tests pass locally. https://github.com/GoogleCloudPlatform/oss-test-infra/blob/b3128a944261abbbc3e23b7db974f95ea6c93192/prow/prowjobs/kubeflow/pipelines/kubeflow-pipelines-presubmits.yaml#L9 and I've opened a pr GoogleCloudPlatform/oss-test-infra#1864

zijianjoy · 2023-01-12T00:46:10Z

/test kubeflow-pipeline-frontend-test

ryansteakley · 2023-01-12T00:55:12Z

Good to see that the node14 update lets the tests run. Let me fix this unit-test looks like this is expected now since the prow has no credentials and it has exhausted all options. Missed this locally since my local env must have had some lingering creds somewhere.

ryansteakley · 2023-01-12T01:14:41Z

/retest

zijianjoy · 2023-01-13T07:27:31Z

frontend/server/minio-helper.ts

@@ -39,22 +39,21 @@ export interface MinioClientOptionsWithOptionalSecrets extends Partial<MinioClie
 export async function createMinioClient(config: MinioClientOptionsWithOptionalSecrets) {
  if (!config.accessKey || !config.secretKey) {
    try {
-      if (await awsInstanceProfileCredentials.ok()) {
-        const credentials = await awsInstanceProfileCredentials.getCredentials();


In the design doc, you mention that we will fail back to existing solution if the IRSA doesn't work. It doesn't look like the case in this block. Can we retain the existing solution?

zijianjoy · 2023-01-13T07:30:36Z

frontend/server/minio-helper.ts

-          return new MinioClient({ ...config, accessKey, secretKey, sessionToken });
-        }
-        console.error('unable to get credentials from AWS metadata store.');
+      const credentials = fromNodeProviderChain();


Is it possible to detect the source of object storage is AWS first, and perform IRSA only when it is AWS S3? (Pattern like "arn:aws:s3:::$mybucket/*") If it is generic S3 API, then we shouldn't use IRSA solution.

@zijianjoy The code that used to be in this block was checking for AWS metadata store credentials. The fromNodeProviderChain() will try to access IRSA credentials then fallback and one of those would be AWS metadata store credentials which retains the exsiting functionality.

If the option is preferred, then what I can do is call the existing

pipelines/frontend/server/aws-helper.ts

Line 54 in 834d966

export function isS3Endpoint(endpoint: string = ''): boolean {

and have this IRSA code live as an if statement above the rest of the code in this method. i.e (if isS3Endpoint) ->my new code

Hello Ryan, thank you for replying to my comment, Sounds good on using isS3Endpoint , I would also suggest adding a comment to explain that which logic area is specific to AWS, and which logic is generic S3.

@zijianjoy What is an example of generic S3 which has s3:// in the source/URL?
https://github.com/kubeflow/pipelines/blob/master/frontend/server/handlers/artifacts.ts#L33

~~Want to make sure we do not introduce a breaking change, ref #8651 (comment)~~

surajkota · 2023-01-18T21:45:01Z

frontend/server/minio-helper.ts

@@ -37,6 +38,25 @@ export interface MinioClientOptionsWithOptionalSecrets extends Partial<MinioClie
 * @param config minio client options where `accessKey` and `secretKey` are optional.
 */
 export async function createMinioClient(config: MinioClientOptionsWithOptionalSecrets) {
+  // This logic is AWS S3 specific
+  if (isS3Endpoint(config.endPoint)) {


I think this would be a breaking change. Please correct me if my understanding is not correct

as of now no endpoint is set when running on an AWS by default. Afaik, it hits the s3 case based of the URL of the object i.e. source being s3 s3://

When createMinioClient is called in the s3 case it passes in the aws artifactsConfig which contains endpoint, that resolves to the default value of s3.amazonaws.com or the env variable of AWS_S3_ENDPOINT. I'm not sure what you mean by breaking change?

pipelines/frontend/server/configs.ts

Line 128 in 88a9a31

endPoint: AWS_S3_ENDPOINT || 's3.amazonaws.com',

Got it, thanks

frontend/server/aws-helper.test.ts

frontend/server/minio-helper.test.ts

ryansteakley · 2023-01-18T22:08:59Z

/test kubeflow-pipeline-frontend-test

surajkota

Good work

surajkota · 2023-01-18T23:45:57Z

frontend/server/minio-helper.ts

+    }
+  }
+
+  // This logic is S3 generic
  if (!config.accessKey || !config.secretKey) {
    try {
      if (await awsInstanceProfileCredentials.ok()) {


This code is outdated and should be removed since there is an updated implementation for AWS S3. New implementation supports multiple ways to provide credentials including better security practices.

Better to discuss it in a separate PR

ryansteakley · 2023-01-19T02:16:12Z

@zijianjoy Can you take another look?

surajkota · 2023-01-24T21:34:35Z

friendly reminder @zijianjoy since next release might be planned soon. We want to test this as part of distribution testing. Thanks

jlyaoyuli · 2023-01-25T17:23:09Z

Hi @surajkota @ryansteakley ,
This is Joe from KFP frontend, I will review the PR as soon as possible. Thank you!

jlyaoyuli · 2023-01-25T18:38:56Z

/lgtm
/approve
Thank you @ryansteakley and @surajkota for this implementation.

google-oss-prow · 2023-01-25T18:39:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlyaoyuli, mvoitko, surajkota

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:

~~frontend/OWNERS~~ [jlyaoyuli]

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

… s3 (#8651) * use aws js sdk for credentials concerning aws * format code * catch exception if no credentials are found after using credentialchain * revert changes and add section for specific aws changes * revert deleting aws-helper mock * add privatelink test for isS3endpiont * fix try block

use aws js sdk for credentials concerning aws

8bf1d35

google-oss-prow bot added the size/XXL label Jan 4, 2023

google-oss-prow bot requested review from jlyaoyuli and zijianjoy January 4, 2023 21:09

ryansteakley added 2 commits January 4, 2023 13:13

format code

26a9061

Merge remote-tracking branch 'upstream/master' into s3-irsa

0d0fc91

Linchin removed request for zijianjoy and jlyaoyuli January 5, 2023 23:38

Linchin assigned jlyaoyuli Jan 5, 2023

Linchin requested a review from zijianjoy January 5, 2023 23:38

mvoitko approved these changes Jan 10, 2023

View reviewed changes

catch exception if no credentials are found after using credentialchain

7af6725

zijianjoy reviewed Jan 13, 2023

View reviewed changes

revert changes and add section for specific aws changes

165fdb6

surajkota reviewed Jan 18, 2023

View reviewed changes

ryansteakley added 2 commits January 18, 2023 13:51

revert deleting aws-helper mock

c19752d

add privatelink test for isS3endpiont

37137f3

fix try block

06b530c

surajkota approved these changes Jan 18, 2023

View reviewed changes

google-oss-prow bot added the lgtm label Jan 25, 2023

google-oss-prow bot added the approved label Jan 25, 2023

google-oss-prow bot merged commit 9b9b3ac into kubeflow:master Jan 25, 2023

surajkota mentioned this pull request Mar 7, 2023

Proposal to release 2.0-alpha7 with bug fixes and awaited features for Kubeflow 1.7 release #8938

Closed

ryansteakley mentioned this pull request Mar 10, 2023

[feature] S3 Integration for Artifact Storage (IRSA) #8502

Closed

ryansteakley mentioned this pull request May 25, 2023

feat(backend): Implement aws-go-sdk crendentials to support IRSA for s3 in V2 #9502

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): Implement aws-js-sdk crendentials to support IRSA for s3 #8651

feat(frontend): Implement aws-js-sdk crendentials to support IRSA for s3 #8651

ryansteakley commented Jan 4, 2023

ryansteakley commented Jan 4, 2023

ryansteakley commented Jan 5, 2023 •

edited

Loading

zijianjoy commented Jan 12, 2023

ryansteakley commented Jan 12, 2023 •

edited

Loading

ryansteakley commented Jan 12, 2023

zijianjoy Jan 13, 2023

zijianjoy Jan 13, 2023

ryansteakley Jan 13, 2023

zijianjoy Jan 17, 2023

surajkota Jan 18, 2023 •

edited

Loading

surajkota Jan 18, 2023 •

edited

Loading

ryansteakley Jan 18, 2023

surajkota Jan 18, 2023

ryansteakley commented Jan 18, 2023

surajkota left a comment

surajkota Jan 18, 2023

ryansteakley commented Jan 19, 2023

surajkota commented Jan 24, 2023

jlyaoyuli commented Jan 25, 2023

jlyaoyuli commented Jan 25, 2023

google-oss-prow bot commented Jan 25, 2023

feat(frontend): Implement aws-js-sdk crendentials to support IRSA for s3 #8651

feat(frontend): Implement aws-js-sdk crendentials to support IRSA for s3 #8651

Conversation

ryansteakley commented Jan 4, 2023

ryansteakley commented Jan 4, 2023

ryansteakley commented Jan 5, 2023 • edited Loading

zijianjoy commented Jan 12, 2023

ryansteakley commented Jan 12, 2023 • edited Loading

ryansteakley commented Jan 12, 2023

zijianjoy Jan 13, 2023

Choose a reason for hiding this comment

zijianjoy Jan 13, 2023

Choose a reason for hiding this comment

ryansteakley Jan 13, 2023

Choose a reason for hiding this comment

zijianjoy Jan 17, 2023

Choose a reason for hiding this comment

surajkota Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

surajkota Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

ryansteakley Jan 18, 2023

Choose a reason for hiding this comment

surajkota Jan 18, 2023

Choose a reason for hiding this comment

ryansteakley commented Jan 18, 2023

surajkota left a comment

Choose a reason for hiding this comment

surajkota Jan 18, 2023

Choose a reason for hiding this comment

ryansteakley commented Jan 19, 2023

surajkota commented Jan 24, 2023

jlyaoyuli commented Jan 25, 2023

jlyaoyuli commented Jan 25, 2023

google-oss-prow bot commented Jan 25, 2023

ryansteakley commented Jan 5, 2023 •

edited

Loading

ryansteakley commented Jan 12, 2023 •

edited

Loading

surajkota Jan 18, 2023 •

edited

Loading

surajkota Jan 18, 2023 •

edited

Loading