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 for KubeadmConfigSpec defaulting #8147

Open
sbueringer opened this issue Feb 22, 2023 · 7 comments
Open

Add validation for KubeadmConfigSpec defaulting #8147

sbueringer opened this issue Feb 22, 2023 · 7 comments
Labels
area/control-plane Issues or PRs related to control-plane lifecycle management help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sbueringer
Copy link
Member

sbueringer commented Feb 22, 2023

We repeatedly had the issue that we added new defaulting to KubeadmConfigSpec fields, which lead to rollouts in KCP after CAPI upgrade:

The goal of this issue is to add some sort of validation that we can run as part of presubmits to ensure this doesn't happen again.

We basically want to validate that all defaulting that we implement for KubeadmConfigSpec is done in DefaultKubeadmConfigSpec and not via OpenAPI.

Some ideas:

  • Add a defaulting unit test:
    • It would compare results of local KubeadmConfig defaulting with KubeadmConfig.Default() vs. KubeadmConfig create with envtest
    • => If there is a difference => there was additional defaulting via OpenAPI
  • Introduce a new linter. Options:
    • check for default field in final CRD YAMLs
    • check for // +kubebuilder:default marker in KubeadmConfigSpec struct and below (controller-tools based linter)

/kind feature
/area control-plane

@sbueringer sbueringer self-assigned this Feb 22, 2023
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. area/control-plane Issues or PRs related to control-plane lifecycle management needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 22, 2023
@sbueringer
Copy link
Member Author

@vincepri @fabriziopandini @ykakarap

The unit tests sounds sufficient and pretty straightforward to me, WDYT?

@sbueringer sbueringer changed the title Add validation for KCP validation Add validation for KubeadmConfigSpec defaulting Feb 22, 2023
@ykakarap
Copy link
Contributor

Yup, unit test looks like a good idea. +1.
Since we will use envtest with the defaulting webhook it will closely reflect the real world case too.

@fabriziopandini
Copy link
Member

/triage accepted
/help
💯 this will be very useful!

@k8s-ci-robot
Copy link
Contributor

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/triage accepted
/help
💯 this will be very useful!

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.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 23, 2023
@sbueringer sbueringer added this to the v1.5 milestone Feb 24, 2023
@sbueringer
Copy link
Member Author

Let's see if some takes this over. I added it to the v1.5 milestone to ensure we'll implement it until then.

I would take over if nobody volunteers until then.

@sbueringer sbueringer added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Feb 24, 2023
@sbueringer sbueringer removed their assignment Feb 26, 2023
@sbueringer sbueringer removed this from the v1.5 milestone Jun 21, 2023
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jun 20, 2024
@sbueringer
Copy link
Member Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Issues or PRs related to control-plane lifecycle management help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

5 participants