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 5 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
4 changes: 4 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,10 @@ 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 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
1 change: 0 additions & 1 deletion frontend/server/minio-helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { createMinioClient, isTarball, maybeTarball, getObjectStream } from './m
import { V1beta1JobTemplateSpec } from '@kubernetes/client-node';

jest.mock('minio');
jest.mock('./aws-helper');
ryansteakley marked this conversation as resolved.
Show resolved Hide resolved

describe('minio-helper', () => {
const MockedMinioClient: jest.Mock = MinioClient as any;
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

const credentials = fromNodeProviderChain();
const aws_credentials = await credentials();
try {
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