x-pack/filebeat/input/streaming: apply configured timeout to OAuth2 token requests#50492
Conversation
efd6
added
Filebeat
botelastic
Bot
added
needs_team
🤖 GitHub commentsJust comment with:
|
|
/test |
1 similar comment
|
/test |
TL;DRThe failing Buildkite job is caused by a timing-sensitive unit test, not an implementation regression: Remediation
Investigation detailsRoot Cause
Admission control rejects only when Evidence
Verification
Follow-upIf you want to harden this further, mirror the deterministic pattern already used in Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | noneWhat is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a changelog fragment documenting a bug fix for CrowdStrike streaming input in Filebeat. Implements per-request timeout handling in the CrowdStrike rate-limit transport and wires the configured auth client timeout into follower initialization. Adds a unit test that verifies OAuth2 token requests respect the configured timeout and updates an existing HTTP endpoint test to wait on an in-flight counter instead of using a fixed sleep. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x-pack/filebeat/input/http_endpoint/handler_test.go (1)
660-660:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPipeline failing: missing
//nolint:errcheckon existing(*handler)type assertions.Line 846 (new code) has the nolint comment, but the identical pattern at line 660 (and lines 684, 709, 737, 763, 791) does not, causing the CI
errcheckfailure. Add the same comment to all of them.📝 Example fix (apply to all occurrences)
- handler := newHandler(ctx, c, nil, pub.Publish, nil, logp.NewLogger("test"), metrics).(*handler) + handler := newHandler(ctx, c, nil, pub.Publish, nil, logp.NewLogger("test"), metrics).(*handler) //nolint:errcheck // newHandler is statically known to return a *handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x-pack/filebeat/input/http_endpoint/handler_test.go` at line 660, The CI errcheck failure is caused by unchecked errors in the type assertion using "(*handler)" after calls to newHandler; update each occurrence (the call pattern "handler := newHandler(ctx, c, nil, pub.Publish, nil, logp.NewLogger(\"test\"), metrics).(*handler)" at lines like 660, 684, 709, 737, 763, 791) to append the nolint directive for errcheck (i.e., add "//nolint:errcheck" to the same line) so the type assertion is exempted from the errcheck linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog/fragments/1777958918-49988-streaming.yaml`:
- Line 1: Replace the YAML entry key value "kind: feature" with "kind: bug-fix"
so the changelog fragment correctly classifies this PR as a bug fix; locate the
fragment that currently contains the string "kind: feature" and update it to
"kind: bug-fix" (ensure the rest of the YAML formatting remains unchanged).
---
Outside diff comments:
In `@x-pack/filebeat/input/http_endpoint/handler_test.go`:
- Line 660: The CI errcheck failure is caused by unchecked errors in the type
assertion using "(*handler)" after calls to newHandler; update each occurrence
(the call pattern "handler := newHandler(ctx, c, nil, pub.Publish, nil,
logp.NewLogger(\"test\"), metrics).(*handler)" at lines like 660, 684, 709, 737,
763, 791) to append the nolint directive for errcheck (i.e., add
"//nolint:errcheck" to the same line) so the type assertion is exempted from the
errcheck linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f847eddd-d0a3-45f1-96df-3a2d5c4ea346
📒 Files selected for processing (5)
changelog/fragments/1777958918-49988-streaming.yamlx-pack/filebeat/input/http_endpoint/handler_test.gox-pack/filebeat/input/streaming/crowdstrike.gox-pack/filebeat/input/streaming/crowdstrike_ratelimit.gox-pack/filebeat/input/streaming/crowdstrike_test.go
…oken requests The CrowdStrike FalconHose input constructed the OAuth2 HTTP client with only the Transport from the auth client, dropping the Timeout set by httpcommon.HTTPTransportSettings. Token endpoint requests could hang indefinitely instead of respecting the configured timeout. Apply the timeout via context.WithTimeout in rateLimitTransport rather than http.Client.Timeout to avoid the deprecated oauth2.Transport.CancelRequest path. Also fix flaky test.
…oken requests (#50492) (#50533) The CrowdStrike FalconHose input constructed the OAuth2 HTTP client with only the Transport from the auth client, dropping the Timeout set by httpcommon.HTTPTransportSettings. Token endpoint requests could hang indefinitely instead of respecting the configured timeout. Apply the timeout via context.WithTimeout in rateLimitTransport rather than http.Client.Timeout to avoid the deprecated oauth2.Transport.CancelRequest path. Also fix flaky test. (cherry picked from commit fd8ae04) Co-authored-by: Dan Kortschak <dan.kortschak@elastic.co>
…oken requests (#50492) (#50531) The CrowdStrike FalconHose input constructed the OAuth2 HTTP client with only the Transport from the auth client, dropping the Timeout set by httpcommon.HTTPTransportSettings. Token endpoint requests could hang indefinitely instead of respecting the configured timeout. Apply the timeout via context.WithTimeout in rateLimitTransport rather than http.Client.Timeout to avoid the deprecated oauth2.Transport.CancelRequest path. Also fix flaky test. (cherry picked from commit fd8ae04) Co-authored-by: Dan Kortschak <dan.kortschak@elastic.co>
…oken requests (#50492) (#50532) The CrowdStrike FalconHose input constructed the OAuth2 HTTP client with only the Transport from the auth client, dropping the Timeout set by httpcommon.HTTPTransportSettings. Token endpoint requests could hang indefinitely instead of respecting the configured timeout. Apply the timeout via context.WithTimeout in rateLimitTransport rather than http.Client.Timeout to avoid the deprecated oauth2.Transport.CancelRequest path. Also fix flaky test. (cherry picked from commit fd8ae04) Co-authored-by: Dan Kortschak <dan.kortschak@elastic.co>
2 participants
Proposed commit message
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
How to test this PR locally
Related issues
Use cases
Screenshots
Logs