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

[release-1.1] 🐛 Ensure controlplane coredns update deploys the ClusterRole if CoreDNS was already updated #6760

Merged
merged 1 commit into from
Jun 28, 2022
Merged

[release-1.1] 🐛 Ensure controlplane coredns update deploys the ClusterRole if CoreDNS was already updated #6760

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
23 changes: 20 additions & 3 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 @@ -125,6 +125,12 @@ func (w *Workload) UpdateCoreDNS(ctx context.Context, kcp *controlplanev1.Kubead
return err
}

// Update the cluster role independent of image change. Kubernetes may get updated
// to v1.22 which requires updating the cluster role without image changes.
if err := w.updateCoreDNSClusterRole(ctx, version, info); err != nil {
return err
}

// Return early if the from/to image is the same.
if info.FromImage == info.ToImage {
return nil
Expand All @@ -142,9 +148,7 @@ func (w *Workload) UpdateCoreDNS(ctx context.Context, kcp *controlplanev1.Kubead
if err := w.updateCoreDNSCorefile(ctx, info); err != nil {
return err
}
if err := w.updateCoreDNSClusterRole(ctx, version, info); err != nil {
return err
}

if err := w.updateCoreDNSDeployment(ctx, info); err != nil {
return errors.Wrap(err, "unable to update coredns deployment")
}
Expand Down Expand Up @@ -273,6 +277,19 @@ func (w *Workload) updateCoreDNSClusterRole(ctx context.Context, kubernetesVersi
return nil
}

sourceCoreDNSVersion, err := extractImageVersion(info.FromImageTag)
if err != nil {
return err
}
// Do nothing for Kubernetes > 1.22 and sourceCoreDNSVersion >= 1.8.1.
// With those versions we know that the ClusterRole has already been updated,
// as there must have been a previous upgrade to Kubernetes 1.22
// (Kubernetes minor versions cannot be skipped) and to CoreDNS >= v1.8.1.
if kubernetesVersion.GTE(semver.Version{Major: 1, Minor: 23, Patch: 0}) &&
sourceCoreDNSVersion.GTE(semver.Version{Major: 1, Minor: 8, Patch: 1}) {
return nil
}

key := ctrlclient.ObjectKey{Name: coreDNSClusterRoleName, Namespace: metav1.NamespaceSystem}
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
currentClusterRole := &rbacv1.ClusterRole{}
Expand Down
131 changes: 129 additions & 2 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 @@ -108,6 +108,16 @@ func TestUpdateCoreDNS(t *testing.T) {
"Corefile": expectedCorefile,
},
}
updatedCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: coreDNSKey,
Namespace: metav1.NamespaceSystem,
},
Data: map[string]string{
"Corefile": "updated-core-file",
"Corefile-backup": expectedCorefile,
},
}
kubeadmCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: kubeadmConfigKey,
Expand All @@ -124,15 +134,48 @@ func TestUpdateCoreDNS(t *testing.T) {
`),
},
}
kubeadmCM181 := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: kubeadmConfigKey,
Namespace: metav1.NamespaceSystem,
},
Data: map[string]string{
"ClusterConfiguration": yaml.Raw(`
apiServer:
apiVersion: kubeadm.k8s.io/v1beta2
dns:
type: CoreDNS
imageTag: v1.8.1
imageRepository: k8s.gcr.io
kind: ClusterConfiguration
`),
},
}

oldCR := &rbacv1.ClusterRole{
TypeMeta: metav1.TypeMeta{
Kind: "ClusterRole",
APIVersion: "rbac.authorization.k8s.io/v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: coreDNSClusterRoleName,
},
}

semver1191 := semver.MustParse("1.19.1")
semver1221 := semver.MustParse("1.22.1")
semver1230 := semver.MustParse("1.23.0")

tests := []struct {
name string
kcp *controlplanev1.KubeadmControlPlane
migrator coreDNSMigrator
semver semver.Version
objs []client.Object
expectErr bool
expectUpdates bool
expectImage string
expectRules []rbacv1.PolicyRule
}{
{
name: "returns early without error if skip core dns annotation is present",
Expand All @@ -150,6 +193,7 @@ func TestUpdateCoreDNS(t *testing.T) {
},
},
},
semver: semver1191,
objs: []client.Object{badCM},
expectErr: false,
},
Expand All @@ -160,23 +204,27 @@ func TestUpdateCoreDNS(t *testing.T) {
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{},
},
},
semver: semver1191,
objs: []client.Object{badCM},
expectErr: false,
},
{
name: "returns early without error if CoreDNS info is not found",
kcp: validKCP,
semver: semver1191,
expectErr: false,
},
{
name: "returns error if there was a problem retrieving CoreDNS info",
kcp: validKCP,
semver: semver1191,
objs: []client.Object{badCM},
expectErr: true,
},
{
name: "returns early without error if CoreDNS fromImage == ToImage",
kcp: validKCP,
semver: semver1191,
objs: []client.Object{depl, cm},
expectErr: false,
},
Expand All @@ -198,6 +246,7 @@ func TestUpdateCoreDNS(t *testing.T) {
},
},
},
semver: semver1191,
objs: []client.Object{depl, cm},
expectErr: true,
},
Expand All @@ -218,6 +267,7 @@ func TestUpdateCoreDNS(t *testing.T) {
},
},
},
semver: semver1191,
// no kubeadmConfigMap available so it will trigger an error
objs: []client.Object{depl, cm},
expectErr: true,
Expand All @@ -242,6 +292,7 @@ func TestUpdateCoreDNS(t *testing.T) {
migrator: &fakeMigrator{
migrateErr: errors.New("failed to migrate"),
},
semver: semver1191,
objs: []client.Object{depl, cm, kubeadmCM},
expectErr: true,
},
Expand All @@ -265,6 +316,7 @@ func TestUpdateCoreDNS(t *testing.T) {
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
semver: semver1191,
objs: []client.Object{depl, cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
Expand All @@ -290,6 +342,7 @@ func TestUpdateCoreDNS(t *testing.T) {
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
semver: semver1191,
objs: []client.Object{deplWithImage("k8s.gcr.io/some-repo/coredns:1.7.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
Expand All @@ -314,6 +367,7 @@ func TestUpdateCoreDNS(t *testing.T) {
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
semver: semver1191,
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.6.7"), cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
Expand All @@ -338,6 +392,7 @@ func TestUpdateCoreDNS(t *testing.T) {
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
semver: semver1191,
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.7.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: false,
Expand All @@ -361,6 +416,7 @@ func TestUpdateCoreDNS(t *testing.T) {
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
semver: semver1191,
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.7.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: true,
Expand All @@ -382,12 +438,61 @@ func TestUpdateCoreDNS(t *testing.T) {
},
},
},
semver: semver1191,
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.0"), cm, kubeadmCM},
expectErr: false,
expectUpdates: false,
expectRules: oldCR.Rules,
},
{
name: "upgrade from Kubernetes v1.21.x to v1.22.y and update cluster role",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "k8s.gcr.io",
ImageTag: "v1.8.1", // NOTE: ImageTags requires the v prefix
},
},
},
},
},
},
migrator: &fakeMigrator{
migratedCorefile: "updated-core-file",
},
semver: semver1221,
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.1"), updatedCM, kubeadmCM181, oldCR},
expectErr: false,
expectUpdates: true,
expectImage: "k8s.gcr.io/coredns/coredns:v1.8.1", // NOTE: ImageName has coredns/coredns
expectRules: coreDNS181PolicyRules,
},
{
name: "returns early without error if kubernetes version is >= v1.23",
kcp: &controlplanev1.KubeadmControlPlane{
Spec: controlplanev1.KubeadmControlPlaneSpec{
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
DNS: bootstrapv1.DNS{
ImageMeta: bootstrapv1.ImageMeta{
ImageRepository: "k8s.gcr.io",
ImageTag: "v1.8.1", // NOTE: ImageTags requires the v prefix
},
},
},
},
},
},
semver: semver1230,
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.1"), updatedCM, kubeadmCM181},
expectErr: false,
expectRules: oldCR.Rules,
},
}

Expand All @@ -412,7 +517,7 @@ func TestUpdateCoreDNS(t *testing.T) {
Client: env.GetClient(),
CoreDNSMigrator: tt.migrator,
}
err := w.UpdateCoreDNS(ctx, tt.kcp, semver.MustParse("1.19.1"))
err := w.UpdateCoreDNS(ctx, tt.kcp, tt.semver)

if tt.expectErr {
g.Expect(err).To(HaveOccurred())
Expand Down Expand Up @@ -455,6 +560,15 @@ func TestUpdateCoreDNS(t *testing.T) {
g.Expect(env.Get(ctx, client.ObjectKey{Name: coreDNSKey, Namespace: metav1.NamespaceSystem}, &actualDeployment)).To(Succeed())
return actualDeployment.Spec.Template.Spec.Containers[0].Image
}, "5s").Should(Equal(tt.expectImage))

// assert CoreDNS ClusterRole
if tt.expectRules != nil {
var actualClusterRole rbacv1.ClusterRole
g.Eventually(func() []rbacv1.PolicyRule {
g.Expect(env.Get(ctx, client.ObjectKey{Name: coreDNSClusterRoleName}, &actualClusterRole)).To(Succeed())
return actualClusterRole.Rules
}, "5s").Should(Equal(tt.expectRules))
}
}
})
}
Expand Down Expand Up @@ -550,6 +664,7 @@ func TestUpdateCoreDNSClusterRole(t *testing.T) {
name string
kubernetesVersion semver.Version
coreDNSVersion string
sourceCoreDNSVersion string
coreDNSPolicyRules []rbacv1.PolicyRule
expectErr bool
expectCoreDNSPolicyRules []rbacv1.PolicyRule
Expand All @@ -568,31 +683,43 @@ func TestUpdateCoreDNSClusterRole(t *testing.T) {
coreDNSPolicyRules: coreDNS180PolicyRules,
expectCoreDNSPolicyRules: coreDNS180PolicyRules,
},
{
name: "does not patch ClusterRole: Kubernetes > 1.22 && CoreDNS >= 1.8.1",
kubernetesVersion: semver.Version{Major: 1, Minor: 23, Patch: 0},
coreDNSVersion: "1.8.1",
sourceCoreDNSVersion: "1.8.1",
coreDNSPolicyRules: coreDNS180PolicyRules,
expectCoreDNSPolicyRules: coreDNS180PolicyRules,
},
{
name: "does not patch ClusterRole: CoreDNS < 1.8.1",
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 0},
coreDNSVersion: "1.8.0",
sourceCoreDNSVersion: "1.7.0",
coreDNSPolicyRules: coreDNS180PolicyRules,
expectCoreDNSPolicyRules: coreDNS180PolicyRules,
},
{
name: "patch ClusterRole: Kubernetes == 1.22 and CoreDNS == 1.8.1",
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 0},
coreDNSVersion: "1.8.1",
sourceCoreDNSVersion: "1.8.1",
coreDNSPolicyRules: coreDNS180PolicyRules,
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
},
{
name: "patch ClusterRole: Kubernetes > 1.22 and CoreDNS > 1.8.1",
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 2},
coreDNSVersion: "1.8.5",
sourceCoreDNSVersion: "1.8.1",
coreDNSPolicyRules: coreDNS180PolicyRules,
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
},
{
name: "patch ClusterRole: Kubernetes > 1.22 and CoreDNS > 1.8.1: no-op",
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 2},
coreDNSVersion: "1.8.5",
sourceCoreDNSVersion: "1.8.5",
coreDNSPolicyRules: coreDNS181PolicyRules,
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
},
Expand All @@ -615,7 +742,7 @@ func TestUpdateCoreDNSClusterRole(t *testing.T) {
Client: fakeClient,
}

err := w.updateCoreDNSClusterRole(ctx, tt.kubernetesVersion, &coreDNSInfo{ToImageTag: tt.coreDNSVersion})
err := w.updateCoreDNSClusterRole(ctx, tt.kubernetesVersion, &coreDNSInfo{ToImageTag: tt.coreDNSVersion, FromImageTag: tt.sourceCoreDNSVersion})

if tt.expectErr {
g.Expect(err).To(HaveOccurred())
Expand Down