Che server should connect to ws-agent on internal URL (#2030) #2837

amisevsk · 2016-10-19T20:09:55Z

What does this PR do?

Enables che-server to use the internal address of wsagent containers when it is running
in docker through setting an environment variable.

When CHE_DOCKER_USE_INTERNAL_CONTAINER_ADDRESS is set to "true", the
ServerProperties objects returned as part of DockerInstanceRuntimeInfo#getServers()
will use the internal address of the container as obtained from docker inspect.

If the environment variable is not set, then behaviour is unchanged.

This is a WIP so comments / suggestions are greatly appreciated.

What issues does this PR fix or reference?

Che server should connect to ws-agent on internal URL (#2030)

Previous behavior

Value used for internal address could cause issues contacting wsagent if firewall interfered.

New behavior

Che-server can now optionally communicate with wsagent directly.

PR type

Minor change = no change to existing features or docs
Major change = changes existing features or docs

Minor change checklist

New API required?
API updated
Tests updated
Tests passed

Signed-off-by: Angel Misevski amisevsk@redhat.com

codenvy-ci · 2016-10-19T20:14:01Z

Can one of the admins verify this patch?

TylerJewell · 2016-10-19T20:16:00Z

This improvement addresses issues on certain linux systems where Che does not work with iptables or firewalld. By connecting over the internal address, communication flow will not go through the firewall which blocks access.

TylerJewell · 2016-10-19T20:16:27Z

-[ ] Requires updates to the Networking docs for Eclipse Che page to explain the scenario.

benoitf · 2016-10-19T21:49:21Z

ci-build

codenvy-ci · 2016-10-19T22:35:55Z

Build # 747 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/747/ to view the results.

amisevsk · 2016-10-31T19:37:45Z

I've rebased this PR, and dropped a commit that fixed an issue that has been since fixed in master (see d74b24f).

garagatyi

Please add fixes in accordance to comments

garagatyi · 2016-11-01T08:57:49Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

+     * @return true if {@code CHE_DOCKER_USE_INTERNAL_CONTAINER_ADDRESS} is "true", false otherwise.
+     */
+    protected boolean useInternalContainerAddresses() {
+        String useInternalContainerAddresses = System.getenv(CHE_DOCKER_USE_INTERNAL_CONTAINER_ADDRESS);


In Che we use system variables in a different way. We declare property from DI container and it should use environment variable if it is needed.

Sorry, could you clarify this point? Do you mean that it is preferable to control this behaviour through a property defined in che.properties?

Exactly! And it can also be configured using env variable that match rules of CHE of conversion env variable into property.
The idea is that we use only named injection in Java code and CheBootstrap handles injection of all properties and environment variables and has everything needed to improve code readability.
If you want to know more about that, please, ask.

Thank you, that makes a lot of sense! I've updated the PR to use a property (currently called che.docker.ip.useinternaladdress -- open to suggestions).

A few questions regarding the conversion of env vars to properties though:

in CheBootstrap, I see that env vars are converted by lowercasing them and replacing underscores with periods. However, some of the properties have underscores in their name, which makes it impossible to set them with environment variables, and prevents the new property here from having underscores (che.docker.ip.use_internal_address would make more sense).

In LocalDockerInstanceRuntimeInfo, it looks like this convention is ignored: the methods externalHostnameWithPrecedence and internalHostnameWithPrecedence use environment variables to override their behavior -- is this intentional or should it be fixed?

I submitted a PR to fix that. I'm going to merge it soon. Configuring of named variable with underscore in name doesn't work if it was set using environment variables #2454

Probably it was implemented before we merged feature with automatic conversion. Or we missed it in PR review.
I believe that it should be fixed. @amisevsk If you want you can fix it also.
@TylerJewell should env variables be renamed to match properties names or it is better to try to use new property aliases feature?

@garagatyi Thanks for pointing me to that PR -- I've renamed the property to che.docker.ip.use_internal_address for readability. This has the side effect of not being able to override the property with an environment variable until your change is merged.

I've modified LocalDockerInstanceRuntimeInfo to not use environment variables anymore and updated the docs. Previously, the properties che.docker.ip and che.docker.ip.external were overridden through both the environment variable CHE_DOCKER_IP/CHE_DOCKER_IP_EXTERNAL but also through CHE_DOCKER_MACHINE_HOST and CHE_DOCKER_MACHINE_HOST_EXTERNAL. With the change only the former has an effect.

garagatyi · 2016-11-01T09:00:30Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

@@ -310,16 +316,33 @@ public String projectsRoot() {
    protected Map<String, ServerImpl> getServersWithFilledPorts(final String externalHostame, final String internalHostname, final Map<String, List<PortBinding>> exposedPorts) {
        final HashMap<String, ServerImpl> servers = new LinkedHashMap<>();

+        boolean useMappedPorts = true;
+        if (useInternalContainerAddresses()) {


It is better to do this inside of constructor to keep main code clean

garagatyi · 2016-11-01T09:02:26Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

+        boolean useMappedPorts = true;
+        if (useInternalContainerAddresses()) {
+            if (info.getNetworkSettings() != null && internalHostname.equals(info.getNetworkSettings().getIpAddress())) {
+                useMappedPorts = false;


Can you elaborate why we should care about internalHostname field if usage of IP address provided by Docker is set by configuration?

This was meant mostly as a sanity check, although even as that I'm not sure it's handled as well as it could be. internalHostname is still the address used to communicate with wsagent, and I was concerned that it may be possible to create an instance of DockerInstanceRuntimeInfo with internalHostname not matching info.getNetworkSettings().getIpAddress(), in which case contacting wsagent would fail even if internalHostname is valid.

If you feel that it's not a concern, I will remove it.

My point that if someone configured property/env variable that enables useInternalContainerAddresses mode then he definitely want to use it. And If it does match internalHostname then this mode doesn't make sense.
So I treat this mode as an alternative to default mode with set internalHostname.
Please correct me if I misunderstood the goal of this contribution.

You're right, that was the goal. I'll remove this check and rename the variable for clarity.

garagatyi · 2016-11-01T09:23:58Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

+            if (useMappedPorts) {
+                internalHostnameAndPort = internalHostname + ":" + externalPort;
+            } else {
+                String internalPort = portEntry.getKey().split("/")[0];


It is better to use portProtocol variable here instead of portEntry.getKey()

Add ability to make che-server use internal address of workspaces when che-server is running in docker through property `che.docker.ip.useinternaladdress` or environment variable CHE_DOCKER_IP_USEINTERNALADDRESS. When property is set to "true", internal address is set to the internal address of the relevant wsagent docker container. Otherwise, behavior is unchanged. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

garagatyi · 2016-11-14T10:47:14Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

+            String internalHostnameAndPort;
+            if (useInternalAddress) {
+                String internalPort = portProtocol.split("/")[0];
+                internalHostnameAndPort = internalHostname + ":" + internalPort;


In that case it is supposed that browser will connect to ephemeral port or exposed port? I'm asking because as far as I remember it should be exposed port, but I'm not sure.

To my understanding, the browser will use the exposed port; the internalHostname and internalPort should be used only be che-server.

Do you really mean exposed port or ephemeral port which is mapped to exposed port?

@l0rd Maybe you can help to understand how this workflow is supposed to work? I know you explain such things very clear.

Let's make an example. wsagent is available on ports:

4401 on docker0 network (let's call it the exposed port)

32801 on host network (let's call it the ephemeral port).

Browsers use address: externalHostname:32801
Che server used address internalHostname:32801 (before this PR)

After this PR Che server should use address containerIP:4401

The difference is between ephemeral and exposed ports but also between internalHostname and containerIP:

internalHostname= the hostname of the host where docker is running and in most cases is identical to externalHostname

containerIP= is the IP address of the workspace container, it's an IP of the docker0 network (you can get this IP address using docker inspect -f '{{ .NetworkSettings.IPAddress }}' <containerID>).

Looking to the code of this PR it seems to me internalHostname is still used by the Che server. containerIP should be used instead.

@amisevsk there is a typo in variable externalHostame. You haven't introduced that (that was probably me 😊) but it would be cool if you could fix it => externalHostname

@l0rd I've fixed the typo now. Could you elaborate more on the issue of containerIP vs internalHostname?

@garagatyi Sorry for the confusion -- I meant exposed port in the sense of "exposed to the outside world", e.g. Docker would map the container at 172.17.0.2 with port 4401 (docker0 network) to 172.17.0.1 (address of docker0 network) with port 32771. The browser should contact the container at 172.17.0.1:32771 while the che-server should use 172.17.0.2:4401. I had my definition of exposed and ephemeral backwards.

@amisevsk I understand now that internaHostname can be either containerIP or internalHostname here. The logic to set the right is in class LocalDockerInstanceRuntimeInfo.

Ok, now it is clearer for me. Thanks @l0rd and @amisevsk. Mario explains such things in a really clear way, as always. 😄

garagatyi · 2016-11-14T10:50:25Z

...rc/main/java/org/eclipse/che/plugin/docker/machine/local/LocalDockerInstanceRuntimeInfo.java

- * <p>Value of external hostname can be retrieved from property ${code machine.docker.local_node_host.external} or
- * from environment variable {@code CHE_DOCKER_MACHINE_HOST_EXTERNAL}.<br>
- * Environment variables have higher priority.
+ * <p>If environment variable {@code CHE_DOCKER_IP_USE__INTERNAL__ADDRESS} or


Javadocs should use properties for explanation of usage. Whereas dependency container can inject it in several ways.

I've removed references to environment variables in the javadoc.

l0rd · 2016-11-15T14:53:27Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

@@ -142,6 +142,12 @@ che.docker.ip=NULL
 # This is unusual, but happens for example in Docker for Mac when containers are in a VM.
 che.docker.ip.external=NULL

+# If true, then uses the internal address and port of workspace Docker containers (i.e. within the Docker
+# Docker network) instead of the address and port provided by the Docker daemon. May be necessary if the


I find confusing the sentence "instead of the address and port provided by the Docker daemon". In fact the Docker daemon never provides the address of the container as far as I know. It does maps the ports of a container (that are exposed to the docker0 network) to some corresponding ephemeral ports exposed to the host network. I would rather change it to "instead of the docker host address and the port provided by the Docker daemon".

I've updated the sentence.

l0rd · 2016-11-15T15:55:32Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

+            String internalHostnameAndPort;
+            if (useInternalAddress) {
+                String internalPort = portProtocol.split("/")[0];
+                internalHostnameAndPort = internalHostname + ":" + internalPort;


Let's make an example. wsagent is available on ports:

4401 on docker0 network (let's call it the exposed port)

32801 on host network (let's call it the ephemeral port).

Browsers use address: externalHostname:32801
Che server used address internalHostname:32801 (before this PR)

After this PR Che server should use address containerIP:4401

The difference is between ephemeral and exposed ports but also between internalHostname and containerIP:

internalHostname= the hostname of the host where docker is running and in most cases is identical to externalHostname

containerIP= is the IP address of the workspace container, it's an IP of the docker0 network (you can get this IP address using docker inspect -f '{{ .NetworkSettings.IPAddress }}' <containerID>).

Looking to the code of this PR it seems to me internalHostname is still used by the Che server. containerIP should be used instead.

l0rd · 2016-11-15T16:00:05Z

...r-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstanceRuntimeInfo.java

+            String internalHostnameAndPort;
+            if (useInternalAddress) {
+                String internalPort = portProtocol.split("/")[0];
+                internalHostnameAndPort = internalHostname + ":" + internalPort;


@amisevsk there is a typo in variable externalHostame. You haven't introduced that (that was probably me 😊) but it would be cool if you could fix it => externalHostname

l0rd · 2016-11-15T16:25:35Z

...rc/main/java/org/eclipse/che/plugin/docker/machine/local/LocalDockerInstanceRuntimeInfo.java

+        if (useInternalAddress) {
+            String containerHostName = null;
+            if (networkSettings != null) {
+                containerHostName = networkSettings.getIpAddress();


Is this IP address the IP of the Che server container? What we need is instead the IP addresses of the workspace containers.

As far as I can tell, the ContainerInfo (which provides NetworkSettings) injected into the LocalDockerInstanceRuntimeInfo constructor is obtained from DockerConnector.inspectContainer(), where the container inspect is the workspace container. See DockerInstance.java.

@amisevsk you are right. Sorry about that :-)

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

l0rd · 2016-11-16T12:39:05Z

LGTM good job @amisevsk!

garagatyi · 2016-11-16T15:27:52Z

@l0rd @amisevsk Honestly existing code is already over-complicated. And this PR makes what is happening here even less clear. I'll suggest a way how to refactor this code below. Please share your thoughts about this idea. Do you think that this approach is clearer and more extensible than current one?

We can refactor this code in such a way:
DockerInstanceRuntimeInfo accepts:

servers internal host (nullable, can be IP)
container JSON data object
provider of new class which represents host&port evaluation strategy

... other things not related to this topic
it doesn't accept:

servers external host, since it is always set to null for now - looks like it is useless now

HostPortEvaluationStrategyProvider (name just for example, feel free to use another name) uses property that defines chosen strategy impl and set of strategies impls and return needed HostPortEvaluationStrategy implementation.
HostPortEvaluationStrategyProvider accepts internal host and container JSON and pass it to HostPortEvaluationStrategy impl.
DockerInstanceRuntimeInfo calls HostPortEvaluationStrategy#getServer(String exposedPortProtocol).
HostPortEvaluationStrategy impl forms external/internal addresses using its own logic.
In that case each vendor may bind his own implementation of HostPortEvaluationStrategy and set it as current one for Che assembly.

My description looks a bit complicated when I read it, but it is not really complex. So please ask me for an explanation if this algorithm is not clear enough to you.

l0rd · 2016-11-16T16:07:24Z

@garagatyi I like your approach and I agree that current code is overcomplicated. Just a couple of comments:

Why do you think that servers external host is useless? It should be used when using docker4mac or with K8s/OpenShift etc..
Would it be ok to do the refactoring as a separate PR? That would make the refactoring PR easier to review.

garagatyi · 2016-11-16T16:33:05Z

Why do you think that servers external host is useless? It should be used when using docker4mac or with K8s/OpenShift etc..

I mean it is useless in DockerInstanceRuntimeInfo constructor. Later I would want to refactor DockerInstance code again and remove DockerNode because it looks odd to me. I think we can move internal/external address setting into suggested HostPortEvaluationStrategy.
WDYT?

Would it be ok to do the refactoring as a separate PR? That would make the refactoring PR easier to review

Do you suggest to merge this PR and make another one with refactoring or just open another one with the refactoring? If first then we will have to pass 2 QA cycles - for 1st and 2nd PRs.

l0rd · 2016-11-17T13:18:22Z

I mean it is useless in DockerInstanceRuntimeInfo constructor. Later I would want to refactor DockerInstance code again and remove DockerNode because it looks odd to me. I think we can move internal/external address setting into suggested HostPortEvaluationStrategy.
WDYT?

Ok I see what you mean. It makes sense. And we should move internalhost parameter to HostPortEvaluationStrategy too.

Do you suggest to merge this PR and make another one with refactoring or just open another one with the refactoring? If first then we will have to pass 2 QA cycles - for 1st and 2nd PRs.

I guess that's not ideal because QA cycles are manual and take time. That's ok I will talk with @amisevsk to see how to handle this.

amisevsk · 2016-11-23T07:10:44Z

@garagatyi I agree that the current setup is probably more complicated than it should be, and would be happy to do the refactor you suggest.

For my understanding: Currently, DockerInstanceRuntimeInfo takes internal/external hostname, the ContainerInfo json, and Sets of ServerConf. When DockerInstanceRuntimeInfo#getServers() is called, DockerInstanceRuntimeInfo

gets port mappings and labels from ContainerInfo json
calls DockerInstanceRuntimeInfo#getServersWithFilledPorts(), which
1. parses the ports json object from (1)
2. for each port mapping (e.g. 22/tcp=[PortBinding{hostIp='0.0.0.0', hostPort='32898'}]) gets the protocol (22/tcp) and the external port (32898)
3. adds an entry in a Map<String, ServerImpl> which pairs the desired port (32898) with internal and external hostname (from the constructor)
The Map from (2) is passed to DockerInstanceRuntimeInfo#addRefAndUrlToServers() which updates each ServerImpl, adding ref (e.g 22/tcp -> ssh) and relevant parts of ServerPropertiesImpl (e.g. path, internal URL)
The updated Map from (3) is passed to DockerInstanceRuntimeInfo#AddDefaultReferenceForServersWithoutReference(), which puts a default value for ref if it is not set.

The result of (4) is what is returned by getServers().

If I understand you correctly, you're suggesting that steps 2-4 are moved into the HostPortEvaluationStrategy impl, and DockerInstanceRuntimeInfo#getServers() instead iterates over the port mappings obtained in (1), calling HostPortEvaluationStrategy#getServer(String portProtocol) and adding the returned ServerImpl to the map it returns.

That makes sense to me. A few things I'm still not clear on though. It's maybe important to note that with the set up I'm running, the constructor for DockerInstanceRuntimeInfo is invoked from LocalDockerInstanceRuntimeInfo.

How do we define HostPortEvaluationStrategies? Currently, the modes of function defined by properties seem to be
- Default: use InternalHostname for internal and external address, and external ports
- If containerExternalHostname is provided, use that for external address
- (My changes) If property che.docker.ip.use_internal_address is set, ephemeral ports should be used on internal addresses instead. Additionally, we assume that containerInternalHostname is set correctly so that a route can be established.
This gets muddy quickly; these don't really represent different strategies for evaluating address and ports, so much as they are two options applied on top of default. This means potentially 4 different strategies, since the two options don't affect one another. Or is there one default strategy that takes these options into account, with the potential for more strategies in the future?
I'm not sure I totally understand the role of LocalDockerInstanceRuntimeInfo, as it seems to just call the DockerInstanceRuntimeInfo constructor with the parameters we want.
It is where the options listed above actually take effect, since
- Its constructor is injected with che.docker.ip.external, which it uses as containerExternalHostname in DockerInstanceRuntimeInfo
- With my changes above, it's also resposible for setting containerInternalHostname correctly
This kind of obfuscates the effects of these properties and I think adds to the confusion. Would the refactor get rid of this class, and move this functionality into HostPortEvaluationStrategy? I like this idea since with my changes, the property has to be injected twice -- in LocalDockerInstanceRuntimeInfo to set containerInternalHostname correctly, and in DockerInstanceRuntimeInfo to cause it to use ephemeral ports.

Sorry for the novel, I want to make sure I understand.

garagatyi · 2016-11-23T15:09:27Z

If I understand you correctly, you're suggesting...

Yes, It is what I'm thinking about.

How do we define HostPortEvaluationStrategies? ...

Yes, I suppose it should be a set of different strategies. They can be simple enough and don't need to override some default values. Apparently we will have some default strategy. I assume that we can set property with some strategy as a default and allow to override strategy with properties instead of changing assembly by binding needed strategy in Guice module. If vendor wants to implement new strategy it can contribute it into Che or bind it in customized assembly.

I'm not sure I totally understand the role of LocalDockerInstanceRuntimeInfo ...

Yes, I believe we should get rid of LocalDockerInstanceRuntimeInfo

amisevsk · 2016-12-05T22:44:05Z

I've opened a new pull request which includes the refactor suggested above: #3282

amisevsk · 2016-12-15T15:23:02Z

Closed; issue will be solved in #3282

TylerJewell added this to the 5.0.0-M7 milestone Oct 19, 2016

TylerJewell added kind/enhancement A feature request - must adhere to the feature request template. team/enterprise labels Oct 19, 2016

vkuznyetsov added sprint/next team/production and removed team/enterprise labels Oct 21, 2016

amisevsk force-pushed the CHE-2030-s branch from 7055b1c to 339eb71 Compare October 31, 2016 19:33

garagatyi suggested changes Nov 1, 2016

View reviewed changes

bmicklea modified the milestones: 5.0.0-M8, 5.0.0-M7 Nov 1, 2016

amisevsk force-pushed the CHE-2030-s branch 2 times, most recently from 2504e74 to a0edb9c Compare November 2, 2016 21:31

amisevsk force-pushed the CHE-2030-s branch from a0edb9c to ffdc8d0 Compare November 3, 2016 17:46

vkuznyetsov removed sprint/next team/production labels Nov 10, 2016

garagatyi reviewed Nov 14, 2016

View reviewed changes

garagatyi suggested changes Nov 15, 2016

View reviewed changes

l0rd reviewed Nov 15, 2016

View reviewed changes

l0rd requested changes Nov 15, 2016

View reviewed changes

Update to reflect requested changes

492c067

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

l0rd approved these changes Nov 16, 2016

View reviewed changes

bmicklea added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 16, 2016

riuvshin modified the milestones: 5.0.0-M9, 5.0.0-M8 Dec 2, 2016

amisevsk mentioned this pull request Dec 5, 2016

Refactor DockerInstanceRuntimeInfo#getServers() (#2030) #3282

Merged

2 tasks

amisevsk closed this Dec 15, 2016

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 2, 2017

Che server should connect to ws-agent on internal URL (#2030) #2837

Che server should connect to ws-agent on internal URL (#2030) #2837

Conversation

amisevsk commented Oct 19, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

PR type

Minor change checklist

codenvy-ci commented Oct 19, 2016

TylerJewell commented Oct 19, 2016

TylerJewell commented Oct 19, 2016

benoitf commented Oct 19, 2016

codenvy-ci commented Oct 19, 2016

amisevsk commented Oct 31, 2016

garagatyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rd commented Nov 16, 2016

garagatyi commented Nov 16, 2016

l0rd commented Nov 16, 2016

garagatyi commented Nov 16, 2016 • edited by l0rd Loading

l0rd commented Nov 17, 2016

amisevsk commented Nov 23, 2016

garagatyi commented Nov 23, 2016

amisevsk commented Dec 5, 2016

amisevsk commented Dec 15, 2016

amisevsk commented Oct 19, 2016 •

edited

Loading

garagatyi commented Nov 16, 2016 •

edited by l0rd

Loading