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

Added label-based mechanism to identify docker-gen container. This is needed w… #205

Closed
wants to merge 4 commits into from

Conversation

chicco785
Copy link

Added label-based mechanism to identify docker-gen container. This is needed when using docker compose or other container mechanisms that define dynamically container's name.

Copy link

@helderco helderco left a comment

Choose a reason for hiding this comment

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

You're making changes to app/nginx_location.conf, which is out of scope here.

Copy link

@helderco helderco left a comment

Choose a reason for hiding this comment

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

There's the assumption here that you always have separate containers, but you could have jwilder/nginx-proxy instead. So this needs to be optional.

@chicco785
Copy link
Author

the change to app/nginx_location.conf may not be required. it's the results of different attempts to fix the issues i was facing in getting the certificate created. but the problem indeed was all related to the impossibility of connecting to the correct container due to the dynamic naming. the com.github.jrcs.letsencrypt_nginx_proxy_companion.nginx_gen label can be optional of course. let me know what you think.

Copy link

@helderco helderco left a comment

Choose a reason for hiding this comment

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

There's the assumption here that you always have separate containers, but you could have jwilder/nginx-proxy instead. So this needs to be optional.

@@ -79,6 +92,7 @@ source /app/functions.sh

if [[ "$*" == "/bin/bash /app/start.sh" ]]; then
check_docker_socket
get_nginx_gen_cid
Copy link

Choose a reason for hiding this comment

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

I think this should be:

[[ -z "${NGINX_DOCKER_GEN_CONTAINER:-}" ]] && get_nginx_gen_cid

to allow still using a manual environment variable when you do have access to a static name.

if [[ ! -z "${labeled_cid:-}" ]]; then
export NGINX_DOCKER_GEN_CONTAINER=$labeled_cid
fi
if [[ -z "${NGINX_DOCKER_GEN_CONTAINER:-}" ]]; then
Copy link

Choose a reason for hiding this comment

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

Remove this condition to make it optional, don't error out. Document it instead.

@@ -1,4 +1,5 @@
location ^~ /.well-known/acme-challenge/ {
location ~ /.well-known {
Copy link

Choose a reason for hiding this comment

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

Out of scope, changes should be discarded.

@helderco
Copy link

helderco commented Jul 6, 2017

I need this, if you hadn't, I would! 👍

@helderco
Copy link

helderco commented Jul 8, 2017

I found an issue with containers getting redeployed during letsencrypt lifecycle, so I made my own attempt with #231. It's working in production right now, with the helder/letsencrypt-nginx-proxy-companion fork (temporary until the PR is merged).

@helderco
Copy link

#231 was merged so this can be closed.

@JrCs
Copy link
Collaborator

JrCs commented Jul 14, 2017

Closed by #231

@JrCs JrCs closed this Jul 14, 2017
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.

3 participants