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 <[email protected]> --- test(docker-build): add tests for merging build arguments in BuildService Signed-off-by: Sun Seng David TAN <[email protected]> --- refactor: replace custom isEmpty with StringUtils.isNotBlank Signed-off-by: Sun Seng David TAN <[email protected]>
manusa · Apr 29, 2024 · dbd1067 · dbd1067
1 parent 1c695c7
commit dbd1067
Show file tree

Hide file tree

Showing 6 changed files with 232 additions and 22 deletions.
diff --git 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/...it/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/DockerFileUtilTest.java b/...it/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<String, String> 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<String> 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/...ources/org/eclipse/jkube/kit/build/api/helper/Dockerfile_multi_stage_with_args_no_default b/...ources/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/...service/docker/src/main/java/org/eclipse/jkube/kit/build/service/docker/BuildService.java b/...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<String, String> 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<String, String> mergeBuildArgs(ImageConfiguration imageConfig, JKubeConfiguration configuration) {
+        return prepareBuildArgs(addBuildArgs(configuration), imageConfig.getBuildConfiguration());
     }
 
     public void tagImage(String imageName, ImageConfiguration imageConfig) throws DockerAccessException {
@@ -129,16 +136,14 @@ 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<String, String> mergedBuildMap = prepareBuildArgs(buildArgs, buildConfig);
-
         // auto is now supported by docker, consider switching?
         BuildOptions opts =
                 new BuildOptions(buildConfig.getBuildOptions())
                         .dockerfile(getDockerfileName(buildConfig))
                         .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<String, String> prepareBuildArgs(Map<String, String> buildArgs, BuildConfiguration buildConfig) {
+    private static Map<String, String> prepareBuildArgs(Map<String, String> buildArgs, BuildConfiguration buildConfig) {
         ImmutableMap.Builder<String, String> builder = ImmutableMap.<String, String>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<String, String> addBuildArgs(JKubeConfiguration configuration) {
-        Map<String, String> buildArgsFromProject = addBuildArgsFromProperties(configuration.getProject().getProperties());
+    private static Map<String, String> addBuildArgs(JKubeConfiguration configuration) {
+        Properties props = configuration.getProject().getProperties();
+        Map<String, String> buildArgsFromProject = addBuildArgsFromProperties(props);
         Map<String, String> buildArgsFromSystem = addBuildArgsFromProperties(System.getProperties());
         Map<String, String> buildArgsFromDockerConfig = addBuildArgsFromDockerConfig();
         return ImmutableMap.<String, String>builder()
@@ -194,24 +200,23 @@ private Map<String, String> addBuildArgs(JKubeConfiguration configuration) {
                 .build();
     }
 
-    private Map<String, String> addBuildArgsFromProperties(Properties properties) {
+    private static Map<String, String> addBuildArgsFromProperties(Properties properties) {
         Map<String, String> buildArgs = new HashMap<>();
         for (Object keyObj : properties.keySet()) {
             String key = (String) keyObj;
             if (key.startsWith(ARG_PREFIX)) {
                 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<String, String> addBuildArgsFromDockerConfig() {
+    private static Map<String, String> addBuildArgsFromDockerConfig() {
         final Map<String, Object> dockerConfig = DockerFileUtil.readDockerConfig();
         if (dockerConfig == null) {
             return Collections.emptyMap();
@@ -237,11 +242,11 @@ private Map<String, String> 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<String, String> mergedBuildArgs)
             throws IOException {
         BuildConfiguration buildConfig = imageConfig.getBuildConfiguration();
 
@@ -252,7 +257,7 @@ private void autoPullBaseImage(ImageConfiguration imageConfig, ImagePullManager
 
         List<String> 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<String> extractBaseFromDockerfile(BuildConfiguration buildConfig, JKubeConfiguration configuration) {
+    private List<String> extractBaseFromDockerfile(BuildConfiguration buildConfig, JKubeConfiguration configuration,
+            Map<String, String> mergedBuildArgs) {
         List<String> 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/...ice/docker/src/test/java/org/eclipse/jkube/kit/build/service/docker/BuildServiceTest.java b/...ice/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<String, String> 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<String, String> 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<String, String> 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;
+  }
 }