refactor createS3Client to separate types and make testable (#967)

This PR makes the following changes:
- Refactor `createS3Client` into testable functions.
- Remove `requestTimeout` and `connectionTimeout` handlerOptions logic,
as it has never been used and made things overly complicated.
- Add tests for all created functions.

ref #960

asset/package.json

@@ -1,7 +1,7 @@
 {
     "name": "file",
     "displayName": "Asset",
-    "version": "2.9.1",
+    "version": "2.9.2",
     "private": true,
     "description": "A set of processors for working with files",
     "repository": {
@@ -20,8 +20,8 @@
         "test": "yarn --cwd ../ test"
     },
     "dependencies": {
-        "@terascope/file-asset-apis": "^0.12.1",
-        "@terascope/job-components": "^0.70.0",
+        "@terascope/file-asset-apis": "^0.12.2",
+        "@terascope/job-components": "^0.71.0",
         "csvtojson": "^2.0.10",
         "fs-extra": "^11.2.0",
         "json2csv": "5.0.7",

package.json

@@ -1,7 +1,7 @@
 {
     "name": "file-assets-bundle",
     "displayName": "File Assets Bundle",
-    "version": "2.9.1",
+    "version": "2.9.2",
     "private": true,
     "description": "A set of processors for working with files",
     "repository": "https://github.com/terascope/file-assets.git",
@@ -30,9 +30,9 @@
     "dependencies": {},
     "devDependencies": {
         "@terascope/eslint-config": "^0.8.0",
-        "@terascope/file-asset-apis": "^0.12.1",
-        "@terascope/job-components": "^0.70.0",
-        "@terascope/scripts": "^0.74.0",
+        "@terascope/file-asset-apis": "^0.12.2",
+        "@terascope/job-components": "^0.71.0",
+        "@terascope/scripts": "^0.75.1",
         "@types/fs-extra": "^11.0.2",
         "@types/jest": "^29.5.12",
         "@types/json2csv": "^5.0.7",

packages/file-asset-apis/package.json

@@ -1,7 +1,7 @@
 {
     "name": "@terascope/file-asset-apis",
     "displayName": "File Asset Apis",
-    "version": "0.12.1",
+    "version": "0.12.2",
     "description": "file reader and sender apis",
     "homepage": "https://github.com/terascope/file-assets",
     "repository": "[email protected]:terascope/file-assets.git",
@@ -30,7 +30,7 @@
         "node-gzip": "^1.1.2"
     },
     "devDependencies": {
-        "@terascope/scripts": "^0.74.0",
+        "@terascope/scripts": "^0.75.1",
         "@types/jest": "^29.5.12",
         "aws-sdk-client-mock": "^4.0.0",
         "jest": "^29.7.0",

packages/file-asset-apis/src/s3/createS3Client.ts

@@ -2,19 +2,23 @@ import fs from 'fs-extra';
 import { Agent, AgentOptions as HttpsAgentOptions } from 'https';
 import { S3Client as BaseClient } from '@aws-sdk/client-s3';
 import { NodeHttpHandler, NodeHttpHandlerOptions } from '@smithy/node-http-handler';
-import type { S3ClientConfig as baseConfig } from '@aws-sdk/client-s3';
+import type { S3ClientConfig as BaseConfig } from '@aws-sdk/client-s3';
 import {
-    debugLogger, has, isEmpty, isNumber
+    debugLogger
 } from '@terascope/utils';
 import type { S3Client } from './client-types';
 
-export interface S3ClientConfig extends baseConfig {
+export interface S3ClientConfig extends BaseConfig {
     sslEnabled?: boolean,
     certLocation?: string,
-    httpOptions?: HttpsAgentOptions,
-    handlerOptions?: Pick<NodeHttpHandlerOptions, 'requestTimeout'|'connectionTimeout'>,
     secretAccessKey?: string,
-    accessKeyId?: string
+    accessKeyId?: string,
+    maxRetries?: number
+}
+
+export interface S3ClientCredentials {
+    accessKeyId: string;
+    secretAccessKey: string;
 }
 
 export async function createS3Client(
@@ -23,63 +27,108 @@ export async function createS3Client(
 ): Promise<S3Client> {
     logger.info(`Using S3 endpoint: ${config.endpoint}`);
 
+    const finalConfig = await genFinalS3ClientConfig(config);
+
     // The aws v3 client logs every request and its metadata, it is too intrusive
     // so should only be used in trace mode otherwise it will log the body request
     // which has heavy performance implications
     if (logger.level() === 10) {
-        config.logger = logger;
+        finalConfig.logger = logger;
+    }
+    logger.debug(`createS3Client finalConfig:\n${JSON.stringify(finalConfig, null, 2)}`);
+
+    return new BaseClient(finalConfig);
+}
+
+/**
+ * Given an S3ClientConfig object, modify to make compatible with S3 Client V3
+ * @param {S3ClientConfig} config Starting S3 client configuration object
+ * @returns {S3ClientConfig} Final S3 client configuration object
+ */
+export async function genFinalS3ClientConfig(config: S3ClientConfig): Promise<BaseConfig> {
+    if (config.maxRetries) {
+        config.maxAttempts = config.maxRetries;
     }
 
-    let httpOptions = Object.assign(
-        config.httpOptions || {}
-    );
+    if (config.sslEnabled) {
+        if (typeof config.endpoint === 'string' && !config.endpoint.includes('https')) {
+            throw new Error(`S3 endpoint ${config.endpoint} must be https if sslEnabled`);
+        }
+        const httpOptions = await createHttpOptions(config);
+        const requestHandlerOptions = createRequestHandlerOptions(httpOptions);
+        config.requestHandler = new NodeHttpHandler(requestHandlerOptions);
+    }
 
+    if (!config.sslEnabled && typeof config.endpoint === 'string' && config.endpoint?.includes('https')) {
+        throw new Error(`S3 endpoint ${config.endpoint} cannot be https if sslEnabled is false`);
+    }
+
+    if (!config.credentials) {
+        config.credentials = createCredentialsObject(config);
+    }
+
+    return config;
+}
+
+/**
+ * Given the S3 client configuration, return httpOptions containig CA certs.
+ * @param {S3ClientConfig} config S3 client configuration object
+ * @returns {Promise<HttpsAgentOptions>} Options used to make HttpsAgent
+ */
+export async function createHttpOptions(
+    config: S3ClientConfig
+): Promise<HttpsAgentOptions> {
     // pull certLocation from env
     // https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/node-registering-certs.html
     // Instead of updating the client, we can just update the config before creating the client
-    if (config.sslEnabled) {
-        const certPath = config.certLocation ?? '/etc/ssl/certs/ca-certificates.crt';
-        const pathFound = await fs.exists(certPath);
 
-        if (!pathFound) {
-            throw new Error(
-                `No cert path was found in config.certLocation: "${config.certLocation}" or in default "/etc/ssl/certs/ca-certificates.crt" location`
-            );
-        }
-        // Assumes all certs needed are in a single bundle
-        const certs = await fs.readFile(certPath);
-
-        httpOptions = {
-            ...httpOptions,
-            rejectUnauthorized: true,
-            ca: [certs]
-        };
+    const certPath = config.certLocation ?? '/etc/ssl/certs/ca-certificates.crt';
+    const pathFound = await fs.exists(certPath);
+
+    if (!pathFound) {
+        throw new Error(
+            `No cert path was found in config.certLocation: "${config.certLocation}" or in default "/etc/ssl/certs/ca-certificates.crt" location`
+        );
     }
+    // Assumes all certs needed are in a single bundle
+    const certs = await fs.readFile(certPath);
 
-    const { connectionTimeout, requestTimeout } = config.handlerOptions || {};
-    const requestHandlerOptions = {
-        ...isNumber(connectionTimeout) && { connectionTimeout },
-        ...isNumber(requestTimeout) && { requestTimeout },
-        ...!isEmpty(httpOptions) && { httpsAgent: new Agent(httpOptions) }
+    return {
+        rejectUnauthorized: true,
+        ca: [certs]
     };
+}
 
-    if (!isEmpty(requestHandlerOptions)) {
-        config.requestHandler = new NodeHttpHandler(requestHandlerOptions);
-    }
+/**
+ * Given HttpAgentOptions, create NodeHttpHandlerOptions
+ * @param {HttpsAgentOptions} httpOptions
+ * @returns {NodeHttpHandlerOptions}
+ */
+export function createRequestHandlerOptions(
+    httpOptions: HttpsAgentOptions
+): NodeHttpHandlerOptions {
+    return { httpsAgent: new Agent(httpOptions) };
+}
 
-    // config specified old style, need to move top level values into credentials
-    if (!has(config, 'credentials') && has(config, 'accessKeyId') && has(config, 'secretAccessKey')) {
-        const { accessKeyId = '', secretAccessKey = '' } = config;
-        config.credentials = {
-            accessKeyId,
-            secretAccessKey
-        };
+/**
+ * Given an S3 client configuration, create a credentials object
+ * containing accessKeyId and secretAccessKey.
+ * S3Client V3 expects accessKeyId and secretAccessKey to be within a credentials object.
+ * @param {S3ClientConfig} config S3 client configuration object
+ * @returns {S3ClientCredentials}
+ */
+export function createCredentialsObject(
+    config: S3ClientConfig
+): S3ClientCredentials {
+    if (!config.accessKeyId) {
+        throw new Error('S3 accessKeyId must be defined in S3ClientConfig');
     }
-
-    // specified in terafoundation connector config but is now maxAttempts
-    if (!has(config, 'maxAttempts') && has(config, 'maxRetries')) {
-        config.maxAttempts = (config as any).maxRetries;
+    if (!config.secretAccessKey) {
+        throw new Error('S3 secretAccessKey must be defined in S3ClientConfig');
     }
-
-    return new BaseClient(config);
+    const { accessKeyId, secretAccessKey } = config;
+    return {
+        accessKeyId,
+        secretAccessKey
+    };
 }

packages/file-asset-apis/src/s3/s3-sender.ts

@@ -106,6 +106,12 @@ export class S3Sender extends ChunkedFileSender implements RouteSenderAPI {
             try {
                 await createS3Bucket(this.client, params);
             } catch (err) {
+                // Unclear TypeError when trying to connect to a secure S3 bucket over http
+                if (err instanceof TypeError && err.message.includes('Cannot read properties of undefined (reading \'#text\')')) {
+                    throw new TSError(err, {
+                        reason: `Failure connecting to secure bucket ${this.path} from an http endpoint`
+                    });
+                }
                 throw new TSError(err, {
                     reason: `Failure to setup bucket ${this.path}`
                 });

packages/file-asset-apis/test/__fixtures__/cert/fakeCert.pem

@@ -0,0 +1,2 @@
+createS3Client-spec just needs to find a file in this directory.
+There is no need to use an actual cert with the tests running currently.