Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(feat) Allow sending of only aspect-specific audit events. #141

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

(feat) Allow sending of only aspect-specific audit events. #141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76cb29a
Allow sending of only aspect-specific audit events.
Dec 17, 2021
03f5a28
Merge branch 'linkedin:master' into master
sacsar Feb 1, 2022
b876fdf
Remove surplus method
Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.linkedin.metadata.dao;

import com.google.common.base.Preconditions;
import com.linkedin.common.AuditStamp;
import com.linkedin.common.urn.Urn;
import com.linkedin.data.schema.validation.CoercionMode;
Expand Down Expand Up @@ -123,6 +124,9 @@ static class AddResult<ASPECT extends RecordTemplate> {
// Always emit MAE on every update regardless if there's any actual change in value
private boolean _alwaysEmitAuditEvent = false;

// Whether to emit the general MAE. This takes precedence over _alwaysEmitAuditEvent if general audit events are disabled.
private boolean _enableGeneralAuditEvent = true;

private boolean _emitAspectSpecificAuditEvent = false;

private boolean _alwaysEmitAspectSpecificAuditEvent = false;
Expand Down Expand Up @@ -250,6 +254,15 @@ public void enableModelValidationOnWrite(boolean enabled) {
_modelValidationOnWrite = enabled;
}

/**
* Enables or disables the emission of general audit events.
*/
public void enableGeneralAuditEvent(boolean generalAuditEventEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this method public? If it's public then what is the difference between this method and setEmitGeneralAuditEvent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No difference :) I was trying to be consistent in the naming and it turned out it wasn't possible--there's enableModelValidationOnWrite, but setEmitAspectSpecificAuditEvent. I thought enableGeneralAuditEvent was the "better" name, but added the other for symmetry with setEmitAspectSpecificAuditEvent. I can delete one or the other or, if you think enableFoo is the way forward, mark the setFoo one as deprecated and note that it just routes to enableFoo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just stick with one method, enableGeneralAuditEvent sounds good to me.

Preconditions.checkState(generalAuditEventEnabled || _emitAspectSpecificAuditEvent,
"Emission of general audit events cannot be disabled without enabling aspect-specific audit events");
_enableGeneralAuditEvent = generalAuditEventEnabled;
}

/**
* Sets if MAE should be always emitted after each update even if there's no actual value change.
*/
Expand All @@ -264,6 +277,13 @@ public void setEmitAspectSpecificAuditEvent(boolean emitAspectSpecificAuditEvent
_emitAspectSpecificAuditEvent = emitAspectSpecificAuditEvent;
}

/**
* Sets whether to emit the general MAE. Either general or aspect-specific MAE must be enabled.
*/
public void setEmitGeneralAuditEvent(boolean emitGeneralAuditEvent) {
enableGeneralAuditEvent(emitGeneralAuditEvent);
}

/**
* Sets if aspect specific MAE should be always emitted after each update even if there's no actual value change.
*/
Expand Down Expand Up @@ -381,7 +401,7 @@ public <ASPECT extends RecordTemplate> ASPECT add(@Nonnull URN urn, @Nonnull Cla
final ASPECT newValue = result.getNewValue();

// Produce MAE after a successful update
if (_alwaysEmitAuditEvent || oldValue != newValue) {
if (_enableGeneralAuditEvent && (_alwaysEmitAuditEvent || oldValue != newValue)) {
_producer.produceMetadataAuditEvent(urn, oldValue, newValue);
}

Expand Down
58 changes: 58 additions & 0 deletions dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,64 @@ public void testMAEWithNullValue() throws URISyntaxException {
verifyNoMoreInteractions(_mockEventProducer);
}

@Test
sacsar marked this conversation as resolved.
Show resolved Hide resolved
public void testGeneralMAEDisabled() throws URISyntaxException {
FooUrn urn = new FooUrn(1);
AspectFoo foo = new AspectFoo().setValue("foo");
_dummyLocalDAO.setEmitAspectSpecificAuditEvent(true);
_dummyLocalDAO.enableGeneralAuditEvent(false);

expectGetLatest(urn, AspectFoo.class,
Arrays.asList(makeAspectEntry(null, null), makeAspectEntry(foo, _dummyAuditStamp)));

_dummyLocalDAO.add(urn, foo, _dummyAuditStamp);
_dummyLocalDAO.delete(urn, AspectFoo.class, _dummyAuditStamp);
verify(_mockEventProducer, times(0)).produceMetadataAuditEvent(urn, null, foo);
verify(_mockEventProducer, times(1)).produceAspectSpecificMetadataAuditEvent(urn, null, foo);
verifyNoMoreInteractions(_mockEventProducer);
}

@Test
public void testGeneralMAEDefaultsEnabled() throws URISyntaxException {
FooUrn urn = new FooUrn(1);
AspectFoo foo = new AspectFoo().setValue("foo");

expectGetLatest(urn, AspectFoo.class,
Arrays.asList(makeAspectEntry(null, null), makeAspectEntry(foo, _dummyAuditStamp)));

_dummyLocalDAO.add(urn, foo, _dummyAuditStamp);
_dummyLocalDAO.delete(urn, AspectFoo.class, _dummyAuditStamp);
verify(_mockEventProducer, times(1)).produceMetadataAuditEvent(urn, null, foo);
verify(_mockEventProducer, times(0)).produceAspectSpecificMetadataAuditEvent(urn, null, foo);
verifyNoMoreInteractions(_mockEventProducer);
}

@Test(expectedExceptions = {
IllegalStateException.class},
expectedExceptionsMessageRegExp = "Emission of general audit events cannot be disabled without enabling aspect-specific audit events")
public void testCannotDisableGeneralMAEWithoutEnablingAspectMAE() {
_dummyLocalDAO.setEmitAspectSpecificAuditEvent(false);
_dummyLocalDAO.enableGeneralAuditEvent(false);
}

@Test
public void testMAEDualWrite() throws URISyntaxException {
FooUrn urn = new FooUrn(1);
AspectFoo foo = new AspectFoo().setValue("foo");

expectGetLatest(urn, AspectFoo.class,
Arrays.asList(makeAspectEntry(null, null), makeAspectEntry(foo, _dummyAuditStamp)));

_dummyLocalDAO.setEmitAspectSpecificAuditEvent(true);

_dummyLocalDAO.add(urn, foo, _dummyAuditStamp);
_dummyLocalDAO.delete(urn, AspectFoo.class, _dummyAuditStamp);
verify(_mockEventProducer, times(1)).produceMetadataAuditEvent(urn, null, foo);
verify(_mockEventProducer, times(1)).produceAspectSpecificMetadataAuditEvent(urn, null, foo);
verifyNoMoreInteractions(_mockEventProducer);

}

@Test
public void testAddSamePreUpdateHookTwice() {
BiConsumer<FooUrn, AspectFoo> hook = (urn, foo) -> {
Expand Down