-
-
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
reverseproxy: Implement modular CA provider for TLS transport #6065
reverseproxy: Implement modular CA provider for TLS transport #6065
Conversation
d605170
to
ab39dd4
Compare
Looks like the failed test is just a server timeout. |
ab39dd4
to
c3a6efe
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.
GitHub does not let me comment on the unchanged lines, so excuse the non-inlined comments.
Mark the old way as deprecated and subject to removal in the future, and emit logs saying the same when used. You can get a logger from ctx.Logger()
.
c3a6efe
to
00b1449
Compare
I just added the "deprecated" comments to |
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 doing the rounds, Aziz! Sorry for not giving the review points in full at once. I'm squeezing the review time in tight time slots 😅
We're missing tests and Caddyfile support
No worries at all Mohammed! Thank you for your feedback. I implemented the caddy file support and added both caddy file integration tests as well as tests for the |
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.
Sorry for falling behind -- this is looking better now! I'd be willing to give this a try. Thank you!!
@mohammed90 Anything you can think of we need to change for this? |
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.
A few minor nitpicks and I believe I got everything
f8ba559
to
5e7a4e7
Compare
5e7a4e7
to
391e352
Compare
@mohammed90 I think all your feedback items have been resolved Thanks for reviewing! |
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.
LGTM! Waiting on @mholt review then we're good to go.
Closes #6063