Set up an stunnel endpoint for the engine.

Set up an stunnel endpoint for the engine.

This puts an stunnel proxy in the engine's Docker container. The client
will connect on the same port, stunnel will intercept and forward the
underlying traffic to the engine which is now listening on another port.

BUG=575421
Committed: https://crrev.com/cc9d2934094ab0dfdbaf210fe7cc2ea924b1edab
Cr-Commit-Position: refs/heads/master@{#372199}

[marcinjb, 2016-01-27 01:39:23]

marcinjb@chromium.org changed reviewers:
+ maniscalco@chromium.org

[marcinjb, 2016-01-27 01:39:23]

First pass review for Docker and stunnel usage.

[nyquist, 2016-01-27 02:02:26]

nyquist@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2016-01-27 02:02:26]

drive-by-review

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:52: const uint16_t kDefaultPort =
12345;
Why this change? Couldn't the internal and external port be the same? That way
we wouldn't have to change the port number if the client connects to a locally
run engine or one running inside a docker container.

[marcinjb, 2016-01-27 16:38:09]

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:52: const uint16_t kDefaultPort =
12345;
On 2016/01/27 02:02:26, nyquist wrote:
> Why this change? Couldn't the internal and external port be the same? That way
> we wouldn't have to change the port number if the client connects to a locally
> run engine or one running inside a docker container.

I don't think we want to expose the unencrypted port when running in the Docker
container

[maniscalco, 2016-01-27 17:02:11]

https://codereview.chromium.org/1638153002/diff/20001/blimp/docs/container.md
File blimp/docs/container.md (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/docs/container.md...
blimp/docs/container.md:91: ### Mapping Ports for the Docker Container
I think it might be time to revamp some of the docs that explain how to run the
engine.   As of this CL, in order to run the engine in a container we need both
-v and -p right?  How about we rewrite/combine this section with the two
sections above into one section and reconcile with the running.md doc:
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/docs/running.md

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:98: _rebased_manifest,
I believe you want the startup script in this list as well, right?

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/start_engi...
File blimp/engine/start_engine.sh (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/start_engi...
blimp/engine/start_engine.sh:7: stunnel -p /engine/data/stunnel.pem -P
/engine/stunnel.pid -d 25467 -r 12345
What happens when stunnel fails?  Will this script (and the container) be
terminated?  Seems like we'd want the script to terminate if either stunnel or
the engine terminate.  That way the higher level will restart the container.

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/start_engi...
blimp/engine/start_engine.sh:8: exec /engine/blimp_engine_app \
One of the feature we're losing with this change is the ability to pass
arbitrary arguments to the engine when running the container from the command
line.  Maybe we should make start_engine.sh a more full-featured script and have
it pass some/all of its args to blimp_engine_app.  Let's chat about this more in
person.

[marcinjb, 2016-01-28 19:29:45]

PTAL

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:98: _rebased_manifest,
On 2016/01/27 17:02:10, maniscalco wrote:
> I believe you want the startup script in this list as well, right?

Derp.  Done.

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/browser/bl...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/browser/bl...
blimp/engine/browser/blimp_engine_session.cc:52: const uint16_t kDefaultPort =
12345;
On 2016/01/27 16:38:09, marcinjb wrote:
> On 2016/01/27 02:02:26, nyquist wrote:
> > Why this change? Couldn't the internal and external port be the same? That
way
> > we wouldn't have to change the port number if the client connects to a
locally
> > run engine or one running inside a docker container.
> 
> I don't think we want to expose the unencrypted port when running in the
Docker
> container

Chatted offline- this is back to 25467

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/start_engi...
File blimp/engine/start_engine.sh (right):

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/start_engi...
blimp/engine/start_engine.sh:7: stunnel -p /engine/data/stunnel.pem -P
/engine/stunnel.pid -d 25467 -r 12345
On 2016/01/27 17:02:10, maniscalco wrote:
> What happens when stunnel fails?  Will this script (and the container) be
> terminated?  Seems like we'd want the script to terminate if either stunnel or
> the engine terminate.  That way the higher level will restart the container.

Done.

https://codereview.chromium.org/1638153002/diff/20001/blimp/engine/start_engi...
blimp/engine/start_engine.sh:8: exec /engine/blimp_engine_app \
On 2016/01/27 17:02:10, maniscalco wrote:
> One of the feature we're losing with this change is the ability to pass
> arbitrary arguments to the engine when running the container from the command
> line.  Maybe we should make start_engine.sh a more full-featured script and
have
> it pass some/all of its args to blimp_engine_app.  Let's chat about this more
in
> person.

Done.

[maniscalco, 2016-01-28 21:17:24]

LGTM modulo adding a comment to start_engine.sh and some doc changes we talked
about OOB.

https://codereview.chromium.org/1638153002/diff/40001/blimp/engine/start_engi...
File blimp/engine/start_engine.sh (right):

https://codereview.chromium.org/1638153002/diff/40001/blimp/engine/start_engi...
blimp/engine/start_engine.sh:17: wait -n
I think it's worth adding a comment explaining why we use wait -n here (i.e. we
want the container to terminate if either stunnel or the engine terminate).

[marcinjb, 2016-01-28 21:41:09]

marcinjb@chromium.org changed reviewers:
+ marshallk@google.com
- nyquist@chromium.org

[marcinjb, 2016-01-28 21:41:10]

Punting over to marshallk@ for engine review

https://codereview.chromium.org/1638153002/diff/40001/blimp/engine/start_engi...
File blimp/engine/start_engine.sh (right):

https://codereview.chromium.org/1638153002/diff/40001/blimp/engine/start_engi...
blimp/engine/start_engine.sh:17: wait -n
On 2016/01/28 21:17:24, maniscalco wrote:
> I think it's worth adding a comment explaining why we use wait -n here (i.e.
we
> want the container to terminate if either stunnel or the engine terminate).

Done.

[Kevin M, 2016-01-28 21:54:11]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-01-28 21:54:12]

https://codereview.chromium.org/1638153002/diff/50006/blimp/engine/Dockerfile
File blimp/engine/Dockerfile (right):

https://codereview.chromium.org/1638153002/diff/50006/blimp/engine/Dockerfile...
blimp/engine/Dockerfile:10: RUN apt-get update && apt-get install -yq stunnel4
This "update" looks redundant?

Also, can stunnel4 be rolled in with the other args to apt-get install?

[marcinjb, 2016-01-28 22:15:38]

PTAL

https://codereview.chromium.org/1638153002/diff/50006/blimp/engine/Dockerfile
File blimp/engine/Dockerfile (right):

https://codereview.chromium.org/1638153002/diff/50006/blimp/engine/Dockerfile...
blimp/engine/Dockerfile:10: RUN apt-get update && apt-get install -yq stunnel4
On 2016/01/28 21:54:12, Kevin M wrote:
> This "update" looks redundant?
> 
> Also, can stunnel4 be rolled in with the other args to apt-get install?

I'd like to keep it separate as one apt-get is the runtime dependencies of the
engine app, the other is our stunnel wrapper.  

The extra update has been excised.

[Kevin M, 2016-01-28 22:18:10]

lgtm

[marcinjb, 2016-01-28 22:25:04]

The CQ bit was checked by marcinjb@chromium.org

[marcinjb, 2016-01-28 22:25:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
maniscalco@chromium.org
Link to the patchset: https://codereview.chromium.org/1638153002/#ps90001
(title: "No loger apt-get updating twice")

[commit-bot: I haz the power, 2016-01-28 22:26:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1638153002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1638153002/90001

[commit-bot: I haz the power, 2016-01-28 23:05:09]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-01-28 23:06:02]

Description was changed from

==========
Set up an stunnel endpoint for the engine.

This puts an stunnel proxy in the engine's Docker container. The client
will connect on the same port, stunnel will intercept and forward the
underlying traffic to the engine which is now listening on another port.

BUG=575421
==========

to

==========
Set up an stunnel endpoint for the engine.

This puts an stunnel proxy in the engine's Docker container. The client
will connect on the same port, stunnel will intercept and forward the
underlying traffic to the engine which is now listening on another port.

BUG=575421

Committed: https://crrev.com/cc9d2934094ab0dfdbaf210fe7cc2ea924b1edab
Cr-Commit-Position: refs/heads/master@{#372199}
==========

[commit-bot: I haz the power, 2016-01-28 23:06:03]

Patchset 6 (id:??) landed as
https://crrev.com/cc9d2934094ab0dfdbaf210fe7cc2ea924b1edab
Cr-Commit-Position: refs/heads/master@{#372199}