Expose Che server and workspaces through one single TCP port #4269

l0rd · 2017-02-28T22:13:24Z

What does this PR do?

Add a new ServerEvaluationStrategy: SinglePortLocalDockerServerEvaluationStrategy.

This server strategy can be enabled adding che.docker.server_evaluation_strategy=single-port
in che.properties.

When the single-port strategy is enabled a workspace server address has the format server-ref.workspace-id.che-address like for example:

terminal.79rfwhqaztq2ru2k.che.local

To use the single-port strategy a reverse proxy should be used. This PR comes with the changes needed to use OpenShift embedded reverse-proxy too.

What issues does this PR fix or reference?

#2847 #1560

Changelog

Added single-port server evaluation strategy to expose one single TCP port for all Che services

Release Notes

Expose Che server and workspace servers through one TCP port adding che.docker.server_evaluation_strategy=single-port
in che.properties.

Docs PR

How to use single-port strategy with OpenShift embedded reverse proxy will described in the upcoming documentation to run Che with OpenShift.

Signed-off-by: Mario Loriedo <[email protected]>

codenvy-ci · 2017-02-28T22:14:02Z

Can one of the admins verify this patch?

garagatyi · 2017-03-01T07:26:15Z

Can you point in issue description relation of this PR to mentioned issues. Some people might think that it fixes both issues whereas it is just related to it.

Please re-phrase changelog to reflect then fact that it works with external reverse proxy (Openshift only or can be another one?).

Can you elaborate on how to access terminals from 2 machines in single workspace with this notation?

TylerJewell · 2017-03-01T07:50:59Z

@l0rd - thank you for the contribution!

Let's identify a plan for what we would like to have accomplished before we merge this PR. First, I have added a couple additional maintainers as reviewers for this issue. We'd like to make sure that any changes here are compatible with other custom assemblies, particularly Codenvy as well.

Some items that I'd like to organize for this PR:

A demo review of this implementation with a couple maintainers for a Q&A.
Instructions on how to deploy this configuration with traefik or nginx.
Discuss how to expose configuring this in the che.env file.
Discuss a configuration of the che.env that also launches the reverse proxy.
Updating the docs to reflect these various configurations - particularly port requirements.
Are there any implications to the new diagnostic page in the UD?

benoitf · 2017-03-01T08:24:01Z

@TylerJewell
if URL provided by REST API are in the format "terminal.79rfwhqaztq2ru2k.che.local" that is not directly reachable yes that would be an issue for diagnostic tool that connect directly to the URLs provided by server side.

l0rd · 2017-03-01T08:25:29Z

@TylerJewell please note that this single port strategy is enabled only if property che.docker.server_evaluation_strategy is set to singel-port. And by default it is not.

In other words this PR, as the rest of the OpenShift connector related PR, should not break anything and can be merged safely. And even if I agree with all your points this PR should not be blocked by these. We can merge it now (that will help us and won't break anything) and still continue improving it together via other PRs in the next weeks.

benoitf · 2017-03-01T08:27:19Z

for me the wording "single-port" is not obvious as it requires extra front-end tools to use that. It could be seen by end users as a property that can be used directly while it won't work directly.

TylerJewell · 2017-03-01T08:37:28Z

@l0rd - while the initiation of this was for openshift, it's a core evaluation strategy, so will only want to merge this into the system until we have provided for enhancements to the various utilities that will be affected by it - including diagnostic utility, CLI, configuration and docs. Otherwise, if we have a policy that we'll commit the core and then delay the other PRs, there is a chance that the other PRs are never completed.

garagatyi · 2017-03-01T08:38:33Z

...src/main/java/org/eclipse/che/plugin/docker/machine/LocalDockerServerEvaluationStrategy.java


-        return getExposedPortsToAddressPorts(externalAddress, containerInfo.getNetworkSettings().getPorts());
+    protected String getCheExternalAddress(ContainerInfo containerInfo, String internalHost) {


Looks like it is external address of container in WS, not Che itself. Previous name of variable with this value was more precise.

garagatyi · 2017-03-01T08:46:37Z

...ava/org/eclipse/che/plugin/docker/machine/LocalDockerSinglePortServerEvaluationStrategy.java

+ * Represents a server evaluation strategy for the configuration where the workspace server and workspace
+ * containers are running on the same Docker network and are exposed through the same single port.
+ *
+ * This server evaluation strategy will return a completed {@link ServerImpl} with internal addresses set


Can you elaborate on what completed {@link ServerImpl} means? I don't really understand, so probably I'm missing something.

garagatyi · 2017-03-01T08:48:10Z

...ava/org/eclipse/che/plugin/docker/machine/LocalDockerSinglePortServerEvaluationStrategy.java

+ * This server evaluation strategy will return a completed {@link ServerImpl} with internal addresses set
+ * as {@link LocalDockerServerEvaluationStrategy} does. Contrarily external addresses will have the following format:
+ *
+ *       serverName.workspaceID.cheExternalAddress (e.g. terminal.79rfwhqaztq2ru2k.che.local)


As I said in comment to the PR it's not clear how workspaces with multiple machines will work.

garagatyi · 2017-03-01T09:04:15Z

...er-machine/src/main/java/org/eclipse/che/plugin/docker/machine/ServerEvaluationStrategy.java

@@ -61,10 +61,13 @@
     * @param internalAddress
     *         address passed into {@code getServers}; used as fallback if address cannot be
     *         retrieved from containerInfo.
+     * @param serverConfMap
+     *         additional Map of {@link ServerConfImpl}.


It is unclear from javadocs why this additional map is needed. Please explain it in javadocs.

benoitf

While I agree more than 100% on the final goal, I find the implementation of the strategy too specific for a given product.
Let's say we use a different reverse proxy, we would need another pattern, etc

Could it be possible to name it instead of "single-port" something more generic ?

and instead of always using pattern
ref.workspace-id.che-address

to allow to pickup the pattern through a list of predefined properties
like in your case it could be
".."

but we could have
<che.protocol>://:/-

or any pattern built upon a set of properties

l0rd · 2017-03-08T08:43:47Z

@garagatyi @benoitf @TylerJewell thanks for your review. I've opened a second PR (#4351) to merge that work in the openshift-connector branch as it is to get it tested/run by our CI. In the meantime I will push more commits on this PR to address your comments and to get that merged to master too.

TylerJewell · 2017-03-08T13:26:55Z

I am going to invite Florent to help us explore how we can get it ready for a master merge. It is a challenging exercise but one we have in our objectives for the year.

benoitf · 2017-06-14T09:49:25Z

I think we can close this PR
(overlap with #5115 and #5366 + #5052 )

l0rd · 2017-06-14T13:09:59Z

We have merged that on openshift connector branch a long time ago.

Che server and workpaces exposed on the same single TCP port

f77b60a

Signed-off-by: Mario Loriedo <[email protected]>

l0rd requested review from garagatyi and amisevsk February 28, 2017 22:13

l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Feb 28, 2017

l0rd added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 28, 2017

TylerJewell requested review from TylerJewell and skabashnyuk March 1, 2017 07:47

garagatyi reviewed Mar 1, 2017

View reviewed changes

benoitf requested changes Mar 3, 2017

View reviewed changes

benoitf mentioned this pull request Mar 8, 2017

Che server and workpaces exposed on the same single TCP port #4351

Merged

slemeur mentioned this pull request Mar 8, 2017

Use a single port to route all traffic #4361

Closed

benoitf mentioned this pull request May 18, 2017

Enable single port exposure on Che #5115

Merged

l0rd closed this Jun 14, 2017

l0rd deleted the single-port branch June 14, 2017 13:31

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose Che server and workspaces through one single TCP port #4269

Expose Che server and workspaces through one single TCP port #4269

l0rd commented Feb 28, 2017

codenvy-ci commented Feb 28, 2017

garagatyi commented Mar 1, 2017

TylerJewell commented Mar 1, 2017

benoitf commented Mar 1, 2017

l0rd commented Mar 1, 2017

benoitf commented Mar 1, 2017

TylerJewell commented Mar 1, 2017

garagatyi Mar 1, 2017

garagatyi Mar 1, 2017

garagatyi Mar 1, 2017

garagatyi Mar 1, 2017

benoitf left a comment

l0rd commented Mar 8, 2017

TylerJewell commented Mar 8, 2017

benoitf commented Jun 14, 2017

l0rd commented Jun 14, 2017


		return getExposedPortsToAddressPorts(externalAddress, containerInfo.getNetworkSettings().getPorts());
		protected String getCheExternalAddress(ContainerInfo containerInfo, String internalHost) {

Expose Che server and workspaces through one single TCP port #4269

Expose Che server and workspaces through one single TCP port #4269

Conversation

l0rd commented Feb 28, 2017

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

codenvy-ci commented Feb 28, 2017

garagatyi commented Mar 1, 2017

TylerJewell commented Mar 1, 2017

benoitf commented Mar 1, 2017

l0rd commented Mar 1, 2017

benoitf commented Mar 1, 2017

TylerJewell commented Mar 1, 2017

garagatyi Mar 1, 2017

Choose a reason for hiding this comment

garagatyi Mar 1, 2017

Choose a reason for hiding this comment

garagatyi Mar 1, 2017

Choose a reason for hiding this comment

garagatyi Mar 1, 2017

Choose a reason for hiding this comment

benoitf left a comment

Choose a reason for hiding this comment

l0rd commented Mar 8, 2017

TylerJewell commented Mar 8, 2017

benoitf commented Jun 14, 2017

l0rd commented Jun 14, 2017