You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by liu-zhaokun <gi...@git.apache.org> on 2017/07/13 07:57:18 UTC
[GitHub] storm pull request #2208: [STORM-2627] The annotation of storm.zookeeper.top...
GitHub user liu-zhaokun opened a pull request:
https://github.com/apache/storm/pull/2208
[STORM-2627] The annotation of storm.zookeeper.topology.auth.scheme in Config.java is wrong
[https://issues.apache.org/jira/browse/STORM-2627](https://issues.apache.org/jira/browse/STORM-2627)
The annotation of "storm.zookeeper.topology.auth.scheme" in Config.java is wrong.As StormSubmitter.java,line 91, says "STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME is should always be set to digest.", the annotation of "storm.zookeeper.topology.auth.scheme" in Config.java should be consistent with it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/liu-zhaokun/storm master07131533
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2208.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 #2208
----
commit 64b9913e108922a5b85249bf1647e235b0d6dcb0
Author: liuzhaokun <li...@zte.com.cn>
Date: 2017-07-13T07:55:23Z
[STORM-2627] The annotation of storm.zookeeper.topology.auth.scheme in Config.java is wrong
----
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@HeartSaVioR
OK, I will do it.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@HeartSaVioR
I have modify this PR follow @revans2 suggestion.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2208
I think the best way to avoid misleading is just removing the configuration and set the value as constant. We don't need to provide an option to user while the value is actually fixed.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2208
+1 for the change. @revans2 How about the last change?
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2208
@liu-zhaokun @Ethanlm is not a committer yet but I will try to pull it in.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2208
@revans2 Thanks for the detailed explanation. I'm OK to leave this as it is, and maybe better to just add comment that it is the internal config and user shouldn't set it.
@liu-zhaokun Sorry to guide to the wrong way. Could you roll back the change and follow @revans2 suggestion?
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2208
@liu-zhaokun I think the comment there meant to say by default it will be "No Authentication". I.e Its users responsibility to set to digest in a secure clusters. But since the default settings for non-secure the comment looks ok to me.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@Ethanlm
Hi,Ethan. @HeartSaVioR and @revans2 both approved this PR,could you help me merge it?
---
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] storm pull request #2208: [STORM-2627] The annotation of storm.zookeeper.top...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2208
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2208
Sorry I have been out of town or I would have joined the conversation sooner.
Yes @HeartSaVioR is correct. The code is really confusing and overly complicated. I comes from when we were adding in security to storm. It is a long story where we ran into several bugs/limitations in ZK itself that prevented us from doing what we initial wanted to do. It also comes from the really confusing way that ZK handles authentication.
ZK supports both SASL authentication which is per connection, and a special digest authentication with can be per command and can override the connection authentication if any. (don't confuse this special built in ZK digest authentication with the SASL digest authentication which ZK also supports, as they are different) SASL authentication with ZK is handled through the jaas conf. There is a "Client" section that says how you want to authenticate with the zookeeper servers. Using kerberos for this is the recommended way to authenticate ZK as it very secure. STORM_ZOOKEEPER_AUTH_SCHEME allows you to use the built in ZK digest auth scheme in the daemons. This is really only there for testing purposes. Although it could work in production if you wanted it to, it just will be no where near as secure as kerberos is.
One of the issues we ran into with allowing the workers to communicate with ZK was that the workers needed some way to authenticate with ZK that didn't expire. We could force everyone to send a TGT to the worker and then use that to authenticate with ZK, but TGTs expire and so users would have to push a new one periodically. So we made the decision that we would use the built in ZK digest like a ticket. We would randomly generate a 128 bit number as the secret, and then let nimbus setup the ACLs in ZK to only allow a user with that 128 bit secret to access the data.
That is how it currently works. We didn't really like that and wanted to leave open the possibility of using a different auth scheme with ZK in the future. That is why we have the configs as a string and not a boolean. If we want to update the documentation for the cofnigs we can. But I would say for now just mark them as being internal configs relating to security that the end user should not actually set.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2208
@HeartSaVioR wouldn't that be an issue incase of non-secure cluster if we are defaulting to "digest"?
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@HeartSaVioR
I have modified this PR according to your opinion,and it has passed all the test.Could you help me merge it?Thank you very much.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@HeartSaVioR
I am so sorry to bother you.Do you have time to help me review it?
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
Can one of the admins verify this patch?
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@harshach
It has always been the case.In other words,the value of this configuration is digest all the time.You can see StormSubmitter.java,line 92.It doesn't matter even that is in case of non-secure cluster.
---
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] storm pull request #2208: [STORM-2627] The annotation of storm.zookeeper.top...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2208#discussion_r128345781
--- Diff: storm-client/src/jvm/org/apache/storm/Config.java ---
@@ -967,12 +967,6 @@
public static final String STORM_ZOOKEEPER_SUPERACL = "storm.zookeeper.superACL";
/**
- * The topology Zookeeper authentication scheme to use, e.g. "digest". Defaults to no authentication.
- */
- @isString
- public static final String STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME="storm.zookeeper.topology.auth.scheme";
-
- /**
--- End diff --
-1 the problem with deleting these is that we don't get any config checks that it is a string.
We could move it to a different file so that it is hidden from end users.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2208
@liu-zhaokun I didn't mean we should replace configuration with normal string. I meant if the value is constant, don't add it to Storm configuration (even config map) and use the constant value directly from all usages.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@HeartSaVioR
Thanks for your reply.I will remove it from Config.java.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2208
OK now I'm seeing how it works...
From topology side it's always set to "digest" from Storm Submitter, so it is not effectively configurable from topology level.
From Daemon (Nimbus, Supervisor, etc) side, it **determines** whether the cluster is using ZK auth via STORM_ZOOKEEPER_AUTH_SCHEME (cluster-wise ZK auth).
Especially, Nimbus just **removes** STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME (topology-wise ZK auth) from topology config map when topology is submitted but STORM_ZOOKEEPER_AUTH_SCHEME is not set.
The tricky part is a ZookeeperAuthInfo class. If STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME is set to cluster configuration, ZookeeperAuthInfo uses this. That means, if ZookeeperAuthInfo class is used from Daemon side, topology-wise configuration will be used. (I feel this behavior could be misleading.) So it shouldn't be set to "digest" with no auth.
Long story in short, javadoc comment is effectively right even though it is really tricky. I'm not an expert of ZK auth and wondering why cluster wise configuration and topology wise configuration co-exist, given that it connects to same ZK. Could someone knowing about ZK auth explain this?
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@harshach
Thanks for your reply.You can see StormSubmitter.java,line 91.STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME is should always be set to digest.It can't be and won't be other value.So,I think we should declare “"storm.zookeeper.topology.auth.scheme” should always be set to digest.We shouldn't mislead others.
---
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] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2208
@haitaoyao
I am so sorry to bother you.Do you have time to help me review it?
---
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.
---