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

GKE Ingress Health Check middleware #93

Closed
wants to merge 2 commits into from
Closed

GKE Ingress Health Check middleware #93

wants to merge 2 commits into from

Conversation

benfdking
Copy link
Contributor

@benfdking benfdking commented Mar 8, 2019

The ingress requires the root path of the target to return a 200 (OK) to
indicate the service's good health. The middleware handles the specific
health check call.

Description

This change adds the middle ware required to handle that specific call.

Motivation and Context

This is required to run the proxy smoothly on GKE

How Has This Been Tested?

The code has been tested and it has been tested on our clusters.

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.

The ingress requires the root path of the target to return a 200 (OK) to
indicate the service's good health. The middleware handles the specific
health check call.
@benfdking benfdking requested a review from a team March 8, 2019 22:41
@benfdking benfdking closed this Mar 8, 2019
@benfdking benfdking reopened this Mar 8, 2019
@JoelSpeed
Copy link
Member

Hey @benfdking, thanks for submitting this. I'm reasonably happy with the implementation from my initial scan but I would like to request that we put this behind a feature flag.

At present this would add the wrapper to all requests for all users of the project which, while minimal, will increase the processing due to the additional handler. I think it would be better to have the middleware enabled only for those who actually need it, WDYT?

@timothy-spencer
Copy link
Contributor

Hey, I somehow missed this PR. I wrote something similar for GCP App Engine here: #110

Do you think it would be a good idea to combine these, or at least do them in the same way?

@JoelSpeed
Copy link
Member

Personally I prefer the approach in #110 as it keeps the logic in-line with the rest of the endpoints, is there any way we can fix this GKE issue in a similar way?

@benfdking
Copy link
Contributor Author

Sorry I have been AWOL. Work... I think you can replicate what #110 has done and merge them together. All we need to do is merge the two into a GCP wrapper. In line 29 of http.go just add a check that matches that uses this essentially:

const userAgentHeader = "User-Agent"
const googleHealthCheckUserAgent = "GoogleHC/1.0"
const rootPath = "/"

// IngressHealthCheck is a middleware that is designed to be used with the Ingress in GKE. The ingress requires the root
// path of the target to return a 200 (OK) to indicate the service's good health. This can be quite a challenging demand
// depending on the application's path structure. This middleware filters out the requests from the health check by
//
// 1. checking that the request path is indeed the root path
// 2. ensuring that the User-Agent is "GoogleHC/1.0", the health checker
// 3. ensuring the request method is "GET"
//
// The ping can be used to indicate the health of the service. Like the sql package implementation, if it returns an
// error, the middleware will return a unhealthy response by returning a 500 (Internal). Otherwise if healthy, it
// returns a 200 (OK).
//
// An error that causes an "unhealthy" is not returned to the health check for security purposes.
func IngressHealthCheck(next http.Handler, ping func() error) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.URL.Path == rootPath &&
			r.Header.Get(userAgentHeader) == googleHealthCheckUserAgent &&
			r.Method == http.MethodGet {
			if err := ping(); err != nil {
				w.WriteHeader(http.StatusInternalServerError)
				return
			}
			w.WriteHeader(http.StatusOK)
			return
		}
		next.ServeHTTP(w, r)
	})
}

@timothy-spencer
Copy link
Contributor

I incorporated your suggestion in #110. Let me know if that works properly for you!

@JoelSpeed
Copy link
Member

@benfdking Happy to close this now the changes are incorporated in #110?

@benfdking
Copy link
Contributor Author

Definitely go for it!

@benfdking benfdking closed this Mar 26, 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

3 participants