ekaterinadimitrova2 commented on a change in pull request #1051:
URL: https://github.com/apache/cassandra/pull/1051#discussion_r676791413
##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -177,6 +195,16 @@ public synchronized void enable(AuditLogOptions
auditLogOptions) throws Configur
oldLogger.stop();
}
+ private void updateAuditOptions(final AuditLogOptions options, final
AuditLogFilter filter)
Review comment:
```suggestion
private void updateAuditLogOptions(final AuditLogOptions options, final
AuditLogFilter filter)
```
##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -148,6 +154,8 @@ public synchronized void disableAuditLog()
unregisterAsListener();
IAuditLogger oldLogger = auditLogger;
auditLogger = new NoOpAuditLogger(Collections.emptyMap());
+ // when we disable audit logging, we should also reset options so we
return default (from cassandra.yml)
Review comment:
```suggestion
// when we disable audit logging, we should also reset options so we
return default ones (from cassandra.yaml)
```
##########
File path: test/unit/org/apache/cassandra/audit/AuditLogFilterTest.java
##########
@@ -28,6 +28,22 @@
public class AuditLogFilterTest
{
+ @Test
+ public void testInputWithSpaces()
+ {
+ AuditLogOptions auditLogOptions = new AuditLogOptions.Builder()
+ .withIncludedKeyspaces(" ks, ks1, ks3, ")
+ .withEnabled(true)
+ .build();
Review comment:
nit: alignment
##########
File path: src/java/org/apache/cassandra/config/CassandraRelevantProperties.java
##########
@@ -159,6 +159,10 @@
*/
REPLACEMENT_ALLOW_EMPTY("cassandra.allow_empty_replace_address", "true"),
+ LOG_DIR("cassandra.logdir", "."),
+
+ LOG_DIR_AUDIT("cassandra.logdir.audit"),
+
Review comment:
I suggest we add a java doc for those two, similar to other properties.
##########
File path: src/java/org/apache/cassandra/audit/AuditLogFilter.java
##########
@@ -31,12 +30,12 @@
private static ImmutableSet<String> EMPTY_FILTERS = ImmutableSet.of();
- private final ImmutableSet<String> excludedKeyspaces;
- private final ImmutableSet<String> includedKeyspaces;
- private final ImmutableSet<String> excludedCategories;
- private final ImmutableSet<String> includedCategories;
- private final ImmutableSet<String> includedUsers;
- private final ImmutableSet<String> excludedUsers;
+ public final ImmutableSet<String> excludedKeyspaces;
+ public final ImmutableSet<String> includedKeyspaces;
+ public final ImmutableSet<String> excludedCategories;
+ public final ImmutableSet<String> includedCategories;
+ public final ImmutableSet<String> includedUsers;
+ public final ImmutableSet<String> excludedUsers;
Review comment:
I am not sure it is a good idea to directly make those `public `. If we
make a change, let it be at least to `protected`
##########
File path: src/java/org/apache/cassandra/service/StorageService.java
##########
@@ -5786,35 +5786,40 @@ public void disableAuditLog()
public void enableAuditLog(String loggerName, String includedKeyspaces,
String excludedKeyspaces, String includedCategories, String excludedCategories,
String includedUsers, String excludedUsers)
throws ConfigurationException, IllegalStateException
{
- enableAuditLog(loggerName, Collections.emptyMap(), includedKeyspaces,
excludedKeyspaces, includedCategories, excludedCategories, includedUsers,
excludedUsers);
+ enableAuditLog(loggerName, Collections.emptyMap(), includedKeyspaces,
excludedKeyspaces, includedCategories, excludedCategories, includedUsers,
excludedUsers,
+ Integer.MIN_VALUE, null, null, Long.MIN_VALUE,
Integer.MIN_VALUE, null);
}
public void enableAuditLog(String loggerName, Map<String, String>
parameters, String includedKeyspaces, String excludedKeyspaces, String
includedCategories, String excludedCategories,
String includedUsers, String excludedUsers)
throws ConfigurationException, IllegalStateException
{
- loggerName = loggerName != null ? loggerName :
DatabaseDescriptor.getAuditLoggingOptions().logger.class_name;
-
- Preconditions.checkNotNull(loggerName, "cassandra.yaml did not have
logger in audit_logging_option and not set as parameter");
-
Preconditions.checkState(FBUtilities.isAuditLoggerClassExists(loggerName),
"Unable to find AuditLogger class: "+loggerName);
Review comment:
Are you sure you don't want these two preconditions anymore?
##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -158,16 +179,26 @@ public synchronized void disableAuditLog()
*/
public synchronized void enable(AuditLogOptions auditLogOptions) throws
ConfigurationException
{
- // always reload the filters
- filter = AuditLogFilter.create(auditLogOptions);
-
- // next, check to see if we're changing the logging implementation; if
not, keep the same instance and bail.
- // note: auditLogger should never be null
Review comment:
Shouldn't we keep this comment?
##########
File path: test/unit/org/apache/cassandra/tools/nodetool/GetAuditLogTest.java
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.tools.nodetool;
+
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.cassandra.OrderedJUnit4ClassRunner;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.tools.ToolRunner;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@RunWith(OrderedJUnit4ClassRunner.class)
+public class GetAuditLogTest extends CQLTester
+{
+ @BeforeClass
+ public static void setup() throws Exception
+ {
+ startJMXServer();
+ }
+
+ @After
+ public void afterTest()
+ {
+ disableAuditLog();
+ }
+
+ @Test
+ public void getAuditLogTest()
+ {
+ testDefaultOutput(getAuditLog());
+
+ enableAuditLogSimple();
+ testChangedOutputSimple(getAuditLog());
+
+ enableAuditLogComplex();
+ testChangedOutputComplex(getAuditLog());
+
+ disableAuditLog();
+ testDefaultOutput(getAuditLog());
+ }
Review comment:
I feel we have four tests combined in one, can we split to make it
easier when we debug tests?
I appreciate the consistency with the `GetFullQueryLogTest` but I think they
had only two tests there.
##########
File path: src/java/org/apache/cassandra/tools/NodeProbe.java
##########
@@ -134,6 +138,7 @@
protected HintsServiceMBean hsProxy;
protected BatchlogManagerMBean bmProxy;
protected ActiveRepairServiceMBean arsProxy;
+ protected AuditLogManagerMBean aloProxy;
Review comment:
I guess you wanted to type `almProxy`, not `aloProxy`
##########
File path: src/java/org/apache/cassandra/audit/AuditLogOptions.java
##########
@@ -37,11 +46,242 @@
public String included_users = StringUtils.EMPTY;
public String excluded_users = StringUtils.EMPTY;
- /**
- * AuditLogs directory can be configured using `cassandra.logdir.audit` or
default is set to `cassandra.logdir` + /audit/
- */
- public String audit_logs_dir = System.getProperty("cassandra.logdir.audit",
-
System.getProperty("cassandra.logdir",".")+"/audit/");
+ public String audit_logs_dir;
+
+ public AuditLogOptions()
+ {
+ String auditLogDir =
CassandraRelevantProperties.LOG_DIR_AUDIT.getString();
+ String logDir = CassandraRelevantProperties.LOG_DIR.getString() +
"/audit";
+ Path path = auditLogDir == null ? Paths.get(logDir) :
Paths.get(auditLogDir);
+ audit_logs_dir = path.normalize().toString();
+ }
+
+ public static AuditLogOptions validate(final AuditLogOptions options)
throws ConfigurationException
+ {
+ // not validating keyspaces nor users on purpose,
+ // logging might be enabled on these entities before they exist
+ // so they are picked up automatically
+
+ validateCategories(options.included_categories);
+ validateCategories(options.excluded_categories);
+
+ // other fields in BinLogOptions are validated upon BinAuditLogger
initialisation
+
+ return options;
+ }
+
+ public static class Builder
+ {
+ private boolean enabled;
+ private ParameterizedClass logger;
+ private String includedKeyspaces;
+ private String excludedKeyspaces;
+ private String includedCategories;
+ private String excludedCategories;
+ private String includedUsers;
+ private String excludedUsers;
+ private String auditLogDir;
+ private int maxQueueWeight;
+ private int maxArchiveRetries;
+ private String rollCycle;
+ private String archiveCommand;
+ private boolean block;
+ private long maxLogSize;
+
+ public Builder()
+ {
+ this(new AuditLogOptions());
+ }
+
+ public Builder(final AuditLogOptions opts)
+ {
+ this.enabled = opts.enabled;
+ this.logger = opts.logger;
+ this.includedKeyspaces = opts.included_keyspaces;
+ this.excludedKeyspaces = opts.excluded_keyspaces;
+ this.includedCategories = opts.included_categories;
+ this.excludedCategories = opts.excluded_categories;
+ this.includedUsers = opts.included_users;
+ this.excludedUsers = opts.excluded_users;
+ this.auditLogDir = opts.audit_logs_dir;
+ this.maxQueueWeight = opts.max_queue_weight;
+ this.maxArchiveRetries = opts.max_archive_retries;
+ this.rollCycle = opts.roll_cycle;
+ this.archiveCommand = opts.archive_command;
+ this.block = opts.block;
+ this.maxLogSize = opts.max_log_size;
+ }
+
+ public Builder withEnabled(boolean enabled)
+ {
+ this.enabled = enabled;
Review comment:
Do I get it wrong or this will be always true?
##########
File path: src/java/org/apache/cassandra/tools/NodeTool.java
##########
@@ -209,6 +209,7 @@ public int execute(String... args)
RelocateSSTables.class,
ViewBuildStatus.class,
ReloadSslCertificates.class,
+ GetAuditLog.class,
Review comment:
I think this is fine where it is but at the same time get and set
methods are grouped together and all other Getters are in a separate group
L127-139. Maybe we should move it there? WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
