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.

Sign up for GitHub

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

🐛 Allow KCP to Update when CoreDNS version doesn't change #5986

Merged
merged 1 commit into from
Jan 27, 2022
Merged

🐛 Allow KCP to Update when CoreDNS version doesn't change #5986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,11 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane)
)
return allErrs
}

// If the versions are equal return here without error.
// This allows an upgrade where the version of CoreDNS in use is not supported by the migration tool.
if toVersion.Equals(fromVersion) {
return allErrs
}
if err := migration.ValidUpMigration(fromVersion.String(), toVersion.String()); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Based on the log messages I don't think that's true:
https://storage.googleapis.com/kubernetes-jenkins/logs/periodic-cluster-api-e2e-workload-upgrade-1-23-latest-release-1-0/1483442872060481536/artifacts/clusters/bootstrap/controllers/capi-kubeadm-control-plane-controller-manager/capi-kubeadm-control-plane-controller-manager-744575bddc-qwdqp/manager.log

Copy link
Member

@sbueringer sbueringer Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Would be nice if you can also adjust the error message in l.509:

fmt.Sprintf("cannot migrate CoreDNS from '%v' to '%v': %v", fromVersion, toVersion, err),

Feels way easier to read like that to me :)

Copy link
Member

Choose a reason for hiding this comment

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

I think if we want to be able to support "no-ops" regarding CoreDNS version we can only use the validation of the migration tool here when fromVersion and toVersion are not equal

Copy link
Member

Choose a reason for hiding this comment

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

Agreed
The behavior of ValidUpMigration is kind of subtle because it handles fromVersion==ToVersion but only if the version is known (I got this digging into the code; also the inner error in the failing jobs is "start version '1.8.6' not supported").

So, if I got it right, what we want to do here is to support upgrading the kubernetes version of Clusters created with unsupported CoreDNS version, but without changing the CoreDNS version.

In order to achieve this, IMO the simplest option is to not call ValidUpMigration if fromVersion==ToVersion; eventually in a follow-up PR we can work on a better error message the return the maximum CoreDNS version a KCP release supports (by looking at migration.ValidVersions), but let's keep this out of the table for now so we have a minimal fix that can be backported

Copy link
Member

Choose a reason for hiding this comment

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

Another thing we should consider, it to prevent users from creating clusters with unsupported CoreDNS versions; this will make things less clear and enforce the up limit of our version support matrix, but I think we should discuss this with the community first

allErrs = append(
allErrs,
Expand Down
18 changes: 18 additions & 0 deletions controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,13 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
ImageTag: "v1.6.6_foobar.2",
},
}
validUnsupportedCoreDNSVersion := dns.DeepCopy()
validUnsupportedCoreDNSVersion.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS = bootstrapv1.DNS{
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "gcr.io/capi-test",
ImageTag: "v99.99.99",
},
}

unsetCoreDNSToVersion := dns.DeepCopy()
unsetCoreDNSToVersion.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS = bootstrapv1.DNS{
Expand Down Expand Up @@ -744,6 +751,16 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
before: before,
kcp: dnsBuildTag,
},
{
name: "should succeed when using the same CoreDNS version",
killianmuldoon marked this conversation as resolved.
Show resolved Hide resolved
before: dns,
kcp: dns.DeepCopy(),
},
{
name: "should succeed when using the same CoreDNS version - not supported",
before: validUnsupportedCoreDNSVersion,
kcp: validUnsupportedCoreDNSVersion,
},
{
name: "should fail when using an invalid DNS build",
expectErr: true,
Expand All @@ -756,6 +773,7 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
before: dns,
kcp: dnsInvalidCoreDNSToVersion,
},

{
name: "should fail when making a change to the cluster config's certificatesDir",
expectErr: true,
Expand Down