Fix v2 client reuse regression, and make it even better than before

GrahamCampbell · GrahamCampbell · commit 252ea8044ab5 · 2025-06-09T01:01:30.000+01:00
diff --git a/lib/aws/v2-client-factory.js b/lib/aws/v2-client-factory.js
@@ -0,0 +1,136 @@
+'use strict';
+
+const memoize = require('memoizee');
+const sdk = require('./sdk-v2');
+const deepSortObjectByKey = require('../utils/deep-sort-object-by-key');
+
+// Map service names to their SDK v2 classes
+const SERVICE_MAP = {
+  APIGateway: sdk.APIGateway,
+  ApiGatewayV2: sdk.ApiGatewayV2,
+  CloudFormation: sdk.CloudFormation,
+  CloudWatch: sdk.CloudWatch,
+  CloudWatchLogs: sdk.CloudWatchLogs,
+  CognitoIdentityServiceProvider: sdk.CognitoIdentityServiceProvider,
+  DynamoDB: sdk.DynamoDB,
+  ECR: sdk.ECR,
+  EventBridge: sdk.EventBridge,
+  IAM: sdk.IAM,
+  Iot: sdk.Iot,
+  IotData: sdk.IotData,
+  Kafka: sdk.Kafka,
+  Kinesis: sdk.Kinesis,
+  Lambda: sdk.Lambda,
+  MQ: sdk.MQ,
+  S3: sdk.S3,
+  SecretsManager: sdk.SecretsManager,
+  SNS: sdk.SNS,
+  SQS: sdk.SQS,
+  STS: sdk.STS,
+};
+
+class AWSV2ClientFactory {
+  constructor(baseConfig = {}) {
+    this.baseConfig = baseConfig;
+    this.clients = new Map();
+
+    // Create memoized request function for response caching
+    this.memoizedRequest = memoize(this._executeRequest.bind(this), {
+      promise: true,
+      normalizer: ([serviceName, method, params, clientConfig]) => {
+        return [
+          serviceName,
+          method,
+          JSON.stringify(deepSortObjectByKey(params)),
+          JSON.stringify(deepSortObjectByKey(clientConfig)),
+        ].join('|');
+      },
+    });
+  }
+
+  /**
+   * Get a configured AWS service client
+   * @param {string} serviceName - Name of the AWS service (e.g., 'S3', 'CloudFormation')
+   * @param {Object} overrideConfig - Configuration to override base config
+   * @returns {Object} AWS SDK v2 service instance
+   */
+  getClient(serviceName, overrideConfig = {}) {
+    const ServiceClass = SERVICE_MAP[serviceName];
+    if (!ServiceClass) {
+      throw new Error(`Unknown AWS service: ${serviceName}`);
+    }
+
+    // Create cache key - include service + sorted config for consistent keys
+    const configKey = JSON.stringify({
+      serviceName,
+      ...deepSortObjectByKey({ ...this.baseConfig, ...overrideConfig }),
+    });
+
+    if (!this.clients.has(configKey)) {
+      const clientConfig = { ...this.baseConfig, ...overrideConfig };
+      this.clients.set(configKey, new ServiceClass(clientConfig));
+    }
+
+    return this.clients.get(configKey);
+  }
+
+  /**
+   * Execute an AWS request with optional response caching
+   * @param {string} serviceName - Name of the AWS service
+   * @param {string} method - Method to call on the service
+   * @param {Object} params - Parameters for the AWS API call
+   * @param {Object} clientConfig - Client configuration
+   * @param {Object} options - Request options
+   * @param {boolean} options.useCache - Whether to cache the response
+   * @returns {Promise} Result of the AWS API call
+   */
+  async request(serviceName, method, params, clientConfig = {}, options = {}) {
+    const { useCache = false } = options;
+
+    if (useCache) {
+      return this.memoizedRequest(serviceName, method, params, clientConfig);
+    }
+
+    return this._executeRequest(serviceName, method, params, clientConfig);
+  }
+
+  /**
+   * Execute the actual AWS request
+   * @private
+   */
+  async _executeRequest(serviceName, method, params, clientConfig) {
+    const client = this.getClient(serviceName, clientConfig);
+    return client[method](params).promise();
+  }
+
+  /**
+   * Clear the client cache
+   */
+  clearCache() {
+    this.clients.clear();
+    // Clear response cache if it exists
+    if (this.memoizedRequest.clear) {
+      this.memoizedRequest.clear();
+    }
+  }
+
+  /**
+   * Update the base configuration for all future clients
+   * @param {Object} newConfig - New base configuration
+   */
+  updateBaseConfig(newConfig) {
+    this.baseConfig = { ...this.baseConfig, ...newConfig };
+    // Clear cache to force recreation with new config
+    this.clearCache();
+  }
+
+  /**
+   * Get list of supported AWS services
+   * @returns {Array} Array of supported service names
+   */
+  getSupportedServices() {
+    return Object.keys(SERVICE_MAP);
+  }
+}
+
+module.exports = AWSV2ClientFactory;
diff --git a/lib/aws/v3-client-factory.js b/lib/aws/v3-client-factory.js
@@ -31,7 +31,7 @@ const CLIENT_MAP = {
   STS: STSClient,
 };
 
-class AWSClientFactory {
+class AWSV3ClientFactory {
   constructor(baseConfig = {}) {
     this.baseConfig = baseConfig;
     this.clients = new Map();
@@ -98,4 +98,4 @@ class AWSClientFactory {
   }
 }
 
-module.exports = AWSClientFactory;
+module.exports = AWSV3ClientFactory;
diff --git a/lib/plugins/aws/provider.js b/lib/plugins/aws/provider.js
@@ -13,14 +13,13 @@ const d = require('d');
 const path = require('path');
 const spawnExt = require('child-process-ext/spawn');
 const ServerlessError = require('../../serverless-error');
-const awsRequest = require('../../aws/request');
 const { cfValue } = require('../../utils/aws-schema-get-cf-value');
 const reportDeprecatedProperties = require('../../utils/report-deprecated-properties');
 const deepSortObjectByKey = require('../../utils/deep-sort-object-by-key');
 const { progress, log } = require('@serverless/utils/log');
 
-// AWS SDK v3 infrastructure
-const AWSClientFactory = require('../../aws/client-factory');
+const AWSV2ClientFactory = require('../../aws/v2-client-factory');
+const AWSV3ClientFactory = require('../../aws/v3-client-factory');
 const { createCommand } = require('../../aws/commands');
 const { buildClientConfig, shouldUseS3Acceleration } = require('../../aws/config');
 const { transformV3Error } = require('../../aws/error-utils');
@@ -1736,20 +1735,33 @@ class AwsProvider {
    * @private
    */
   async _requestV2(service, method, params, options) {
-    // TODO: Determine calling module and log that
     const requestOptions = _.isObject(options) ? options : {};
     const shouldCache = _.get(requestOptions, 'useCache', false);
-    // Copy is required as the credentials may be modified during the request
-    const credentials = Object.assign({}, this.getCredentials());
-    const serviceOptions = {
-      name: service,
-      params: {
-        ...credentials,
-        region: _.get(requestOptions, 'region', this.getRegion()),
-        isS3TransferAccelerationEnabled: this.isS3TransferAccelerationEnabled(),
-      },
+    const region = _.get(requestOptions, 'region', this.getRegion());
+
+    // Initialize factory if needed
+    if (!this.v2ClientFactory) {
+      this.v2ClientFactory = new AWSV2ClientFactory(this._getV2BaseConfig());
+    }
+
+    // Build client config (only configuration that affects client creation)
+    const clientConfig = {
+      ...this.getCredentials(),
+      region,
     };
-    return (shouldCache ? awsRequest.memoized : awsRequest)(serviceOptions, method, params);
+
+    // Handle S3 acceleration
+    if (service === 'S3') {
+      const accelerationCompatibleMethods = new Set(['upload', 'putObject']);
+      if (accelerationCompatibleMethods.has(method) && this.isS3TransferAccelerationEnabled()) {
+        clientConfig.useAccelerateEndpoint = true;
+      }
+    }
+
+    // Use factory's request method which handles both client caching and response caching
+    return this.v2ClientFactory.request(service, method, params, clientConfig, {
+      useCache: shouldCache,
+    });
   }
 
   /**
@@ -1764,8 +1776,8 @@ class AwsProvider {
   async _requestV3(service, method, params = {}, options = {}) {
     try {
       // Initialize client factory if not already done
-      if (!this.clientFactory) {
-        this.clientFactory = new AWSClientFactory(this._getV3BaseConfig());
+      if (!this.v3ClientFactory) {
+        this.v3ClientFactory = new AWSV3ClientFactory(this._getV3BaseConfig());
       }
 
       // Build client configuration
@@ -1775,7 +1787,7 @@ class AwsProvider {
       const command = createCommand(service, method, params);
 
       // Send request
-      const result = await this.clientFactory.send(service, command, clientConfig);
+      const result = await this.v3ClientFactory.send(service, command, clientConfig);
 
       return result;
     } catch (error) {
@@ -1791,12 +1803,12 @@ class AwsProvider {
    * @returns {Object} AWS SDK v3 client instance
    */
   getV3Client(service, options = {}) {
-    if (!this.clientFactory) {
-      this.clientFactory = new AWSClientFactory(this._getV3BaseConfig());
+    if (!this.v3ClientFactory) {
+      this.v3ClientFactory = new AWSV3ClientFactory(this._getV3BaseConfig());
     }
 
     const clientConfig = this._buildV3ClientConfig(service, null, options);
-    return this.clientFactory.getClient(service, clientConfig);
+    return this.v3ClientFactory.getClient(service, clientConfig);
   }
 
   /**
@@ -1837,6 +1849,72 @@ class AwsProvider {
     return baseConfig;
   }
 
+  /**
+   * Build base configuration for AWS SDK v2 clients
+   * @private
+   */
+  _getV2BaseConfig() {
+    const config = {};
+
+    // Handle proxy configuration
+    const proxy =
+      process.env.proxy ||
+      process.env.HTTP_PROXY ||
+      process.env.http_proxy ||
+      process.env.HTTPS_PROXY ||
+      process.env.https_proxy;
+
+    const proxyOptions = {};
+    if (proxy) {
+      Object.assign(proxyOptions, require('url').parse(proxy));
+    }
+
+    // Handle CA certificates
+    const ca = process.env.ca || process.env.HTTPS_CA || process.env.https_ca;
+    let caCerts = [];
+
+    if (ca) {
+      const caArr = ca.split(',');
+      caCerts = caCerts.concat(caArr.map((cert) => cert.replace(/\\n/g, '\n')));
+    }
+
+    const cafile = process.env.cafile || process.env.HTTPS_CAFILE || process.env.https_cafile;
+    if (cafile) {
+      const caPathArr = cafile.split(',');
+      caCerts = caCerts.concat(
+        caPathArr.map((cafilePath) => require('fs').readFileSync(cafilePath.trim()))
+      );
+    }
+
+    if (caCerts.length > 0) {
+      Object.assign(proxyOptions, {
+        rejectUnauthorized: true,
+        ca: caCerts,
+      });
+    }
+
+    // Set up HTTP options
+    const httpOptions = {};
+
+    if (proxy) {
+      httpOptions.agent = new (require('https-proxy-agent'))(proxyOptions);
+    } else if (proxyOptions.ca) {
+      httpOptions.agent = new (require('https').Agent)(proxyOptions);
+    }
+
+    // Handle timeout
+    const timeout = process.env.AWS_CLIENT_TIMEOUT || process.env.aws_client_timeout;
+    if (timeout) {
+      httpOptions.timeout = parseInt(timeout, 10);
+    }
+
+    if (Object.keys(httpOptions).length > 0) {
+      config.httpOptions = httpOptions;
+    }
+
+    return config;
+  }
+
   /**
    * Fetch credentials directly or using a profile from serverless yml configuration or from the
    * well known environment variables

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ const CLIENT_MAP = {`
`31`	`31`	`STS: STSClient,`
`32`	`32`	`};`
`33`	`33`
`34`		`-class AWSClientFactory {`
	`34`	`+class AWSV3ClientFactory {`
`35`	`35`	`constructor(baseConfig = {}) {`
`36`	`36`	`this.baseConfig = baseConfig;`
`37`	`37`	`this.clients = new Map();`
`@@ -98,4 +98,4 @@ class AWSClientFactory {`
`98`	`98`	`}`
`99`	`99`	`}`
`100`	`100`
`101`		`-module.exports = AWSClientFactory;`
	`101`	`+module.exports = AWSV3ClientFactory;`