-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add support for newer Docker versions #7860
Conversation
Hi @hakman. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @justinsb |
@hakman: Re-titling can only be requested by trusted users, like repository collaborators. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @mikesplain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok for me.
/lgtm
/ok-to-test
pkg/model/components/docker.go
Outdated
if sv.Major == 1 && sv.Minor >= 12 { | ||
if sv.Major == 1 && sv.Minor >= 17 { | ||
dockerVersion = "19.03.4" | ||
} else if sv.Major == 1 && sv.Minor >= 14 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this might be a good idea, we do have a policy of not changing the defaults on existing clusters, and we have shipped kops 1.14. Would you mind making this >= 15, therefore?
(And we'll have to cherry-pick back to 1.15...)
Otherwise I cross checked and the versions look good, k8s 1.15 supports 18.09, k8s 1.17 adds support for 19.03 - thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still haven't figured out the support policy for Docker, but I have nothing against bumping to >= 15.
From security point of view, it is hard to tell if there are any issues with 18.06.3. Not being an EE release, it is unmaintained since Feb. I didn't see anything that caught my eye in the Docker release notes, but Contained seems to have many "notable updates" including CVE fixes since then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea would be to cherry-pick only the package definitions to released versions. This way, any user can upgrade Docker at its own risk.
Thank you so much for doing this @hakman - this is great, except that we really shouldn't change the behaviour for k8s/kops 1.14 (now that we've released kops 1.14.0). If there's a really important security issue we can consider it, but I'm not aware of one, so I just proposed changing >= 1.15 instead? |
/test pull-kops-bazel-test |
/test pull-kops-e2e-kubernetes-aws |
Thank you @justinsb. I appreciate you taking the time to review this and considering it for the 1.15 release. |
99c75bc
to
07312a3
Compare
/test pull-kops-e2e-kubernetes-aws |
@justinsb minimum version to apply defaults changed to 1.16 as discussed during Office Hours. Thanks again! |
9abe4eb
to
3e07810
Compare
Thanks @hakman! This looks great and thank you for adjusting things for 1.16. Since that looks like the only issue, lets get this in. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman, mikesplain 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 |
I also confirmed that these are the validated docker version from release docs. Thanks! 👍 |
Thanks @mikesplain @justinsb. |
Cherry pick of #7860 onto release-1.15
Not sure this is working, set the version per the above w/ 1.15.0 final, and still end up with 18.06-ce. |
@jhohertz I already have a cluster upgraded with Docker 19.03.4. No clues in kops output during the upgrade? |
I'd forgotten that the docker version management is a no-op for CoreOS @hakman, sorry for any confusion. |
No problem @jhohertz. I see some Flatcar has some 19.03.4 in their Edge channel. I guess it will take a bit longer to get to their stable release. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Updates supported Docker versions to latest stable. Previous versions for Docker CE are EOL.
Which issue(s) this PR fixes:
Fixes #7463
Fixes #7853
Special notes for your reviewer:
At the moment the default Docker version in Kops is 18.06.3, quite old and EOL. Docker version 18.09 has been officially validated since k8s 1.14, but it's EOL also at the moment also.
Docker 19.03 has been stable for some time and has been validated for k8s 1.14+ since June, just not documented. kubernetes/kubernetes#82326 (comment)
Test infrastructure has been moved to Docker 19.03 also. kubernetes/test-infra#14784
Website should be getting updated instructions for installing Docker soon. kubernetes/website#17405
IMHO, adding support Docker 19.03 and 18.09 should be a useful from stability and security point of view.
Hash check passed for all packages:
I tested the changes with Debian Stretch and Buster, Ubuntu Bionic, RHEL 7 and 8.
The only issue I noticed was the
iptables-legacy
setup that will be addressed by #7379.Does this PR introduce a user-facing change?:
Default Docker version will change to 18.09.9 for k8s >= 1.14 and 19.03.4 for k8s >= 1.17.
Users will be able to manually select version 19.03.4.