diff --git a/src/storage.rs b/src/storage.rs
index 55917c1f424f7036bb8724ddb453c28918bae839..6eac40cadda57e103591b410b6c52513c2d8c16c 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -962,6 +962,10 @@ impl Clusterwide {
}
/// Perform the `dml` operation on the local storage.
+ /// When possible, return the new tuple produced by the operation:
+ /// * `Some(tuple)` in case of insert and replace;
+ /// * `Some(tuple)` or `None` depending on update's result (it may be NOP);
+ /// * `None` in case of delete (because the tuple is gone).
#[inline]
pub fn do_dml(&self, dml: &Dml) -> tarantool::Result<Option<Tuple>> {
let space = space_by_id_unchecked(dml.space());
@@ -969,7 +973,7 @@ impl Clusterwide {
Dml::Insert { tuple, .. } => space.insert(tuple).map(Some),
Dml::Replace { tuple, .. } => space.replace(tuple).map(Some),
Dml::Update { key, ops, .. } => space.update(key, ops),
- Dml::Delete { key, .. } => space.delete(key),
+ Dml::Delete { key, .. } => space.delete(key).map(|_| None),
}
}
}
@@ -1170,6 +1174,22 @@ impl PropertyName {
Ok(())
}
+
+ /// Try decoding property's value specifically for audit log.
+ /// Returns `Some(string)` if it should be logged, `None` otherwise.
+ pub fn should_be_audited(&self, tuple: &Tuple) -> Result<Option<String>> {
+ let bad_value = || Error::other("bad value");
+ Ok(match self {
+ Self::PasswordMinLength
+ | Self::MaxLoginAttempts
+ | Self::SnapshotChunkMaxSize
+ | Self::MaxPgPortals => {
+ let v = tuple.field::<usize>(1)?.ok_or_else(bad_value)?;
+ Some(format!("{v}"))
+ }
+ _ => None,
+ })
+ }
}
////////////////////////////////////////////////////////////////////////////////
@@ -1225,7 +1245,8 @@ impl Properties {
// Not a builtin property.
// Cannot be a wrong type error, because tarantool checks
// the format for us.
- if old.is_none() { // Insert
+ if old.is_none() {
+ // Insert
// FIXME: this is currently printed twice
tlog!(Warning, "non builtin property inserted into _pico_property, this may be an error in a future version of picodata");
}
diff --git a/src/traft/node.rs b/src/traft/node.rs
index a845747d1cbdc9a054c29ea46da10eaabfc5dffc..f0e46c53a215e300193900e6cab4341cde73e40e 100644
--- a/src/traft/node.rs
+++ b/src/traft/node.rs
@@ -17,6 +17,7 @@ use crate::schema::{Distribution, IndexDef, TableDef};
use crate::sentinel;
use crate::storage::acl;
use crate::storage::ddl_meta_drop_space;
+use crate::storage::space_by_id;
use crate::storage::SnapshotData;
use crate::storage::{ddl_abort_on_master, ddl_meta_space_update_operable};
use crate::storage::{local_schema_version, set_local_schema_version};
@@ -60,7 +61,7 @@ use ::tarantool::space::FieldType as SFT;
use ::tarantool::time::Instant;
use ::tarantool::tlua;
use ::tarantool::transaction::transaction;
-use ::tarantool::tuple::Decode;
+use ::tarantool::tuple::{Decode, Tuple};
use ::tarantool::vclock::Vclock;
use protobuf::Message as _;
@@ -728,32 +729,101 @@ impl NodeImpl {
match op {
Op::Nop => {}
Op::Dml(op) => {
- let res = self.storage.do_dml(&op);
+ let space = op.space().try_into();
+
+ // In order to implement the audit log events, we have to compare
+ // tuples from certain system spaces before and after a DML operation.
+ //
+ // In theory, we could move this code to `do_dml`, but in practice we
+ // cannot do that just yet, because we aren't allowed to universally call
+ // `extract_key` for arbitrary tuple/space pairs due to insufficient validity
+ // checks on its side -- it might just crash for user input.
+ //
+ // Remeber, we're not the only ones using CaS; users may call it for their
+ // own spaces, thus emitting unrestricted (and unsafe) DML records.
+ //
+ // TODO: merge this into `do_dml` once `box_tuple_extract_key` is fixed.
+ let old = match space {
+ Ok(s @ (ClusterwideTable::Property | ClusterwideTable::Instance)) => {
+ let s = space_by_id(s.id()).expect("system space must exist");
+ match &op {
+ // There may be no previous version for inserts.
+ Dml::Insert { .. } => Ok(None),
+ Dml::Update { key, .. } => s.get(key),
+ Dml::Delete { key, .. } => s.get(key),
+ Dml::Replace { tuple, .. } => {
+ let key = s.primary_key().extract_key(Tuple::from(tuple));
+ s.get(&key)
+ }
+ }
+ }
+ _ => Ok(None),
+ };
+
+ // Perform DML and combine both tuple versions into a pair.
+ let res = old.and_then(|old| {
+ let new = self.storage.do_dml(&op)?;
+ Ok((old, new))
+ });
+
match &res {
Err(e) => {
tlog!(Error, "clusterwide dml failed: {e}");
}
- Ok(Some(tuple))
- if op.space() == ClusterwideTable::Instance.id()
- // TODO: do we need to log something into the audit when deleting instances?
- && !matches!(op, Dml::Delete { .. }) =>
- {
+ // Handle delete from _pico_property
+ Ok((Some(old), None)) if space == Ok(ClusterwideTable::Property) => {
+ assert!(matches!(op, Dml::Delete { .. }));
+
+ if let Ok(Some(key)) = old.field::<PropertyName>(0) {
+ crate::audit!(
+ message: "property `{key}` was deleted",
+ title: "change_config",
+ severity: High,
+ );
+ }
+ }
+ // Handle insert, replace, update in _pico_property
+ Ok((_old, Some(new))) if space == Ok(ClusterwideTable::Property) => {
+ // Dml::Delete mandates that new tuple is None.
+ assert!(!matches!(op, Dml::Delete { .. }));
+
+ // Keep in mind that currently we allow invalid names,
+ // which means that we have to ignore the decoding error.
+ let key = new.field::<PropertyName>(0).ok().flatten();
+
+ // Try decoding the value iff the name's known.
+ let value = key
+ .map(|p| p.should_be_audited(new))
+ .transpose()
+ .expect("property value decoding should not fail")
+ .flatten();
+
+ if let Some((key, value)) = key.zip(value) {
+ crate::audit!(
+ message: "property `{key}` was changed to {value}",
+ title: "change_config",
+ severity: High,
+ );
+ }
+ }
+ // Handle insert, replace, update in _pico_instance
+ Ok((old, Some(new))) if space == Ok(ClusterwideTable::Instance) => {
+ // Dml::Delete mandates that new tuple is None.
+ assert!(!matches!(op, Dml::Delete { .. }));
+
+ let old: Option<Instance> =
+ old.as_ref().map(|x| x.decode().expect("must be Instance"));
+
// FIXME: we do this prematurely, because the
// transaction may still be rolled back for some reason.
- let new: Instance = tuple
+ let new: Instance = new
.decode()
.expect("tuple already passed format verification");
- if has_grades!(new, Expelled -> *) && new.raft_id == self.raft_id() {
- // cannot exit during a transaction
- *expelled = true;
- }
-
// Check if we're handling a "new node joined" event:
// * Either there's no tuple for this node in the storage;
// * Or its raft id has changed, meaning it's no longer the same node.
- let prev = self.storage.instances.get(&new.instance_id).ok();
- if prev.as_ref().map(|x| x.raft_id) != Some(new.raft_id) {
+ if old.as_ref().map(|x| x.raft_id) != Some(new.raft_id) {
let instance_id = &new.instance_id;
crate::audit!(
message: "a new instance `{instance_id}` joined the cluster",
@@ -764,7 +834,7 @@ impl NodeImpl {
);
}
- if prev.as_ref().map(|x| x.current_grade) != Some(new.current_grade) {
+ if old.as_ref().map(|x| x.current_grade) != Some(new.current_grade) {
let instance_id = &new.instance_id;
let grade = &new.current_grade;
crate::audit!(
@@ -772,11 +842,12 @@ impl NodeImpl {
title: "change_current_grade",
severity: Medium,
instance_id: %instance_id,
+ raft_id: %new.raft_id,
new_grade: %grade,
);
}
- if prev.as_ref().map(|x| x.target_grade) != Some(new.target_grade) {
+ if old.as_ref().map(|x| x.target_grade) != Some(new.target_grade) {
let instance_id = &new.instance_id;
let grade = &new.target_grade;
crate::audit!(
@@ -785,8 +856,25 @@ impl NodeImpl {
severity: Low,
instance_id: %instance_id,
raft_id: %new.raft_id,
+ new_grade: %grade,
);
}
+
+ if has_grades!(new, Expelled -> *) {
+ let instance_id = &new.instance_id;
+ crate::audit!(
+ message: "instance `{instance_id}` was expelled from the cluster",
+ title: "expel_instance",
+ severity: Low,
+ instance_id: %instance_id,
+ raft_id: %new.raft_id,
+ );
+
+ if new.raft_id == self.raft_id() {
+ // cannot exit during a transaction
+ *expelled = true;
+ }
+ }
}
Ok(_) => {}
}
diff --git a/src/traft/op.rs b/src/traft/op.rs
index 2f03e89ae0164672cdb4b3e29e43be1cf9e27400..3cbaf2972d45066c833db03bd392c10d2dc55e41 100644
--- a/src/traft/op.rs
+++ b/src/traft/op.rs
@@ -339,7 +339,7 @@ pub enum Dml {
// TODO: remove this
impl OpResult for Dml {
- type Result = tarantool::Result<Option<Tuple>>;
+ type Result = tarantool::Result<(Option<Tuple>, Option<Tuple>)>;
fn result(&self) -> Self::Result {
unreachable!()
}
diff --git a/test/int/test_audit.py b/test/int/test_audit.py
new file mode 100644
index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391