Extend forensic logging with Postgres capability

[tomek]

One user requested to extend forensic logging library with the capability to write log entries to PostgreSQL rather than to a log file.

This is one of two similar tickets. This one is for PostgreSQL, while others cover MySQL (#5421) and Cassandra (#5500).

[fdupont]

Done for PostgresQL, MySQL and Cassandra CQL (the last one for completion only).
Ready for review (design notes are at the end of the .dox file).

[tmark]

load_unload.cc

You need #ifdef HAVE_ s around the backend includes, or it won't compile, with
one or more backends not configured.

---

load_unload.cc

load() - I'm not sure I agree with using "memfile" as a type for this. It may
mislead people into thinking there is some connection with lease memfile and or
LFC. I'd suggest treating "memfile" as invalid and use "file" or "diskfile".

---

I think we need additional commentary in the db logger related files in both
legal_log and dhcpsrv directories so people understand what they're looking at
and what they need to do to add or remove messages. I think you're solution
for this is clever but it's not entirely obvious to the casual observer.

---

Have you system tested all of this?

---

Running legal log unit tests under Ubuntu 16.04:

Unit test LegalLibLoadTest?.invalidType segfaults:

make[3]: Entering directory '/d1/labspace/build/keadev/sandbox/trac5420/kea/premium/src/hooks/dhcp/legal_log/libloadtests'
make check-TESTS
make[4]: Entering directory '/d1/labspace/build/keadev/sandbox/trac5420/kea/premium/src/hooks/dhcp/legal_log/libloadtests'
[==========] Running 9 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 5 tests from LegalLibLoadTest?
[ RUN ] LegalLibLoadTest?.invalidType
/bin/bash: line 5: 1530 Segmentation fault /bin/bash ../../../../../../libtool --mode=execute ${dir}$tst
FAIL: libdhcp_legal_log_unittests

I tried a few others under LegalLibLoadTest? and they also crash.

---

I have not set up Cassandra so I did not try to compile with it or run unit tests.

---

I think you're solution is a good, clean approach. Extracting the common bits from RotatingFile? into
BackendStore? worked very nicely. So well done on that.

There are few things you did with this ticket that go against our policies. You expanded the scope of
the ticket to include all the backends. MySQL is called for under 5421, and Cassandra isn't scoped at all.

Also you merged some unreviewed tickets into this branch which are unrelated to this work.
That makes reviewing the changes specific to this ticket more complicated and it also makes them more
or less dependent on that work. You would not have needed Cassandra related changes if this ticket
didn't include the Cassandra implementation.

We scope the work involved in tickets to make changes modular and to them managable. You should
follow the ticket scope. You also ran a risk of there being issues with your implementation,
and since you did all three it would have been a more to rework.

In the future, please try to stick to ticket scope and not mix in unrelated tickets.

[fdupont]

Replying to tmark:

load_unload.cc

You need #ifdef HAVE_ s around the backend includes, or it won't compile, with
one or more backends not configured.

=> oops, fixed.

load_unload.cc

load() - I'm not sure I agree with using "memfile" as a type for this. It may
mislead people into thinking there is some connection with lease memfile and or
LFC. I'd suggest treating "memfile" as invalid and use "file" or "diskfile".

=> I was not sure "memfile" was a bit too much. What about "logfile"?

I think we need additional commentary in the db logger related files in both
legal_log and dhcpsrv directories so people understand what they're looking at
and what they need to do to add or remove messages. I think you're solution
for this is clever but it's not entirely obvious to the casual observer.

=> where I should put them in dhcpsrv? headers? .dox? both?

Have you system tested all of this?

=> yes, this is why I merged some pending branches before tagging the base.
Note all dependencies (trac does not know what it is...) have higher priority
or lower number so I expect they will be reviewed before.

Running legal log unit tests under Ubuntu 16.04:

=> I tried on macOS.

Unit test LegalLibLoadTest?.invalidType segfaults:

make[3]: Entering directory '/d1/labspace/build/keadev/sandbox/trac5420/kea/premium/src/hooks/dhcp/legal_log/libloadtests'
make check-TESTS
make[4]: Entering directory '/d1/labspace/build/keadev/sandbox/trac5420/kea/premium/src/hooks/dhcp/legal_log/libloadtests'
[==========] Running 9 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 5 tests from LegalLibLoadTest?
[ RUN ] LegalLibLoadTest?.invalidType
/bin/bash: line 5: 1530 Segmentation fault /bin/bash ../../../../../../libtool --mode=execute ${dir}$tst
FAIL: libdhcp_legal_log_unittests

I tried a few others under LegalLibLoadTest? and they also crash.

=> there is something which makes any test to crash, perhaps no bound to legal_logs
as this test is very basic... I have a 16.04 VM ready, I'll see that on it.

I think you're solution is a good, clean approach. Extracting the common bits from RotatingFile? into
BackendStore? worked very nicely. So well done on that.

There are few things you did with this ticket that go against our policies. You expanded the scope of
the ticket to include all the backends. MySQL is called for under 5421, and Cassandra isn't scoped at all.

=> as I just reuse a small part of lease database backend code it was IMHO better to address
the same backends before forgetting how to do. Cassandra was easy as soon as you do
the effort to setup it and it was my case...

Also you merged some unreviewed tickets into this branch which are unrelated to this work.

=> not so unrelated as it did not build on my platform without them.

In the future, please try to stick to ticket scope and not mix in unrelated tickets.

=> I was asked to address this ticket so I didn't wait for dependencies to be reviewed
and merged before putting it on the review queue. And it will be fine to agree on a
way to mark dependencies in trac so things will be reviewed in order.
Note anyway we have to stall this ticket and wait for dependencies so I am not sure
we really won some time...

[fdupont]

This ticket has dependencies so I stall it until they are reviewed and merged.

[fdupont]

Added a rationale in src/lib/dhcpsrv/db_log.h. Moved to "logfile" vs "memfile".

[fdupont]

Changed code style of database logger push/pop from macro/try-catch to RAII. Added tests.

[tmark]

Your changes all look good. The RAII change to logger is MUCH cleaner, nicely done. Good to merge.

[fdupont]

Wait for 5494 to be merged so it won't be by unwanted side effect...

[tomek]

#5494 is now ready for merge.

[tomek]

Is there anything blocking this ticket from being merged?

[fdupont]

#5450, there are 2 solutions:

• remove the one line commit to 5420 branch and merge.
• review 5450 change, merge 5420 and close 5420 and 5450.

I tried a few times to get a jabber review for #5450 without success. BTW there is no trac5450 branch nor a scheduled ChangeLog entry.

[fdupont]

I am removing #5450 code. What about the ChangeLog entry?

[fdupont]

Merged. Closing.