You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrea Turli <no...@github.com> on 2014/11/25 15:56:37 UTC
[jclouds-labs] support for docker 1.3.2 (#113)
- update value objects (Container, Config)
- add new value object (Resource, StatusCode)
- add more options for ContainerApi
- add support for pause/unpause, restart, kill, copy and attach APIs + Mock and Live tests
You can merge this Pull Request by running:
git pull https://github.com/andreaturli/jclouds-labs 1.3.2
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/113
-- Commit Summary --
* support for docker 1.3.2
-- File Changes --
M docker/src/main/java/org/jclouds/docker/compute/strategy/DockerComputeServiceAdapter.java (4)
M docker/src/main/java/org/jclouds/docker/domain/Config.java (214)
M docker/src/main/java/org/jclouds/docker/domain/Container.java (118)
A docker/src/main/java/org/jclouds/docker/domain/Resource.java (33)
M docker/src/main/java/org/jclouds/docker/domain/State.java (10)
A docker/src/main/java/org/jclouds/docker/domain/StatusCode.java (32)
M docker/src/main/java/org/jclouds/docker/features/ContainerApi.java (123)
M docker/src/main/java/org/jclouds/docker/features/ImageApi.java (4)
M docker/src/main/java/org/jclouds/docker/handlers/DockerErrorHandler.java (4)
A docker/src/main/java/org/jclouds/docker/options/AttachOptions.java (97)
A docker/src/main/java/org/jclouds/docker/options/KillOptions.java (50)
A docker/src/main/java/org/jclouds/docker/options/RestartOptions.java (46)
A docker/src/main/java/org/jclouds/docker/options/StopOptions.java (46)
M docker/src/main/java/org/jclouds/docker/suppliers/SSLContextWithKeysSupplier.java (1)
M docker/src/test/java/org/jclouds/docker/compute/DockerComputeServiceAdapterLiveTest.java (4)
M docker/src/test/java/org/jclouds/docker/compute/functions/ContainerToNodeMetadataTest.java (3)
M docker/src/test/java/org/jclouds/docker/features/ContainerApiLiveTest.java (56)
M docker/src/test/java/org/jclouds/docker/features/ContainerApiMockTest.java (242)
M docker/src/test/java/org/jclouds/docker/features/MiscApiLiveTest.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/113.patch
https://github.com/jclouds/jclouds-labs/pull/113.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +public class RestartOptions extends BaseHttpRequestOptions {
> +
> + public static final RestartOptions NONE = new RestartOptions();
> +
> + /**
> + * @param t number of seconds to wait before killing the container
> + * @return RestartOptions
> + */
> + public RestartOptions t(String t) {
agreed, re-using same name given by Docker team
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875710
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> throw propagate(e);
> }
> }
> +
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
fixed
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r22327843
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Christopher Dancy <no...@github.com>.
"Anyway, I reverted to the previous impl with System.property but I really hope we can find a final approach soon so that docker can go out of labs in the next release"
+1 (I'm following this PR. Any help you guys need, whether testing or something else, feel free to reach out)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64698848
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> }
>
> public Builder fromConfig(Config in) {
> return hostname(in.hostname()).domainname(in.domainname()).user(in.user()).memory(in.memory())
> .memorySwap(in.memorySwap()).cpuShares(in.cpuShares()).attachStdin(in.attachStdin())
> - .attachStdout(in.attachStdout()).attachStderr(in.attachStderr()).exposedPorts(in.exposedPorts())
> - .tty(in.tty()).openStdin(in.openStdin()).stdinOnce(in.stdinOnce()).env(in.env()).cmd(in.cmd())
> - .dns(in.dns()).image(in.image()).volumes(in.volumes()).volumesFrom(in.volumesFrom())
> - .workingDir(in.workingDir()).entrypoint(in.entrypoint()).networkDisabled(in.networkDisabled())
> - .onBuild(in.onBuild());
> + .attachStdout(in.attachStdout()).attachStderr(in.attachStderr()).tty(in.tty())
> + .image(in.image()).volumes(in.volumes()).workingDir(in.workingDir())
> + .networkDisabled(in.networkDisabled()).exposedPorts(in.exposedPorts()).securityOpts(in.securityOpts())
> + .hostConfig(in.hostConfig()).binds(in.binds()).links(in.links()).lxcConf(in.lxcConf())
> + .portBindings(in.portBindings()).publishAllPorts(in.publishAllPorts()).privileged(in.privileged())
> + .dns(in.dns()).dnsSearch(in.dnsSearch()).volumesFrom(in.volumesFrom()).capAdd(in.capAdd())
> + .capDrop(in.capDrop()).restartPolicy(in.restartPolicy()).networkMode(in.networkMode()).devices(in.devices());
Also, there are several properties that are *not* annotated as `@Nullable`. The builder should check that those properties are not null when building the object.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21864467
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + * @param containerId The id of the container to be unpaused.
> + */
> + @Named("container:unpause")
> + @POST
> + @Path("/containers/{id}/unpause")
> + void unpause(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
> + *
> + * @param containerId The id of the container to be attached.
> + */
> + @Named("container:attach")
> + @POST
> + @Path("/containers/{id}/attach")
> + InputStream attach(@PathParam("id") String containerId);
I'd like to get it right in this PR, rather than add a TODO.
Do you have suggestions/examples about a better return type?
Thanks
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21010899
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> return this;
> }
>
> - public Builder networkDisabled(boolean networkDisabled) {
> - this.networkDisabled = networkDisabled;
> + public Builder securityOpts(List<String> securityOpts) {
> + this.securityOpts = ImmutableList.copyOf(checkNotNull(securityOpts, "securityOpts"));
ok
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875233
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
Closed #113.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#event-218832236
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> }
>
> @Override
> public void suspendNode(String id) {
> - throw new UnsupportedOperationException("suspend not supported");
> + api.getContainerApi().pause(id);
progress!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874337
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -144,6 +135,25 @@
> void stopContainer(@PathParam("id") String containerId);
>
> /**
> + * @param containerId The id of the container to be stopped.
> + * @param options the stop options @see org.jclouds.docker.options.StopOptions
> + */
> + @Named("container:stop")
> + @POST
> + @Path("/containers/{id}/stop")
> + void stopContainer(@PathParam("id") String containerId, StopOptions options);
here! instead of making an options cruft to enclose a single option, use `@QueryParam("t") int waitSeconds`. Less code, less confusion.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21004959
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #385](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/385/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64412056
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Resource {
> +
> + public abstract String resource();
> +
> + @SerializedNames({ "Resource" })
> + public static Resource create(String resource) {
> + return new AutoValue_Resource(resource);
> + }
add a package private ctor, as auto-values shouldn't be subclassed.
ex.
```java
Resource(){
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874639
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
The build fails due to checkstyle violations. Address them while squashing and +1!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-69918918
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * @param containerId The id of the container to be unpaused.
> + */
> + @Named("container:unpause")
> + @POST
> + @Path("/containers/{id}/unpause")
> + void unpause(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
> + *
> + * @param containerId The id of the container to be attached.
> + */
> + @Named("container:attach")
> + @POST
> + @Path("/containers/{id}/attach")
> + InputStream attach(@PathParam("id") String containerId);
as mentioned before, this is dubious. Unless there's a chance of binary content, we should use reader. PS the javadoc isn't helpful, so remove it (unless you plan to explain this result)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874852
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> @@ -63,6 +63,7 @@ public SSLContext get() {
> kmf.init(keyStore.get(), keyStorePassword.toCharArray());
> SSLContext sc = SSLContext.getInstance("TLS");
> sc.init(kmf.getKeyManagers(), trustManager, new SecureRandom());
> + System.setProperty("https.protocols", "TLSv1");
I was afraid about that, any suggestions?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875748
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +public class RestartOptions extends BaseHttpRequestOptions {
> +
> + public static final RestartOptions NONE = new RestartOptions();
> +
> + /**
> + * @param t number of seconds to wait before killing the container
> + * @return RestartOptions
> + */
> + public RestartOptions t(String t) {
overload. this is nonsense. and clutters up the project with a builder class to enclose a cryptic field. Use `@QueryParam("t") secondsToWait`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21004878
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -63,6 +63,9 @@ public SSLContext get() {
> kmf.init(keyStore.get(), keyStorePassword.toCharArray());
> SSLContext sc = SSLContext.getInstance("TLS");
> sc.init(kmf.getKeyManagers(), trustManager, new SecureRandom());
> + // TODO this is a temporary solution to disable SSL v3.0 in JDK and JRE b/c of https://github.com/docker/docker/pull/8588/files.
> + // This system property will be removed once we can use an http driver with strict control on TLS protocols
> + System.setProperty("https.protocols", "TLSv1");
thx cc @demobox @nacx This is what we'll replace with okhttp before moving to core.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21004904
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
@nacx tests are green, it is rebased to master but if we are happy with that, probably best to squash them before merging it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-66914824
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
@nacx I'd like to have your thoughts, particularly if that was something similar to your suggestion on IRC
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64663355
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +public class RestartOptions extends BaseHttpRequestOptions {
do we really need an options class for this? why not overload with the seconds to wait?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875139
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
There just a couple remaining minors. lgtm once they're fixed. Thanks @andreaturli !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-69892625
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
> +
> + try {
> + PEMParser pemParser = new PEMParser(new StringReader(privateKey));
> + Object object = pemParser.readObject();
> + if (Security.getProvider("BC") == null) {
> + Security.addProvider(new BouncyCastleProvider());
Yes, but do you really want to "rely" on a specific ssh driver to be able to run your authentication code? Wouldn't it be better to declare the BC driver dependency too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21886630
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> bind(new TypeLiteral<Supplier<SSLContext>>() {
> }).to(new TypeLiteral<SSLContextWithKeysSupplier>() {
> });
> - bind(new TypeLiteral<Supplier<KeyStore>>() {
> - }).to(new TypeLiteral<KeyStoreSupplier>() {
> - });
> + bind(OkHttpClientSupplier.class).to(new TypeLiteral<DockerOkHttpClientSupplier>() {});
Do it simpler:
```java
bind(OkHttpClientSupplier.class).to(DockerOkHttpClientSupplier.class);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21864224
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1912](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1912/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64413269
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +/**
> + * Options to customize attach
see comment about javadoc that doesn't clarify anything
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875074
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -99,7 +135,7 @@ public Builder toBuilder() {
> return builder().fromConfig(this);
> }
>
> - public static final class Builder {
> + public static class Builder {
please put final back. autovalue implies final
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874410
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Resource {
> +
> + public abstract String resource();
> +
> + @SerializedNames({ "Resource" })
> + public static Resource create(String resource) {
> + return new AutoValue_Resource(resource);
> + }
https://github.com/google/auto/tree/master/value#best-practices
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21004822
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1919](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1919/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64701278
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #388](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/388/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64534298
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
Thanks again @adriancole and @demobox
I've added a couple of comments to explain why Docker requires TLS and how this PR deals with it (temporarily)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64760771
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #392](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/392/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64760664
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> private final TrustManager[] trustManager;
> private final Supplier<Credentials> creds;
>
> @Inject
> - SSLContextWithKeysSupplier(Supplier<KeyStore> keyStore, @Provider Supplier<Credentials> creds, HttpUtils utils,
> - TrustAllCerts trustAllCerts) {
> - this.keyStore = keyStore;
> - this.trustManager = utils.trustAllCerts() ? new TrustManager[] { trustAllCerts } : null;
> + SSLContextWithKeysSupplier(@Provider Supplier<Credentials> creds, TrustAllCerts trustAllCerts) {
> + this.trustManager = new TrustManager[]{trustAllCerts};
Trusting all certificates is not related to self-signed certificates. A TrustManager *tells* the JVM if it can trust a server certificate or not, regardless of how those certificates are signed or generated. Instructing the JVM to "trust every single certificate you are presented" is not a good approach to trust one particular certificate just because it is self-signed.
You should install the trust all certs if the users have configured that, or configure the default one (by passing `null`, read the [SSLContext#init javadocs](http://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLContext.html#init(javax.net.ssl.KeyManager[],%20javax.net.ssl.TrustManager[],%20java.security.SecureRandom)). This way the appropriate default TrustManager will be picked and users that don't use the "trust-all-certs" thing will be able to install just the certificates they trust.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21887247
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1923](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1923/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64761665
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Resource {
> +
> + public abstract String resource();
> +
> + @SerializedNames({ "Resource" })
> + public static Resource create(String resource) {
> + return new AutoValue_Resource(resource);
> + }
that applies to all the value classes?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874874
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
@nacx I think I need to rebase and squash so that you can merge it, yes?
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-69901231
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> public Builder fromConfig(Config in) {
> return hostname(in.hostname()).domainname(in.domainname()).user(in.user()).memory(in.memory())
> .memorySwap(in.memorySwap()).cpuShares(in.cpuShares()).attachStdin(in.attachStdin())
> - .attachStdout(in.attachStdout()).attachStderr(in.attachStderr()).exposedPorts(in.exposedPorts())
> - .tty(in.tty()).openStdin(in.openStdin()).stdinOnce(in.stdinOnce()).env(in.env()).cmd(in.cmd())
> - .dns(in.dns()).image(in.image()).volumes(in.volumes()).volumesFrom(in.volumesFrom())
> - .workingDir(in.workingDir()).entrypoint(in.entrypoint()).networkDisabled(in.networkDisabled())
> - .onBuild(in.onBuild());
> + .attachStdout(in.attachStdout()).attachStderr(in.attachStderr()).tty(in.tty())
> + .image(in.image()).volumes(in.volumes()).workingDir(in.workingDir())
> + .networkDisabled(in.networkDisabled()).exposedPorts(in.exposedPorts()).securityOpts(in.securityOpts())
> + .hostConfig(in.hostConfig()).binds(in.binds()).links(in.links()).lxcConf(in.lxcConf())
> + .portBindings(in.portBindings()).publishAllPorts(in.publishAllPorts()).privileged(in.privileged())
> + .dns(in.dns()).dnsSearch(in.dnsSearch()).volumesFrom(in.volumesFrom()).capAdd(in.capAdd())
> + .capDrop(in.capDrop()).restartPolicy(in.restartPolicy()).networkMode(in.networkMode()).devices(in.devices());
[minor] Indenting?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20977776
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1924](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1924/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64763177
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + } catch (ResourceNotFoundException ex) {
> + // Expected exception
> + } finally {
> + dockerApi.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testPauseContainer() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(204));
> + DockerApi dockerApi = api(server.getUrl("/"));
> + ContainerApi api = dockerApi.getContainerApi();
> + try {
> + api.pause("1");
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/containers/1/pause");
bad name
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875370
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrew Phillips <no...@github.com>.
Thanks for the review @adriancole!
> TLSv1 is suggested here http://www.oracle.com/technetwork/java/javase/documentation/cve-2014-
> 3566-2342133.html and it is not a docker requirement.
W.r.t. this, if this change is not a requirement to support this new API version, my vote would be for removing it from this PR and, if desired, submitting it separately.
This will allow us to merge this more quickly and get the desired functionality it, while keeping the more contentious fix separate and making it much easier to revert when/if desired.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64742014
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Resource {
yeah that was nice!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874830
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1916](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1916/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64662075
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> private final TrustManager[] trustManager;
> private final Supplier<Credentials> creds;
>
> @Inject
> - SSLContextWithKeysSupplier(Supplier<KeyStore> keyStore, @Provider Supplier<Credentials> creds, HttpUtils utils,
> - TrustAllCerts trustAllCerts) {
> - this.keyStore = keyStore;
> - this.trustManager = utils.trustAllCerts() ? new TrustManager[] { trustAllCerts } : null;
> + SSLContextWithKeysSupplier(@Provider Supplier<Credentials> creds, TrustAllCerts trustAllCerts) {
> + this.trustManager = new TrustManager[]{trustAllCerts};
Are you going to *always* trust all certs? That should be configured by users when setting the corresponding properties when creating the context. You should check the `utils.trustAllCerts()` value and act accordingly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21864915
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -63,6 +63,7 @@ public SSLContext get() {
> kmf.init(keyStore.get(), keyStorePassword.toCharArray());
> SSLContext sc = SSLContext.getInstance("TLS");
> sc.init(kmf.getKeyManagers(), trustManager, new SecureRandom());
> + System.setProperty("https.protocols", "TLSv1");
look up what this property actually does. It likely can be represented in
code. System property is a race condition and also pollutes the JVM.
Here is a hint
http://www.oracle.com/technetwork/java/javase/documentation/cve-2014-3566-2342133.html
On Tue, Nov 25, 2014 at 8:44 AM, Andrea Turli <no...@github.com>
wrote:
> In
> docker/src/main/java/org/jclouds/docker/suppliers/SSLContextWithKeysSupplier.java:
>
> > @@ -63,6 +63,7 @@ public SSLContext get() {
> > kmf.init(keyStore.get(), keyStorePassword.toCharArray());
> > SSLContext sc = SSLContext.getInstance("TLS");
> > sc.init(kmf.getKeyManagers(), trustManager, new SecureRandom());
> > + System.setProperty("https.protocols", "TLSv1");
>
> I was afraid about that, any suggestions?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/jclouds/jclouds-labs/pull/113/files#r20875748>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20880849
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +/**
> + * Options to kill container builder.
> + */
> +public class KillOptions extends BaseHttpRequestOptions {
> +
> + public static final KillOptions NONE = new KillOptions();
remove this
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875035
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
@nacx Any chance to give your thoughts on that?
FYI I've been also experimenting with the latest boot2docker available which runs docker 1.4.1 and docker API v1.16 and the code needs only a minor adjustment for Info class as that version is returning a richer object. Other than that, the other LiveTests will be ok.
So as soon as we merge it, there'll be a simple PR to support latest docker API as well!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-67775999
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +public class RestartOptions extends BaseHttpRequestOptions {
> +
> + public static final RestartOptions NONE = new RestartOptions();
> +
> + /**
> + * @param t number of seconds to wait before killing the container
> + * @return RestartOptions
> + */
> + public RestartOptions t(String t) {
horrible name
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875102
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #393](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/393/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64760764
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1931](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1931/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64818012
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #389](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/389/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64661769
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class StatusCode {
> +
> + public abstract int statusCode();
> +
> + @SerializedNames({ "StatusCode" })
> + public static StatusCode create(int statusCode) {
> + return new AutoValue_StatusCode(statusCode);
> + }
same comment wrt package private ctor. Also, if a field of this type isn't declared in more than one class, please nest it. We don't want to pollute or directories with single-field value classes, unless there's a good reason. Same for "Resource"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874772
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrew Phillips <no...@github.com>.
> Docker did change and set a floor wrt TLS
Clear. Thanks for the investigation, @andreaturli and @adriancole. In that case, I'm fine with merging this here but would like to see this replaced by use of a different HTTP driver before a move to core.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64806914
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
@andreaturli Any update on this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-66355949
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
argh! @nacx
Fixing them now
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-69925743
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
TLSv1 is suggested here http://www.oracle.com/technetwork/java/javase/documentation/cve-2014-3566-2342133.html and it is not a docker requirement. It is only a simple way to avoid SSLv3 support, which is one of the protocol supported by default by `SSLContext.getInstance("TLS")`
Anyway, I reverted to the previous impl with System.property but I really hope we can find a final approach soon so that docker can go out of labs in the next release
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64697939
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #396](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/396/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64817652
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1930](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1930/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64814992
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -70,7 +71,8 @@
> @Named("image:inspect")
> @GET
> @Path("/images/{name}/json")
> - @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
nit: import Fallbacks.NullOnNotFoundOr404 explicitly
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874925
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * @param containerId The id of the container to be unpaused.
> + */
> + @Named("container:unpause")
> + @POST
> + @Path("/containers/{id}/unpause")
> + void unpause(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
> + *
> + * @param containerId The id of the container to be attached.
> + */
> + @Named("container:attach")
> + @POST
> + @Path("/containers/{id}/attach")
> + InputStream attach(@PathParam("id") String containerId);
I'm trying to ask if the stream is binary (ex. Unrepresentable data) or a
character stream. If it is console output, it is likely a character stream.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21006996
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.http.okhttp.OkHttpClientSupplier;
> +
> +import com.google.common.collect.ImmutableList;
> +import com.squareup.okhttp.ConnectionSpec;
> +import com.squareup.okhttp.OkHttpClient;
> +import com.squareup.okhttp.TlsVersion;
> +import javax.inject.Inject;
> +import javax.inject.Singleton;
> +
> +@Singleton
> +public class DockerOkHttpClientSupplier implements OkHttpClientSupplier {
> +
> + private final SSLContextWithKeysSupplier sslContextWithKeysSupplier;
> +
> + @Inject
> + public DockerOkHttpClientSupplier(SSLContextWithKeysSupplier sslContextWithKeysSupplier) {
Make the constructor package private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21864283
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <ad...@gmail.com>.
Generic is irrelevant.
So to answer my question with the link you posted, the docker stream is
binary. InputStream is better than Reader for this reason. However, as the
encoding of the stream is documented, this is just punting the problem of
how to decode back to callers. Please add a TODO to return a better type
than inputstream with the link you mentioned.
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + * @param containerId The id of the container to be unpaused.
> + */
> + @Named("container:unpause")
> + @POST
> + @Path("/containers/{id}/unpause")
> + void unpause(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
> + *
> + * @param containerId The id of the container to be attached.
> + */
> + @Named("container:attach")
> + @POST
> + @Path("/containers/{id}/attach")
> + InputStream attach(@PathParam("id") String containerId);
InputStream is more generic and it seems better to cover all the combinations specified [there](https://docs.docker.com/reference/api/docker_remote_api_v1.15/#attach-to-a-container)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21008408
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #391](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/391/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64697805
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> private final TrustManager[] trustManager;
> private final Supplier<Credentials> creds;
>
> @Inject
> - SSLContextWithKeysSupplier(Supplier<KeyStore> keyStore, @Provider Supplier<Credentials> creds, HttpUtils utils,
> - TrustAllCerts trustAllCerts) {
> - this.keyStore = keyStore;
> - this.trustManager = utils.trustAllCerts() ? new TrustManager[] { trustAllCerts } : null;
> + SSLContextWithKeysSupplier(@Provider Supplier<Credentials> creds, TrustAllCerts trustAllCerts) {
> + this.trustManager = new TrustManager[]{trustAllCerts};
Really don't know how to solve that.
>From Docker doc, even if you specify the certs, you may need to run curl command with -insecure as the certificates generated for boot2docker are self-signed.
I think the equivalent in jclouds of `insecure` is `trustAllCerts` even if you pass identity and credential. Initially I was using `utils.trustAllCerts()` to act accordingly but `OkHttpCommandExecutorServiceModule` overrides the decision taken at level of docker.
I think we should we change `OkHttpCommandExecutorServiceModule`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21886574
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
@nacx I think you are right, better to remove the password as we don't suggest to use encrypted PEM file
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-69907182
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +/**
> + * Options to customize attach
> + */
> +public class AttachOptions extends BaseHttpRequestOptions {
> +
> + public static final AttachOptions NONE = new AttachOptions();
> +
> + public AttachOptions stream(Boolean stream) {
javadoc this stuff.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874995
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -53,11 +54,11 @@ protected void bindErrorHandlers() {
> @Override
> protected void configure() {
> super.configure();
> + install(new OkHttpCommandExecutorServiceModule());
> bind(new TypeLiteral<Supplier<SSLContext>>() {
Do you need this binding? You are directly injecting the `SSLContextWithKeysSupplier` type.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21864338
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> private final TrustManager[] trustManager;
> private final Supplier<Credentials> creds;
>
> @Inject
> - SSLContextWithKeysSupplier(Supplier<KeyStore> keyStore, @Provider Supplier<Credentials> creds, HttpUtils utils,
> - TrustAllCerts trustAllCerts) {
> - this.keyStore = keyStore;
> - this.trustManager = utils.trustAllCerts() ? new TrustManager[] { trustAllCerts } : null;
> + SSLContextWithKeysSupplier(@Provider Supplier<Credentials> creds, TrustAllCerts trustAllCerts) {
> + this.trustManager = new TrustManager[]{trustAllCerts};
TrustManager is responsible to validate the certificate chains, so if the certificate coming from the server is self-signed (not issued by a CA included in the JVM default trustStore) will fail the request. Also I don't think I'm affecting the entire JVM with that code, but only the jclouds-docker calls.
Again, whatever decision I make using `utils.trustAllCerts()` will be overridden by `OkHttpCommandExecutorServiceModule` and this will break things
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21887851
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #394](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/394/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64768856
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -69,9 +69,7 @@ else if (message != null && message.indexOf("currently in use") != -1)
> exception = new IllegalStateException(message, exception);
> break;
> case 404:
> - if (!command.getCurrentRequest().getMethod().equals("DELETE")) {
> - exception = new ResourceNotFoundException(message, exception);
> - }
> + exception = new ResourceNotFoundException(message, exception);
why?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874956
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Resource {
I bet you loved creating this :P
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874586
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +/**
> + * Options to customize attach
> + */
> +public class AttachOptions extends BaseHttpRequestOptions {
> +
> + public static final AttachOptions NONE = new AttachOptions();
remove this
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875005
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
> +
> + try {
> + PEMParser pemParser = new PEMParser(new StringReader(privateKey));
> + Object object = pemParser.readObject();
> + if (Security.getProvider("BC") == null) {
> + Security.addProvider(new BouncyCastleProvider());
How do you know you'll have this available? Does Docker depend on the BouncyCastle driver? Do you rely on the ssh driver dependency for this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21865029
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #395](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/395/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64814677
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> throw propagate(e);
> }
> }
> +
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
Why is the `password` parameter an array?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21864940
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + */
> +package org.jclouds.docker.domain;
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class Resource {
> +
> + public abstract String resource();
> +
> + @SerializedNames({ "Resource" })
> + public static Resource create(String resource) {
> + return new AutoValue_Resource(resource);
> + }
yep
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21004802
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> +
> + public void testKillContainer() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(204));
> + DockerApi dockerApi = api(server.getUrl("/"));
> + ContainerApi api = dockerApi.getContainerApi();
> + try {
> + api.kill("1");
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/containers/1/kill");
> + } finally {
> + dockerApi.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testKillNonExistingContainer() throws Exception {
please go through and delete any test like this that isn't on a method you declared a fallback on.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875422
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
> +
> + try {
> + PEMParser pemParser = new PEMParser(new StringReader(privateKey));
> + Object object = pemParser.readObject();
> + if (Security.getProvider("BC") == null) {
> + Security.addProvider(new BouncyCastleProvider());
added bouncycastle provider explicitly
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r22328070
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + * @param containerId The id of the container to be unpaused.
> + */
> + @Named("container:unpause")
> + @POST
> + @Path("/containers/{id}/unpause")
> + void unpause(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
> + *
> + * @param containerId The id of the container to be attached.
> + */
> + @Named("container:attach")
> + @POST
> + @Path("/containers/{id}/attach")
> + InputStream attach(@PathParam("id") String containerId);
Not sure I understand you here. Sorry the docker doc says:
```
Example response:
HTTP/1.1 200 OK
Content-Type: application/vnd.docker.raw-stream
{{ STREAM }}
```
what do you suggest instead of `InputStream` ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21005834
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> return this;
> }
>
> - public Builder networkDisabled(boolean networkDisabled) {
> - this.networkDisabled = networkDisabled;
> + public Builder securityOpts(List<String> securityOpts) {
> + this.securityOpts = ImmutableList.copyOf(checkNotNull(securityOpts, "securityOpts"));
weird indent. also this class is inconsistent on safe copies. as auto will likely generate builders and these won't do safe copies, better to just not do it, and ensure whoever is calling this safe copies in.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874510
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
> +
> + try {
> + PEMParser pemParser = new PEMParser(new StringReader(privateKey));
> + Object object = pemParser.readObject();
> + if (Security.getProvider("BC") == null) {
> + Security.addProvider(new BouncyCastleProvider());
I think
```
<groupId>org.apache.jclouds.driver</groupId>
<artifactId>jclouds-sshj</artifactId>
```
is giving us the bouncy-caste provider in the classpath
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r21886316
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + assertRequestHasCommonFields(server.takeRequest(), "POST", "/commit");
> + } finally {
> + dockerApi.close();
> + server.shutdown();
> + }
> + }
> +
> + public void testCommitNonExistingContainer() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(404));
> + DockerApi dockerApi = api(server.getUrl("/"));
> + ContainerApi api = dockerApi.getContainerApi();
> + try {
> + api.commit(CommitOptions.NONE);
> + fail("Commit container must fail on 404");
> + } catch (ResourceNotFoundException ex) {
don't test default fallbacks. This distracts the reader. Only test code you wrote.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875350
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <ad...@gmail.com>.
Andrea, so I would put a documentation or code link which explains why
docker needs tls 1.1 (vs 1.0 or 1.2). In hindsight, it may be better to use
system property for now, as this whole approach is temporary.
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1917](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1917/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64666473
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Andrea Turli <no...@github.com>.
I'll probably be able to close that later this week or during weekend.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-66356676
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> throw propagate(e);
> }
> }
> +
> + private static X509Certificate getCertificate(String certificate) {
> + try {
> + return (X509Certificate) CertificateFactory.getInstance("X.509").generateCertificate(
> + new ByteArrayInputStream(certificate.getBytes(Charsets.UTF_8)));
> + } catch (CertificateException ex) {
> + throw new RuntimeException("Invalid certificate", ex);
> + }
> + }
> +
> + private static PrivateKey getKey(String privateKey, String... password) {
Hmmm still an array. Does it really need to be an array?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r22926268
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.http.okhttp.OkHttpClientSupplier;
> +
> +import com.google.common.collect.ImmutableList;
> +import com.squareup.okhttp.ConnectionSpec;
> +import com.squareup.okhttp.OkHttpClient;
> +import com.squareup.okhttp.TlsVersion;
> +import javax.inject.Inject;
> +import javax.inject.Singleton;
> +
> +@Singleton
> +public class DockerOkHttpClientSupplier implements OkHttpClientSupplier {
> +
> + private final SSLContextWithKeysSupplier sslContextWithKeysSupplier;
> +
> + @Inject
> + private DockerOkHttpClientSupplier(SSLContextWithKeysSupplier sslContextWithKeysSupplier) {
Make it package private by removint the `private` modifier:
```java
@Inject
DockerOkHttpClientSupplier(SSLContextWithKeysSupplier sslContextWithKeysSupplier) {
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r22926081
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1915](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1915/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64535341
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> @@ -63,6 +63,7 @@ public SSLContext get() {
> kmf.init(keyStore.get(), keyStorePassword.toCharArray());
> SSLContext sc = SSLContext.getInstance("TLS");
> sc.init(kmf.getKeyManagers(), trustManager, new SecureRandom());
> + System.setProperty("https.protocols", "TLSv1");
find a better way, this is super dodgy
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875190
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #1925](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1925/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64769851
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + @POST
> + @Path("/containers/{id}/unpause")
> + void unpause(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
> + *
> + * @param containerId The id of the container to be attached.
> + */
> + @Named("container:attach")
> + @POST
> + @Path("/containers/{id}/attach")
> + InputStream attach(@PathParam("id") String containerId);
> +
> + /**
> + * Attach to a container
please don't boiler-plate docs like this. if you read this, you'll see how it adds zero value
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20874876
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> public void testStopContainer() {
> api().stopContainer(container.id());
> assertFalse(api().inspectContainer(container.id()).state().running());
> }
>
> + @Test(dependsOnMethods = "testStopContainer")
> + public void testRestartContainer() {
> + api().restart(container.id(), RestartOptions.NONE);
> + assertTrue(api().inspectContainer(container.id()).state().running());
> + }
> +
> + @Test(dependsOnMethods = "testRestartContainer")
> + public void testWaitContainer() {
> + api().stopContainer(container.id(), StopOptions.Builder.t("1"));
> + assertEquals(api().wait(container.id()).statusCode(), -1);
> + }
> +
> + @Test(dependsOnMethods = "testWaitContainer", expectedExceptions = NullPointerException.class)
NullPointerException is never something we should expect. make the test assertNull or something.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875291
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
Summary of long offline conversation. Docker did change and set a floor wrt
TLS. Andrea will document the actual requirement and the corresponding
docker issue in comments before merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64745695
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #390](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/390/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64664879
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
Lots of cleanup needed, similar comments to things made in GCE. As a committer, hoping you can pay more attention so that you can tell other people the things I'm telling you.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64430397
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
OK SSLv3 is a separate issue than docker. Please don't put distracting
changes into a pull request to support latest docker.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113#issuecomment-64699183
Re: [jclouds-labs] support for docker 1.3.2 (#113)
Posted by Adrian Cole <no...@github.com>.
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.docker.options;
> +
> +import org.jclouds.http.options.BaseHttpRequestOptions;
> +
> +public class StopOptions extends BaseHttpRequestOptions {
same comment
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/113/files#r20875152