Skip to content

Add ignore_vary option #2318

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.

Sign up for GitHub

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

Open
DrDaveD wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add ignore_vary option #2318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/HttpHeader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,14 @@ HttpHeader::parse(const char *header_start, size_t hdrLen, Http::ContentLengthIn
}
}

if (Config.onoff.ignore_vary) {
String rawVary;
if (getByIdIfPresent(Http::HdrType::VARY, &rawVary)) {
Copy link
Contributor

@rousskov rousskov Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either this code is missing Http::HdrType::HDR_X_ACCELERATOR_VARY treatment or the new directive documentation is missing a disclaimer about the difference in treatment of those two Vary-like headers (that are treated the same way in many other Squid contexts). The former is more likely.

delById(Http::HdrType::VARY);
Copy link
Contributor

@rousskov rousskov Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, this low-level hack will trigger "no leftovers" assertion in Adaptation::Icap::ModXact::prepEchoing() if ignore_vary is enabled during reconfiguration that happened after the affected response header has been received but before ICAP decided to echo that response.

Most likely, this code should be moved to (i.e. triggered from) higher level code to avoid such difficult-to-find bugs.

debugs(55, 5, "Ignoring Vary: " << rawVary);
}
}

return 1; /* even if no fields where found, it is a valid header */
}

Expand Down
1 change: 1 addition & 0 deletions src/SquidConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ class SquidConfig
#endif

int vary_ignore_expire;
int ignore_vary;
int surrogate_is_remote;
int detect_broken_server_pconns;
int relaxed_header_parser;
Expand Down
18 changes: 18 additions & 0 deletions src/cf.data.pre
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6969,6 +6969,24 @@ DOC_START
varying objects not intended for caching to get cached.
DOC_END

NAME: ignore_vary
COMMENT: on|off
TYPE: onoff
LOC: Config.onoff.ignore_vary
DEFAULT: off
DOC_START
Some HTTP servers send a Vary header even though the clients
never send requests with different request headers. This
option causes Squid to completely ignore the Vary header and
Copy link
Contributor

@rousskov rousskov Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this description misleads the reader into thinking that Squid is just going to ignore the Vary header. In reality, Squid is going to delete that header upon receipt and, hence, never forward it. The header will not even appear in well-known level-2 response header dumps (as if it was never received)!

Also, it may surprise some admins that Squid will send some Vary headers even if this directive is enabled (because Squid adds Vary to some of its generated responses).

Please rephrase to clarify these effects of the proposed directive. Renaming the directive to delete_received_vary or similar may be required.

P.S. AFAICT, the header is going to be counted in Squid named header statistics, arguably contradicting the "completely ignore" promise as well.

go ahead and cache the response using normal rules.
Copy link
Contributor

@rousskov rousskov Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree with the following assertion, please adjust PR description to explain that the proposed ignore_vary on differs from the already supported reply_header_access Vary deny all because the new feature is applied to all responses, while existing reply_header_access directive only applies to responses sent by Squid.


WARNING: Doing this this VIOLATES the HTTP standard. Enabling
Copy link
Contributor

@rousskov rousskov Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add IFDEF: USE_HTTP_VIOLATIONS to the directive metadata then, right?

this can cause varying responses not intended for caching to get
cached. It is intended as a workaround until HTTP/1.1 is fully
implemented and only for use when client behavior is fully
controlled.
DOC_END
Copy link
Contributor

@rousskov rousskov Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also document how the new ignore_vary option interacts with the existing vary_ignore_expire. Warning the admin when both directives are enabled is probably a good idea!


NAME: request_header_access
IFDEF: USE_HTTP_VIOLATIONS
TYPE: http_header_access
Expand Down
Loading