[controller-manager] Switch `Bastion` controller to `controller-runtime` and introduce common indexers #6358

rfranzke · 2022-07-19T14:27:46Z

How to categorize this PR?

/area dev-productivity scalability
/kind enhancement

What this PR does / why we need it:
Refactor the Bastion controller to controller-runtime.

Also, introduce a new pkg/api/indexer package which hosts functions for adding field indexes to the controller-runtime cache.
The package is commonly used by controller-manager, gardenlet and integration tests, so that we can add indexes in symmetry and without duplication.

Partly co-authored by @timebertt.

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

Special notes for your reviewer:
Generally, we want to follow this cookbook while refactoring existing controllers:

Add documentation
Add integration test based on envtest
Switch controller to controller-runtime

Release note:

Some signatures in `pkg/controllerutils/mapper` have changed to support the simple injection of a proper context and logger.

plkokanov

Very nice PR! I just have a few questions.

pkg/controllermanager/controller/bastion/add.go

extensions/pkg/controller/worker/mapper.go

oliver-goetz

Shouldn't we create unit tests for pkg/api/indexer as it is a new package now? 😅

pkg/controllerutils/mapper/enqueue_mapped.go

pkg/controllermanager/controller/add.go

timebertt · 2022-07-21T07:51:50Z

/assign
I will review this PR and also address the open suggestions, while @rfranzke is absent.

Otherwise, IDEs might show an error that we don't import these packages "correctly". Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

This way, they will become reusable (e.g. for integration tests, see next commit). Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

This makes it easier to construct functions which can be casted via `mapper.MapFunc` to the `mapper.Mapper` interface (see next commit). Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

same as in a173f8a

This allows us to use one manager for all cases (next commit)

timebertt

Not completely through yet

pkg/controllermanager/controller/bastion/bastion_control.go

pkg/api/indexer/core.go

test/integration/controllermanager/bastion/bastion_test.go

extensions/pkg/controller/backupbucket/controller.go

timebertt

This is my remaining feedback

pkg/controllermanager/controller/bastion/add.go

pkg/controllermanager/controller/bastion/add_test.go

timebertt · 2022-07-22T06:46:50Z

I have finished my review and pushed a few more commits addressing the open suggestions.
@oliver-goetz @plkokanov PTAL :)

pkg/controllermanager/controller/add.go

oliver-goetz

The scope of this PR is now a bit bigger than its name and description suggests.
Maybe it is worth mentioning that the new common indexer functions are used by gardenlet too.

pkg/gardenlet/controller/factory.go

timebertt · 2022-07-26T06:02:17Z

@oliver-goetz @plkokanov thanks for your feedback, PTAL :)

gardener-prow · 2022-07-26T06:03:29Z

@rfranzke: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gardener-apidiff	`415afb0`	link	false	`/test pull-gardener-apidiff`

Full PR test history. Your PR dashboard. Command help for this repository.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

plkokanov · 2022-07-26T06:14:06Z

/lgtm on my side

oliver-goetz · 2022-07-26T07:57:02Z

/lgtm

timebertt · 2022-07-26T08:29:02Z

/approve

gardener-prow · 2022-07-26T08:29:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timebertt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [timebertt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rfranzke · 2022-07-26T09:13:43Z

pkg/controllermanager/controller/bastion/add_test.go

@@ -189,3 +209,60 @@ var _ = Describe("Add", func() {
 		})
 	})
 })
+
+// TODO: remove this again once the controller-runtime fake client supports field selectors


Is there already a PR for it or can you open one with this code? :)

Unfortunately, there is no PR for this upstream yet, AFAIK.
This will probably involve some design discussion, so expect some delay. But I will try my best :)

😄 Okay, thank you!

acumino · 2022-07-26T14:07:34Z

/milestone v1.52

gardener-prow bot added area/dev-productivity Developer productivity related (how to improve development) area/scalability Scalability related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Jul 19, 2022

gardener-prow bot requested review from krgostev and wyb1 July 19, 2022 14:27

gardener-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 19, 2022

This was referenced Jul 19, 2022

Refactor Gardener components to native controller-runtime components #4251

Closed

[controller-manager] Add documentation and integration test for CloudProfile controller #6360

Merged

plkokanov reviewed Jul 20, 2022

View reviewed changes

pkg/controllermanager/controller/bastion/add.go Show resolved Hide resolved

extensions/pkg/controller/worker/mapper.go Show resolved Hide resolved

rfranzke requested a review from plkokanov July 20, 2022 11:15

oliver-goetz reviewed Jul 20, 2022

View reviewed changes

pkg/controllerutils/mapper/enqueue_mapped.go Outdated Show resolved Hide resolved

pkg/controllerutils/mapper/enqueue_mapped.go Outdated Show resolved Hide resolved

rfranzke force-pushed the refactor/bastion-ctrl branch from 7325298 to 804b367 Compare July 20, 2022 15:24

rfranzke requested a review from oliver-goetz July 20, 2022 15:24

plkokanov reviewed Jul 21, 2022

View reviewed changes

pkg/controllermanager/controller/add.go Show resolved Hide resolved

gardener-prow bot assigned timebertt Jul 21, 2022

gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2022

rfranzke and others added 12 commits July 21, 2022 16:17

[Nit] Remove import directives from third_party packages

b8f335f

Otherwise, IDEs might show an error that we don't import these packages "correctly". Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Rework documentation

633f66b

Move field indexers into pkg/api/indexer package

3687844

This way, they will become reusable (e.g. for integration tests, see next commit). Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Add integration test

a18b0a8

Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Extend mapper.Mapper interface with context, logger and reader

a2d9698

This makes it easier to construct functions which can be casted via `mapper.MapFunc` to the `mapper.Mapper` interface (see next commit). Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Switch controller to native controller-runtime controller

e9335fc

Co-Authored-By: Tim Ebert <tim.ebert@sap.com>

Address PR review feedback

c784daa

Nits in docs

77143bd

Reuse indexer package in gardenlet

5c462aa

Remove pointer for config

e89e97e

same as in a173f8a

Consequently use fake clock in reconciler and tests

53b8316

This allows us to use one manager for all cases (next commit)

Use one manager for all integration test cases

dbd5d86

timebertt force-pushed the refactor/bastion-ctrl branch from 804b367 to e55398e Compare July 21, 2022 15:00

timebertt reviewed Jul 21, 2022

View reviewed changes

gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2022

timebertt requested a review from plkokanov July 22, 2022 06:02

timebertt added 2 commits July 22, 2022 08:39

Simplify predicate tests and move to test package

358e47f

Also test field selector of mapper

c749828

timebertt reviewed Jul 22, 2022

View reviewed changes

oliver-goetz reviewed Jul 25, 2022

View reviewed changes

pkg/controllermanager/controller/add.go Show resolved Hide resolved

oliver-goetz reviewed Jul 25, 2022

View reviewed changes

plkokanov requested changes Jul 25, 2022

View reviewed changes

pkg/gardenlet/controller/factory.go Outdated Show resolved Hide resolved

timebertt changed the title ~~Switch Bastion controller to controller-runtime~~ Switch Bastion controller to controller-runtime and introduce common indexers Jul 26, 2022

Nits

415afb0

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2022

gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2022

rfranzke commented Jul 26, 2022

View reviewed changes

gardener-prow bot added this to the v1.52 milestone Jul 26, 2022

oliver-goetz added component/gardener Gardener and removed component/gardener Gardener labels Jul 26, 2022

gardener-prow bot merged commit 5b34567 into gardener:master Jul 26, 2022

rfranzke deleted the refactor/bastion-ctrl branch July 26, 2022 15:50

rfranzke changed the title ~~Switch Bastion controller to controller-runtime and introduce common indexers~~ [controller-manager] Switch Bastion controller to controller-runtime and introduce common indexers Sep 23, 2022

AleksandarSavchev mentioned this pull request Oct 21, 2022

Vendor gardener 1.57.1 and upgrade ginkgo to v2 gardener/etcd-druid#450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller-manager] Switch `Bastion` controller to `controller-runtime` and introduce common indexers #6358

[controller-manager] Switch `Bastion` controller to `controller-runtime` and introduce common indexers #6358

rfranzke commented Jul 19, 2022 •

edited by timebertt

plkokanov left a comment

oliver-goetz left a comment

timebertt commented Jul 21, 2022

timebertt left a comment

timebertt left a comment

timebertt commented Jul 22, 2022 •

edited

oliver-goetz left a comment

timebertt commented Jul 26, 2022

gardener-prow bot commented Jul 26, 2022

plkokanov commented Jul 26, 2022

oliver-goetz commented Jul 26, 2022

timebertt commented Jul 26, 2022

gardener-prow bot commented Jul 26, 2022

rfranzke Jul 26, 2022

timebertt Jul 26, 2022

rfranzke Jul 26, 2022

acumino commented Jul 26, 2022

[controller-manager] Switch Bastion controller to controller-runtime and introduce common indexers #6358

[controller-manager] Switch Bastion controller to controller-runtime and introduce common indexers #6358

Conversation

rfranzke commented Jul 19, 2022 • edited by timebertt

plkokanov left a comment

Choose a reason for hiding this comment

oliver-goetz left a comment

Choose a reason for hiding this comment

timebertt commented Jul 21, 2022

timebertt left a comment

Choose a reason for hiding this comment

timebertt left a comment

Choose a reason for hiding this comment

timebertt commented Jul 22, 2022 • edited

oliver-goetz left a comment

Choose a reason for hiding this comment

timebertt commented Jul 26, 2022

gardener-prow bot commented Jul 26, 2022

plkokanov commented Jul 26, 2022

oliver-goetz commented Jul 26, 2022

timebertt commented Jul 26, 2022

gardener-prow bot commented Jul 26, 2022

rfranzke Jul 26, 2022

Choose a reason for hiding this comment

timebertt Jul 26, 2022

Choose a reason for hiding this comment

rfranzke Jul 26, 2022

Choose a reason for hiding this comment

acumino commented Jul 26, 2022

[controller-manager] Switch `Bastion` controller to `controller-runtime` and introduce common indexers #6358

[controller-manager] Switch `Bastion` controller to `controller-runtime` and introduce common indexers #6358

rfranzke commented Jul 19, 2022 •

edited by timebertt

timebertt commented Jul 22, 2022 •

edited