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

Docker: Added warning if network could not be found #1310

Merged
merged 4 commits into from
Mar 19, 2017
Merged

Docker: Added warning if network could not be found #1310

merged 4 commits into from
Mar 19, 2017

Conversation

zweizeichen
Copy link
Contributor

Closes #1308 - avoids rebase entanglements and general time-travel-related corruption :)

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Question from someone who's barely familiar with the Docker integration: would it theoretically be possible to detect the multiple network situation, find the correct prefix, and insert it automatically?

I'm not saying this PR should or should not adjusted if it turns out we can act more intelligently. Just wondering if we can convert a warning asking for user actions into an automatism.

@@ -6,6 +6,7 @@ import (
"math"
"net"
"net/http"
"regexp"
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to remove this line. The CI doesn't like unused imports. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that one slipped through from master ;)

@@ -412,6 +413,8 @@ func (provider *Docker) getIPAddress(container dockerData) string {
if network != nil {
return network.Addr
}

log.Warn("Could not find network named '%s' for container '%s'! Maybe you're missing the project's prefix in the label? Defaulting to first available network.", label, container.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be Warnf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I always mix those up

@timoreimann
Copy link
Contributor

Closes #1156.

@zweizeichen
Copy link
Contributor Author

zweizeichen commented Mar 18, 2017

We could try to detect if the instance of traefik runs under a prefix and then could try to find the missing network by prepending the prefix. If you both have a network proxy and prefix_proxy, but both instances try to connect on network proxy - is this intentional or just a configuration mistake like in #1156? That situation would be interesting to resolve. If we got two potentially matching networks IMHO we should go with the non-prefixed version and print a warning (it could be unintended) as otherwise you could not explicitly use the non-prefixed version at all.

EDIT: I just looked up the situation on my box and the containers get the label com.docker.compose.project, however this probably is not the only way these containers can get prefixes. @CyrilPeponnet for example mentioned stacks. I honestly do not know of all ways containers do and will get prefixed. Maybe we could go with the warning for now and open a new issue for a potential automated solution later on.

@timoreimann
Copy link
Contributor

@zweizeichen definitely sounds like a more sophisticated solution will take some time.

Agree we should get this PR merged now and worry about more cleverness in a separate issue. 👍

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Thanks for the valuable feedback!

@timoreimann
Copy link
Contributor

@emilevauge PTAL. :-)

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks @zweizeichen
LGTM :)

@emilevauge emilevauge merged commit 8010758 into traefik:v1.2 Mar 19, 2017
emilevauge pushed a commit that referenced this pull request Apr 4, 2017
* Added warning if network could not be found

* Removed regex import from master

* Corrected wrong function call
emilevauge pushed a commit that referenced this pull request Apr 4, 2017
* Added warning if network could not be found

* Removed regex import from master

* Corrected wrong function call
emilevauge pushed a commit that referenced this pull request Apr 4, 2017
* Added warning if network could not be found

* Removed regex import from master

* Corrected wrong function call
emilevauge pushed a commit that referenced this pull request Apr 11, 2017
* Added warning if network could not be found

* Removed regex import from master

* Corrected wrong function call
emilevauge pushed a commit that referenced this pull request Apr 11, 2017
* Added warning if network could not be found

* Removed regex import from master

* Corrected wrong function call
@ldez ldez added kind/enhancement a new or improved feature. and removed status/2-needs-review labels Apr 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants