Base etcd-empty-dir-cleanup on busybox, run as nobody, and update to etcdctl 3.0.14 #41674

ixdy · 2017-02-17T23:59:55Z

What this PR does / why we need it: since the etcd-empty-dir-cleanup image just uses a simple shell script and etcdctl, we can base it on busybox, which is a smaller target than alpine.

I've also updated this to use an etcdctl from etcd 3.0.14, which matches the version of etcd we're running in 1.6 clusters (I believe), and changed the tag to match the etcdctl version.

Tested in my own e2e cluster, where it seems to work.

I haven't pushed the image yet, so e2e tests may fail. Tagging do-not-merge; if you think this looks good, I'll push the image and retest.

Release note:

cc @timstclair @mml @wojtek-t

k8s-reviewable · 2017-02-18T00:01:08Z

This change is

roberthbailey · 2017-02-18T00:07:09Z

cluster/images/etcd-empty-dir-cleanup/Makefile

 IMAGE = gcr.io/google_containers/etcd-empty-dir-cleanup
-TAG = 0.0.1
+TAG = 3.0.14


We are likely to want to rev this faster than etcd versions (e.g. to pick up security fixes in the container base image). Should we make the tag start with the etcd version but have an extension too? e.g. 3.0.14.0

potentially, though I expect busybox to be less of a security issue than other base images.

an open question is how exactly to update images when we only want to update the base image: do we use explicit tags, which requires us to update manifests, or do we just move the tag to the newer image?

I guess I can tag this 3.0.14.0 while we're trying to answer that question.

Agreed, we need to come up with a convention for all our images. Semantic versioning calls for 3.0.14-whatever. Also, this image is more than the etcd version, what happens when we modify the cleanup script? I suggest keeping the image version scheme (0.1.0 or maybe 0.1.0-etcd3.0.14)

spxtr · 2017-02-18T00:17:42Z

@k8s-bot bazel test this

mbohlool · 2017-02-18T00:32:35Z

Look good to proceed to e2e test.

timstclair · 2017-02-18T00:23:48Z

cluster/images/etcd-empty-dir-cleanup/Dockerfile

-CMD bash /etcd-empty-dir-cleanup.sh
+
+# nobody:nobody
+USER 65534:65534


I'm surprised this works... I guess etcd doesn't need any credentials? Did you verify that the empty directories actually are cleanup up?

etcdctl doesn't seem to care which user it runs as.

I tested this, and it seems to work, except that I think this conditional is buggy, possibly under ash, possibly in general. digging some more...

yeah, I made a /registry/foo/bar directory, and deleting it fails with this image:

Starting cleanup... sh: /registry/foo/bar: unknown operand Removing empty key /registry/foo/ ... Error: 108: Directory not empty (/registry/foo) [17] Done with cleanup.

works fine with 0.0.1:

Starting cleanup... Removing empty key /registry/foo/bar/ ... Done with cleanup.

I think this conditional might be wrong:

if [[ $(${ETCDCTL} ls $1) ]]; then

it happens to work in bash, but I'm not sure what it's actually trying to test. maybe it wants -n?

timstclair · 2017-02-18T00:27:18Z

cluster/images/etcd-empty-dir-cleanup/Makefile

 IMAGE = gcr.io/google_containers/etcd-empty-dir-cleanup
-TAG = 0.0.1
+TAG = 3.0.14


Agreed, we need to come up with a convention for all our images. Semantic versioning calls for 3.0.14-whatever. Also, this image is more than the etcd version, what happens when we modify the cleanup script? I suggest keeping the image version scheme (0.1.0 or maybe 0.1.0-etcd3.0.14)

timstclair · 2017-02-18T00:44:43Z

cluster/images/etcd-empty-dir-cleanup/Dockerfile

-RUN chmod +x etcd-empty-dir-cleanup.sh
-CMD bash /etcd-empty-dir-cleanup.sh
+
+# nobody:nobody


FYI - busybox maps nobody to 99. We could do USER nobody:nobody, but maybe using 65534 is preferable? We need Kubernetes managed UIDs....

using nobody:nobody probably makes more sense for busybox images, though it's frustrating that we can't be consistent.

timstclair · 2017-02-18T00:51:56Z

cluster/images/etcd-empty-dir-cleanup/Dockerfile

@@ -12,12 +12,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-FROM gliderlabs/alpine
+FROM busybox


The problem with busybox is we don't currently have a way to track CVEs. It's not so much an issue with this particular image, but something to think about...

I'm tempted to put the version number in here (busybox:1.26.2). On the one hand, it would make it easier to see which version images were on. On the other hand, it makes it easier for the versions to be left behind.... what are your thoughts?

I'm not really sure what is best - we seem to have a mix of both, though unversioned busybox is generally more common.

I'm also generally less worried about CVEs affecting busybox, since historically there have been very few affecting it.

spxtr · 2017-02-18T01:01:51Z

@k8s-bot test this
My bad.

ixdy · 2017-02-18T01:03:15Z

gcr.io/google-containers/etcd-empty-dir-cleanup:3.0.14.0 pushed.

ixdy · 2017-02-18T01:07:00Z

oops, didn't see @timstclair's comments.

ixdy · 2017-02-22T00:18:43Z

Changed to use USER nobody:nobody, and fixed the shell script by adding quotes. I think this works now.

timstclair · 2017-02-22T18:49:01Z

/lgtm

I think we should revisit the question of busybox, but this is ok for now.

k8s-ci-robot · 2017-02-22T18:49:01Z

@timstclair: you can't LGTM a PR unless you are an assignee.

In response to this comment:

/lgtm

I think we should revisit the question of busybox, but this is ok for now.

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. I understand the commands that are listed here.

ixdy · 2017-02-22T19:44:43Z

@k8s-bot cvm gce e2e test this
@k8s-bot gce etcd3 e2e test this
@k8s-bot gci gce e2e test this

ixdy · 2017-02-22T19:44:51Z

@mbohlool does this still LG to you?

mbohlool · 2017-02-22T20:59:59Z

cluster/images/etcd-empty-dir-cleanup/etcd-empty-dir-cleanup.sh

@@ -17,7 +17,7 @@
 echo "Removing empty directories from etcd..."

 cleanup_empty_dirs () {
-  if [[ $(${ETCDCTL} ls $1) ]]; then
+  if [[ "$(${ETCDCTL} ls $1)" ]]; then


I am not a sh/bash expert, but my understanding is [[..]] is built-in (either supported or not but if it is supported it is a built-in check) and does not require quotes. This make me wonder if this works as expect. of course if the if statement returns true all the time, the script would still remote empty folders, but then we can simplify it by removing if statement. Have you check this if statement in busybox's sh?

ref: http://serverfault.com/questions/52034/what-is-the-difference-between-double-and-single-square-brackets-in-bash

It's not exactly a built-in in busybox:

$ docker run --rm -i -t busybox which [[ /bin/[[

That's probably why this command broke when switched from bash to busybox's ash.

I think busybox's ash is faking [[..]] support.

Simple example inside the busybox container:

# cat test.txt foo bar # if [[ $(cat test.txt) ]]; then echo a; else echo b; fi sh: bar: unknown operand b # if [[ "$(cat test.txt)" ]]; then echo a; else echo b; fi a

I could probably change this to [..] if we want to be clearer that this isn't bash.

changed to [..].

…etcdctl 3.0.14

ixdy · 2017-02-22T22:13:23Z

@k8s-bot test this

ixdy · 2017-02-23T22:01:43Z

@k8s-bot test this

roberthbailey · 2017-02-24T04:34:04Z

any reason not to squash your commits? The second one is a one line change that doesn't seem necessary to keep separate.

/approve

k8s-github-robot · 2017-02-24T04:34:24Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: ixdy, roberthbailey, timstclair

Needs approval from an approver in each of these OWNERS Files:

~~cluster/OWNERS~~ [roberthbailey]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-02-24T05:25:56Z

Automatic merge from submit-queue

ixdy · 2017-02-24T19:29:22Z

I kept them separate as "implementation" and "activation".

ixdy · 2017-02-24T19:31:25Z

also, of course after I opened this PR we bumped to etcd 3.0.17.

I'm guessing it probably doesn't matter for this container, though. @wojtek-t @mml agree?

ixdy added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 17, 2017

ixdy assigned mbohlool Feb 17, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 17, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 18, 2017

roberthbailey reviewed Feb 18, 2017

View reviewed changes

ixdy force-pushed the etcd-empty-dir-cleanup-busybox branch from 9b6a437 to c2c8632 Compare February 18, 2017 00:28

mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2017

mbohlool removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2017

timstclair reviewed Feb 18, 2017

View reviewed changes

ixdy force-pushed the etcd-empty-dir-cleanup-busybox branch 2 times, most recently from 2b0f5ab to b5d77e3 Compare February 18, 2017 01:04

ixdy force-pushed the etcd-empty-dir-cleanup-busybox branch 2 times, most recently from 10e4823 to 6ef01d6 Compare February 22, 2017 00:17

ixdy removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 22, 2017

ixdy force-pushed the etcd-empty-dir-cleanup-busybox branch from 6ef01d6 to 11e09fc Compare February 22, 2017 00:38

ixdy 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 Feb 22, 2017

mbohlool reviewed Feb 22, 2017

View reviewed changes

ixdy added 2 commits February 22, 2017 13:22

Base etcd-empty-dir-cleanup on busybox, run as nobody, and update to …

1f3ba7f

…etcdctl 3.0.14

Bump etcd-empty-dir-cleanup to 3.0.14.0

511bdc1

ixdy force-pushed the etcd-empty-dir-cleanup-busybox branch from 11e09fc to 511bdc1 Compare February 22, 2017 21:23

mbohlool added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2017

k8s-github-robot merged commit 84b7407 into kubernetes:master Feb 24, 2017

ixdy deleted the etcd-empty-dir-cleanup-busybox branch May 15, 2018 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base etcd-empty-dir-cleanup on busybox, run as nobody, and update to etcdctl 3.0.14 #41674

Base etcd-empty-dir-cleanup on busybox, run as nobody, and update to etcdctl 3.0.14 #41674

ixdy commented Feb 17, 2017 •

edited

k8s-reviewable commented Feb 18, 2017

roberthbailey Feb 18, 2017

ixdy Feb 18, 2017

timstclair Feb 18, 2017

spxtr commented Feb 18, 2017

mbohlool commented Feb 18, 2017

timstclair Feb 18, 2017

ixdy Feb 18, 2017

ixdy Feb 18, 2017

ixdy Feb 18, 2017 •

edited

timstclair Feb 18, 2017

timstclair Feb 18, 2017

ixdy Feb 18, 2017

timstclair Feb 18, 2017

ixdy Feb 18, 2017

spxtr commented Feb 18, 2017

ixdy commented Feb 18, 2017

ixdy commented Feb 18, 2017

ixdy commented Feb 22, 2017

timstclair commented Feb 22, 2017

k8s-ci-robot commented Feb 22, 2017

ixdy commented Feb 22, 2017

ixdy commented Feb 22, 2017

mbohlool Feb 22, 2017

timstclair Feb 22, 2017

ixdy Feb 22, 2017

ixdy Feb 22, 2017

ixdy commented Feb 22, 2017

ixdy commented Feb 23, 2017

roberthbailey commented Feb 24, 2017

k8s-github-robot commented Feb 24, 2017

k8s-github-robot commented Feb 24, 2017

ixdy commented Feb 24, 2017

ixdy commented Feb 24, 2017

Base etcd-empty-dir-cleanup on busybox, run as nobody, and update to etcdctl 3.0.14 #41674

Base etcd-empty-dir-cleanup on busybox, run as nobody, and update to etcdctl 3.0.14 #41674

Conversation

ixdy commented Feb 17, 2017 • edited

k8s-reviewable commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spxtr commented Feb 18, 2017

mbohlool commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy Feb 18, 2017 • edited

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

spxtr commented Feb 18, 2017

ixdy commented Feb 18, 2017

ixdy commented Feb 18, 2017

ixdy commented Feb 22, 2017

timstclair commented Feb 22, 2017

k8s-ci-robot commented Feb 22, 2017

ixdy commented Feb 22, 2017

ixdy commented Feb 22, 2017

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

ixdy commented Feb 22, 2017

ixdy commented Feb 23, 2017

roberthbailey commented Feb 24, 2017

k8s-github-robot commented Feb 24, 2017

k8s-github-robot commented Feb 24, 2017

ixdy commented Feb 24, 2017

ixdy commented Feb 24, 2017

ixdy commented Feb 17, 2017 •

edited

ixdy Feb 18, 2017 •

edited