Skip to content

WIP Expose sslcontext after it was set. #591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
KostyaSha wants to merge 2 commits into from
Closed

WIP Expose sslcontext after it was set. #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95ea1ee
Expose sslcontext after it was set.
KostyaSha Jun 5, 2016
bc94435
Don't verify dockerCertPath. SSL can be set without FS.
KostyaSha Jun 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/java/com/github/dockerjava/api/command/DockerCmdExecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.io.Closeable;
import java.io.IOException;

import javax.annotation.CheckForNull;
import javax.net.ssl.SSLContext;

import com.github.dockerjava.core.DockerClientConfig;
Expand Down Expand Up @@ -119,6 +120,9 @@ public interface DockerCmdExecFactory extends Closeable {

DockerCmdExecFactory withSSLContext(SSLContext sslContext);

@CheckForNull
SSLContext getSSLContext();

@Override
void close() throws IOException;

Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/github/dockerjava/core/DockerClientConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,9 @@ private URI checkDockerHostScheme(URI dockerHost) {
private String checkDockerCertPath(boolean dockerTlsVerify, String dockerCertPath) {
if (dockerTlsVerify) {
if (StringUtils.isEmpty(dockerCertPath)) {
throw new DockerClientException(
"Enabled TLS verification (DOCKER_TLS_VERIFY=1) but certifate path (DOCKER_CERT_PATH) is not defined.");
return dockerCertPath;
// throw new DockerClientException(
Copy link
Member Author

@KostyaSha KostyaSha Jun 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem is here

Copy link
Contributor

@marcuslinke marcuslinke Jun 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I see... I don't like this. Maybe we have to rethink config class(es) again then. What about introducing a DockerClientConfig interface that must provide a SSLContext. Then we have a DefaultDockerClientConfig implementation of this DockerClientConfig interface that acts like the former DockerClientConfig class and someone (like you) could implement a second class that constructs the SSLContext in a different way (maybe from a database or whatever).

Copy link
Member Author

@KostyaSha KostyaSha Jun 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we have the mess in configs for creating connections i..e i hade DockerClientCOnfigBuilderForPlugin, then Builder for factory and then the whole builder for two builders... Now ssl moved...
Is it possible to make one Config for creating connection?
🏧 i can be satisfied even with additional boolean in DockerClientConfig...

Copy link
Contributor

@marcuslinke marcuslinke Jun 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said: I really don't like this. This is bad design in my eyes. I will experiment locally with interface and different impl. I think only the default config impl needs a builder. If someone wants a different configuration implementation its his / her reponsibility to create the instance, maybe even with a custom builder. WDYT?

Copy link
Member Author

@KostyaSha KostyaSha Jun 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anybody would be glad to fill configuration only in single DockerClientBuilder and get connection :)

As I said: I really don't like this.

Didn't get what exactly. Making one builder, adding boolean for now while there is no better solution to unblock programmatic usage or having multiple configs/builders?

Copy link
Contributor

@marcuslinke marcuslinke Jun 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean its bad design because for using a custom SSLContext you would have to do two things:

  1. disable usage of dockerCertPath in the config
  2. Set custom SSLContext into DockerCmdExecFActory

Copy link
Contributor

@marcuslinke marcuslinke Jun 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the existing config builder could create the SSLContext from dockerCertPath by default and pass it to the constructor of the config when build() is called. Then we could introduce a withCustomSSLContext method to the builder that overrides the default behavior. I think this would be the simplest and cleanest solution.

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

Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/github/dockerjava/jaxrs/DockerCmdExecFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,11 @@ public DockerCmdExecFactoryImpl withSSLContext(SSLContext sslContext) {
return this;
}

@Override
public SSLContext getSSLContext() {
return sslContext;
}

public DockerCmdExecFactoryImpl withReadTimeout(Integer readTimeout) {
this.readTimeout = readTimeout;
return this;
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/github/dockerjava/netty/DockerCmdExecFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,11 @@ public DockerCmdExecFactory withSSLContext(SSLContext sslContext) {
return this;
}

@Override
public SSLContext getSSLContext() {
return sslContext;
}

private WebTarget getBaseResource() {
return new WebTarget(channelProvider);
}
Expand Down
7 changes: 7 additions & 0 deletions src/test/java/com/github/dockerjava/core/TestDockerCmdExecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import com.github.dockerjava.api.command.WaitContainerCmd;
import com.github.dockerjava.api.model.BuildResponseItem;

import javax.annotation.CheckForNull;
import javax.net.ssl.SSLContext;
import java.io.IOException;
import java.security.SecureRandom;
Expand Down Expand Up @@ -436,4 +437,10 @@ public List<String> getNetworkIds() {
public DockerCmdExecFactory withSSLContext(SSLContext sslContext) {
return delegate.withSSLContext(sslContext);
}

@CheckForNull
@Override
public SSLContext getSSLContext() {
return delegate.getSSLContext();
}
}