-
Notifications
You must be signed in to change notification settings - Fork 3
feat(spoe): Migrate implementation to dropmorepackets/haproxy-go #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Replace github.com/negasus/haproxy-spoe-go with github.com/dropmorepackets/haproxy-go - Implement zero-allocation message parsing with single-pass KV extraction - Add struct pooling for HTTPMessageData and IPMessageData with embedded buffers - Use bytes.Equal for key matching to avoid string allocations - Update all action setting to use encoding.ActionWriter - Optimize buffer reuse by embedding buffers in pooled structs - Remove unused code and simplify pooling strategy
fe9c247 to
c43c867
Compare
- Remove ctx field from Spoa struct (containedctx) - Use context parameter instead of context.Background() (contextcheck) - Add type assertion checks with ok for all type assertions (revive) - Prefix unused ctx parameters with _ (unparam) - Remove unused readKeyFromMessage function (unused)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the SPOE (Stream Processing Offload Engine) implementation from github.com/negasus/haproxy-spoe-go to github.com/dropmorepackets/haproxy-go, introducing significant performance optimizations through zero-allocation message parsing, struct pooling with embedded buffers, and efficient key matching using byte comparisons.
Key Changes:
- Replace SPOE library dependency and update all integration points to use the new API (encoding.ActionWriter, encoding.Message)
- Implement struct pooling for HTTPMessageData and IPMessageData with embedded buffer reuse to reduce GC pressure
- Optimize message parsing with single-pass KV extraction using bytes.Equal for key matching instead of string conversions
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/spoa/root.go | Core migration: replaces handler wrapper with HandleSPOE interface, implements struct pooling, adds zero-allocation message parsing with extractHTTPMessageData and extractIPMessageData functions |
| internal/remediation/captcha/root.go | Updates InjectKeyValues to use encoding.ActionWriter instead of action.Actions |
| internal/remediation/ban/root.go | Updates InjectKeyValues to use encoding.ActionWriter instead of action.Actions |
| go.mod | Replaces negasus/haproxy-spoe-go v1.0.7 with dropmorepackets/haproxy-go v0.0.7 |
| go.sum | Updates dependency checksums for library migration |
Comments suppressed due to low confidence (2)
pkg/spoa/root.go:723
- Missing panic check on type assertion. If
storedURLValis not astring, this will panic. Add a check:storedURL, ok := storedURLVal.(string); if !ok { return remediation.Allow, httpData }or use a two-value type assertion to handle unexpected types gracefully.
"session": uuid,
pkg/spoa/root.go:905
- The
readKeyFromMessagefunction (lines 869-905) and its associated error variables (lines 865-867) appear to be unused. The new implementation usesextractHTTPMessageDataandextractIPMessageDatafor extracting message data. Per the PR description "Remove unused code and simplify pooling strategy", this function should be removed.
ipTypeLabel := "ipv4"
if ipAddr.Is6() {
ipTypeLabel = "ipv6"
}
// Count processed requests - use WithLabelValues to avoid map allocation on hot path
metrics.TotalProcessedRequests.WithLabelValues(ipTypeLabel).Inc()
// Check IP directly against dataset
r, origin := s.getIPRemediation(ctx, writer, ipAddr)
// Count blocked requests
if r > remediation.Unknown {
// Label order: origin, ip_type, remediation (as defined in metrics.go)
metrics.TotalBlockedRequests.WithLabelValues(origin, ipTypeLabel, r.String()).Inc()
}
_ = writer.SetString(encoding.VarScopeTransaction, "remediation", r.String())
}
var (
ErrMessageKeyNotFound = errors.New("message key not found")
ErrMessageKeyTypeMismatch = errors.New("message key type mismatch")
)
func readHeaders(headers []byte) (http.Header, error) {
h := http.Header{}
if len(headers) == 0 {
return nil, fmt.Errorf("no headers found")
}
// Split by \r\n
hs := strings.Split(string(headers), "\r\n")
for _, header := range hs {
if header == "" {
continue
}
kv := strings.SplitN(header, ":", 2)
if len(kv) != 2 {
return nil, fmt.Errorf("invalid header: %q", header)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix double-free bug: remove Put(nil) call when extractIPMessageData returns error - Add max buffer size limits (64KB headers, 512KB body) to prevent unbounded memory growth - Remove redundant clear() calls before copy() operations - All type assertions already have proper ok checks - Context usage already correct (using ctx parameter, not context.Background()) - Unused ctx field already removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove ErrMessageKeyNotFound and ErrMessageKeyTypeMismatch as they are no longer used after migration - These were left over from the old readKeyFromMessage function that was removed
- Add reset() method to IPMessageData struct, matching HTTPMessageData pattern - Update extractIPMessageData to use reset() instead of manual field clearing - Update handleIPRequest defer to use reset() method - Improves code consistency and maintainability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…onversion - Use bytes.Split() instead of converting entire headers byte slice to string - Use bytes.IndexByte() to find colon separator directly in byte slice - Only convert individual key/value parts to strings when needed - Reduces allocations, especially for requests with large header blocks
- Replace bytes.Split with bytes.SplitSeq to avoid allocating intermediate slice - SplitSeq uses iterator pattern, yielding lines one at a time - Reduces memory allocations when parsing headers - Addresses linting suggestion for more efficient iteration
- Remove HTTPRequestData struct to fix memory safety issue with pooled buffers - Eliminates risk of holding pointers to pooled slices that could be reused - Simplifies code since all values are extracted upfront in msgData - handleCaptchaRemediation now only returns remediation (not HTTPRequestData) - Fix misleading comment about URL nil check (line 666) - Clarify that msgData.URL is guaranteed non-nil at that point - Fix buffer reallocation edge case for headers and body buffers - Change > to >= to properly handle capacity exactly equal to max size - Prevents wasting pooled buffers for common boundary cases
Running the stress test and capturing pprof profiles for comparison:
Test: 50,000 requests, 100 concurrent connections