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

Fix docker-gen breaking on new docker version (See #335) #336

Merged
merged 1 commit into from
Apr 3, 2021

Conversation

NicolasDorier
Copy link
Contributor

This PR is scanning for the full container ID by getting the short container id via HOSTNAME, then trying to fetch the rest of it in "/proc/self/cgroup" or "/proc/self/mountinfo".

So no breaking change to old versions of docker.

Alternative to #335

Note I did not tested on new docker, but I tested on old one and nothing break.

@NicolasDorier
Copy link
Contributor Author

Tested on new docker, and this fix the issue.

@NicolasDorier
Copy link
Contributor Author

This comment indicates there is a corner case that might not be handled by this PR. #335 (comment)

@NicolasDorier NicolasDorier deleted the quwn3e branch April 4, 2021 14:01
@buchdag
Copy link
Member

buchdag commented Apr 4, 2021

@NicolasDorier does the comment in #335 means that docker-gen now cannot get its own ID if you set a custom host name for the container ?

@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Apr 5, 2021

@buchdag I don't really know, I don't use a custom host name, so I did not tried. Just mentioning it here. The comment mention /etc/hostname but I use $HOSTNAME so that might be OK, I just did not tried.

@@ -185,7 +186,8 @@ func GetCurrentContainerID() string {
}

func matchDockerCurrentContainerID(lines string) string {
regex := "/docker[/-]([[:alnum:]]{64})(\\.scope)?$"
hostname := os.Getenv("HOSTNAME")
regex := fmt.Sprintf("(%s[[:alnum:]]{52})", hostname)
Copy link
Member

Choose a reason for hiding this comment

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

@NicolasDorier @jwilder this breaks the tests:

--- FAIL: TestGetCurrentContainerID_DockerCE (0.00s)
    context_test.go:49: id mismatch: got 18862cabc2e0d24142cf93c46ccb6e070c2ea7b996c81c0311ec, exp 18862cabc2e0d24142cf93c46ccb6e070c2ea7b996c81c0311ec0309abcbcdfb
FAIL
exit status 1
FAIL    github.com/jwilder/docker-gen   0.596s

@buchdag
Copy link
Member

buchdag commented Apr 5, 2021

Unfortunately I confirm that this PR broke docker-gen containers that uses a custom host name via --hostname (and result in empty upstream blocks on the rendered nginx config).

@buchdag
Copy link
Member

buchdag commented Apr 5, 2021

Steps to reproduce:

$ docker network create nginx-proxy

$ docker run --detach \
> --name nginx-proxy \
> --network nginx-proxy \
> --publish 80:80 \
> --publish 443:443 \
> --volume conf:/etc/nginx/conf.d \
> nginx

$ docker run --detach \
> --name nginx-proxy-gen \
> --network nginx-proxy \
> --hostname dockergen \
> --volumes-from nginx-proxy \
> --volume /path/to/nginx.tmpl:/etc/docker-gen/templates/nginx.tmpl:ro \
> --volume /var/run/docker.sock:/tmp/docker.sock:ro \
> jwilder/docker-gen \
> -notify-sighup nginx-proxy -watch -wait 5s:30s /etc/docker-gen/templates/nginx.tmpl /etc/nginx/conf.d/default.conf

$ docker run --detach \
> --name proxyed-app \
> --network nginx-proxy \
> --env "VIRTUAL_HOST=domain.tld" \
> nginx

$ sleep 10; docker exec nginx-proxy cat /etc/nginx/conf.d/default.conf

The resulting /etc/nginx/conf.d/default.conf:

# If we receive X-Forwarded-Proto, pass it through; otherwise, pass along the
# scheme used to connect to this server
map $http_x_forwarded_proto $proxy_x_forwarded_proto {
  default $http_x_forwarded_proto;
  ''      $scheme;
}
# If we receive X-Forwarded-Port, pass it through; otherwise, pass along the
# server port the client connected to
map $http_x_forwarded_port $proxy_x_forwarded_port {
  default $http_x_forwarded_port;
  ''      $server_port;
}
# If we receive Upgrade, set Connection to "upgrade"; otherwise, delete any
# Connection header that may have been passed to this server
map $http_upgrade $proxy_connection {
  default upgrade;
  '' close;
}
# Apply fix for very long server names
server_names_hash_bucket_size 128;
# Default dhparam
# Set appropriate X-Forwarded-Ssl header
map $scheme $proxy_x_forwarded_ssl {
  default off;
  https on;
}
gzip_types text/plain text/css application/javascript application/json application/x-javascript text/xml application/xml application/xml+rss text/javascript;
log_format vhost '$host $remote_addr - $remote_user [$time_local] '
                 '"$request" $status $body_bytes_sent '
                 '"$http_referer" "$http_user_agent"';
access_log off;
		ssl_protocols TLSv1.2 TLSv1.3;
		ssl_ciphers 'ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384';
		ssl_prefer_server_ciphers off;
# HTTP 1.1 support
proxy_http_version 1.1;
proxy_buffering off;
proxy_set_header Host $http_host;
proxy_set_header Upgrade $http_upgrade;
proxy_set_header Connection $proxy_connection;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $proxy_x_forwarded_proto;
proxy_set_header X-Forwarded-Ssl $proxy_x_forwarded_ssl;
proxy_set_header X-Forwarded-Port $proxy_x_forwarded_port;
# Mitigate httpoxy attack (see README for details)
proxy_set_header Proxy "";
server {
	server_name _; # This is just an invalid value which will never trigger on a real hostname.
	listen 80;
	access_log /var/log/nginx/access.log vhost;
	return 503;
}
# domain.tld
upstream domain.tld {
}
server {
	server_name domain.tld;
	listen 80 ;
	access_log /var/log/nginx/access.log vhost;
	location / {
		proxy_pass http://domain.tld;
	}
}

@hanhpv
Copy link

hanhpv commented Apr 11, 2021

Docker version 20.10.5, build 55c4c88966
Got an empty upstream:
image

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