Skip Docker install

[austinmoore-]

Adds configuration (spec.dockerInstall.skipInstall) that indicates to nodeup that it should skip the Docker tasks. See the added docs for details on how to use it. My thinking with this configuration is that if the user specifies it, nodeup should not do anything related to Docker and it's up to the user to ensure Docker is setup as it needs to be.

The motivation for this came from an old situation I encountered. The base AMIs we were building from already had Docker install. So to get kops working with these AMIs, we would simply uninstall Docker from our AMIs and then just let kops install Docker. 🙄

Even though I personally don't have a use for this right now, I figured that while I was working on #6956 I could do this real quick. Hopefully someone out there in a similar situation finds this useful!

[k8s-ci-robot]

Hi @austinmoore-. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[austinmoore-]

/assign @geojaz

[rdrgmnzs]

Thanks @austinmoore- this looks great, I actually have a use case for this similar to the one you described.

I'm wondering if instead of adding yet another feature flag skipInstall, if there is any way of just leveraging the existing docker spec and setting version to something like noop to simplify things such as:

spec:
  docker:
    version: noop

What do you think?

/ok-to-test

[austinmoore-]

My personal opinion is that I usually prefer to have the key indicate the operation, rather than a value. It feels more explicit to me that way. In my mind, docker.version is unrelated to whether or not kops should install Docker or not. But I don't really have strong feelings one way or the other though, so if simplifying it to just noop fits kops' solution pattern better, just let me know I can definitely make that change!

[chrisz100]

I would love if people could choose their own docker version here (and noop if none) looking at all the CVE's from the last year. Do you think something getting close to this goal could be doable?

Also - if possible - could you rebase the last two commits?

[austinmoore-]

I cleaned up those commits. And yes I think it's totally doable. Since there are two voice support for that, I can go ahead and make that change.

So just to confirm the new spec will look like:

spec:
  docker:
    version: noop

[austinmoore-]

@chrisz100 Actually, I might have misread what you said. By specifying your own Docker version are you thinking of something like this: #6956

Or were you referring to @rdrgmnzs suggestion?

[granular-ryanbonham]

/retest

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[ReillyTevera]

/remove-lifecycle stale

[austinmoore-]

/retest

#7571 makes me wonder if this was a flaky failure

[mikesplain]

Thanks for the ping on this @rdrgmnzs, I agree with everyones comments above.

A separate DockerInstallSpec feels like a bit much for me considering the simplicity of what's being accomplished here, since as you noted, this isn't new behavior, just allowing users to configure it.

Is there a reason this couldn't just be part of the DockerConfig spec? If all of those attributes only drove docker I would say no, but we already have nodeup using the Version attribute.

So I think my preference would be:

spec:
  docker:
    skipInstall: true

[mikesplain]

/assign @rdrgmnzs
/assign @mikesplain

[austinmoore-]

The reason I had added a new DockerInstallSpec field was because I knew that I would also be implementing #6956 which also uses that. But I agree that

spec:
  docker:
    skipInstall: true

is a good place for this and removes the need to add a new field. So if we go through with this, I think we should also move overrideSources from the other PR to spec.docker. What do you think?

[mikesplain]

Ahh yeah I see, that makes sense but personally, I don't mind having that in DockerConfig as well, others may feel more strongly. Before you move things over, lets confirm that @rdrgmnzs and @chrisz100 agree. Thanks for the work on this!

[rdrgmnzs]

👍 to having everything under DockerConfig.

[austinmoore-]

@mikesplain @rdrgmnzs How's it look?

[rdrgmnzs]

This looks great @austinmoore- ! Thanks you so much for sticking with this and getting it to this point.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: austinmoore-, rdrgmnzs

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 [rdrgmnzs]

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