Allow to use wget in addition to curl when installing workspace agent + terminal agent #4029

benoitf · 2017-02-06T17:27:56Z

What does this PR do?

For example alpine images have natively wget tool installed (in a busybox version) but not curl by default
So, we could check that either curl or wget is installed instead of only checking if curl is installed as we only need to download binaries which wget can perform easily as well
It is speeding up the first boot of the workspaces based on this configuration. (small alpine images)

What issues does this PR fix or reference?

#4024

Changelog

Allow to use wget instead of only curl for installing ws agent or terminal agent

Release Notes

Allow to use wget instead of only curl for installing ws agent or terminal agent

Docs PR

N/A

Change-Id: I7d02fb70f5f2fb03432072f4f33ff27532c8601e
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

JamesDrummond · 2017-02-06T17:48:06Z

@benoitf Should we add wget or curl to custom recipe requirements?

benoitf · 2017-02-06T17:56:44Z

@JamesDrummond it's not a requirement : if it's not there it's installed

codenvy-ci · 2017-02-06T23:26:19Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1894/

garagatyi · 2017-02-09T09:27:53Z

I don't see pros in installing curl if scripts are adapted to work with wget. Wget is more lightweight and widespread. It is also usually pre-installed in most linux distros

garagatyi · 2017-02-09T09:28:05Z

@tolusha WDYT?

tolusha · 2017-02-09T09:32:23Z

@benoitf
Would new script be able to download agent binaries from mounted directory?

garagatyi · 2017-02-09T09:34:02Z

So I'm +1 on this PR. I also +1 on installing wget instead of curl if both are missing.
@tolusha It doesn't have to download it. It can check existence of file on local FS. I took 1 look at PR content and looks like it does exactly that.

garagatyi · 2017-02-09T09:44:48Z

I also see that all of the language servers agents (c#, json, php, python, TS) can be improved in the same way. It would be great if someone managed that.

garagatyi · 2017-02-09T09:47:15Z

@benoitf WDYT about installing wget instead of curl in case both are missing? BTW I don't see use-case when usage of curl is needed at all since I can't imagine situation when curl is present and wget is not.

benoitf · 2017-02-09T13:09:18Z

so after some investigation, my changes are working fine on http
but it fails on https :-/ so for codenvy.io it doesn't work :-(( we need for https to install wget and/or ca-certificates packages

@tolusha

@benoitf
Would new script be able to download agent binaries from mounted directory?

yes it works when agent binaries are on a mounted directory. For these one we don't use curl or wget, we use file directly

@garagatyi

@benoitf WDYT about installing wget instead of curl in case both are missing? BTW I don't see use-case when usage of curl is needed at all since I can't imagine situation when curl is present and wget is not.
yes I'm ok as well. It's just that at first I didn't want to replace curl by wget if both are missing to keep more compliant stuff with the existing. But I'm fine with that as well

garagatyi · 2017-02-09T13:52:39Z

but it fails on https :-/ so for codenvy.io it doesn't work :-(( we need for https to install wget and/or ca-certificates packages

Can you elaborate on that. I'm not really following you.

benoitf · 2017-02-10T09:44:04Z

@garagatyi

$ docker run --rm -ti alpine:3.5 sh
/ # wget http://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
Connecting to codenvy.io (52.55.17.101:443)
wget: can't execute 'ssl_helper': No such file or directory
wget: error getting response: Connection reset by peer

If we try on http there is still a 443 redirect so it fails as well

$ docker run --rm -ti alpine:3.5 sh
/ # wget http://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
Connecting to codenvy.io (52.55.17.101:80)
Connecting to codenvy.io (52.55.17.101:443)
wget: can't execute 'ssl_helper': No such file or directory
wget: error getting response: Connection reset by peer

so we need to add ca-certificates or openssl packages

$ docker run --rm -ti alpine:3.5 sh
/ # apk add --update openssl
fetch http://dl-cdn.alpinelinux.org/alpine/v3.5/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.5/community/x86_64/APKINDEX.tar.gz
(1/3) Installing libcrypto1.0 (1.0.2k-r0)
(2/3) Installing libssl1.0 (1.0.2k-r0)
(3/3) Installing openssl (1.0.2k-r0)
Executing busybox-1.25.1-r0.trigger
OK: 7 MiB in 14 packages
/ #  wget https://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
Connecting to codenvy.io (52.55.17.101:443)
websocket-terminal-l 100% |***********************************************************************************************************************************|  2761k  0:00:00 ETA

or by adding GNU wget package (not busybox) + --no-check-certificate (or also add the ca-certificates package)

$ docker run --rm -ti alpine:3.5 sh
 apk add --update wget
fetch http://dl-cdn.alpinelinux.org/alpine/v3.5/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.5/community/x86_64/APKINDEX.tar.gz
(1/1) Installing wget (1.18-r1)
Executing busybox-1.25.1-r0.trigger
OK: 4 MiB in 12 packages
/ # wget https://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
--2017-02-10 09:36:25--  https://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
Resolving codenvy.io... 52.55.17.101
Connecting to codenvy.io|52.55.17.101|:443... connected.
ERROR: cannot verify codenvy.io's certificate, issued by 'CN=Go Daddy Secure Certificate Authority - G2,OU=http://certs.godaddy.com/repository/,O=GoDaddy.com\\, Inc.,L=Scottsdale,ST=Arizona,C=US':
  Self-signed certificate encountered.
To connect to codenvy.io insecurely, use `--no-check-certificate'.
/ # wget --no-check-certificate https://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
--2017-02-10 09:36:38--  https://codenvy.io/agent-binaries/linux_amd64/terminal/websocket-terminal-linux_amd64.tar.gz
Resolving codenvy.io... 52.55.17.101
Connecting to codenvy.io|52.55.17.101|:443... connected.
WARNING: cannot verify codenvy.io's certificate, issued by 'CN=Go Daddy Secure Certificate Authority - G2,OU=http://certs.godaddy.com/repository/,O=GoDaddy.com\\, Inc.,L=Scottsdale,ST=Arizona,C=US':
  Self-signed certificate encountered.
HTTP request sent, awaiting response... 200 OK
Length: 2828045 (2.7M) [application/x-gzip]
Saving to: 'websocket-terminal-linux_amd64.tar.gz'

so

do we need that agent binaries always served over https/443 and not also on http/80 ? (agent is part of the "infra" so I would say it's in a trusted zone) --> then we could save extra packages
if not, then we should pickup an extra package for wget busybox for https url

garagatyi · 2017-02-10T13:42:45Z

Thank for the detailed explanation. Situation is clear for me now.

garagatyi · 2017-02-13T08:45:02Z

I think @riuvshin is against serving it by non secured protocol.
So IMO it is up to you to choose the way (curl or wget + certificates).
Also it can be useful to check whether protocol is http before installing certificates. We can also check if installation is needed to not perform apt-get update if it is not needed. But these are just additional features, so they are not mandatory.

benoitf · 2017-02-13T08:49:14Z

@garagatyi yes I was thinking as you, to check http protocol (http/https) before requiring extra packages and to install packages only if they're really required

@riuvshin as I understand, the access to the resources are accessed by the swarm nodes that are close to the workspace master so I'm not sure to see what are the benefits on forcing https over http for /agent-binaries/. It's a read-only operation and we don't provide any security/token details.

riuvshin · 2017-02-13T12:47:06Z

@benoitf does this pr cover using http for /agent-binaries/ or it is still something to do?

benoitf · 2017-02-13T12:52:24Z

@riuvshin It does not cover this way but it's something that could be great if you agree.

small recap:
alpine images come with wget busybox installed by default
in Che (if not mounted) and in Codenvy, agents are downloaded on haproxy /agent-binaries

for now, it is using curl. So on alpine it requires to install curl but we have wget
so the current PR fixes the fact that if wget is installed we avoid to install curl (we boot faster and limit downloads)
But for https (which is the case for codenvy.io and other ppl using codenvy with https), we have the issue that wget busybox doesn't support ssl
so :
option 1. for /agent-binaries we allow both http and https (agents will use http)
option 2. it means that on alpine images, we will have to install extra packages (like openssl, GNU wget, etc) only to download the binary while with http support no extra package is required

riuvshin · 2017-02-13T16:43:18Z

ok I've got the idea need to check if we can do this with hapoxy, i mean allow some path tobe http when https is on

benoitf · 2017-02-13T16:54:32Z

@riuvshin http://stackoverflow.com/a/36794795/4201908
http://stackoverflow.com/a/25007004/4201908

riuvshin · 2017-02-13T16:57:23Z

@benoitf here is my branch I believe this will work codenvy/codenvy@835a17b

benoitf · 2017-02-13T16:58:26Z

@riuvshin Ok I'm not sure how to test ssl install quickly

riuvshin · 2017-02-13T17:00:45Z

@benoitf that is not easy to test locally I think I can play with it on stg tomorrow

garagatyi · 2017-03-03T09:41:42Z

agents/exec/src/main/resources/org.eclipse.che.terminal.script.sh

+
+# no curl, no wget, install curl
+if [ ${CURL_INSTALLED} = false ] && [ ${WGET_INSTALLED} = false ]; then
+  { PACKAGES=${PACKAGES}" curl"; }


I think this statement can be simplified by removing curly brackets

garagatyi · 2017-03-03T09:44:29Z

agents/exec/src/main/resources/org.eclipse.che.terminal.script.sh

    AGENT_BINARIES_URI=${DOWNLOAD_AGENT_BINARIES_URI}
 fi

+AGENT_BINARIES_URI=${DOWNLOAD_AGENT_BINARIES_URI}


Looks like either this or line 169 is not needed

garagatyi · 2017-03-03T09:49:07Z

wsagent/agent/src/main/resources/org.eclipse.che.ws-agent.script.sh

+
+# no curl, no wget, install curl
+if [ ${CURL_INSTALLED} = false ] && [ ${WGET_INSTALLED} = false ]; then
+  { PACKAGES=${PACKAGES}" curl"; }


garagatyi · 2017-03-03T09:52:17Z

wsagent/agent/src/main/resources/org.eclipse.che.ws-agent.script.sh

+      curl -s  ${AGENT_BINARIES_URI} | tar  xzf - -C ${CHE_DIR}/ws-agent
+    else
+      # use wget
+      wget -qO- ${AGENT_BINARIES_URI} | tar xzf - -C ${CHE_DIR}/ws-agent


Wget downloading section for terminal agent is more complex. Can you elaborate on why they differ and why we can't make them pretty the same?

I don't know, I follow the same approach than curl/terminal agent section which is already complex ^^ https://github.com/eclipse/che/blob/master/agents/exec/src/main/resources/org.eclipse.che.terminal.script.sh#L162-L166

It's due to different sed expressions
sed 's/${PREFIX}/'${PREFIX}'/g'); vs sed 's/-${PREFIX}//g');

like if we need to support different naming scheme for the terminal agent file but I don't know the reason

I don't mean PREFIX stuff. I mean that terminal script uses spider mode in wget and check if current wget doesn't have certificates stuff

benoitf · 2017-03-03T11:01:54Z

I need to make a change to replace https by http if https is used

…ybox version) but not curl by default so, we could check that either curl or wget is installed instead of only checking if curl is installed as we only need to download binaries which wget can perform easily as well Change-Id: I7d02fb70f5f2fb03432072f4f33ff27532c8601e Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

…n a busybox version) but not curl by default Change-Id: I5e95d345ab2ed83d1fc90b914f33d9e006c80226 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf · 2017-03-30T14:21:06Z

ok, PR updated

benoitf · 2017-03-30T15:52:12Z

@JamesDrummond it's ok for release notes ?

codenvy-ci · 2017-03-30T16:10:20Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2332/

garagatyi · 2017-03-31T07:04:01Z

This should improve UX in certain situations a lot!

… + terminal agent (eclipse-che#4029) * For example alpine images have natively wget tool installed (in a busybox version) but not curl by default so, we could check that either curl or wget is installed instead of only checking if curl is installed as we only need to download binaries which wget can perform easily as well Change-Id: I7d02fb70f5f2fb03432072f4f33ff27532c8601e Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Feb 6, 2017

benoitf self-assigned this Feb 6, 2017

benoitf requested review from tolusha, voievodin, JamesDrummond and skabashnyuk February 6, 2017 17:27

benoitf mentioned this pull request Feb 6, 2017

Use of smallest possible workspace (/container) #4024

Closed

4 tasks

JamesDrummond approved these changes Feb 6, 2017

View reviewed changes

garagatyi self-requested a review February 9, 2017 09:34

tolusha approved these changes Feb 9, 2017

View reviewed changes

voievodin approved these changes Feb 9, 2017

View reviewed changes

riuvshin mentioned this pull request Feb 14, 2017

do not redirect /agent-binaries to from http to https codenvy/codenvy#1772

Merged

skabashnyuk requested a review from riuvshin March 3, 2017 08:55

garagatyi reviewed Mar 3, 2017

View reviewed changes

riuvshin approved these changes Mar 3, 2017

View reviewed changes

benoitf added 2 commits March 30, 2017 16:09

fixup! For example alpine images have natively wget tool installed (i…

c0992a1

…n a busybox version) but not curl by default Change-Id: I5e95d345ab2ed83d1fc90b914f33d9e006c80226 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf force-pushed the che#4024-3 branch from d9c673d to c0992a1 Compare March 30, 2017 14:09

garagatyi approved these changes Mar 30, 2017

View reviewed changes

skabashnyuk approved these changes Mar 30, 2017

View reviewed changes

benoitf added this to the 5.7.0 milestone Mar 30, 2017

benoitf merged commit 950dd69 into master Mar 31, 2017

benoitf deleted the che#4024-3 branch March 31, 2017 06:57

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 31, 2017

skabashnyuk mentioned this pull request Apr 6, 2017

Agent installation scripts fail to download binaries if both curl/wget are not installed in container #4721

Closed

JamesDrummond mentioned this pull request Apr 9, 2017

5.7.0 Milestone Overview #4665

Closed

9 tasks

Allow to use wget in addition to curl when installing workspace agent + terminal agent #4029

Allow to use wget in addition to curl when installing workspace agent + terminal agent #4029

Conversation

benoitf commented Feb 6, 2017

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

JamesDrummond commented Feb 6, 2017

benoitf commented Feb 6, 2017

codenvy-ci commented Feb 6, 2017

garagatyi commented Feb 9, 2017

garagatyi commented Feb 9, 2017

tolusha commented Feb 9, 2017

garagatyi commented Feb 9, 2017

garagatyi commented Feb 9, 2017

garagatyi commented Feb 9, 2017

benoitf commented Feb 9, 2017

garagatyi commented Feb 9, 2017

benoitf commented Feb 10, 2017 • edited Loading

garagatyi commented Feb 10, 2017

garagatyi commented Feb 13, 2017

benoitf commented Feb 13, 2017

riuvshin commented Feb 13, 2017

benoitf commented Feb 13, 2017

riuvshin commented Feb 13, 2017

benoitf commented Feb 13, 2017 • edited Loading

riuvshin commented Feb 13, 2017

benoitf commented Feb 13, 2017

riuvshin commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Mar 3, 2017

benoitf commented Mar 30, 2017

benoitf commented Mar 30, 2017

codenvy-ci commented Mar 30, 2017

garagatyi commented Mar 31, 2017

benoitf commented Feb 10, 2017 •

edited

Loading

benoitf commented Feb 13, 2017 •

edited

Loading