Add commands for AuditLogging #108

jtymel · 2016-05-30T08:37:58Z

No description provided.

Ladicek · 2016-05-30T08:41:30Z

...src/main/java/org/wildfly/extras/creaper/commands/auditlog/AbstractSyslogHandlerCommand.java

+import org.wildfly.extras.creaper.core.offline.OfflineCommand;
+import org.wildfly.extras.creaper.core.online.OnlineCommand;
+
+public abstract class AbstractSyslogHandlerCommand implements OnlineCommand, OfflineCommand {


Should it really be public?

Ladicek · 2016-05-30T09:00:00Z

I like this, mostly. The command names are too generic -- I started to comment about that, but I'd have to add a comment to all commands :-) The names must be more specific. Ideally, all would contain the phrase AuditLog. If you feel like that, adding an AuditLogging class (similar to existing Logging) would be nice.

Good job on test coverage :-)

Ladicek · 2016-05-30T09:13:03Z

Also, I'm not very familiar with audit logging. How does it work? That would explain the structure of the commands. So for audit logging, you first configure a formatter (do we have/need commands for that?), then a handler (which refers to a formatter) and finally a logger (which refers to a handler)? I guess that some javadocs for the commands would help. And if you add the AuditLogging entrypoint, that class's javadoc could quickly explain the overall architecture (similarly to how I explained it in this comment, if it's correct).

jtymel · 2016-05-30T12:00:36Z

Thank you very much for the first review :)
I would say that the most important part of AuditLog is File-handler/Syslog-handler. It defines what, where and how will be logged.
These two handlers must refer the formatter they are using. We're fine with using the default json-formatter at the moment, therefore there is no command for creating/removing formatters right now. I can add them in next iteration of this PR if you would like to have these in library.
The final step of AuditLog settings is to refer the syslog/file handler to which will be auditable events (e.g. (un)deploy, change in configuration, server start/stop, ...) sent to within <logger><handlers> tag.

There is always only one logger node. It contains two crucial pieces of information:

whether is AuditLogging enabled
which syslog/file handlers are used as a destination of AuditLogging

The default configuration then looks following:

<audit-log>
    <formatters>
        <json-formatter name="json-formatter"/>
    </formatters>
    <handlers>
        <file-handler name="file" formatter="json-formatter" path="audit-log.log" relative-to="jboss.server.data.dir"/>
    </handlers>
    <logger log-boot="true" log-read-only="false" enabled="false">
        <handlers>
            <handler name="file"/>
        </handlers>
    </logger>
</audit-log>

I'll create an entrypoint class with javadoc in next commit.

Thanks once again for the review.

Ladicek · 2016-05-30T12:05:21Z

Aha. If there's always exactly one logger, why do we have commands AddLogger and RemoveLogger? Should we instead have Change[Audit]Logger?

jtymel · 2016-05-30T12:13:59Z

Oh, I'm terribly sorry. I definitely should have written at most one logger instead of exactly one. In that case it makes IMHO sense to keep both commands.

Ladicek · 2016-05-30T12:24:49Z

OK, what happens if I try to AddLogger twice in a row?

EDIT: I mean, clearly audit loggers are different from other resources for which we have the Add/Remove pairs, so I wonder how should we represent that difference in the API.

jtymel · 2016-05-30T12:28:05Z

It will fail with CommandFailedException (Duplicate resource)

Ladicek · 2016-05-30T12:54:43Z

I wonder if it would make sense to keep Remove[Audit]Logger but replace Add[Audit]Logger with ConfigureAuditLogger that would change the existing one or add it if missing.

That would not be very future-proof, though, in case the ability to add more audit loggers will be added some time in the future...

In the end, I think I agree that we should probably keep Add[Audit]Logger and Remove[Audit]Logger and add a note to the Add[Audit]Logger javadoc that currently, there can only be one audit logger, so you most likely want to use replaceExisting.

Ladicek · 2016-06-07T13:13:32Z

commands/src/main/java/org/wildfly/extras/creaper/commands/auditlog/AuditLogging.java

+        return AuditLogHandler.INSTANCE;
+    }
+
+// ---


Indentation :-)

Also appears on other places in this class.

The // --- comment is my personal habit, I don't require anyone to use it.

Ladicek · 2016-06-10T12:07:17Z

Generally, all public non-abstract classes of commands and their builders should be final (unless there's a good reason not to).

Otherwise looks good. I'll have to have a look at the API in my IDE, but I feel like there are no blockers now.

Oh and BTW, if you push new commits, I don't get a notification. Please always add a comment when you want me to have a look. Just saying "PTAL" is enough.

jtymel · 2016-06-10T12:46:01Z

Ok, final modifiers added/removed and AbstractAuditLogSyslogReliableProtocolCommand removed as well.
Could you please take a look on it?
I'm still thinking about the apply methods in AddAuditLogTcpSyslogHandler, AddAuditLogTlsSyslogHandler and AddAuditLogUdpSyslogHandler. There are common parts that are the same in each class. Do you have any idea how to avoid repeating such code? The only thing that comes to my mind is some util class(/method)..

Ladicek · 2016-06-10T12:54:31Z

OK, I do see some similarities in the apply methods of those classes, but there are also differences and I don't think that it would add much value to try to extract the common parts out. Feel free to try creating an util class, you'll see for yourself if it makes sense or not.

Another option that comes to mind -- would it make sense to merge those 3 classes into one?

jtymel · 2016-06-10T13:11:01Z

I don't think so. IMO UDP is quite different in comparison to TLS. This would bring necessity to properly document each builder method in style this parameter can be used only with TCP and UDP and user could be easily confused.
I'll give it a try to create some util class. However, I'm quite sceptical about the result (whether would be reasonable).

Ladicek · 2016-06-16T13:40:13Z

commands/src/main/java/org/wildfly/extras/creaper/commands/auditlog/AddAuditLogger.java

+
+        /**
+         * <b>Note that when enabled, this can cause server reload!</b>
+         */


The comment belongs to the replaceExisting method.

Ladicek · 2016-06-16T13:41:23Z

Yea, totally agree -- we should have ChangeAuditLogger that will contain this. If you agree, feel free to implement that fully :-)

Ladicek · 2016-06-20T06:28:29Z

...nds/src/main/java/org/wildfly/extras/creaper/commands/auditlog/AddAuditLogSyslogHandler.java

+        this.truncate = builder.truncate;
+        this.replaceExisting = builder.replaceExisting;
+        this.host = builder.host;
+        this.port = builder.port;


You could also have a single constructor that would take AbstractBuilder and set all these common fields. It would be called at the beginning of these 3 constructors, saving a bit of code.

I must have changed constructors to current state because otherwise I was getting: variable XYZ might already have been assigned or variable XYZ might not have been assigned respectively. I'm not sure about the code cleanliness though. Any ideas are welcome ;-)

Ah, true, the fun with definite assignment rules :-( I think 3 different constructors, each for one builder, are better, even with slight code duplication.

Ladicek · 2016-06-20T06:34:11Z

Once you address the last comments, could you please squash? I'd like to do a final round of review, and it's easier like that for me :-) I don't expect any big findings, this seems very good now.

jtymel · 2016-06-20T08:25:08Z

PTAK

Ladicek · 2016-06-20T12:31:34Z

Should the @After cleanup methods use ChangeAuditLogger? I'd expect that using AddAuditLogger would be relatively rare.

jtymel · 2016-06-21T07:05:21Z

Ok, I rewrote one occurrence of AddAuditLogged to ChangeAuditLogger.

Much more importantly: I also added an option for removing handlers that IMHO might be useful. What do you think?

Ladicek · 2016-06-21T07:26:57Z

commands/src/main/java/org/wildfly/extras/creaper/commands/auditlog/ChangeAuditLogger.java

+                List<String> auxList = new ArrayList<String>();
+                auxList.addAll(addHandlers);
+                auxList.removeAll(removeHandlers);
+                if (auxList.size() < addHandlers.size()) {


Set<String> intersection = new HashSet<String>(addHandlers); intersection.retainAll(removeHandlers); if (!intersection.isEmpty()) { ... }

There's also Sets.intersection in Guava, but that requires to create two sets instead of one, so I'd prefer to use what I wrote above.

Ladicek · 2016-06-21T07:27:38Z

Aye, I thought that ChangeAuditLogger.handlers is a list of all handlers that will be there. If it wasn't, then the current split into addHandlers and removeHandlers makes sense. +1 for that.

Ladicek · 2016-06-21T07:30:02Z

The change from AddAuditLogger to ChangeAuditLogger should be done everywhere :-) There are still 2 @After cleanup methods that use AddAuditLogger.

jtymel · 2016-06-21T07:49:48Z

Damn, I didn't notice usage of AddAuditLogger in ChangeAuditLoggerOnlineTest -> fixed.
However, the second usage in RemoveAuditLoggerOnlineTest makes sense. The test removes logger, therefore the logger should be added back afterwards.

Ladicek · 2016-06-21T07:53:19Z

Ah, right you are! If you have no other plans, please squash.

jtymel · 2016-06-21T07:57:38Z

squashed

Ladicek · 2016-06-21T14:02:04Z

Please rebase. This should be the last time, promise :-)

jtymel · 2016-06-22T05:59:46Z

aaand rebased :-)

Ladicek · 2016-06-22T10:14:45Z

Please remind me again -- what's RemoveAuditLogHandler good for?

EDIT: it seems to be covered by ChangeAuditLogger.removeHandlers, no?

Ladicek · 2016-06-22T10:16:56Z

The AddAuditLogSyslogHandler.groovy script is unused, no?

Ladicek · 2016-06-22T10:35:03Z

Otherwise this is good to merge.

jtymel · 2016-06-22T10:45:02Z

Thank you, RemoveAuditLogHandler is absolutely redundant and I removed it (as well as corresponding test and groovy script)
AddAuditLogSyslogHandler.groovy is used in AddAuditLogSyslogHandler.java :-) Anyway, I get your point, AddAuditLogTcpSyslogHandler.groovy, AddAuditLogTlsSyslogHandler.groovy andAddAuditLogUdpSyslogHandler.groovy are totally useless too -> also removed, thanks again!

Squashed once more

Ladicek · 2016-06-22T10:47:41Z

Ah, I thought that the UDP/TCP/TLS variants are used and the generic variant is unused. The opposite variant is actually better, I like 3 removed files :-) Thanks, LGTM now.

Ladicek · 2016-06-22T11:00:07Z

Merged, thanks for all the work :-) I know it's been a long time, but rest assured that there were PRs that took even longer to merge :-)

jtymel · 2016-06-22T11:03:43Z

Thank you.
So I can be quite disappointed that I didn't even brake the record, can't I? :-)

Ladicek · 2016-06-22T11:11:21Z

If you feel like breaking the record, please submit a PR that touches 100 or 200 files :-)

Ladicek self-assigned this May 30, 2016

Ladicek added this to the 1.3.0 milestone May 30, 2016

Ladicek reviewed May 30, 2016
View reviewed changes

Ladicek reviewed Jun 7, 2016
View reviewed changes

Ladicek reviewed Jun 16, 2016
View reviewed changes

Ladicek reviewed Jun 20, 2016
View reviewed changes

jtymel force-pushed the auditlog branch from b68d7a5 to 90d4a3d Compare June 20, 2016 08:19

Ladicek reviewed Jun 21, 2016
View reviewed changes

jtymel force-pushed the auditlog branch from 0469099 to 4ef9f85 Compare June 21, 2016 07:56

jtymel force-pushed the auditlog branch from 4ef9f85 to 8039f1d Compare June 22, 2016 05:56

Add AuditLogging commands

dbf33f6

jtymel force-pushed the auditlog branch from 8039f1d to dbf33f6 Compare June 22, 2016 10:44

Ladicek merged commit dbf33f6 into wildfly-extras:master Jun 22, 2016

Add commands for AuditLogging #108

Add commands for AuditLogging #108

Conversation

jtymel commented May 30, 2016

Choose a reason for hiding this comment

Ladicek commented May 30, 2016

Ladicek commented May 30, 2016

jtymel commented May 30, 2016

Ladicek commented May 30, 2016

jtymel commented May 30, 2016

Ladicek commented May 30, 2016 • edited Loading

jtymel commented May 30, 2016

Ladicek commented May 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Jun 10, 2016 • edited Loading

jtymel commented Jun 10, 2016

Ladicek commented Jun 10, 2016

jtymel commented Jun 10, 2016

Choose a reason for hiding this comment

Ladicek commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Jun 20, 2016

jtymel commented Jun 20, 2016

Ladicek commented Jun 20, 2016

jtymel commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Jun 21, 2016

Ladicek commented Jun 21, 2016

jtymel commented Jun 21, 2016

Ladicek commented Jun 21, 2016

jtymel commented Jun 21, 2016

Ladicek commented Jun 21, 2016

jtymel commented Jun 22, 2016

Ladicek commented Jun 22, 2016 • edited Loading

Ladicek commented Jun 22, 2016

Ladicek commented Jun 22, 2016

jtymel commented Jun 22, 2016 • edited Loading

Ladicek commented Jun 22, 2016

Ladicek commented Jun 22, 2016

jtymel commented Jun 22, 2016

Ladicek commented Jun 22, 2016

Ladicek commented May 30, 2016 •

edited

Loading

Ladicek commented Jun 10, 2016 •

edited

Loading

Ladicek commented Jun 22, 2016 •

edited

Loading

jtymel commented Jun 22, 2016 •

edited

Loading