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

Add validation of Kubernetes feature gates #4149

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

stoyanr
Copy link
Contributor

@stoyanr stoyanr commented Jun 1, 2021

How to categorize this PR?

/area usability
/area ops-productivity
/kind enhancement

What this PR does / why we need it:
Adds validation of Kubernetes feature gates, as requested in #3987.

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

Special notes for your reviewer:

The initial implementation was based on the information contained in https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates. We agreed with @rfranzke to change the approach completely and base it on the diff of https://github.com/kubernetes/kubernetes/blob/master/pkg/features/kube_features.go from one version to the other, starting with 1.15 (the minimum supported one). This should give us much better quality of the data.

I added the script hack/compare-k8s-feature-gates.sh to prepare this diff automatically and used it to fill the initial map. For each new Kubernetes version from now on, the person manually maintaining the map in featuregates.go would have to:

  • Run hack/compare-k8s-feature-gates.sh <old-version> <new-version>. It will present 2 lists of feature gates: those added and those removed in <new-version> compared to <old-version>.
  • Add all added feature gates to the map with <new-version> as MinVersion and no MaxVersion.
  • For any removed feature gates, add <new-version> as MaxVersion to the already existing feature gate in the map.

The manual effort is really low now and requires no more than 5 minutes for each new version.

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 stoyanr requested a review from a team as a code owner June 1, 2021 16:05
@gardener-robot gardener-robot added area/ops-productivity Operator productivity related (how to improve operations) area/usability Usability related kind/enhancement Enhancement, improvement, extension needs/review size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2021
@stoyanr stoyanr marked this pull request as draft June 1, 2021 16:05
@stoyanr
Copy link
Contributor Author

stoyanr commented Jun 1, 2021

/invite @BeckerMax @timebertt @rfranzke Even though this is still draft (unit tests are missing), could you please take a look and let me know if you are ok with the overall approach, having in mind the manual effort mentioned in "special notes" above?

@rfranzke
Copy link
Member

rfranzke commented Jun 2, 2021

/assign

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.

I think the approach is fine, I had something similar in mind. Thank you!

pkg/apis/core/validation/shoot.go Outdated Show resolved Hide resolved
pkg/utils/kubernetes/featuregates.go Show resolved Hide resolved
pkg/utils/version/version.go Outdated Show resolved Hide resolved
pkg/utils/kubernetes/featuregates.go Outdated Show resolved Hide resolved
pkg/utils/kubernetes/featuregates.go Outdated Show resolved Hide resolved
pkg/utils/kubernetes/featuregates.go Outdated Show resolved Hide resolved
pkg/utils/kubernetes/featuregates.go Outdated Show resolved Hide resolved
@stoyanr stoyanr force-pushed the add-feature-gates-validation branch from facad4e to 2b8fb41 Compare June 2, 2021 16:12
@gardener-robot gardener-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 2, 2021
@stoyanr stoyanr marked this pull request as ready for review June 2, 2021 16:24
@stoyanr
Copy link
Contributor Author

stoyanr commented Jun 2, 2021

/cc @BeckerMax @timebertt @rfranzke I consider this PR ready for review now.

Note: I compare the new map prepared as described above with the one I had previously (based on the docu). Most of the differences were exactly what I would expect, except:

  • There are a few feature gates in the code that are completely missing from the docu (marked with // Missing from docu?)
  • There are 2 feature gates that have completely different min versions in the docu than what's in the code (marked with // Docu says 1.*. For example, PodDisruptionBudget is mentioned in the docu to appear as alpha in 1.3, however in the code it really only appears in 1.17. I double and triple-checked this, there are no kube_features.go or similar files I could have missed in old releases, the string "PodDisruptionBudget" is simply not used as a feature gate before 1.17 (although the feature itself is definitely much older).

In both cases, I assume the docu is wrong and the code is right. If you have time, you might want to take a look at this, I could still be making a mistake somewhere.

@gardener-robot
Copy link

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

@stoyanr
Copy link
Contributor Author

stoyanr commented Jun 4, 2021

Addressed all feedback so far in a separate commit.
/squash

@gardener-robot
Copy link

@stoyanr You have pull request review with status CHANGES_REQUESTED, please check

1 similar comment
@gardener-robot
Copy link

@stoyanr You have pull request review with status CHANGES_REQUESTED, please check

@BeckerMax
Copy link
Contributor

BeckerMax commented Jun 7, 2021

WDYT - should the release responsible check if the feature gates where updated if needed?

@gardener gardener deleted a comment from gardener-robot Jun 7, 2021
@rfranzke
Copy link
Member

rfranzke commented Jun 7, 2021

WDYT - should the release responsible check if the feature gates where updated if needed?

No, the it's not needed, usually feature gates are only added/deleted with new minor Kubernetes version. Hence, there is no necessity for the Gardener release responsible to check the feature gates when a new Gardener version is validated. It's only needed when support for a new Kubernetes version is contributed (e.g., #3860 or similar). :)

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

Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm

@timebertt timebertt merged commit 5800108 into gardener:master Jun 8, 2021
@stoyanr stoyanr deleted the add-feature-gates-validation branch June 8, 2021 06:52
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
* Add validation of Kubernetes feature gates

* Add validation of Kubernetes feature gates code review changes
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
* Add validation of Kubernetes feature gates

* Add validation of Kubernetes feature gates code review changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ops-productivity Operator productivity related (how to improve operations) area/usability Usability related kind/enhancement Enhancement, improvement, extension 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.

Validate feature flags upon Shoot K8s version update
9 participants