feat: Add support for S3

[anteeek]

About Me

superfly.tv

Type of Contribution

This is a:

Feature

Current Behavior

no S3 support

New Behavior

Added AWS S3 support

Testing Instructions

Add working S3 key credentials to expectedPackages.json, then everything

Other Information

Status

• PR is ready to be reviewed.
• The functionality has been tested by the author.
• Relevant unit tests has been added / updated.
• Relevant documentation (code comments, system documentation) has been added / updated.

[nytamin]

Looks good!

I added a few comments regarding docs and style that would be good to address before merging.

[nytamin]

add docs explaining what does undefined imply

[nytamin]

Do you know any solutions that require this? if so, maybe name them in the doc?

[nytamin]

Minor note: To me, passing the S3Options object would be cleaner

Suggested change

	constructor(
	private readonly bucketId: string,
	private readonly region: string,
	private readonly accessKey: string,
	private readonly secretAccessKey: string,
	private readonly endpoint?: string,
	private readonly forcePathStyle: boolean = false
	) {
	constructor(
	private readonly options: S3Options
	) {

[nytamin]

Please use the function stringifyerror(error) to stringify errors (it ensures that stack traces etc are included)

(this also applies to other places in the code)

[nytamin]

readability: can fullPath.replace(/^\//, '') be DRY'ed up here?

[nytamin]

I just realized that useTemporaryFilePath might not be needed for S3 storage.
It is used when writing to files on disk to ensure atomic operations, but since S3 operations are atomic by definition, I think we can omit any uses of useTemporaryFilePath completely.

[nytamin]

This can be simplified using worker.cacheData().

Suggested change

	private async getS3Client(): Promise<S3BucketClient> {
	let client = this.getS3ClientFromCache(
	this.s3Options.region,
	this.s3Options.accessKey,
	this.s3Options.secretAccessKey,
	this.s3Options.s3PublicBaseUrl,
	this.s3Options.endpoint,
	this.s3Options.forcePathStyle
	)
	if (!client) {
	client = this.createNewS3Client()
	this.cacheS3Client(
	client,
	this.s3Options.region,
	this.s3Options.accessKey,
	this.s3Options.secretAccessKey,
	this.s3Options.s3PublicBaseUrl,
	this.s3Options.endpoint,
	this.s3Options.forcePathStyle
	)
	}
	return client
	}
	private getS3ClientFromCache(
	region: string,
	accessKey: string,
	secretAccessKey: string,
	publicBaseUrl: string,
	endpoint?: string,
	forcePathStyle?: boolean
	): S3BucketClient | undefined {
	const accessorCache = this.worker.accessorCache as {
	[accessorType: string]: S3BucketClient | undefined
	}
	return accessorCache[
	S3AccessorHandle.getCacheKeyForS3Client(
	accessKey,
	secretAccessKey,
	region,
	publicBaseUrl,
	endpoint,
	forcePathStyle
	)
	]
	}
	private cacheS3Client(
	client: S3BucketClient,
	region: string,
	accessKey: string,
	secretAccessKey: string,
	publicBaseUrl: string,
	endpoint?: string,
	forcePathStyle?: boolean
	): void {
	const accessorCache = this.worker.accessorCache as {
	[accessorType: string]: S3BucketClient | undefined
	}
	accessorCache[
	S3AccessorHandle.getCacheKeyForS3Client(
	accessKey,
	secretAccessKey,
	region,
	publicBaseUrl,
	endpoint,
	forcePathStyle
	)
	] = client
	}
	static getCacheKeyForS3Client(
	accessKey: string,
	secretAccessKey: string,
	region: string,
	publicBaseUrl: string,
	endpoint?: string,
	forcePathStyle?: boolean
	): string {
	return JSON.stringify([accessKey, secretAccessKey, region, publicBaseUrl, endpoint, forcePathStyle])
	}
	private createNewS3Client(): S3BucketClient {
	return new S3BucketClient(
	this.s3Options.bucketId,
	this.s3Options.region,
	this.s3Options.accessKey,
	this.s3Options.secretAccessKey,
	this.s3Options.endpoint,
	this.s3Options.forcePathStyle
	)
	}
	private async getS3Client(): Promise<S3BucketClient> {
	const cacheKey = JSON.stringify(this.s3Options)
	return this.worker.cacheData(
	this.type,
	`s3Client${cacheKey}`,
	() => {
	return this.createNewS3Client()
	},
	10 * 60 * 1000
	) // 10 min cache time
	}
	private createNewS3Client(): S3BucketClient {
	return new S3BucketClient(this.s3Options)
	}

[sonarqubecloud]

Quality Gate failed

Failed conditions
11.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[Julusian]

As a side comment, I was hoping that it would be possible to do this without propagating the credentials through to each worker (to minimise the spread and risk of leaking them). At least any keys which can write to the bucket, instead having the workforce or something provide presigned urls for uploading.
I may well have overlooked something which means that isnt viable though, and this could be something added later by someone who is worried enough about security to do it.

Is there an opensource s3 server we should recommend for use during development? (either as a docker image, or cross platform binary)

[anteeek]

@Julusian after investigation - it is of course possible to pass it through presigned URLS and it's feasible for this use case, but architecturally it seems to be a big intervention in terms of data flow direction
as for S3 server locally used for testing & development it was minio in a docker container, should I add it to README or somewhere?

[Julusian]

as for S3 server locally used for testing & development it was minio in a docker container, should I add it to README or somewhere?

hmm Im hesistant to put minio as a recommendation as it is a little dead these days https://github.com/minio/minio#maintenance-mode. That said for my personal usage I havent figured out what to move to, so havent migrated away.