Skip to content
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

Add property machine.docker.local_node_host.external #2402

Merged
merged 1 commit into from
Sep 28, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ machine.docker.privilege_mode=false
# address and provide this host or IP address here.
machine.docker.local_node_host=NULL

# If the browser cannot access directly to the IP of the Docker host (e.g. when docker host is behind a NAT or
# with Docker for Mac) we can set this external address to let the browser access Docker containers.
# wsmaster will still use machine.docker.local_node_host to access Docker containers containers.
machine.docker.local_node_host.external=NULL

# Allows to use registry for machine snapshots, you should set this property to {true},
# otherwise workspace snapshots would be saved locally.
machine.docker.snapshot_use_registry=false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,13 @@ public interface Server {
String getRef();

/**
* Address of the server in form <b>host:port</b>
* External address of the server in form <b>hostname:port</b>.
* <p>
* This address is used by the browser to communicate with the server.
* <b>hostname</b> can be configured using property machine.docker.local_node_host.external
* or environment variable CHE_DOCKER_MACHINE_HOST_EXTERNAL.
* <b>port</b> is the external port and cannot be configured.
* If not explicitly configured that address is set using {@link ServerProperties#getInternalAddress()}
*/
String getAddress();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this method returns both host and port of the server. Are you going to change the behavior in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I'm not:

"address": "localhost:32929"

Is that ok?


Expand All @@ -36,14 +42,15 @@ public interface Server {
String getProtocol();

/**
* Path to access the server.
* Url of the server, e.g.&nbsp;http://localhost:8080
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean "regular space". If I add a regular space and I try to generate the Javadoc I have the following result:

image

If I use &nbsp; instead the sentence is not broken:

image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, interesting

*/
@Nullable
String getPath();
String getUrl();


/**
* Url of the server, e.g. http://localhost:8080
* Non mandatory properties of the server.
*/
@Nullable
String getUrl();
ServerProperties getProperties();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*******************************************************************************
* Copyright (c) 2012-2016 Codenvy, S.A.
* 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:
* Codenvy, S.A. - initial API and implementation
*******************************************************************************/
package org.eclipse.che.api.core.model.machine;

import org.eclipse.che.commons.annotation.Nullable;

/**
* Not mandatory properties of a {@link Server}
*
* @author Mario Loriedo
*/
public interface ServerProperties {

/**
* Path to access the server.
*/
@Nullable
String getPath();

/**
* Internal address of the server in form <b>host:port</b>.
* <p>
* Used by wsmaster to communicate with the server
*/
@Nullable
String getInternalAddress();
Copy link

@garagatyi garagatyi Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all fields in that object are nullable.



/**
* Internal Url of the server, e.g.&nbsp;http://localhost:8080.
* <p>
* Used by wsmaster to comunicate with the server
*/
@Nullable
String getInternalUrl();
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,32 @@
package org.eclipse.che.ide.api.machine;

import org.eclipse.che.api.core.model.machine.Server;
import org.eclipse.che.api.core.model.machine.ServerProperties;
import org.eclipse.che.api.machine.shared.dto.ServerDto;

import java.util.Objects;

/**
* Describe development machine server instance.
* {@link Server}
*
* @link Server
* @author Vitalii Parfonov
*/
public class DevMachineServer implements Server {


private final String path;
private final String address;
private final String protocol;
private final String ref;
private final String url;
private final String address;
private final String protocol;
private final String ref;
private final String url;
private final ServerProperties properties;

public DevMachineServer(Server dto) {
path = dto.getPath();
address = dto.getAddress();
protocol = dto.getProtocol();
ref = dto.getRef();
url = dto.getUrl();
properties = dto.getProperties();
}


Expand All @@ -53,12 +56,45 @@ public String getProtocol() {
}

@Override
public String getPath() {
return path;
public String getUrl() {
return url;
}

@Override
public String getUrl() {
return url;
public ServerProperties getProperties() { return properties; };

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof DevMachineServer)) return false;
final DevMachineServer other = (DevMachineServer) o;
return Objects.equals(ref, other.ref) &&
Objects.equals(protocol, other.protocol) &&
Objects.equals(address, other.address) &&
Objects.equals(url, other.url) &&
Objects.equals(properties, other.properties);
}
}

@Override
public int hashCode() {
int hash = 7;
hash = hash * 31 + Objects.hashCode(ref);
hash = hash * 31 + Objects.hashCode(protocol);
hash = hash * 31 + Objects.hashCode(address);
hash = hash * 31 + Objects.hashCode(url);
hash = hash * 31 + Objects.hashCode(properties);
return hash;
}

@Override
public String toString() {
return "DevMachineServer{" +
"ref='" + ref + '\'' +
", protocol='" + protocol + '\'' +
", address='" + address + '\'' +
", url='" + url + '\'' +
", properties='" + properties + '\'' +
'}';
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*******************************************************************************
* Copyright (c) 2012-2016 Codenvy, S.A.
* 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:
* Codenvy, S.A. - initial API and implementation
*******************************************************************************/
package org.eclipse.che.ide.api.machine;

import org.eclipse.che.api.core.model.machine.ServerProperties;

import java.util.Objects;

/**
* Describe development machine server instance.
Copy link

@garagatyi garagatyi Sep 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line break by entering 'enter' key doesn't work in javadocs. Please add <p/> or <br/>

*
* @link ServerProperties
* @author Mario Loriedo
*/
public class DevMachineServerProperties implements ServerProperties {

private final String path;
private final String internalAddress;
private final String internalUrl;

public DevMachineServerProperties(ServerProperties properties) {
path = properties.getPath();
internalAddress = properties.getInternalAddress();
internalUrl = properties.getInternalUrl();
}

@Override
public String getInternalAddress() {
return internalAddress;
}

@Override
public String getPath() {
return path;
}

@Override
public String getInternalUrl() {
return internalUrl;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof DevMachineServerProperties)) return false;
final DevMachineServerProperties other = (DevMachineServerProperties)o;

return Objects.equals(path, other.path) &&
Objects.equals(internalAddress, other.internalAddress) &&
Objects.equals(internalUrl, other.internalUrl);
}

@Override
public int hashCode() {
int hash = 7;
hash = hash * 31 + Objects.hashCode(path);
hash = hash * 31 + Objects.hashCode(internalAddress);
hash = hash * 31 + Objects.hashCode(internalUrl);
return hash;
}

@Override
public String toString() {
return "DevMachineServerProperties{" +
"path='" + path + '\'' +
", internalAddress='" + internalAddress + '\'' +
", internalUrl='" + internalUrl + '\'' +
'}';
}

}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add equals/hashcode/toString methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public MachineRuntimeInfoImpl getRuntime() {
try {
final ContainerInfo containerInfo = docker.inspectContainer(container);
machineRuntime = new MachineRuntimeInfoImpl(dockerMachineFactory.createMetadata(containerInfo,
null,
Copy link

@garagatyi garagatyi Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it looks like this argument is always null. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to add more details to your questions if that was not what you meant. I think you are wondering how Server.url is ever set if this parameter is always null. It should be set using property machine.docker.local_node_host.external but it looks as it's always null.

Property machine.docker.local_node_host.external is injected in LocalDockerModule objects that are binded to DockerInstanceRuntimeInfo in LocalDockerModule.

Therefore method DockerInstance.getRuntime() is not used by the API to get the machine runtime. That explains why Server.url is correctly returned by the API.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me explain how I understand what is happening with these changes. Please correct me if I'm mistaken.
DockerInstance always pass null as external host. DockerInstanceRuntimeInfo doesn't change that. LocalDockerInstanceRuntimeInfo uses assisted injection, so this null is passed here too.
So I don't see how another than null value can be accepted by DockerInstanceRuntimeInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed on Slack I've cleaned-up LocalDockerInstanceRuntimeInfo class and made it easier to read.

node.getHost(),
getConfig()));
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import org.eclipse.che.api.core.model.machine.ServerConf;
import org.eclipse.che.api.machine.server.model.impl.ServerConfImpl;
import org.eclipse.che.api.machine.server.model.impl.ServerImpl;
import org.eclipse.che.api.machine.server.model.impl.ServerPropertiesImpl;
import org.eclipse.che.commons.annotation.Nullable;
import org.eclipse.che.plugin.docker.client.json.ContainerConfig;
import org.eclipse.che.plugin.docker.client.json.ContainerInfo;
import org.eclipse.che.plugin.docker.client.json.ContainerState;
Expand Down Expand Up @@ -83,17 +85,21 @@ public class DockerInstanceRuntimeInfo implements MachineRuntimeInfo {
protected static final String SERVER_CONF_LABEL_PATH_SUFFIX = ":path";

private final ContainerInfo info;
private final String containerHost;
private final String containerExternalHostname;
private final String containerInternalHostname;
private final Map<String, ServerConfImpl> serversConf;

@Inject
public DockerInstanceRuntimeInfo(@Assisted ContainerInfo containerInfo,
@Assisted String containerHost,
@Assisted("externalhost") @Nullable String containerExternalHostname,
@Assisted("internalhost") String containerInternalHostname,
@Assisted MachineConfig machineConfig,
@Named("machine.docker.dev_machine.machine_servers") Set<ServerConf> devMachineSystemServers,
@Named("machine.docker.machine_servers") Set<ServerConf> allMachinesSystemServers) {
this.info = containerInfo;
this.containerHost = containerHost;
this.containerExternalHostname = containerExternalHostname == null ?
containerInternalHostname : containerExternalHostname;
this.containerInternalHostname = containerInternalHostname;
Stream<ServerConf> confStream = Stream.concat(machineConfig.getServers().stream(), allMachinesSystemServers.stream());
if (machineConfig.isDev()) {
confStream = Stream.concat(confStream, devMachineSystemServers.stream());
Expand Down Expand Up @@ -244,7 +250,8 @@ public Map<String, ServerImpl> getServers() {
}
return addDefaultReferenceForServersWithoutReference(
addRefAndUrlToServers(
getServersWithFilledPorts(containerHost,
getServersWithFilledPorts(containerExternalHostname,
containerInternalHostname,
ports),
labels));
}
Expand All @@ -264,33 +271,44 @@ private Map<String, ServerImpl> addDefaultReferenceForServersWithoutReference(Ma
protected Map<String, ServerImpl> addRefAndUrlToServers(final Map<String, ServerImpl> servers, final Map<String, String> labels) {
final Map<String, ServerConfImpl> serversConfFromLabels = getServersConfFromLabels(servers.keySet(), labels);
for (Map.Entry<String, ServerImpl> serverEntry : servers.entrySet()) {
ServerPropertiesImpl serverProperties = new ServerPropertiesImpl(serverEntry.getValue().getProperties());
ServerConf serverConf = serversConf.getOrDefault(serverEntry.getKey(), serversConfFromLabels.get(serverEntry.getKey()));
if (serverConf != null) {
if (serverConf.getRef() != null) {
serverEntry.getValue().setRef(serverConf.getRef());
}
if (serverConf.getPath() != null) {
serverEntry.getValue().setPath(serverConf.getPath());
serverProperties.setPath(serverConf.getPath());
}
if (serverConf.getProtocol() != null) {
serverEntry.getValue().setProtocol(serverConf.getProtocol());

String url = serverConf.getProtocol() + "://" + serverEntry.getValue().getAddress();
String externalUrl = serverConf.getProtocol() + "://" + serverEntry.getValue().getAddress();
if (!isNullOrEmpty(serverConf.getPath())) {
if (serverConf.getPath().charAt(0) != '/') {
url = url + '/';
externalUrl = externalUrl + '/';
}
url = url + serverConf.getPath();
externalUrl = externalUrl + serverConf.getPath();
}
serverEntry.getValue().setUrl(url);
serverEntry.getValue().setUrl(externalUrl);

String internalUrl = serverConf.getProtocol() + "://" + serverProperties.getInternalAddress();
if (serverConf.getPath() != null) {
if (serverConf.getPath().charAt(0) != '/') {
internalUrl = internalUrl + '/';
}
internalUrl = internalUrl + serverConf.getPath();
}
serverProperties.setInternalUrl(internalUrl);
}
serverEntry.getValue().setProperties(serverProperties);
}
}

return servers;
}

protected Map<String, ServerImpl> getServersWithFilledPorts(final String host, final Map<String, List<PortBinding>> exposedPorts) {
protected Map<String, ServerImpl> getServersWithFilledPorts(final String externalHostame, final String internalHostname, final Map<String, List<PortBinding>> exposedPorts) {
final HashMap<String, ServerImpl> servers = new LinkedHashMap<>();

for (Map.Entry<String, List<PortBinding>> portEntry : exposedPorts.entrySet()) {
Expand All @@ -300,9 +318,9 @@ protected Map<String, ServerImpl> getServersWithFilledPorts(final String host, f
String externalPort = portEntry.getValue().get(0).getHostPort();
servers.put(portProtocol, new ServerImpl(null,
null,
host + ":" + externalPort,
externalHostame + ":" + externalPort,
null,
null));
new ServerPropertiesImpl(null, internalHostname + ":" + externalPort, null)));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure that null value won't be treated as string "null"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After fixup externalHostname, internalHostname and externalPorts can never be null.

}

return servers;
Expand Down
Loading