Install container runtime packages as assets #10048

hakman · 2020-10-14T05:44:38Z

The main purpose for this PR is to move the logic of choosing the container runtime (Docker or containerd) package from noddup to the assets building phase, when running kops update cluster. An additional benefit is that it's easy to add new versions and added all existing Docker packages to the supported list.

Feature related changes are quite small and limited to the "Main" and "Misc" commits.
Tests changes are more extensive to be able to do changes with confidence and are separated in the "Tests" commit.
Integration tests changes are about adding the new asset to conf/kube_env.yaml, but touches all these tests. Fortunately changes are separated in the "Integration" commit.

k8s-ci-robot · 2020-10-14T05:45:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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~~ [hakman]

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

hakman · 2020-10-14T13:06:15Z

/cc @justinsb @mikesplain @rifelpet

johngmyers · 2020-10-22T05:43:12Z

upup/pkg/fi/cloudup/containerd.go

+	containerdFallbackVersion = "1.2.13"
+)
+
+func findContainerdAssets(c *kops.Cluster, assetBuilder *assets.AssetBuilder, arch architectures.Architecture) (*url.URL, *hashing.Hash, error) {


Shouldn't this be singular: findContainerdAsset()?

Wanted to keep the same naming convention as for findCNIAssets and findLyftVPCAssets.
I also think the singular is the better solution. Will change.

johngmyers · 2020-10-22T06:00:08Z

upup/pkg/fi/cloudup/containerd.go

+}
+
+func findAllContainerdDockerMappings() map[string]string {
+	versions := map[string]string{


So:

drops 1.2.4

adds 1.2.6 and 1.2.12

changes 1.2.10 from containerd to docker

changes 1.2.13 from docker 19.03.11 to 19.03.12

Shouldn't at least the dropping of 1.2.4 be mentioned in release notes?

Containerd support is a bit of a puzzle here. Will have to think about it.

I would be okay with only moving the newer versions of containerd to assets phase.

The problem comes mostly from packaging bugs in containerd and package URL changes and lack of ARM builds...
So, to the point:

drops 1.2.4 - adding back pretty easy to do

adds 1.2.6 and 1.2.12 - it is not a big deal, added them just because it was easy to map to Docker packages

changes 1.2.10 from containerd to docker - intended, shouldn't make any difference

changes 1.2.13 from docker 19.03.11 to 19.03.12 - reverted this

johngmyers · 2020-10-22T06:05:18Z

upup/pkg/fi/cloudup/docker.go

+	dockerFallbackVersion = "17.09.0"
+)
+
+func findDockerAssets(c *kops.Cluster, assetBuilder *assets.AssetBuilder, arch architectures.Architecture) (*url.URL, *hashing.Hash, error) {


findDockerAsset()?

Wanted to keep the same naming convention as for findCNIAssets and findLyftVPCAssets.
I also think the singular is the better solution. Will change.

johngmyers · 2020-10-22T06:10:39Z

upup/pkg/fi/cloudup/docker.go

+	hashes := map[string]string{
+		"17.03.0":  "aac08524db82d3fdc8fc092f495e1174f5e1dd774b95a6d081544997d34b4855",
+		"17.03.1":  "3e070e7b34e99cf631f44d0ff5cf9a127c0b8af5c53dfc3e1fce4f9615fbf603",
+		"17.03.2":  "183b31b001e7480f3c691080486401aa519101a5cfe6e05ad01b9f5521c4112d",


So moving all distributions to the "static" variant. Is the removal of the docker-ce-selinux package on RHEL-7 and derived distributions not a problem?

(This is a change for 18.06.3 and earlier, with the exception of 17.03.2 on Ubuntu 18.04.)

@justinsb also noticed this, so will try to explain below.

Kops support for SELinux was added in 1.19 (mostly experimental from my point of view) and could not be enabled in docker config in the past. Contents of the packages show that it contains only the SELinux profiles:

> rpm -qlp ./docker-ce-selinux-17.03.2.ce-1.el7.centos.noarch.rpm /usr/share/doc/docker-ce-selinux-17.03.2.ce /usr/share/doc/docker-ce-selinux-17.03.2.ce/LICENSE /usr/share/selinux/devel/include/services/docker.if /usr/share/selinux/packages/docker.pp.bz2

The docker-ce-selinux package is distributed only for 17.03.x and later versions switch to the container-selinux dependency, distributed with RHEL/CentOS.

Considering that SELinux could not be used in the past and we already install by default a replacement package, I don't think this is an issue.

johngmyers · 2020-10-22T06:25:08Z

nodeup/pkg/model/docker.go

-					Mode: "+i",
-					Deps: []fi.Task{packageTask},
-				})
-			}


Do we not need this mitigation anymore?

I saw this done only for 17.03.x and didn't seem like something that important anymore considering the number of security issues with that version. Anyone interested in security should really use a newer Docker version.
Do you think it has any value for newer versions?

It doesn't have value for newer versions, but I don't think we should reintroduce a significant security vulnerability for 17.03.

Perhaps we should work on our deprecation policy for container runtimes.

Good call on digging a bit more. Seems that this is wider than initially thought and affects about anything < 18.09.2.
Re-added the mitigation and updated the tests output.

Perhaps we should work on our deprecation policy for container runtimes.

+1 from me 😄

johngmyers · 2020-10-22T06:38:17Z

nodeup/pkg/model/context.go

@@ -598,29 +599,31 @@ func (c *NodeupModelContext) GetPrivateKey(name string) ([]byte, error) {

 func (b *NodeupModelContext) AddCNIBinAssets(c *fi.ModelBuilderContext, assetNames []string) error {
 	for _, assetName := range assetNames {
-		if err := b.addCNIBinAsset(c, assetName); err != nil {
+		re, err := regexp.Compile(fmt.Sprintf("^%s$", assetName))


I'm a little worried about regexp metacharacters, such as ".", in the assetName.

Perhaps use regexp.Compile(fmt.Sprintf("^%s$", regexp.QuoteMeta(assetName))) instead?

Sounds reasonable. Thanks for the suggestion.

johngmyers · 2020-10-22T06:40:39Z

nodeup/pkg/model/context.go

+func (b *NodeupModelContext) addCNIBinAsset(c *fi.ModelBuilderContext, assetPath *regexp.Regexp) error {
+	a := b.Assets.FindMatches(assetPath)
+	if len(a) != 1 {
+		return fmt.Errorf("unable to locate asset %q", assetPath.String())


"unable to locate" is misleading if len(a) > 1.

Agreed. Will change.

hakman · 2020-10-23T08:12:16Z

@johngmyers I think I addressed all the review comments in my latest commit.

johngmyers · 2020-10-26T03:18:48Z

/lgtm

justinsb · 2020-10-27T13:49:22Z

Extracted out the package scripts for a few representative packages, the only thing I saw was adding the docker group and some apparmor (and we didn't support apparmor for these older versions).

Thanks for doing this @hakman !

hakman · 2020-10-27T13:58:09Z

Thanks for checking @justinsb 😄

…-upstream-release-1.19 Automated cherry pick of #10048: Install container runtime packages as assets

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/api labels Oct 14, 2020

k8s-ci-robot requested review from johngmyers and KashifSaadat October 14, 2020 05:45

k8s-ci-robot added area/nodeup approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 14, 2020

hakman force-pushed the container-runtime-assets branch 5 times, most recently from e54e4ea to ee427c9 Compare October 14, 2020 12:20

Ciprian Hacman added 4 commits October 14, 2020 15:41

Install container runtime packages as assets - Main

732a161

Install container runtime packages as assets - Misc

852bebe

Install container runtime packages as assets - Tests

b27431d

Install container runtime packages as assets - Integration

e44037f

hakman force-pushed the container-runtime-assets branch from ee427c9 to e44037f Compare October 14, 2020 13:02

k8s-ci-robot requested review from justinsb, mikesplain and rifelpet October 14, 2020 13:06

johngmyers requested changes Oct 22, 2020

View reviewed changes

hakman force-pushed the container-runtime-assets branch from 4f5826b to 9046e32 Compare October 23, 2020 07:53

Install container runtime packages as assets - Code Review 1

c362620

hakman force-pushed the container-runtime-assets branch from 9046e32 to c362620 Compare October 23, 2020 08:05

hakman requested a review from johngmyers October 23, 2020 08:12

k8s-ci-robot assigned johngmyers Oct 26, 2020

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

k8s-ci-robot merged commit c9aa538 into kubernetes:master Oct 26, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 26, 2020

hakman deleted the container-runtime-assets branch October 26, 2020 06:47

This was referenced Oct 27, 2020

Cluster cannot start with older versions of k8s #10122

Closed

Automated cherry pick of #10048: Install container runtime packages as assets #10123

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 28, 2020

Merge pull request #10123 from hakman/automated-cherry-pick-of-#10048…

a2e4009

…-upstream-release-1.19 Automated cherry pick of #10048: Install container runtime packages as assets

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install container runtime packages as assets #10048

Install container runtime packages as assets #10048

hakman commented Oct 14, 2020 •

edited

Loading

k8s-ci-robot commented Oct 14, 2020

hakman commented Oct 14, 2020

johngmyers Oct 22, 2020

hakman Oct 22, 2020

johngmyers Oct 22, 2020

hakman Oct 22, 2020

johngmyers Oct 22, 2020

hakman Oct 23, 2020

johngmyers Oct 22, 2020

hakman Oct 22, 2020

johngmyers Oct 22, 2020

hakman Oct 22, 2020

johngmyers Oct 22, 2020

hakman Oct 22, 2020 •

edited

Loading

johngmyers Oct 22, 2020

hakman Oct 23, 2020 •

edited

Loading

johngmyers Oct 22, 2020

hakman Oct 22, 2020

johngmyers Oct 22, 2020

hakman Oct 22, 2020

hakman commented Oct 23, 2020

johngmyers commented Oct 26, 2020

justinsb commented Oct 27, 2020

hakman commented Oct 27, 2020

Install container runtime packages as assets #10048

Install container runtime packages as assets #10048

Conversation

hakman commented Oct 14, 2020 • edited Loading

k8s-ci-robot commented Oct 14, 2020

hakman commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakman commented Oct 23, 2020

johngmyers commented Oct 26, 2020

justinsb commented Oct 27, 2020

hakman commented Oct 27, 2020

hakman commented Oct 14, 2020 •

edited

Loading

hakman Oct 22, 2020 •

edited

Loading

hakman Oct 23, 2020 •

edited

Loading