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

added an option to enable GCP healthcheck endpoints #110

Merged
merged 11 commits into from Mar 27, 2019

Conversation

timothy-spencer
Copy link
Contributor

@timothy-spencer timothy-spencer commented Mar 20, 2019

Description

This adds an option that lets you enable GCP healthcheck endpoints.
https://cloud.google.com/appengine/docs/flexible/custom-runtimes/configuring-your-app-with-app-yaml#updated_health_checks

It also incorporates suggestions from #93 that lets it work with GKE ingresses.

Motivation and Context

GCP likes to know when the app is up and running. Enabling these endpoints makes it so that it is easy to run the proxy in the GCP App Engine environment

How Has This Been Tested?

I use the go test ./... tests, as well as tested this by hand by launching with and without the flag enabled and attempting to go to locahost:4180/liveness_check and localhost:4180/readiness_check. With the flag enabled, I got back "OK", and without, I got the normal auth redirect.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@timothy-spencer timothy-spencer requested a review from a team March 20, 2019 21:42
@ploxiln
Copy link
Contributor

ploxiln commented Mar 21, 2019

Have you considered using -skip-auth-regex to allow these two paths, and implementing the health check paths in the reverse-proxied application?

@timothy-spencer
Copy link
Contributor Author

@ploxiln Hmm. Interesting idea. I think it's not quite the right approach, though, because if the upstream app were to get sad, GCP would begin killing and restarting the proxy, even if it were actually operating perfectly. I'd rather the proxy remain up and happy, waiting for the upstream to return.

@benfdking
Copy link
Contributor

benfdking commented Mar 26, 2019

Looks good to me. Thank you for incorporating those changes! Good stuff

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

One minor nit but I'm happy after that, thanks for incorporating #93 in with this 😄

main.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Great, Thanks for your work on this @timothy-spencer (and @benfdking for #93), I'll get this merged!

@JoelSpeed JoelSpeed merged commit e9d4f6e into oauth2-proxy:master Mar 27, 2019
@benfdking benfdking mentioned this pull request Mar 27, 2019
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.

None yet

4 participants