On Thu, Aug 25, 2011 at 02:30:51PM +0200, Johannes Renner wrote: > > Well, the general 'problem' is that Spacewalk consists of a lot of > different components and this audit log should gather log events coming > from these sources (web ui, frontend api, backend api, taskomatic), > while sending them as one 'trail' to a destination that might be whatever > (syslog, database, etc.). Therefore we decided to use yet again xmlrpc > for the interface, because there is at least an easy-to-use client > implementation available for every scripting language that is used within > Spacewalk.
I'd much rather see a patch which would make the set of used technologies / stacks smaller, for example a patch which would finish the Perl-to-Java migration of the outstanding .pxt Web pages. Also, the completness and accuracy of the trail depends on thousands of new calls to be added throughout the code base. Do we really need that when we already have a place where all actions/changes end (the database) and we could do the auditing/logging from there? > While the implementation of the daemon is a totally different story, I > want to show you how the code might look like for the Java/Web UI part. > I attached some initial patches I have written and would really like to > get review and feedback on this. > > So (1.) there are some helper classes that I put into a new separate > package, see 01-new-classes.patch: > > - AuditLog is a Singleton that serves as the entry point for calls to I don't like the fact that the essential information of true/false is massaged into a string -- shouldn't that be a first-class citizen fields there? The same for the user name -- what if I got and create user named 'none'? Do you really want the message to be localized? That would mean that they can change in time easily. Shouldn't the audit messages be as solid/stable as possible for automatic processing, and only be localized upon presentation? > the log. The interface of the log daemon conists of only one method > that is called log(). > - This method is called via xmlrpc from within the AuditLogAppender. So this AuditLog is server (the daemon) side, or the client (the tomcat or taskomatic) side? > - AuditLogMessage is just a wrapper object representing a log event. Don't you need to have toString there? What constructor of AuditLogMessage does the AuditLogAppender use? > - LogUtil is just a class containing helper methods. The most > interesting part is probably the createExtMap() method that takes an > HTTPServletRequest and extracts its parameters into a HashMap while > ignoring some of them using a blacklist. This map is also passed on > to the logger. The LogUtil.get*String stringifies the parameters without quoting them, making it virtually impossible to process in an automatic manner later. Shouldn't the values stay as map as long as possible, so that for example the database backend can then easily and safely detect values of those parameter and store them in table columns specific for the given type, ideally with foreign keys? > For the actual logging of web UI actions, I investigated into generic > approaches first (using a ServletFilter, do it in RhnRequestProcessor), > but I found out the result of all these approaches is not really what we > want. Interesting events are not really correlating to general HTTP POST > requests, unfortunately :-/ Can you be more specific about this? > I further added (3.) generic audit logging of API calls, please see > my attached 04-xmlrpc-api.patch: > > - This patch also fixes missing IP adresses in already existent logs > (I can also provide this as a single patch, if I find the time..). That would be the right thing to do, obviously. > - While most of the struts actions are RhnAction or RhnLookupDispatchAction, > there is still some that are not derived from these classes. Therefore it's > not enough to put a shortcut method to the logger in the(se) superclass(es). By all means, refactor code which needs to be refactored and submit as patch against master, instead of supporting actions coded not in the latest-greatest technology style. > - Also, because sometimes an action contains the implementation of actually > different things, one log message per action class will not be enough. In > addition to that sometimes singular as well as pluralized messages would > be needed. As mentioned above, I believe localization has no business in this side of the code. You have languages that distinguish more than just singular and plural. > - Sometimes when a form is submitted it is actually hard to find out if > _really_ something has changed, Why is that possible? Isn't this confirming my idea that this all should be done on the database level where you'd know for sure if something has been modified or not (because if not, the trigger would not fire)? -- Jan Pazdziora Principal Software Engineer, Satellite Engineering, Red Hat _______________________________________________ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
