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.

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

Update kube-dns container images. #68430

Merged
merged 1 commit into from
Sep 19, 2018
Merged

Conversation

prameshj
Copy link
Contributor

@prameshj prameshj commented Sep 8, 2018

What this PR does / why we need it:
This fixes an issue where SRV records were incorrectly being compressed. Go 1.11 added checks to reject compressed SRV records https://golang.org/cl/100055
kubeDNS image needs to be updated in order for those clients to work.

I verified manually that the new kubedns version works with Go 1.11 clients
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

New kubeDNS image fixes an issue where SRV records were incorrectly being compressed. Added manifest file for multiple arch images.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 8, 2018
@prameshj
Copy link
Contributor Author

prameshj commented Sep 8, 2018

/assign @bowei

@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 8, 2018
@bowei
Copy link
Member

bowei commented Sep 8, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 8, 2018
@bowei
Copy link
Member

bowei commented Sep 8, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 8, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@rajansandeep
Copy link
Contributor

Does this need to be updated for kubeadm as well?

@bowei
Copy link
Member

bowei commented Sep 9, 2018

/milestone v1.12

@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Sep 9, 2018
@dims
Copy link
Member

dims commented Sep 9, 2018

/sig network

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Sep 9, 2018
@neolit123
Copy link
Member

Does this need to be updated for kubeadm as well?

for ref:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/constants/constants.go#L302

i see the multi arch images are already up at gcr, but there are no manifest lists in place.

also this PR only bumps for amd64:

image: k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.11

@dims
Copy link
Member

dims commented Sep 9, 2018

@neolit123 if i remember right since CoreDNS feature was marked GA and kube-dns was kinda deprecated ... the build harness was not updated to generate manifests:
https://github.com/kubernetes/dns/tree/master/images/dnsmasq

@mkumatag can you please confirm?

@bowei
Copy link
Member

bowei commented Sep 9, 2018

kube-dns is not deprecated yet.

@dims
Copy link
Member

dims commented Sep 9, 2018

@bowei sorry i used wrong words :)

@mkumatag
Copy link
Member

@neolit123 if i remember right since CoreDNS feature was marked GA and kube-dns was kinda deprecated ... the build harness was not updated to generate manifests:
https://github.com/kubernetes/dns/tree/master/images/dnsmasq

@mkumatag can you please confirm?

you are right! kube-dns is not yet multi-arched.

@neolit123
Copy link
Member

kube-dns not being multi-arch can break for a user that wants to switch from core-dns to kube-dns on a cluster that is not only amd64 via the CoreDNS feature gate.

two options:

  • add schema2s for kube-dns until it's deprecated.
  • not support anything but amd64 and document it. this is worse because bug reports can spawn and these are not that obvious to debug.

@bowei
Copy link
Member

bowei commented Sep 10, 2018

What does multi-arched mean? We build kube-dns for amd64 arm arm64 ppc64le s390x.

@dims
Copy link
Member

dims commented Sep 19, 2018

@prameshj @MrHohn Here's a note from @neolit123 on slack

neolit123 [9:11 PM]
there is something fishy about the manifest lists for kube-dns 1.14.12.

https://console.cloud.google.com/gcr/images/google-containers/GLOBAL/k8s-dns-kube-dns?gcrImageListsize=50
https://console.cloud.google.com/gcr/images/google-containers/GLOBAL/k8s-dns-kube-dns-amd64?gcrImageListsize=50
the manifest list for 1.14.12 is there, but say the amd64 1.14.12 is missing.

@MrHohn
Copy link
Member

MrHohn commented Sep 19, 2018

@dims I think we were confused about how the multi-arched images work.

https://pantheon.corp.google.com/gcr/images/google-containers/GLOBAL/k8s-dns-kube-dns contains:

  • Manifest list which includes all supprted archs (amd64, arm, arm64, ppc64le, s390x)
  • Docker Manifest for each arch.

Is that not enough? Do we need to push images to k8s-dns-kube-dns-{arch} as well?

@neolit123
Copy link
Member

@MrHohn
please have a look at how the kube-apiserver manifest list and related images are defined at GCR:
https://console.cloud.google.com/gcr/images/google-containers/GLOBAL/kube-apiserver?gcrImageListsize=50

those are correct.

@MrHohn
Copy link
Member

MrHohn commented Sep 19, 2018

Okay, seems like kube-apiserver pushes the same image to both --- kube-apiserver and kube-apiserver-{arch}. We will do the same for kube-dns then.

@dims
Copy link
Member

dims commented Sep 19, 2018

thanks @MrHohn yes, all the individual {arch} images and the manifest (without the arch)

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@prameshj
Copy link
Contributor Author

prameshj commented Sep 19, 2018

@dims Do we need the individual {arch} images to support docker pull commands like k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.12 ?
I had tried downloading images without the suffix and it worked. Wasn't sure whether to push the individual images, so followed example of coredns which has only one image name, but the other arch images have different tags. How do we decide when the individual images are needed and when they aren't?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 19, 2018
@MrHohn
Copy link
Member

MrHohn commented Sep 19, 2018

We will do the same for kube-dns then.

@dims @neolit123 All the individual {arch} images should have been pushed now. e.g. https://pantheon.corp.google.com/gcr/images/google-containers/GLOBAL/k8s-dns-kube-dns-amd64

@k8s-ci-robot k8s-ci-robot merged commit 191949d into kubernetes:master Sep 19, 2018
@dims
Copy link
Member

dims commented Sep 19, 2018

@prameshj @MrHohn thanks!

@prameshj we are in that weird zone where we are getting things to work but not really told everyone to switch over to manifests. So for now since there are many tools other than kubeadm, we should just publish what we used to publish before AND the new manifest. when we get to v1.13 we can make a stronger statement around requiring manifests all the time (may be).

Yea, coredns was ahead of us if i remember right. So let's follow what i outlined above.

( cc @mkumatag )

@neolit123
Copy link
Member

@MrHohn
thank you, i have just verified that the 1.14.12 images and manifests are all there!

@Random-Liu
Copy link
Member

@MrHohn The image is not correctly built... Similar issue with #67027 and #65253.

Please use docker 18.06 to build the image if you want multi-arch support.

@MrHohn
Copy link
Member

MrHohn commented Sep 19, 2018

The image is not correctly built...

That is unfortunately :/ Working with @Random-Liu now to repush the images.

@dims
Copy link
Member

dims commented Sep 19, 2018

Thanks @Random-Liu @MrHohn !

@neolit123
Copy link
Member

@Random-Liu thanks for the heads up on the issue, indeed!
i'm working on a e2e test to verify the manifests and the "digest vs size" verification is currently missing. i need to add that.

@Random-Liu
Copy link
Member

@neolit123 Thanks a lot!

@mkumatag
Copy link
Member

@Random-Liu thanks for the heads up on the issue, indeed!
i'm working on a e2e test to verify the manifests and the "digest vs size" verification is currently missing. i need to add that.

we already have a check for making sure we are using docker 18.06 version - https://github.com/kubernetes/kubernetes/blob/master/test/images/image-util.sh#L100

@neolit123
Copy link
Member

@mkumatag
yep, the e2e was agreed upon regardless as an extra verification step.

@MrHohn
could you please update us when this is done, or perhaps drop a note in #sig-release ?
thank you.

@MrHohn
Copy link
Member

MrHohn commented Sep 20, 2018

More updates:

  1. 1.14.12 images haven't be re-pushed yet and is pending on the discussion internally regarding whether or not we should re-push images with the same label.
  2. Rebuild docker images to pick up Alpine 3.8.1 dns#263 just came in and we are building images with a new label (1.14.13).

Likely we will need another PR to bump kube-dns to 1.14.13, which should be correctly built and includes the RCE fix.

@neolit123
Copy link
Member

@MrHohn

1.14.12 images haven't be re-pushed yet and is pending on the discussion internally regarding whether or not we should re-push images with the same label.

there was a similar discussion for etcd and it was decided to bump revisions:
#59664
https://github.com/kubernetes/kubernetes/pull/59664/files#diff-ff03ae6284132dbd066dc35d350259c9R37

kubernetes/dns#263 just came in and we are building images with a new label (1.14.13).
Likely we will need another PR to bump kube-dns to 1.14.13, which should be correctly built and includes the RCE fix.

sounds good.

@tpepper
Copy link
Member

tpepper commented Sep 20, 2018

Likely we will need another PR to bump kube-dns to 1.14.13, which should be correctly built and includes the RCE fix.

Please get that submitted ASAP today!!!

@MrHohn
Copy link
Member

MrHohn commented Sep 20, 2018

Sent #68900.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet