Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rewrite eradius #241

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

rewrite eradius #241

wants to merge 8 commits into from

Conversation

RoadRunnr
Copy link
Member

v3 rewrite of eradius, major changes:

  • clients and servers are now started and configured through APIs, not
    app env settings any more
  • IPv6 support
  • supports multiple server and client instances
  • metrics are optional and callback based (allows the easy use of other
    metrics frameworks)
  • distributed handlers are no longer support, use erpc to replicate in
    use case specific code if needed.
  • removed proxy support (use freeradius or similar instead)

README.md Outdated Show resolved Hide resolved
dicts_compiler.erl Outdated Show resolved Hide resolved
dicts_compiler.erl Outdated Show resolved Hide resolved
Seperate concerns better. Socket managing is not in the mngr module,
while the sending logic is in the client module.
There was no logic in place to remove entries from the pending
registry once the timeout hits. The client side timeout would only
handle re-transmission, but not the cleanup.

Move the timeout logic to the sender and let it also cleanup the
pending registry.
Fully prepare client suite for full IPv6 support and add some minor
tweaks to the others to move them closer to IPv6 support.
@0xAX
Copy link
Member

0xAX commented May 6, 2024

removed proxy support (use freeradius or similar instead)

Was it approved for the use-cases where it was is used?

clients and servers are now started and configured through APIs, not
app env settings any more

What does it give? The projects that are using old configuration need to have adjustments to handle previous eradius configuration by themselves (or to completely change/remove it what is not always possible) and to call API instead of eradius did it automatically (start servers/clients 'manually' via API). What is the purpose of it? To have some ability to do things in runtime? The old version reconfigure API, why not to add API/wrappers around set_env like add_server,remove_server and so on but break the behaviour?

@RoadRunnr RoadRunnr force-pushed the feature/next-major branch 3 times, most recently from 022331f to dc3bc8e Compare May 6, 2024 10:20
@RoadRunnr
Copy link
Member Author

removed proxy support (use freeradius or similar instead)

Was it approved for the use-cases where it was is used?

There is no need to move existing use-case to the new code. If there is something that uses proxing, it can continue to use the old version.
However, I would strongly recommend to move those project to something else.

clients and servers are now started and configured through APIs, not
app env settings any more

What does it give? The projects that are using old configuration need to have adjustments to handle previous eradius configuration by themselves (or to completely change/remove it what is not always possible) and to call API instead of eradius did it automatically (start servers/clients 'manually' via API). What is the purpose of it? To have some ability to do things in runtime? The old version reconfigure API, why not to add API/wrappers around set_env like add_server,remove_server and so on but break the behaviour?

Again, existing projects can stay with the version they are using. There is nothing that forces them to move.
The changes in configuration are just to big to provide sensible compatibility mappers. It is saner if projects want to use the version, that they migrate to the new API.

@RoadRunnr RoadRunnr force-pushed the feature/next-major branch 2 times, most recently from ec43b3c to 5a4066d Compare May 7, 2024 15:41
@RoadRunnr RoadRunnr marked this pull request as ready for review May 8, 2024 07:02
@RoadRunnr RoadRunnr requested a review from a team as a code owner May 8, 2024 07:02
ebengt
ebengt previously approved these changes May 8, 2024
Copy link

@ebengt ebengt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

dicts_compiler.erl Outdated Show resolved Hide resolved
src/eradius_client_socket.erl Outdated Show resolved Hide resolved
src/eradius_client_socket.erl Show resolved Hide resolved
}.
%% Options to configure the RADIUS server UDP socket.

-type socket_config() :: #{family := inet | inet6,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks the same as socket_opts().
Why have both? (Pease keep socket_config() if one is enough)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, they are different. In socket_opts() all map entries are optional, in socket_config() some keys are mandatory.

It is not too obvious, but using := vs. => in map type specs make a difference.


%% @private
handle_cast(_Msg, State) ->
{noreply, State}.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps log that something unexpected was casted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is casting anything to the server. The method is only there because the gen_server behavior mandates it.

v3 rewrite of eradius, major changes:
* clients and servers are now started and configured through APIs, not
  app env settings any more
* IPv6 support
* supports multiple server and client instances
* metrics are optional and callback based (allows the easy use of other
  metrics frameworks)
* distributed handlers are no longer support, use erpc to replicate in
  use case specific code if needed.
* removed proxy support (use freeradius or similar instead)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants