Cannot read properties of undefined (reading 'hostname') in @aws-sdk/client-s3 instrumentation

[trentm]

In #3454 (comment) it was reported that instrumentation of @aws-sdk/client-s3 could throw:

I'm not sure if directly related to your test failures but the upgrade of elastic-apm-node from 3.45.0 to 3.47.0 broke S3 interactions like generating pre-signed URLs, throwing:

Cannot read properties of undefined (reading 'hostname')
at (node_modules/elastic-apm-node/lib/instrumentation/modules/@aws-sdk/client-s3.js:155)

Using client-s3 version 3.332.0.

---

@AndKiel Thanks for reporting this! Instrumentation of @aws-sdk/client-s3 was first added in v3.46.0 of this APM agent. I'm almost certain this is a separate issue from #3454, so I have moved it here.

If you had a small reproduction script, or even just a small snippet of JavaScript code showing the S3 Client API calls that hit this, that would be very helpful for me attempting to reproduce.

---

Our instrumentation is adding aws-sdk v3 middleware -- to the "initialize" and "finalizeRequest" steps. The error above could happen if the "finalizeRequest" step is not run for a particular client call.

[trentm]

@AndKiel For what it is worth, I cannot repro using:

const {
  S3Client,
  GetObjectCommand,
} = require('@aws-sdk/client-s3')
const { getSignedUrl } = require('@aws-sdk/s3-request-presigner')
...
  command = new GetObjectCommand({ Bucket: bucketName, Key: key })
  const signedUrl = await getSignedUrl(s3Client, command, { expiresIn: 3600 })
  console.log('signedUrl: ', signedUrl)
...

I'm using the latest elastic-apm-node (the HEAD of the "main" git branch), and @aws-sdk/client-s3@3.363.0 and @aws-sdk/s3-request-presigner@3.363.0.

[AndKiel]

The full stack trace we got would be:

Cannot read properties of undefined (reading 'hostname')
  at (node_modules/elastic-apm-node/lib/instrumentation/modules/@aws-sdk/client-s3.js:155)
  at processTicksAndRejections (node:internal/process/task_queues:95)
  at getSignedUrl (node_modules/@aws-sdk/s3-request-presigner/dist-cjs/getSignedUrl.js:54)
  at middleware (node_modules/graphql-shield/cjs/generator.js:30)

It happened on a graphQL field resolver that's supposed to return a signed URL for downloading a file. What we're doing boils down to the:

import { GetObjectCommand, S3Client } from "@aws-sdk/client-s3";
import { getSignedUrl } from "@aws-sdk/s3-request-presigner";

// In our code client is cached in a global injectable service (we're using typedi)
const s3 = new S3Client({ region: "us-east-1" })
// That global service is a wrapper and it does signing just like your snippet above
getSignedUrl(s3, new GetObjectCommand({ Bucket, Key }), { expiresIn: 60 * 60 * 24 * 7 });

As mentioned, we were on 3.332.0 version of aws packages.

[trentm]

@AndKiel Thanks. I was able to reproduce, finally. The subtlety is that there needs to be a current APM span when the getSignedUrl(...) function is called, otherwise it doesn't hit the code path in our instrumentation that has the bug.

I'll get a fix together soon.

implementation details

Our instrumentation does this:

• adds middleware to the client for the "initialize" and "finalizeRequest" steps
• wraps client.send(...) to start a span
• Our "initialize" middleware: adds some span data, awaits the full middleware chain, adds some span data when complete, and ends the span.
• Our "finalizeRequest" middleware stashes some data from the built request object that is used by that "adds some span data when complete".

The first surprise is that client.send() isn't the only code path in the aws-sdk that executes the handler stack. The main one that we expect is here: https://github.com/awslabs/smithy-typescript/blob/b1e7afa9bc419d6f5f20945c150cd1a9e9ffa0b3/packages/smithy-client/src/client.ts#L56-L70

However, here are two others (the only two others I could find):

• getSignedUrl: https://github.com/aws/aws-sdk-js-v3/blob/eb03dbdee3dd804e8bebf4c6279640387107b0ed/packages/s3-request-presigner/src/getSignedUrl.ts#L71-L72
• updateDiscoveredEndpointInCache: https://github.com/aws/aws-sdk-js-v3/blob/eb03dbdee3dd804e8bebf4c6279640387107b0ed/packages/middleware-endpoint-discovery/src/updateDiscoveredEndpointInCache.ts#L40-L45

The result with getSignedUrl was two issues:

1. The if (!span) guard in our "initialize" middleware is insufficient, because there might be some span other than the span we expect to have been started in our client.send() wrapper.
2. Middleware added by getSignedUrl does not call next(). This means that the finalizeRequest step middleware never gets run -- including our middleware that stashes some data. When the initialize middleware then finishes up (after await next()) it crashed because it assumed that stash would be there.

Fixing either of those cases deals with this issue, but I'll have a PR that addresses both.