-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Performs KCP version check on control plane update #3508
🐛 Performs KCP version check on control plane update #3508
Conversation
Hi @srm09. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
@vincepri Does it make sense to add logic for major versions? |
hi, would there be a way to override the behavior of this webhook and not error out on downgrade / minor skip? |
/ok-to-test |
controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
00d971c
to
c5158cb
Compare
3b84ffd
to
6f7ffc3
Compare
controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
/milestone v0.3.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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:
Approvers can indicate their approval by writing |
controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
ae835ba
to
7ebc477
Compare
This patch adds the ability to check the version of the KubeadmControlPlane on updates. The update fails if the user tries to downgrade the control plane version or skip minor versions. Signed-off-by: Sagar Muchhal <muchhals@vmware.com> Co-authored-by: Vince Prignano <vince@vincepri.com>
7ebc477
to
53cbd91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold to wait for @ncdc since he made a previous comment to change
LGTM, thanks! /hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we're restricting downgrades either with this. I was concerned when I read that in your PR description since I don't know if we want to actually do that, but the logic as is in this doesn't seem to do that.
} | ||
|
||
// since upgrades to the next minor version are allowed, irrespective of the patch version | ||
ceilVersion := semver.Version{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the ceiling but actually the first disallowed version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I make a tiny PR to address this @benmoss ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, I don't think you need to bother
What this PR does / why we need it:
This PR is needed to disallow users from downgrading the control plane versions as well as skipping minor versions during upgrades.
Which issue(s) this PR fixes:
Fixes #2578