[AMBARI-15646] Audit Log Code Cleanup & Safety

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 2.4.0
• Component/s: ambari-server
• Assignee: Daniel Gergely
• Reporter: Daniel Gergely
• Created: 31/Mar/16 13:23
• Updated: 08/Apr/16 18:22
• Resolved: 08/Apr/16 07:52

Description

As a follow-up to AMBARI-15241, there are concerns brought up in review which should be addressed but didn't need to hold up the feature being committed. These can be further broken out to into separate Jiras if needed:

• When initializing a ThreadLocal, you can specify an initial value. This code is unnecessary:

  private ThreadLocal<DateFormat> dateFormatThreadLocal = new ThreadLocal<>();
      if(dateFormatThreadLocal.get() == null) {
        //2016-03-11T10:42:36.376Z
        dateFormatThreadLocal.set(new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX"));
      }
  

• There are no tests for a majority of events and event creators.
• Using a multibinder is fine to be able to inject a Set<Foo>, but it's not clear to developers adding code that this is what must be done.
  • We either need to document the super interface to make it clear how to have new creators bound
  • Or annotate creators with an annotation which then be automatically picked up by the AuditLoggerModule and bound without the need to explicitly define each creator.

•     // binding for audit event creators
      Multibinder<RequestAuditEventCreator> auditLogEventCreatorBinder = Multibinder.newSetBinder(binder(), RequestAuditEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(DefaultEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ComponentEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ServiceEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(UnauthorizedEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ConfigurationChangeEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(UserEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(GroupEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(MemberEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(PrivilegeEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(BlueprintExportEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ServiceConfigDownloadEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(BlueprintEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ViewInstanceEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ViewPrivilegeEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(RepositoryEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(RepositoryVersionEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(AlertGroupEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(AlertTargetEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(HostEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(UpgradeEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(UpgradeItemEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(RecommendationIgnoreEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(ValidationIgnoreEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(CredentialEventCreator.class);
      auditLogEventCreatorBinder.addBinding().to(RequestEventCreator.class);
  
      bind(RequestAuditLogger.class).to(RequestAuditLoggerImpl.class);
  

• Event creators have nested invocations which is not only hard to read, but can potentially cause NPE's; it's a dangerous practice. As an example:
  AlertGroupEventCreator

  String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup", "name")));
  

  • Additionally, this references properties by building them, instead of by their registration in the property provider. If the property name ever changed, this could easily be missed.
• Some of the auditLog methods check to ensure that the logger is enabled first. This is very good, as building objects which won't be logged is a waste and potential performance problem. However, not all of them do. All auditLog methods should check this first, and return if not enabled. You can do this using AOP or just brute-force every method.

    private void auditLog(HostRoleCommandEntity commandEntity, Long requestId) {
      if(!auditLogger.isEnabled()) {
        return;
      }
  

• The temporary status caches in ActionDBAccessorImpl are never cleared.

Issue Links

• links to: Review Board