[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Mahadev konar updated AMBARI-15646:
---
Fix Version/s: 2.4.0
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Fix For: 2.4.0
>
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(Ho
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sebastian Toader updated AMBARI-15646:
--
Resolution: Fixed
Status: Resolved (was: Patch Available)
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> pri
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Gergely updated AMBARI-15646:
Status: Patch Available (was: In Progress)
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(HostRole
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Gergely updated AMBARI-15646:
Attachment: (was: AMBARI-15646.patch)
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(HostRoleCo
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Gergely updated AMBARI-15646:
Attachment: AMBARI-15646.patch
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(HostRoleCommandEntity
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Gergely updated AMBARI-15646:
Attachment: AMBARI-15646.patch
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(HostRoleCommandEntity
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Gergely updated AMBARI-15646:
Attachment: (was: AMBARI-15646.patch)
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(HostRoleCo
[jira] [Updated] (AMBARI-15646) Audit Log Code Cleanup & Safety
[
https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Gergely updated AMBARI-15646:
Attachment: AMBARI-15646.patch
> Audit Log Code Cleanup & Safety
> ---
>
> Key: AMBARI-15646
> URL: https://issues.apache.org/jira/browse/AMBARI-15646
> Project: Ambari
> Issue Type: Bug
> Components: ambari-server
>Reporter: Daniel Gergely
>Assignee: Daniel Gergely
> Attachments: AMBARI-15646.patch
>
>
> 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:
> {code}
> private ThreadLocal dateFormatThreadLocal = new ThreadLocal<>();
> if(dateFormatThreadLocal.get() == null) {
> //2016-03-11T10:42:36.376Z
> dateFormatThreadLocal.set(new
> SimpleDateFormat("-MM-dd'T'HH:mm:ss.SSSX"));
> }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set}}, 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.
> - {code}
> // binding for audit event creators
> Multibinder 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);
> {code}
> - 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:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
> "name")));
> {code}
> -- 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.
> {code}
> private void auditLog(HostRoleCommandEntity
