Opened 2 years ago

Closed 20 months ago

#5420 closed enhancement (complete)

Extend forensic logging with Postgres capability

Reported by: tomek Owned by: fdupont
Priority: high Milestone: Kea1.4
Component: hooks Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Premium Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by fdupont)

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).

Subtickets

Change History (18)

comment:1 Changed 2 years ago by tomek

  • Description modified (diff)

comment:2 Changed 2 years ago by tomek

  • Priority changed from medium to high

comment:3 Changed 22 months ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:4 Changed 22 months ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

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).

comment:5 Changed 22 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:6 follow-up: Changed 22 months ago by tmark

  • Owner changed from tmark to fdupont

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.

comment:7 in reply to: ↑ 6 Changed 22 months ago by 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...

comment:8 Changed 22 months ago by fdupont

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

comment:9 Changed 22 months ago by fdupont

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

comment:10 Changed 22 months ago by fdupont

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

comment:11 Changed 21 months ago by tmark

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

comment:12 Changed 21 months ago by fdupont

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

comment:13 Changed 21 months ago by fdupont

  • Description modified (diff)

comment:14 Changed 21 months ago by tomek

#5494 is now ready for merge.

comment:15 Changed 21 months ago by tomek

Is there anything blocking this ticket from being merged?

comment:16 Changed 21 months ago by 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.

comment:17 Changed 20 months ago by fdupont

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

comment:18 Changed 20 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged. Closing.

Note: See TracTickets for help on using tickets.