Skip to content

Update program versions and remodel Dockerfile#54

Merged
thomasjpfan merged 7 commits intodocker-flow:masterfrom
aisbergg:master
Sep 9, 2018
Merged

Update program versions and remodel Dockerfile#54
thomasjpfan merged 7 commits intodocker-flow:masterfrom
aisbergg:master

Conversation

@aisbergg
Copy link
Copy Markdown
Contributor

Notable Changes:

  • Update HAProxy and Golang version
  • Use Alpine version of Golang to prevent problems with linking
  • Reduce number of Docker image layers and also overall size
  • Replace deprecated 'MAINTAINER' command with label scheme from opencontainers.org
  • Use more secure TLS ciphers as default

Notable Changes:
* Update HAProxy and Golang version
* Use Alpine version of Golang to prevent problems with linking
* Reduce number of Docker image layers and also overall size
* Replace deprecated 'MAINTAINER' command with label scheme from opencontainers.org
* Use more secure TLS ciphers as default
@thomasjpfan
Copy link
Copy Markdown
Contributor

Thank you for the PR!

It looks like changing c < 200 to c < 1000, fixes the issue on the CI:

for c := 0; c < 200; c++ {

Copy link
Copy Markdown
Contributor

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

The label spec is nice to have. Overall a good PR!

Comment thread Dockerfile Outdated
@@ -1,16 +1,24 @@
FROM golang:1.9.6 AS build
FROM golang:1.10-alpine AS build
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the build to be reproducible, please include the patch number: 1.10.3-alpine3.8.

Comment thread Dockerfile Outdated

FROM haproxy:1.8.8-alpine
MAINTAINER Viktor Farcic <viktor@farcic.com>
FROM haproxy:1.8-alpine
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For reproducibility, please include the patch number: 1.8.13-alpine.

Comment thread Dockerfile Outdated
MAINTAINER Viktor Farcic <viktor@farcic.com>
FROM haproxy:1.8-alpine
LABEL org.opencontainers.image.title="Docker Flow Proxy" \
org.opencontainers.image.version="18.08.26" \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The jenkins CI server generates the version depending on the date. Hard coding the version into the Dockerfile would most likely lead the mismatching of the tag and this version label.

@thomasjpfan
Copy link
Copy Markdown
Contributor

#54 (comment) to fix the CI issue.

@aisbergg
Copy link
Copy Markdown
Contributor Author

I hope this will do

Comment thread Dockerfile
RUN go get -d -v -t
RUN go test --cover ./... --run UnitTest
RUN go build -v -o docker-flow-proxy
RUN set -x \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why set -x?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the commands are chained together into one large command it is somtimes hard to figure which of those provoked an error occurred in the build process. The set -x turns the tracing functionality on so before execution of each individual command the command will be printed as text. This is also common practise in a lot of official Dockerfiles, for example: MariaDB or Nextcloud

@thomasjpfan
Copy link
Copy Markdown
Contributor

@aisbergg Can you do a empty commit to trigger the CI by running the following and pushing?

git commit --allow-empty -m "Trigger CI"

It is strange how the CI did not run on the latest commit.

@aisbergg
Copy link
Copy Markdown
Contributor Author

aisbergg commented Sep 4, 2018

Well, 10s should be fairly enough for syslog to start. Is this maybe not a timing problem?

@thomasjpfan
Copy link
Copy Markdown
Contributor

I ran into this issue when trying to use golang 1.10.* before. The logging code is found here:

https://github.com/docker-flow/docker-flow-proxy/blob/master/logging/logging.go

It has a dependency on https://github.com/ziutek/syslog. We would need to do some debugging to figure out what is causing the issue.

@aisbergg
Copy link
Copy Markdown
Contributor Author

aisbergg commented Sep 4, 2018

Using the latest 1.11.0 version works

@thomasjpfan
Copy link
Copy Markdown
Contributor

@vfarcic Are you okay with using the org.opencontainers.img labels? The spec is defined here: https://github.com/opencontainers/image-spec/blob/master/annotations.md

@vfarcic
Copy link
Copy Markdown
Contributor

vfarcic commented Sep 4, 2018

That sounds great. As a matter of fact, I have it on my TODO list but I never managed to get time.

@thomasjpfan
Copy link
Copy Markdown
Contributor

@aisbergg The docs needs to be updated to reflect the change in SSL bind ciphers/options:

https://github.com/docker-flow/docker-flow-proxy/blob/master/docs/config.md

@aisbergg
Copy link
Copy Markdown
Contributor Author

aisbergg commented Sep 5, 2018

The other TLS related options for example in https://github.com/docker-flow/docker-flow-proxy/blob/master/proxy/ha_proxy.go#L331-L332 could also be changed. But then you have to make sure the ARM version also uses an up to date version of HAProxy and OpenSSL. I can take on the ARM Dockerfile and remodel this one as well if you like.

@thomasjpfan
Copy link
Copy Markdown
Contributor

I will be expecting some backwards compatibility issues with changing the default SSL bind/ciphers. There are some that need to support Win XP browers/clients. We would need to direct them to set the SSL options themselves. @vfarcic Do you have any thoughts on this matter?

@aisbergg The ARM version can be done with another PR.

@aisbergg
Copy link
Copy Markdown
Contributor Author

aisbergg commented Sep 6, 2018

For configurations for backward compatibility a reference link to this Mozilla wiki page might helpful to the end user. There are three profiles listed for different compatibility levels: ‘Modern’, ‘Intermediate’ and ‘Old’. An example Docker-Compose file for the Intermediate one would be:

proxy:
  image: dockerflow/docker-flow-proxy
...
  environment:
    - SSL_BIND_OPTIONS=ssl-min-ver TLSv1.0 no-tls-tickets
    - SSL_BIND_CIPHERS=ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
...

But as for the Old standard I would highly suggest not to enable SSLv3.

@thomasjpfan
Copy link
Copy Markdown
Contributor

@aisbergg I'll merge this PR with the change in defaults. Thank you for the PR!

@thomasjpfan thomasjpfan merged commit 83a7fd8 into docker-flow:master Sep 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants