Skip to content

Commit 2dc10c6

Browse files
kuhesiddsriv
kuhe
and
siddsriv
authored
fix(ec2-metadata-service): discard response body stream on failed request (#7543)
* fix(ec2-metadata-service): discard response body stream on failed request * test(ec2-metadata-service): add unit test/socket leak checks * test(ec2-metadata-service): declaration merging for Node util to get resources --------- Co-authored-by: Siddharth Srivastava <sidddsri@amazon.com>
1 parent da6eee7 commit 2dc10c6

File tree

2 files changed

+271
-8
lines changed

2 files changed

+271
-8
lines changed

‎packages/ec2-metadata-service/src/MetadataService.spec.ts‎

Lines changed: 255 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,255 @@
1+
import { NodeHttpHandler } from "@smithy/node-http-handler";
2+
import { Readable } from "stream";
3+
import { beforeEach, describe, expect, test as it, vi } from "vitest";
4+
5+
import { MetadataService } from "./MetadataService";
6+
7+
// Type declaration (declaration merging) for process.getActiveResourcesInfo()
8+
declare global {
9+
namespace NodeJS {
10+
interface Process {
11+
getActiveResourcesInfo(): string[];
12+
}
13+
}
14+
}
15+
16+
vi.mock("@smithy/node-http-handler");
17+
18+
// gh#7538
19+
describe("MetadataService Socket Leak Checks", () => {
20+
let metadataService: MetadataService;
21+
22+
beforeEach(() => {
23+
vi.clearAllMocks();
24+
metadataService = new MetadataService({
25+
endpoint: "http://169.254.169.254",
26+
httpOptions: { timeout: 1000 },
27+
});
28+
});
29+
30+
const createMockResponse = (statusCode: number, body: string) => {
31+
const stream = Readable.from([body]);
32+
return {
33+
response: {
34+
statusCode,
35+
body: stream,
36+
headers: {},
37+
},
38+
};
39+
};
40+
41+
describe("fetchMetadataToken - body consumption checks", () => {
42+
it("should consume response body on 200 status and return token", async () => {
43+
const mockToken = "test-token-123";
44+
const mockResponse = createMockResponse(200, mockToken);
45+
46+
vi.mocked(NodeHttpHandler).mockImplementation(
47+
() =>
48+
({
49+
handle: vi.fn().mockResolvedValue(mockResponse),
50+
} as any)
51+
);
52+
53+
const token = await metadataService.fetchMetadataToken();
54+
55+
expect(token).toBe(mockToken);
56+
expect(mockResponse.response.body.readableEnded).toBe(true);
57+
});
58+
59+
it("should consume response body on 400 status before throwing", async () => {
60+
const mockResponse = createMockResponse(400, "Bad Request");
61+
62+
vi.mocked(NodeHttpHandler).mockImplementation(
63+
() =>
64+
({
65+
handle: vi.fn().mockResolvedValue(mockResponse),
66+
} as any)
67+
);
68+
69+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow(
70+
"Failed to fetch metadata token with status code 400"
71+
);
72+
73+
expect(mockResponse.response.body.readableEnded).toBe(true);
74+
});
75+
76+
it("should consume response body on 500 status before throwing", async () => {
77+
const mockResponse = createMockResponse(500, "Server Error");
78+
79+
vi.mocked(NodeHttpHandler).mockImplementation(
80+
() =>
81+
({
82+
handle: vi.fn().mockResolvedValue(mockResponse),
83+
} as any)
84+
);
85+
86+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow(
87+
"Failed to fetch metadata token with status code 500"
88+
);
89+
90+
expect(mockResponse.response.body.readableEnded).toBe(true);
91+
});
92+
93+
it("should include statusCode in error object for non-200 responses", async () => {
94+
const mockResponse = createMockResponse(400, "Bad Request");
95+
96+
vi.mocked(NodeHttpHandler).mockImplementation(
97+
() =>
98+
({
99+
handle: vi.fn().mockResolvedValue(mockResponse),
100+
} as any)
101+
);
102+
103+
try {
104+
await metadataService.fetchMetadataToken();
105+
expect.fail("Should have thrown an error");
106+
} catch (error: any) {
107+
expect(error.message).toContain("400");
108+
expect(error.message).toContain("Failed to fetch metadata token");
109+
}
110+
});
111+
});
112+
113+
describe("fetchMetadataToken - error handling", () => {
114+
it("should set disableFetchToken on 403 error", async () => {
115+
const mockResponse = createMockResponse(403, "Forbidden");
116+
117+
vi.mocked(NodeHttpHandler).mockImplementation(
118+
() =>
119+
({
120+
handle: vi.fn().mockResolvedValue(mockResponse),
121+
} as any)
122+
);
123+
124+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow("[disableFetchToken] is now set to true");
125+
126+
expect((metadataService as any).disableFetchToken).toBe(true);
127+
});
128+
129+
it("should set disableFetchToken on 404 error", async () => {
130+
const mockResponse = createMockResponse(404, "Not Found");
131+
132+
vi.mocked(NodeHttpHandler).mockImplementation(
133+
() =>
134+
({
135+
handle: vi.fn().mockResolvedValue(mockResponse),
136+
} as any)
137+
);
138+
139+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow("[disableFetchToken] is now set to true");
140+
141+
expect((metadataService as any).disableFetchToken).toBe(true);
142+
});
143+
144+
it("should set disableFetchToken on 405 error", async () => {
145+
const mockResponse = createMockResponse(405, "Method Not Allowed");
146+
147+
vi.mocked(NodeHttpHandler).mockImplementation(
148+
() =>
149+
({
150+
handle: vi.fn().mockResolvedValue(mockResponse),
151+
} as any)
152+
);
153+
154+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow("[disableFetchToken] is now set to true");
155+
156+
expect((metadataService as any).disableFetchToken).toBe(true);
157+
});
158+
159+
it("should set disableFetchToken on TimeoutError", async () => {
160+
vi.mocked(NodeHttpHandler).mockImplementation(
161+
() =>
162+
({
163+
handle: vi.fn().mockRejectedValue({ message: "TimeoutError" }),
164+
} as any)
165+
);
166+
167+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow("[disableFetchToken] is now set to true");
168+
169+
expect((metadataService as any).disableFetchToken).toBe(true);
170+
});
171+
172+
it("should NOT set disableFetchToken on 400 error", async () => {
173+
const mockResponse = createMockResponse(400, "Bad Request");
174+
175+
vi.mocked(NodeHttpHandler).mockImplementation(
176+
() =>
177+
({
178+
handle: vi.fn().mockResolvedValue(mockResponse),
179+
} as any)
180+
);
181+
182+
await expect(metadataService.fetchMetadataToken()).rejects.toThrow();
183+
184+
expect((metadataService as any).disableFetchToken).toBe(false);
185+
});
186+
});
187+
188+
describe("socket cleanup verification", () => {
189+
it("should not leave open handles after 400 response", async () => {
190+
const initialResources = process.getActiveResourcesInfo();
191+
const mockResponse = createMockResponse(400, "Bad Request");
192+
193+
vi.mocked(NodeHttpHandler).mockImplementation(
194+
() =>
195+
({
196+
handle: vi.fn().mockResolvedValue(mockResponse),
197+
} as any)
198+
);
199+
200+
try {
201+
await metadataService.fetchMetadataToken();
202+
} catch {
203+
// expected to throw
204+
}
205+
// allowing event loop to finish processing (test could be flaky otherwise if socket isn't closed because it remained scheduled in the event loop here)
206+
await new Promise((resolve) => setImmediate(resolve));
207+
208+
const finalResources = process.getActiveResourcesInfo();
209+
const socketCount = (resources: string[]) =>
210+
resources.filter((r) => r.includes("Socket") || r.includes("TCP")).length;
211+
expect(socketCount(finalResources)).toBeLessThanOrEqual(socketCount(initialResources));
212+
});
213+
214+
it("should not leave open handles after successful 200 response", async () => {
215+
const initialResources = process.getActiveResourcesInfo();
216+
const mockResponse = createMockResponse(200, "test-token");
217+
218+
vi.mocked(NodeHttpHandler).mockImplementation(
219+
() =>
220+
({
221+
handle: vi.fn().mockResolvedValue(mockResponse),
222+
} as any)
223+
);
224+
225+
await metadataService.fetchMetadataToken();
226+
227+
await new Promise((resolve) => setImmediate(resolve));
228+
229+
const finalResources = process.getActiveResourcesInfo();
230+
const socketCount = (resources: string[]) =>
231+
resources.filter((r) => r.includes("Socket") || r.includes("TCP")).length;
232+
expect(socketCount(finalResources)).toBeLessThanOrEqual(socketCount(initialResources));
233+
});
234+
});
235+
236+
describe("throwOnRequestTimeout flag", () => {
237+
it("should pass throwOnRequestTimeout: true to NodeHttpHandler", async () => {
238+
const mockResponse = createMockResponse(200, "test-token");
239+
const mockHandle = vi.fn().mockResolvedValue(mockResponse);
240+
241+
vi.mocked(NodeHttpHandler).mockImplementation((config: any) => {
242+
expect(config.throwOnRequestTimeout).toBe(true);
243+
return { handle: mockHandle } as any;
244+
});
245+
246+
await metadataService.fetchMetadataToken();
247+
248+
expect(NodeHttpHandler).toHaveBeenCalledWith(
249+
expect.objectContaining({
250+
throwOnRequestTimeout: true,
251+
})
252+
);
253+
});
254+
});
255+
});

‎packages/ec2-metadata-service/src/MetadataService.ts‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { MetadataServiceOptions } from "./MetadataServiceOptions";
1313
export class MetadataService {
1414
private disableFetchToken: boolean;
1515
private config: Promise<MetadataServiceOptions>;
16+
1617
/**
1718
* Creates a new MetadataService object with a given set of options.
1819
*/
@@ -35,6 +36,7 @@ export class MetadataService {
3536
const { endpoint, ec2MetadataV1Disabled, httpOptions } = await this.config;
3637
const handler = new NodeHttpHandler({
3738
requestTimeout: httpOptions?.timeout,
39+
throwOnRequestTimeout: true,
3840
connectionTimeout: httpOptions?.timeout,
3941
});
4042
const endpointUrl = new URL(endpoint!);
@@ -88,6 +90,7 @@ export class MetadataService {
8890
const { endpoint, httpOptions } = await this.config;
8991
const handler = new NodeHttpHandler({
9092
requestTimeout: httpOptions?.timeout,
93+
throwOnRequestTimeout: true,
9194
connectionTimeout: httpOptions?.timeout,
9295
});
9396
const endpointUrl = new URL(endpoint!);
@@ -102,18 +105,23 @@ export class MetadataService {
102105
});
103106
try {
104107
const { response } = await handler.handle(tokenRequest, {} as HttpHandlerOptions);
105-
if (response.statusCode === 200 && response.body) {
106-
// handle response.body as a stream
107-
return sdkStreamMixin(response.body).transformToString();
108+
109+
let bodyString = "";
110+
if (response.body) {
111+
bodyString = await sdkStreamMixin(response.body).transformToString();
112+
}
113+
114+
if (response.statusCode === 200 && bodyString) {
115+
return bodyString;
108116
} else {
109-
throw new Error(`Failed to fetch metadata token with status code ${response.statusCode}`);
117+
throw Object.assign(new Error(`Failed to fetch metadata token with status code ${response.statusCode}`), {
118+
statusCode: response.statusCode,
119+
});
110120
}
111121
} catch (error) {
112-
if (error?.statusCode === 400) {
113-
throw new Error(`Error fetching metadata token: ${error}`);
114-
} else if (error.message === "TimeoutError" || [403, 404, 405].includes(error.statusCode)) {
122+
if (error.message === "TimeoutError" || [403, 404, 405].includes(error.statusCode)) {
115123
this.disableFetchToken = true; // as per JSv2 and fromInstanceMetadata implementations
116-
throw new Error(`Error fetching metadata token: ${error}. disableFetchToken is enabled`);
124+
throw new Error(`Error fetching metadata token: ${error}. [disableFetchToken] is now set to true.`);
117125
}
118126
throw new Error(`Error fetching metadata token: ${error}`);
119127
}

0 commit comments

Comments
 (0)