fix: @aws-sdk/client-s3 instrumentation could cause getSignedUrl to crash (#3467)

Closes: #3464

Author: trentm

CHANGELOG.asciidoc

@@ -81,6 +81,9 @@ See <<esm>> for full details.
 * Fix aws-sdk v3 instrumentation (currently just `@aws-sdk/client-s3`) for
   versions 3.363.0 and later. ({pull}3455[#3455])
 
+* Fix a possible crash when using `getSignedUrl()` from `@aws-sdk/s3-request-presigner`
+  due to a bug in `@aws-sdk/client-s3` instrumentation. ({issues}3464[#3464])
+
 [float]
 ===== Chores
 

lib/instrumentation/modules/@aws-sdk/client-s3.js

@@ -61,14 +61,15 @@ function s3MiddlewareFactory (client, agent) {
   return [
     {
       middleware: (next, context) => async (args) => {
-        const input = args.input
-        const bucket = input && input.Bucket
-        const resource = resourceFromBucket(bucket)
+        // Ensure there is a span from the wrapped `client.send()`.
         const span = agent._instrumentation.currSpan()
-
-        if (!span) {
+        if (!span || !(span.type === TYPE && span.subtype === SUBTYPE)) {
           return await next(args)
         }
+
+        const input = args.input
+        const bucket = input && input.Bucket
+        const resource = resourceFromBucket(bucket)
         // The given span comes with the operation name and we need to
         // add the resource if applies
         if (resource) {
@@ -152,17 +153,18 @@ function s3MiddlewareFactory (client, agent) {
 
           // Destination context.
           const destContext = {
-            address: context[elasticAPMStash].hostname,
-            port: context[elasticAPMStash].port,
             service: {
               name: SUBTYPE,
               type: TYPE
             }
           }
+          if (context[elasticAPMStash]) {
+            destContext.address = context[elasticAPMStash].hostname
+            destContext.port = context[elasticAPMStash].port
+          }
           if (resource) {
             destContext.service.resource = resource
           }
-
           if (region) {
             destContext.cloud = { region }
           }
@@ -190,7 +192,6 @@ function s3MiddlewareFactory (client, agent) {
         }
 
         context[elasticAPMStash] = {
-          protocol: req.protocol,
           hostname: req.hostname,
           port
         }

package-lock.json

@@ -123,6 +123,7 @@
   "devDependencies": {
     "@apollo/server": "^4.2.2",
     "@aws-sdk/client-s3": "^3.363.0",
+    "@aws-sdk/s3-request-presigner": "^3.363.0",
     "@babel/cli": "^7.8.4",
     "@babel/core": "^7.8.4",
     "@babel/preset-env": "^7.8.4",

package.json

@@ -77,7 +77,7 @@ const testFixtures = [
         spans.length, 'all spans have sync=false')
       t.equal(spans.filter(s => s.sample_rate === 1).length,
         spans.length, 'all spans have sample_rate=1')
-      const failingSpanId = spans[7].id // index of `getObjNonExistantObject`
+      const failingSpanId = spans[8].id // index of `getObjNonExistantObject`
       spans.forEach(s => {
         // Remove variable and common fields to facilitate t.deepEqual below.
         delete s.id
@@ -224,6 +224,14 @@ const testFixtures = [
         outcome: 'success'
       }, 'getObj produced expected span')
 
+      t.deepEqual(spans.shift(), {
+        name: 'get-signed-url',
+        type: 'custom',
+        subtype: null,
+        action: null,
+        outcome: 'success'
+      }, 'custom span for getSignedUrl call')
+
       t.deepEqual(spans.shift(), {
         name: 'S3 GetObject elasticapmtest-bucket-3',
         type: 'storage',

test/instrumentation/modules/@aws-sdk/client-s3.test.js

@@ -63,11 +63,34 @@ const {
   waitUntilBucketExists,
   waitUntilObjectExists
 } = require('@aws-sdk/client-s3')
+const { getSignedUrl } = require('@aws-sdk/s3-request-presigner')
 
 const TEST_BUCKET_NAME_PREFIX = 'elasticapmtest-bucket-'
 
 // ---- support functions
 
+/**
+ * Slurp everything from the given ReadableStream and return the content,
+ * converted to a string.
+ */
+async function slurpStream (stream) {
+  return new Promise((resolve, reject) => {
+    const chunks = []
+    stream.on('error', (err) => {
+      reject(err)
+    })
+    stream.on('readable', function () {
+      let chunk
+      while ((chunk = this.read()) !== null) {
+        chunks.push(chunk)
+      }
+    })
+    stream.on('end', () => {
+      resolve(Buffer.concat(chunks).toString('utf8'))
+    })
+  })
+}
+
 // https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-s3/index.html
 async function useClientS3 (s3Client, bucketName) {
   const region = await s3Client.config.region()
@@ -129,14 +152,30 @@ async function useClientS3 (s3Client, bucketName) {
   // https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-s3/classes/getobjectcommand.html
   command = new GetObjectCommand({ Bucket: bucketName, Key: key })
   data = await s3Client.send(command)
-  log.info({ data }, 'getObject')
+  // `data.Body` is a *stream*, so we cannot just log it
+  // https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-s3/interfaces/getobjectcommandoutput.html#body
+  const body = await slurpStream(data.Body)
+  assert(body === content)
+  delete data.Body
+  log.info({ data, body }, 'getObject')
+
+  // Get a signed URL.
+  // This is interesting to test, because `getSignedUrl` uses the command
+  // `middlewareStack` -- including our added middleware -- **without** calling
+  // `s3Client.send()`. The test here is to ensure this doesn't break.
+  const customSpan = apm.startSpan('get-signed-url')
+  const signedUrl = await getSignedUrl(
+    s3Client,
+    new GetObjectCommand({ Bucket: bucketName, Key: key }),
+    { expiresIn: 3600 })
+  log.info({ signedUrl }, 'getSignedUrl')
+  customSpan.end()
 
   command = new GetObjectCommand({
     IfNoneMatch: etag,
     Bucket: bucketName,
     Key: key
   })
-
   try {
     data = await s3Client.send(command)
     throw new Error('expected NotModified error for conditional request')
@@ -228,7 +267,8 @@ function main () {
       s3Client.destroy()
       process.exitCode = 0
     },
-    function () {
+    function (err) {
+      apm.logger.error(err, 'useClientS3 rejected')
       tx.setOutcome('failure')
       tx.end()
       s3Client.destroy()