Skip to content
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

header: match subdirective for response matching #6765

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Jan 4, 2025

Allows the header directive to specify a response matcher to apply the header operation only when the response has a certain header or status.

This PR only adds the syntax to the caddyfile parser. The runtime uses the preexisting logic implemented for the "Require" field. I went with the match name for parity with the encode directive.

There is a slight breaking concern: similar to how "defer" cannot be used a header name, a header entry where the header name is exactly "match" will no longer be considered. If there is someone using it, they will have to rename the header to "Match" (different case) instead.

Syntax example:

example.com {
	encode gzip {
		match {
			header Content-Type application/json*
		}
	}
	header Cache-Control "public, max-age=3600" {
		match {
			status 2xx
		}
	}
	file_server
}

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2025

CLA assistant check
All committers have signed the CLA.

@lilnasy lilnasy changed the title feat(caddyfile: match block for header directive feat(caddyfile): match block for header directive Jan 4, 2025
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

There is a slight breaking concern: similar to how "defer" cannot be used a header name, a header entry where the header name is exactly "match" will no longer be considered. If there is someone using it, they will have to rename the header to "Match" (different case) instead.

I believe this is only true when using a block; but when using the single-line syntax, there should be no conflict. IIRC.


Anyway, LGTM at a glance! Maybe @francislavoie could double-check but I think we can merge this.

@mholt mholt added this to the v2.9.1 milestone Jan 7, 2025
@mholt
Copy link
Member

mholt commented Jan 7, 2025

j/k -- we need to fix the lint error first:

Error: modules/caddyhttp/headers/caddyfile.go:104:39: Error return value of caddyhttp.ParseNamedResponseMatcher is not checked (errcheck)
caddyhttp.ParseNamedResponseMatcher(h.NewFromNextSegment(), responseMatchers)

@francislavoie francislavoie changed the title feat(caddyfile): match block for header directive header: match subdirective for response matching Jan 8, 2025
@mholt mholt enabled auto-merge (squash) January 8, 2025 05:20
@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 8, 2025

Thanks for the reviews! Seem like it just needs a final round tests after rebasing on top of the recent pushes.

auto-merge was automatically disabled January 8, 2025 05:25

Head branch was pushed to by a user without write access

@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 8, 2025

@mholt Could you approve the workflow run?

@mholt mholt merged commit e48b758 into caddyserver:master Jan 8, 2025
33 checks passed
@lilnasy lilnasy deleted the header-match branch January 8, 2025 05:49
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.

4 participants