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

fileserver: Allow Etag override #5547

Merged
merged 1 commit into from
May 20, 2023

Conversation

charles-dyfis-net
Copy link
Contributor

@charles-dyfis-net charles-dyfis-net commented May 20, 2023

Fixes #5546

Tested with a Caddyfile with a form roughly resembling:

route {
  header {
    ETag "\"${content-hash-here}\""
    -Last-Modified
  }
  root * /content/by-hash/${content-hash-here}
  file_server
}

(with ${content-hash-here} replaced with an actual hash rendered to base-16).

@CLAassistant
Copy link

CLAassistant commented May 20, 2023

CLA assistant check
All committers have signed the CLA.

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.

I'd say this LGTM. Thank you!

@francislavoie francislavoie changed the title If user sets an etag before request reaches staticfiles, do not overwrite it (#5546) fileserver: Allow Etag override May 20, 2023
@francislavoie francislavoie added the feature ⚙️ New feature or request label May 20, 2023
@francislavoie francislavoie added this to the v2.7.0 milestone May 20, 2023
@francislavoie
Copy link
Member

FYI regarding your example, you can use backticks instead of double quotes to avoid needing to escape them. See the docs https://caddyserver.com/docs/caddyfile/concepts#tokens-and-quotes

@emilylange
Copy link
Member

@charles-dyfis-net FYI there is also a slightly different approach in NixOS/nixpkgs#222354

@charles-dyfis-net
Copy link
Contributor Author

@charles-dyfis-net FYI there is also a slightly different approach in NixOS/nixpkgs#222354

This change strikes me as complimentary: if fileserver honors existing ETag values, a plugin can be used to set them so there's a cleaner way to use out-of-tree code to extract an ETag from the filesystem path.

@charles-dyfis-net
Copy link
Contributor Author

charles-dyfis-net commented May 20, 2023

Hmm. There's something I don't understand here.

Going back over the configuration I successfully tested it with, a closer approximation would be:

route {
  root * /content/by-hash/${content-hash-here}
  header ETag `"${content-hash-here}"`
  file_server
  header -Last-Modified
}

Note, two separate header blocks. If I combine them into a single header block as shown above, Etag is no longer set before file_server is invoked.

@mholt
Copy link
Member

mholt commented May 20, 2023

Correct. If you remove a header it defers the operation until the response is written. The two header lines is the correct way to do it in your case where you need one set before the write.

@charles-dyfis-net
Copy link
Contributor Author

Ah! I didn't pay enough attention to where exactly the deferred tag was in the adapt output. Makes perfect sense, then.

@francislavoie
Copy link
Member

francislavoie commented May 20, 2023

Yeah if you use - to delete in the same header block it enables defer mode which makes header operations happen on the way back up the middleware pipeline instead of on the way down, so it runs after the file server.

@mholt
Copy link
Member

mholt commented May 20, 2023

I didn't even notice that in the original config. 😅 (I wonder if we should print a warning in the logs.)

I like to think of it as: add first, delete later.

@charles-dyfis-net
Copy link
Contributor Author

Yup. I saw it in the docs, but read them to be saying that that individual operation would be deferred, as opposed to the whole batch.

@mholt
Copy link
Member

mholt commented May 20, 2023

Gotcha. I'll try to clarify that. That might be a bug though. 🤔 I think I know why it is that way but I'm not sure if that's ideal/intentional. (Anyway, off-topic.)

Is this good to merge then?

@charles-dyfis-net
Copy link
Contributor Author

I'm not aware of anything that should block merge.

@mholt mholt merged commit 2615c9c into caddyserver:master May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETag headers explicitly set in configuration should be honored rather than overridden
5 participants