Cleanup after #8855 #8846

hakman · 2020-04-04T11:15:26Z

The repo containing the binaries for Docker 1.11, 1.12 and 1.13 was shut down on March 31. This means support for older docker versions cannot be maintained.
https://www.docker.com/blog/changes-dockerproject-org-apt-yum-repositories/

On the 31st of March 2020, we will be shutting down the legacy APT and YUM repositories hosted at dockerproject.org and dockerproject.com. These repositories haven’t been updated with the latest releases of Docker and so the packages hosted there contain security vulnerabilities. Removing these repositories will make sure that people download the latest version of Docker ensuring their security and providing the best experience possible

This PR will need to be cherry-picked to 1.16+.

hakman · 2020-04-04T11:17:18Z

/assign @justinsb
/cc @rifelpet

rifelpet · 2020-04-04T13:15:10Z

With this PR what exactly will happen if a user specifies one of the impacted docker versions in the cluster manifest? Will nodeup fallback to the new default docker version of 17.03.2?

Should we add some api-level validation, so that a kops update cluster on a cluster that specifies one of these docker versions will fail with a useful error message?

hakman · 2020-04-04T13:27:38Z

@rifelpet I think it will install 17.03.2, this is what I saw happening in the past when the Docker version was not found by NodeUp.
Api-level validation on the docker version would be nice to have. As long as we keep the list of supported Docker versions there, should be a big improvement.

hakman · 2020-04-04T15:11:05Z

Validation for Docker versions is now added. Thanks for the idea @rifelpet.

hakman · 2020-04-04T15:40:51Z

/retest

nodeup/pkg/model/docker.go

johngmyers · 2020-04-04T21:46:58Z

nodeup/pkg/model/docker.go

@@ -43,270 +43,9 @@ var _ fi.ModelBuilder = &DockerBuilder{}

 // DefaultDockerVersion is the (legacy) docker version we use if one is not specified in the manifest.
 // We don't change this with each version of kops, we expect newer versions of kops to populate the field.
-const DefaultDockerVersion = "1.12.3"
+const DefaultDockerVersion = "17.03.2"


Might be better to instead require a value in the manifest.

This is used as a fallback when the dirstro or arch is not listed. For example, if anyone chooses to use a Debian Jessie based image with latest Docker, NodeUp will not find it and use this.

How does that work? I only see this referenced from the dockerVersion() receiver of DockerBuilder, when Cluster.Spec.Docker.Version is not specified.

It seems odd to be updating a field to the oldest version we can. The comment suggests it's a backwards-compatibility strategy that is never supposed to be updated.

I looked at the code and I also don't see any other reference. It is used just if the version doesn't exist as you also noticed.

This is a very old piece of code that exists since the beginnings. No idea why it was needed then and why we still keep it. At this point there should always be a Docker version.

kops/pkg/model/components/docker.go

Lines 50 to 62 in 62c5100

if fi.StringValue(clusterSpec.Docker.Version) == "" {

if b.IsKubernetesGTE("1.18") {

docker.Version = fi.String("19.03.8")

} else if b.IsKubernetesGTE("1.17") {

docker.Version = fi.String("19.03.4")

} else if b.IsKubernetesGTE("1.16") {

docker.Version = fi.String("18.09.9")

} else if b.IsKubernetesGTE("1.12") {

docker.Version = fi.String("18.06.3")

} else {

docker.Version = fi.String("17.03.2")

}

}

The only explanation I have is this, since containerd support was added: #7986 (comment).

Yeah, the components/docker.go code that presumably ensures the field is set was added in the same commit that adds DefaultDockerVersion. @justinsb would have the context.

This is indeed very very old. The kops CLI is supposed to populate the docker version in the "expanded" form of the cluster spec (the one you can see with --full). When it was first introduced, we didn't lock nodeup to the kops version, and so there was the risk of skew. To tolerate that, we introduced a default docker version that nodeup would use, when kops didn't populate it.

At this point, it's probably easier to remove the nodeup defaulting entirely - just skip installing docker if isn't populated, I would suggest (with a warning so we can figure out what happened!).

justinsb · 2020-04-06T13:18:19Z

nodeup/pkg/model/docker.go

@@ -43,270 +43,9 @@ var _ fi.ModelBuilder = &DockerBuilder{}

 // DefaultDockerVersion is the (legacy) docker version we use if one is not specified in the manifest.
 // We don't change this with each version of kops, we expect newer versions of kops to populate the field.
-const DefaultDockerVersion = "1.12.3"
+const DefaultDockerVersion = "17.03.2"


This is indeed very very old. The kops CLI is supposed to populate the docker version in the "expanded" form of the cluster spec (the one you can see with --full). When it was first introduced, we didn't lock nodeup to the kops version, and so there was the risk of skew. To tolerate that, we introduced a default docker version that nodeup would use, when kops didn't populate it.

At this point, it's probably easier to remove the nodeup defaulting entirely - just skip installing docker if isn't populated, I would suggest (with a warning so we can figure out what happened!).

justinsb · 2020-04-06T13:21:34Z

pkg/apis/kops/validation/validation.go

+
+	if config.Version != nil {
+		valid := []string{"17.03.2", "17.09.0", "18.03.1", "18.06.1", "18.06.2", "18.06.3", "18.09.3", "18.09.9", "19.03.4", "19.03.8"}
+		allErrs = append(allErrs, IsValidValue(fldPath.Child("version"), config.Version, valid)...)


This validation is the critical thing to backport, I believe. (I think the others might have a higher risk of collisions).

Should we split this out into its own commit that we cherry-pick back? And I'm wondering whether it's easier just to exclude the known-bad versions (1.11, 1.12 etc) with an explicit "docker removed these versions" message... Technically I guess it doesn't really matter if we let through 19.03.8 on an older version that doesn't support it - we're no worse off than we are today.

justinsb · 2020-04-06T13:24:01Z

Thanks for doing this @hakman . On the cherry-pick, I think the critical thing to cherry pick is a warning/error about the no-longer available docker versions; so I suggested splitting that into its own PR. Removal of the mappings to URLs is internal machinery and so less important to cherry-pick I suggest, and I worry that there has been some changes there that might make it harder to cherry-pick.

hakman · 2020-04-06T14:30:27Z

Thanks @justinsb. I create a new PR that can be easily cherry-picked and will update this to remove default Docker version.

justinsb · 2020-04-07T03:49:31Z

Thanks for splitting this out - looks great @hakman

/approve
/lgtm

k8s-ci-robot · 2020-04-07T03:50:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, justinsb

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:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hakman · 2020-04-07T04:13:10Z

Thanks @justinsb. Will take care of the cherry-picks for Kops 1.16 and 1.17.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 4, 2020

k8s-ci-robot requested review from joshbranham and robinpercy April 4, 2020 11:15

k8s-ci-robot added area/documentation area/nodeup labels Apr 4, 2020

k8s-ci-robot assigned justinsb Apr 4, 2020

k8s-ci-robot requested a review from rifelpet April 4, 2020 11:17

hakman force-pushed the remove-old-docker branch from 2b8e299 to 9580e39 Compare April 4, 2020 11:26

hakman force-pushed the remove-old-docker branch from 9580e39 to efc89bc Compare April 4, 2020 15:06

k8s-ci-robot added the area/api label Apr 4, 2020

johngmyers reviewed Apr 4, 2020

View reviewed changes

nodeup/pkg/model/docker.go Show resolved Hide resolved

johngmyers reviewed Apr 4, 2020

View reviewed changes

hakman requested a review from johngmyers April 5, 2020 05:20

hakman force-pushed the remove-old-docker branch 2 times, most recently from 3c4d875 to 726c7ce Compare April 5, 2020 19:52

hakman mentioned this pull request Apr 6, 2020

Docker-engine repo is no longer available for Centos. #8854

Closed

justinsb reviewed Apr 6, 2020

View reviewed changes

hakman changed the title ~~Remove support for older Docker 1.11, 1.12 and 1.13~~ Cleanup after #8855 Apr 6, 2020

hakman mentioned this pull request Apr 6, 2020

Remove support for Docker 1.11, 1.12 and 1.13 #8855

Merged

Ciprian Hacman added 3 commits April 6, 2020 17:34

Remove support for unreleased Docker 19.03.7

73903bc

Remove the default Docker version

7e021c6

Remove no longer available Docker versions

cb2f667

Update release notes with removed Docker versions

1389510

hakman force-pushed the remove-old-docker branch 2 times, most recently from 89d380a to 3471155 Compare April 6, 2020 15:35

Update tests for removed Docker versions

3851a41

hakman force-pushed the remove-old-docker branch from 3471155 to 3851a41 Compare April 6, 2020 15:40

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2020

k8s-ci-robot merged commit 324a34c into kubernetes:master Apr 7, 2020

k8s-ci-robot added this to the v1.18 milestone Apr 7, 2020

hakman deleted the remove-old-docker branch April 30, 2020 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup after #8855 #8846

Cleanup after #8855 #8846

hakman commented Apr 4, 2020

hakman commented Apr 4, 2020

rifelpet commented Apr 4, 2020

hakman commented Apr 4, 2020

hakman commented Apr 4, 2020

hakman commented Apr 4, 2020

johngmyers Apr 4, 2020

hakman Apr 5, 2020 •

edited

Loading

johngmyers Apr 5, 2020

hakman Apr 5, 2020

johngmyers Apr 6, 2020

justinsb Apr 6, 2020

justinsb Apr 6, 2020

justinsb Apr 6, 2020

justinsb commented Apr 6, 2020

hakman commented Apr 6, 2020

justinsb commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

hakman commented Apr 7, 2020

	if fi.StringValue(clusterSpec.Docker.Version) == "" {
	if b.IsKubernetesGTE("1.18") {
	docker.Version = fi.String("19.03.8")
	} else if b.IsKubernetesGTE("1.17") {
	docker.Version = fi.String("19.03.4")
	} else if b.IsKubernetesGTE("1.16") {
	docker.Version = fi.String("18.09.9")
	} else if b.IsKubernetesGTE("1.12") {
	docker.Version = fi.String("18.06.3")
	} else {
	docker.Version = fi.String("17.03.2")
	}
	}

Cleanup after #8855 #8846

Cleanup after #8855 #8846

Conversation

hakman commented Apr 4, 2020

hakman commented Apr 4, 2020

rifelpet commented Apr 4, 2020

hakman commented Apr 4, 2020

hakman commented Apr 4, 2020

hakman commented Apr 4, 2020

johngmyers Apr 4, 2020

Choose a reason for hiding this comment

hakman Apr 5, 2020 • edited Loading

Choose a reason for hiding this comment

johngmyers Apr 5, 2020

Choose a reason for hiding this comment

hakman Apr 5, 2020

Choose a reason for hiding this comment

johngmyers Apr 6, 2020

Choose a reason for hiding this comment

justinsb Apr 6, 2020

Choose a reason for hiding this comment

justinsb Apr 6, 2020

Choose a reason for hiding this comment

justinsb Apr 6, 2020

Choose a reason for hiding this comment

justinsb commented Apr 6, 2020

hakman commented Apr 6, 2020

justinsb commented Apr 7, 2020

k8s-ci-robot commented Apr 7, 2020

hakman commented Apr 7, 2020

hakman Apr 5, 2020 •

edited

Loading