fix(#7712): S2i build no longer renames artifacts #7749

iocanel · 2020-03-10T19:03:29Z

Fixes: #7712 #7766 #7740

This pull request changes the way that the artifact is handled during the s2i build process.

The artifact is no longer renamed.
The user is able to specify the target directory and or the target filename (if the s2i builder image changes the name).
The artifact name that is used in the manifests comes from JarBuildItem / NativeBuildItem unless the user specifies something else (see above).

iocanel · 2020-03-10T19:04:00Z

CC @Ladicek @maxandersen

iocanel · 2020-03-10T19:22:17Z

Found some issues with the PR related to native mode ...
I am on it.

It should be ok now.

maxandersen · 2020-03-11T06:19:32Z

as this changes the behavior of kubernetes/openshift setup best to get this fixed before final.

...age-s2i/deployment/src/main/java/io/quarkus/container/image/s2i/deployment/S2iProcessor.java

geoand · 2020-03-11T07:14:08Z

I would like @Ladicek to test this if possible.

I agree with @maxandersen that we need this in for Final from a usability perspective. Moreover it changes the s2i configuration which we really want to do before the Final is out to avoid unnecessary breaking changes down the line

Ladicek · 2020-03-11T07:28:37Z

/me looking

Ladicek

This seems to work fine from my perspective. I don't test native binaries in OpenShift yet, but for the JVM mode, this is OK.

(I'm still not sure why we need the full java ... cmdline, but it can't hurt either, so I'm fine with that.)

geoand · 2020-03-11T10:12:15Z

Thanks for checking @Ladicek

geoand

Added some minor comments about the config property javadoc (which should be fixed since they end up in the docs)

...-image-s2i/deployment/src/main/java/io/quarkus/container/image/s2i/deployment/S2iConfig.java

iocanel · 2020-03-11T10:57:25Z

This seems to work fine from my perspective. I don't test native binaries in OpenShift yet, but for the JVM mode, this is OK.

It's the ubi quarkus native binary image that does rename the artifact. It doesn't affect JVM mode.

(I'm still not sure why we need the full java ... cmdline, but it can't hurt either, so I'm fine with that.)

Because there is no guarantee that the s2i builder image the user selects will support JAVA_APP_JAR etc.

geoand · 2020-03-11T11:04:12Z

Please squash the commits when you are done :)

iocanel · 2020-03-11T11:04:44Z

Please squash the commits when you are done :)

Done!

geoand

LGTM

Ladicek · 2020-03-11T11:14:29Z

(I'm still not sure why we need the full java ... cmdline, but it can't hurt either, so I'm fine with that.)

Because there is no guarantee that the s2i builder image the user selects will support JAVA_APP_JAR etc.

I have actually noticed that it might hurt, or at least be suboptimal. I filed #7766 for that.

gsmet · 2020-03-11T12:31:14Z

@geoand please don't assign PRs to 1.3.0.Final. I do it when I backport things.

As long as they are not backported, they don't end up in 1.3.0.Final as we already have a branch.

Thanks!

geoand · 2020-03-11T12:33:31Z

So the milestone should be 1.4.0.Final?

iocanel · 2020-03-11T20:16:42Z

Added a couple more commits that fixes: #7766 and #7740.
Also it changes the default s2i java base image to openjdk.

iocanel · 2020-03-12T13:07:12Z

This case still causes duplicate entry in the YAML file:

As this is a non-blocker issue, I think that the fix for that particular uses case should be deferred post 1.3.0.Final.

geoand · 2020-03-12T13:15:04Z

This case still causes duplicate entry in the YAML file:

As this is a non-blocker issue, I think that the fix for that particular uses case should be deferred post 1.3.0.Final.

I agree

Ladicek · 2020-03-12T13:31:19Z

Yea, there's no way how to indicate priority in GitHub issues (except labels, but I can't assign those). If it were, I'd mark that particular issue as not a blocker -- but since you already tried to fix it, I wanted to report that the fix is incomplete :-)

Ladicek · 2020-03-12T13:33:41Z

Ah, one thing. This doesn't mean I didn't find any other issue. I'm still checking. This one was just too easy to find :-)

geoand · 2020-03-12T13:37:23Z

@Ladicek I propose we merge now and if you find any issues open them along with an indication of how serious they are for .Final

Ladicek · 2020-03-12T13:56:47Z

I agree with merging (as I didn't spot any other glaring issue yet), but would like to have these 2 questions answered first:

Can we use the registry.access.redhat.com/openjdk/openjdk-11-rhel7 image by default?
Can the fact that "known image" matching algorithm ignores the registry name have security consequences? Or any other consequences? I can only think of the possibility to abuse that by creating identically named image in an internal registry. But I'd like other folks to think about that too, at least a little.

iocanel · 2020-03-12T14:07:59Z

I agree with merging (as I didn't spot any other glaring issue yet), but would like to have these 2 questions answered first:

Can we use the registry.access.redhat.com/openjdk/openjdk-11-rhel7 image by default?

This is already the image we use. If your question is if we should, then maybe @maxandersen can answer that.

Can the fact that "known image" matching algorithm ignores the registry name have security consequences? Or any other consequences? I can only think of the possibility to abuse that by creating identically named image in an internal registry. But I'd like other folks to think about that too, at least a little.

Let me start by saying that we can't take the registry into consideration, as this would break all private registries. Now, let's see what the implications might be:

Security wise, nothing changes. We don't grant any special privileges to known images, we just align our generated manifests (something anyone can do by hand anyway).

Other consequences?

Yeah, if someone creates an identically named image that doesn't respect the same environment variable conventions as the original one, publish that to a private, and use that instead, then the resulting container will probably fail to start. But I think we can live with that (at least for now).

Ladicek · 2020-03-12T14:16:58Z

I agree with merging (as I didn't spot any other glaring issue yet), but would like to have these 2 questions answered first:

Can we use the registry.access.redhat.com/openjdk/openjdk-11-rhel7 image by default?

This is already the image we use.

No we don't. We currently use the Fabric8 image by default. This PR changes that to the Red Hat image. I'm not sure if that's right. Technically, the image is available for everyone. However, I think that "conditions apply". If you don't have a Red Hat subscription (even the $0 developer subscription), you shouldn't use it. Or something like that.

Can the fact that "known image" matching algorithm ignores the registry name have security consequences? Or any other consequences? I can only think of the possibility to abuse that by creating identically named image in an internal registry. But I'd like other folks to think about that too, at least a little.

Let me start by saying that we can't take the registry into consideration, as this would break all private registries. Now, let's see what the implications might be:

Security wise, nothing changes. We don't grant any special privileges to known images, we just align our generated manifests (something anyone can do by hand anyway).

Other consequences?

Yeah, if someone creates an identically named image that doesn't respect the same environment variable conventions as the original one, publish that to a private, and use that instead, then the resulting container will probably fail to start. But I think we can live with that (at least for now).

OK, agree.

iocanel · 2020-03-12T14:19:12Z

No we don't. We currently use the Fabric8 image by default. This PR changes that to the Red Hat image.

I stand corrected!

geoand · 2020-03-12T14:27:49Z

No we don't. We currently use the Fabric8 image by default. This PR changes that to the Red Hat image.

I stand corrected!

Which leaves us where with regards to @Ladicek 's comment?

iocanel · 2020-03-12T14:38:15Z

No we don't. We currently use the Fabric8 image by default. This PR changes that to the Red Hat image.

I stand corrected!

Which leaves us where with regards to @Ladicek 's comment?

We need to check if we can use registry.access.redhat.com/openjdk/openjdk-11-rhel7 as a default image. It seems that @maxandersen is swarmed though. @tqvarnst since we discussed about this on Monday, are there any legal obstacles in proceeding with this change (adopting openjdk-11-rhel7 as the default s2i base image)?

Ladicek · 2020-03-12T14:38:15Z

I don't like us using the Fabric8 image, but I don't think there's a better image that we can use by default. When UBI image with Java comes out, we should switch to that for sure, but until then...

gsmet · 2020-03-12T14:42:08Z

For the default Dockerfiles, we use ubi and install Java manually.

geoand · 2020-03-12T14:43:03Z

We can't do that for s2i however

maxandersen · 2020-03-12T14:51:46Z

long thread and not sure what questions are left but just two things to state:

we are stuck with fabric8 image until ubi-java comes out - for customers they'll need to explicitly set whatever supported image is needed.
we are not going to use any image that require auth as default.

Ladicek · 2020-03-12T16:49:46Z

...age-s2i/deployment/src/main/java/io/quarkus/container/image/s2i/deployment/S2iProcessor.java

-                .map(d -> Paths.get(s2iConfig.jarPath).getParent().resolve("lib")
-                        .resolve(d.getArtifact().getGroupId() + "." + d.getArtifact().getPath().getFileName()).toAbsolutePath()
+                .map(d -> String.valueOf(d.getArtifact().getGroupId() + "." + d.getArtifact().getPath().getFileName()))
+                .map(s -> Paths.get(s2iConfig.jarDirectory).resolve("lib").resolve(s).toAbsolutePath()


I wonder what happens on Windows, would this possibly generate Windows-style paths into the Kubernetes YAML? (Didn't check, just thinking aloud.)

Ladicek

This works fine for my usecase. There might be follow-up issues, but this is a good base IMHO.

geoand · 2020-03-12T16:59:50Z

Since we are practically out of time, I'm going to merge this to get the needed fixes into Final

boring-cyborg bot added the area/kubernetes label Mar 10, 2020

iocanel requested a review from geoand March 10, 2020 19:03

iocanel force-pushed the s2i-jar-path branch from 34aa591 to 8ed7832 Compare March 10, 2020 19:05

iocanel changed the title ~~fix(#7712): S2i build no longer renames artifacts~~ [WIP] fix(#7712): S2i build no longer renames artifacts Mar 10, 2020

iocanel force-pushed the s2i-jar-path branch from 8ed7832 to 2a5afba Compare March 10, 2020 20:11

iocanel changed the title ~~[WIP] fix(#7712): S2i build no longer renames artifacts~~ fix(#7712): S2i build no longer renames artifacts Mar 10, 2020

gsmet added the triage/backport? label Mar 10, 2020

maxandersen added the priority/blocker label Mar 11, 2020

maxandersen reviewed Mar 11, 2020

View reviewed changes

...age-s2i/deployment/src/main/java/io/quarkus/container/image/s2i/deployment/S2iProcessor.java Show resolved Hide resolved

Ladicek approved these changes Mar 11, 2020

View reviewed changes

geoand reviewed Mar 11, 2020

View reviewed changes

iocanel force-pushed the s2i-jar-path branch from e781b1d to f82472d Compare March 11, 2020 11:04

geoand approved these changes Mar 11, 2020

View reviewed changes

geoand added this to the 1.3.0.Final milestone Mar 11, 2020

gsmet modified the milestones: 1.3.0.Final, 1.4.0 Mar 11, 2020

Ladicek mentioned this pull request Mar 11, 2020

The native image binary should retain its original name quarkusio/quarkus-images#58

Closed

iocanel added 5 commits March 12, 2020 17:25

fix(quarkusio#7712): S2i build no longer renames artifacts

c224346

fix: wrong value passed to working dir decorator

0f991bd

fix: Work around duplicating environment variables

fccaaeb

feat: Know s2i base images now leverage env vars.

55b9d84

chore: minor s2i & kubernetes improvements

66dd28c

iocanel force-pushed the s2i-jar-path branch from 7e7d652 to 66dd28c Compare March 12, 2020 15:25

Ladicek reviewed Mar 12, 2020

View reviewed changes

Ladicek approved these changes Mar 12, 2020

View reviewed changes

geoand merged commit f02e1c0 into quarkusio:master Mar 12, 2020

This was referenced Mar 12, 2020

quarkus-container-image-s2i bypasses the S2I run script, resulting in environmental differences #7766

Closed

Kubernetes extension generates environment variables twice #7740

Closed

gsmet removed the triage/backport? label Mar 12, 2020

gsmet modified the milestones: 1.4.0, 1.3.0.Final Mar 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#7712): S2i build no longer renames artifacts #7749

fix(#7712): S2i build no longer renames artifacts #7749

iocanel commented Mar 10, 2020 •

edited

iocanel commented Mar 10, 2020

iocanel commented Mar 10, 2020 •

edited

maxandersen commented Mar 11, 2020

geoand commented Mar 11, 2020

Ladicek commented Mar 11, 2020

Ladicek left a comment

geoand commented Mar 11, 2020

geoand left a comment

iocanel commented Mar 11, 2020

geoand commented Mar 11, 2020

iocanel commented Mar 11, 2020 •

edited

geoand left a comment

Ladicek commented Mar 11, 2020

gsmet commented Mar 11, 2020

geoand commented Mar 11, 2020

iocanel commented Mar 11, 2020

iocanel commented Mar 12, 2020

geoand commented Mar 12, 2020

Ladicek commented Mar 12, 2020

Ladicek commented Mar 12, 2020

geoand commented Mar 12, 2020

Ladicek commented Mar 12, 2020

iocanel commented Mar 12, 2020

Ladicek commented Mar 12, 2020

iocanel commented Mar 12, 2020

geoand commented Mar 12, 2020

iocanel commented Mar 12, 2020

Ladicek commented Mar 12, 2020

gsmet commented Mar 12, 2020

geoand commented Mar 12, 2020

maxandersen commented Mar 12, 2020

Ladicek Mar 12, 2020

Ladicek left a comment

geoand commented Mar 12, 2020

fix(#7712): S2i build no longer renames artifacts #7749

fix(#7712): S2i build no longer renames artifacts #7749

Conversation

iocanel commented Mar 10, 2020 • edited

iocanel commented Mar 10, 2020

iocanel commented Mar 10, 2020 • edited

maxandersen commented Mar 11, 2020

geoand commented Mar 11, 2020

Ladicek commented Mar 11, 2020

Ladicek left a comment

Choose a reason for hiding this comment

geoand commented Mar 11, 2020

geoand left a comment

Choose a reason for hiding this comment

iocanel commented Mar 11, 2020

geoand commented Mar 11, 2020

iocanel commented Mar 11, 2020 • edited

geoand left a comment

Choose a reason for hiding this comment

Ladicek commented Mar 11, 2020

gsmet commented Mar 11, 2020

geoand commented Mar 11, 2020

iocanel commented Mar 11, 2020

iocanel commented Mar 12, 2020

geoand commented Mar 12, 2020

Ladicek commented Mar 12, 2020

Ladicek commented Mar 12, 2020

geoand commented Mar 12, 2020

Ladicek commented Mar 12, 2020

iocanel commented Mar 12, 2020

Ladicek commented Mar 12, 2020

iocanel commented Mar 12, 2020

geoand commented Mar 12, 2020

iocanel commented Mar 12, 2020

Ladicek commented Mar 12, 2020

gsmet commented Mar 12, 2020

geoand commented Mar 12, 2020

maxandersen commented Mar 12, 2020

Ladicek Mar 12, 2020

Choose a reason for hiding this comment

Ladicek left a comment

Choose a reason for hiding this comment

geoand commented Mar 12, 2020

iocanel commented Mar 10, 2020 •

edited

iocanel commented Mar 10, 2020 •

edited

iocanel commented Mar 11, 2020 •

edited