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

Refactor gardener-admission-controller #3577

Merged
merged 9 commits into from Feb 19, 2021
Merged

Refactor gardener-admission-controller #3577

merged 9 commits into from Feb 19, 2021

Conversation

timebertt
Copy link
Member

@timebertt timebertt commented Feb 17, 2021 •
edited

How to categorize this PR?

/area quality dev-productivity
/kind cleanup
/priority normal

What this PR does / why we need it:

This PR refactors/enhances GAC similar to GSAC in #3459.

  • migrate the component to a full controller-runtime component (use webhook handlers/decoders, metrics, health endpoints and so on) instead of custom coding for those mechanisms
  • enable both admission review versions (including admission/v1) in the webhook configs, as it is now supported by the c-r webhook handlers
  • decrease the timeouts of the extension deletion protection webhook to 10s according to best pratice

Which issue(s) this PR fixes:
Part of #3109

Special notes for your reviewer:
/squash

Release note:

The `gardener-admission-controller` configuration API and http endpoints were changed in several aspects:
- the fields `server.https.tls.server{Cert,Key}Path` have been removed in favor of `server.https.tls.serverCertDir` (the cert directory is expected to contain a `tls.crt` and `tls.key` file)
- metrics and health endpoints are now exposed as plain HTTP endpoints on dedicated ports (configurable via `server.{healthProbes,metrics}.port`
- the `gardener-admission-controller` service included in Gardener's helm chart has a new named port (`metrics`) for exposing the metrics endpoint
If you deploy this component/configuration manually, please adapt your usage accordingly. Gardener's helm charts were adapted to the changes.

`gardener-admission-controller`'s webhooks now also accept reviews in version `admission/v1`. Also, webhook timeouts have been lowered to `10s`.

`gardener-admission-controller` now exposes several metrics about its webhooks (e.g. `controller_runtime_webhook_latency_seconds_bucket`, `controller_runtime_webhook_requests_in_flight` and `controller_runtime_webhook_requests_total`)
The metric `gardener_admission_controller_invalid_webhook_requests_total` was removed in favor of the newly added metrics.

@timebertt timebertt requested a review from a team as a code owner February 17, 2021 11:46
@gardener-robot gardener-robot added area/dev-productivity Developer productivity related (how to improve development) area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/cleanup Something that is not needed anymore and can be cleaned up merge/squash size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 17, 2021
@rfranzke rfranzke self-assigned this Feb 17, 2021
rfranzke
rfranzke previously approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

@timuthy timuthy mentioned this pull request Feb 18, 2021
Merged
@timuthy
Copy link
Contributor

timuthy commented Feb 18, 2021

/assign

timuthy
timuthy reviewed Feb 19, 2021
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.

Nice, I really like it! Two comments from my side, otherwise looks good.

cmd/gardener-admission-controller/app/gardener_admission_controller.go Outdated Show resolved Hide resolved
pkg/admissioncontroller/webhooks/validate_resource_size.go Outdated Show resolved Hide resolved
timuthy
timuthy approved these changes Feb 19, 2021
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

@timuthy timuthy merged commit 0c762c8 into gardener:master Feb 19, 2021
@timebertt timebertt deleted the refactor/admission-controller branch February 19, 2021 14:13
@rfranzke rfranzke mentioned this pull request Mar 2, 2021
Merged
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
@timebertt
Refactor gardener-admission-controller (gardener#3577)
e2db048 
* Replace server.https.tls.server{Cert,Key}Path by server.https.tls.serverCertDir

* Add metrics and healthProbe server config

* refactor admission-controller into controller-runtime component

* adapt tests for webhook handlers

* Accept admission/v1, set timeoutSeconds=10

* remove unused metric InvalidWebhookRequest

* Limit number of shoots listed to 1

* Make metrics and healthProbes config pointers

* Address review comments
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
@timebertt
Refactor gardener-admission-controller (gardener#3577)
b77f01d 
* Replace server.https.tls.server{Cert,Key}Path by server.https.tls.serverCertDir

* Add metrics and healthProbe server config

* refactor admission-controller into controller-runtime component

* adapt tests for webhook handlers

* Accept admission/v1, set timeoutSeconds=10

* remove unused metric InvalidWebhookRequest

* Limit number of shoots listed to 1

* Make metrics and healthProbes config pointers

* Address review comments
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

@rfranzke rfranzke rfranzke left review comments

Assignees

@rfranzke rfranzke

@timuthy timuthy

Labels
area/dev-productivity Developer productivity related (how to improve development) area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/cleanup Something that is not needed anymore and can be cleaned up size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@timebertt @timuthy @rfranzke @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot