Restore support of single-port Che mode (on docker infra) #8471

mshaposhnik · 2018-01-26T10:29:03Z

What does this PR do?

Restores single-port execution mode using docker infra & Traefik as a proxy.

What issues does this PR fix or reference?

#7362

Release Notes

Restored support of single-port Che mode (on docker infra)

Docs PR

N/A

garagatyi · 2018-01-26T12:28:48Z

...ructure/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerInfraModule.java

@@ -67,5 +70,11 @@ protected void configure() {
    bind(
        org.eclipse.che.workspace.infrastructure.docker.monit.DockerAbandonedResourcesCleaner
            .class);
+
+    if (Boolean.parseBoolean(System.getenv("CHE_SINGLE_PORT"))) {


I would suggest not to use conditional binding. We are usually complaining such solutions. Instead of that you can use technique that was used in Che 5 https://github.com/eclipse/che/blob/5.22.x/dockerfiles/init/modules/che/templates/che.env.erb#L81

garagatyi · 2018-01-26T12:30:23Z

...ipse/che/workspace/infrastructure/docker/provisioner/labels/SinglePortLabelsProvisioner.java

+      Map<String, String> containerLabels = new HashMap<>();
+      for (Map.Entry<String, ServerConfig> serverEntry :
+          machineEntry.getValue().getServers().entrySet()) {
+        final String serverName = serverEntry.getKey().replace('/', '-');


I believe that server name can contain other symbols that are not allowed in the DNS entry. Can you check whether such a use case breaks this code?

garagatyi · 2018-01-26T12:31:53Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+   * @return wildcard domain
+   */
+  private String getWildcardNipDomain(String localAddress) {
+    return String.format("%s.%s", getExternalIp(localAddress), "nip.io");


Since nip.io is not always available it would be nice to allow to override this with another hostname, such as xip.io or user specific.

garagatyi · 2018-01-26T12:34:12Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+    if (machineName != null) {
+      joiner.add(machineName);
+    }
+    if (workspaceID != null) {


When any of arguments is null this method produces a hostname that differs from what is declared in the javadocs to this class. Are you sure that null arguments should be allowed?

garagatyi · 2018-01-26T12:35:58Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+  public String build(String serverName, String machineName, String workspaceID) {
+    StringJoiner joiner = new StringJoiner(".");
+    if (serverName != null) {
+      joiner.add("server-" + serverName.replace('/', '-'));


What if multiple dashes happen, is it valid host that works in single port case?

garagatyi · 2018-01-26T12:37:44Z

...va/org/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortUrlRewriter.java

+      throws InfrastructureException {
+    final String host = hostnameBuilder.build(serverName, machineName, identity.getWorkspaceId());
+    try {
+      URI uri = UriBuilder.fromUri(url).host(host).build();


I think that in this case port is also rewritten, but this code changes a hostname only.

garagatyi · 2018-01-26T12:41:32Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+  public String build(String serverName, String machineName, String workspaceID) {
+    StringJoiner joiner = new StringJoiner(".");
+    if (serverName != null) {
+      joiner.add("server-" + serverName.replace('/', '-'));


Can you elaborate on why this prefix server- is needed?

It's not necessary. Just to be more clear where the link points to.

skabashnyuk · 2018-01-30T07:23:42Z

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

@@ -255,6 +255,9 @@ che.infra.docker.bootstrapper.installer_timeout_sec=180
 # Once servers for one installer available - checks stopped.
 che.infra.docker.bootstrapper.server_check_period_sec=3

+# URL rewriter. May be overriden in case of single-port mode e.t.c
+che.infra.docker.url_rewriter=default
+


Make sure you are able to configure it with CLI

skabashnyuk · 2018-01-30T07:23:50Z

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

@@ -379,3 +382,7 @@ che.infra.openshift.pvc.access_mode=ReadWriteOnce
 # then OpenShift infrastructure will reconfigure installer to use first available from this range
 che.infra.openshift.installer_server_min_port=10000
 che.infra.openshift.installer_server_max_port=20000
+
+# Single port mode wildcard domain host & port. nip.io is used by default
+che.singleport.wildcard_domain.host=NULL


Make sure you are able to configure it with CLI

skabashnyuk · 2018-01-30T07:25:33Z

...va/org/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortUrlRewriter.java

+      @Nullable @Named("che.docker.ip.external") String externalIpOfContainers,
+      @Nullable @Named("che.singleport.wildcard_domain.host") String wildcardHost,
+      @Nullable @Named("che.singleport.wildcard_domain.port") String wildcardPort) {
+    this.hostnameBuilder =


Why it's not injected from container

To avoid one more conditional binding since this class is not used in normal mode.

garagatyi · 2018-01-31T07:26:16Z

dockerfiles/init/manifests/che.env

@@ -245,6 +245,12 @@
 # user configuration.
 CHE_SINGLE_PORT=false

+# Wildcard DNS host and port for the single port mode. Default is nip.io
+#CHE.SINGLEPORT.WILDCARD__DOMAIN.HOST=NULL


Replace dots with underscores

garagatyi · 2018-01-31T07:28:12Z

dockerfiles/init/manifests/che.env

+#CHE.SINGLEPORT.WILDCARD__DOMAIN.PORT=NULL
+
+# Url rewriter overrides default used in single port mode.
+#CHE.INFRA.DOCKER.URL_REWRITER=SINGLEPORT


Please add a note that this env var doesn't work when single port is enabled.

garagatyi · 2018-01-31T07:31:22Z

...cture/src/main/java/org/eclipse/che/workspace/infrastructure/docker/UrlRewriterProvider.java

+  }
+
+  @PostConstruct
+  private void checkRewriterIsPresent() throws Exception {


Can we do that right in the constructor? Then we would need neither post construct method nor rewriterValue.

garagatyi · 2018-01-31T07:46:40Z

...ipse/che/workspace/infrastructure/docker/provisioner/labels/SinglePortLabelsProvisioner.java

+        internalEnv.getMachines().entrySet()) {
+      final String machineName = machineEntry.getKey();
+      Map<String, String> containerLabels = new HashMap<>();
+      for (Map.Entry<String, ServerConfig> serverEntry :


We don't need traefik rules for internal servers

added check

garagatyi · 2018-01-31T07:48:42Z

...ipse/che/workspace/infrastructure/docker/provisioner/labels/SinglePortLabelsProvisioner.java

+        final String port = serverEntry.getValue().getPort().split("/")[0];
+
+        containerLabels.put(format("traefik.%s.port", serviceName), port);
+        containerLabels.put(format("traefik.%s.frontend.entryPoints", serviceName), "http");


Since single port mode effectively supports only HTTP protocol it may be useful to specify this in che.env and docs

garagatyi · 2018-01-31T08:57:11Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+   * @return wildcard domain
+   */
+  private String getWildcardDomain(String localAddress) {
+    return String.format(


Can we do this and getExternalIp a one-time operation in the constructor in a singleton class?

garagatyi · 2018-01-31T08:59:03Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+  }
+
+  private String normalize(String input) {
+    return pattern.matcher(input).replaceAll("-");


I think that with this logic it is possible that:

hyphen become the first char

hyphen become the last char
Those cases are forbidden in hostnames.

garagatyi · 2018-01-31T09:02:47Z

...ipse/che/workspace/infrastructure/docker/local/LocalCheDockerEnvironmentProvisionerTest.java

            settingsProvisioners,
            projectsVolumeProvisioner,
            installerConfigProvisioner,
            labelsProvisioner,
            dockerApiEnvProvisioner,
            wsAgentServerConfigProvisioner,
+            singlePortLabelsProvisioner,


Looks like single port labels provisioning is not covered in this test.

garagatyi · 2018-01-31T09:14:05Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+    if (serverName != null) {
+      joiner.add(normalize(serverName));
+    }
+    if (machineName != null) {


Can you describe in javadocs the case when some component is null?

garagatyi · 2018-01-31T09:15:04Z

...rg/eclipse/che/workspace/infrastructure/docker/server/mapping/SinglePortHostnameBuilder.java

+   */
+  private final Pattern pattern = Pattern.compile("[^a-zA-Z0-9\\-]");
+
+  public SinglePortHostnameBuilder(


Can you cover this class with unit tests?

mshaposhnik · 2018-01-31T10:41:39Z

ci-build

codenvy-ci · 2018-01-31T11:16:37Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4439/

hkolvenbach · 2018-02-22T15:20:18Z

i just tried to run che6 in single port on docker, but it seems some changes to https://github.com/eclipse/che/blob/master/dockerfiles/cli/version/latest/images#L4 didn't make it into master, 6.1.0 or 6.2.0. (it is still traefik 1.3.0-rc3 instead of 1.5.0)

skabashnyuk · 2018-02-22T16:59:18Z

why do you think so?

riuvshin · 2018-02-22T18:50:09Z

@hkolvenbach what is the CHE version you are trying with?

hkolvenbach · 2018-02-23T08:56:54Z

@skabashnyuk the dockerfiles for the 6.1.0 and 6.2.0 releases still contain IMAGE_TRAEFIK=traefik:v1.3.0-rc3 (https://github.com/eclipse/che/blob/master/dockerfiles/cli/version/6.2.0/images#L4)

@riuvshin we are currently using 6.1.0. I solved it by switching to nightly to test the CHE_SINGLE_PORT with multiuser.

I was just mentioning the mismatching version number of traefik because from the timing of the pull request i think it should have ended up in 6.1.0 and later versions, so maybe there was something going wrong during merging.

riuvshin · 2018-02-23T10:37:44Z

@hkolvenbach
that supposed to be in 6.1.0 but yeah, it has wrong version.
btw 6.2.0 is future tag latest is 6.1.0
here is PR to fix that #8890
as it will be merged I will update image and you will just need to repull 6.1.0 tag of CLI image to have this fix

hkolvenbach · 2018-02-23T11:00:39Z

thanks!

mshaposhnik added 12 commits January 25, 2018 11:24

Add url rewriting

8aaac3e

Merge branch 'master' into singleport

1e3db9d

Merge branch 'master' into singleport

dbea016

Add url rewriter

2349a21

remove traefik plugin

b3b1b57

Merge branch 'master' into singleport

c4281d9

Add test

2c3e505

Fix javadoc & traefik version

528cc61

Fixup code

84fa2eb

FIx machine link

1812d26

Merge branch 'master' into singleport

d98ae7e

Minor fixup

b5cfe9f

mshaposhnik requested review from benoitf, garagatyi, riuvshin and vparfonov as code owners January 26, 2018 10:29

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 26, 2018

garagatyi reviewed Jan 26, 2018

View reviewed changes

mshaposhnik added 3 commits January 29, 2018 12:28

refactor singleport binging and validations

78dac6b

Merge branch 'master' into singleport

0ec6d7a

Code fixups

d5de6d3

mshaposhnik requested a review from skabashnyuk as a code owner January 29, 2018 13:24

mshaposhnik added 2 commits January 29, 2018 15:32

Code fixups

7bd8b28

Code fixups

d8f0a9c

skabashnyuk reviewed Jan 30, 2018

View reviewed changes

Conf fixups

2072f62

riuvshin approved these changes Jan 30, 2018

View reviewed changes

skabashnyuk approved these changes Jan 30, 2018

View reviewed changes

benoitf approved these changes Jan 30, 2018

View reviewed changes

garagatyi reviewed Jan 31, 2018

View reviewed changes

mshaposhnik added 2 commits January 31, 2018 12:29

review fixups

51f8112

review fixups

3f874eb

garagatyi approved these changes Jan 31, 2018

View reviewed changes

mshaposhnik merged commit 0d57c48 into master Jan 31, 2018

mshaposhnik deleted the singleport branch January 31, 2018 12:27

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 Jan 31, 2018

benoitf added this to the 6.0.0-M5 milestone Jan 31, 2018

davidwindell mentioned this pull request Feb 1, 2018

CHE_SINGLE_PORT still not working #8554

Closed

This pull request was closed.

Restore support of single-port Che mode (on docker infra) #8471

Restore support of single-port Che mode (on docker infra) #8471

Conversation

mshaposhnik commented Jan 26, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

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

mshaposhnik Jan 30, 2018 • edited Loading

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

mshaposhnik commented Jan 31, 2018

codenvy-ci commented Jan 31, 2018

hkolvenbach commented Feb 22, 2018

skabashnyuk commented Feb 22, 2018

riuvshin commented Feb 22, 2018

hkolvenbach commented Feb 23, 2018

riuvshin commented Feb 23, 2018

hkolvenbach commented Feb 23, 2018

mshaposhnik Jan 30, 2018 •

edited

Loading