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.

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

Migrate from s3 sdk v2 to v3 #6731

Merged
merged 10 commits into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@
"> 0.25%, not dead"
],
"dependencies": {
"@aws-sdk/client-s3": "3.535.0",
"@aws-sdk/lib-storage": "3.535.0",
"@aws-sdk/s3-presigned-post": "3.535.0",
Copy link
Contributor Author

@lng2020 lng2020 Mar 29, 2024

Choose a reason for hiding this comment

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

My local env can work without importing @aws-sdk/s3-presigned-post as expected. But CI cannot find this module so I imported it here. Don't know the root cause and I found a similar issue with aws-sdk/middleware-endpoint before(aws/aws-sdk-js-v3#3983).

"@aws-sdk/s3-request-presigner": "3.535.0",
"@aws-sdk/signature-v4-crt": "^3.535.0",
"@babel/core": "^7.23.7",
"@babel/plugin-proposal-decorators": "^7.23.2",
"@babel/plugin-transform-destructuring": "^7.23.3",
Expand Down Expand Up @@ -81,7 +86,6 @@
"@vitejs/plugin-react": "^3.1.0",
"addressparser": "^1.0.1",
"autotrack": "^2.4.1",
"aws-sdk": "^2.1550.0",
"babel-plugin-styled-components": "^2.1.4",
"babel-plugin-transform-class-properties": "^6.24.1",
"body-scroll-lock": "^4.0.0-beta.0",
Expand Down
4 changes: 0 additions & 4 deletions server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import env from "./env";

import "./logging/tracer"; // must come before importing any instrumented module

import maintenance from "aws-sdk/lib/maintenance_mode_message";
import http from "http";
import https from "https";
import Koa from "koa";
Expand All @@ -28,9 +27,6 @@ import RedisAdapter from "./storage/redis";
import Metrics from "./logging/Metrics";
import { PluginManager } from "./utils/PluginManager";

// Suppress the AWS maintenance message until upgrade to v3.
maintenance.suppress = true;

// The number of processes to run, defaults to the number of CPU's available
// for the web service, and 1 for collaboration during the beta period.
let webProcessCount = env.WEB_CONCURRENCY;
Expand Down
2 changes: 1 addition & 1 deletion server/storage/files/BaseStorage.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Blob } from "buffer";
import { Readable } from "stream";
import { PresignedPost } from "aws-sdk/clients/s3";
import { PresignedPost } from "@aws-sdk/s3-presigned-post";
import { isBase64Url } from "@shared/utils/urls";
import env from "@server/env";
import Logger from "@server/logging/Logger";
Expand Down
105 changes: 61 additions & 44 deletions server/storage/files/S3Storage.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
import path from "path";
import util from "util";
import AWS, { S3 } from "aws-sdk";
import { Readable } from "stream";
import {
S3Client,
DeleteObjectCommand,
GetObjectCommand,
ObjectCannedACL,
} from "@aws-sdk/client-s3";
import { Upload } from "@aws-sdk/lib-storage";
import "@aws-sdk/signature-v4-crt"; // https://github.com/aws/aws-sdk-js-v3#functionality-requiring-aws-common-runtime-crt
import {
PresignedPostOptions,
createPresignedPost,
} from "@aws-sdk/s3-presigned-post";
import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
import fs from "fs-extra";
import invariant from "invariant";
import compact from "lodash/compact";
Expand All @@ -13,14 +25,14 @@ export default class S3Storage extends BaseStorage {
constructor() {
super();

this.client = new AWS.S3({
s3BucketEndpoint: env.AWS_S3_ACCELERATE_URL ? true : undefined,
s3ForcePathStyle: env.AWS_S3_FORCE_PATH_STYLE,
accessKeyId: env.AWS_ACCESS_KEY_ID,
secretAccessKey: env.AWS_SECRET_ACCESS_KEY,
this.client = new S3Client({
forcePathStyle: env.AWS_S3_FORCE_PATH_STYLE,
credentials: {
accessKeyId: env.AWS_ACCESS_KEY_ID || "",
secretAccessKey: env.AWS_SECRET_ACCESS_KEY || "",
},
region: env.AWS_REGION,
endpoint: this.getEndpoint(),
signatureVersion: "v4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition is deprecated and now is replaced by import "@aws-sdk/signature-v4-crt"; according to https://github.com/aws/aws-sdk-js-v3#functionality-requiring-aws-common-runtime-crt

});
}

Expand All @@ -30,8 +42,9 @@ export default class S3Storage extends BaseStorage {
maxUploadSize: number,
contentType = "image"
) {
const params = {
Bucket: env.AWS_S3_UPLOAD_BUCKET_NAME,
const params: PresignedPostOptions = {
Bucket: env.AWS_S3_UPLOAD_BUCKET_NAME as string,
Key: key,
Conditions: compact([
["content-length-range", 0, maxUploadSize],
["starts-with", "$Content-Type", contentType],
Expand All @@ -45,9 +58,7 @@ export default class S3Storage extends BaseStorage {
Expires: 3600,
};

return util.promisify(this.client.createPresignedPost).bind(this.client)(
params
);
return createPresignedPost(this.client, params);
}

private getPublicEndpoint(isServerUpload?: boolean) {
Expand Down Expand Up @@ -96,7 +107,7 @@ export default class S3Storage extends BaseStorage {
key,
acl,
}: {
body: S3.Body;
body: Buffer | Uint8Array | string | Readable;
contentLength?: number;
contentType?: string;
key: string;
Expand All @@ -107,17 +118,20 @@ export default class S3Storage extends BaseStorage {
"AWS_S3_UPLOAD_BUCKET_NAME is required"
);

await this.client
.putObject({
ACL: acl,
const upload = new Upload({
client: this.client,
params: {
ACL: acl as ObjectCannedACL,
Bucket: env.AWS_S3_UPLOAD_BUCKET_NAME,
Key: key,
ContentType: contentType,
ContentLength: contentLength,
ContentDisposition: "attachment",
Body: body,
})
.promise();
},
});
await upload.done();

const endpoint = this.getPublicEndpoint(true);
return `${endpoint}/${key}`;
};
Expand All @@ -128,12 +142,12 @@ export default class S3Storage extends BaseStorage {
"AWS_S3_UPLOAD_BUCKET_NAME is required"
);

await this.client
.deleteObject({
await this.client.send(
new DeleteObjectCommand({
Bucket: env.AWS_S3_UPLOAD_BUCKET_NAME,
Key: key,
})
.promise();
);
}

public getSignedUrl = async (
Expand All @@ -148,18 +162,21 @@ export default class S3Storage extends BaseStorage {
ResponseContentDisposition: "attachment",
};

const url = isDocker
? `${this.getPublicEndpoint()}/${key}`
: await this.client.getSignedUrlPromise("getObject", params);
if (isDocker) {
return `${this.getPublicEndpoint()}/${key}`;
} else {
const command = new GetObjectCommand(params);
const url = await getSignedUrl(this.client, command);

if (env.AWS_S3_ACCELERATE_URL) {
return url.replace(
env.AWS_S3_UPLOAD_BUCKET_URL,
env.AWS_S3_ACCELERATE_URL
);
}
if (env.AWS_S3_ACCELERATE_URL) {
return url.replace(
env.AWS_S3_UPLOAD_BUCKET_URL,
env.AWS_S3_ACCELERATE_URL
);
}

return url;
return url;
}
};

public getFileHandle(key: string): Promise<{
Expand Down Expand Up @@ -193,29 +210,29 @@ export default class S3Storage extends BaseStorage {
});
}

public getFileStream(key: string) {
public getFileStream(key: string): NodeJS.ReadableStream | null {
invariant(
env.AWS_S3_UPLOAD_BUCKET_NAME,
"AWS_S3_UPLOAD_BUCKET_NAME is required"
);

try {
return this.client
.getObject({
this.client
Copy link
Member

Choose a reason for hiding this comment

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

The response is no longer returned, can we restore this to async/await style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in d520b03 (#6731) . I'm not so familiar with all promise, await, and async things. Please feel free to correct me if I'm doing wrong.

.send(
new GetObjectCommand({
Bucket: env.AWS_S3_UPLOAD_BUCKET_NAME,
Key: key,
})
.createReadStream();
} catch (err) {
Logger.error("Error getting file stream from S3 ", err, {
key,
)
.then((item) => item.Body as NodeJS.ReadableStream)
.catch((err) => {
Logger.error("Error getting file stream from S3 ", err, {
key,
});
});
}

return null;
}

private client: AWS.S3;
private client: S3Client;

private getEndpoint() {
if (env.AWS_S3_ACCELERATE_URL) {
Expand All @@ -231,6 +248,6 @@ export default class S3Storage extends BaseStorage {
}
}

return new AWS.Endpoint(env.AWS_S3_UPLOAD_BUCKET_URL);
return env.AWS_S3_UPLOAD_BUCKET_URL;
}
}
34 changes: 22 additions & 12 deletions server/test/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,28 @@ jest.mock("bull");
jest.mock("../queues");

// We never want to make real S3 requests in test environment
jest.mock("aws-sdk", () => {
const mS3 = {
createPresignedPost: jest.fn(),
putObject: jest.fn().mockReturnThis(),
deleteObject: jest.fn().mockReturnThis(),
promise: jest.fn(),
};
return {
S3: jest.fn(() => mS3),
Endpoint: jest.fn(),
};
});
jest.mock("@aws-sdk/client-s3", () => ({
S3Client: jest.fn(() => ({
send: jest.fn(),
})),
DeleteObjectCommand: jest.fn(),
GetObjectCommand: jest.fn(),
ObjectCannedACL: {},
}));

jest.mock("@aws-sdk/lib-storage", () => ({
Upload: jest.fn(() => ({
done: jest.fn(),
})),
}));

jest.mock("@aws-sdk/s3-presigned-post", () => ({
createPresignedPost: jest.fn(),
}));

jest.mock("@aws-sdk/s3-request-presigner", () => ({
getSignedUrl: jest.fn(),
}));

afterAll(() => Redis.defaultClient.disconnect());

Expand Down
7 changes: 0 additions & 7 deletions server/typings/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,3 @@ declare module "@joplin/turndown-plugin-gfm" {
export const taskListItems: Plugin;
export const gfm: Plugin;
}

declare module "aws-sdk/lib/maintenance_mode_message" {
const maintenance: {
suppress: boolean;
};
export default maintenance;
}