Ignore ctrlreg in deletion when computing required ctrlinsts

[vpnachev]

How to categorize this PR?

/area quality
/kind bug
/priority normal

What this PR does / why we need it:
Ignore ControllerRegistration in deletion when computing required ControllerInstalations.

To properly apply gardener/gardener-extension-os-suse-chost#20 on existing landscapes, it is necessary to manually delete the existing controller installations. However, on large landscapes GCM creates new controller installations before the last old one to be deleted. With this change, once the deletion of the controller registration is triggered, no new installations will be made out of it.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

`ControllerInstallation`s are no longer created for `ControllerRegistration`s that are in deletion.

[rfranzke]

Thanks, the change makes sense 👍 Can you add a new ControllerRegistration with a deletionTimestamp and support for resource kind/type that are used by one of the shoots here? That'd probably be enough to test this case (expectation would be that the computed list of required controller registrations doesn't change).

[vpnachev]

I have amend it to make it easier for testing.

[timebertt]

Thanks for the PR, makes sense!

[rfranzke]

/status author-action

[gardener-robot]

@vpnachev The pull request was assigned to you under author-action. Please unassign yourself when you are done. Thank you.

[vpnachev]

An admission plugin has been implemented, in this way it is guaranteed that creation of a controller installation for controller registration in deletion will not succeed.

The change in the controller is re-worked again and it now silently skips the creation of controller installation, however it still can update existing installations. I tried to implement some unit test about this change, something similar to

gardener/pkg/controllermanager/controller/controllerregistration/controllerregistration_seed_control_test.go

Lines 772 to 823 in f2f47cc

	Describe("#deployNeededInstallations", func() {
	It("should return an error", func() {
	var (
	wantedControllerRegistrations = sets.NewString(controllerRegistration2.Name)
	registrationNameToInstallationName = map[string]string{
	controllerRegistration1.Name: controllerInstallation1.Name,
	controllerRegistration2.Name: controllerInstallation2.Name,
	controllerRegistration3.Name: controllerInstallation3.Name,
	}
	fakeErr = errors.New("err")
	)
	k8sClient.EXPECT().Get(ctx, kutil.Key(controllerInstallation2.Name), gomock.AssignableToTypeOf(&gardencorev1beta1.ControllerInstallation{})).Return(fakeErr)
	err := deployNeededInstallations(ctx, nopLogger, k8sClient, seedWithShootDNSEnabled, wantedControllerRegistrations, controllerRegistrations, registrationNameToInstallationName)
	Expect(err).To(Equal(fakeErr))
	})
	It("should correctly deploy needed controller installations", func() {
	var (
	wantedControllerRegistrations = sets.NewString(controllerRegistration2.Name, controllerRegistration3.Name)
	registrationNameToInstallationName = map[string]string{
	controllerRegistration1.Name: controllerInstallation1.Name,
	controllerRegistration2.Name: controllerInstallation2.Name,
	controllerRegistration3.Name: controllerInstallation3.Name,
	}
	)
	installation2 := controllerInstallation2.DeepCopy()
	installation2.Labels = map[string]string{
	common.RegistrationSpecHash: "b24405c0d68a538e",
	common.SeedSpecHash: "6668c8b5c30659ab",
	}
	installation3 := controllerInstallation3.DeepCopy()
	installation3.Labels = map[string]string{
	common.RegistrationSpecHash: "b24405c0d68a538e",
	common.SeedSpecHash: "6668c8b5c30659ab",
	}
	k8sClient.EXPECT().Get(ctx, kutil.Key(controllerInstallation2.Name), gomock.AssignableToTypeOf(&gardencorev1beta1.ControllerInstallation{}))
	k8sClient.EXPECT().Update(ctx, installation2)
	k8sClient.EXPECT().Get(ctx, kutil.Key(controllerInstallation3.Name), gomock.AssignableToTypeOf(&gardencorev1beta1.ControllerInstallation{}))
	k8sClient.EXPECT().Update(ctx, installation3)
	err := deployNeededInstallations(ctx, nopLogger, k8sClient, seedWithShootDNSEnabled, wantedControllerRegistrations, controllerRegistrations, registrationNameToInstallationName)
	Expect(err).NotTo(HaveOccurred())
	})
	})

but I failed to understand how these tests works and whether it is possible such a test to be implemented.

[rfranzke]

An admission plugin has been implemented, in this way it is guaranteed that creation of a controller installation for controller registration in deletion will not succeed.

Hm, I can't follow - why is such an admission plugin needed? I mean, what's the reason this is so important that we have to forbid it API-wise? Can't we save this code for good? The only code that creates ControllerRegistration in Gardener is the controller you already changed, and if a user creates a ControllerInstallation manually then why should we forbid this?

but I failed to understand how these tests works and whether it is possible such a test to be implemented.

What exactly didn't you understand? If I see it correctly, the only thing you have to do is to add a ControllerRegistration with a deletionTimestamp to this list. As it's not expected that it's going to be created you don't need EXPECT() calls for the k8sClient.
As a second test case, you could also add a ControllerRegistration with a deletionTimestamp which already has an existing ControllerInstallation. In this case you would expect that an Update() call is sent.

[vpnachev]

... and if a user creates a ControllerInstallation manually then why should we forbid this?

I have no strong opinion, it was implemented just as a second prevention mechanism. Basically, applying the same approach by k8s community with namespaces - if it is in deletion, no new resources can be created in the namespace.

What exactly didn't you understand?

Exactly these .EXPECT(). calls are not clear to me what are they doing and how are they supposed to be used.
Looking through the generated code also does not help. I tried to find some documentation or guides or to look through https://github.com/kubernetes-sigs/controller-runtime/tree/master/pkg/client but failed here as well. Could you point me to some guides/documentation about this framework?

What I really tried to do was to call deployNeededInstallations with a ControllerRegistration with deletionTimestamp set and then list the ControllerInstallations with the provided fake client expecting empty list as result - this failed on multiple places, so I have decided that I am doing it completely wrong.

[vpnachev]

Ok, I was thinking that this is coming from k8s.io, while it is actually from gomock - I will spend some time with this package.

Together with @ialidzhikov we have taken a look through the change and he helped me a lot to understand more about the tests, especially for mocks and fake clients, and to implement one (thanks for that, @ialidzhikov !).

In parallel, there is a validation that disallow ControllerRigistration to be updated once it is in deletion, thus we have decided to also not update existing installations because it should not possible due to this validation.

[rfranzke]

/invite @timuthy @danielfoehrKn @BeckerMax

[ialidzhikov]

/assign @ialidzhikov

vacation

[vpnachev]

The admission plugin has been removed.

[rfranzke]

/lgtm

[ialidzhikov]

/lgtm