Skip to content

[varnish] new 6.4#7662

Merged
yosifkit merged 1 commit intodocker-library:masterfrom
gquintard:master
Mar 25, 2020
Merged

[varnish] new 6.4#7662
yosifkit merged 1 commit intodocker-library:masterfrom
gquintard:master

Conversation

@gquintard
Copy link
Contributor

No description provided.

@tianon
Copy link
Member

tianon commented Mar 23, 2020

Diff:
diff --git a/_bashbrew-list b/_bashbrew-list
index c420637..1f7d311 100644
--- a/_bashbrew-list
+++ b/_bashbrew-list
@@ -2,9 +2,9 @@ varnish:6
 varnish:6.0
 varnish:6.0.6
 varnish:6.0.6-1
-varnish:6.3
-varnish:6.3.2
-varnish:6.3.2-1
+varnish:6.4
+varnish:6.4.0
+varnish:6.4.0-1
 varnish:fresh
 varnish:latest
 varnish:stable
diff --git a/varnish_fresh/Dockerfile b/varnish_fresh/Dockerfile
index 747fb8b..e084341 100644
--- a/varnish_fresh/Dockerfile
+++ b/varnish_fresh/Dockerfile
@@ -1,6 +1,7 @@
-FROM debian:stretch-slim
+FROM debian:buster-slim
 
-ENV VARNISH_VERSION 6.3.2-1~stretch
+ENV VARNISH_VERSION 6.4.0-1~buster
+ENV SIZE 100M
 
 RUN set -ex; \
 	fetchDeps=" \
@@ -9,13 +10,13 @@ RUN set -ex; \
 	"; \
 	apt-get update; \
 	apt-get install -y --no-install-recommends apt-transport-https ca-certificates $fetchDeps; \
-	key=920A8A7AA7120A8604BCCD294A42CD6EB810E55D; \
+	key=A9897320C397E3A60C03E8BF821AD320F71BFF3D; \
 	export GNUPGHOME="$(mktemp -d)"; \
-	gpg --batch --keyserver http://ha.pool.sks-keyservers.net/ --recv-keys $key; \
+	gpg --batch --keyserver hkps://hkps.pool.sks-keyservers.net --recv-keys $key; \
 	gpg --batch --export export $key > /etc/apt/trusted.gpg.d/varnish.gpg; \
 	gpgconf --kill all; \
 	rm -rf $GNUPGHOME; \
-	echo deb https://packagecloud.io/varnishcache/varnish63/debian/ stretch main > /etc/apt/sources.list.d/varnish.list; \
+	echo deb https://packagecloud.io/varnishcache/varnish64/debian/ buster main > /etc/apt/sources.list.d/varnish.list; \
 	apt-get update; \
 	apt-get install -y --no-install-recommends varnish=$VARNISH_VERSION; \
 	apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false $fetchDeps; \
@@ -26,5 +27,5 @@ WORKDIR /etc/varnish
 COPY docker-varnish-entrypoint /usr/local/bin/
 ENTRYPOINT ["docker-varnish-entrypoint"]
 
-EXPOSE 80
-CMD ["varnishd", "-F", "-f", "/etc/varnish/default.vcl"]
+EXPOSE 80 8443
+CMD varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE
diff --git a/varnish_stable/Dockerfile b/varnish_stable/Dockerfile
index 496d655..5baf337 100644
--- a/varnish_stable/Dockerfile
+++ b/varnish_stable/Dockerfile
@@ -1,6 +1,7 @@
 FROM debian:stretch-slim
 
 ENV VARNISH_VERSION 6.0.6-1~stretch
+ENV SIZE 100M
 
 RUN set -ex; \
 	fetchDeps=" \
@@ -11,7 +12,7 @@ RUN set -ex; \
 	apt-get install -y --no-install-recommends apt-transport-https ca-certificates $fetchDeps; \
 	key=48D81A24CB0456F5D59431D94CFCFD6BA750EDCD; \
 	export GNUPGHOME="$(mktemp -d)"; \
-	gpg --batch --keyserver http://ha.pool.sks-keyservers.net/ --recv-keys $key; \
+	gpg --batch --keyserver hkps://hkps.pool.sks-keyservers.net --recv-keys $key; \
 	gpg --batch --export export $key > /etc/apt/trusted.gpg.d/varnish.gpg; \
 	gpgconf --kill all; \
 	rm -rf $GNUPGHOME; \
@@ -26,5 +27,5 @@ WORKDIR /etc/varnish
 COPY docker-varnish-entrypoint /usr/local/bin/
 ENTRYPOINT ["docker-varnish-entrypoint"]
 
-EXPOSE 80
-CMD ["varnishd", "-F", "-f", "/etc/varnish/default.vcl"]
+EXPOSE 80 8443
+CMD varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE

@pacifier17
Copy link

@tianon I was wondering will this update be merged in today and the version available on Docker Hub?

@gquintard
Copy link
Contributor Author

gquintard commented Mar 23, 2020

@pacifier17, I appreciate the enthusiasm, but please let people work on these issue. Hounding us is distracting and doesn't make things happen faster.

@pacifier17
Copy link

Sure, I didn't mean to. It was just an innocent query :-)

@tianon
Copy link
Member

tianon commented Mar 23, 2020

@gquintard this change has me a little concerned:

-CMD ["varnishd", "-F", "-f", "/etc/varnish/default.vcl"]
+CMD varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE

I've got two concerns with this -- the $SIZE name is really generic, and could potentially have more overlap with other software (limited impact since this is a container, but something to consider).

The larger concern is the switch from JSON syntax to going through the shell, since we've seen a lot of issues over the years around passing through the shell. Is there not another way to configure this default? If not, we should probably at least add an explicit exec to make sure the shell goes away and is replaced by varnishd, ala:

CMD exec varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE

@gquintard
Copy link
Contributor Author

@gquintard this change has me a little concerned:

-CMD ["varnishd", "-F", "-f", "/etc/varnish/default.vcl"]
+CMD varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE

I've got two concerns with this -- the $SIZE name is really generic, and could potentially have more overlap with other software (limited impact since this is a container, but something to consider).

@tianon, very valid concern indeed. We'll change it

The larger concern is the switch from JSON syntax to going through the shell, since we've seen a lot of issues over the years around passing through the shell. Is there not another way to configure this default? If not, we should probably at least add an explicit exec to make sure the shell goes away and is replaced by varnishd, ala:

CMD exec varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE

Just to be sure I understand you correctly, you are concerned that we are not doing:

CMD [ "varnishd", "-F", "-f", "/etc/varnish/default.vcl", "-a", "http=:80,HTTP", "-a", "proxy=:8443,PROXY", "-s", "malloc,$SIZE" ]

? If so, I'm sure we can reestablish it. I think we didn't realize the impact of it.

@yosifkit
Copy link
Member

Yeah, unfortunately the json syntax (skipping the sh -c that docker adds) won't work with your new line since you wanted the variable to be interpreted.

CMD [ "varnishd", "-F", "-f", "/etc/varnish/default.vcl", "-a", "http=:80,HTTP", "-a", "proxy=:8443,PROXY", "-s", "malloc,$SIZE" ]

So @tianon's solution is to make sure the shell gets out of the way by adding exec; this will ensure that signals get correctly delivered to varnishd.

CMD exec varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE
# equivalent json version would be:
CMD [ "sh", "-c", "exec varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE" ]

Not required, just food for thought:

You could alternatively expand that by adding a small entrypoint script (like memcached) that will look at the first argument for a flag and then add defaults; so something like this:

#!/bin/sh
set -e

# first arg is a flag like `-f`
if [ "${1#-}" != "$1" ]; then
	# possibly some way to disable the defaults?
	# or just tell users to use "docker run -d --entrypoint varnishd varnish:6 [varnishd-flags]"
	set -- varnishd -F -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE "$@"
fi

exec "$@"

With the Dockerfile being similar to:

ENTRYPOINT ["name-of-entrypoint.sh"]
# unsure about how much of the defaults would be here vs in the script
CMD ["-f", "/etc/varnish/default.vcl"]

@gquintard
Copy link
Contributor Author

thank you both of you, that make things a lot clearer.

I think we messed up somewhere, because we already have https://github.com/varnish/docker-varnish/blob/master/docker-varnish-entrypoint , but we aren't using it...

So, if I read you correctly, we just need to update the CMD line to

CMD [ "-F", "-f", "/etc/varnish/default.vcl", "-a", "http=:80,HTTP", "-a", "proxy=:8443,PROXY", "-s", "malloc,$SIZE" ]

and docker-varnish-entrypoint will do the exec for us. Did I get that right?

@tianon
Copy link
Member

tianon commented Mar 23, 2020

Unfortunately not -- that will make the value of -s the literal string malloc,$SIZE (it won't interpret the $SIZE environment variable).

@gquintard
Copy link
Contributor Author

nyyyyyh, getting there...
so, have docker-varnish-entrypoint define the default command line and use it if called without argument?

meaning CMD [] in the Dockerfile?

I'm also happy with

CMD exec varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$SIZE

I think both are fine (if I understood), so it's really up to best practices

@yosifkit
Copy link
Member

I think I'd go with putting them all in the entrypoint, rather than the CMD exec. This will keep it to a single shell process that execs varnishd rather than having the shell (entrypoint) exec another shell (sh -c) to then exec varnishd.

It is up to you if you want to only provide the default flags when the CMD (args to the entrypoint script) is empty or to have some other detection to add the defaults to user-supplied flags (I am uncertain if later flags to varnishd override earlier flags).

One quick reminder is to make sure that the entrypoint interface doesn't dramatically change so that things like docker run -it --rm varnish bash still work (https://github.com/docker-library/official-images#consistency).

@gquintard
Copy link
Contributor Author

gquintard commented Mar 25, 2020

@yosifkit , @tianon , thanks a lot for the lesson, that makes sense.

I've renamed the SIZE variable and stash the command in the entrypoint script, so I think we are ok now. Please let me know I missed anything

@yosifkit
Copy link
Member

Diff:
diff --git a/_bashbrew-list b/_bashbrew-list
index c420637..1f7d311 100644
--- a/_bashbrew-list
+++ b/_bashbrew-list
@@ -2,9 +2,9 @@ varnish:6
 varnish:6.0
 varnish:6.0.6
 varnish:6.0.6-1
-varnish:6.3
-varnish:6.3.2
-varnish:6.3.2-1
+varnish:6.4
+varnish:6.4.0
+varnish:6.4.0-1
 varnish:fresh
 varnish:latest
 varnish:stable
diff --git a/varnish_fresh/Dockerfile b/varnish_fresh/Dockerfile
index 747fb8b..365876c 100644
--- a/varnish_fresh/Dockerfile
+++ b/varnish_fresh/Dockerfile
@@ -1,6 +1,7 @@
-FROM debian:stretch-slim
+FROM debian:buster-slim
 
-ENV VARNISH_VERSION 6.3.2-1~stretch
+ENV VARNISH_VERSION 6.4.0-1~buster
+ENV VARNISH_SIZE 100M
 
 RUN set -ex; \
 	fetchDeps=" \
@@ -9,13 +10,13 @@ RUN set -ex; \
 	"; \
 	apt-get update; \
 	apt-get install -y --no-install-recommends apt-transport-https ca-certificates $fetchDeps; \
-	key=920A8A7AA7120A8604BCCD294A42CD6EB810E55D; \
+	key=A9897320C397E3A60C03E8BF821AD320F71BFF3D; \
 	export GNUPGHOME="$(mktemp -d)"; \
-	gpg --batch --keyserver http://ha.pool.sks-keyservers.net/ --recv-keys $key; \
+	gpg --batch --keyserver hkps://hkps.pool.sks-keyservers.net --recv-keys $key; \
 	gpg --batch --export export $key > /etc/apt/trusted.gpg.d/varnish.gpg; \
 	gpgconf --kill all; \
 	rm -rf $GNUPGHOME; \
-	echo deb https://packagecloud.io/varnishcache/varnish63/debian/ stretch main > /etc/apt/sources.list.d/varnish.list; \
+	echo deb https://packagecloud.io/varnishcache/varnish64/debian/ buster main > /etc/apt/sources.list.d/varnish.list; \
 	apt-get update; \
 	apt-get install -y --no-install-recommends varnish=$VARNISH_VERSION; \
 	apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false $fetchDeps; \
@@ -26,5 +27,5 @@ WORKDIR /etc/varnish
 COPY docker-varnish-entrypoint /usr/local/bin/
 ENTRYPOINT ["docker-varnish-entrypoint"]
 
-EXPOSE 80
-CMD ["varnishd", "-F", "-f", "/etc/varnish/default.vcl"]
+EXPOSE 80 8443
+CMD []
diff --git a/varnish_fresh/docker-varnish-entrypoint b/varnish_fresh/docker-varnish-entrypoint
index bb1e360..e1f02c8 100755
--- a/varnish_fresh/docker-varnish-entrypoint
+++ b/varnish_fresh/docker-varnish-entrypoint
@@ -6,7 +6,7 @@ set -e
 # but only works if all arguments require a hyphenated flag
 # -v; -SL; -f arg; etc will work, but not arg1 arg2
 if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
-    set -- varnishd "$@"
+    set -- varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$VARNISH_SIZE "$@"
 fi
 
 exec "$@"
diff --git a/varnish_stable/Dockerfile b/varnish_stable/Dockerfile
index 496d655..68dc1d9 100644
--- a/varnish_stable/Dockerfile
+++ b/varnish_stable/Dockerfile
@@ -1,6 +1,7 @@
 FROM debian:stretch-slim
 
 ENV VARNISH_VERSION 6.0.6-1~stretch
+ENV VARNISH_SIZE 100M
 
 RUN set -ex; \
 	fetchDeps=" \
@@ -11,7 +12,7 @@ RUN set -ex; \
 	apt-get install -y --no-install-recommends apt-transport-https ca-certificates $fetchDeps; \
 	key=48D81A24CB0456F5D59431D94CFCFD6BA750EDCD; \
 	export GNUPGHOME="$(mktemp -d)"; \
-	gpg --batch --keyserver http://ha.pool.sks-keyservers.net/ --recv-keys $key; \
+	gpg --batch --keyserver hkps://hkps.pool.sks-keyservers.net --recv-keys $key; \
 	gpg --batch --export export $key > /etc/apt/trusted.gpg.d/varnish.gpg; \
 	gpgconf --kill all; \
 	rm -rf $GNUPGHOME; \
@@ -26,5 +27,5 @@ WORKDIR /etc/varnish
 COPY docker-varnish-entrypoint /usr/local/bin/
 ENTRYPOINT ["docker-varnish-entrypoint"]
 
-EXPOSE 80
-CMD ["varnishd", "-F", "-f", "/etc/varnish/default.vcl"]
+EXPOSE 80 8443
+CMD []
diff --git a/varnish_stable/docker-varnish-entrypoint b/varnish_stable/docker-varnish-entrypoint
index bb1e360..e1f02c8 100755
--- a/varnish_stable/docker-varnish-entrypoint
+++ b/varnish_stable/docker-varnish-entrypoint
@@ -6,7 +6,7 @@ set -e
 # but only works if all arguments require a hyphenated flag
 # -v; -SL; -f arg; etc will work, but not arg1 arg2
 if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
-    set -- varnishd "$@"
+    set -- varnishd -F -f /etc/varnish/default.vcl -a http=:80,HTTP -a proxy=:8443,PROXY -s malloc,$VARNISH_SIZE "$@"
 fi
 
 exec "$@"

@yosifkit
Copy link
Member

Build test of #7662; 528d1f2; amd64 (varnish):

$ bashbrew build varnish:6.0
Building bashbrew/cache:b2faa73d03b58bea3d9ca1b0ac152b053a48c01b158d5928f2ef5947751e2c58 (varnish:6.0)
Tagging varnish:6.0
Tagging varnish:6.0.6-1
Tagging varnish:6.0.6
Tagging varnish:stable

$ test/run.sh varnish:6.0
testing varnish:6.0
	'utc' [1/4]...passed
	'cve-2014--shellshock' [2/4]...passed
	'no-hard-coded-passwords' [3/4]...passed
	'override-cmd' [4/4]...passed


$ bashbrew build varnish:6.4
Building bashbrew/cache:96cba38967c1a9625b9dd0f49c5e03ebd4894b229098722bf56f8106e38ddb75 (varnish:6.4)
Tagging varnish:6.4
Tagging varnish:6.4.0-1
Tagging varnish:6.4.0
Tagging varnish:6
Tagging varnish:latest
Tagging varnish:fresh

$ test/run.sh varnish:6.4
testing varnish:6.4
	'utc' [1/4]...passed
	'cve-2014--shellshock' [2/4]...passed
	'no-hard-coded-passwords' [3/4]...passed
	'override-cmd' [4/4]...passed

@yosifkit yosifkit merged commit bfa2057 into docker-library:master Mar 25, 2020
@tianon tianon mentioned this pull request May 8, 2020
9 tasks
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