search

fabric8io-images / s2i

OpenShift S2I images for Java and Karaf applications
Apache License 2.0
70 stars 84 forks source link

problem with GRADLE_OPTS containing "|" (pipeline character) #261

Open 62mkv opened 3 years ago

62mkv commented 3 years ago

Hi all. I assume this is the source repository for images, found at https://hub.docker.com/r/fabric8/s2i-java/

I've just spent some time trying to understand the problem I have, when I run builder images based on "*-java11" versions of images. (NB: I did not try with latest -java8, they might be also susceptible)

Anyway, when our private OpenShift 3.11 installation starts running an "s2i-java" build, it's being given GRADLE_OPTS variable (see below)

==================================================================
Starting S2I Java Build .....
S2I source build for Gradle detected, due to presence of a *.gradle* in /tmp/src
Using GRADLE_OPTS '-Dhttps.proxyHost=proxy.my.corp -Dhttps.proxyPort=80 -Dhttp.proxyHost=proxy.my.corp -Dhttp.proxyPort=80 -Dhttp.nonProxyHosts="*.cluster.local|*.my.corp|*.svc|127.0.0.1|169.254.169.254|172.28.0.1|cp-ham-internal.my.corp|localhost" -XX:+UseParallelGC -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MinHeapFreeRatio=20 -XX:MaxHeapFreeRatio=40 -XX:+ExitOnOutOfMemoryError'
Running './gradlew --no-daemon build -x test '

Builder image, that used to be "s2i-java:latest" (digest a1688dbedce5f19bfe99be506a555676a0280f5de8ae5ee9ee1a0043d306b545) works fine with such variable, just as the one tagged 3.0-java11. But both 3.1-java11 and latest-java11 fail utterly, like this: 

Running './gradlew --no-daemon build -x test '
./gradlew: line 181: *.my.corp: command not found
./gradlew: line 181: *.svc: command not found
./gradlew: line 181: 127.0.0.1: command not found
./gradlew: line 181: 169.254.169.254: command not found
./gradlew: line 181: 172.28.0.1: command not found
... (to be continued)

the only difference between working and failing configuration is the builder image tag (3.0-java11 works, some ancient *-java8 works, 3.1-java11 and latest-java11 fail)

I hope this can be fixed... or if there're any suggestions how to override that (I guess I could implement some fixes in the "run" script, by putting one into my source code; but so far has no idea how to go about)

thanks!

rhuss commented 3 years ago

The issue is that the pipe symbol is not escaped. This is probably fixed in one version of the image but not in the other one. You can try to unset the nonProxyHost variable if this is possible for you. I guess that 3.0 just did not pick up nonProxyHost env var, so that it does not fail.

rhuss commented 3 years ago

Can you try to use \| instead of | in the GRADLE_OPTS variable ?

Sorry, I can't help much more here, as I'm not working on this project anymore (and don't really know much about the Gradle support)

62mkv commented 3 years ago

thanks @rhuss I have no idea yet, how those environment variables are composed, tbh (I just see that GRADLE_OPTS value in the build log when I trigger the Build Configuration on our OpenShift cluster). Probably it's the way this value (NO_PROXY) is being set in the VM that is running the pod. It's not set explicitly in the Build Configuration environment. Will try to sort it out with our OpenShift team. Thanks!

62mkv commented 3 years ago

had a closer look at the code, and this seems to be somehow related: https://github.com/fabric8io-images/run-java-sh/commit/1bfb36459b8131d50d554c43f09285c6a986c9ca

probably, after that change, pipe character began breaking stuff. Also, looking at the test provided, it seems like it should accept unescaped pipe character in a NO_PROXY env variable? maybe @valdar could chime in? Should I raise an issue in https://github.com/fabric8io-images/run-java-sh/ ?

rhuss commented 3 years ago

ah, yes: https://github.com/fabric8io-images/s2i/blob/2e00b33e33dc1479f568e9f120f9602f9dc3080d/java/templates/s2i/assemble#L213-L220 (run-java.sh is evaluated by s2i's assemble.sh)

So I guess its a matter to make a release that leverages the fixed version of run-java.sh ...

62mkv commented 3 years ago

Actually this particular change seems to be included in s2i: https://github.com/fabric8io-images/s2i/blob/2e00b33e33dc1479f568e9f120f9602f9dc3080d/java/images/centos-java11/run-java.sh#L508

so I suspect it is actually the reason for observed problems with the "latest" images

62mkv commented 3 years ago

this comment from you @rhuss looks ... encouraging :) https://github.com/fabric8io-images/run-java-sh/pull/85#pullrequestreview-209021316

valdar commented 3 years ago

@62mkv I am not quite sure how the changes in handling NO_PROXY and http.nonProxyHosts env vars can interact with GRADL_OPTS env var...

What I remember from the issue is that the intended way (as the only scenario tested in tests) is to have quotes (") removed around the value of http.nonProxyHosts= jvm var. It seems from your initial log that you are missing this fix https://github.com/fabric8io-images/run-java-sh/pull/85/commits/52b35013dfe699fbe1f0d7af096c9494e315c322 in particular the part removing the "..." around ret=?

62mkv commented 3 years ago

I don't provide the value of GRADLE_OPTS! run-java.sh does it, based on whatever env variables are set on the host that runs the build. The problem is : 3.0 image works fine with exactly the same environment variables values. Only 3.1 and latest are suffering.

Sent from Mail.ru app for Android Tuesday, 08 June 2021, 05:26pm +03:00 from Andrea Tarocchi @.*** :

Actually double checking your output it seems you provide the value of GRADLE_OPTS as-Dhttps.proxyHost=proxy.my.corp -Dhttps.proxyPort=80 -Dhttp.proxyHost=proxy.my.corp -Dhttp.proxyPort=80 -Dhttp.nonProxyHosts=".cluster.local|.my.corp|.svc|127.0.0.1|169.254.169.254|172.28.0.1|cp-ham-internal.my.corp|localhost" -XX:+UseParallelGC -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MinHeapFreeRatio=20 -XX:MaxHeapFreeRatio=40 -XX:+ExitOnOutOfMemoryError try to se it to -Dhttps.proxyHost=proxy.my.corp -Dhttps.proxyPort=80 -Dhttp.proxyHost=proxy.my.corp -Dhttp.proxyPort=80 -Dhttp.nonProxyHosts=.cluster.local|.my.corp|.svc|127.0.0.1|169.254.169.254|172.28.0.1|cp-ham-internal.my.corp|localhost -XX:+UseParallelGC -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MinHeapFreeRatio=20 -XX:MaxHeapFreeRatio=40 -XX:+ExitOnOutOfMemoryError i.e. same stuff but with the removed "..." around http.nonProxyHosts value. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or unsubscribe .

valdar commented 3 years ago

Yep @62mkv see my prev comment https://github.com/fabric8io-images/s2i/issues/261#issuecomment-856800929

62mkv commented 3 years ago

You mean the one "I am not quite sure..."? Well my understanding is that run-java.sh is used to create GRADLE_OPTS (if it is not set) based on HTTP_PROXY, NO_PROXY, etc. What I am trying to tell is that for exactly same values of those variables, the builder image used to work fine. But it does not, when it's builder image version 3.1 or latest For now, I just define GRADLE_OPTS manually and not include any pipe characters at all, but that's a hack I would like to avoid. If you say "oh, then your cluster admins should not define those values then", then it's not a solution because those values are there for a reason. They are defined cluster wide and surely no one will change them just for myself. That's why I think it makes sense to address this on the builder image level

Sent from Mail.ru app for Android Tuesday, 08 June 2021, 06:28pm +03:00 from Andrea Tarocchi @.*** :

Yep @62mkv see my prev comment #261 (comment) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or unsubscribe .

valdar commented 3 years ago

@62mkv can you do this test for me? Instead of removing pipes character try to se GRADEL_OPTS to -Dhttps.proxyHost=proxy.my.corp -Dhttps.proxyPort=80 -Dhttp.proxyHost=proxy.my.corp -Dhttp.proxyPort=80 -Dhttp.nonProxyHosts=*.cluster.local|*.my.corp|*.svc|127.0.0.1|169.254.169.254|172.28.0.1|cp-ham-internal.my.corp|localhost -XX:+UseParallelGC -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MinHeapFreeRatio=20 -XX:MaxHeapFreeRatio=40 -XX:+ExitOnOutOfMemoryError i.e. same stuff but with the removed "..." around http.nonProxyHosts value.

62mkv commented 3 years ago

@valdar not sure I follow, it was without any quotes since the very beginning? may be you want me to run with them? I can confirm, that with the quotes around -Dhttp.nonProxyHosts value it works ok

valdar commented 3 years ago

Ah @62mkv in that case can you try escaping pipes? like setting GRADEL_OPTS to -Dhttps.proxyHost=proxy.my.corp -Dhttps.proxyPort=80 -Dhttp.proxyHost=proxy.my.corp -Dhttp.proxyPort=80 -Dhttp.nonProxyHosts=*.cluster.local\|*.my.corp\|*.svc\|127.0.0.1\|169.254.169.254\|172.28.0.1\|cp-ham-internal.my.corp\|localhost -XX:+UseParallelGC -XX:GCTimeRatio=4 -XX:AdaptiveSizePolicyWeight=90 -XX:MinHeapFreeRatio=20 -XX:MaxHeapFreeRatio=40 -XX:+ExitOnOutOfMemoryError

62mkv commented 3 years ago

hi @valdar I confirm that with escaped pipes it works too (escaped pipe character, no quotes around "value"). in other words - exactly as per your suggestion

valdar commented 3 years ago

Is that a viable approach? can you change the value of NO_PROXY env var to take in account the escaped pipes? "\|".

The problem with quotes (and the original issue my Pr solved) is that it does not work if the whole constructed command is used with exec. And that is done on a number of images based on this one...

62mkv commented 3 years ago

This is not a huge problem for me now that I know how to work it around. Concerning this issue.. you decide ) Thanks for your attention!

Sent from Mail.ru app for Android Friday, 11 June 2021, 01:35pm +03:00 from Andrea Tarocchi @.*** :

Is that a viable approach? can you change the value of NO_PROXY env var to take in account the escaped pipes? `"|". The problem with quotes (and the original issue my Pr solved) is that it does not work if the whole constructed command is used with exec. And that is done on a number of images based on this one... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or unsubscribe .

62mkv commented 3 years ago

now things get even funnier: cluster admins say "we don't use pipe characters at all, we separate values with comma". so if they are to be believed, the NO_PROXY env variable looks along the lines of

.cluster.local,.corp.org,.svc,10.61.105.10,localhost

so. probably those pipes are created by run-java.sh itself when GRADLE_OPTS is produced, so maybe there's a chance they could be escaped?

valdar commented 3 years ago

Mmmmm probably there is: https://github.com/fabric8io-images/run-java-sh/blob/v1.3.8/fish-pepper/run-java-sh/fp-files/run-java.sh#L508 basically there is a substitution "," -> "|" that could be changed to "," -> "\|" and checked the tests....