Opened 2 years ago
Closed 22 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)
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 2 years ago by fdupont
- Owner set to fdupont
- Status changed from new to accepted
comment:4 Changed 2 years ago by fdupont
- Owner changed from fdupont to UnAssigned
- Status changed from accepted to reviewing
comment:5 Changed 2 years ago by tmark
- Owner changed from UnAssigned to tmark
comment:6 follow-up: ↓ 7 Changed 2 years 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 2 years 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 2 years ago by fdupont
This ticket has dependencies so I stall it until they are reviewed and merged.
comment:9 Changed 2 years ago by fdupont
Added a rationale in src/lib/dhcpsrv/db_log.h. Moved to "logfile" vs "memfile".
comment:10 Changed 23 months ago by fdupont
Changed code style of database logger push/pop from macro/try-catch to RAII. Added tests.
comment:11 Changed 23 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 23 months ago by fdupont
Wait for 5494 to be merged so it won't be by unwanted side effect...
comment:13 Changed 22 months ago by fdupont
- Description modified (diff)
comment:14 Changed 22 months ago by tomek
#5494 is now ready for merge.
comment:15 Changed 22 months ago by tomek
Is there anything blocking this ticket from being merged?
comment:16 Changed 22 months ago by fdupont
comment:17 Changed 22 months ago by fdupont
I am removing #5450 code. What about the ChangeLog entry?
comment:18 Changed 22 months ago by fdupont
- Resolution set to complete
- Status changed from reviewing to closed
Merged. Closing.
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).