Skip to content

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented Oct 31, 2025

This adds an optional disable_pristine_host_hdr configuration parameter to cookie_remap that, when enabled, disables the
proxy.config.url_remap.pristine_host_hdr setting for matched transactions. This allows the Host header to be updated to match the hostname in the sendto URL, which is useful when downstream routing (such as parent proxies or origin server selection) depends on the Host header value. The feature is off by default to preserve backward compatibility and only affects the sendto path, while the else path continues to use the configured pristine host header setting.

@bneradt bneradt added this to the 10.2.0 milestone Oct 31, 2025
@bneradt bneradt self-assigned this Oct 31, 2025
@zwoop zwoop requested a review from Copilot October 31, 2025 22:33
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 adds a disable_pristine_host_hdr configuration option to the cookie_remap plugin. This allows the Host header to be updated to match the remapped destination when routing based on cookie values, which is useful for downstream routing decisions in production environments where pristine host headers are normally preserved.

  • Added disable_pristine_host_hdr configuration parameter to control Host header behavior on the sendto path
  • Modified the operation processing to track whether the sendto or else path was taken and conditionally disable pristine host headers
  • Comprehensive test coverage with two test scenarios (enabled/disabled) and associated replay files

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugins/experimental/cookie_remap/cookie_remap.cc Implements the disable_pristine_host_hdr feature with getter/setter methods, configuration parsing, and logic to disable pristine host headers on the sendto path
tests/gold_tests/pluginTest/cookie_remap/disable_pristine_host_hdr.test.py Test class that validates the feature with both enabled and disabled configurations
tests/gold_tests/pluginTest/cookie_remap/disable_pristine_host_hdr_true.replay.yaml Replay file verifying Host header updates when feature is enabled
tests/gold_tests/pluginTest/cookie_remap/disable_pristine_host_hdr_false.replay.yaml Replay file verifying Host header preservation when feature is disabled
tests/gold_tests/pluginTest/cookie_remap/configs/disable_pristine_host_hdr_config_true.txt Configuration file with disable_pristine_host_hdr set to true
tests/gold_tests/pluginTest/cookie_remap/configs/disable_pristine_host_hdr_config_false.txt Configuration file with disable_pristine_host_hdr not set (default behavior)
doc/admin-guide/plugins/cookie_remap.en.rst Documentation for the new configuration option with detailed examples

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zwoop
Copy link
Contributor

zwoop commented Oct 31, 2025

I reviewed this, and it looks good. It seems Github Copilot found some typos etc. that should be addressed. For fun, I also ran this PR through Claude, and it produces this ...

PR #12631 Review: Add disable_pristine_host_hdr to cookie_remap

Status: ✅ APPROVE (with minor fix)
Risk: Low

Issues Found

Minor

Assessment

  • ✅ Clean implementation following ATS patterns
  • ✅ Proper use of TSHttpTxnConfigIntSet()
  • ✅ Feature correctly scoped to sendto path only
  • ✅ Backward compatible (disabled by default)
  • ✅ Comprehensive test coverage with correct logic
  • ✅ Good documentation with canary deployment examples

Recommendation: Approve with comment fix suggestion.

This adds an optional disable_pristine_host_hdr configuration parameter
to cookie_remap that, when enabled, disables the
proxy.config.url_remap.pristine_host_hdr setting for matched
transactions. This allows the Host header to be updated to match the
hostname in the sendto URL, which is useful when downstream routing
(such as parent proxies or origin server selection) depends on the Host
header value. The feature is off by default to preserve backward
compatibility and only affects the sendto path, while the else path
continues to use the configured pristine host header setting.
@bneradt bneradt force-pushed the cookie_remap_set_host branch from e6692b2 to 3f5f0fb Compare November 3, 2025 15:41
@bneradt
Copy link
Contributor Author

bneradt commented Nov 3, 2025

I reviewed this, and it looks good. It seems Github Copilot found some typos etc. that should be addressed. For fun, I also ran this PR through Claude, and it produces this ...

Sounds good. Thank you @zwoop. I updated the comment typos.

@bneradt bneradt merged commit 2667e2a into apache:master Nov 4, 2025
15 checks passed
@bneradt bneradt deleted the cookie_remap_set_host branch November 4, 2025 16:53
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.

3 participants