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

acl for zookeeper is added #2258

Merged
merged 1 commit into from
Jan 15, 2016
Merged

acl for zookeeper is added #2258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/content/configuration/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


#### Path Configuration
Druid interacts with ZK through a set of standard path configurations. We recommend just setting the base ZK path, but all ZK paths that Druid uses can be overwritten to absolute paths.
Expand Down
18 changes: 17 additions & 1 deletion server/src/main/java/io/druid/curator/CuratorConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package io.druid.curator;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;

import javax.validation.constraints.Min;

Expand All @@ -37,6 +38,9 @@ public class CuratorConfig
@JsonProperty("compress")
private boolean enableCompression = true;

@JsonProperty("acl")
private boolean enableAcl = false;

public String getZkHosts()
{
return zkHosts;
Expand All @@ -57,13 +61,25 @@ public void setZkSessionTimeoutMs(Integer zkSessionTimeoutMs)
this.zkSessionTimeoutMs = zkSessionTimeoutMs;
}

public Boolean getEnableCompression()
public boolean getEnableCompression()
{
return enableCompression;
}

public void setEnableCompression(Boolean enableCompression)
{
Preconditions.checkNotNull(enableCompression, "enableCompression");
this.enableCompression = enableCompression;
}

public boolean getEnableAcl()
{
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)

{
Preconditions.checkNotNull(enableAcl, "enableAcl");
this.enableAcl = enableAcl;
}
}
37 changes: 32 additions & 5 deletions server/src/main/java/io/druid/curator/CuratorModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,23 @@
import com.google.inject.Provides;
import com.metamx.common.lifecycle.Lifecycle;
import com.metamx.common.logger.Logger;

import io.druid.guice.JsonConfigProvider;
import io.druid.guice.LazySingleton;

import org.apache.curator.framework.api.ACLProvider;
import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.framework.CuratorFrameworkFactory;
import org.apache.curator.framework.imps.DefaultACLProvider;
import org.apache.curator.retry.BoundedExponentialBackoffRetry;

import java.io.IOException;

import java.util.List;

import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.data.ACL;

/**
*/
public class CuratorModule implements Module
Expand All @@ -43,19 +52,22 @@ public void configure(Binder binder)
{
JsonConfigProvider.bind(
binder, "druid.zk.service",
CuratorConfig.class);
CuratorConfig.class
);
}

@Provides @LazySingleton
@Provides
@LazySingleton
public CuratorFramework makeCurator(CuratorConfig config, Lifecycle lifecycle) throws IOException
{
final CuratorFramework framework =
CuratorFrameworkFactory.builder()
.connectString(config.getZkHosts())
.sessionTimeoutMs(config.getZkSessionTimeoutMs())
.retryPolicy(new BoundedExponentialBackoffRetry(1000, 45000, 30))
.compressionProvider(new PotentiallyGzippedCompressionProvider(config.getEnableCompression()))
.build();
.retryPolicy(new BoundedExponentialBackoffRetry(1000, 45000, 30))
.compressionProvider(new PotentiallyGzippedCompressionProvider(config.getEnableCompression()))
.aclProvider(config.getEnableAcl() ? new SecuredACLProvider() : new DefaultACLProvider())
.build();

lifecycle.addHandler(
new Lifecycle.Handler()
Expand All @@ -78,4 +90,19 @@ public void stop()

return framework;
}

class SecuredACLProvider implements ACLProvider
{
@Override
public List<ACL> getDefaultAcl()
{
return ZooDefs.Ids.CREATOR_ALL_ACL;
}

@Override
public List<ACL> getAclForPath(String path)
{
return ZooDefs.Ids.CREATOR_ALL_ACL;
}
}
}
6 changes: 4 additions & 2 deletions server/src/test/java/io/druid/curator/CuratorConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@
public class CuratorConfigTest extends JsonConfigTesterBase<CuratorConfig>
{
@Test
public void testHostName() throws IllegalAccessException, NoSuchMethodException, InvocationTargetException
public void testSerde() throws IllegalAccessException, NoSuchMethodException, InvocationTargetException
{
propertyValues.put(getPropertyKey("host"),"fooHost");
propertyValues.put(getPropertyKey("host"), "fooHost");
propertyValues.put(getPropertyKey("acl"), "true");
testProperties.putAll(propertyValues);
configProvider.inject(testProperties, configurator);
CuratorConfig config = configProvider.get().get();
Assert.assertEquals("fooHost", config.getZkHosts());
Assert.assertEquals(true, config.getEnableAcl());
}
}