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.

Sign up for GitHub

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

Jump to bottom

reverseproxy: fix breakage in handling SRV lookup introduced by d55d50b #3756

Merged
merged 7 commits into from
Oct 1, 2020
Merged

reverseproxy: fix breakage in handling SRV lookup introduced by d55d50b #3756

merged 7 commits into from
Oct 1, 2020

Conversation

mohammed90
Copy link
Member

@mohammed90 mohammed90 commented Sep 26, 2020 •
edited
Loading

This is the naive fix. I still need to test it some more and figure whether this fix is the best.
@waiyip-aquabyte and @danlsgiga, is it possible to test using the CI artifacts? You can find them here.

Fixes #3753

@mohammed90 mohammed90 added in progress 🏃‍♂️ Being actively worked on do not merge ⛔ Not ready yet! labels Sep 26, 2020
mholt
mholt approved these changes Sep 26, 2020
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for making time to help fix this!

FWIW, I think it looks fine; I'd have probably done something similar. Since health checks don't make sense on srv upstreams, should we take this opportunity to make it an error?

@danlsgiga
Copy link
Contributor

Awesome @mohammed90, I validated the build and it fixes the issue!

@waiyip-aquabyte
Copy link

Thank you so much. What is the release process? I really want to upgrade because of Prometheus metrics.

@danlsgiga
Copy link
Contributor

@mholt is it possible to have this into a 2.2.1 release whenever its merged?

mholt
mholt previously approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@mholt mholt left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix LGTM! Are we comfortable merging it then? Will wait for the do not merge and in progress tags to be removed.

@mholt mholt added the under review 🧐 Review is pending before merging label Sep 28, 2020
@mholt mholt added this to the v2.2.1 milestone Sep 28, 2020
@mohammed90
Copy link
Member Author

This fix LGTM! Are we comfortable merging it then? Will wait for the do not merge and in progress tags to be removed.

Just want to add a few validations for incompatible config and few integration tests. I should be able to shoot them later tonight.

@mohammed90 mohammed90 changed the title reverseproxy: fix breakage in handling SRV lookup introduced by 3695 reverseproxy: fix breakage in handling SRV lookup introduced by d55d50b Sep 28, 2020
@mohammed90 mohammed90 removed do not merge ⛔ Not ready yet! in progress 🏃‍♂️ Being actively worked on labels Sep 28, 2020
@mohammed90 mohammed90 requested a review from mholt September 28, 2020 20:45
mholt
mholt previously approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look great! Willing to merge this, my only nit is with a comment. Thanks!

modules/caddyhttp/reverseproxy/reverseproxy.go Outdated Show resolved Hide resolved
mholt
mholt previously approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that'll work! As usual, thank you sooo much!

@mholt mholt removed the under review 🧐 Review is pending before merging label Sep 28, 2020
@mholt mholt self-requested a review September 28, 2020 21:40
mholt
mholt previously approved these changes Sep 28, 2020
View reviewed changes
@mholt
Copy link
Member

mholt commented Sep 28, 2020

CI is failing apparently because Microsoft is having a major outage right now, so GitHub is naturally affected...

@francislavoie
Copy link
Member

Okay that's pretty stupid, it looks like it didn't dismiss the failing jobs after rerunning them all... 🤔 I think if you just force push again @mohammed90 it should be okay (force re-run CI)

@aleert aleert mentioned this pull request Oct 1, 2020
Closed
@mohammed90 mohammed90 mentioned this pull request Oct 1, 2020
Merged
@francislavoie francislavoie added the bug 🐞 Something isn't working label Oct 1, 2020
mholt
mholt approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the helpful fixes @mohammed90 😁

@mholt mholt merged commit 6722426 into master Oct 1, 2020
@mholt mholt deleted the fix-srv-3753 branch October 1, 2020 20:05
mholt referenced this pull request Oct 5, 2020
@mohammed90
reverseproxy: Enforce port range size of 1 at provision (#3695)
d55d50b 
* reverse_proxy: ensure upstream address has port range of only 1

* reverse_proxy: don't log the error if upstream range size is more than 1
mholt added a commit that referenced this pull request Oct 7, 2020
@mholt
reverseproxy: Fix dial placeholders, SRV, active health checks
aeceb32 
Supercedes #3776
Partially reverts or updates #3756, #3693, and #3695
@mholt mholt mentioned this pull request Oct 7, 2020
Merged
@caddyserver caddyserver deleted a comment from lorenckrzysztof Oct 11, 2020
mholt added a commit that referenced this pull request Oct 13, 2020
@mholt @mohammed90
reverseproxy: Fix dial placeholders, SRV, active health checks (#3780)
c7efb03 
* reverseproxy: Fix dial placeholders, SRV, active health checks

Supercedes #3776
Partially reverts or updates #3756, #3693, and #3695

* reverseproxy: add integration tests

Co-authored-by: Mohammed Al Sahaf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francislavoie francislavoie francislavoie approved these changes

@mholt mholt mholt approved these changes

Assignees
No one assigned
Labels
bug 🐞 Something isn't working
Projects
None yet
Milestone
v2.2.1
Development

Successfully merging this pull request may close these issues.

upgrade to v2.2.0 break SRV Caddyfile
5 participants
@mohammed90 @danlsgiga @waiyip-aquabyte @mholt @francislavoie