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

Add /healthz endpoint for Gardenlet #2309

Merged
merged 1 commit into from May 19, 2020
Merged

Add /healthz endpoint for Gardenlet #2309

merged 1 commit into from May 19, 2020

Conversation

rfranzke
Copy link
Member

@rfranzke rfranzke commented May 11, 2020 •
edited by vpnachev

What this PR does / why we need it:
This PR enhances the Gardenlet with a HTTPS server that serves the /healthz and /metrics endpoints. The /healthz endpoint returns 200 OK if the Gardenlet can regularly renew its seed lease. Otherwise, it will return 500 Internal Server Error.

Release note:

The Gardenlet does now run a HTTPS server that serves a `/healthz` and `/metrics` endpoint. You should generate a server certificate for the `gardenlet`, `gardenlet.garden`, `gardenlet.garden.svc` hosts. The bind address, port, and TLS certificate paths are configurable [in its component config](https://github.com/gardener/gardener/blob/master/example/20-componentconfig-gardenlet.yaml#L75-L81). Also, the `gardenlet` Helm chart was enhanced with a liveness probe that targets the `/healthz` endpoint.

@rfranzke rfranzke requested a review from a team as a code owner May 11, 2020 06:30
@rfranzke
Copy link
Member Author

/kind/enhancement

@ghost ghost added the kind/enhancement Enhancement, improvement, extension label May 11, 2020
@timuthy
Copy link
Contributor

timuthy commented May 11, 2020

Nice PR!
Can you elaborate why we expose the new endpoints via HTTP? I think especially the /metrics endpoint should be exposed via HTTPS only, shouldn't it?

@rfranzke
Copy link
Member Author

No specific reason, it just was like this before. I can also move both to HTTPS, should be no problem.

@rfranzke rfranzke requested a review from timuthy May 11, 2020 09:54
@timuthy timuthy self-assigned this May 11, 2020
timuthy
timuthy requested changes May 11, 2020
View reviewed changes
pkg/gardenlet/apis/config/v1alpha1/defaults.go Outdated Show resolved Hide resolved
cmd/gardenlet/app/gardenlet.go Show resolved Hide resolved
pkg/gardenlet/apis/config/v1alpha1/defaults.go Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
@rfranzke rfranzke requested a review from timuthy May 12, 2020 05:20
timuthy
timuthy reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

Looks good to me in general.

I noticed one issue with the health check though, if the Gardenlet is responsible for more than one seed and the lease of one of the seeds cannot be renewed. This scenario will end up in a constantly swapping health state. OTOH, such a setup is not recommended for production use cases, so I can understand if you don't want to tackle it. WDYT?

@rfranzke
Copy link
Member Author

Do you have a suggestion how what the expected behaviour should be and how to implement it?

@rfranzke rfranzke requested a review from timuthy May 13, 2020 05:23
timuthy
timuthy previously approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

/lgtm

@rfranzke
Copy link
Member Author

/hold
I have to adapt the gardenlet self-deployment to also configure the new server section.

@rfranzke
Copy link
Member Author

/area/quality
/area/robustness
/size/l
/topology/seed
/needs/changes
/platform/all
/priority/normal

@ghost ghost added area/quality Output qualification (tests, checks, scans, automation in general, etc.) related area/robustness Robustness, reliability, resilience related size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topology/seed Affects Seed clusters needs/changes platform/all labels May 14, 2020
@rfranzke
Copy link
Member Author

/unhold
/drop needs changes
/needs/review
/cc @timuthy

@ghost ghost requested a review from timuthy May 14, 2020 12:41
@rfranzke rfranzke merged commit 0677d2a into gardener:master May 19, 2020
@rfranzke rfranzke deleted the feature/server branch May 19, 2020 15:55
@rfranzke rfranzke mentioned this pull request May 20, 2020
Merged
@vpnachev
Copy link
Member

Now when #2361 is merged, I've removed the release note

action developer
Developers have to run `make dev-setup` in order to populate the Gardenlet certificates to their `dev` directories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timuthy timuthy timuthy approved these changes

Assignees

@timuthy timuthy

Labels
area/quality Output qualification (tests, checks, scans, automation in general, etc.) related area/robustness Robustness, reliability, resilience related kind/enhancement Enhancement, improvement, extension platform/all size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topology/seed Affects Seed clusters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rfranzke @timuthy @vpnachev @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3