Docker config file defaults from environment

[stefanoborini]

Fixes #85

[codecov-io]

Current coverage is 85.04%

Merging #126 into master will increase coverage by 2.91%

@@             master       #126   diff @@
==========================================
  Files            37         36     -1   
  Lines          1343       1344     +1   
  Methods           0          0          
  Messages          0          0          
  Branches        123        123          
==========================================
+ Hits           1103       1143    +40   
+ Misses          200        165    -35   
+ Partials         40         36     -4   

Powered by Codecov. Last updated by 734c6f9...1d378b3

[kitchoi]

I like the fact that default values for tls parameters are extracted from docker.utils.kwargs_from_env. However I would prefer they be defined as traitlet's defaults so that docker.utils.kwargs_from_env is not triggered altogether if one provides all the required parameters in the config file. It might be a bit repetitive but this is because docker.utils.kwargs_from_env automatically checks if the default cert files exist and such check might fail for exactly the same reason why one would want to set the parameters in the file config.
Ref: https://github.com/docker/docker-py/blob/1.8.1/docker/tls.py#L44
https://github.com/docker/docker-py/blob/986a14a152d20df6bb8622516ab2d18af73e5e20/docker/utils/utils.py#L523

[stefanoborini]

Well, if you have the environment variables defined but the certificates are not there, you surely want an error to be raised.

[kitchoi]

Well, if you have the environment variables defined but the certificates are not there, you surely want an error to be raised.

Isn't environment variables such as DOCKER_CERT_PATH set by docker toolbox (and our spawner)? So if one wants to override, e.g. the tls cert parameters, using the file config, the relevant environment variables have to be manually unset? Is this the behaviour we want?

[kitchoi]

Isn't environment variables such as DOCKER_CERT_PATH set by docker toolbox (and our spawner)? So if one wants to override, e.g. the tls cert parameters, using the file config, the relevant environment variables have to be manually unset? Is this the behaviour we want?

Imagine a scenario where the DOCKER_CERT_PATH is set but it does not have the cert files. Instead you have cert files somewhere else. If my mental compilation of the PR is correct, you'd have to either (1) set the environment variable DOCKER_CERT_PATH appropriately or (2) unset the DOCKER_CERT_PATH and then set the parameters in the config file or (3) put the cert files in the default location.

[kitchoi]

Same as traited_instance.trait_names()

[kitchoi]

Regarding the comment on the non-existence of cert/key/ca.pem files in the default cert path, whatever we decide the behaviour should be, I think there should be a test for such scenario.
Apart from this, the rest looks good to merge to me.

[kitchoi]

Should we set these defaults no matter what?

[stefanoborini]

No because otherwise it will enable the tls verification later on, when you generate the dict...

[kitchoi]

when you generate the dict...

Can you point me to this dict please? Just wanted to understand it better.

[stefanoborini]

docker_config, although I noticed it creates a tls section only if you have tls_verify. It should also check for certificate paths. tls_verify == False does not imply that you don't perform tls.

[kitchoi]

Two last minor comments, good to merge then! Thanks @sbo!

[kitchoi]

if tls is True but tls_verify is False, the default of tls_cert and tls_key are not set. Why do we need cert and key when tls is True?

[kitchoi]

Note that docker.Client(tls=True) does completely different thing: https://github.com/docker/docker-py/blob/master/docker/client.py#L90

[stefanoborini]

ok, but we never have tls=True. we have it in the file config but our meaning is the same as the —tls and —tlsverify in docker as a result, we always pass a TLSConfig to the client or nothing, in that case it’s False

[stefanoborini]

if tls is True but tls_verify is False, the default of tls_cert and tls_key are not set. Why do we need cert and key when tls is True?

Yes they are. They are defined on the cert_path variable. Check the init

[kitchoi]

I meant this:

In [1]: from remoteappmanager.file_config import FileConfig

In [2]: tmp = FileConfig(tls=True)

In [3]: tmp.tls_cert, tmp.tls_key
Out[3]: ('', '')

[kitchoi]

@sbo merge?