Opened 8 months ago

Closed 5 months ago

#5527 closed enhancement (complete)

Implement Host backend using Radius data

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: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 24
Total Hours: 0 Internal?: no

Description

Once #5525 (config structures) and #5526 (Radius communication) are done, the next step will be to implement Host Data Source implementation that will use Radius to retrieve the actual information about specific clients.

For details, see RadiusDesign.

Subtickets

Change History (9)

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

What is the right behavior in case of errors (e.g. RADIUS server not answering)?

comment:3 Changed 6 months ago by fdupont

  • Add Hours to Ticket changed from 0 to 24
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Code and tests done. Even the doc is still todo (despite there is a doc ticket) I believe it is ready for early review (after preceding Radius tickets).

comment:4 Changed 5 months ago by fdupont

Tried to rebase using git without success. Retried manually, it worked well but git diff is still broken. Fortunately plain diff works.
Going on trac5530 to do the same.

comment:5 Changed 5 months ago by fdupont

Got 4000 lines diff for git diff master trac5527 or git diff master..trac5527 (but not for git diff master...trac5527). Same for trac5530.

comment:6 Changed 5 months ago by tomek

  • Owner changed from UnAssigned to tomek

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

  • Owner changed from tomek to fdupont

Ok. The trac5527 was broken beyond all repair. I tried to rebase it,
but due to the code being copied between branches and comitted again,
the amount of conflicts was insane. I gave up around 28th
commit. Therefore I did a clean start: compared master and trac5527
using diff, then committed those changes there.The branch name is
trac5527_clean2.

With that in place, I was able to use git diff again.

Note this changed the git log history. I did mention you explicitly in
the commit log. However, care about this and want the git log to show
you as the actual author, feel free to do this:

git checkout master
git branch -b trac5527_clean3
git apply < git diff 4d600b50665d8ce9931e8e557df1a86daa8b10e0 6ce97e0705f1e099469eb36be438abe370544e04
git commit
git cherry-pick 67a0f40da23f9f272303c666b7c75d95b577fb63
git cherry-pick d2d41c5ff34cf6fdef62eb70f4a16108abade13f
git cherry-pick 9686fcaa84f21ade629e3eebabf04beb01bb2167
git cherry-pick c7bdf0f9575e0a2ee377be8519a4b447445d56c3

And then push your trac5527_clean3 branch.

radius_backend.h

The method descriptions are not necessary if the method doesn't do
anything. Removed. Please pull and review.

radius_backend.cc
Extra return at the end of get6. Removed.

I'm not sure I understand how the get4, get6 methods in
RadiusBackendImpl? are supposed to work. Both of them do only logging
and bump up the unexpected counter. There are no calls to Radius,
except checking id_type_ and id_type_6.

It seems the actual Radius calls are done in subnetX_select callouts.
Why not do them in the get4, get6 methods of RadiusBackendImpl?.

radius_access.c
There was some code commented out. Removed.

radius_access.h
The comment for RadiusAuthEnv? should explain what the environment is.


I've pushed some fixes to trac5527_clean2. Please pull and review.


The code builds and unit-tests pass on Mac OS 10.12.6.
The code buillds on CentOS7 with boost 1.65.1 installed.


This change requires a changelog. Here's my proposal:

41.	[func]		fdupont
	Radius backend implemented. It is now possible to retrieve
	client information including reserved address and client
	pool from a RADIUS server.
	(Trac #5527, git tbd)
	(Trac #5529, git tbd)

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

Replying to tomek:

Ok. The trac5527 was broken beyond all repair. I tried to rebase it,
but due to the code being copied between branches and comitted again,
the amount of conflicts was insane. I gave up around 28th
commit. Therefore I did a clean start: compared master and trac5527
using diff, then committed those changes there.The branch name is
trac5527_clean2.

=> I'll see tomorrow what are your changes as I can't see them
by any single git command outside new files.

radius_backend.h

The method descriptions are not necessary if the method doesn't do
anything. Removed. Please pull and review.

=> I disagree about not implemented: it is not the same than
doing nothing. I have the right comment in the host cache code so
I'll update them. And you missed the role of the backend, it really
does nothing because it is called late and RADIUS code is earlier
and puts results in the host cache. So the comment is wrong.

radius_backend.cc

=> add must not throw.

I'm not sure I understand how the get4, get6 methods in
RadiusBackendImpl? are supposed to work. Both of them do only logging
and bump up the unexpected counter. There are no calls to Radius,
except checking id_type_ and id_type_6.

=> answers are supposed to be in the host cache so these methods
only check if they are called in expected cases. Unexpected cases
are really unexpected, i.e., they are not supposed to happen if the
hook is used correctly.

It seems the actual Radius calls are done in subnetX_select callouts.
Why not do them in the get4, get6 methods of RadiusBackendImpl?.

=> because the subnet select callout gives more informations,
for instance the query message. Note it is the first callout in processing
which has every needed pieces, in particular the selected subnet ID.

radius_access.cc
There was some code commented out. Removed.

=> the diff shows no change. IMHO your comment is about the
setIOService commented call.

radius_access.h
The comment for RadiusAuthEnv? should explain what the environment is.

=> I noted you didn't change it (so I have to address it).

I've pushed some fixes to trac5527_clean2. Please pull and review.

=> if you remember the commit number I'll be able to cherry-pick it
into trac5527.

The code builds and unit-tests pass on Mac OS 10.12.6.

=> and likely 10.13.4

The code buillds on CentOS7 with boost 1.65.1 installed.

=> I have updated my CentOS 7 VM so I should be soon able
to reproduce things on it.

This change requires a changelog. Here's my proposal:

41.	[func]		fdupont
	Radius backend implemented. It is now possible to retrieve
	client information including reserved address and client
	pool from a RADIUS server.
	(Trac #5527, git tbd)
	(Trac #5529, git tbd)

=> fine. I'll finish this tomorrow.

comment:9 Changed 5 months ago by fdupont

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

Merge done. A test merge shows a reasonable difference set with clean2 so it should be good.
Closing.

Note: See TracTickets for help on using tickets.