-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
acmeserver: Configurable resolvers
, fix smallstep deprecations
#5500
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.
Looks ok, I've added some minor comments.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no rush on that one. I think we can merge.
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! |
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 for implementing this! Still think it'd be good to implement that optimization at some point.
Closes #5476