Add validation of Kubernetes feature gates

[stoyanr]

How to categorize this PR?

/area usability
/area ops-productivity
/kind enhancement
/platform gcp

What this PR does / why we need it:
Adds validation of Kubernetes feature gates (in CloudControllerManager field of the ControlPlaneConfig custom resource), as requested in gardener/gardener#3987 and #263.

Which issue(s) this PR fixes:
Fixes #263

Special notes for your reviewer:

• Also vendors g/g v1.24.1-0.20210608063816-580010846480 (and k8s v0.20.7) to be able to use the ValidateFeatureGates function.

Release note:

When creating or updating shoots, any Kubernetes feature gates mentioned are validated against the Kubernetes version. If any feature gates are unknown or not supported in the Kubernetes version, the validation fails.

[stoyanr]

/invite @rfranzke @BeckerMax @voelzmo

Similar PR should be opened for all other provider extensions. I will do this after gardener/gardener#4149 and this one are merged.

[voelzmo]

Hey @stoyanr, thanks for the ping! I'll set aside some time to really look at this and the corresponding gardener PR later today. Looking at the sheer numbers of the PR, I'm already feeling kind of anxious to look at ~50k changed lines within a single commit. I'll trust that there's a lot of things going on that I don't really need to care about in detail (e.g. updating vendored dependencies) – would it be possible to split these actions out to a separate commit in the future, such that I can easily select what to focus the review on? Thanks!

[stoyanr]

@voelzmo The ~50k changed lines come from the revendor. This is really quite normal, nothing to worry about. You shouldn't look into all the vendor files at all. No, it's not possible (or not at all required) to split this commit into multiple ones, without the revendor I wouldn't be able to use the ValidateFeatureGates function that's only introduced in a newer gardener (that's based on a newer kubernetes).

When you ignore go.sum and all the vendor files, it's really a rather small PR.

[voelzmo]

Hey @stoyanr, thanks for your perspective! Just to clarify: I'm not asking to make this two separate PRs, it is just about committing things separately if they can be separated – some people call this working mode micro commits. I understand that this is not required at all, it is just that for me as a reviewer having an easy means to separate things to look at during a review makes my life much easier (e.g. by using the 'only look at changes from a single commit' dropdown during the review).

TL;DR
you, as the code author, say

When you ignore go.sum and all the vendor files, it's really a rather small PR.

I, as a reviewer, ask

please make it easy for me to understand that this is a rather small PR by separating the stuff you "just had to do" (e.g. vendoring, formatting, renaming variables that were already there) from the things your change is about (introducing the additional validation logic)

I'm not sure if this is a thing y'all have been discussing in the team already, I just wanted to share my feelings when opening a PR tab with big numbers of changed lines and a very low number of commits ;)

[stoyanr]

@voelzmo Please understand that what I am doing here is a standard practice. Micro commits is what we do as well, no need to discuss it here. This is a micro commit. You simply have to ignore vendor in this case.

Once again: I need to use a function from a newer gardener, which is itself based on a newer kubernetes. There is no other way to do it besides vendoring this new gardener / kubernetes. This brings all these 50k changes you are worried about, and which you basically should ignore. In extension projects, this happens all the time, you will find a number of such PRs already merged.

I would have agreed it makes sense to have the revendor itself in a different commit if after the revendor a lot of changes were needed to the project (due to incompatible changes, etc.). However, this is not the case. I therefore see no value in this (vendor is very easy to ignore) and since it adds effort, I wouldn't do it.

If you still have concerns about it, let's discuss it privately first before continuing the discussion here.

[stoyanr]

@voelzmo I split it into 2 commits after all, I realized it might be a bit more challenging if you are not used to working with such commits. Hopefully it's easier for you now.

[voelzmo]

Highly appreciated, thanks!

[gardener-robot]

@rfranzke, @BeckerMax You have pull request review open invite, please check

[stoyanr]

Infrastructure tests fail with "Invalid service account email (@sap-gcp-k8s-canary-custom.iam.gserviceaccount.com)." (nothing to do with my change). Any idea how to fix this?

[rfranzke]

@dguendisch @schrodit Can you check, please?

[schrodit]

@stoyanr we had a look and found the issue:

In fact you did one change to the tests.
This change passes the infra extension to the WaitUntilExtensionObjectDeleted function. Later that same object is used in the verifyDeletion function to calculate the service account name.
see

gardener-extension-provider-gcp/test/integration/infrastructure/infrastructure_test.go

Line 597 in ae6cef4

serviceAccountName := getServiceAccountName(project, infra.Namespace)

We suspect that the infra object is cleared in WaitUntilExtensionObjectDeleted so that the namespace is empty.
With that, the service name could not be correctly resolved.
So you should be able to fix the issue by creating a deep copy of the infra object and pass the copy to the verifyDeletion func.

[stoyanr]

@schrodit Thanks, I fixed it now.

[vpnachev]

/test

[testmachinery]

Testrun: e2e-gfds4
Workflow: e2e-gfds4-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Succeeded | 8m21s    |
+---------------------+---------------------+-----------+----------+

[stoyanr]

/cc @rfranzke @ialidzhikov Tests are passing now, could you please take another look?

[rfranzke]

/lgtm

[timebertt]

FYI:

We suspect that the infra object is cleared in WaitUntilExtensionObjectDeleted so that the namespace is empty.

Yes that's the case. I fixed it in gardener/gardener#4191 (namely the first commit).
So once that change is vendored, there shouldn't be the need to DeepCopy the object before passing it down anymore.

[ialidzhikov]

/invite @ialidzhikov

Sorry for the long delay, I will try to check this PR later today. :(

[ialidzhikov]

/lgtm

Generally I had some doubts for OOT CCMs that they accept the same set of feature gates, but I now checked the --help of OOT CCMs that we currently use (alicloud and openstack) and it seems that even the OOT CCMs accept the same set of feature gates. So, lgtm.