fix getDockerHostIpAddress within docker container #648
Conversation
| @@ -17,42 +17,43 @@ | |||
| // See https://github.com/docker/docker/blob/a9fa38b1edf30b23cae3eade0be48b3d4b1de14b/daemon/initlayer/setup_unix.go#L25 | |||
| public static final boolean IN_A_CONTAINER = new File("/.dockerenv").exists(); | |||
|
|
|||
| @Getter(lazy = true) | |||
There was a problem hiding this comment.
Any reason why you dropped lazily cached result? This change will make it run every time somebody queries the host ip, and I don't see how does it fix it.
Also, was it broken?
There was a problem hiding this comment.
my bad you are right i will refactor it to lazy cache again.
and yes it is broken if we run it in GitLab CI directly in the build container with the DinD service it resolves to the default gateway, however it should resolve to tcp://docker:2375 or just docker
| )) | ||
| .map(StringUtils::trimToEmpty) | ||
| .filter(StringUtils::isNotBlank); | ||
| private static final Optional<String> defaultGateway = Optional |
There was a problem hiding this comment.
I know that the indentation was incorrect, but could you please revert the change to the actual change? We will fix the indentation afterwards, but it will help to understand what was modified :)
There was a problem hiding this comment.
I think the whole change is as simple as getDefaultGateway().orElse("localhost") in getDockerHostIpAddress
| @@ -18,10 +18,8 @@ | |||
| public static final boolean IN_A_CONTAINER = new File("/.dockerenv").exists(); | |||
|
|
|||
| @Getter(lazy = true) | |||
| private static final Optional<String> detectedDockerHostIp = Optional | |||
There was a problem hiding this comment.
Since this getter is public, I don't think we can just change the behaviour of it :( Could you please make this change backward compatible?
|
so finally it is backward compatible so |
|
|
||
| @Deprecated | ||
| @Getter(lazy = true) | ||
| private static final Optional<String> detectedDockerHostIp = IN_A_CONTAINER ? getDefaultGateway() : Optional.empty(); |
There was a problem hiding this comment.
could you please put this method upper where it was, so that the diff will be smaller?
| } catch (Exception e) { | ||
| log.warn("Can't parse the default gateway IP", e); | ||
| return null; | ||
| } | ||
| } | ||
| )) | ||
| .map(StringUtils::trimToEmpty) |
There was a problem hiding this comment.
TBH I would prefer to keep this chain
… differ from default
|
now the diff look quite neat |
|
@qoomon merged, thanks! |
|
@qoomon oh snap, I forgot the changelog :D Will add it myself, thanks for your contribution 👍 |
only determine default gateway within docker container if docker host differ from default, otherwise it was set manually and it should be used as it was set