From acbd1813c9ddb724a71589c2518c01e1db66bf8e Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Mon, 29 Apr 2024 23:23:52 +0530 Subject: [PATCH] fix (jkube-kit/config) : `docker.buildArg.*` properties not taken into account in OpenShift plugins Added call to resolve build args from all sources in OpenShiftBuildServiceUtils so that build args get resolved from all the available sources (compared to just ImageConfig) Signed-off-by: Rohan Kumar --- CHANGELOG.md | 1 + .../api/helper/BuildArgResolverUtil.java | 56 +++++-- ...uildArgResolverUtilMergeBuildArgsTest.java | 57 +++++-- .../build/service/docker/BuildService.java | 4 +- .../openshift/OpenShiftBuildServiceUtils.java | 5 +- .../OpenShiftBuildServiceUtilsTest.java | 156 +++++++++++------- 6 files changed, 185 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 378616ddc1..22846a6a53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ Usage: * Fix #2860: Correctly pass Docker build-arg from the build configuration to the Openshift build strategy * Fix #2885: Provide a way to set labels on images defined by Generators * Fix #2901: Ensure Docker build arguments from properties are used during images pre-pulling +* Fix #2904: `docker.buildArg.*` properties not taken into account in OpenShift plugins ### 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/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java b/jkube-kit/build/api/src/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java index f4da8356e6..42f4330121 100644 --- a/jkube-kit/build/api/src/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java +++ b/jkube-kit/build/api/src/main/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtil.java @@ -44,24 +44,46 @@ private BuildArgResolverUtil() { } * @param configuration {@link JKubeConfiguration}. * @return a Map containing merged Build Args from all sources. */ - public static Map mergeBuildArgs(ImageConfiguration imageConfig, JKubeConfiguration configuration) { + public static Map mergeBuildArgsIncludingLocalDockerConfigProxySettings(ImageConfiguration imageConfig, JKubeConfiguration configuration) { + return mergeBuildArgsFrom(imageConfig.getBuild().getArgs(), + buildArgsFromProperties(System.getProperties()), + buildArgsFromProperties(configuration.getProject().getProperties()), + configuration.getBuildArgs(), + buildArgsFromDockerConfig()); + } + + /** + * Merges Docker Build Args from the following sources: + *
    + *
  • Build Args specified directly in ImageConfiguration
    • + *
  • Build Args specified via System Properties
    • + *
  • Build Args specified via Project Properties
    • + *
  • Build Args specified via Plugin configuration
    • + *
+ * @param imageConfig ImageConfiguration where to get the Build Args from. + * @param configuration {@link JKubeConfiguration}. + * @return a Map containing merged Build Args from all sources. + */ + public static Map mergeBuildArgsWithoutLocalDockerConfigProxySettings(ImageConfiguration imageConfig, JKubeConfiguration configuration) { + return mergeBuildArgsFrom(imageConfig.getBuild().getArgs(), + buildArgsFromProperties(System.getProperties()), + buildArgsFromProperties(configuration.getProject().getProperties()), + configuration.getBuildArgs()); + } + + @SafeVarargs + private static Map mergeBuildArgsFrom(Map... buildArgSources) { final Map buildArgs = new HashMap<>(); - Stream.of( - imageConfig.getBuild().getArgs(), - buildArgsFromProperties(System.getProperties()), - buildArgsFromProperties(configuration.getProject().getProperties()), - configuration.getBuildArgs(), - buildArgsFromDockerConfig() - ) - .filter(Objects::nonNull) - .flatMap(map -> map.entrySet().stream()) - .forEach(entry -> { - if (buildArgs.containsKey(entry.getKey())) { - throw new JKubeException(String.format("Multiple Build Args with the same key: %s=%s and %s=%s", - entry.getKey(), buildArgs.get(entry.getKey()), entry.getKey(), entry.getValue())); - } - buildArgs.put(entry.getKey(), entry.getValue()); - }); + Stream.of(buildArgSources) + .filter(Objects::nonNull) + .flatMap(map -> map.entrySet().stream()) + .forEach(entry -> { + if (buildArgs.containsKey(entry.getKey())) { + throw new JKubeException(String.format("Multiple Build Args with the same key: %s=%s and %s=%s", + entry.getKey(), buildArgs.get(entry.getKey()), entry.getKey(), entry.getValue())); + } + buildArgs.put(entry.getKey(), entry.getValue()); + }); return buildArgs; } diff --git a/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java b/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java index 41e0dbb631..ad5aa9775e 100644 --- a/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java +++ b/jkube-kit/build/api/src/test/java/org/eclipse/jkube/kit/build/api/helper/BuildArgResolverUtilMergeBuildArgsTest.java @@ -22,6 +22,7 @@ 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; @@ -74,7 +75,7 @@ void whenBuildArgsFromImageConfigAndFromProjectProperties_shouldMergeBuildArgs() .build(); // When - Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration); + Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration); // Then assertThat(mergedBuildArgs) @@ -94,7 +95,7 @@ void fromAllSourcesWithDifferentKeys_shouldMergeBuildArgs() { givenBuildArgsFromJKubeConfiguration("FULL_IMAGE", "busybox:latest"); // When - Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration); + Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration); // Then assertThat(mergedBuildArgs) @@ -113,7 +114,7 @@ void fromBuildConfigurationAndSystemPropertiesWithSameKey_shouldNotMergeBuildArg // When & Then assertThatExceptionOfType(JKubeException.class) - .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration)) + .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration)) .withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0"); } @@ -126,7 +127,7 @@ void fromBuildConfigurationAndProjectPropertiesWithSameKey_shouldNotMergeBuildAr // When & Then assertThatExceptionOfType(JKubeException.class) - .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration)) + .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration)) .withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0"); } @@ -139,31 +140,55 @@ void fromBuildConfigurationAndJKubeConfigurationWithSameKey_shouldNotMergeBuildA // When & Then assertThatExceptionOfType(JKubeException.class) - .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration)) + .isThrownBy(() -> BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration)) .withMessage("Multiple Build Args with the same key: VERSION=latest and VERSION=1.0.0"); } - @Test - @DisplayName("should add proxy build args from ~/.docker/config.json") - void shouldAddBuildArgsFromDockerConfig(@TempDir File temporaryFolder) throws IOException { - try { - // Given + @Nested + @DisplayName("local ~/.docker/config.json contains proxy settings") + class LocalDockerConfigContainsProxySettings { + @TempDir + private File temporaryFolder; + + @BeforeEach + void setUp() throws IOException { Path dockerConfig = temporaryFolder.toPath(); final Map env = Collections.singletonMap("DOCKER_CONFIG", dockerConfig.toFile().getAbsolutePath()); EnvUtil.overrideEnvGetter(env::get); Files.write(dockerConfig.resolve("config.json"), ("{\"proxies\": {\"default\": {\n" + - " \"httpProxy\": \"http://proxy.example.com:3128\",\n" + - " \"httpsProxy\": \"https://proxy.example.com:3129\",\n" + - " \"noProxy\": \"*.test.example.com,.example.org,127.0.0.0/8\"\n" + - " }}}").getBytes()); + " \"httpProxy\": \"http://proxy.example.com:3128\",\n" + + " \"httpsProxy\": \"https://proxy.example.com:3129\",\n" + + " \"noProxy\": \"*.test.example.com,.example.org,127.0.0.0/8\"\n" + + " }}}").getBytes()); + + } + + @Test + @DisplayName("mergeBuildArgsIncludingLocalDockerConfigProxySettings, should add proxy build args for docker build strategy") + void shouldAddBuildArgsFromDockerConfigInDockerBuild() { // When - final Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgs(imageConfiguration, jKubeConfiguration); + final Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration); // Then assertThat(mergedBuildArgs) .containsEntry("docker.buildArg.http_proxy", "http://proxy.example.com:3128") .containsEntry("docker.buildArg.https_proxy", "https://proxy.example.com:3129") .containsEntry("docker.buildArg.no_proxy", "*.test.example.com,.example.org,127.0.0.0/8"); - } finally { + } + + @Test + @DisplayName("mergeBuildArgsWithoutIncludingLocalDockerConfigProxySettings, should not add proxy build args for OpenShift build strategy") + void shouldNotAddBuildArgsFromDockerConfig() { + // When + final Map mergedBuildArgs = BuildArgResolverUtil.mergeBuildArgsWithoutLocalDockerConfigProxySettings(imageConfiguration, jKubeConfiguration); + // Then + assertThat(mergedBuildArgs) + .doesNotContainEntry("docker.buildArg.http_proxy", "http://proxy.example.com:3128") + .doesNotContainEntry("docker.buildArg.https_proxy", "https://proxy.example.com:3129") + .doesNotContainEntry("docker.buildArg.no_proxy", "*.test.example.com,.example.org,127.0.0.0/8"); + } + + @AfterEach + void tearDown() { EnvUtil.overrideEnvGetter(System::getenv); } } 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 afe53693a6..ad0ed0c05f 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 @@ -34,7 +34,7 @@ import org.eclipse.jkube.kit.config.image.build.BuildConfiguration; import org.eclipse.jkube.kit.config.image.build.CleanupMode; -import static org.eclipse.jkube.kit.build.api.helper.BuildArgResolverUtil.mergeBuildArgs; +import static org.eclipse.jkube.kit.build.api.helper.BuildArgResolverUtil.mergeBuildArgsIncludingLocalDockerConfigProxySettings; import static org.eclipse.jkube.kit.build.api.helper.BuildUtil.extractBaseFromConfiguration; public class BuildService { @@ -64,7 +64,7 @@ public class BuildService { public void buildImage(ImageConfiguration imageConfig, ImagePullManager imagePullManager, JKubeConfiguration configuration) throws IOException { - Map mergedBuildArgs = mergeBuildArgs(imageConfig, configuration); + Map mergedBuildArgs = mergeBuildArgsIncludingLocalDockerConfigProxySettings(imageConfig, configuration); if (imagePullManager != null) { autoPullBaseImage(imageConfig, imagePullManager, configuration, mergedBuildArgs); diff --git a/jkube-kit/config/service/src/main/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtils.java b/jkube-kit/config/service/src/main/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtils.java index e2144dcfb1..eb30e953c5 100644 --- a/jkube-kit/config/service/src/main/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtils.java +++ b/jkube-kit/config/service/src/main/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtils.java @@ -47,6 +47,7 @@ import java.util.Optional; import java.util.stream.Collectors; +import static org.eclipse.jkube.kit.build.api.helper.BuildArgResolverUtil.mergeBuildArgsWithoutLocalDockerConfigProxySettings; import static org.eclipse.jkube.kit.build.api.helper.BuildUtil.extractBaseFromDockerfile; import static org.eclipse.jkube.kit.config.service.openshift.ImageStreamService.resolveImageStreamName; import static org.eclipse.jkube.kit.config.service.openshift.OpenshiftBuildService.DEFAULT_BUILD_OUTPUT_KIND; @@ -133,7 +134,9 @@ protected static BuildStrategy createBuildStrategy( .withNamespace(StringUtils.isEmpty(fromNamespace) ? null : fromNamespace) .endFrom() .withEnv(checkForEnv(imageConfig)) - .withBuildArgs(Optional.ofNullable(buildConfig.getArgs()).orElse(Collections.emptyMap()).entrySet().stream() + .withBuildArgs(mergeBuildArgsWithoutLocalDockerConfigProxySettings(imageConfig, jKubeServiceHub.getConfiguration()) + .entrySet() + .stream() .map(bcArg -> new EnvVarBuilder() .withName(bcArg.getKey()) .withValue(bcArg.getValue()).build()) diff --git a/jkube-kit/config/service/src/test/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtilsTest.java b/jkube-kit/config/service/src/test/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtilsTest.java index 9b1fec0d2a..c553b495a0 100644 --- a/jkube-kit/config/service/src/test/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtilsTest.java +++ b/jkube-kit/config/service/src/test/java/org/eclipse/jkube/kit/config/service/openshift/OpenShiftBuildServiceUtilsTest.java @@ -18,12 +18,15 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Properties; import io.fabric8.openshift.api.model.ImageStreamTag; import io.fabric8.openshift.api.model.ImageStreamTagBuilder; +import org.assertj.core.api.InstanceOfAssertFactories; import org.eclipse.jkube.kit.build.api.assembly.ArchiverCustomizer; import org.eclipse.jkube.kit.build.api.assembly.JKubeBuildTarArchiver; import org.eclipse.jkube.kit.common.JKubeConfiguration; +import org.eclipse.jkube.kit.common.JavaProject; import org.eclipse.jkube.kit.config.image.ImageConfiguration; import org.eclipse.jkube.kit.config.image.ImageName; import org.eclipse.jkube.kit.config.image.build.BuildConfiguration; @@ -40,6 +43,8 @@ import io.fabric8.openshift.api.model.BuildStrategy; 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 org.mockito.ArgumentCaptor; @@ -75,6 +80,10 @@ void setUp() { jKubeServiceHub = mock(JKubeServiceHub.class, RETURNS_DEEP_STUBS); when(jKubeServiceHub.getBuildServiceConfig().getBuildDirectory()) .thenReturn(temporaryFolder.getAbsolutePath()); + when(jKubeServiceHub.getConfiguration()).thenReturn(JKubeConfiguration.builder() + .project(JavaProject.builder().build()) + .buildArgs(Collections.emptyMap()) + .build()); imageConfiguration = ImageConfiguration.builder() .name("myapp") .build(BuildConfiguration.builder() @@ -183,64 +192,95 @@ void createBuildStrategy_withS2iBuildStrategyAndPullSecret_shouldReturnValidBuil .hasFieldOrPropertyWithValue("pullSecret.name", "my-secret-for-pull"); } - @Test - void createBuildStrategy_withDockerBuildStrategyAndNoPullSecret_shouldReturnValidBuildStrategy() { - // Given - when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy()) - .thenReturn(JKubeBuildStrategy.docker); - // When - final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, null); - // Then - assertThat(result) - .hasFieldOrPropertyWithValue("type", "Docker") - .extracting(BuildStrategy::getDockerStrategy) - .hasFieldOrPropertyWithValue("from.kind", "DockerImage") - .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base"); - } - - @Test - void createBuildStrategy_withDockerBuildArgs_shouldReturnValidBuildStrategyWithBuildArgs() { - // Given - when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy()) - .thenReturn(JKubeBuildStrategy.docker); - ImageConfiguration imageConfig = ImageConfiguration.builder() - .name("myapp") - .build(BuildConfiguration.builder() - .from("ubi8/s2i-base") - .args(Collections.singletonMap("BUILD_ARGS_KEY", "build-args-value")) - .build()) - .build(); - // When - final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfig, "my-secret-for-pull"); - // Then - assertThat(result) - .hasFieldOrPropertyWithValue("type", "Docker") - .extracting(BuildStrategy::getDockerStrategy) - .hasFieldOrPropertyWithValue("from.kind", "DockerImage") - .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base") - .extracting("buildArgs").asList().hasSize(1).first().satisfies( - envVar -> { - assertThat(envVar) - .hasFieldOrPropertyWithValue("name", "BUILD_ARGS_KEY") - .hasFieldOrPropertyWithValue("value", "build-args-value"); - }); - } - - @Test - void createBuildStrategy_withDockerBuildStrategyAndPullSecret_shouldReturnValidBuildStrategy() { - // Given - when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy()) - .thenReturn(JKubeBuildStrategy.docker); - // When - final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull"); - // Then - assertThat(result) - .hasFieldOrPropertyWithValue("type", "Docker") - .extracting(BuildStrategy::getDockerStrategy) - .hasFieldOrPropertyWithValue("from.kind", "DockerImage") - .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base") - .hasFieldOrPropertyWithValue("noCache", false) - .hasFieldOrPropertyWithValue("pullSecret.name", "my-secret-for-pull"); + @Nested + @DisplayName("docker BuildStrategy") + class DockerBuildStrategy { + @BeforeEach + void setUp() { + when(jKubeServiceHub.getBuildServiceConfig().getJKubeBuildStrategy()) + .thenReturn(JKubeBuildStrategy.docker); + } + + @Test + @DisplayName("no pull secret specified") + void withDockerBuildStrategyAndNoPullSecret_shouldReturnValidBuildStrategy() { + // When + final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, null); + // Then + assertThat(result) + .hasFieldOrPropertyWithValue("type", "Docker") + .extracting(BuildStrategy::getDockerStrategy) + .hasFieldOrPropertyWithValue("from.kind", "DockerImage") + .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base"); + } + + @Test + @DisplayName("pull secret specified, should reflect in Build Strategy") + void withPullSecret_shouldReturnValidBuildStrategy() { + // When + final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull"); + // Then + assertThat(result) + .hasFieldOrPropertyWithValue("type", "Docker") + .extracting(BuildStrategy::getDockerStrategy) + .hasFieldOrPropertyWithValue("from.kind", "DockerImage") + .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base") + .hasFieldOrPropertyWithValue("noCache", false) + .hasFieldOrPropertyWithValue("pullSecret.name", "my-secret-for-pull"); + } + + @Nested + @DisplayName("with build args") + class WithDockerBuildArgs { + @Test + @DisplayName("when docker build args in ImageConfig, then add them to BuildStrategy") + void withDockerBuildArgsInImageConfiguration_shouldReturnValidBuildStrategyWithBuildArgs() { + // Given + imageConfiguration = imageConfiguration.toBuilder() + .build(imageConfiguration.getBuild().toBuilder() + .args(Collections.singletonMap("BUILD_ARGS_KEY", "build-args-value")) + .build()) + .build(); + // When + final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull"); + // Then + assertBuildArgPresentInBuildStrategy(result); + } + + @Test + @DisplayName("when docker build args in project properties, then add them to BuildStrategy") + void withDockerBuildArgsInProjectProperties_shouldReturnValidBuildStrategyWithBuildArgs() { + // Given + Properties properties = new Properties(); + properties.put("docker.buildArg.BUILD_ARGS_KEY", "build-args-value"); + when(jKubeServiceHub.getConfiguration()).thenReturn(JKubeConfiguration.builder() + .project(JavaProject.builder() + .properties(properties) + .build()) + .build()); + // When + final BuildStrategy result = createBuildStrategy(jKubeServiceHub, imageConfiguration, "my-secret-for-pull"); + // Then + assertBuildArgPresentInBuildStrategy(result); + } + + void assertBuildArgPresentInBuildStrategy(BuildStrategy result) { + assertThat(result) + .hasFieldOrPropertyWithValue("type", "Docker") + .extracting(BuildStrategy::getDockerStrategy) + .hasFieldOrPropertyWithValue("from.kind", "DockerImage") + .hasFieldOrPropertyWithValue("from.name", "ubi8/s2i-base") + .extracting("buildArgs") + .asInstanceOf(InstanceOfAssertFactories.LIST) + .singleElement() + .satisfies( + envVar -> { + assertThat(envVar) + .hasFieldOrPropertyWithValue("name", "BUILD_ARGS_KEY") + .hasFieldOrPropertyWithValue("value", "build-args-value"); + }); + } + } } @Test