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

Jump to bottom

🐛 Fix CoreDNS upgrade from v1.20 to v1.21 #4476

Merged
merged 1 commit into from
Apr 14, 2021
Merged

🐛 Fix CoreDNS upgrade from v1.20 to v1.21 #4476

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
30 changes: 21 additions & 9 deletions controlplane/kubeadm/internal/workload_cluster_coredns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ package internal
import (
"context"
"fmt"
"strings"

"github.com/blang/semver"
"sigs.k8s.io/cluster-api/util/version"

"github.com/coredns/corefile-migration/migration"
Expand All @@ -40,6 +42,10 @@ const (
corefileBackupKey = "Corefile-backup"
coreDNSKey = "coredns"
coreDNSVolumeKey = "config-volume"

kubernetesImageRepository = "k8s.gcr.io"
oldCoreDNSImageName = "coredns"
coreDNSImageName = "coredns/coredns"
)

type coreDNSMigrator interface {
Expand Down Expand Up @@ -156,12 +162,12 @@ func (w *Workload) getCoreDNSInfo(ctx context.Context, clusterConfig *bootstrapv
}

// Handle imageRepository.
toImageRepository := fmt.Sprintf("%s/%s", parsedImage.Repository, parsedImage.Name)
toImageRepository := parsedImage.Repository
if clusterConfig.ImageRepository != "" {
toImageRepository = fmt.Sprintf("%s/%s", clusterConfig.ImageRepository, coreDNSKey)
toImageRepository = strings.TrimSuffix(clusterConfig.ImageRepository, "/")
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the trim suffix here?

Copy link
Member Author

Choose a reason for hiding this comment

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

given that the image repository is provider by the user, and that down we are concatenating it with image name and tag, I thought that removing the suffix makes the concat operation more robust (it will work with the user providing "k8s.gcr.io" but also "k8s.gcr.io/")

Copy link
Member

Choose a reason for hiding this comment

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

Should we instead validate input and provide guidance to users?

Copy link
Member Author

@fabriziopandini fabriziopandini Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We can add validation and documentation as well (in another PR)
Nevertheless, I don't think this extra caution will hurt, but if you prefer I can remove it; it is not core to the fix we are discussing here

Copy link
Member

Choose a reason for hiding this comment

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

Do we know how kubeadm itself handles / suffixes? (depending on how they are doing it, consistency might be a factor)

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK kubeadm is not doing any validation on ImageTag/ImageRepository, nor we are doing in CAPI
Also kubeadm, is not involved in immutable upgrades.

I just thought it making the operation more robust, but happy to remove it if it is blocking this PR...

Copy link
Member

Choose a reason for hiding this comment

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

was just an idea, it's fine for me either way

Copy link
Member

Choose a reason for hiding this comment

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

Could we open a follow-up PR to add validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding validation could have some implications. I opened an issue to discuss them #4482

}
if clusterConfig.DNS.ImageRepository != "" {
toImageRepository = fmt.Sprintf("%s/%s", clusterConfig.DNS.ImageRepository, coreDNSKey)
toImageRepository = strings.TrimSuffix(clusterConfig.DNS.ImageRepository, "/")
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the trim suffix here?

Copy link
Member Author

Choose a reason for hiding this comment

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

(see comment above)

}

// Handle imageTag.
Expand All @@ -181,15 +187,21 @@ func (w *Workload) getCoreDNSInfo(ctx context.Context, clusterConfig *bootstrapv
return nil, err
}

// Handle the renaming of the upstream image from "k8s.gcr.io/coredns" to "k8s.gcr.io/coredns/coredns"
toImageName := parsedImage.Name
if toImageRepository == kubernetesImageRepository && toImageName == oldCoreDNSImageName && targetMajorMinorPatch.GTE(semver.MustParse("1.8.0")) {
toImageName = coreDNSImageName
}

return &coreDNSInfo{
Corefile: corefile,
Deployment: deployment,
CurrentMajorMinorPatch: currentMajorMinorPatch,
TargetMajorMinorPatch: targetMajorMinorPatch,
CurrentMajorMinorPatch: currentMajorMinorPatch.String(),
TargetMajorMinorPatch: targetMajorMinorPatch.String(),
FromImageTag: parsedImage.Tag,
ToImageTag: toImageTag,
FromImage: container.Image,
ToImage: fmt.Sprintf("%s:%s", toImageRepository, toImageTag),
ToImage: fmt.Sprintf("%s/%s:%s", toImageRepository, toImageName, toImageTag),
}, nil
}

Expand Down Expand Up @@ -298,12 +310,12 @@ func patchCoreDNSDeploymentImage(deployment *appsv1.Deployment, image string) {
}
}

func extractImageVersion(tag string) (string, error) {
func extractImageVersion(tag string) (semver.Version, error) {
ver, err := version.ParseMajorMinorPatchTolerant(tag)
if err != nil {
return "", err
return semver.Version{}, err
}
return fmt.Sprintf("%d.%d.%d", ver.Major, ver.Minor, ver.Patch), nil
return ver, nil
}

// validateCoreDNSImageTag returns error if the versions don't meet requirements.
Expand Down
142 changes: 137 additions & 5 deletions controlplane/kubeadm/internal/workload_cluster_coredns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestUpdateCoreDNS(t *testing.T) {
"BadCoreFileKey": "",
},
}
expectedImage := "k8s.gcr.io/some-folder/coredns:1.6.2"

depl := &appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
Kind: "Deployment",
Expand All @@ -82,7 +82,7 @@ func TestUpdateCoreDNS(t *testing.T) {
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: coreDNSKey,
Image: expectedImage,
Image: "k8s.gcr.io/some-folder/coredns:1.6.2",
vincepri marked this conversation as resolved.
Show resolved Hide resolved
}},
},
},
Expand All @@ -92,6 +92,12 @@ func TestUpdateCoreDNS(t *testing.T) {
},
}

deplWithImage := func(image string) *appsv1.Deployment {
d := depl.DeepCopy()
d.Spec.Template.Spec.Containers[0].Image = image
return d
}

expectedCorefile := "coredns-core-file"
cm := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -125,6 +131,7 @@ kind: ClusterConfiguration
objs []client.Object
expectErr bool
expectUpdates bool
expectImage string
}{
{
name: "returns early without error if skip core dns annotation is present",
Expand Down Expand Up @@ -282,6 +289,131 @@ kind: ClusterConfiguration
objs: []client.Object{depl, cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
expectImage: "k8s.gcr.io/some-repo/coredns:1.7.2",
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
},
{
name: "updates everything successfully to v1.8.0 with a custom repo should not change the image name",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: cabpkv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
Type: bootstrapv1.CoreDNS,
ImageMeta: bootstrapv1.ImageMeta{
// provide an newer image to update to
ImageRepository: "k8s.gcr.io/some-repo",
ImageTag: "1.8.0",
},
},
},
},
},
},
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
objs: []client.Object{deplWithImage("k8s.gcr.io/some-repo/coredns:1.7.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
expectImage: "k8s.gcr.io/some-repo/coredns:1.8.0",
},
{
name: "kubeadm defaults, upgrade from Kubernetes v1.18.x to v1.19.y (from k8s.gcr.io/coredns:1.6.7 to k8s.gcr.io/coredns:1.7.0)",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: cabpkv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
Type: bootstrapv1.CoreDNS,
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "k8s.gcr.io",
ImageTag: "1.7.0",
},
},
},
},
},
},
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.6.7"), cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
expectImage: "k8s.gcr.io/coredns:1.7.0",
},
{
name: "kubeadm defaults, upgrade from Kubernetes v1.19.x to v1.20.y (stay on k8s.gcr.io/coredns:1.7.0)",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: cabpkv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
Type: bootstrapv1.CoreDNS,
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "k8s.gcr.io",
ImageTag: "1.7.0",
},
},
},
},
},
},
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.7.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: false,
},
{
name: "kubeadm defaults, upgrade from Kubernetes v1.20.x to v1.21.y (from k8s.gcr.io/coredns:1.7.0 to k8s.gcr.io/coredns/coredns:v1.8.0)",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: cabpkv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
Type: bootstrapv1.CoreDNS,
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "k8s.gcr.io",
ImageTag: "v1.8.0", // NOTE: ImageTags requires the v prefix
},
},
},
},
},
},
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.7.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
expectImage: "k8s.gcr.io/coredns/coredns:v1.8.0", // NOTE: ImageName has coredns/coredns
},
{
name: "kubeadm defaults, upgrade from Kubernetes v1.21.x to v1.22.y (stay on k8s.gcr.io/coredns/coredns:v1.8.0)",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: cabpkv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
Type: bootstrapv1.CoreDNS,
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "k8s.gcr.io",
ImageTag: "v1.8.0", // NOTE: ImageTags requires the v prefix
},
},
},
},
},
},
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: false,
},
}

Expand Down Expand Up @@ -336,8 +468,8 @@ kind: ClusterConfiguration
// assert kubeadmConfigMap
var expectedKubeadmConfigMap corev1.ConfigMap
g.Expect(testEnv.Get(ctx, ctrlclient.ObjectKey{Name: kubeadmConfigKey, Namespace: metav1.NamespaceSystem}, &expectedKubeadmConfigMap)).To(Succeed())
g.Expect(expectedKubeadmConfigMap.Data).To(HaveKeyWithValue("ClusterConfiguration", ContainSubstring("1.7.2")))
g.Expect(expectedKubeadmConfigMap.Data).To(HaveKeyWithValue("ClusterConfiguration", ContainSubstring("k8s.gcr.io/some-repo")))
g.Expect(expectedKubeadmConfigMap.Data).To(HaveKeyWithValue("ClusterConfiguration", ContainSubstring(tt.kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageTag)))
g.Expect(expectedKubeadmConfigMap.Data).To(HaveKeyWithValue("ClusterConfiguration", ContainSubstring(tt.kcp.Spec.KubeadmConfigSpec.ClusterConfiguration.DNS.ImageRepository)))

// assert CoreDNS corefile
var expectedConfigMap corev1.ConfigMap
Expand All @@ -351,7 +483,7 @@ kind: ClusterConfiguration
g.Eventually(func() string {
g.Expect(testEnv.Get(ctx, ctrlclient.ObjectKey{Name: coreDNSKey, Namespace: metav1.NamespaceSystem}, &actualDeployment)).To(Succeed())
return actualDeployment.Spec.Template.Spec.Containers[0].Image
}, "5s").Should(Equal("k8s.gcr.io/some-repo/coredns:1.7.2"))
}, "5s").Should(Equal(tt.expectImage))
}
})
}
Expand Down