fix(imds): close response body in HealthCheck and New

[abtreece]

Summary

• Fixes HTTP response body leak in IMDS HealthCheck method
• Fixes HTTP response body leak in IMDS New (client creation) validation check
• Adds tests to verify response bodies are properly closed

Details

Previously, GetMetadata response bodies were not closed in two places:

1. New() - validation check during client creation (line 114)
2. HealthCheck() - basic health check method (line 335)

This is inconsistent with other methods in the same file (HealthCheckDetailed, getMetadata) which properly close response bodies. The leak could cause HTTP connection and file descriptor exhaustion over time, especially when health checks run frequently.

Changes

pkg/backends/imds/client.go:

• Added output.Content.Close() call in New() after validation check
• Added output.Content.Close() call in HealthCheck() after successful check

pkg/backends/imds/client_test.go:

• Added trackingReadCloser helper to track Close() calls
• Added mockResponseWithCloseTracking() helper for creating trackable mock responses
• Added TestHealthCheck_ClosesResponseBody test
• Added TestHealthCheckDetailed_ClosesResponseBody test

Test plan

• All existing IMDS unit tests pass
• New response body close verification tests pass
• Full test suite passes

Fixes #508

[Copilot]

Pull request overview

This PR fixes HTTP response body leaks in the IMDS client by ensuring response bodies are properly closed in two methods that were previously missing these cleanup calls, preventing potential connection and file descriptor exhaustion.

Changes:

• Added response body closure in New() validation check and HealthCheck() method
• Added comprehensive tests to verify response bodies are closed correctly
• Introduced helper utilities for tracking Close() calls in tests

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/backends/imds/client.go	Added Close() calls for response bodies in New() and HealthCheck() methods
pkg/backends/imds/client_test.go	Added test infrastructure and tests to verify response body closure

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The error message uses %v format verb in log.Debug which expects format arguments, but log.Debug likely doesn't support format arguments. Either use fmt.Sprintf to format the message, or pass closeErr as a separate argument if log.Debug supports it.

Suggested change

	log.Debug("Failed to close IMDS validation response: %v", closeErr)
	log.Debug(fmt.Sprintf("Failed to close IMDS validation response: %v", closeErr))

[Copilot]

The error message uses %v format verb in log.Debug which expects format arguments, but log.Debug likely doesn't support format arguments. Either use fmt.Sprintf to format the message, or pass closeErr as a separate argument if log.Debug supports it.

[codecov]

Codecov Report

❌ Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.10%. Comparing base (68bb80f) to head (20cf92a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/backends/imds/client.go	37.50%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
+ Coverage   62.00%   62.10%   +0.09%     
==========================================
  Files          48       48              
  Lines        5393     5399       +6     
==========================================
+ Hits         3344     3353       +9     
+ Misses       1842     1837       -5     
- Partials      207      209       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[abtreece]

Reviewed. The response bodies are now closed in and , and the new tests validate closure behavior. Looks correct and addresses #508. Optional: add a test covering the validation close path for full coverage.

[abtreece]

Review Response

Copilot comments (Lines 121 & 346)

These are false positives. The log.Debug function does support format arguments - it internally calls fmt.Sprintf(format, v...):

// From pkg/log/log.go:182-184
func Debug(format string, v ...interface{}) {
    logger.Debug(fmt.Sprintf(format, v...))
}

This pattern is used consistently throughout the codebase (see lines 148, 249, 276, 298, 318, 371 in the same file).

@abtreece's suggestion (test for New() close path)

The New() function creates a real AWS IMDS client and validates IMDS availability, making it difficult to unit test without:

1. Running on an actual EC2 instance, or
2. Significant refactoring to inject the IMDS client interface into New()

The close path in New() follows the exact same pattern as the tested HealthCheck close path. Since this is optional and the fix is straightforward, I'd suggest accepting the current coverage and relying on integration tests for the New() path.

Let me know if you'd prefer I pursue the refactoring approach for full coverage.

[abtreece]

Added test coverage for New() validation path

Pushed commit 20cf92a which adds test coverage for the newWithClient validation logic:

Changes

Refactored New(): Extracted client initialization into newWithClient(ctx, client, cacheTTL) to allow testing with mock clients. New() now creates the real IMDS client and delegates to newWithClient().

Added tests:

• TestNewWithClient_Success: Verifies successful client creation and that the validation response body is closed
• TestNewWithClient_IMDSUnavailable: Verifies proper error handling when IMDS is unavailable

This addresses the optional coverage suggestion while maintaining the existing public API.