Skip to content
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

update Crate Docker image #1664

Merged
merged 3 commits into from
May 5, 2016
Merged

update Crate Docker image #1664

merged 3 commits into from
May 5, 2016

Conversation

kovrus
Copy link
Contributor

@kovrus kovrus commented Apr 26, 2016

The Crate Docker image is based Alpine Linux now

@yosifkit
Copy link
Member

We usually just provide both a Debian version and an Alpine version, so I am hesitant to just switch image bases without a transition period, but I couldn't really find users doing FROM crate so the fallout should be minimal.

The vendored dependency of libsigar does not show any provenance about where it came from. I would rather be pulling from their official releases on sourceforge or build from their source on github. Or maybe this need could provoke the Alpine team to finish packaging it; there is PR hyperic/sigar#75 to make it build-able within Alpine.

Other feedback on the Dockerfile:

# specific version rather than latest
- FROM alpine:latest
+ FROM alpine:3.3

# unneeded as "http://dl-cdn.alpinelinux.org/alpine/v3.3/community" is already in the file
-RUN echo 'http://nl.alpinelinux.org/alpine/latest-stable/community' >> /etc/apk/repositories

# combine layers, otherwise apk del doesn't actually save space
# use --no-cache :)
 RUN set -ex \
-    && apk update \
-    && apk add --update-cache openssl ca-certificates libtirpc \
-        --virtual .fetch-deps tar wget \
-    && wget -nv "$CDN/sigar/$LIB_SIGAR.1.0" -P /usr/local/lib \
+    && apk add --no-cache --virtual .fetch-deps \
+         openssl ca-certificates libtirpc \
+         tar wget \
+    && wget -nv "sourceforgeurl?" -P /usr/local/lib \
     && ln /usr/local/lib/$LIB_SIGAR.1.0 /usr/local/lib/$LIB_SIGAR.1 \
     && runDeps="$(\
         scanelf --needed --nobanner --recursive /usr/local \
             | awk '{ gsub(/,/, "\nso:", $2); print "so:" $2 }' \
             | sort -u \
             | xargs -r apk info --installed \
             | sort -u \
     )" \
-    && apk add --no-cache --virtual .libsigar-rundeps $runDeps
-
-RUN mkdir /crate \
+    && apk add --no-cache --virtual .libsigar-rundeps $runDeps \
+    && mkdir /crate \
     && wget -nv -O - "$CDN/releases/crate-$CRATE_VERSION.tar.gz" \
         | tar -xzC /crate --strip-components=1 \
# not necessary, but provides nice symmetry with libsigar-rundeps
-    && apk add --update-cache openjdk8-jre-base python3 \
+    && apk add --no-cache --virtual .crate-rundeps openjdk8-jre-base python3 \
     && apk del .fetch-deps \
-    && rm -rf /var/cache/apk/*
-
# not strictly necessary:
-RUN ln -sf /usr/bin/python3 /usr/bin/python \
+    && ln -sf /usr/bin/python3 /usr/bin/python \
     && ln -sf /usr/local/lib/$LIB_SIGAR.1 /crate/lib/sigar/$LIB_SIGAR

# lastly, should move the user and group creation to the beginning of the Dockerfile so that the layer could be shared between versions of crate

@kovrus
Copy link
Contributor Author

kovrus commented Apr 28, 2016

@yosifkit Thanks for the feedback. Here is the fixup. Now we explicitly build sigarlib from source.

@yosifkit
Copy link
Member

We recommend embedding the gpg keys directly in the dockerfile like tomcat and also using [gpg --batch --verify myfile.asc myfile](https://github.com/docker-library/httpd/blob/master/2.4/Dockerfile#L43-L46) since it is [insecure](https://github.com/docker-library/official-images/pull/1420) with just --verify`. Whether you keep the keys in the image like tomcat or purge them like httpd is up to you.

You have one apk update just before the apk add --no-cache; the no-cache will do the update and delete the cache once the packages are installed.

Might want to move ENV CRATE_VERSION line to be after the RUN that builds sigar so that the sigar layer can be shared between crate versions.

@kovrus
Copy link
Contributor Author

kovrus commented May 2, 2016

Hi @yosifkit, we have addressed your comments, here is the follow up pull request.

@joemoe
Copy link

joemoe commented May 3, 2016

hi @yosifkit, do you already have some feedback for us? thanks in advance.

@tianon
Copy link
Member

tianon commented May 5, 2016

Hey folks, sorry for the delay. Just took a look, and everything looks good.

For the next revision, I'd like to see the key ID replaced with a full fingerprint, but I don't think we need to hold this up for that (ie, gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 90C23FC6585BC0717F8FBFC37FAAE51A06F6EAEB instead of gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 7faae51a06f6eaeb).

LGTM (crate/docker-crate@c9cbce8...abc17a2)

Build test of #1664; f81242c (crate):

$ bashbrew build "crate"
Cloning crate (git://github.com/crate/docker-crate) ...
Processing crate:latest ...
Processing crate:0.52 ...
Processing crate:0.52.4 ...
Processing crate:0.54 ...
Processing crate:0.54.8 ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing crate:latest
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed
testing crate:0.52
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...passed

@tianon tianon merged commit 8accad2 into docker-library:master May 5, 2016
@tianon
Copy link
Member

tianon commented May 5, 2016

Looks like the build server is having a rough time trying to reach http://apache.uib.no reliably, so if we could swap that to a US-based mirror or something soon, that'd be helpful. 😅

The way solr solved this was pretty reasonable: https://github.com/docker-solr/docker-solr/blob/344f9ac984824870819cd1d2c20139dc479cbb7f/Dockerfile-alpine.template#L5-L26

(adding an ARG for SOLR_DOWNLOAD_SERVER, then using ENV SOLR_URL ${SOLR_DOWNLOAD_SERVER:-http://www-us.apache.org/dist/lucene/solr}/$SOLR_VERSION/solr-$SOLR_VERSION.tgz such that it has a sane default for our builds)

@tianon
Copy link
Member

tianon commented May 6, 2016

Fixes proposed in crate/docker-crate#54

@kovrus kovrus deleted the 0.54.8-2 branch June 27, 2016 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants