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

Adding support to Radius IPV6 address #115

Merged
merged 1 commit into from Dec 7, 2023

Conversation

leonardomeres
Copy link
Contributor

This change enables ipv6 connections between accel-ppp and radius server. It was implemented to meet an user request:
https://phabricator.accel-ppp.org/T71

To use ipv6 address just add the server address to the radius section in the config file. This code will check if the address is ipv4 or ipv6.

[radius]
gw-ip-address=10.10.0.1
nas-identifier=accel-ppp
server=2001:db8::1,password,auth-port=1812,acct-port=1813,req-limit=0,fail-timeout=0,max-fail=0,weight=1

It also can be used with ipv4 backup servers

[radius]
gw-ip-address=10.10.0.1
nas-identifier=accel-ppp
server=192.168.0.169,password,auth-port=1812,acct-port=1813,req-limit=0,fail-timeout=0,max-fail=0,weight=1
server=2001:db8::1,password,auth-port=1812,acct-port=1813,req-limit=0,fail-timeout=0,max-fail=0,weight=1

@leonardomeres
Copy link
Contributor Author

leonardomeres commented Nov 20, 2023

Hi @DmitriyEshenko , can you run these tests again? I had a look in the test (test_ipoe_shared_session_wo_auth.py) logs and didn't find a reason for this failure due to the changes made in this pull request. The same test passed on the other Debian versions, so i'm im thinking if it was not a issue at the test execution.
I also run the tests in my forked repo and all passed. What do you think?

@DmitriyEshenko
Copy link
Member

Thanks for contributing. It looks like need to add additional functionality to properly show RADIUS IP address in accel-cmd show stat output.

@micron10
Copy link

Is it possible to add support of domain name .
i will share patch that work for ipv4 domain name.

have one little problem if system on time of boot and start service dont have internet not resolv domain and service start without radius.

when radius is set as domain name and change ip of domain after reload service get new ip and set as radius server.

@leonardomeres
Copy link
Contributor Author

Thanks for contributing. It looks like need to add additional functionality to properly show RADIUS IP address in accel-cmd show stat output.

Hi @DmitriyEshenko . Well noted. I will add this in a new commit to this pull request.

Thanks

@micron10
Copy link

Hi @leonardomeres

If its possible to implement this patch over your will be more easy to type ipv4 or ipv6 addr as domain name.
accelppp-dpdk-T25-radius-module-allow-fqdn-based-server-addresses.patch

@leonardomeres
Copy link
Contributor Author

Hi @leonardomeres

If its possible to implement this patch over your will be more easy to type ipv4 or ipv6 addr as domain name. accelppp-dpdk-T25-radius-module-allow-fqdn-based-server-addresses.patch

Hi @micron10, I think that this fqdn implementations should be done in a separated pull request. Also you should add this request to the https://phabricator.accel-ppp.org/ site to be properly tracked by the Accel-PPP community and maintainers.

Regarding the problems with your patch implementation I believe that there are some ways of working around it and make it work, but this should be discussed with the maintainers to follow the best approach. As a personal opinion I don't see a reason for a radius server be changing it's IP addresses frequently, so I would like to ask you why do you need this functionality? Maybe there are other better ways to achieve what you need.
Thanks

@micron10
Copy link

This is only idea for more easy write ipv6 addr

@DmitriyEshenko
Copy link
Member

DmitriyEshenko commented Nov 30, 2023

Thanks for improving. Before code master review, could you please rebase your commits?

This change enables ipv6 connections between accel-ppp and radius server
@leonardomeres
Copy link
Contributor Author

Thanks for improving. Before code master review, could you please rebase your commits?

Hi @DmitriyEshenko . Rebase done. Thanks

@nuclearcat nuclearcat self-requested a review December 1, 2023 10:30
Copy link
Member

@nuclearcat nuclearcat 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 to me

@DmitriyEshenko DmitriyEshenko merged commit c08e87e into accel-ppp:master Dec 7, 2023
19 checks passed
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.

None yet

4 participants