Refactor configuration of SSL to allow override with custom config #596

marcuslinke · 2016-06-09T18:32:33Z

This change is

KostyaSha · 2016-06-09T19:17:17Z

src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java

+    // CHECKSTYLE:OFF
+    @Override
+    public boolean equals(Object o) {
+        if (this == o)


doesn't reflectionEquals checks all this things?

KostyaSha · 2016-06-09T19:22:16Z

from first lookup LGTM

KostyaSha · 2016-06-12T01:14:19Z

src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java

+            if (customSslContext == null) {
+                if (dockerTlsVerify) {
+                    dockerCertPath = checkDockerCertPath(dockerCertPath);
+                    sslContext = new LocalDirectorySSLConfig(dockerCertPath).getSSLContext();


Is it possible to store SSLConfig in DefaultClientConfig instead context?
I need know from what dockerCertPath connection was build.
Use case: calling default dockerclientconfig builder that gather configs, env vars, properties and they i extract dockercertPath for creating special remote test connection.

KostyaSha · 2016-06-12T01:15:21Z

From second look i can't extract dockerCertPath from SSLContext, DefaultDockerClientConfig should store SSLConfig.

KostyaSha · 2016-06-12T01:32:00Z

717d9f8

KostyaSha · 2016-06-12T01:40:59Z

Pushed commit into this branch

codecov-io · 2016-06-12T01:44:37Z

Current coverage is 30.54%

Merging #596 into master will increase coverage by 7.16%

2 files in ...thub/dockerjava/core were modified. more
- Misses -4
- Hits +4
2 files in ...om/github/dockerjava were modified. more
- Misses -21
- Hits +21
3 files (not in diff) in ...erjava/netty/handler were modified. more
- Misses -19
- Partials -2
- Hits +21
17 files (not in diff) in ...ockerjava/netty/exec were modified. more
- Misses -36
- Hits +36
2 files (not in diff) in ...hub/dockerjava/netty were modified. more
- Misses -11
- Hits +11
5 files (not in diff) in ...kerjava/jaxrs/filter were modified. more
- Misses -23
- Hits +23
2 files (not in diff) in ...java/jaxrs/connector were modified. more
- Misses -49
- Hits +49
21 files (not in diff) in ...hub/dockerjava/jaxrs were modified. more
- Misses -46
- Hits +46
5 files (not in diff) in ...dockerjava/core/util were modified. more
- Misses -24
- Partials -6
- Hits +30
2 files (not in diff) in ...java/core/dockerfile were modified. more
- Misses -15
- Partials -11
- Hits +26

@@             master       #596   diff @@
==========================================
  Files           296        296          
  Lines          6262       6260     -2   
  Methods           0          0          
  Messages          0          0          
  Branches        561        560     -1   
==========================================
+ Hits           1464       1912   +448   
+ Misses         4704       4348   -356   
+ Partials         94          0    -94

Powered by Codecov. Last updated by 3a2dcc4...0063563

marcuslinke · 2016-06-12T07:15:46Z

What's the goal of your latest commit? Why you want DockerClientConfig expose SSLConfig?

KostyaSha · 2016-06-12T18:52:01Z

What's the goal of your latest commit? Why you want DockerClientConfig expose SSLConfig?

Because i need know exact SSLConfig configuration that provided SSL connection. I can get hosts, names, etc, but not ssl part. That's why SSLConfig (like it was before) should be stored and not SSLContext.
Fix is not final, still testing it. Found NPE (lost if check).

PS Found that com.github.dockerjava.netty.DockerCmdExecFactoryImpl.InetSocketInitializer#init has
final SocketAddress proxyAddress = new InetSocketAddress(addr, 8008); that never used.

KostyaSha · 2016-06-13T00:11:26Z

Seems now works fine. Tested from my fork https://github.com/KostyaSha/docker-java/commits/3.0.0-kostyasha

KostyaSha · 2016-06-13T00:12:05Z

src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java

+ */
+public class DefaultDockerClientConfig implements Serializable, DockerClientConfig {
+
+    private static final long serialVersionUID = -4307357472441531489L;


1L if it new class

KostyaSha · 2016-06-13T00:21:13Z

TODO update README.md after merge because builder will change?

marcuslinke · 2016-06-13T06:50:56Z

@KostyaSha OK, understood now. Feel free to fix your comments and merge.

KostyaSha · 2016-06-13T07:08:18Z

ok

KostyaSha · 2016-06-13T20:17:41Z

src/main/java/com/github/dockerjava/core/DefaultDockerClientConfig.java

                        "Enabled TLS verification (DOCKER_TLS_VERIFY=1) but certifate path (DOCKER_CERT_PATH) is not defined.");
-            } else {
-                File certPath = new File(dockerCertPath);
+            }


removed nested ifs, should be better readable now

KostyaSha · 2016-06-13T20:20:37Z

@marcuslinke please review

marcuslinke · 2016-06-13T20:57:07Z

@KostyaSha Thanks!

KostyaSha · 2016-06-13T20:57:56Z

@marcuslinke thanks to you for better solution!

KostyaSha · 2016-06-13T20:58:23Z

Would it be 3.0.1? I created 3.0.1 milestone.

KostyaSha · 2016-06-16T23:39:03Z

Causes issues in integration tests. Will try fix later

…ocker-java#596) * Refactor configuration of SSL to allow override with custom config * Store SSLConfig instead SSLContext * avoid NPE * prepare release * Coding style changes

Refactor configuration of SSL to allow override with custom config

8ca1932

marcuslinke mentioned this pull request Jun 9, 2016

WIP Expose sslcontext after it was set. #591

Closed

KostyaSha reviewed Jun 9, 2016
View reviewed changes

KostyaSha reviewed Jun 12, 2016
View reviewed changes

Store SSLConfig instead SSLContext

e0a8573

avoid NPE

57098cd

prepare release

5c65346

KostyaSha reviewed Jun 13, 2016
View reviewed changes

Coding style changes

0063563

KostyaSha reviewed Jun 13, 2016
View reviewed changes

KostyaSha added the type/enhancement label Jun 13, 2016

marcuslinke merged commit 9ae14cb into master Jun 13, 2016

KostyaSha added this to the 3.0.1 milestone Jun 13, 2016

marcuslinke deleted the refact-sslconfig branch June 13, 2016 21:03

Refactor configuration of SSL to allow override with custom config #596

Refactor configuration of SSL to allow override with custom config #596

Uh oh!

Conversation

marcuslinke commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha Jun 9, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Jun 9, 2016

Uh oh!

KostyaSha Jun 12, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Jun 12, 2016

Uh oh!

codecov-io commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 30.54%

Uh oh!

marcuslinke commented Jun 12, 2016

Uh oh!

KostyaSha commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha commented Jun 13, 2016

Uh oh!

KostyaSha Jun 13, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Jun 13, 2016

Uh oh!

marcuslinke commented Jun 13, 2016

Uh oh!

KostyaSha commented Jun 13, 2016

Uh oh!

KostyaSha Jun 13, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Jun 13, 2016

Uh oh!

marcuslinke commented Jun 13, 2016

Uh oh!

KostyaSha commented Jun 13, 2016

Uh oh!

KostyaSha commented Jun 13, 2016

Uh oh!

KostyaSha commented Jun 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcuslinke commented Jun 9, 2016 •

edited

Loading

KostyaSha commented Jun 12, 2016 •

edited

Loading

KostyaSha commented Jun 12, 2016 •

edited

Loading

codecov-io commented Jun 12, 2016 •

edited

Loading

KostyaSha commented Jun 12, 2016 •

edited

Loading