From 5241094885f095e7e3db0182c1029ac0adc01d4b Mon Sep 17 00:00:00 2001 From: Ioannis Canellos Date: Fri, 14 Jul 2023 13:56:38 +0300 Subject: [PATCH 1/2] refactor: protect container image segment config --- .../container/image/deployment/ContainerImageConfig.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java b/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java index 9db9a56e09fc7..d7f1f6889472b 100644 --- a/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java +++ b/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java @@ -17,20 +17,20 @@ public class ContainerImageConfig { */ @ConfigItem(defaultValue = "${user.name}") @ConvertWith(TrimmedStringConverter.class) - public Optional group; + Optional group; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead /** * The name of the container image. If not set defaults to the application name */ @ConfigItem(defaultValue = "${quarkus.application.name:unset}") @ConvertWith(TrimmedStringConverter.class) - public String name; + String name; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead /** * The tag of the container image. If not set defaults to the application version */ @ConfigItem(defaultValue = "${quarkus.application.version:latest}") - public Optional tag; + Optional tag; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead /** * Additional tags of the container image. From 06ee62481cbd87ac9066a48e16e6400af53115a0 Mon Sep 17 00:00:00 2001 From: Ioannis Canellos Date: Mon, 24 Jul 2023 18:20:40 +0300 Subject: [PATCH 2/2] fix: single segment images for ocp image streams --- .../deployment/ContainerImageConfig.java | 24 +---- .../deployment/ContainerImageProcessor.java | 69 ++++++++++--- ...ontainerImageConfigEffectiveGroupTest.java | 19 ++-- .../deployment/ContainerImageInfoTest.java | 3 +- ...SegmentContainerImageRequestBuildItem.java | 10 ++ .../deployment/OpenshiftProcessor.java | 5 +- ...hDeploymentResourceAndLocalLookupTest.java | 99 +++++++++++++++++++ 7 files changed, 179 insertions(+), 50 deletions(-) create mode 100644 extensions/container-image/spi/src/main/java/io/quarkus/container/spi/SingleSegmentContainerImageRequestBuildItem.java create mode 100644 integration-tests/kubernetes/quarkus-standard-way/src/test/java/io/quarkus/it/kubernetes/OpenshiftWithDeploymentResourceAndLocalLookupTest.java diff --git a/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java b/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java index d7f1f6889472b..fc1af2b9dca50 100644 --- a/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java +++ b/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageConfig.java @@ -15,7 +15,7 @@ public class ContainerImageConfig { /** * The group the container image will be part of */ - @ConfigItem(defaultValue = "${user.name}") + @ConfigItem @ConvertWith(TrimmedStringConverter.class) Optional group; //used only by ContainerImageProcessor, use ContainerImageInfoBuildItem instead @@ -110,26 +110,4 @@ public boolean isPushExplicitlyEnabled() { public boolean isPushExplicitlyDisabled() { return push.isPresent() && !push.get(); } - - /** - * Since user.name which is default value can be uppercase and uppercase values are not allowed - * in the repository part of image references, we need to make the username lowercase. - * If spaces exist in the user name, we replace them with the dash character. - * - * We purposely don't change the value of an explicitly set group. - */ - public Optional getEffectiveGroup() { - if (group.isPresent()) { - String originalGroup = group.get(); - if (originalGroup.equals(System.getProperty("user.name"))) { - if (originalGroup.isEmpty()) { - return Optional.empty(); - } - return Optional.of(originalGroup.toLowerCase().replace(' ', '-')); - } else if (originalGroup.isEmpty()) { - return Optional.empty(); - } - } - return group; - } } diff --git a/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageProcessor.java b/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageProcessor.java index 742e2a718e54e..5a1f02ff9384c 100644 --- a/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageProcessor.java +++ b/extensions/container-image/deployment/src/main/java/io/quarkus/container/image/deployment/ContainerImageProcessor.java @@ -1,12 +1,15 @@ package io.quarkus.container.image.deployment; -import static io.quarkus.container.image.deployment.util.EnablementUtil.*; +import static io.quarkus.container.image.deployment.util.EnablementUtil.buildOrPushContainerImageNeeded; import static io.quarkus.container.spi.ImageReference.DEFAULT_TAG; import static io.quarkus.deployment.builditem.ApplicationInfoBuildItem.UNSET_VALUE; import java.util.Collections; import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.StreamSupport; +import org.eclipse.microprofile.config.ConfigProvider; import org.jboss.logging.Logger; import io.quarkus.container.spi.ContainerImageBuildRequestBuildItem; @@ -15,6 +18,7 @@ import io.quarkus.container.spi.ContainerImagePushRequestBuildItem; import io.quarkus.container.spi.FallbackContainerImageRegistryBuildItem; import io.quarkus.container.spi.ImageReference; +import io.quarkus.container.spi.SingleSegmentContainerImageRequestBuildItem; import io.quarkus.deployment.Capabilities; import io.quarkus.deployment.annotations.BuildProducer; import io.quarkus.deployment.annotations.BuildStep; @@ -22,10 +26,10 @@ import io.quarkus.deployment.builditem.SuppressNonRuntimeConfigChangedWarningBuildItem; import io.quarkus.deployment.pkg.builditem.ArtifactResultBuildItem; import io.quarkus.deployment.pkg.steps.NativeSourcesBuild; +import io.quarkus.runtime.util.StringUtil; public class ContainerImageProcessor { - private static final String UNKNOWN_USER = "?"; private static final Logger log = Logger.getLogger(ContainerImageProcessor.class); @BuildStep(onlyIf = NativeSourcesBuild.class) @@ -48,6 +52,7 @@ public void ignoreCredentialsChange(BuildProducer singleSegmentImageRequest, Optional containerImageRegistry, Optional containerImageCustomName, Capabilities capabilities, @@ -55,7 +60,8 @@ public void publishImageInfo(ApplicationInfoBuildItem app, ensureSingleContainerImageExtension(capabilities); - // additionalTags are used even containerImageConfig.image is set because that string cannot contain multiple tags + // additionalTags are used even containerImageConfig.image is set because that + // string cannot contain multiple tags if (containerImageConfig.additionalTags.isPresent()) { for (String additionalTag : containerImageConfig.additionalTags.get()) { if (!ImageReference.isValidTag(additionalTag)) { @@ -65,35 +71,37 @@ public void publishImageInfo(ApplicationInfoBuildItem app, } } - String effectiveGroup = containerImageConfig.getEffectiveGroup().orElse(""); - //This can be the case when running inside kubernetes/minikube in dev-mode. Instead of failing, we should just catch and log. - if (UNKNOWN_USER.equals(effectiveGroup)) { - log.warn( - "Can't get the current user name, which is used as default for container image group. Can't publish container image info."); - return; - } + Optional effectiveGroup = getEffectiveGroup(containerImageConfig.group, singleSegmentImageRequest.isPresent()); // if the user supplied the entire image string, use it if (containerImageConfig.image.isPresent()) { ImageReference imageReference = ImageReference.parse(containerImageConfig.image.get()); String repository = imageReference.getRepository(); - containerImage.produce(new ContainerImageInfoBuildItem(Optional.of(imageReference.getRegistry()), repository, + if (singleSegmentImageRequest.isPresent() && imageReference.getRepository().contains("/") + && StringUtil.isNullOrEmpty(imageReference.getRegistry())) { + log.warn("A single segment image is preferred, but a local multi segment has been provided: " + + containerImageConfig.image.get()); + } + containerImage.produce(new ContainerImageInfoBuildItem(Optional.of(imageReference.getRegistry()), + repository, imageReference.getTag(), containerImageConfig.additionalTags.orElse(Collections.emptyList()))); return; } String registry = containerImageConfig.registry - .orElseGet(() -> containerImageRegistry.map(FallbackContainerImageRegistryBuildItem::getRegistry).orElse(null)); + .orElseGet(() -> containerImageRegistry.map(FallbackContainerImageRegistryBuildItem::getRegistry) + .orElse(null)); if ((registry != null) && !ImageReference.isValidRegistry(registry)) { throw new IllegalArgumentException("The supplied container-image registry '" + registry + "' is invalid"); } String effectiveName = containerImageCustomName.map(ContainerImageCustomNameBuildItem::getName) .orElse(containerImageConfig.name); - String repository = (containerImageConfig.getEffectiveGroup().map(s -> s + "/").orElse("")) + effectiveName; + String group = effectiveGroup.orElse(""); + String repository = group.isBlank() ? effectiveName : group + "/" + effectiveName; if (!ImageReference.isValidRepository(repository)) { throw new IllegalArgumentException("The supplied combination of container-image group '" - + effectiveGroup + "' and name '" + effectiveName + "' is invalid"); + + group + "' and name '" + effectiveName + "' is invalid"); } String effectiveTag = containerImageConfig.tag.orElse(app.getVersion()); @@ -106,7 +114,7 @@ public void publishImageInfo(ApplicationInfoBuildItem app, } containerImage.produce(new ContainerImageInfoBuildItem(Optional.ofNullable(registry), - containerImageConfig.getEffectiveGroup(), + effectiveGroup, effectiveName, effectiveTag, containerImageConfig.additionalTags.orElse(Collections.emptyList()))); } @@ -114,4 +122,35 @@ public void publishImageInfo(ApplicationInfoBuildItem app, private void ensureSingleContainerImageExtension(Capabilities capabilities) { ContainerImageCapabilitiesUtil.getActiveContainerImageCapability(capabilities); } + + /** + * Since user.name which is default value can be uppercase and uppercase values + * are not allowed + * in the repository part of image references, we need to make the username + * lowercase. + * If spaces exist in the user name, we replace them with the dash character. + * + * We purposely don't change the value of an explicitly set group. + */ + static Optional getEffectiveGroup(Optional group, boolean isSingleSegmentRequested) { + return group.or(() -> isSingleSegmentRequested || isGroupSpecified() + ? Optional.empty() + : Optional.ofNullable(System.getProperty("user.name")).map(s -> s.replace(' ', '-'))) + .map(String::toLowerCase) + .filter(Predicate.not(StringUtil::isNullOrEmpty)); + } + + static Optional getEffectiveGroup() { + return getEffectiveGroup(Optional.empty(), false); + } + + /** + * Users are allowed to specify an empty group, however this is mapped to Optional.emtpy(). + * We need to know if the user has actually specified a group or not. + * The only way is to check the property names provided. + **/ + static boolean isGroupSpecified() { + return StreamSupport.stream(ConfigProvider.getConfig().getPropertyNames().spliterator(), false) + .anyMatch(n -> n.equals("quarkus.container-image.group") || n.equals("QUARKUS_CONTAINER_IMAGE_GROUP")); + } } diff --git a/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageConfigEffectiveGroupTest.java b/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageConfigEffectiveGroupTest.java index d319ae11875f3..b7921ee7f4c6b 100644 --- a/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageConfigEffectiveGroupTest.java +++ b/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageConfigEffectiveGroupTest.java @@ -9,38 +9,37 @@ class ContainerImageConfigEffectiveGroupTest { public static final String USER_NAME_SYSTEM_PROPERTY = "user.name"; - - private ContainerImageConfig sut = new ContainerImageConfig(); + private final Optional EMPTY = Optional.empty(); @Test void testFromLowercaseUsername() { testWithNewUsername("test", () -> { - sut.group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY)); - assertEquals(sut.getEffectiveGroup(), Optional.of("test")); + Optional group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY)); + assertEquals(ContainerImageProcessor.getEffectiveGroup(EMPTY, false), Optional.of("test")); }); } @Test void testFromUppercaseUsername() { testWithNewUsername("TEST", () -> { - sut.group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY)); - assertEquals(sut.getEffectiveGroup(), Optional.of("test")); + Optional group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY)); + assertEquals(ContainerImageProcessor.getEffectiveGroup(EMPTY, false), Optional.of("test")); }); } @Test void testFromSpaceInUsername() { testWithNewUsername("user name", () -> { - sut.group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY)); - assertEquals(sut.getEffectiveGroup(), Optional.of("user-name")); + Optional group = Optional.of(System.getProperty(USER_NAME_SYSTEM_PROPERTY)); + assertEquals(ContainerImageProcessor.getEffectiveGroup(EMPTY, false), Optional.of("user-name")); }); } @Test void testFromOther() { testWithNewUsername("WhateveR", () -> { - sut.group = Optional.of("OtheR"); - assertEquals(sut.getEffectiveGroup(), Optional.of("OtheR")); + Optional group = Optional.of("OtheR"); + assertEquals(ContainerImageProcessor.getEffectiveGroup(group, false), Optional.of("other")); }); } diff --git a/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageInfoTest.java b/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageInfoTest.java index 0a702f198d631..a864a948fb3fc 100644 --- a/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageInfoTest.java +++ b/extensions/container-image/deployment/src/test/java/io/quarkus/container/image/deployment/ContainerImageInfoTest.java @@ -122,7 +122,8 @@ private void whenPublishImageInfo() { Capabilities capabilities = new Capabilities(Collections.emptySet()); BuildProducer containerImage = actualImageConfig -> actualContainerImageInfo = actualImageConfig; ContainerImageProcessor processor = new ContainerImageProcessor(); - processor.publishImageInfo(app, containerImageConfig, Optional.empty(), Optional.empty(), capabilities, containerImage); + processor.publishImageInfo(app, containerImageConfig, Optional.empty(), Optional.empty(), Optional.empty(), + capabilities, containerImage); } private void thenImageIs(String expectedImage) { diff --git a/extensions/container-image/spi/src/main/java/io/quarkus/container/spi/SingleSegmentContainerImageRequestBuildItem.java b/extensions/container-image/spi/src/main/java/io/quarkus/container/spi/SingleSegmentContainerImageRequestBuildItem.java new file mode 100644 index 0000000000000..507d794d05d20 --- /dev/null +++ b/extensions/container-image/spi/src/main/java/io/quarkus/container/spi/SingleSegmentContainerImageRequestBuildItem.java @@ -0,0 +1,10 @@ +package io.quarkus.container.spi; + +import io.quarkus.builder.item.SimpleBuildItem; + +/** + * There are cases where a single segment image (an image without group) is preferred. + * This build item is used to express this preferrence. + **/ +public final class SingleSegmentContainerImageRequestBuildItem extends SimpleBuildItem { +} diff --git a/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java b/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java index ab3f4f5a3fb7d..148ba29a4548f 100644 --- a/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java +++ b/extensions/kubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/OpenshiftProcessor.java @@ -43,6 +43,7 @@ import io.quarkus.container.spi.ContainerImageInfoBuildItem; import io.quarkus.container.spi.ContainerImageLabelBuildItem; import io.quarkus.container.spi.FallbackContainerImageRegistryBuildItem; +import io.quarkus.container.spi.SingleSegmentContainerImageRequestBuildItem; import io.quarkus.deployment.Capabilities; import io.quarkus.deployment.Capability; import io.quarkus.deployment.annotations.BuildProducer; @@ -109,13 +110,15 @@ public void populateCustomImageName(OpenshiftConfig openshiftConfig, @BuildStep public void populateInternalRegistry(OpenshiftConfig openshiftConfig, ContainerImageConfig containerImageConfig, Capabilities capabilities, - BuildProducer containerImageRegistry) { + BuildProducer containerImageRegistry, + BuildProducer singleSegmentContainerImageRequest) { if (!containerImageConfig.registry.isPresent() && !containerImageConfig.image.isPresent()) { DeploymentResourceKind deploymentResourceKind = openshiftConfig.getDeploymentResourceKind(capabilities); if (deploymentResourceKind != DeploymentResourceKind.DeploymentConfig) { if (openshiftConfig.isOpenshiftBuildEnabled(containerImageConfig, capabilities)) { //Don't need fallback namespace, we use local lookup instead. + singleSegmentContainerImageRequest.produce(new SingleSegmentContainerImageRequestBuildItem()); } else { containerImageRegistry.produce(new FallbackContainerImageRegistryBuildItem(DOCKERIO_REGISTRY)); } diff --git a/integration-tests/kubernetes/quarkus-standard-way/src/test/java/io/quarkus/it/kubernetes/OpenshiftWithDeploymentResourceAndLocalLookupTest.java b/integration-tests/kubernetes/quarkus-standard-way/src/test/java/io/quarkus/it/kubernetes/OpenshiftWithDeploymentResourceAndLocalLookupTest.java new file mode 100644 index 0000000000000..ffbb34a7cd4e6 --- /dev/null +++ b/integration-tests/kubernetes/quarkus-standard-way/src/test/java/io/quarkus/it/kubernetes/OpenshiftWithDeploymentResourceAndLocalLookupTest.java @@ -0,0 +1,99 @@ + +package io.quarkus.it.kubernetes; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.entry; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.fabric8.openshift.api.model.ImageStream; +import io.quarkus.builder.Version; +import io.quarkus.maven.dependency.Dependency; +import io.quarkus.test.ProdBuildResults; +import io.quarkus.test.ProdModeTestResults; +import io.quarkus.test.QuarkusProdModeTest; + +// +// The purpose of this test is to assert that +// When: +// - We run an in-cluster container builds targeting Openshift (and `Deployment` is used). +// - No group / image is explicitly specified +// +// Then: +// - A BuildConfg is generated. +// - Two ImageStream are generated (one named after the app). +// - A Deployment resource was created +// - local lookup is enabled +// - single segement image is requested +// +public class OpenshiftWithDeploymentResourceAndLocalLookupTest { + + private static final String NAME = "openshift-with-deployment-resource-and-local-lookup"; + + @RegisterExtension + static final QuarkusProdModeTest config = new QuarkusProdModeTest() + .withApplicationRoot((jar) -> jar.addClasses(GreetingResource.class)) + .setApplicationName(NAME) + .setApplicationVersion("0.1-SNAPSHOT") + .overrideConfigKey("quarkus.openshift.deployment-kind", "Deployment") + .setLogFileName("k8s.log") + .setForcedDependencies(List.of(Dependency.of("io.quarkus", "quarkus-openshift", Version.getVersion()))); + + @ProdBuildResults + private ProdModeTestResults prodModeTestResults; + + @Test + public void assertGeneratedResources() throws IOException { + final Path kubernetesDir = prodModeTestResults.getBuildDir().resolve("kubernetes"); + assertThat(kubernetesDir) + .isDirectoryContaining(p -> p.getFileName().endsWith("openshift.json")) + .isDirectoryContaining(p -> p.getFileName().endsWith("openshift.yml")); + List kubernetesList = DeserializationUtil + .deserializeAsList(kubernetesDir.resolve("openshift.yml")); + + assertThat(kubernetesList).filteredOn(h -> "BuildConfig".equals(h.getKind())).hasSize(1); + assertThat(kubernetesList).filteredOn(h -> "ImageStream".equals(h.getKind())).hasSize(2); + assertThat(kubernetesList).filteredOn(h -> "ImageStream".equals(h.getKind()) + && h.getMetadata().getName().equals(NAME)).hasSize(1); + + assertThat(kubernetesList).filteredOn(i -> i instanceof Deployment).singleElement().satisfies(i -> { + assertThat(i).isInstanceOfSatisfying(Deployment.class, d -> { + assertThat(d.getMetadata()).satisfies(m -> { + assertThat(m.getName()).isEqualTo(NAME); + }); + + assertThat(d.getSpec()).satisfies(deploymentSpec -> { + assertThat(deploymentSpec.getTemplate()).satisfies(t -> { + assertThat(t.getMetadata()).satisfies(metadata -> assertThat(metadata.getLabels()).containsAnyOf( + entry("app.kubernetes.io/name", NAME), + entry("app.kubernetes.io/version", "0.1-SNAPSHOT"))); + + assertThat(t.getSpec()).satisfies(podSpec -> { + assertThat(podSpec.getContainers()).singleElement().satisfies(container -> { + assertThat(container.getImage()) + .isEqualTo("openshift-with-deployment-resource-and-local-lookup:0.1-SNAPSHOT"); + }); + }); + }); + }); + }); + }); + + assertThat(kubernetesList).filteredOn(r -> r instanceof ImageStream && r.getMetadata().getName().equals(NAME)) + .singleElement().satisfies(r -> { + assertThat(r).isInstanceOfSatisfying(ImageStream.class, i -> { + assertThat(i.getSpec()).satisfies(spec -> { + assertThat(spec.getLookupPolicy().getLocal()).isEqualTo(true); + }); + }); + }); + + } +}