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

Swift #2500

Merged
merged 5 commits into from
Jan 21, 2017
Merged

Swift #2500

merged 5 commits into from
Jan 21, 2017

Conversation

swizzlr
Copy link
Contributor

@swizzlr swizzlr commented Dec 31, 2016

Hi all,

Mario Ponticello got in touch with us last year regarding an official Swift image. He's apparently no longer with Docker, but all the same we'd love to take him up on this offer.

A few notes:

We'd love more information about how these image manifest files could be auto-generated from the GitHub repo, and also to find out how automated builds work/are authenticated.

Happy new year!

Dockerization Review Feedback

  • Justify Ubuntu (officially supported by Swift – we've had compatibility issues with other distros, but we would love to have an alpine/busybox variant)
  • Replace maintainer with label instruction
  • See if we can remove build-essential from package list (investigate make and libc6-dev)
  • Remove apt-get clean (no op)
  • Embed Swift GPG key fingerprints in Dockerfile (see Tianon's below suggestion)
  • Update GPG verification code to use --batch
  • Evaluate changing Swift tar wget to use -O to simplify file handling
  • Check if we need to rm -rf the /tmp and /var/tmp directories – if not, remove that instruction

Checklist for Review

NOTE: This checklist is intended for the use of the Official Images maintainers both to track the status of your PR and to help inform you and others of where we're at. As such, please leave the "checking" of items to the repository maintainers. If there is a point below for which you would like to provide additional information or note completion, please do so by commenting on the PR. Thanks! (and thanks for staying patient with us ❤️)

@swizzlr swizzlr mentioned this pull request Jan 7, 2017
@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 7, 2017

@tianon thanks so much for reviewing what we have so far. I've opened the docs/ PR.

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 7, 2017

Docs PR is passing!

@hamin
Copy link

hamin commented Jan 7, 2017

@swizzlr amazing! What else is left?

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 7, 2017

@hamin In depth review by Docker people – including checking that your Dockerfile is good and the documentation isn't terrible.

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 7, 2017

I'll help make this easier:

existing official images have been considered as a base? (ie, if foobar needs Node.js, has FROM node:... instead of grabbing node via other means been considered?)

It would be lovely to have a "FROM llvm", but we don't.

passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test)

We've added tests, and it's passing CI.

@tianon
Copy link
Member

tianon commented Jan 12, 2017

Thanks @swizzlr -- I think we're down to just Dockerization review and docs review. 👍

Here's the full build context for simpler review:

diff --git a/swift_latest/Dockerfile b/swift_latest/Dockerfile
new file mode 100644
index 0000000..be9a175
--- /dev/null
+++ b/swift_latest/Dockerfile
@@ -0,0 +1,31 @@
+FROM ubuntu:16.04
+MAINTAINER Haris Amin <[email protected]>
+
+# Install related packages and set LLVM 3.6 as the compiler
+RUN apt-get update && \
+    apt-get install -y build-essential wget clang-3.6 curl libedit-dev python2.7 python2.7-dev libicu-dev rsync libxml2 git libcurl4-openssl-dev && \
+    update-alternatives --install /usr/bin/clang clang /usr/bin/clang-3.6 100 && \
+    update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-3.6 100 && \
+    apt-get clean && \
+    rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*
+
+# Set Swift Path
+ENV PATH /usr/bin:$PATH
+# Everything up to here should cache nicely between Swift versions, assuming dev dependencies change little
+ENV SWIFT_BRANCH=swift-3.0.2-release SWIFT_VERSION=swift-3.0.2-RELEASE SWIFT_PLATFORM=ubuntu16.04
+
+# Install Swift keys
+RUN wget -q -O - https://swift.org/keys/all-keys.asc | gpg --import - && \
+    gpg --keyserver hkp://pool.sks-keyservers.net --refresh-keys Swift
+
+# Install Swift Ubuntu Snapshot
+RUN SWIFT_ARCHIVE_NAME=$SWIFT_VERSION-$SWIFT_PLATFORM && \
+    SWIFT_URL=https://swift.org/builds/$SWIFT_BRANCH/$(echo "$SWIFT_PLATFORM" | tr -d .)/$SWIFT_VERSION/$SWIFT_ARCHIVE_NAME.tar.gz && \
+    wget $SWIFT_URL && \
+    wget $SWIFT_URL.sig && \
+    gpg --verify $SWIFT_ARCHIVE_NAME.tar.gz.sig && \
+    tar -xvzf $SWIFT_ARCHIVE_NAME.tar.gz --directory / --strip-components=1 && \
+    rm -rf $SWIFT_ARCHIVE_NAME* /tmp/* /var/tmp/*
+
+# Print Installed Swift Version
+RUN swift --version

@tianon
Copy link
Member

tianon commented Jan 12, 2017

Some general review notes after taking a look -- for any of the changes I'm requesting here, I'm happy to instead turn these into a PR against your repo directly. 👍

+FROM ubuntu:16.04

Any particular reason for Ubuntu? Available LLVM version? Officially supported by Swift upstream?
(just curious -- not necessarily something that ought to change)

+MAINTAINER Haris Amin <[email protected]>

Just FYI, the MAINTAINER instruction is on the road to deprecation (moby/moby#25466), so probably worth removing this line in a later update.

+    apt-get install -y build-essential ...

From https://packages.debian.org/jessie/build-essential (Ubuntu's page doesn't include the description):

If you do not plan to build Debian packages, you don't need this package.

I think rather than build-essential, you probably want to include the dependencies it's pulling in directly, and since you're using clang rather than gcc, I think you're probably just looking for make and libc6-dev?

+    apt-get clean && \

Invoking apt-get clean is unnecessary and will no-op -- the base image already performs the same function as apt-get clean automatically after installing packages:

+# Install Swift keys
+RUN wget -q -O - https://swift.org/keys/all-keys.asc | gpg --import - && \
+    gpg --keyserver hkp://pool.sks-keyservers.net --refresh-keys Swift

So, this method of download the PGP keys works, but it doesn't provide any additional download verification -- if someone were to compromise the https://swift.org infra (where we're also downloading the binaries from), they could give us a compromised all-keys.asc just as well as giving us a compromised swift-3.0.2-RELEASE-ubuntu16.04.tar.gz, so as noted in https://github.com/docker-library/official-images/blob/master/README.md#security, that ought to be expanded to the list of full fingerprints for verification.

For reference, here's a general pattern (with the Swift keys embedded) that we've found successful:

RUN set -ex; \
	for key in \
# pub   4096R/412B37AD 2015-11-19 [expires: 2017-11-18]
#       Key fingerprint = 7463 A81A 4B2E EA1B 551F  FBCF D441 C977 412B 37AD
# uid                  Swift Automatic Signing Key #1 <[email protected]>
		7463A81A4B2EEA1B551FFBCFD441C977412B37AD \
# pub   4096R/21A56D5F 2015-11-28 [expires: 2017-11-27]
#       Key fingerprint = 1BE1 E29A 084C B305 F397  D62A 9F59 7F4D 21A5 6D5F
# uid                  Swift 2.2 Release Signing Key <[email protected]>
		1BE1E29A084CB305F397D62A9F597F4D21A56D5F \
# pub   4096R/91D306C6 2016-05-31 [expires: 2018-05-31]
#       Key fingerprint = A3BA FD35 56A5 9079 C068  94BD 63BC 1CFE 91D3 06C6
# uid                  Swift 3.x Release Signing Key <[email protected]>
		A3BAFD3556A59079C06894BD63BC1CFE91D306C6 \
	; do \
		gpg --keyserver ha.pool.sks-keyservers.net --recv-keys "$key"; \
	done
+    gpg --verify $SWIFT_ARCHIVE_NAME.tar.gz.sig && \

This should be gpg --batch --verify $SWIFT_ARCHIVE_NAME.tar.gz.sig $SWIFT_ARCHIVE_NAME.tar.gz -- see #1420 (comment) for more information.

Also, it might be worth using -O on those wget invocations so that $SWIFT_ARCHIVE_NAME can be replaced with something simpler like swift.tar.gz.

+    rm -rf $SWIFT_ARCHIVE_NAME* /tmp/* /var/tmp/*

From what I can see, nothing in this RUN line is putting files in /tmp or /var/tmp -- can you elaborate on why those are being cleared here too?

$ docker run -it --rm swift swift
error: failed to launch REPL process: process launch failed: 'A' packet returned an error: 8

Any idea why launching the REPL is failing? If we can get the REPL working, we can add CMD ["swift"] so that when users do docker run -it --rm swift they get dropped directly into the REPL by default (https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#/cmd). 👍

@tianon
Copy link
Member

tianon commented Jan 12, 2017

Ah, the REPL bit is the reason you had --privileged -- I can confirm the note in swiftlang/swift-docker#9 (comment): this is seccomp related, and full --privileged isn't necessary:

$ docker run -it --rm --security-opt seccomp=unconfined swift swift
Welcome to Swift version 3.0.2 (swift-3.0.2-RELEASE). Type :help for assistance.
  1> print("hi");
hi

@hamin
Copy link

hamin commented Jan 12, 2017

@tianon let me answer a couple of the questions for you.

With ubuntu 16.04, that's because aswift is officially supporting that version. I think they might have 16.10 support too but we are trying to only support Ubuntu LTS releases if possible.

Regarding the swift repl not working, this has been a thorn for us for a little while. Our solution this far has been to create the container using the '--privilege ' flag which we know is not advised. We think it's an issue that at some point broke with the swift tolling and docker but are not sure.

Regarding the other comments I think they all make sense. What do you think @swizzlr ?

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 12, 2017

@tianon wow, thank you so much for that amazing feedback! Pardon our dust in places, a lot of the crud was due to various incantations to get it to work in the early days.

To clarify one thing – do you want the REPL to be working before approving? https://bugs.swift.org/browse/SR-54 shows that the tight integration between REPL and LLDB means that it would be a seriously heavy bit of work to make the REPL not require kernel privileges (this would be an interesting task – perhaps LLDB could have a user mode). I agree it would be awesome if you could drop into a REPL like Python or Ruby, but I don't think that would realistically happen without passing in a seccomp for a while yet.

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 12, 2017

@tianon @hamin I've updated the PR with a checklist based on Tianon's feedback. Tianon, please feel free to check things off if you're satisfied. I will open PRs today, based on your feedback!

@tianon As Haris mentioned, Swift on Linux officially supports Ubuntu up to 16.10 and back to 14.04 (all current LTS releases plus latest stable). We thought that choosing the latest LTS release would be the safest thing to do, but please correct us if you think another version may be better.

@hamin
Copy link

hamin commented Jan 12, 2017

@tianon I've seen other projects support multiple ubuntu versions before... I guess we could do that with Ubuntu 16.10 but is there a particular convention for docker images regarding that?

@tianon
Copy link
Member

tianon commented Jan 12, 2017

I'm totally +:100: on LTS -- I was more curious about Ubuntu specifically, but sounds like it's one that upstream officially supports/recommends, which is great (please do stick to LTS :smile:). :+1:

I don't necessarily need the REPL to be working -- I think unconfined seccomp is a fine workaround for now (and worth documenting in the description with a link to the discussion/bugs about it). Totally unconfined is still a bit hairy, but not anywhere close to as bad as --privileged, so we're fine. 🤘

@tianon
Copy link
Member

tianon commented Jan 12, 2017

Regarding MAINTAINER -- you don't necessarily need to swap to LABEL unless you really want to. The maintainer information contained here is sufficient for our purposes, so we leave that choice up to you. My goal was to make sure you're aware that MAINTAINER will go away at some point. 👍

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 12, 2017

Alright: swiftlang/swift-docker#46 @tianon @hamin feedback incorporated, dockerfile is more organized, more secure and SUBSTANTIALLY less noisy when building.

Not sure what key standards have been set on for label yet so we'll keep at maintainer for now.

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 13, 2017

@tianon alright, done! thank you so much for everything you've done so far!

@hamin
Copy link

hamin commented Jan 13, 2017

@tianon yes thank you so much! Big foot forward for server side swift community. @swizzlr we should announce this on swift mailing list and newsletters soon.

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 13, 2017

Documentation updated with info about running the REPL. Who does the extra dockerization review? @tianon

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 20, 2017

@tianon What an awful week it's been! Perhaps finalizing Swift as an official docker image could be what we all need! Is there anything else needed?

Copy link
Member

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending a minor docs comment 🎉

If you don't mind a funny looking docs, then I am good to go! 💚 🍏

@yosifkit
Copy link
Member

diff --git a/swift_latest/Dockerfile b/swift_latest/Dockerfile
new file mode 100644
index 0000000..c248cbb
--- /dev/null
+++ b/swift_latest/Dockerfile
@@ -0,0 +1,56 @@
+FROM ubuntu:16.04
+MAINTAINER Haris Amin <[email protected]>
+
+# Install related packages and set LLVM 3.6 as the compiler
+RUN apt-get -q update && \
+    apt-get -q install -y \
+    make \
+    libc6-dev \
+    clang-3.6 \
+    curl \
+    libedit-dev \
+    python2.7 \
+    python2.7-dev \
+    libicu-dev \
+    rsync \
+    libxml2 \
+    git \
+    libcurl4-openssl-dev \
+    && update-alternatives --quiet --install /usr/bin/clang clang /usr/bin/clang-3.6 100 \
+    && update-alternatives --quiet --install /usr/bin/clang++ clang++ /usr/bin/clang++-3.6 100 \
+    && rm -r /var/lib/apt/lists/*
+
+# Everything up to here should cache nicely between Swift versions, assuming dev dependencies change little
+ENV SWIFT_BRANCH=swift-3.0.2-release \
+    SWIFT_VERSION=swift-3.0.2-RELEASE \
+    SWIFT_PLATFORM=ubuntu16.04 \
+    PATH=/usr/bin:$PATH
+
+# Download GPG keys, signature and Swift package, then unpack and cleanup
+RUN SWIFT_URL=https://swift.org/builds/$SWIFT_BRANCH/$(echo "$SWIFT_PLATFORM" | tr -d .)/$SWIFT_VERSION/$SWIFT_VERSION-$SWIFT_PLATFORM.tar.gz \
+    && curl -fSsL $SWIFT_URL -o swift.tar.gz \
+    && curl -fSsL $SWIFT_URL.sig -o swift.tar.gz.sig \
+    && export GNUPGHOME="$(mktemp -d)" \
+    && set -e; \
+        for key in \
+      # pub   4096R/412B37AD 2015-11-19 [expires: 2017-11-18]
+      #       Key fingerprint = 7463 A81A 4B2E EA1B 551F  FBCF D441 C977 412B 37AD
+      # uid                  Swift Automatic Signing Key #1 <[email protected]>
+          7463A81A4B2EEA1B551FFBCFD441C977412B37AD \
+      # pub   4096R/21A56D5F 2015-11-28 [expires: 2017-11-27]
+      #       Key fingerprint = 1BE1 E29A 084C B305 F397  D62A 9F59 7F4D 21A5 6D5F
+      # uid                  Swift 2.2 Release Signing Key <[email protected]>
+          1BE1E29A084CB305F397D62A9F597F4D21A56D5F \
+      # pub   4096R/91D306C6 2016-05-31 [expires: 2018-05-31]
+      #       Key fingerprint = A3BA FD35 56A5 9079 C068  94BD 63BC 1CFE 91D3 06C6
+      # uid                  Swift 3.x Release Signing Key <[email protected]>
+          A3BAFD3556A59079C06894BD63BC1CFE91D306C6 \
+        ; do \
+          gpg --quiet --keyserver ha.pool.sks-keyservers.net --recv-keys "$key"; \
+        done \
+    && gpg --batch --verify --quiet swift.tar.gz.sig swift.tar.gz \
+    && tar -xzf swift.tar.gz --directory / --strip-components=1 \
+    && rm -r "$GNUPGHOME" swift.tar.gz.sig swift.tar.gz
+
+# Print Installed Swift Version
+RUN swift --version

Build test of #2500; c03f413 (swift):

$ bashbrew build swift:3.0.2
Building bashbrew/cache:a4affb128ed89fb65d8dbb8002b984788dfebb96f699b69f835bafc1a97b9b90 (swift:3.0.2)
Tagging swift:3.0.2
Tagging swift:3.0
Tagging swift:3
Tagging swift:latest

$ test/run.sh swift:3.0.2
testing swift:3.0.2
	'utc' [1/5]...passed
	'cve-2014--shellshock' [2/5]...passed
	'no-hard-coded-passwords' [3/5]...passed
	'override-cmd' [4/5]...passed
	'swift-hello-world' [5/5]...passed

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 21, 2017

@yosifkit - thanks so much! I'll fixup the docs today but please merge when ready!

@swizzlr
Copy link
Contributor Author

swizzlr commented Jan 21, 2017

And docs are fixed @tianon @yosifkit .

@tianon tianon merged commit 3f37bb5 into docker-library:master Jan 21, 2017
@hamin
Copy link

hamin commented Jan 21, 2017

❤️❤️❤️❤️👌👌👌😍😍😍😍❤️❤️❤️❤️

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