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

reverseproxy: Implement health_uri, replaces health_path, supports query #4050

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Mar 4, 2021

Closes #4049

reverse_proxy localhost:54321 {
	health_uri /health?ready=1
	health_status 2xx
}

Also fixes a bug with health_status Caddyfile parsing, it would always only take the first character of the status code even if it didn't end with "xx". I fixed the same bug in #4021 but it's relevant here too because it affects the adapt test I wrote (will rebase #4021 after and remove the redundant fix)

@francislavoie francislavoie added this to the v2.4.0 milestone Mar 4, 2021
@francislavoie francislavoie requested a review from mholt March 4, 2021 19:05
@francislavoie
Copy link
Member Author

Flaky tests 😢

@mholt
Copy link
Member

mholt commented Mar 12, 2021

So here's a curveball, what if we introduce health_uri and deprecate health_path? The URI is just the path + optional query string in this case. That way the user doesn't have to separate them, seems kinda odd to require them separate...

@francislavoie
Copy link
Member Author

Sure, that works 👍 I'll rework it

@francislavoie francislavoie changed the title reverseproxy: Support active health check query reverseproxy: Implement health_uri, replaces health_path, supports query Mar 13, 2021
@francislavoie
Copy link
Member Author

@mholt Done 😄

@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Mar 13, 2021
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 working on this!

modules/caddyhttp/reverseproxy/healthchecks.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/reverseproxy.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/reverseproxy.go Outdated Show resolved Hide resolved
Also fixes a bug with `health_status` Caddyfile parsing , it would always only take the first character of the status code even if it didn't end with "xx".
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.

Thank you for helping with this! This is a nice improvement.

@mholt mholt merged commit 75f797d into master Mar 30, 2021
@mholt mholt deleted the health-query branch March 30, 2021 00:36
@mholt mholt removed the under review 🧐 Review is pending before merging label Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support query parameters on active healthcheck
2 participants