You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by RaghavendraNandagopal <gi...@git.apache.org> on 2014/09/05 01:17:55 UTC
[GitHub] incubator-storm pull request: STORM-430: Allow netty SASL to suppo...
GitHub user RaghavendraNandagopal opened a pull request:
https://github.com/apache/incubator-storm/pull/250
STORM-430: Allow netty SASL to support encryption as well
Hi Bobby,
I have made the changes for Netty SASL to support encryption through wrap and unwrap functionality. Please review and let me know if you require additional changes.
Thanks,
Raghavendra Nandagopal
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/RaghavendraNandagopal/incubator-storm security
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/250.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 #250
----
commit 63d5be871dab1ae13f7083aed29470f4de2e9a11
Author: Raghavendra Nandagopal <sp...@gmail.com>
Date: 2014-09-04T23:10:39Z
STORM-430: Allow netty SASL to support encryption as well
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-430: Allow netty SASL to suppo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/250#discussion_r17376755
--- Diff: storm-core/src/jvm/backtype/storm/messaging/netty/Client.java ---
@@ -333,6 +348,27 @@ private void flushRequest(Channel channel, final MessageBatch requests) {
if (requests == null)
return;
+ // Wait until the netty authentication completes
+ boolean isNettyAuth = (Boolean) this.storm_conf
+ .get(Config.STORM_MESSAGING_NETTY_AUTHENTICATION);
+ if (isNettyAuth && authenticatedSignal != null) {
+ try {
+ LOG.debug("sasl Waiting until the netty authentication completes");
+ authenticatedSignal.await();
+ authenticatedSignal = null;
+ LOG.debug("sasl Netty authentication completed, proceeding further");
+ } catch (InterruptedException e) {
+ LOG.error(
+ "failed to send requests to " + remote_addr.toString()
+ + ": ", e.getMessage());
--- End diff --
I think we just want e, not e.getMessage() here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-430: Allow netty SASL to suppo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/250#issuecomment-55190671
For the most part things look really good.
There are a number of whitespace issues all throughout. Mostly looks like tabs are in there when there should be spaces. This if fairly minor, but I would like to fix them.
I would also like to see some basic sanity unit tests added as well. I wrote a couple that all passed inside storm-core/test/clj/backtype/storm/messaging/netty_unit_test.clj they are all more or less like test-basic but with auth, auth-conf, and auth-int turned on. Something like the following.
```
(deftest test-basic-auth
(let [req_msg (String. "0123456789abcdefghijklmnopqrstuvwxyz")
storm-conf {STORM-MESSAGING-TRANSPORT "backtype.storm.messaging.netty.Context"
STORM-MESSAGING-NETTY-AUTHENTICATION true
STORM-MESSAGING-NETTY-BUFFER-SIZE 1024
STORM-MESSAGING-NETTY-MAX-RETRIES 10
STORM-MESSAGING-NETTY-MIN-SLEEP-MS 1000
STORM-MESSAGING-NETTY-MAX-SLEEP-MS 5000
STORM-MESSAGING-NETTY-SERVER-WORKER-THREADS 1
STORM-MESSAGING-NETTY-CLIENT-WORKER-THREADS 1
STORM-MESSAGING-NETTY-PROTECTION "auth"
TOPOLOGY-NAME "testing"
STORM-ZOOKEEPER-TOPOLOGY-AUTH-PAYLOAD "shhhh:don'ttell"
}
context (TransportFactory/makeContext storm-conf)
server (.bind context nil port)
client (.connect context nil "localhost" port)
_ (.send client task (.getBytes req_msg))
iter (.recv server 0 0)
resp (.next iter)]
(is (= task (.task resp)))
(is (= req_msg (String. (.message resp))))
(.close client)
(.close server)
(.term context)))
```
Things are also starting to get kind of complex with the different classes, especially the Encoders/Decoders. It looks like there is some copy and paste code shared between them. Would it be possible to try and combine some of the code together again. Especially the part in MessageDecoder that appreas to be the same in ClientUnwrapMessageDecoder and ServerUnwrapMessageDecoder?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-430: Allow netty SASL to suppo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/250#discussion_r17376646
--- Diff: storm-core/src/jvm/backtype/storm/messaging/netty/Client.java ---
@@ -66,8 +67,14 @@
private AtomicLong flushCheckTimer;
private int flushCheckInterval;
private ScheduledExecutorService scheduler;
+
+ private CountDownLatch authenticatedSignal = null;
+
+ public CountDownLatch getAuthenticatedSignal() {
+ return authenticatedSignal;
--- End diff --
There appears to be some whitespace issues here. Probably tabs instead of spaces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: STORM-430: Allow netty SASL to suppo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/250#discussion_r17376657
--- Diff: storm-core/src/jvm/backtype/storm/messaging/netty/Client.java ---
@@ -66,8 +67,14 @@
private AtomicLong flushCheckTimer;
private int flushCheckInterval;
private ScheduledExecutorService scheduler;
+
+ private CountDownLatch authenticatedSignal = null;
+
+ public CountDownLatch getAuthenticatedSignal() {
+ return authenticatedSignal;
+ }
- @SuppressWarnings("rawtypes")
+ @SuppressWarnings("rawtypes")
--- End diff --
White space issue here too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---