From 4cb555a6158eaf9c1ca6b53181f1953d4b1d462f Mon Sep 17 00:00:00 2001 From: Richard North Date: Wed, 22 Apr 2020 20:54:27 +0100 Subject: [PATCH] Revert "Support ImageFromDockerfile authenticated image pulls (#2573)" f631023576288234e7aacdce867520200b2dcfda --- .../images/builder/ImageFromDockerfile.java | 49 +++------ .../utility/AuthenticatedImagePullTest.java | 103 ++++-------------- 2 files changed, 40 insertions(+), 112 deletions(-) diff --git a/core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java b/core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java index 11a7e2f4cb6..1140247960a 100644 --- a/core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java +++ b/core/src/main/java/org/testcontainers/images/builder/ImageFromDockerfile.java @@ -83,6 +83,16 @@ protected final String resolve() { DockerClient dockerClient = DockerClientFactory.instance().client(); + dependencyImageNames.forEach(imageName -> { + try { + log.info("Pre-emptively checking local images for '{}', referenced via a Dockerfile. If not available, it will be pulled.", imageName); + DockerClientFactory.instance().checkAndPullImage(dockerClient, imageName); + } catch (Exception e) { + log.warn("Unable to pre-fetch an image ({}) depended upon by Dockerfile - image build will continue but may fail. Exception message was: {}", imageName, e.getMessage()); + } + }); + + try { if (deleteOnExit) { ResourceReaper.instance().registerImageForCleanup(dockerImageName); @@ -108,8 +118,6 @@ public void onNext(BuildResponseItem item) { BuildImageCmd buildImageCmd = dockerClient.buildImageCmd(in); configure(buildImageCmd); - prePullDependencyImages(dependencyImageNames); - BuildImageResultCallback exec = buildImageCmd.exec(resultCallback); long bytesToDockerDaemon = 0; @@ -146,30 +154,11 @@ protected void configure(BuildImageCmd buildImageCmd) { this.dockerfile.ifPresent(p -> { buildImageCmd.withDockerfile(p.toFile()); dependencyImageNames = new ParsedDockerfile(p).getDependencyImageNames(); - - if (dependencyImageNames.size() > 0) { - // if we'll be pre-pulling images, disable the built-in pull as it is not necessary and will fail for - // authenticated registries - buildImageCmd.withPull(false); - } }); this.buildArgs.forEach(buildImageCmd::withBuildArg); } - private void prePullDependencyImages(Set imagesToPull) { - final DockerClient dockerClient = DockerClientFactory.instance().client(); - - imagesToPull.forEach(imageName -> { - try { - log.info("Pre-emptively checking local images for '{}', referenced via a Dockerfile. If not available, it will be pulled.", imageName); - DockerClientFactory.instance().checkAndPullImage(dockerClient, imageName); - } catch (Exception e) { - log.warn("Unable to pre-fetch an image ({}) depended upon by Dockerfile - image build will continue but may fail. Exception message was: {}", imageName, e.getMessage()); - } - }); - } - public ImageFromDockerfile withBuildArg(final String key, final String value) { this.buildArgs.put(key, value); return this; @@ -182,23 +171,19 @@ public ImageFromDockerfile withBuildArgs(final Map args) { /** * Sets the Dockerfile to be used for this image. - * - * @param relativePathFromBuildContextDirectory relative path to the Dockerfile, relative to the image build context directory - * @deprecated It is recommended to use {@link #withDockerfile} instead because it honors .dockerignore files and - * will therefore be more efficient. Additionally, using {@link #withDockerfile} supports Dockerfiles that depend - * upon images from authenticated private registries. + * @deprecated It is recommended to use {@link #withDockerfile} instead because it honors + * .dockerignore files and therefore will be more efficient + * @param relativePathFromBuildRoot */ @Deprecated - public ImageFromDockerfile withDockerfilePath(String relativePathFromBuildContextDirectory) { - this.dockerFilePath = Optional.of(relativePathFromBuildContextDirectory); + public ImageFromDockerfile withDockerfilePath(String relativePathFromBuildRoot) { + this.dockerFilePath = Optional.of(relativePathFromBuildRoot); return this; } /** - * Sets the Dockerfile to be used for this image. Honors .dockerignore files for efficiency. - * Additionally, supports Dockerfiles that depend upon images from authenticated private registries. - * - * @param dockerfile path to Dockerfile on the test host. + * Sets the Dockerfile to be used for this image. + * @param dockerfile */ public ImageFromDockerfile withDockerfile(Path dockerfile) { this.dockerfile = Optional.of(dockerfile); diff --git a/core/src/test/java/org/testcontainers/utility/AuthenticatedImagePullTest.java b/core/src/test/java/org/testcontainers/utility/AuthenticatedImagePullTest.java index 56945643a88..78b54798cd1 100644 --- a/core/src/test/java/org/testcontainers/utility/AuthenticatedImagePullTest.java +++ b/core/src/test/java/org/testcontainers/utility/AuthenticatedImagePullTest.java @@ -1,27 +1,19 @@ package org.testcontainers.utility; import com.github.dockerjava.api.DockerClient; -import com.github.dockerjava.api.async.ResultCallback; -import com.github.dockerjava.api.command.PullImageResultCallback; import com.github.dockerjava.api.model.AuthConfig; -import org.intellij.lang.annotations.Language; +import com.github.dockerjava.core.command.PullImageResultCallback; +import com.github.dockerjava.core.command.PushImageResultCallback; import org.junit.AfterClass; -import org.junit.Before; import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; import org.mockito.Mockito; import org.testcontainers.DockerClientFactory; -import org.testcontainers.containers.ContainerState; -import org.testcontainers.containers.DockerComposeContainer; import org.testcontainers.containers.GenericContainer; import org.testcontainers.containers.wait.strategy.HttpWaitStrategy; import org.testcontainers.images.builder.ImageFromDockerfile; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.concurrent.TimeUnit; import static org.mockito.ArgumentMatchers.any; @@ -33,17 +25,14 @@ * This test checks the integration between Testcontainers and an authenticated registry, but uses * a mock instance of {@link RegistryAuthLocator} - the purpose of the test is solely to ensure that * the auth locator is utilised, and that the credentials it provides flow through to the registry. - *

+ * * {@link RegistryAuthLocatorTest} covers actual credential scenarios at a lower level, which are * impractical to test end-to-end. */ public class AuthenticatedImagePullTest { - /** - * Containerised docker image registry, with simple hardcoded credentials - */ @ClassRule - public static GenericContainer authenticatedRegistry = new GenericContainer<>(new ImageFromDockerfile() + public static GenericContainer authenticatedRegistry = new GenericContainer(new ImageFromDockerfile() .withDockerfileFromBuilder(builder -> { builder.from("registry:2") .run("htpasswd -Bbn testuser notasecret > /htpasswd") @@ -57,17 +46,28 @@ public class AuthenticatedImagePullTest { private static RegistryAuthLocator originalAuthLocatorSingleton; private static DockerClient client; + private static String testRegistryAddress; private static String testImageName; private static String testImageNameWithTag; @BeforeClass - public static void setUp() throws InterruptedException { + public static void setUp() { originalAuthLocatorSingleton = RegistryAuthLocator.instance(); client = DockerClientFactory.instance().client(); - String testRegistryAddress = authenticatedRegistry.getContainerIpAddress() + ":" + authenticatedRegistry.getFirstMappedPort(); + testRegistryAddress = authenticatedRegistry.getContainerIpAddress() + ":" + authenticatedRegistry.getFirstMappedPort(); testImageName = testRegistryAddress + "/alpine"; testImageNameWithTag = testImageName + ":latest"; + } + + @AfterClass + public static void tearDown() { + RegistryAuthLocator.setInstance(originalAuthLocatorSingleton); + client.removeImageCmd(testImageNameWithTag).withForce(true).exec(); + } + + @Test + public void testThatAuthLocatorIsUsed() throws Exception { final DockerImageName expectedName = new DockerImageName(testImageNameWithTag); final AuthConfig authConfig = new AuthConfig() @@ -83,23 +83,9 @@ public static void setUp() throws InterruptedException { // a push will use the auth locator for authentication, although that isn't the goal of this test putImageInRegistry(); - } - - @Before - public void removeImageFromLocalDocker() { - // remove the image tag from local docker so that it must be pulled before use - client.removeImageCmd(testImageNameWithTag).withForce(true).exec(); - } - @AfterClass - public static void tearDown() { - RegistryAuthLocator.setInstance(originalAuthLocatorSingleton); - } - - @Test - public void testThatAuthLocatorIsUsedForContainerCreation() { // actually start a container, which will require an authenticated pull - try (final GenericContainer container = new GenericContainer<>(testImageNameWithTag) + try (final GenericContainer container = new GenericContainer<>(testImageNameWithTag) .withCommand("/bin/sh", "-c", "sleep 10")) { container.start(); @@ -107,53 +93,7 @@ public void testThatAuthLocatorIsUsedForContainerCreation() { } } - @Test - public void testThatAuthLocatorIsUsedForDockerfileBuild() throws IOException { - // Prepare a simple temporary Dockerfile which requires our custom private image - Path tempContext = Files.createTempDirectory(Paths.get("."), this.getClass().getSimpleName() + "-test-"); - Path tempFile = Files.createTempFile(tempContext, "test", ".Dockerfile"); - String dockerFileContent = "FROM " + testImageNameWithTag; - Files.write(tempFile, dockerFileContent.getBytes()); - - // Start a container built from a derived image, which will require an authenticated pull - try (final GenericContainer container = new GenericContainer<>( - new ImageFromDockerfile() - .withDockerfile(tempFile) - ) - .withCommand("/bin/sh", "-c", "sleep 10")) { - container.start(); - - assertTrue("container started following an authenticated pull", container.isRunning()); - } - } - - @Test - public void testThatAuthLocatorIsUsedForDockerComposePull() throws IOException { - // Prepare a simple temporary Docker Compose manifest which requires our custom private image - Path tempContext = Files.createTempDirectory(Paths.get("."), this.getClass().getSimpleName() + "-test-"); - Path tempFile = Files.createTempFile(tempContext, "test", ".docker-compose.yml"); - @Language("yaml") String composeFileContent = - "version: '2.0'\n" + - "services:\n" + - " privateservice:\n" + - " command: /bin/sh -c 'sleep 60'\n" + - " image: " + testImageNameWithTag; - Files.write(tempFile, composeFileContent.getBytes()); - - // Start the docker compose project, which will require an authenticated pull - try (final DockerComposeContainer compose = new DockerComposeContainer<>(tempFile.toFile())) { - compose.start(); - - assertTrue("container started following an authenticated pull", - compose - .getContainerByServiceName("privateservice_1") - .map(ContainerState::isRunning) - .orElse(false) - ); - } - } - - private static void putImageInRegistry() throws InterruptedException { + private void putImageInRegistry() throws InterruptedException { // It doesn't matter which image we use for this test, but use one that's likely to have been pulled already final String dummySourceImage = TestcontainersConfiguration.getInstance().getRyukImage(); @@ -169,7 +109,10 @@ private static void putImageInRegistry() throws InterruptedException { client.tagImageCmd(id, testImageName, "latest").exec(); client.pushImageCmd(testImageNameWithTag) - .exec(new ResultCallback.Adapter<>()) + .exec(new PushImageResultCallback()) .awaitCompletion(1, TimeUnit.MINUTES); + + // remove the image tag from local docker so that it must be pulled before use + client.removeImageCmd(testImageNameWithTag).withForce(true).exec(); } }