gcp-resources: add connect and read timeout, add response body limit#2883
gcp-resources: add connect and read timeout, add response body limit#2883jack-berg wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds explicit connect and read timeouts to the GCP metadata HTTP fetch and bounds the response body size read from the metadata server to 4096 characters, replacing the prior BufferedReader.readLine() call with a fixed-size character read.
Changes:
- Introduce
CONNECT_TIMEOUT_MS(1s) andREAD_TIMEOUT_MS(1s) constants and apply them to theHttpURLConnection. - Cap metadata response reads at
MAX_METADATA_VALUE_CHARS(4096) via a singleReader.readinto a char buffer. - Add a unit test asserting an oversized metadata body is truncated to 4096 characters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| gcp-resources/src/main/java/io/opentelemetry/contrib/gcp/resource/GcpMetadataConfig.java | Adds connect/read timeouts and replaces line-based read with bounded char-buffer read. |
| gcp-resources/src/test/java/io/opentelemetry/contrib/gcp/resource/GcpMetadataConfigTest.java | Adds test verifying 8192-char response is truncated to 4096 chars. |
| char[] buffer = new char[MAX_METADATA_VALUE_CHARS]; | ||
| int n = reader.read(buffer, 0, buffer.length); | ||
| return n > 0 ? new String(buffer, 0, n) : null; |
| int n = reader.read(buffer, 0, buffer.length); | ||
| return n > 0 ? new String(buffer, 0, n) : null; |
psx95
left a comment
psx95
left a comment
There was a problem hiding this comment.
Hi @jack-berg ,
Thanks for the PR, adding a connect and read timeout makes sense. I'm just wondering if something specific prompted this change.
| return n > 0 ? new String(buffer, 0, n) : null; | ||
| } | ||
| } | ||
| } catch (IOException ignore) { |
There was a problem hiding this comment.
When adding the read connect timeouts, I think the catch block should explicitly check for SocketTimeoutException and log a warning.
Might be better to place the connection.getResponseCode() and connection.getInputStream() in individual try catch blocks to explicitly log if it was a read timeout or connect timeout.
I'm hoping if this becomes a frequent issue, the users can inform us about the exact cause.
| try (BufferedReader reader = | ||
| new BufferedReader(new InputStreamReader(input, StandardCharsets.UTF_8))) { | ||
| return reader.readLine(); | ||
| char[] buffer = new char[MAX_METADATA_VALUE_CHARS]; |
There was a problem hiding this comment.
Not sure if this required. Would readLine really be that inefficient?
There was a problem hiding this comment.
This is all about limiting the size of the payload read into memory in the event the server is malicious. I don't think readLine would impose any limits if there's a huge payload all on one line.
Its unlikely, since we're connecting to google servers, but its just a bit more cautious. (I don't think there's technically anything that guarantees that the server at http://metadata.google.internal/computeMetadata/v1/ is googles in the case that the network / DNS has been compromised.
| private static final int CONNECT_TIMEOUT_MS = 1_000; | ||
| private static final int READ_TIMEOUT_MS = 1_000; |
There was a problem hiding this comment.
While I think these timeouts make sense (since we are only doing simple gets), I am wondering if it would be better to expose them as optional configuration.
| static final GcpMetadataConfig DEFAULT_INSTANCE = new GcpMetadataConfig(); | ||
|
|
||
| private static final String DEFAULT_URL = "http://metadata.google.internal/computeMetadata/v1/"; | ||
| // google-auth-library-java uses 500 ms connect timeout (with up to 3 retries) for its metadata |
There was a problem hiding this comment.
Question: Why does auth-library matter here? It does not seem to be a dependency of this component.
There was a problem hiding this comment.
Its a reference for another library that interacts with these same APIs to inform the decisions here
No description provided.