Skip to content

[bugfix] apply the configured timeout to SSL certificate collection - #4492

Open
SiriusTL666 wants to merge 1 commit into
apache:masterfrom
SiriusTL666:fix/ssl-cert-collect-timeout
Open

SiriusTL666 wants to merge 1 commit into
apache:masterfrom
SiriusTL666:fix/ssl-cert-collect-timeout

Conversation

@SiriusTL666

Copy link
Copy Markdown

What's changed?

SslCertificateCollectImpl opened an HttpsURLConnection and called connect() without setting any connect or read timeout. When the peer blackholes the TLS handshake — a firewalled host, a dead load balancer, a silently dropping middlebox — the call blocks until the OS-level TCP timeout, which is minutes rather than seconds. The collection task holds its worker for that whole time while the monitor shows NO DATA, and this is one of the ways a "no data" state stays stuck far longer than the configured collection interval.

Changes

  1. Bound both phases. Apply HttpProtocol.timeout to setConnectTimeout and setReadTimeout, defaulting to 10s when unset.
  2. Make it configurable. app-ssl_cert.yml previously exposed host, port, verify and uri only. It now has an optional timeout parameter (range 1000–120000 ms, default 10000), wired into the ssl_cert protocol block.
  3. Test. collectHonoursConfiguredTimeout points the collector at TEST-NET-1 (192.0.2.1, reserved by RFC 5737 and unroutable) with an 800ms timeout and asserts the call returns well inside it and reports a failure rather than rows.

Verification

The new test is a genuine regression test — I confirmed both directions:

Code Elapsed
Without setConnectTimeout / setReadTimeout 21086 ms (test fails)
With the fix 1.77 ms… 1.7 s (test passes)
  • mvn -pl hertzbeat-collector/hertzbeat-collector-basic test -Dtest=SslCertificateCollectImplTest — 3/3 pass.
  • checkstyle:check on the module — 0 violations.

The two pre-existing tests in this class (self-signed cert with a mismatched SAN, both verify on and off) still pass, so the change does not alter certificate parsing.

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

No related issue exists for this one — I found it while auditing the collector modules for unbounded network waits. Happy to file an issue first if the project prefers that; let me know.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

No HTTP API change. The timeout monitor parameter is optional and defaults to 10s.

SslCertificateCollectImpl called HttpsURLConnection.connect() without setting
connect or read timeouts, so collecting a certificate from a blackholed or
silently dropping peer blocked until the OS-level TCP timeout. In the test
below that is 21 seconds, and in the field it can be minutes; meanwhile the
collection task occupies a worker and the monitor shows NO DATA.

- Apply HttpProtocol.timeout (default 10s) to both setConnectTimeout and
  setReadTimeout.
- Expose an optional timeout parameter in app-ssl_cert.yml.
- Add collectHonoursConfiguredTimeout, which points at TEST-NET-1 (RFC 5737)
  and asserts the call returns well inside the timeout. Verified to fail
  without the fix (21086ms) and pass with it (~1.7s).
@SiriusTL666

Copy link
Copy Markdown
Author

Hi maintainers, the workflows on this PR are showing action_required because this is my first contribution to this repository. Could someone approve the workflow runs when convenient? Happy to address any review comments in the meantime.

For context: linked issue is #3197, and I checked before starting that no IPMI fix PR existed. I left a note on the issue that I would step back if either of the earlier commenters is already working on it — happy to coordinate.

@SiriusTL666

Copy link
Copy Markdown
Author

While waiting for a maintainer to approve the workflows on this PR (this is my first contribution here, so they show action_required), I enabled Actions in my fork and ran the same Backend CI workflow there against this exact commit range. It passes:

https://github.com/SiriusTL666/hertzbeat/actions/runs/40178308108

Job Result
backend-build success
backend-maven-e2e success
backend-image-e2e success

That covers the Maven build, the frontend release-asset build, and the server release-layout verification, so the change compiles and the module tests pass on the project's own CI definition.

To be explicit: this is a run in my fork, not on this PR — it is evidence for the reviewer, not a substitute for the required approval. Once someone approves, the run on this PR should behave the same.

Also worth noting: in the fork run I had to touch a real (non-markdown) file to trigger the push-based workflow, since paths-ignore skips commits with no relevant changes. The PR branch itself is unchanged and clean.

Happy to address any review comments.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant