-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Restore support of single-port Che mode (on docker infra) #8471
Changes from 12 commits
8aaac3e
1e3db9d
dbea016
2349a21
b3b1b57
c4281d9
2c3e505
528cc61
84fa2eb
1812d26
d98ae7e
b5cfe9f
78dac6b
0ec6d7a
d5de6d3
7bd8b28
d8f0a9c
2072f62
51f8112
3f874eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
IMAGE_INIT=${BUILD_ORGANIZATION}/${BUILD_PREFIX}-init:${BUILD_TAG} | ||
IMAGE_CHE=${BUILD_ORGANIZATION}/${BUILD_PREFIX}-server:${BUILD_TAG} | ||
IMAGE_COMPOSE=docker/compose:1.10.1 | ||
IMAGE_TRAEFIK=traefik:v1.3.0-rc3 | ||
IMAGE_TRAEFIK=traefik:v1.5.0 | ||
IMAGE_POSTGRES=centos/postgresql-96-centos7 | ||
IMAGE_KEYCLOACK=jboss/keycloak-openshift:3.3.0.CR2-3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
IMAGE_INIT=eclipse/che-init:latest | ||
IMAGE_CHE=eclipse/che-server:latest | ||
IMAGE_COMPOSE=docker/compose:1.10.1 | ||
IMAGE_TRAEFIK=traefik:v1.3.0-rc3 | ||
IMAGE_TRAEFIK=traefik:v1.5.0 | ||
IMAGE_POSTGRES=centos/postgresql-96-centos7:9.6 | ||
IMAGE_KEYCLOACK=jboss/keycloak-openshift:3.3.0.CR2-3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
IMAGE_INIT=eclipse/che-init:nightly | ||
IMAGE_CHE=eclipse/che-server:nightly | ||
IMAGE_COMPOSE=docker/compose:1.10.1 | ||
IMAGE_TRAEFIK=traefik:v1.3.0-rc3 | ||
IMAGE_TRAEFIK=traefik:v1.5.0 | ||
IMAGE_POSTGRES=centos/postgresql-96-centos7:9.6 | ||
IMAGE_KEYCLOACK=jboss/keycloak-openshift:3.3.0.CR2-3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
/* | ||
* Copyright (c) 2012-2018 Red Hat, Inc. | ||
* All rights reserved. This program and the accompanying materials | ||
* are made available under the terms of the Eclipse Public License v1.0 | ||
* which accompanies this distribution, and is available at | ||
* http://www.eclipse.org/legal/epl-v10.html | ||
* | ||
* Contributors: | ||
* Red Hat, Inc. - initial API and implementation | ||
*/ | ||
package org.eclipse.che.workspace.infrastructure.docker.provisioner.labels; | ||
|
||
import static java.lang.String.format; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
import javax.inject.Inject; | ||
import javax.inject.Named; | ||
import org.eclipse.che.api.core.model.workspace.config.ServerConfig; | ||
import org.eclipse.che.api.core.model.workspace.runtime.RuntimeIdentity; | ||
import org.eclipse.che.api.workspace.server.spi.InfrastructureException; | ||
import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig; | ||
import org.eclipse.che.commons.annotation.Nullable; | ||
import org.eclipse.che.workspace.infrastructure.docker.model.DockerContainerConfig; | ||
import org.eclipse.che.workspace.infrastructure.docker.model.DockerEnvironment; | ||
import org.eclipse.che.workspace.infrastructure.docker.provisioner.ConfigurationProvisioner; | ||
import org.eclipse.che.workspace.infrastructure.docker.server.mapping.SinglePortHostnameBuilder; | ||
|
||
/** | ||
* Sets necessary container labels for the single-port proxy (Traefik). | ||
* | ||
* @author Max Shaposhnik (mshaposh@redhat.com) | ||
*/ | ||
public class SinglePortLabelsProvisioner implements ConfigurationProvisioner { | ||
|
||
private final SinglePortHostnameBuilder hostnameBuilder; | ||
|
||
@Inject | ||
public SinglePortLabelsProvisioner( | ||
@Nullable @Named("che.docker.ip.external") String externalIpOfContainers, | ||
@Named("che.docker.ip") String internalIpOfContainers) { | ||
this.hostnameBuilder = | ||
new SinglePortHostnameBuilder(externalIpOfContainers, internalIpOfContainers); | ||
} | ||
|
||
@Override | ||
public void provision(DockerEnvironment internalEnv, RuntimeIdentity identity) | ||
throws InfrastructureException { | ||
for (Map.Entry<String, InternalMachineConfig> machineEntry : | ||
internalEnv.getMachines().entrySet()) { | ||
final String machineName = machineEntry.getKey(); | ||
Map<String, String> containerLabels = new HashMap<>(); | ||
for (Map.Entry<String, ServerConfig> serverEntry : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need traefik rules for internal servers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added check |
||
machineEntry.getValue().getServers().entrySet()) { | ||
final String serverName = serverEntry.getKey().replace('/', '-'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
final String host = | ||
"Host:" + hostnameBuilder.build(serverName, machineName, identity.getWorkspaceId()); | ||
final String serviceName = machineName + "-" + serverName; | ||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since single port mode effectively supports only HTTP protocol it may be useful to specify this in che.env and docs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added note |
||
containerLabels.put(format("traefik.%s.frontend.rule", serviceName), host); | ||
// Needed to activate per-service rules | ||
containerLabels.put("traefik.frontend.rule", machineName); | ||
} | ||
DockerContainerConfig dockerConfig = internalEnv.getContainers().get(machineName); | ||
dockerConfig.getLabels().putAll(containerLabels); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/* | ||
* Copyright (c) 2012-2018 Red Hat, Inc. | ||
* All rights reserved. This program and the accompanying materials | ||
* are made available under the terms of the Eclipse Public License v1.0 | ||
* which accompanies this distribution, and is available at | ||
* http://www.eclipse.org/legal/epl-v10.html | ||
* | ||
* Contributors: | ||
* Red Hat, Inc. - initial API and implementation | ||
*/ | ||
package org.eclipse.che.workspace.infrastructure.docker.server.mapping; | ||
|
||
import java.net.InetAddress; | ||
import java.net.UnknownHostException; | ||
import java.util.StringJoiner; | ||
|
||
/** | ||
* Produces host names in form: | ||
* server-<serverName>.<machineName>.<workspaceId>.<external_or_internal_address>.nip.io | ||
* | ||
* @author Max Shaposhnik (mshaposh@redhat.com) | ||
*/ | ||
public class SinglePortHostnameBuilder { | ||
|
||
private final String externalAddress; | ||
private final String internalAddress; | ||
|
||
public SinglePortHostnameBuilder(String externalAddress, String internalAddress) { | ||
this.externalAddress = externalAddress; | ||
this.internalAddress = internalAddress; | ||
} | ||
|
||
/** | ||
* Constructs hostname from given params. | ||
* | ||
* @param serverName optional server name | ||
* @param machineName optional machine name | ||
* @param workspaceID optional workspace ID | ||
* @return composite hostname | ||
*/ | ||
public String build(String serverName, String machineName, String workspaceID) { | ||
StringJoiner joiner = new StringJoiner("."); | ||
if (serverName != null) { | ||
joiner.add("server-" + serverName.replace('/', '-')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if multiple dashes happen, is it valid host that works in single port case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on why this prefix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not necessary. Just to be more clear where the link points to. |
||
} | ||
if (machineName != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you describe in javadocs the case when some component is null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
joiner.add(machineName); | ||
} | ||
if (workspaceID != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
joiner.add(workspaceID); | ||
} | ||
joiner.add( | ||
externalAddress != null | ||
? getWildcardNipDomain(externalAddress) | ||
: getWildcardNipDomain(internalAddress)); | ||
return joiner.toString(); | ||
} | ||
|
||
/** | ||
* Gets a Wildcard domain based on the ip using an external provider nip.io | ||
* | ||
* @return wildcard domain | ||
*/ | ||
private String getWildcardNipDomain(String localAddress) { | ||
return String.format("%s.%s", getExternalIp(localAddress), "nip.io"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
private String getExternalIp(String localAddress) { | ||
try { | ||
return InetAddress.getByName(localAddress).getHostAddress(); | ||
} catch (UnknownHostException e) { | ||
throw new UnsupportedOperationException( | ||
"Unable to find the IP for the address '" + localAddress + "'", e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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