search

Ericsson / ecaudit

Ericsson Audit plug-in for Apache Cassandra
Apache License 2.0
43 stars 36 forks source link

Should ecAudit fail Cassandra operation if logging failed in case of pre logging #185

Open manmagic3 opened 3 years ago

manmagic3 commented 3 years ago

I think operation should get failed if ecAudit is not able to log the auditing info in case of pre logging. I think this should be a feature in ecAudit for pre logging.

eperott commented 3 years ago

Hey! Thanks for the feature request.

Do you have any specific kind of failures in mind? Did you encounter scenarios where a pre-audit log failed, but the actual query was later executed? Are you aware of ways to reproduce such a scenario?

At this point I'm able to find one place in our code where we deliberately catch and mute an exception: https://github.com/Ericsson/ecaudit/blob/33c385f1e0c46477b5fd54d7427b5d50ff93e62f/ecaudit/src/main/java/com/ericsson/bss/cassandra/ecaudit/logger/ChronicleAuditLogger.java#L57 But in this case we're raising the interrupt flag on the running thread, so I'd expect the operation to be aborted. Didn't verify this though.

manmagic3 commented 3 years ago

As of now I am not able to reproduce such a scenario. If we are doing auditing and some audit message is dropped due to some reason, should operation go on and complete its activity. Is this acceptable behavior for auditing?. Can user complain that even if my auditing was enabled but particular activity was not registered or is acceptable in general. Consider a case where user do some delete operation and we are not able to record it. Is it fair?

Would like to know the thoughts of contributors here? Should we have that behavior of aborting operation on failed logging or we should live with it.

eperott commented 3 years ago

I think your arguments are valid. It would make sense to abort operations unless pre-auditing is successful. At least as an option.

The point I was trying to make, is that I think this is actually how things already work in ecAudit. As of now, we're not catching/muting exceptions when auditing fails. I believe this would cause the operation to be aborted, and so in practice the feature you're requesting is already implemented. :)

I could be missing details somewhere. Could you point to a place in the code where things need improvement?

manmagic3 commented 3 years ago

I think you are right. Looks like it is already implemented by default.