Skip to content

Conversation

@LaurenceJJones
Copy link
Member

@LaurenceJJones LaurenceJJones commented Dec 4, 2025

  • 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
    Running the stress test and capturing pprof profiles for comparison:

Test: 50,000 requests, 100 concurrent connections

Metric OLD Package NEW Package Improvement
Heap Allocated 85.63 MB 87.82 MB +2.19 MB
GC Sys Bytes 5.93 MB 5.54 MB 6.6% less
Heap Objects 1,472,913 1,260,000 14.4% less

- 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
- 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)
Copilot finished reviewing on behalf of LaurenceJJones December 4, 2025 11:38
Copy link
Contributor

Copilot AI left a 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 storedURLVal is not a string, 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 readKeyFromMessage function (lines 869-905) and its associated error variables (lines 865-867) appear to be unused. The new implementation uses extractHTTPMessageData and extractIPMessageData for 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
Copy link
Contributor

Copilot AI left a 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
Copy link
Contributor

Copilot AI left a 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
@LaurenceJJones LaurenceJJones added this to the 0.3.0 milestone Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants