From dbd106799fe62a5f29715ddd2ffde750c5e78592 Mon Sep 17 00:00:00 2001 From: "Sun S. D. Tan" Date: Mon, 29 Apr 2024 09:29:37 +0200 Subject: [PATCH] fix(docker-build): Ensure Docker build arguments from properties are used during images pre-pulling (2915) fix(docker-build): Ensure Docker build arguments from properties are used during images pre-pulling Signed-off-by: Sun Seng David TAN --- test(docker-build): add tests for merging build arguments in BuildService Signed-off-by: Sun Seng David TAN --- refactor: replace custom isEmpty with StringUtils.isNotBlank Signed-off-by: Sun Seng David TAN --- CHANGELOG.md | 1 + .../build/api/helper/DockerFileUtilTest.java | 16 ++ ...ockerfile_multi_stage_with_args_no_default | 7 + .../build/service/docker/BuildService.java | 45 ++--- .../service/docker/BuildServiceTest.java | 178 +++++++++++++++++- ...ockerfile_multi_stage_with_args_no_default | 7 + 6 files changed, 232 insertions(+), 22 deletions(-) create mode 100644 jkube-kit/build/api/src/test/resources/org/eclipse/jkube/kit/build/api/helper/Dockerfile_multi_stage_with_args_no_default create mode 100644 jkube-kit/build/service/docker/src/test/resources/org/eclipse/jkube/kit/build/service/docker/Dockerfile_multi_stage_with_args_no_default diff --git a/CHANGELOG.md b/CHANGELOG.md index aaa9c0fbc6..22727fe760 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Usage: * Fix #2470: Add configuration option for overriding buildpack builder image * Fix #2662: Sanitize VCS remote URL used in `jkube.eclipse.org/git-url` annotation * Fix #2860: Correctly pass Docker build-arg from the build configuration to the Openshift build strategy +* Fix #2901: Ensure Docker build arguments from properties are used during images pre-pulling ### 1.16.2 (2024-03-27) * Fix #2461: `k8s:watch`/`k8sWatch` should throw error in `buildpacks` build strategy diff --git a/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/DockerFileUtilTest.java b/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/DockerFileUtilTest.java index ff641a8c58..1669996341 100644 --- a/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/DockerFileUtilTest.java +++ b/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/DockerFileUtilTest.java @@ -159,6 +159,22 @@ void extractBaseImages_withMultiStageWithArgs() throws Exception { .containsExactly("fabric8/s2i-java:latest", "busybox:latest", "docker.io/library/openjdk:latest"); } + @Test + void extractBaseImages_withMultiStageAndBuildArgs() throws Exception { + // Given + File toTest = copyToTempDir("Dockerfile_multi_stage_with_args_no_default"); + Map buildArgs = new HashMap<>(); + buildArgs.put("VERSION", "latest"); + buildArgs.put("FULL_IMAGE", "busybox:latest"); + buildArgs.put("REPO_1", "docker.io/library"); + buildArgs.put("IMAGE-1", "openjdk"); + // When + List images = DockerFileUtil.extractBaseImages(toTest, new Properties(), null, buildArgs); + // Then + assertThat(images) + .containsExactly("fabric8/s2i-java:latest", "busybox:latest", "docker.io/library/openjdk:latest"); + } + @Test void extractArgsFromDockerfile() { assertAll( diff --git a/jkube-kit/build/api/src/test/resources/org/eclipse/jkube/kit/build/api/helper/Dockerfile_multi_stage_with_args_no_default b/jkube-kit/build/api/src/test/resources/org/eclipse/jkube/kit/build/api/helper/Dockerfile_multi_stage_with_args_no_default new file mode 100644 index 0000000000..9bce36f575 --- /dev/null +++ b/jkube-kit/build/api/src/test/resources/org/eclipse/jkube/kit/build/api/helper/Dockerfile_multi_stage_with_args_no_default @@ -0,0 +1,7 @@ +ARG VERSION +FROM fabric8/s2i-java:$VERSION AS BUILD +ARG FULL_IMAGE +FROM $FULL_IMAGE AS DEPLOYABLE +ARG REPO_1 +ARG IMAGE-1 +FROM $REPO_1/${IMAGE-1}:$VERSION diff --git a/jkube-kit/build/service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java b/jkube-kit/build/service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java index 37d82752b3..d4cc79d024 100644 --- a/jkube-kit/build/service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java +++ b/jkube-kit/build/service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java @@ -27,6 +27,7 @@ import org.eclipse.jkube.kit.common.JKubeConfiguration; import org.eclipse.jkube.kit.build.api.helper.DockerFileUtil; +import org.apache.commons.lang3.StringUtils; import org.eclipse.jkube.kit.build.api.assembly.AssemblyManager; import org.eclipse.jkube.kit.common.util.EnvUtil; import org.eclipse.jkube.kit.build.service.docker.access.BuildOptions; @@ -69,11 +70,17 @@ public class BuildService { public void buildImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, JKubeConfiguration configuration) throws IOException { + Map mergedBuildArgs = mergeBuildArgs(imageConfig, configuration); + if (imagePullManager != null) { - autoPullBaseImage(imageConfig, imagePullManager, configuration); + autoPullBaseImage(imageConfig, imagePullManager, configuration, mergedBuildArgs); } - buildImage(imageConfig, configuration, checkForNocache(imageConfig), addBuildArgs(configuration)); + buildImage(imageConfig, configuration, checkForNocache(imageConfig), mergedBuildArgs); + } + + static Map mergeBuildArgs(ImageConfiguration imageConfig, JKubeConfiguration configuration) { + return prepareBuildArgs(addBuildArgs(configuration), imageConfig.getBuildConfiguration()); } public void tagImage(String imageName, ImageConfiguration imageConfig) throws DockerAccessException { @@ -129,8 +136,6 @@ protected void buildImage(ImageConfiguration imageConfig, JKubeConfiguration par File dockerArchive = archiveService.createArchive(imageName, buildConfig, params, log); log.info("%s: Created %s in %s", imageConfig.getDescription(), dockerArchive.getName(), EnvUtil.formatDurationTill(time)); - Map mergedBuildMap = prepareBuildArgs(buildArgs, buildConfig); - // auto is now supported by docker, consider switching? BuildOptions opts = new BuildOptions(buildConfig.getBuildOptions()) @@ -138,7 +143,7 @@ protected void buildImage(ImageConfiguration imageConfig, JKubeConfiguration par .forceRemove(cleanupMode.isRemove()) .noCache(noCache) .cacheFrom(buildConfig.getCacheFrom()) - .buildArgs(mergedBuildMap); + .buildArgs(buildArgs); String newImageId = doBuildImage(imageName, dockerArchive, opts); if (newImageId == null) { throw new IllegalStateException("Failure in building image, unable to find image built with name " + imageName); @@ -160,7 +165,7 @@ protected void buildImage(ImageConfiguration imageConfig, JKubeConfiguration par } } - private Map prepareBuildArgs(Map buildArgs, BuildConfiguration buildConfig) { + private static Map prepareBuildArgs(Map buildArgs, BuildConfiguration buildConfig) { ImmutableMap.Builder builder = ImmutableMap.builder().putAll(buildArgs); if (buildConfig.getArgs() != null) { builder.putAll(buildConfig.getArgs()); @@ -182,8 +187,9 @@ private String doBuildImage(String imageName, File dockerArchive, BuildOptions o return queryService.getImageId(imageName); } - private Map addBuildArgs(JKubeConfiguration configuration) { - Map buildArgsFromProject = addBuildArgsFromProperties(configuration.getProject().getProperties()); + private static Map addBuildArgs(JKubeConfiguration configuration) { + Properties props = configuration.getProject().getProperties(); + Map buildArgsFromProject = addBuildArgsFromProperties(props); Map buildArgsFromSystem = addBuildArgsFromProperties(System.getProperties()); Map buildArgsFromDockerConfig = addBuildArgsFromDockerConfig(); return ImmutableMap.builder() @@ -194,7 +200,7 @@ private Map addBuildArgs(JKubeConfiguration configuration) { .build(); } - private Map addBuildArgsFromProperties(Properties properties) { + private static Map addBuildArgsFromProperties(Properties properties) { Map buildArgs = new HashMap<>(); for (Object keyObj : properties.keySet()) { String key = (String) keyObj; @@ -202,16 +208,15 @@ private Map addBuildArgsFromProperties(Properties properties) { String argKey = key.replaceFirst(ARG_PREFIX, ""); String value = properties.getProperty(key); - if (!isEmpty(value)) { + if (StringUtils.isNotBlank(value)) { buildArgs.put(argKey, value); } } } - log.debug("Build args set %s", buildArgs); return buildArgs; } - private Map addBuildArgsFromDockerConfig() { + private static Map addBuildArgsFromDockerConfig() { final Map dockerConfig = DockerFileUtil.readDockerConfig(); if (dockerConfig == null) { return Collections.emptyMap(); @@ -237,11 +242,11 @@ private Map addBuildArgsFromDockerConfig() { } } } - log.debug("Build args set %s", buildArgs); return buildArgs; } - private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, JKubeConfiguration configuration) + private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, + JKubeConfiguration configuration, Map mergedBuildArgs) throws IOException { BuildConfiguration buildConfig = imageConfig.getBuildConfiguration(); @@ -252,7 +257,7 @@ private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager List fromImages; if (buildConfig.isDockerFileMode()) { - fromImages = extractBaseFromDockerfile(buildConfig, configuration); + fromImages = extractBaseFromDockerfile(buildConfig, configuration, mergedBuildArgs); } else { fromImages = new LinkedList<>(); String baseImage = extractBaseFromConfiguration(buildConfig); @@ -267,13 +272,15 @@ private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager } } - private List extractBaseFromDockerfile(BuildConfiguration buildConfig, JKubeConfiguration configuration) { + private List extractBaseFromDockerfile(BuildConfiguration buildConfig, JKubeConfiguration configuration, + Map mergedBuildArgs) { List fromImage; try { File fullDockerFilePath = buildConfig.getAbsoluteDockerFilePath(configuration.getSourceDirectory(), Optional.ofNullable(configuration.getProject().getBaseDirectory()).map(File::toString) .orElse(null)); - fromImage = DockerFileUtil.extractBaseImages(fullDockerFilePath, configuration.getProperties(), buildConfig.getFilter(), buildConfig.getArgs()); + fromImage = DockerFileUtil.extractBaseImages(fullDockerFilePath, configuration.getProperties(), + buildConfig.getFilter(), mergedBuildArgs); } catch (IOException e) { // Cant extract base image, so we wont try an auto pull. An error will occur later anyway when // building the image, so we are passive here. @@ -292,8 +299,4 @@ private boolean checkForNocache(ImageConfiguration imageConfig) { } } - private boolean isEmpty(String str) { - return str == null || str.isEmpty(); - } - } diff --git a/jkube-kit/build/service/docker/src/test/java/org/eclipse/jkube/kit/build/service/docker/BuildServiceTest.java b/jkube-kit/build/service/docker/src/test/java/org/eclipse/jkube/kit/build/service/docker/BuildServiceTest.java index 662cb1a8e1..fff6800585 100644 --- a/jkube-kit/build/service/docker/src/test/java/org/eclipse/jkube/kit/build/service/docker/BuildServiceTest.java +++ b/jkube-kit/build/service/docker/src/test/java/org/eclipse/jkube/kit/build/service/docker/BuildServiceTest.java @@ -13,18 +13,31 @@ */ package org.eclipse.jkube.kit.build.service.docker; +import org.apache.commons.io.FileUtils; import org.eclipse.jkube.kit.build.service.docker.access.DockerAccess; import org.eclipse.jkube.kit.build.service.docker.access.DockerAccessException; import org.eclipse.jkube.kit.common.JKubeConfiguration; import org.eclipse.jkube.kit.common.KitLogger; import org.eclipse.jkube.kit.config.image.ImageConfiguration; import org.eclipse.jkube.kit.config.image.build.BuildConfiguration; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import java.io.File; import java.io.IOException; +import java.io.OutputStream; +import java.nio.file.Files; import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Properties; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -40,12 +53,13 @@ class BuildServiceTest { private ImageConfiguration imageConfiguration; private ImagePullManager mockedImagePullManager; private JKubeConfiguration mockedJKubeConfiguration; + private RegistryService mockedRegistryService; @BeforeEach void setUp() { mockedDockerAccess = mock(DockerAccess.class, RETURNS_DEEP_STUBS); ArchiveService mockedArchiveService = mock(ArchiveService.class, RETURNS_DEEP_STUBS); - RegistryService mockedRegistryService = mock(RegistryService.class, RETURNS_DEEP_STUBS); + mockedRegistryService = mock(RegistryService.class, RETURNS_DEEP_STUBS); KitLogger mockedLog = mock(KitLogger.SilentLogger.class, RETURNS_DEEP_STUBS); mockedImagePullManager = mock(ImagePullManager.class, RETURNS_DEEP_STUBS); mockedJKubeConfiguration = mock(JKubeConfiguration.class, RETURNS_DEEP_STUBS); @@ -93,4 +107,166 @@ void tagImage_whenValidImageConfigurationProvided_shouldTagImage() throws Docker .tag("image-name", "image-name:latest", true); } + @Test + void mergeBuildArgs_whenBuildArgsFromImageConfigAndFromProjectProperties_shouldMergeBuildArgs() { + // Given + Properties props = new Properties(); + props.setProperty("docker.buildArg.VERSION", "latest"); + props.setProperty("docker.buildArg.FULL_IMAGE", "busybox:latest"); + when(mockedJKubeConfiguration.getProject().getProperties()).thenReturn(props); + + Map imgConfigBuildArg = new HashMap<>(); + imgConfigBuildArg.put("REPO_1", "docker.io/library"); + imgConfigBuildArg.put("IMAGE-1", "openjdk"); + imageConfiguration = ImageConfiguration.builder() + .name("image-name") + .build(BuildConfiguration.builder() + .args(imgConfigBuildArg) + .build()) + .build(); + + // When + Map mergedBuildArgs = BuildService.mergeBuildArgs(imageConfiguration, mockedJKubeConfiguration); + + // Then + assertThat(mergedBuildArgs) + .containsEntry("VERSION", "latest") + .containsEntry("FULL_IMAGE", "busybox:latest") + .containsEntry("REPO_1", "docker.io/library") + .containsEntry("IMAGE-1", "openjdk"); + } + + @Nested + @DisplayName("mergeBuildArgs with BuildArgs") + class MergeBuildArgs { + @Test + void fromAllSourcesWithDifferentKeys_shouldMergeBuildArgs() { + // Given + givenBuildArgsFromImageConfiguration("VERSION", "latest"); + givenBuildArgsFromSystemProperties("docker.buildArg.IMAGE-1", "openjdk"); + givenBuildArgsFromProjectProperties("docker.buildArg.REPO_1", "docker.io/library"); + givenBuildArgsFromJKubeConfiguration("FULL_IMAGE", "busybox:latest"); + + // When + Map mergedBuildArgs = BuildService.mergeBuildArgs(imageConfiguration, mockedJKubeConfiguration); + + // Then + assertThat(mergedBuildArgs) + .containsEntry("VERSION", "latest") + .containsEntry("FULL_IMAGE", "busybox:latest") + .containsEntry("REPO_1", "docker.io/library") + .containsEntry("IMAGE-1", "openjdk"); + } + + @Test + void fromBuildConfigurationAndSystemPropertiesWithSameKey_shouldNotMergeBuildArgs() { + // Given + givenBuildArgsFromImageConfiguration("VERSION", "latest"); + givenBuildArgsFromSystemProperties("docker.buildArg.VERSION", "1.0.0"); + + // When & Then + assertThatIllegalArgumentException() + .isThrownBy(() -> BuildService.mergeBuildArgs(imageConfiguration, mockedJKubeConfiguration)) + .withMessage("Multiple entries with same key: VERSION=latest and VERSION=1.0.0"); + } + + @Test + void fromBuildConfigurationAndProjectPropertiesWithSameKey_shouldNotMergeBuildArgs() { + // Given + givenBuildArgsFromImageConfiguration("VERSION", "latest"); + givenBuildArgsFromProjectProperties("docker.buildArg.VERSION", "1.0.0"); + + // When & Then + assertThatIllegalArgumentException() + .isThrownBy(() -> BuildService.mergeBuildArgs(imageConfiguration, mockedJKubeConfiguration)) + .withMessage("Multiple entries with same key: VERSION=latest and VERSION=1.0.0"); + } + + @Test + void fromBuildConfigurationAndJKubeConfigurationWithSameKey_shouldNotMergeBuildArgs() { + // Given + givenBuildArgsFromImageConfiguration("VERSION", "latest"); + givenBuildArgsFromJKubeConfiguration("VERSION", "1.0.0"); + + // When & Then + assertThatIllegalArgumentException() + .isThrownBy(() -> BuildService.mergeBuildArgs(imageConfiguration, mockedJKubeConfiguration)) + .withMessage("Multiple entries with same key: VERSION=latest and VERSION=1.0.0"); + } + + private void givenBuildArgsFromImageConfiguration(String key, String value) { + imageConfiguration = ImageConfiguration.builder() + .name("image-name") + .build(BuildConfiguration.builder() + .args( + Collections.singletonMap(key, value)) + .build()) + .build(); + } + + private void givenBuildArgsFromSystemProperties(String key, String value) { + System.setProperty(key, value); + } + + private void givenBuildArgsFromProjectProperties(String key, String value) { + Properties props = new Properties(); + props.setProperty(key, value); + when(mockedJKubeConfiguration.getProject().getProperties()) + .thenReturn(props); + } + + private void givenBuildArgsFromJKubeConfiguration(String key, String value) { + when(mockedJKubeConfiguration.getBuildArgs()) + .thenReturn(Collections.singletonMap(key, value)); + } + + @AfterEach + void clearSystemPropertiesUsedInTests() { + System.clearProperty("docker.buildArg.IMAGE-1"); + System.clearProperty("docker.buildArg.VERSION"); + } + } + + @Test + void buildImage_whenMultiStageDockerfileWithBuildArgs_shouldPrepullImages() throws IOException { + // Given + when(mockedDockerAccess.getImageId("image-name")).thenReturn("c8003cb6f5db"); + when(mockedJKubeConfiguration.getSourceDirectory()).thenReturn(tempDir.getAbsolutePath()); + when(mockedJKubeConfiguration.getProject().getBaseDirectory()).thenReturn(tempDir); + File multistageDockerfile = copyToTempDir("Dockerfile_multi_stage_with_args_no_default"); + imageConfiguration = ImageConfiguration.builder() + .name("image-name") + .build(BuildConfiguration.builder() + .dockerFile(multistageDockerfile.getPath()) + .dockerFileFile(multistageDockerfile) + .build()) + .build(); + + Properties props = new Properties(); + props.setProperty("docker.buildArg.VERSION", "latest"); + props.setProperty("docker.buildArg.FULL_IMAGE", "busybox:latest"); + props.setProperty("docker.buildArg.REPO_1", "docker.io/library"); + props.setProperty("docker.buildArg.IMAGE-1", "openjdk"); + when(mockedJKubeConfiguration.getProject().getProperties()).thenReturn(props); + + // When + buildService.buildImage(imageConfiguration, mockedImagePullManager, mockedJKubeConfiguration); + + // Then + verify(mockedRegistryService, times(1)).pullImageWithPolicy(eq("fabric8/s2i-java:latest"), any(), any(), any()); + verify(mockedRegistryService, times(1)).pullImageWithPolicy(eq("busybox:latest"), any(), any(), any()); + verify(mockedRegistryService, times(1)).pullImageWithPolicy(eq("docker.io/library/openjdk:latest"), any(), any(), + any()); + } + + @TempDir + private File tempDir; + + private File copyToTempDir(String resource) throws IOException { + File ret = new File(tempDir, "Dockerfile"); + try (OutputStream os = Files.newOutputStream(ret.toPath())) { + FileUtils.copyFile(new File(getClass().getResource(resource).getPath()), os); + } + return ret; + } } diff --git a/jkube-kit/build/service/docker/src/test/resources/org/eclipse/jkube/kit/build/service/docker/Dockerfile_multi_stage_with_args_no_default b/jkube-kit/build/service/docker/src/test/resources/org/eclipse/jkube/kit/build/service/docker/Dockerfile_multi_stage_with_args_no_default new file mode 100644 index 0000000000..9bce36f575 --- /dev/null +++ b/jkube-kit/build/service/docker/src/test/resources/org/eclipse/jkube/kit/build/service/docker/Dockerfile_multi_stage_with_args_no_default @@ -0,0 +1,7 @@ +ARG VERSION +FROM fabric8/s2i-java:$VERSION AS BUILD +ARG FULL_IMAGE +FROM $FULL_IMAGE AS DEPLOYABLE +ARG REPO_1 +ARG IMAGE-1 +FROM $REPO_1/${IMAGE-1}:$VERSION