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

Refactor gardener-seed-admission-controller #3459

Merged
merged 6 commits into from Feb 2, 2021

Conversation

timebertt
Copy link
Member

How to categorize this PR?

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

What this PR does / why we need it:

This PR refactors/enhances several things regarding the seed-admission-controller:

  • migrate the component to a full controller-runtime component (only usage of manager was left)
  • 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 pratices
  • add an envtest-style integration test for the extension deletion protection webhook
  • migrate the handcrafted integration test for the scheduler name webhook to an envtest-style test

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

Special notes for your reviewer:

The PR got a bit bigger than expected, but it's still split into several commits, so I hope this helps with the review.
/squash

Release note:

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

@timebertt timebertt requested a review from a team as a code owner January 26, 2021 16:58
@gardener-robot gardener-robot added needs/review 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 kind/test Test size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2021
@timebertt timebertt changed the title Refactor/seed admission Refactor gardener-seed-admission-controller Jan 27, 2021
@rfranzke
Copy link
Member

/assign
/rebase

rfranzke
rfranzke previously approved these changes Jan 29, 2021
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

Very nice, thanks for adding the integration test!

rfranzke
rfranzke previously approved these changes Jan 29, 2021
@stoyanr
Copy link
Contributor

stoyanr commented Feb 1, 2021

/lgtm

I have a general question: I used to write envtest while working on other projects in the past (SEN). I think it's the first such test added to g/g, possibly because it requires a controller runtime component (is this correct?). Are there plans to migrate other components (e.g. gardenlet) to controller runtime as well, so we could write add such tests for them as well?

@timebertt
Copy link
Member Author

I think it's the first such test added to g/g, possibly because it requires a controller runtime component (is this correct?).

It's not the first one, but the second (#2830 was the first one) 😉
You're right though, there are not many envtests in g/g currently.
I think, we should be able to leverage envtest for integration tests for some of our controllers as well, at least for the ones that already implement the reconciler.Reconcile interface. Though, we haven't done any of those yet.

Are there plans to migrate other components (e.g. gardenlet) to controller runtime as well, so we could write add such tests for them as well?

I plan to do the refactoring for gardener-admission-controller this week (similar to this PR).
I think, the general goal is to do the same for the controller components as well, though it will be much more effort and might reveal some difficulties. As of now, there are no concrete plans for this, I guess.

@timebertt
Copy link
Member Author

Rebased to resolve conflicts.

@rfranzke rfranzke merged commit c69a5b3 into gardener:master Feb 2, 2021
@timebertt timebertt deleted the refactor/seed-admission branch February 2, 2021 07:00
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
* Refactor seed-admission-controller to use controller-runtime manager

* Enable admission/v1 for GSAC

* Decrease timeouts of extension deletion protection webhook

* Add integration test for extension deletion protection

* Migrate scheduler name webhook test to envtest

* Improve test readability
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
* Refactor seed-admission-controller to use controller-runtime manager

* Enable admission/v1 for GSAC

* Decrease timeouts of extension deletion protection webhook

* Add integration test for extension deletion protection

* Migrate scheduler name webhook test to envtest

* Improve test readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 kind/test Test size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants