-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
replacer: Implement file.*
global replacements
#5463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Francis -- let's document this as an experimental feature. I'd also like to see some confirmation from someone that this is in fact useful for them and what they need. And yes, I agree we can lean on the kernel to cache files in hot paths and that will probably get us 99% of the way there in terms of peak performance. 👍
Thanks again!
The Docker secrets argument is compelling enough for me. It's a legitimate trend. |
Ok -- if we don't hear from anyone in a couple days, I'd say go ahead and merge it. Thank you! |
0209d24
to
52459de
Compare
But often a misunderstood one since it's not at parity to the swarm feature, it's just a volume mount under the hood without Docker Swarm. However I think it might work properly with Podman and Kubernetes equivalents. Even with plain Docker, so long as the user is aware none of the Docker Swarm features apply, they can manually adjust ownership/permissions for the file before mounting it if needed. |
Wanted to reach out and mention that I applied this PR to a v2.6.4 build and it works well for me! One confusing thing is that this style of matcher didn't work for me:
but this works:
but I'm unsure whether that's expected. |
I don't understand what you're trying to do with that matcher. Please elaborate with more specifics. What does the request look like, and what's in the file? |
What I am doing is configuring a POST webhook with a secret parameter in the URL, e.g. The request I am matching is (using the above example)
I tried this matcher first, however:
and given the same request, Caddy responds with 404. This is unexpected behavior to me, but I am unsure if the |
I found this PR while trying to understand how to best use docker, podman, or kubernetes secrets in a Caddyfile. That turned out to be quite the pain. Having a way to read secrets from a file directly would definitely help. Podman and Kubernetes can add a secret into an environment variable, but that's not recommended as environment variables tend to leak. |
Francis and I talked about this in Slack and while we agree there are use cases for this, we want to be sure we're careful since this currently is not jailed to the site root, and we're not always confident that placeholders come from trusted sources (though they should). For example, templates can invoke placeholders, and we're not sure that templates are all always trusted (i.e. if a site allows users to customize templates). I might defer this a little later until we can figure out a safe mechanism for this. |
d8cd857
to
6153304
Compare
Took me a while to think about a good way to make it safe. I think a "good enough" approach is to provide a way for specific modules to opt-out of the file placeholders. The simplest way I could think of doing it is by prefixing the providers with one that rejects file placeholders before it reaches the existing one. I wanted to filter out the
|
I don't know what's the design you had in mind, but maybe you can work around needing to compare functions by interface-asserting into single-method interfaces and branch that way? |
a88151f
to
a58d1d7
Compare
a58d1d7
to
df99aeb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change since I'm not 100% committed to this feature/approach, but we can give it a try. :) Thanks
// WithoutFile returns a copy of the current Replacer | ||
// without support for the {file.*} placeholder, which | ||
// may be unsafe in some contexts. | ||
func (r *Replacer) WithoutFile() *Replacer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark this as Experimental. I'm still nervous 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. But I think it's less-so this function needing to be experimental, cause this is actually the opt-out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I mostly am just worried about compatibility promises in the very off-chance this is used externally 😆
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
Co-authored-by: Mohammed Al Sahaf <msaa1990@gmail.com>
df99aeb
to
877220c
Compare
Looking forward to use this feature! Thanks! I tried to reconstruct a Caddyfile example from this to understand how I would use such a replacement in my own configuration, but it's a little too dense for a user to see right away. I hope you'll find the time to add an example to the docs somewhere, that would be great! |
It's just this: |
Closes #5374