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

Export caddyhttp.Server name field #5528

Closed
jjiang-stripe opened this issue May 11, 2023 · 3 comments · Fixed by #5531
Closed

Export caddyhttp.Server name field #5528

jjiang-stripe opened this issue May 11, 2023 · 3 comments · Fixed by #5531
Labels
feature ⚙️ New feature or request
Milestone

Comments

@jjiang-stripe
Copy link
Contributor

Hello!

I'm writing a middleware that's used across a couple caddyhttp.Servers and it has some behaviour that differs based on the server. It'd be nice to be able to do something like serverNameFromContext, but unfortunately the name field is unexported, so I can't use it in my custom middleware :(

Do y'all see any issues with exporting the name field? If not, I'm happy to make a PR to do so, but figured I'd check in first to make sure I'm not missing anything egregious.

@mholt
Copy link
Member

mholt commented May 11, 2023

Hi there!

I guess we can export it, sure. We already trust the programmer(s) to not change things that aren't intended/supposed to be changed, so we could do that for this and just document it as a read-only field ("pretty please") -- or we could have a getter method, like, ServerName() or GetName() or something like that, but I generally feel like those are clunky.

So yeah, maybe we just export it?

What's the middleware do, just out of curiosity? (At a high level -- since I don't think I've heard of a requirement like this before)

@mholt mholt added the feature ⚙️ New feature or request label May 11, 2023
@mholt mholt added this to the v2.7.0 milestone May 11, 2023
@jjiang-stripe
Copy link
Contributor Author

I'm happy to follow whatever convention we have in Caddy whether that's to export it or add a getter! I actually noticed there's already another getter function in server.go so I went ahead and added a getter instead of exporting (but happy to change if you prefer). PR here: #5531

The middleware doesn't do anything particularly interesting–it just sets some headers based on the client certificate of the request and does some other general authorization checks. We have some Servers that require client authentication and some that don't, so it's nice to be able to do different checks based on the Server. The goal is eventually to require client auth on all our listeners, so it's a bit easier to keep it all in one middleware rather than duplicating code for separate ones.

@mholt
Copy link
Member

mholt commented May 11, 2023

Great, thanks for the PR!

Ok, yeah, sounds interesting -- sounds like a reasonable solution then.

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 a pull request may close this issue.

2 participants