Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

acl for zookeeper is added #2258

Merged
merged 1 commit into from
Jan 15, 2016
Merged

acl for zookeeper is added #2258

merged 1 commit into from
Jan 15, 2016

Conversation

fjy
Copy link
Contributor

@fjy fjy commented Jan 13, 2016

#2190 with tests and docs

@fjy fjy mentioned this pull request Jan 13, 2016
Closed
xvrl
xvrl reviewed Jan 13, 2016
View reviewed changes
docs/content/configuration/index.md
@@ -40,6 +40,7 @@ We recommend just setting the base ZK path and the ZK service host, but all ZK p
|--------|-----------|-------|
|`druid.zk.service.sessionTimeoutMs`|ZooKeeper session timeout, in milliseconds.|`30000`|
|`druid.zk.service.compress`|Boolean flag for whether or not created Znodes should be compressed.|`true`|
|`druid.zk.service.acl`|Boolean flag for whether or not to enable ACL security for ZooKeeper.|`false`|
Copy link
Member

Choose a reason for hiding this comment

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

can we specify in the documentation what permissions are set for zNodes when acl is enabled?

@fjy
Copy link
Contributor Author

fjy commented Jan 13, 2016

@drcrallen @xvrl addressed comments

fjy
fjy reviewed Jan 13, 2016
View reviewed changes
server/src/main/java/io/druid/curator/CuratorConfig.java
return enableAcl;
}

public void setEnableAcl(Boolean enableAcl)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drcrallen if someone typos a config, you want an error thrown versus a default value set

Copy link
Contributor

Choose a reason for hiding this comment

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

allright that's reasonable. Can it be Preconditions.notNull(enableAcl, "enableAcl") then?

Copy link
Contributor

Choose a reason for hiding this comment

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

also a unit test to verify the behavior would be awesome. (but IMHO not required)

@drcrallen
Copy link
Contributor

We have a M but not an N in the individual CLA forms. And since I don't have a github name to work off of can you track down Nikita Geer and make certain they have a CLA?

@fjy
Copy link
Contributor Author

fjy commented Jan 13, 2016

@drcrallen there is a typo for the CLA

@fjy
Copy link
Contributor Author

fjy commented Jan 13, 2016

the 'M' is the person

@fjy fjy closed this Jan 13, 2016
@fjy fjy reopened this Jan 13, 2016
fjy
fjy reviewed Jan 13, 2016
View reviewed changes
server/src/main/java/io/druid/curator/CuratorConfig.java
@@ -66,4 +69,14 @@ public void setEnableCompression(Boolean enableCompression)
{
this.enableCompression = enableCompression;
}

public boolean isEnableAcl()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drcrallen i can't seem to change this to isEnableAcl() without unit tests failing

Copy link
Contributor

Choose a reason for hiding this comment

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

try @JsonIgnore

@genevien
Copy link

@fjy thank you for your help.

As for "We have a M but not an N in the individual CLA forms." : in our country we have two forms of my name: Mikita and Nikita. If it's a blocker, tell me how to fix it.

@drcrallen
Copy link
Contributor

@genevien Sorry for the inconvenience, I have your gmail on file with your git user name with an 'M', but I need an entry whose email and name matches the one in the commit ('N' at the pleeco address). The easiest way to remedy this would be to submit another CLA http://druid.io/community/cla.html with a name and email that match the commit information at bae2463 ('N' at pleeco).

Thank you for your help in the matter and sorry for the confusion.

@genevien
Copy link

@drcrallen Ok, I got it. I've submitted it. Could you have a look, is it ok now?

@drcrallen
Copy link
Contributor

@genevien Awesome thanks! Looks good

xvrl
xvrl reviewed Jan 13, 2016
View reviewed changes
docs/content/configuration/index.md
@@ -40,6 +40,7 @@ We recommend just setting the base ZK path and the ZK service host, but all ZK p
|--------|-----------|-------|
|`druid.zk.service.sessionTimeoutMs`|ZooKeeper session timeout, in milliseconds.|`30000`|
|`druid.zk.service.compress`|Boolean flag for whether or not created Znodes should be compressed.|`true`|
|`druid.zk.service.acl`|Boolean flag for whether or not to enable ACL security for ZooKeeper. If ACL is enabled, zNode creators will have all permissions.|`false`|
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible this might break indexing service? The overlord currently has to delete nodes created by peons. Is that still possible if only the node creator has permissions to those nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. @genevien do you know?

Copy link

Choose a reason for hiding this comment

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

@xvrl @fjy Inconsistency in acl config might cause some problems. I assume that person who enables it for one service does it consciously and also enables it for other services. Some warning may be added in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@genevien This config will go in the common runtime properties file so it should be applied to every node in Druid. Do you know if Druid servers on different IPs creating Znodes will cause any issues?

Copy link

Choose a reason for hiding this comment

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

@fjy in our environment we have separate properties files for each node, that's why I've mentioned it. Could you give more details for the situation you are asking about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@genevien ah, usually we recommend having a common.runtime.properties file that holds the configuration that is common to the cluster (ZK IP, metastore location, deep storage location, etc). This common configuration file is copied and included in the classpath of every node. This new ACL config should be a part of the common configuration. It should like though, with proper configuration, things will work.

Copy link

Choose a reason for hiding this comment

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

@fjy ok, thank you. In theory and in my environment it works, so we have to wait until someone need it and try it in the future.

@drcrallen
Copy link
Contributor

👍

@xvrl
Copy link
Member

xvrl commented Jan 15, 2016

👍

@genevien It would be nice to have an example of what how to use this in practice, I'm not that familiar with how ZK ACLs work. Maybe we can add a short description of a use-case in the docs in a separate PR?

@dclim
Copy link
Contributor

dclim commented Jan 15, 2016

👍

dclim added a commit that referenced this pull request Jan 15, 2016
@dclim
Merge pull request #2258 from fjy/acl-zk
34cd8f8 
acl for zookeeper is added
@dclim dclim merged commit 34cd8f8 into apache:master Jan 15, 2016
@fjy fjy deleted the acl-zk branch January 15, 2016 20:09
@fjy
Copy link
Contributor Author

fjy commented Jan 15, 2016

@genevien Can you make sure you followed the instructions here: https://groups.google.com/forum/#!topic/druid-development/1Zs3Vd9LHCQ? Want to make sure you get credit for your contribution

@genevien
Copy link

@xvrl as I've mentioned before I have configured zookeeper server according to https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL (I've used JAAS configuration file: DIGEST-MD5 authentication) and I have run druid process setting
"-Djava.security.auth.login.config=/path/to/client/jaas/file.conf"
Which doc should be updated?

@genevien
Copy link

@fjy done, thank you!

@fjy fjy added this to the 0.9.0 milestone Feb 4, 2016
@fjy fjy mentioned this pull request Feb 5, 2016
Closed
@fjy fjy added the Feature label Feb 5, 2016
@genevien
Copy link

@fjy Hi, I just wonder if I have followed all instructions in order to get credit for the contribution?

@drcrallen
Copy link
Contributor

@genevien you totally do, the oversight is looking at the PR author rather than the commit author. Clerical mistake and sorry for that :)

@genevien
Copy link

@drcrallen thanks a lot)

seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Jan 10, 2020
@navis
apache#2258 Lessen gc pressure of coordinator with a lot of segments
5cc542f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@fjy @drcrallen @genevien @xvrl @dclim @gianm