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.

Sign up for GitHub

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

Jump to bottom

Improve ports parsing #418

Closed
wants to merge 5 commits into from
Closed

Improve ports parsing #418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50f61cf
Better ports
Dec 9, 2019
80fe0e4
Cleanup
Dec 9, 2019
9eefdce
Minor tweaks
Dec 9, 2019
95e19e6
Spelling
Dec 9, 2019
5c0a8da
Callum's feedback: Integration test demonstrating port parsing behavior
Dec 10, 2019
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
107 changes: 107 additions & 0 deletions ...ionTest/java/com/palantir/docker/compose/execution/DockerComposePortsIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.docker.compose.execution;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.palantir.docker.compose.DockerComposeManager;
import com.palantir.docker.compose.configuration.ShutdownStrategy;
import com.palantir.docker.compose.connection.DockerPort;
import com.palantir.docker.compose.connection.Ports;
import java.io.IOException;
import java.util.Set;
import java.util.stream.Collectors;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;

public class DockerComposePortsIntegrationTest {

private static final String LOCALHOST_IP = "127.0.0.1";

private static DockerComposeManager dockerComposeManager = new DockerComposeManager.Builder()
.shutdownStrategy(ShutdownStrategy.KILL_DOWN)
.file("src/test/resources/ports-docker-compose.yaml")
.build();

@BeforeClass
public static void beforeClass() throws IOException, InterruptedException {
dockerComposeManager.before();
}

@Test
public void no_ports_mapped() throws IOException, InterruptedException {
Ports ports = dockerComposeManager.dockerCompose().ports("no-ports-mapped");
Ports expectedPorts = new Ports(ImmutableList.of());
assertThat(ports, is(expectedPorts));
}

@Test
public void ports_mapped_identically() throws IOException, InterruptedException {
Ports ports = dockerComposeManager.dockerCompose().ports("ports-mapped-identically");
Ports expectedPorts = new Ports(ImmutableList.of(
new DockerPort(LOCALHOST_IP, 5432, 5432)));
assertThat(ports, is(expectedPorts));
}

@Test
public void ports_mapped_differently() throws IOException, InterruptedException {
Ports ports = dockerComposeManager.dockerCompose().ports("ports-mapped-differently");
Ports expectedPorts = new Ports(ImmutableList.of(
new DockerPort(LOCALHOST_IP, 1234, 5678)));
assertThat(ports, is(expectedPorts));
}

@Test
public void ports_mapped_with_different_ip() throws IOException, InterruptedException {
Ports ports = dockerComposeManager.dockerCompose().ports("ports-mapped-with-different-ip");
Ports expectedPorts = new Ports(ImmutableList.of(
new DockerPort(LOCALHOST_IP, 8000, 8000)));
assertThat(ports, is(expectedPorts));
}

@Test
public void ports_exposed_but_not_mapped() throws IOException, InterruptedException {
Ports ports = dockerComposeManager.dockerCompose().ports("ports-exposed-but-not-mapped");
Ports expectedPorts = new Ports(ImmutableList.of());
assertThat(ports, is(expectedPorts));
}

@Test
public void lots_of_port_information() throws IOException, InterruptedException {
Ports ports = dockerComposeManager.dockerCompose().ports("lots-of-port-information");
Set<DockerPort> expectedPortsSet = ImmutableSet.of(
new DockerPort(LOCALHOST_IP, 9000, 9000),
new DockerPort(LOCALHOST_IP, 9010, 9010),
new DockerPort(LOCALHOST_IP, 9020, 9020),
new DockerPort(LOCALHOST_IP, 6666, 7777),
new DockerPort(LOCALHOST_IP, 7777, 8888),
new DockerPort(LOCALHOST_IP, 8888, 9999),
new DockerPort(LOCALHOST_IP, 4000, 4000),
new DockerPort(LOCALHOST_IP, 4010, 4010),
new DockerPort(LOCALHOST_IP, 4020, 4020));
assertThat(ports.stream().collect(Collectors.toSet()), is(expectedPortsSet));
}

@AfterClass
public static void afterClass() {
dockerComposeManager.after();
}
}
2 changes: 1 addition & 1 deletion docker-compose-rule-core/src/main/java/com/palantir/docker/compose/DockerComposeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public ShutdownStrategy shutdownStrategy() {
@Value.Default
public com.palantir.docker.compose.execution.DockerCompose dockerCompose() {
com.palantir.docker.compose.execution.DockerCompose
dockerCompose = new DefaultDockerCompose(dockerComposeExecutable(), machine());
dockerCompose = new DefaultDockerCompose(dockerComposeExecutable(), dockerExecutable(), machine());
return new RetryingDockerCompose(retryAttempts(), dockerCompose);
}

Expand Down
20 changes: 11 additions & 9 deletions docker-compose-rule-core/src/main/java/com/palantir/docker/compose/connection/Ports.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
*/
package com.palantir.docker.compose.connection;

import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand All @@ -27,10 +25,15 @@

public class Ports {

private static final Pattern PORT_PATTERN = Pattern.compile("((\\d+).(\\d+).(\\d+).(\\d+)):(\\d+)->(\\d+)/tcp");
private static final int IP_ADDRESS = 1;
private static final int EXTERNAL_PORT = 6;
private static final int INTERNAL_PORT = 7;
// Note: This regex doesn't account for usage of the
// udp protocol or port ranges (ex: 9090-9091:8080-8081)
// Without escape characters: (?<ipAddress>(\d+).(\d+).(\d+).(\d+)):(?<externalPort>\d+)->(?<internalPort>\d+)\/tcp
private static final Pattern PORT_PATTERN = Pattern.compile(
"(?<ipAddress>(\\d+).(\\d+).(\\d+).(\\d+)):(?<externalPort>\\d+)->(?<internalPort>\\d+)\\/tcp");

private static final String IP_ADDRESS = "ipAddress";
private static final String EXTERNAL_PORT = "externalPort";
private static final String INTERNAL_PORT = "internalPort";

private static final String NO_IP_ADDRESS = "0.0.0.0";

Expand All @@ -48,9 +51,8 @@ public Stream<DockerPort> stream() {
return ports.stream();
}

public static Ports parseFromDockerComposePs(String psOutput, String dockerMachineIp) {
Preconditions.checkArgument(!Strings.isNullOrEmpty(psOutput), "No container found");
Matcher matcher = PORT_PATTERN.matcher(psOutput);
public static Ports parseFromPortInformation(String portInformation, String dockerMachineIp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately nearly everything in DCR is public rather than package-private - so we should do some quick github searches to find out if anyone is using this internally or externally before making such a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fortunately in this case it does look safe, but worth bearing in mind!

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah, missed that one, sorry. Can include explicit comments about usage / impact of the break in the future

Matcher matcher = PORT_PATTERN.matcher(portInformation);
List<DockerPort> ports = new ArrayList<>();
while (matcher.find()) {
String matchedIpAddress = matcher.group(IP_ADDRESS);
Expand Down
90 changes: 60 additions & 30 deletions ...e-rule-core/src/main/java/com/palantir/docker/compose/execution/DefaultDockerCompose.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
import static org.joda.time.Duration.standardMinutes;

import com.github.zafarkhaja.semver.Version;
import com.google.common.base.Joiner;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.palantir.docker.compose.configuration.DockerComposeFiles;
import com.palantir.docker.compose.configuration.ProjectName;
import com.palantir.docker.compose.connection.Container;
Expand All @@ -45,102 +47,111 @@ public class DefaultDockerCompose implements DockerCompose {
private static final Duration LOG_TIMEOUT = standardMinutes(1);
private static final Logger log = LoggerFactory.getLogger(DefaultDockerCompose.class);

private final Command command;
private final Command dockerComposeCommand;
private final DockerComposeExecutable dockerComposeRawExecutable;
private final Command dockerCommand;
private final DockerMachine dockerMachine;
private final DockerComposeExecutable rawExecutable;


public DefaultDockerCompose(DockerComposeFiles dockerComposeFiles, DockerMachine dockerMachine, ProjectName projectName) {
this(DockerComposeExecutable.builder()
.dockerComposeFiles(dockerComposeFiles)
.dockerConfiguration(dockerMachine)
.projectName(projectName)
.build(), dockerMachine);
.build(),
DockerExecutable.builder()
.dockerConfiguration(dockerMachine)
.build(),
dockerMachine);
}

public DefaultDockerCompose(DockerComposeExecutable rawExecutable, DockerMachine dockerMachine) {
this.rawExecutable = rawExecutable;
this.command = new Command(rawExecutable, log::trace);
public DefaultDockerCompose(
DockerComposeExecutable dockerComposeRawExecutable,
DockerExecutable dockerRawExecutable,
Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems safe from a brief github search (I think 1 person has used it externally and none internally)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it seems reasonable enough, will add comments about break-impact next time!

DockerMachine dockerMachine) {
this.dockerComposeRawExecutable = dockerComposeRawExecutable;
this.dockerComposeCommand = new Command(dockerComposeRawExecutable, log::trace);
this.dockerCommand = new Command(dockerRawExecutable, log::trace);
this.dockerMachine = dockerMachine;
}

@Override
public void pull() throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "pull");
dockerComposeCommand.execute(Command.throwingOnError(), "pull");
}

@Override
public void build() throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "build");
dockerComposeCommand.execute(Command.throwingOnError(), "build");
}

@Override
public void up() throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "up", "-d");
dockerComposeCommand.execute(Command.throwingOnError(), "up", "-d");
}

@Override
public void down() throws IOException, InterruptedException {
command.execute(swallowingDownCommandDoesNotExist(), "down", "--volumes");
dockerComposeCommand.execute(swallowingDownCommandDoesNotExist(), "down", "--volumes");
}

@Override
public void stop() throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "stop");
dockerComposeCommand.execute(Command.throwingOnError(), "stop");

}

@Override
public void kill() throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "kill");
dockerComposeCommand.execute(Command.throwingOnError(), "kill");
}

@Override
public void rm() throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "rm", "--force", "-v");
dockerComposeCommand.execute(Command.throwingOnError(), "rm", "--force", "-v");
}

@Override
public void up(Container container) throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "up", "-d", container.getContainerName());
dockerComposeCommand.execute(Command.throwingOnError(), "up", "-d", container.getContainerName());
}

@Override
public void start(Container container) throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "start", container.getContainerName());
dockerComposeCommand.execute(Command.throwingOnError(), "start", container.getContainerName());
}

@Override
public void stop(Container container) throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "stop", container.getContainerName());
dockerComposeCommand.execute(Command.throwingOnError(), "stop", container.getContainerName());
}

@Override
public void kill(Container container) throws IOException, InterruptedException {
command.execute(Command.throwingOnError(), "kill", container.getContainerName());
dockerComposeCommand.execute(Command.throwingOnError(), "kill", container.getContainerName());
}

@Override
public String exec(DockerComposeExecOption dockerComposeExecOption, String containerName,
DockerComposeExecArgument dockerComposeExecArgument) throws IOException, InterruptedException {
verifyDockerComposeVersionAtLeast(VERSION_1_7_0, "You need at least docker-compose 1.7 to run docker-compose exec");
String[] fullArgs = constructFullDockerComposeExecArguments(dockerComposeExecOption, containerName, dockerComposeExecArgument);
return command.execute(Command.throwingOnError(), fullArgs);
return dockerComposeCommand.execute(Command.throwingOnError(), fullArgs);
}

@Override
public String run(DockerComposeRunOption dockerComposeRunOption, String containerName,
DockerComposeRunArgument dockerComposeRunArgument) throws IOException, InterruptedException {
String[] fullArgs = constructFullDockerComposeRunArguments(dockerComposeRunOption, containerName, dockerComposeRunArgument);
return command.execute(Command.throwingOnError(), fullArgs);
return dockerComposeCommand.execute(Command.throwingOnError(), fullArgs);
}

private void verifyDockerComposeVersionAtLeast(Version targetVersion, String message) throws IOException, InterruptedException {
validState(version().greaterThanOrEqualTo(targetVersion), message);
}

private Version version() throws IOException, InterruptedException {
String versionOutput = command.execute(Command.throwingOnError(), "-v");
String versionOutput = dockerComposeCommand.execute(Command.throwingOnError(), "-v");
return DockerComposeVersion.parseFromDockerComposeVersion(versionOutput);
}

Expand Down Expand Up @@ -170,7 +181,7 @@ private static String[] constructFullDockerComposeRunArguments(DockerComposeRunO

@Override
public List<ContainerName> ps() throws IOException, InterruptedException {
String psOutput = command.execute(Command.throwingOnError(), "ps");
String psOutput = dockerComposeCommand.execute(Command.throwingOnError(), "ps");
return ContainerNames.parseFromDockerComposePs(psOutput);
}

Expand All @@ -181,12 +192,12 @@ public Optional<String> id(Container container) throws IOException, InterruptedE

@Override
public String config() throws IOException, InterruptedException {
return command.execute(Command.throwingOnError(), "config");
return dockerComposeCommand.execute(Command.throwingOnError(), "config");
}

@Override
public List<String> services() throws IOException, InterruptedException {
String servicesOutput = command.execute(Command.throwingOnError(), "config", "--services");
String servicesOutput = dockerComposeCommand.execute(Command.throwingOnError(), "config", "--services");
return Arrays.asList(servicesOutput.split("(\r|\n)+"));
}

Expand All @@ -210,7 +221,7 @@ public boolean writeLogs(String container, OutputStream output) throws IOExcepti
}

private Optional<String> id(String containerName) throws IOException, InterruptedException {
String id = command.execute(Command.throwingOnError(), "ps", "-q", containerName);
String id = dockerComposeCommand.execute(Command.throwingOnError(), "ps", "-q", containerName);
if (id.isEmpty()) {
return Optional.empty();
}
Expand All @@ -220,12 +231,20 @@ private Optional<String> id(String containerName) throws IOException, Interrupte
private Process logs(String container) throws IOException, InterruptedException {
verifyDockerComposeVersionAtLeast(VERSION_1_7_0,
"You need at least docker-compose 1.7 to run docker-compose logs");
return rawExecutable.execute("logs", "--no-color", container);
return dockerComposeRawExecutable.execute("logs", "--no-color", container);
}

@Override
public Ports ports(String service) throws IOException, InterruptedException {
return Ports.parseFromDockerComposePs(psOutput(service), dockerMachine.getIp());
validState(!Strings.isNullOrEmpty(service), String.format("Service cannot be empty"));

String containerId = dockerComposeCommand.execute(swallowingServiceDoesNotExist(), "ps", "-q", service);
validState(!Strings.isNullOrEmpty(containerId), String.format("No container ID found for service with name '%s'.", service));

String portInformation = dockerCommand.execute(Command.throwingOnError(),
"ps", "--no-trunc", "--format", "{{.Ports}}", "--filter", String.format("id=%s", containerId));

return Ports.parseFromPortInformation(portInformation, dockerMachine.getIp());
}

private static ErrorHandler swallowingDownCommandDoesNotExist() {
Expand All @@ -244,9 +263,20 @@ private static boolean downCommandWasPresent(String output) {
return !output.contains("No such command");
}

private String psOutput(String service) throws IOException, InterruptedException {
String psOutput = command.execute(Command.throwingOnError(), "ps", service);
validState(!Strings.isNullOrEmpty(psOutput), "No container with name '" + service + "' found");
return psOutput;
private static ErrorHandler swallowingServiceDoesNotExist() {
return (exitCode, output, commandName, commands) -> {
if (exitCode == 1 && output.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exit code 1 and not all exit codes? Will this silently fail if another exit code is thrown?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this is pretty specific, its really only trying to add debug information for when docker can't find a service

~/code/foundry-academy-backend
$ docker-compose ps some_service
ERROR: No such service: some_service
~/code/foundry-academy-backend
$ echo $?
1

Seems it would be more proper for this function to check for various failure exit codes and throw an exception explicitly, while still providing output with suggestions around what the potential failure mode was?

// Note: This (badly) checks if the command most likely failed due to
// the service not existing. If the ErrorHandler had access to
// error output, the proper way to check this would be to inspect
// the error output for "ERROR: No such service: <service_name>"

String fullCommand = Joiner.on(" ").join(Lists.asList(commandName, commands));
String serviceName = commands[commands.length - 1];

log.warn("It looks like `{}` failed.", fullCommand);
log.warn("This probably happened because no `{}` service exists.", serviceName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this throw rather than silently continue? I imagine we can't recover from this?

Copy link
Author

Choose a reason for hiding this comment

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

If that output is produced, the command will have failed with empty output and then will trigger an exception right after it here: https://github.com/palantir/docker-compose-rule/pull/418/files#diff-cf7a91b7706a65a0c4cc650e69cf5d81R242

I agree that maybe a little misleading and we could clarify how swallowingServiceDoesNotExist handles the case

}
};
}
}
Loading