acmeserver: Configurable resolvers, fix smallstep deprecations
#5500
Conversation
| acmeCtx := acme.NewContext( | ||
| r.Context(), | ||
| ash.acmeDB, | ||
| ash.acmeClient, | ||
| ash.acmeLinker, | ||
| nil, | ||
| ) | ||
| acmeCtx = authority.NewContext(acmeCtx, ash.acmeAuth) | ||
| r = r.WithContext(acmeCtx) |
There was a problem hiding this comment.
This is ok. You're doing this for every request, and it should be fine. A minor optimization would be to build it once and move it to the server BaseContext, this is what we do. But it will be available everywhere.
Looking at chi, they have a middleware that we've never used named ServerBaseContext. It allows you to define a base context for a handler, and it adds the context added by net/http. I don't think it's going to make an impact on the performance, but you can test and find out.
There was a problem hiding this comment.
Right, I was trying to figure out how to do this. Thanks for pointing to that chi API, I've never used chi before. The ACME server is definitely not a hot path for most users, so it's probably not a big deal. Context wrapping is pretty cheap in general I think.
There was a problem hiding this comment.
@francislavoie Do you want me to hold up on merging until we make that optimization? Or should we go with this and then optimize later?
There was a problem hiding this comment.
There's no rush on that one. I think we can merge.
francislavoie
force-pushed
the
acme-server-resolvers
branch
from
432602e
to
6de83df
Compare
Co-authored-by: itsxaos <33079230+itsxaos@users.noreply.github.com>
|
Changes look good to me, but it will be a couple of hours until I can test them. |
|
Alright, I tried the changes and everything seems to be working as expected! |
Closes #5476