🐛 Fix CoreDNS upgrade from v1.20 to v1.21 #4476
Conversation
k8s-ci-robot
added
cncf-cla: yes
fabriziopandini
force-pushed
the
fix-CoreDNS-1.20-1.21upgrade
branch
from
c3d4051
to
dbdb50e
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold Need more time for a review |
k8s-ci-robot
added
the
do-not-merge/hold
| if clusterConfig.ImageRepository != "" { | ||
| toImageRepository = fmt.Sprintf("%s/%s", clusterConfig.ImageRepository, coreDNSKey) | ||
| toImageRepository = strings.TrimSuffix(clusterConfig.ImageRepository, "/") |
There was a problem hiding this comment.
Do we need the trim suffix here?
There was a problem hiding this comment.
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/")
There was a problem hiding this comment.
Should we instead validate input and provide guidance to users?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Do we know how kubeadm itself handles / suffixes? (depending on how they are doing it, consistency might be a factor)
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
was just an idea, it's fine for me either way
There was a problem hiding this comment.
Could we open a follow-up PR to add validation?
There was a problem hiding this comment.
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, "/") |
There was a problem hiding this comment.
Do we need the trim suffix here?
There was a problem hiding this comment.
(see comment above)
| // Handle the renaming of the upstream image from "k8s.gcr.io/coredns" to "k8s.gcr.io/coredns/coredns" | ||
| toImageName := parsedImage.Name | ||
| if toImageRepository == "k8s.gcr.io" && toImageName == "coredns" && targetMajorMinorPatch.GTE(semver.MustParse("1.8.0")) { | ||
| toImageName = "coredns/coredns" |
There was a problem hiding this comment.
we should add some constants for these
There was a problem hiding this comment.
Similar to how kubeadm does it, probably add a comment which references the kubeadm upstream contant here https://github.com/kubernetes/kubernetes/blob/release-1.21/cmd/kubeadm/app/constants/constants.go#L331?
There was a problem hiding this comment.
Constant added
I would avoid to link to the kubeadm code, because things can change there (e.g constant order was changed during v1.21); also, kubeadm is just adapting to what happen somewhere else
fabriziopandini
force-pushed
the
fix-CoreDNS-1.20-1.21upgrade
branch
from
dbdb50e
to
4053458
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
do-not-merge/hold
|
[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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
The upstream image for CoreDNS was renamed from 'k8s.gcr.io/coredns' to
k8s.gcr.io/coredns/coredns, and this is impacting upgrades of workload clusters from v1.20 to v1.21.This PR makes KCP upgrades to account for this change.
Please note that:
); users relying on custom repository could continue to usemrepo.io/coredns` as image name (no changes required).vprefix on the ImageTag, but this should be taken care by the user when specifying the desired version in KCP.Which issue(s) this PR fixes:
Fixes #4463
/assign @vincepri
@sbueringer