[refactor-opportunist] Consolidate Filebeat HTTP redirect policy into a shared internal helper
- Dominant language
- Go
- Stars
- 12.7k
- Forks
- 5k
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 370
Description
## 🏗️ Refactor Proposal
**Summary:** Centralize duplicated HTTP redirect-header policy used by x-pack Filebeat inputs into one internal helper to reduce drift and make future security/behavior updates single-touch.
## Problem
`x-pack/filebeat/input/cel/input.go` and `x-pack/filebeat/input/httpjson/input.go` both implemented near-identical redirect behavior (`checkRedirect`) and tests, including max-redirect enforcement, previous-header forwarding, cross-origin sensitive-header stripping, and banned-header removal.
Concrete duplication evidence:
- `x-pack/filebeat/input/cel/input.go:1406` (local `checkRedirect` implementation before POC)
- `x-pack/filebeat/input/httpjson/input.go:460` (local `checkRedirect` implementation before POC)
- Duplicate test logic in:
- `x-pack/filebeat/input/cel/check_redirect_test.go:15`
- `x-pack/filebeat/input/httpjson/check_redirect_test.go:15`
Churn/co-change signal from the last 60 days showed both files repeatedly changing during related HTTP behavior/security work (including PR #51434 and backports), which increases the risk of divergence when logic remains duplicated.
## Proposed Approach
Extract shared redirect policy into a single internal utility under `x-pack/filebeat/input/internal/` and make input packages delegate to it.
What changes:
- Shared logic for redirect handling (max redirects, header forwarding, cross-origin stripping, banlist filtering) lives in one place.
What stays the same:
- Existing input-specific config structs (`ResourceConfig` and `requestConfig`) remain unchanged.
- Existing call sites keep package-local `checkRedirect` wrappers to preserve package API/usage.
Why this structure is better:
- Single maintenance point for security-sensitive behavior.
- Lower drift risk between CEL and HTTPJSON.
- Incremental migration path (other inputs can adopt the helper later).
## Proof of Concept
I partially implemented this refactor on one representative slice to verify viability:
**Files changed:**
- `x-pack/filebeat/input/internal/httpclientutil/redirect.go` (new)
- `x-pack/filebeat/input/cel/input.go`
- `x-pack/filebeat/input/httpjson/input.go`
**Before → After:**
- Before: each package had its own ~35-line `checkRedirect` implementation.
- After: both package-level wrappers delegate to:
- `httpclientutil.CheckRedirect(...)` at `x-pack/filebeat/input/internal/httpclientutil/redirect.go:16`
- Updated wrappers:
- `x-pack/filebeat/input/cel/input.go:1406`
- `x-pack/filebeat/input/httpjson/input.go:460`
**Verification:**
- `go test ./x-pack/filebeat/input/cel ./x-pack/filebeat/input/httpjson -run TestCheckRedirectSensitiveHeaders -count=1`
- Result: both packages passed.
## Incremental Rollout Plan
This refactor can be completed incrementally:
1. **Completed POC:** CEL + HTTPJSON redirect policy delegated to shared helper.
2. Migrate additional redirect implementations to the same helper where semantics match (e.g., entityanalytics providers with compatible config).
3. Consolidate duplicated redirect tests into shared table-driven coverage for the helper plus thin package-level smoke tests.
## Risks and Mitigations
- **Risk 1:** Subtle behavior differences across inputs may be unintentionally unified.
- **Mitigation:** Keep package-local wrappers and migrate one input at a time with existing tests retained.
- **Risk 2:** Future input-specific needs could cause helper over-generalization.
- **Mitigation:** Keep helper narrowly scoped to redirect policy and pass explicit parameters instead of broad config types.
## Evidence
- Duplicate implementations observed in the two input packages and duplicate redirect tests (paths above).
- Recent related PR history includes `https://github.com/elastic/beats/pull/51434` and backports touching the same behavior across both inputs.
- Prior refactor-opportunist issues reviewed in `/tmp/previous-findings.json`; closest open entries (#49602, #49783) are narrower auth-helper extractions and do not cover redirect policy consolidation.
---
[What is this?](https://ela.st/github-ai-tools) | [From workflow: Refactor Opportunist](https://github.com/elastic/beats/actions/runs/32037966437)
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
Contributor guide
Research direction
Start with x-pack/filebeat/input/internal/httpclientutil/redirect.go and the wrappers in x-pack/filebeat/input/cel/input.go and x-pack/filebeat/input/httpjson/input.go. Run the existing CEL and HTTPJSON TestCheckRedirectSensitiveHeaders command, then inspect entityanalytics redirect implementations and the duplicate check_redirect_test.go files for compatible behavior. Done means compatible inputs use the shared helper, shared coverage exists, and package smoke tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, security
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 65/100