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: + * + * @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