You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by cfriaszapater <gi...@git.apache.org> on 2018/04/17 10:31:56 UTC
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
GitHub user cfriaszapater opened a pull request:
https://github.com/apache/storm/pull/2636
1.1.x branch - Add SSL functionality
Add functionality to connect to cassandra with SSL enabled (encryption and mutual authentication).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/OrwellGroup/storm 1.1.x-branch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2636.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2636
----
commit e105977f50d4f67ce6a6b79b2588a209b59e90ef
Author: c.friaszapater <c....@...>
Date: 2018-03-09T15:59:04Z
NEXG-1828 Enable SSL support
commit acffeafc2437f912c14d0bce5055f5dd99732046
Author: c.friaszapater <c....@...>
Date: 2018-03-09T16:31:11Z
NEXG-1828 Enable SSL support
commit ab3cdc35816faaed09cb748b5f3d8f09751b0b12
Author: c.friaszapater <c....@...>
Date: 2018-03-14T11:54:09Z
Ignore .checkstyle
----
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater commented on the issue:
https://github.com/apache/storm/pull/2636
Hi danny0405,
In response to HeartSaVioR, I said:
> I'll try to do that pull request in the future to master branch.
> I did this to 1.1.x branch because we use it in our organization.
So, I understand I should close this pull request to 1.1.x branch, and do one to master branch.
Anyway, we use 1.1 branch version of storm-cassandra with the changes, and they work fine. I don't know why other storm modules build fails. I will try on master branch if I have time...
Regards.
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater commented on the issue:
https://github.com/apache/storm/pull/2636
Hi HeartSaVioR,
I saw your comment and understand the reason. Just that I haven't had time to do it yet, as I had problems building the full storm project master branch locally. I'll try to do that pull request in the future to master branch.
I did this to 1.1.x branch because we use it in our organization.
Regards.
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r191669691
--- Diff: external/storm-cassandra/README.md ---
@@ -24,6 +24,12 @@ The following properties may be passed to storm configuration.
| **cassandra.retryPolicy** | - | DefaultRetryPolicy |
| **cassandra.reconnectionPolicy.baseDelayMs** | - | 100 (ms) |
| **cassandra.reconnectionPolicy.maxDelayMs** | - | 60000 (ms) |
+| **cassandra.ssl.security.protocol** | eg: null, SSL, SASL_SSL | |
--- End diff --
Can we let the `| ` in the final line vertically alignment ?
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2636
@cfriaszapater
Thanks for the contribution. Basically we receive PRs which go first to the master branch, and additional PRs (against 1.x-branch) which is expected to not cleanly ported back.
Could you craft a pull request against master branch?
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater commented on the issue:
https://github.com/apache/storm/pull/2636
Ah, that's strange, because I did not change anything out of external/storm-cassandra. So that should be failing before this pull request. Anyway, I'll try what you say, thanks.
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater commented on the issue:
https://github.com/apache/storm/pull/2636
Could someone provide help with the failed checks? I don't know how to fix them, eg:
[ERROR] Failed to execute goal org.apache.rat:apache-rat-plugin:0.12:check (default) on project storm: Too many files with unapproved license: 1 See RAT report in: /home/travis/build/apache/storm/target/rat.txt -> [Help 1]
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2636
@cfriaszapater The failure is because the integration test POM is pointing at Storm 1.2.0-SNAPSHOT, but this branch has Storm at version 1.1.3-SNAPSHOT. You should be able to just replace the version string here https://github.com/apache/storm/blob/1.1.x-branch/integration-test/pom.xml#L25.
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2636
The RAT plugin failure is because the SslProps file doesn't have the Apache license at the top
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r183059691
--- Diff: external/storm-cassandra/.gitignore ---
@@ -0,0 +1 @@
+/.checkstyle
--- End diff --
Why to add checkstyle here?
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater commented on the issue:
https://github.com/apache/storm/pull/2636
Fixed requested issues.
I did not add a test case for this pull request because I could not find any test cases in storm-cassandra in this branch. I tested the changes in topologies not in this project.
Now it's failing somewhere not related to this pull request, any advice? Eg:
[ERROR] Non-resolvable parent POM for org.apache.storm:storm-integration-test:[unknown-version]: Failure to find org.apache.storm:storm:pom:1.2.0-SNAPSHOT in https://oss.sonatype.org/content/repositories/snapshots/ was cached in the local repository, resolution will not be reattempted until the update interval of sonatype-snapshots has elapsed or updates are forced and 'parent.relativePath' points at wrong local POM @ line 22, column 13 -> [Help 2]
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r191670022
--- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/client/SslProps.java ---
@@ -0,0 +1,92 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (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.apache.storm.cassandra.client;
+
+/**
+ * Properties needed for enabling SSL connection to Cassandra.<br/>
+ *
+ * @author c.friaszapater
+ *
--- End diff --
Should remove the author name is the doc.
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/2636
@cfriaszapater
You can see the clojure test cases reports and logs in module targets dir, also the check style plugin logs.
You can see the Java Junit logs in the console.
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r191669779
--- Diff: external/storm-cassandra/README.md ---
@@ -24,6 +24,12 @@ The following properties may be passed to storm configuration.
| **cassandra.retryPolicy** | - | DefaultRetryPolicy |
| **cassandra.reconnectionPolicy.baseDelayMs** | - | 100 (ms) |
| **cassandra.reconnectionPolicy.maxDelayMs** | - | 60000 (ms) |
+| **cassandra.ssl.security.protocol** | eg: null, SSL, SASL_SSL | |
+| **cassandra.ssl.keystore.path** | path to keystore | |
+| **cassandra.ssl.keystore.password** | keystore password | |
+| **cassandra.ssl.truststore.path** | path to truststore | |
+| **cassandra.ssl.truststore.password** | truststore password | |
+
--- End diff --
Same as above
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/2636
@cfriaszapater
You should first build and install your dependencies modules successfully.
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r191670402
--- Diff: integration-test/pom.xml ---
@@ -22,7 +22,7 @@
<parent>
<artifactId>storm</artifactId>
<groupId>org.apache.storm</groupId>
- <version>1.2.0-SNAPSHOT</version>
+ <version>1.1.3-SNAPSHOT</version>
--- End diff --
Why just make the version lower?
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater closed the pull request at:
https://github.com/apache/storm/pull/2636
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r183059386
--- Diff: external/storm-cassandra/README.md ---
@@ -24,6 +24,12 @@ The following properties may be passed to storm configuration.
| **cassandra.retryPolicy** | - | DefaultRetryPolicy |
| **cassandra.reconnectionPolicy.baseDelayMs** | - | 100 (ms) |
| **cassandra.reconnectionPolicy.maxDelayMs** | - | 60000 (ms) |
+| **cassandra.ssl.security.protocol** | - | |
+| **cassandra.ssl.keystore.path** | - | |
+| **cassandra.ssl.keystore.password** | - | |
+| **cassandra.ssl.truststore.path** | - | |
+| **cassandra.ssl.truststore.password** | - | |
+
--- End diff --
Can we have detailed msgs here?
---
[GitHub] storm issue #2636: 1.1.x branch - Add SSL functionality
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on the issue:
https://github.com/apache/storm/pull/2636
@cfriaszapater
Ok, look forward to your master branch patch.
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by cfriaszapater <gi...@git.apache.org>.
Github user cfriaszapater commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r184284475
--- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/client/ClusterFactory.java ---
@@ -67,7 +83,57 @@ protected Cluster make(Map<String, Object> stormConf) {
.setConsistencyLevel(cassandraConf.getConsistencyLevel());
cluster.withQueryOptions(options);
+ SslProps sslProps = cassandraConf.getSslProps();
+ configureIfSsl(cluster, sslProps);
return cluster.build();
}
+
+ /**
+ * If sslProps passed, then set SSL configuration in clusterBuilder.
+ *
+ * @param clusterBuilder cluster builder
+ * @param sslProps SSL properties
+ * @return
+ */
+ private static Builder configureIfSsl(Builder clusterBuilder, SslProps sslProps) {
+ if (sslProps == null || !sslProps.isSsl()) {
+ return clusterBuilder;
+ }
+
+ SSLContext sslContext = getSslContext(sslProps.getTruststorePath(), sslProps.getTruststorePassword(), sslProps.getKeystorePath(),
+ sslProps.getKeystorePassword());
+
+ // Default cipher suites supported by C*
--- End diff --
no need for it, I'll remove it
---
[GitHub] storm pull request #2636: 1.1.x branch - Add SSL functionality
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on a diff in the pull request:
https://github.com/apache/storm/pull/2636#discussion_r184219125
--- Diff: external/storm-cassandra/src/main/java/org/apache/storm/cassandra/client/ClusterFactory.java ---
@@ -67,7 +83,57 @@ protected Cluster make(Map<String, Object> stormConf) {
.setConsistencyLevel(cassandraConf.getConsistencyLevel());
cluster.withQueryOptions(options);
+ SslProps sslProps = cassandraConf.getSslProps();
+ configureIfSsl(cluster, sslProps);
return cluster.build();
}
+
+ /**
+ * If sslProps passed, then set SSL configuration in clusterBuilder.
+ *
+ * @param clusterBuilder cluster builder
+ * @param sslProps SSL properties
+ * @return
+ */
+ private static Builder configureIfSsl(Builder clusterBuilder, SslProps sslProps) {
+ if (sslProps == null || !sslProps.isSsl()) {
+ return clusterBuilder;
+ }
+
+ SSLContext sslContext = getSslContext(sslProps.getTruststorePath(), sslProps.getTruststorePassword(), sslProps.getKeystorePath(),
+ sslProps.getKeystorePassword());
+
+ // Default cipher suites supported by C*
--- End diff --
Any purpose for this comment being retained?
---