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

Jump to bottom

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

Merged
merged 8 commits into from
Jan 25, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8bf1d35
use aws js sdk for credentials concerning aws
ryansteakley Jan 4, 2023
26a9061
format code
ryansteakley Jan 4, 2023
0d0fc91
Merge remote-tracking branch 'upstream/master' into s3-irsa
ryansteakley Jan 4, 2023
7af6725
catch exception if no credentials are found after using credentialchain
ryansteakley Jan 12, 2023
165fdb6
revert changes and add section for specific aws changes
ryansteakley Jan 18, 2023
c19752d
revert deleting aws-helper mock
ryansteakley Jan 18, 2023
37137f3
add privatelink test for isS3endpiont
ryansteakley Jan 18, 2023
06b530c
fix try block
ryansteakley Jan 18, 2023
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
8 changes: 8 additions & 0 deletions frontend/server/aws-helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ describe('isS3Endpoint', () => {
expect(isS3Endpoint('s3.cn-north-1.amazonaws.com.cn')).toBe(true);
});

it('checks a valid s3 fips GovCloud endpoint', () => {
ryansteakley marked this conversation as resolved.
Show resolved Hide resolved
expect(isS3Endpoint('s3-fips.us-gov-west-1.amazonaws.com')).toBe(true);
});

it('checks a valid s3 PrivateLink endpoint', () => {
expect(isS3Endpoint('vpce-1a2b3c4d-5e6f.s3.us-east-1.vpce.amazonaws.com')).toBe(true);
});

it('checks an invalid s3 endpoint', () => {
expect(isS3Endpoint('amazonaws.com')).toBe(false);
});
Expand Down
1 change: 0 additions & 1 deletion frontend/server/handlers/artifacts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ export function getArtifactsHandler({
peek,
)(req, res);
break;

case 's3':
getMinioArtifactHandler(
{
Expand Down
22 changes: 21 additions & 1 deletion frontend/server/minio-helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import * as tar from 'tar-stream';
import peek from 'peek-stream';
import gunzip from 'gunzip-maybe';
import { Client as MinioClient, ClientOptions as MinioClientOptions } from 'minio';
import { awsInstanceProfileCredentials } from './aws-helper';
import { awsInstanceProfileCredentials, isS3Endpoint } from './aws-helper';
const { fromNodeProviderChain } = require('@aws-sdk/credential-providers');

/** MinioRequestConfig describes the info required to retrieve an artifact. */
export interface MinioRequestConfig {
Expand All @@ -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)) {
Copy link
Contributor

@surajkota surajkota Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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://

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

pipelines/frontend/server/configs.ts

Line 128 in 88a9a31

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

Got it, thanks

try {
const credentials = fromNodeProviderChain();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@surajkota surajkota Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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)

const aws_credentials = await credentials();
if (aws_credentials) {
const {
accessKeyId: accessKey,
secretAccessKey: secretKey,
sessionToken: sessionToken,
} = aws_credentials;
return new MinioClient({ ...config, accessKey, secretKey, sessionToken });
}
} catch (err) {
console.error('Unable to get credentials from AWS credential provider chain: ', err);
}
}

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

Choose a reason for hiding this comment

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

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

Expand Down
Loading