FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

WIP Expose sslcontext after it was set. by KostyaSha · Pull Request #591 · docker-java/docker-java · GitHub

  1. docker-java
  2. docker-java

Repository navigation

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
Conversation 12 Commits 2 Checks 0 Files changed
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 .java  (5) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
4 changes: 4 additions & 0 deletions src/main/java/com/github/dockerjava/api/command/DockerCmdExecFactory.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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(

KostyaSha Jun 8, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

problem is here

marcuslinke Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

KostyaSha Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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...

marcuslinke Jun 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

KostyaSha Jun 8, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

marcuslinke Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

marcuslinke Jun 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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();
}
}
Back | FazBrowse Home | New Git URL