Skip to content

Commit 6bbd126

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#6695 from chrischdi/pr-fix-coredns-clusterrole-update
🐛 Ensure controlplane coredns update deploys the ClusterRole if CoreDNS was already updated
2 parents 39d3a88 + 3a6a548 commit 6bbd126

File tree

2 files changed

+150
-5
lines changed

2 files changed

+150
-5
lines changed

‎controlplane/kubeadm/internal/workload_cluster_coredns.go‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,12 @@ func (w *Workload) UpdateCoreDNS(ctx context.Context, kcp *controlplanev1.Kubead
125125
return err
126126
}
127127

128+
// Update the cluster role independent of image change. Kubernetes may get updated
129+
// to v1.22 which requires updating the cluster role without image changes.
130+
if err := w.updateCoreDNSClusterRole(ctx, version, info); err != nil {
131+
return err
132+
}
133+
128134
// Return early if the from/to image is the same.
129135
if info.FromImage == info.ToImage {
130136
return nil
@@ -142,9 +148,7 @@ func (w *Workload) UpdateCoreDNS(ctx context.Context, kcp *controlplanev1.Kubead
142148
if err := w.updateCoreDNSCorefile(ctx, info); err != nil {
143149
return err
144150
}
145-
if err := w.updateCoreDNSClusterRole(ctx, version, info); err != nil {
146-
return err
147-
}
151+
148152
if err := w.updateCoreDNSDeployment(ctx, info); err != nil {
149153
return errors.Wrap(err, "unable to update coredns deployment")
150154
}
@@ -273,6 +277,19 @@ func (w *Workload) updateCoreDNSClusterRole(ctx context.Context, kubernetesVersi
273277
return nil
274278
}
275279

280+
sourceCoreDNSVersion, err := extractImageVersion(info.FromImageTag)
281+
if err != nil {
282+
return err
283+
}
284+
// Do nothing for Kubernetes > 1.22 and sourceCoreDNSVersion >= 1.8.1.
285+
// With those versions we know that the ClusterRole has already been updated,
286+
// as there must have been a previous upgrade to Kubernetes 1.22
287+
// (Kubernetes minor versions cannot be skipped) and to CoreDNS >= v1.8.1.
288+
if kubernetesVersion.GTE(semver.Version{Major: 1, Minor: 23, Patch: 0}) &&
289+
sourceCoreDNSVersion.GTE(semver.Version{Major: 1, Minor: 8, Patch: 1}) {
290+
return nil
291+
}
292+
276293
key := ctrlclient.ObjectKey{Name: coreDNSClusterRoleName, Namespace: metav1.NamespaceSystem}
277294
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
278295
currentClusterRole := &rbacv1.ClusterRole{}

‎controlplane/kubeadm/internal/workload_cluster_coredns_test.go‎

Lines changed: 130 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,16 @@ func TestUpdateCoreDNS(t *testing.T) {
108108
"Corefile": expectedCorefile,
109109
},
110110
}
111+
updatedCM := &corev1.ConfigMap{
112+
ObjectMeta: metav1.ObjectMeta{
113+
Name: coreDNSKey,
114+
Namespace: metav1.NamespaceSystem,
115+
},
116+
Data: map[string]string{
117+
"Corefile": "updated-core-file",
118+
"Corefile-backup": expectedCorefile,
119+
},
120+
}
111121
kubeadmCM := &corev1.ConfigMap{
112122
ObjectMeta: metav1.ObjectMeta{
113123
Name: kubeadmConfigKey,
@@ -124,15 +134,48 @@ func TestUpdateCoreDNS(t *testing.T) {
124134
`),
125135
},
126136
}
137+
kubeadmCM181 := &corev1.ConfigMap{
138+
ObjectMeta: metav1.ObjectMeta{
139+
Name: kubeadmConfigKey,
140+
Namespace: metav1.NamespaceSystem,
141+
},
142+
Data: map[string]string{
143+
"ClusterConfiguration": yaml.Raw(`
144+
apiServer:
145+
apiVersion: kubeadm.k8s.io/v1beta2
146+
dns:
147+
type: CoreDNS
148+
imageTag: v1.8.1
149+
imageRepository: k8s.gcr.io
150+
kind: ClusterConfiguration
151+
`),
152+
},
153+
}
154+
155+
oldCR := &rbacv1.ClusterRole{
156+
TypeMeta: metav1.TypeMeta{
157+
Kind: "ClusterRole",
158+
APIVersion: "rbac.authorization.k8s.io/v1",
159+
},
160+
ObjectMeta: metav1.ObjectMeta{
161+
Name: coreDNSClusterRoleName,
162+
},
163+
}
164+
165+
semver1191 := semver.MustParse("1.19.1")
166+
semver1221 := semver.MustParse("1.22.1")
167+
semver1230 := semver.MustParse("1.23.0")
127168

128169
tests := []struct {
129170
name string
130171
kcp *controlplanev1.KubeadmControlPlane
131172
migrator coreDNSMigrator
173+
semver semver.Version
132174
objs []client.Object
133175
expectErr bool
134176
expectUpdates bool
135177
expectImage string
178+
expectRules []rbacv1.PolicyRule
136179
}{
137180
{
138181
name: "returns early without error if skip core dns annotation is present",
@@ -150,6 +193,7 @@ func TestUpdateCoreDNS(t *testing.T) {
150193
},
151194
},
152195
},
196+
semver: semver1191,
153197
objs: []client.Object{badCM},
154198
expectErr: false,
155199
},
@@ -160,23 +204,27 @@ func TestUpdateCoreDNS(t *testing.T) {
160204
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{},
161205
},
162206
},
207+
semver: semver1191,
163208
objs: []client.Object{badCM},
164209
expectErr: false,
165210
},
166211
{
167212
name: "returns early without error if CoreDNS info is not found",
168213
kcp: validKCP,
214+
semver: semver1191,
169215
expectErr: false,
170216
},
171217
{
172218
name: "returns error if there was a problem retrieving CoreDNS info",
173219
kcp: validKCP,
220+
semver: semver1191,
174221
objs: []client.Object{badCM},
175222
expectErr: true,
176223
},
177224
{
178225
name: "returns early without error if CoreDNS fromImage == ToImage",
179226
kcp: validKCP,
227+
semver: semver1191,
180228
objs: []client.Object{depl, cm},
181229
expectErr: false,
182230
},
@@ -198,6 +246,7 @@ func TestUpdateCoreDNS(t *testing.T) {
198246
},
199247
},
200248
},
249+
semver: semver1191,
201250
objs: []client.Object{depl, cm},
202251
expectErr: true,
203252
},
@@ -218,6 +267,7 @@ func TestUpdateCoreDNS(t *testing.T) {
218267
},
219268
},
220269
},
270+
semver: semver1191,
221271
// no kubeadmConfigMap available so it will trigger an error
222272
objs: []client.Object{depl, cm},
223273
expectErr: true,
@@ -242,6 +292,7 @@ func TestUpdateCoreDNS(t *testing.T) {
242292
migrator: &fakeMigrator{
243293
migrateErr: errors.New("failed to migrate"),
244294
},
295+
semver: semver1191,
245296
objs: []client.Object{depl, cm, kubeadmCM},
246297
expectErr: true,
247298
},
@@ -265,6 +316,7 @@ func TestUpdateCoreDNS(t *testing.T) {
265316
migrator: &fakeMigrator{
266317
migratedCorefile: "updated-core-file",
267318
},
319+
semver: semver1191,
268320
objs: []client.Object{depl, cm, kubeadmCM},
269321
expectErr: false,
270322
expectUpdates: true,
@@ -290,6 +342,7 @@ func TestUpdateCoreDNS(t *testing.T) {
290342
migrator: &fakeMigrator{
291343
migratedCorefile: "updated-core-file",
292344
},
345+
semver: semver1191,
293346
objs: []client.Object{deplWithImage("k8s.gcr.io/some-repo/coredns:1.7.0"), cm, kubeadmCM},
294347
expectErr: false,
295348
expectUpdates: true,
@@ -314,6 +367,7 @@ func TestUpdateCoreDNS(t *testing.T) {
314367
migrator: &fakeMigrator{
315368
migratedCorefile: "updated-core-file",
316369
},
370+
semver: semver1191,
317371
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.6.7"), cm, kubeadmCM},
318372
expectErr: false,
319373
expectUpdates: true,
@@ -338,6 +392,7 @@ func TestUpdateCoreDNS(t *testing.T) {
338392
migrator: &fakeMigrator{
339393
migratedCorefile: "updated-core-file",
340394
},
395+
semver: semver1191,
341396
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.7.0"), cm, kubeadmCM},
342397
expectErr: false,
343398
expectUpdates: false,
@@ -361,6 +416,7 @@ func TestUpdateCoreDNS(t *testing.T) {
361416
migrator: &fakeMigrator{
362417
migratedCorefile: "updated-core-file",
363418
},
419+
semver: semver1191,
364420
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns:1.7.0"), cm, kubeadmCM},
365421
expectErr: false,
366422
expectUpdates: true,
@@ -382,12 +438,61 @@ func TestUpdateCoreDNS(t *testing.T) {
382438
},
383439
},
384440
},
441+
semver: semver1191,
385442
migrator: &fakeMigrator{
386443
migratedCorefile: "updated-core-file",
387444
},
388445
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.0"), cm, kubeadmCM},
389446
expectErr: false,
390447
expectUpdates: false,
448+
expectRules: oldCR.Rules,
449+
},
450+
{
451+
name: "upgrade from Kubernetes v1.21.x to v1.22.y and update cluster role",
452+
kcp: &controlplanev1.KubeadmControlPlane{
453+
Spec: controlplanev1.KubeadmControlPlaneSpec{
454+
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
455+
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
456+
DNS: bootstrapv1.DNS{
457+
ImageMeta: bootstrapv1.ImageMeta{
458+
ImageRepository: "k8s.gcr.io",
459+
ImageTag: "v1.8.1", // NOTE: ImageTags requires the v prefix
460+
},
461+
},
462+
},
463+
},
464+
},
465+
},
466+
migrator: &fakeMigrator{
467+
migratedCorefile: "updated-core-file",
468+
},
469+
semver: semver1221,
470+
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.1"), updatedCM, kubeadmCM181, oldCR},
471+
expectErr: false,
472+
expectUpdates: true,
473+
expectImage: "k8s.gcr.io/coredns/coredns:v1.8.1", // NOTE: ImageName has coredns/coredns
474+
expectRules: coreDNS181PolicyRules,
475+
},
476+
{
477+
name: "returns early without error if kubernetes version is >= v1.23",
478+
kcp: &controlplanev1.KubeadmControlPlane{
479+
Spec: controlplanev1.KubeadmControlPlaneSpec{
480+
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
481+
ClusterConfiguration: &bootstrapv1.ClusterConfiguration{
482+
DNS: bootstrapv1.DNS{
483+
ImageMeta: bootstrapv1.ImageMeta{
484+
ImageRepository: "k8s.gcr.io",
485+
ImageTag: "v1.8.1", // NOTE: ImageTags requires the v prefix
486+
},
487+
},
488+
},
489+
},
490+
},
491+
},
492+
semver: semver1230,
493+
objs: []client.Object{deplWithImage("k8s.gcr.io/coredns/coredns:v1.8.1"), updatedCM, kubeadmCM181},
494+
expectErr: false,
495+
expectRules: oldCR.Rules,
391496
},
392497
}
393498

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

417522
if tt.expectErr {
418523
g.Expect(err).To(HaveOccurred())
@@ -455,6 +560,15 @@ func TestUpdateCoreDNS(t *testing.T) {
455560
g.Expect(env.Get(ctx, client.ObjectKey{Name: coreDNSKey, Namespace: metav1.NamespaceSystem}, &actualDeployment)).To(Succeed())
456561
return actualDeployment.Spec.Template.Spec.Containers[0].Image
457562
}, "5s").Should(Equal(tt.expectImage))
563+
564+
// assert CoreDNS ClusterRole
565+
if tt.expectRules != nil {
566+
var actualClusterRole rbacv1.ClusterRole
567+
g.Eventually(func() []rbacv1.PolicyRule {
568+
g.Expect(env.Get(ctx, client.ObjectKey{Name: coreDNSClusterRoleName}, &actualClusterRole)).To(Succeed())
569+
return actualClusterRole.Rules
570+
}, "5s").Should(Equal(tt.expectRules))
571+
}
458572
}
459573
})
460574
}
@@ -550,6 +664,7 @@ func TestUpdateCoreDNSClusterRole(t *testing.T) {
550664
name string
551665
kubernetesVersion semver.Version
552666
coreDNSVersion string
667+
sourceCoreDNSVersion string
553668
coreDNSPolicyRules []rbacv1.PolicyRule
554669
expectErr bool
555670
expectCoreDNSPolicyRules []rbacv1.PolicyRule
@@ -568,38 +683,51 @@ func TestUpdateCoreDNSClusterRole(t *testing.T) {
568683
coreDNSPolicyRules: coreDNS180PolicyRules,
569684
expectCoreDNSPolicyRules: coreDNS180PolicyRules,
570685
},
686+
{
687+
name: "does not patch ClusterRole: Kubernetes > 1.22 && CoreDNS >= 1.8.1",
688+
kubernetesVersion: semver.Version{Major: 1, Minor: 23, Patch: 0},
689+
coreDNSVersion: "1.8.1",
690+
sourceCoreDNSVersion: "1.8.1",
691+
coreDNSPolicyRules: coreDNS180PolicyRules,
692+
expectCoreDNSPolicyRules: coreDNS180PolicyRules,
693+
},
571694
{
572695
name: "does not patch ClusterRole: CoreDNS < 1.8.1",
573696
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 0},
574697
coreDNSVersion: "1.8.0",
698+
sourceCoreDNSVersion: "1.7.0",
575699
coreDNSPolicyRules: coreDNS180PolicyRules,
576700
expectCoreDNSPolicyRules: coreDNS180PolicyRules,
577701
},
578702
{
579703
name: "patch ClusterRole: Kubernetes == 1.22 alpha and CoreDNS == 1.8.1",
580704
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 0, Pre: []semver.PRVersion{{VersionStr: "alpha"}}},
581705
coreDNSVersion: "1.8.1",
706+
sourceCoreDNSVersion: "1.8.1",
582707
coreDNSPolicyRules: coreDNS180PolicyRules,
583708
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
584709
},
585710
{
586711
name: "patch ClusterRole: Kubernetes == 1.22 and CoreDNS == 1.8.1",
587712
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 0},
588713
coreDNSVersion: "1.8.1",
714+
sourceCoreDNSVersion: "1.8.1",
589715
coreDNSPolicyRules: coreDNS180PolicyRules,
590716
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
591717
},
592718
{
593719
name: "patch ClusterRole: Kubernetes > 1.22 and CoreDNS > 1.8.1",
594720
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 2},
595721
coreDNSVersion: "1.8.5",
722+
sourceCoreDNSVersion: "1.8.1",
596723
coreDNSPolicyRules: coreDNS180PolicyRules,
597724
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
598725
},
599726
{
600727
name: "patch ClusterRole: Kubernetes > 1.22 and CoreDNS > 1.8.1: no-op",
601728
kubernetesVersion: semver.Version{Major: 1, Minor: 22, Patch: 2},
602729
coreDNSVersion: "1.8.5",
730+
sourceCoreDNSVersion: "1.8.5",
603731
coreDNSPolicyRules: coreDNS181PolicyRules,
604732
expectCoreDNSPolicyRules: coreDNS181PolicyRules,
605733
},
@@ -622,7 +750,7 @@ func TestUpdateCoreDNSClusterRole(t *testing.T) {
622750
Client: fakeClient,
623751
}
624752

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

627755
if tt.expectErr {
628756
g.Expect(err).To(HaveOccurred())

0 commit comments

Comments
 (0)