ZOOKEEPER-1260:Audit logging in ZooKeeper servers. by arshadmohammad · Pull Request #1105 · apache/zookeeper

arshadmohammad · 2019-10-02T17:54:58Z

Original PR: #338

eolivelli

Very interesting and useful.

I left a few comments, please take a look

eolivelli · 2019-10-02T21:05:22Z


 public class ZKUtil {
    private static final Logger LOG = LoggerFactory.getLogger(ZKUtil.class);
+    private static final Map<Integer, String> permCache = new HashMap<Integer, String>();


ConcurrentHashMap?

eolivelli · 2019-10-02T21:08:52Z

+
+## ZooKeeper Audit Logs
+
+Apache ZooKeeper supports audit logs form version 3.5.7. By default audit logs are disabled. To enable audit logs configure audit.enable=true in conf/zoo.cfg. Audit logs are not logged on all the ZooKeeper servers, but logged only on the servers where client is connected as depicted in below figure.


Typo: form -> from.

I think we should say 3.6.0

This PR I have raised for branch-3.5. After commit I will raise PR for master branch that time I will change this to 3.6.0. Branch releases are independent. I think this way we can be bit more accurate. In last PR was discussed but on second thought this would be the better way.
are you ok with 3.5.7 for branch-5 and 3.6..0 for master?

corrected typo

eolivelli · 2019-10-02T21:18:35Z


+    public String getUserName() {
+        if (principal == null || principal.isEmpty()) {
+            return System.getProperty("user.name", "<NA>");


You can sample the value as a static variable. You will save an access to the system properties map

eolivelli · 2019-10-02T21:23:06Z

+     * @return string representation of permissions
+     */
+    public static String getPermString(int perms) {
+        String permString = permCache.get(perms);


what about using computeIfAbsent?

eolivelli · 2019-10-02T21:23:57Z

+ */
+package org.apache.zookeeper.audit;
+
+public class AuditConstants {


final

Please also add a private constructor

eolivelli · 2019-10-02T21:26:57Z

+    }
+
+    private String getACLs(Request request) {
+        ByteBuffer reqData = request.request.duplicate();


what about 'slice'?

good point, done

eolivelli · 2019-10-02T21:27:25Z

+        String address = null;
+        String acls = null;
+        String createMode = null;
+        ByteBuffer reqData = request.request.duplicate();


eolivelli · 2019-10-02T21:28:23Z

     */
    static final ByteBuffer closeConn = ByteBuffer.allocate(0);

+    private static String loginUser = System.getProperty("user.name", "<NA>");


I think loginUser can not be marked as final

eolivelli · 2019-10-02T21:29:30Z

+/**
+ * Audit log performance reading
+ */
+public class AuditLogPerfReading {


eolivelli · 2019-10-02T21:30:55Z

+        return parentPath + "zNode" + i;
+    }
+
+    public static void main(String[] args) {


Is this program really needed? Or is it just a tool you used to profile this new code?

This program was just to see the performance impact of audit log. I added here so anybody can run and see the audit log impact on their environment. Also it is added test folder.

anmolnar

@arshadmohammad This is a very useful and long awaited feature. Thanks for working on this.
Without going into the implementation details, I believe it would be better to finalize and submit the code into master and we can talk about backporting to 3.5 after that. We have to ask the community, because 3.5 is already locked down for new features and release process of 3.6 has already been started.

anmolnar · 2019-10-04T09:57:05Z

In terms of the implementation: would you please make the ZKAuditLogger implementation pluggable similarly to zookeeper.clientCnxnSocket. Later we might start to work on Ranger integration and a pluggable audit logger would be very useful.

anmolnar · 2019-10-08T13:19:31Z

@arshadmohammad Fyi. I'm happy to take this over and continue working on it, if you don't have cycles for that. Please let me know.

arshadmohammad · 2019-10-11T07:09:11Z

... because 3.5 is already locked down for new features and release process of 3.6 has already been started.

I think it is locked for backward incompatible features, not for all features and this feature is not backward incompatible.

arshadmohammad · 2019-10-11T07:10:56Z

In terms of the implementation: would you please make the ZKAuditLogger implementation pluggable similarly to zookeeper.clientCnxnSocket. Later we might start to work on Ranger integration and a pluggable audit logger would be very useful.

Sure, I will make it configurable.

anmolnar · 2019-11-03T20:34:37Z

@arshadmohammad PR hasn't been updated for 24 days. How are you getting on?
I'm still happy to take this over and move forward.

arshadmohammad · 2019-11-04T15:23:21Z

@anmolnar, I will update the patch soon, hopefully by tomorrow. Will try to wind up this work by this weekend

arshadmohammad · 2019-11-05T17:19:44Z

Addressed all the comments and created PR for master branch.
#1133

arshadmohammad · 2019-11-12T06:46:51Z

Feature is merged in PR #1133

arshadmohammad added 3 commits September 23, 2019 00:29

ZOOKEEPER-1260:Audit logging in ZooKeeper Servers.

7d6c9cd

ZOOKEEPER-1260: Review comment fix

da20cac

Rebasing it

163d595

eolivelli requested changes Oct 2, 2019

View reviewed changes

ZOOKEEPER-1260:Fixed review comments

ba3a447

anmolnar requested changes Oct 4, 2019

View reviewed changes

arshadmohammad closed this Nov 12, 2019

arshadmohammad deleted the ZOOKEEPER-1260-AuditLog branch December 25, 2019 18:37


		## ZooKeeper Audit Logs

		Apache ZooKeeper supports audit logs form version 3.5.7. By default audit logs are disabled. To enable audit logs configure audit.enable=true in conf/zoo.cfg. Audit logs are not logged on all the ZooKeeper servers, but logged only on the servers where client is connected as depicted in below figure.

Uh oh!

Conversation

arshadmohammad commented Oct 2, 2019

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

anmolnar commented Oct 4, 2019

Uh oh!

anmolnar commented Oct 8, 2019

Uh oh!

arshadmohammad commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arshadmohammad commented Oct 11, 2019

Uh oh!

anmolnar commented Nov 3, 2019

Uh oh!

arshadmohammad commented Nov 4, 2019

Uh oh!

arshadmohammad commented Nov 5, 2019

Uh oh!

arshadmohammad commented Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arshadmohammad commented Oct 11, 2019 •

edited

Loading