Opened 8 months ago

Closed 5 months ago

#5530 closed enhancement (complete)

Radius accounting

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: hook-radius 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

The RadiusDesign requires accounting to be supported.

Note that it does require some of the information being available only during initial client's DORA process. This information will be stored in host reservation using user context, which will be necessary during accounting communication.

Also, since the communication is unidirectional (there's no expectation for Kea to get anything back from accounting), this should be implemented in separate thread.

Subtickets

Change History (11)

comment:1 Changed 6 months ago by fdupont

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

comment:2 Changed 6 months ago by fdupont

Ideas:

  • reuse the legal log code for start and update code.
  • for stop case hooks are expire, release and decline. Should add a Acct-Terminate-Cause (IMHO no because available causes do not match)
  • problem with Acct-Session-Id: lease is identified by the address (primary key in databases), suggest address + creation date.
  • need to catch some state per lease
  • csv file to provide stable storage.
  • format for hwaddr (canonical: separator '-' lower case, text: toText(false))
  • consider relay address in a second time (can be done only from the query as not stored with the lease).

comment:3 Changed 6 months ago by fdupont

Rebased to the last master to use the new dhcp4_srv_configured callout what I cut&pasted to dhcp6_srv_configured. So new the branch is trac5530a.

comment:4 Changed 6 months ago by fdupont

Code can be greatly simplified if leases can take extra information, a feature which seems to be scheduled for 1.5, doesn't?

comment:5 Changed 6 months ago by fdupont

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

Unit tests ready at the exception of session file:

  • session file does not really belongs to basic operation / first step implementation.
  • session file is used to go through server restart / reconfig. In such a case the host cache is lost too so Radius state.
  • current tests just run with no session file so prove it is not required.

In conclusion this branch is ready for review. Note there should be a #5527/5530 merged branch if needed but branches are supposed to be fairly independent.
Last point: I used the Kea trac5527 environment: in theory it is not required but if something goes wrong just change Kea base or wait of 5533a to be merged.

comment:6 Changed 5 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:7 Changed 5 months ago by fdupont

Updated code from last master (including some diffs I pulled between two pushed).
BTW the random date is not random at all and you should have recognized it...
And session history is just the time to get user context in leases so IMHO we should not invest a lot in it, in particular for the file version.
Last point: I got a few random crashed on the timer ASIO code (ASIO vs thread?) in tests.

comment:8 follow-up: Changed 5 months ago by tomek

  • Owner changed from tomek to fdupont

PREMIUM CODE REVIEW:

The way this ticket is handled is another example of a very bad practice.
You copied over the code that was developed in earlier tickets and
committed here on trac5530 again. How do you expect this to go
when you'll be merging this? For example this is the third time
I see the same changes to tier2.m4.

You told me that git diff does not work well. It DOES WORK WELL unless
you do nonsense like this. This code should have been rebased. If you
really dislike rebases (why, oh why?), the second best option was
to cherry-pick changes, so later the merge could be easier. You
chose the worst possible way forward and simply dropped the code
on this branch.

However, since we're working under very tight schedule, so I did what
you suggested - to not use git diff (as reasonable engineers do), but
rather compare directories: one with trac5526 with another with
trac5530. This is completely unworkable. To do this, I needed three
separate repositories. One to check out trac5526, second to checkout
trac5530 and a third one to actually try to build that
thing. Comparing dirty directories does not end up well with diff,
because diff does not have a concept of gitignore.

I tried to compile the code with kea repo on trac5530a and premium
repo being on trac5530. It did not compile.

I did pull serveral fixes from 5526 using cherry-pick, so later the rebase or
merge will be simpler. Also fixed several compilation issues that
are specific to trac5530.

I pushed many improvements to the code. Please pull and review.

radius_parsers.cc
I don't know why the variable was named rtf4361. Renamed to rfc4361.

radius_messages.mes
The work DUMP may be misinterpreted as discarded. It's better to use
work write or store. Renamed.

radius_accounting.cc
I'm not sure if the session-id built in RadiusAccounting::buildAcct,
especially in v6, is constructed properly. To be sure, I've added
explicit toText().

accounting_unittests.cc
There are no tests for session-history, both in memory and disk
operations.


With the improvements the code builds and unit-tests pass on Mac OS
10.12.6. Please pull and review.


KEA CODE REVIEW:
The changes are good, but there was documentation for
dhcp6_srv_configured missing. The code is very light on tests,
but I presume it will be done properly as part of #5458.


This ticket requires two changelog entries. Here's my proposal:

KEA:

13XX.	[func]		fdupont
	dhcp6_srv_configured hook point added.
	(Trac #5530,	git tbd)

PREMIUM:

2X.	[func]		fdupont
	Accounting mechanism implemented in RADIUS hook library.
	(Trac #5530, git tbd)

comment:9 in reply to: ↑ 8 Changed 5 months ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

PREMIUM CODE REVIEW:

The way this ticket is handled is another example of a very bad practice.
You copied over the code that was developed in earlier tickets and
committed here on trac5530 again. How do you expect this to go
when you'll be merging this? For example this is the third time
I see the same changes to tier2.m4.

=> I didn't copy tier2.m4 when I created tickets but based new tickets
on the previous ticket.

You told me that git diff does not work well. It DOES WORK WELL unless
you do nonsense like this. This code should have been rebased. If you
really dislike rebases (why, oh why?),

=> just because it does not work and give a lot of silly conflicts.

I did pull serveral fixes from 5526 using cherry-pick, so later the rebase or
merge will be simpler.

=> unfortunately I tried to rebase before you push them so it didn't help
the rebase... Note when I pulled git messed only one file (conflict
on a Makefile.am trivial to solve) so it should have had a positive
effect.

I pushed many improvements to the code. Please pull and review.

radius_parsers.cc
I don't know why the variable was named rtf4361. Renamed to rfc4361.

=> a typo.

radius_messages.mes
The work DUMP may be misinterpreted as discarded. It's better to use
work write or store. Renamed.

=> I went further and replaced all dump's by store's.

radius_accounting.cc
I'm not sure if the session-id built in RadiusAccounting::buildAcct,
especially in v6, is constructed properly. To be sure, I've added
explicit toText().

=> IOAddress has a << operator.

accounting_unittests.cc
There are no tests for session-history, both in memory and disk
operations.

=> for disk operation it is deliberate: it is something not really
needed in production and will be phased out as soon as
we implement user context for leases.
For memory IMHO it is better to have a multiple accounting test
but unfortunately if it is easy for a system test it is not for
an unit test, in particular if we don't want to wait more than
a few seconds. And to hack the clock does not work with full ASIO...

KEA CODE REVIEW:
The changes are good, but there was documentation for
dhcp6_srv_configured missing. The code is very light on tests,

=> more than light: I didn't copy the test (but it is in trac5458a
and passes). If you like I can copy it to the Kea branch before
merging it.

but I presume it will be done properly as part of #5458.

=> I can confirm.


This ticket requires two changelog entries. Here's my proposal:

KEA:

13XX.	[func]		fdupont
	dhcp6_srv_configured hook point added.
	(Trac #5530,	git tbd)

PREMIUM:

2X.	[func]		fdupont
	Accounting mechanism implemented in RADIUS hook library.
	(Trac #5530, git tbd)

comment:10 Changed 5 months ago by tomek

  • Owner changed from tomek to fdupont

Looks good. Please merge. Just make sure only RADIUS changes are committed and not anything else.

comment:11 Changed 5 months ago by fdupont

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

Merged (14 files in radius directories). Closing.

Note: See TracTickets for help on using tickets.