accel-ppp: T5688: Standardized pool configuration in accel-ppp #2501

aapostoliuk · 2023-11-17T16:56:10Z

Change Summary

Standardized pool configuration for all accel-ppp services.

Only named pools are used now.
Allows all services to use range in x.x.x.x/mask and x.x.x.x-x.x.x.y format
next-pool can be used in all services
Allows to use in ipoe gw-ip-address without pool configuration which allows to use Fraimed-IP-Address attribute by radius.
Default pool name should be explicidly configured with default-pool.
In ipoe netmask and range subnet can be different.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

https://vyos.dev/T5688

Related PR(s)

Component(s) name

l2tp, sstp, pptp, pppoe, ipoe

Proposed changes

Standardized pool configuration for all accel-ppp services.

Only named pools are used now.
Allows all services to use range in x.x.x.x/mask and x.x.x.x-x.x.x.y format
next-pool can be used in all services
Allows to use in ipoe gw-ip-address without pool configuration which allows to use Fraimed-IP-Address attribute by radius.
Default pool name should be explicidly configured with default-pool.
In ipoe netmask and range subnet can be different.
New cli to configure client-ip-pool
For l2tp, sstp, pptp, pppoe:
For example:

set vpn sstp client-ip-pool TEST next-pool 'TEST2'
set vpn sstp client-ip-pool TEST range '10.0.0.1/24'
set vpn sstp client-ip-pool TEST2 range '10.0.1.2-10.0.1.10'
set vpn sstp default-pool 'TEST'
set vpn sstp gateway-address '10.0.10.1'

For ipoe:
Allows multiple gateway-address
For example:

set service ipoe-server client-ip-pool TEST next-pool 'TEST2'
set service ipoe-server client-ip-pool TEST range '10.0.0.3-10.0.0.7'
set service ipoe-server client-ip-pool TEST2 range '10.0.1.2/25'
set service ipoe-server default-pool 'TEST'
set service ipoe-server gateway-address '10.0.0.100/24'
set service ipoe-server gateway-address '10.0.1.1/24'

Smoketests were added
Migration scripts were added

How to test

Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_service_ipoe-server.py
test_accel_ipv4_pool (__main__.TestServiceIPoEServer.test_accel_ipv4_pool) ... ok
test_accel_local_authentication (__main__.TestServiceIPoEServer.test_accel_local_authentication) ...
No IPoE interface configured

ok
test_accel_name_servers (__main__.TestServiceIPoEServer.test_accel_name_servers) ... ok
test_accel_next_pool (__main__.TestServiceIPoEServer.test_accel_next_pool) ... ok
test_accel_radius_authentication (__main__.TestServiceIPoEServer.test_accel_radius_authentication) ... ok

----------------------------------------------------------------------
Ran 5 tests in 17.198s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_service_pppoe-server.py
test_accel_ipv4_pool (__main__.TestServicePPPoEServer.test_accel_ipv4_pool) ... ok
test_accel_local_authentication (__main__.TestServicePPPoEServer.test_accel_local_authentication) ...
User "test" has rate-limit configured for only one direction but both
upload and download must be given!

ok
test_accel_name_servers (__main__.TestServicePPPoEServer.test_accel_name_servers) ... ok
test_accel_next_pool (__main__.TestServicePPPoEServer.test_accel_next_pool) ... ok
test_accel_radius_authentication (__main__.TestServicePPPoEServer.test_accel_radius_authentication) ... ok
test_pppoe_server_authentication_protocols (__main__.TestServicePPPoEServer.test_pppoe_server_authentication_protocols) ... ok
test_pppoe_server_client_ipv6_pool (__main__.TestServicePPPoEServer.test_pppoe_server_client_ipv6_pool) ... ok
test_pppoe_server_ppp_options (__main__.TestServicePPPoEServer.test_pppoe_server_ppp_options) ... ok
test_pppoe_server_shaper (__main__.TestServicePPPoEServer.test_pppoe_server_shaper) ... ok
test_pppoe_server_vlan (__main__.TestServicePPPoEServer.test_pppoe_server_vlan) ... ok

----------------------------------------------------------------------
Ran 10 tests in 35.494s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpn_l2tp.py
test_accel_ipv4_pool (__main__.TestVPNL2TPServer.test_accel_ipv4_pool) ... ok
test_accel_local_authentication (__main__.TestVPNL2TPServer.test_accel_local_authentication) ... ok
test_accel_name_servers (__main__.TestVPNL2TPServer.test_accel_name_servers) ... ok
test_accel_next_pool (__main__.TestVPNL2TPServer.test_accel_next_pool) ... ok
test_accel_radius_authentication (__main__.TestVPNL2TPServer.test_accel_radius_authentication) ... ok

----------------------------------------------------------------------
Ran 5 tests in 15.934s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpn_pptp.py
test_accel_ipv4_pool (__main__.TestVPNPPTPServer.test_accel_ipv4_pool) ... ok
test_accel_local_authentication (__main__.TestVPNPPTPServer.test_accel_local_authentication) ... ok
test_accel_name_servers (__main__.TestVPNPPTPServer.test_accel_name_servers) ... ok
test_accel_next_pool (__main__.TestVPNPPTPServer.test_accel_next_pool) ... ok
test_accel_radius_authentication (__main__.TestVPNPPTPServer.test_accel_radius_authentication) ... ok

----------------------------------------------------------------------
Ran 5 tests in 15.801s

OK
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpn_sstp.py
test_accel_ipv4_pool (__main__.TestVPNSSTPServer.test_accel_ipv4_pool) ... ok
test_accel_local_authentication (__main__.TestVPNSSTPServer.test_accel_local_authentication) ...
User "test" has rate-limit configured for only one direction but both
upload and download must be given!

ok
test_accel_name_servers (__main__.TestVPNSSTPServer.test_accel_name_servers) ... ok
test_accel_next_pool (__main__.TestVPNSSTPServer.test_accel_next_pool) ... ok
test_accel_radius_authentication (__main__.TestVPNSSTPServer.test_accel_radius_authentication) ... ok

----------------------------------------------------------------------
Ran 5 tests in 21.661s

OK

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

dmbaturin

I like the idea but I pointed out a few things that can be improved.

data/templates/accel-ppp/config_chap_secrets_radius.j2

data/templates/accel-ppp/config_ip_pool.j2

dmbaturin · 2023-11-22T13:08:43Z

interface-definitions/include/accel-ppp/client-ip-pool.xml.i

+        </valueHelp>
+        <valueHelp>
+          <format>ipv4range</format>
+          <description>IPv4 address range inside /24 network</description>


accel-ppp disallows larger ranges?

yes. I took it from their manual

x.x.x.x/mask[,name=pool_name][,next=next_pool_name] or x.x.x.x-y[,name=pool_name][,next=next_pool_name] Also specifies range of remote address of ppp interfaces.

Could you link me to the relevant section? The part you quote doesn't say anything about /24 specifically.

https://docs.accel-ppp.org/configuration/ip-pool.html
But these docs are with old info. The latest docs are in manuals from sources of accel-ppp

dmbaturin · 2023-11-22T13:11:13Z

interface-definitions/include/accel-ppp/client-ip-pool.xml.i

+      <description>Name of IP pool</description>
+    </valueHelp>
+    <constraint>
+      <regex>[-_a-zA-Z0-9.]+</regex>


I can't remember if accel-ppp has constraints on pool names, but this regex feels too lax, e.g., it allows ... or -._.- as valid pool names.

I have just copied it from
https://github.com/vyos/vyos-1x/blob/current/interface-definitions/include/accel-ppp/client-ip-pool-name.xml.i
to proceed with compatibility.

interface-definitions/include/accel-ppp/client-ip-pool.xml.i

interface-definitions/include/accel-ppp/default-pool.xml.i

interface-definitions/include/accel-ppp/gateway-address-multi.xml.i

dmbaturin · 2023-11-22T13:16:42Z

interface-definitions/include/accel-ppp/gateway-address-multi.xml.i

+      <description>Default Gateway, mask send to the client</description>
+    </valueHelp>
+    <constraint>
+      <validator name="ipv4-prefix"/>


Do we really allow network addresses like 192.0.2.0/25 as gateway addresses? Technically, in peer-to-peer networks, it may work, but I'd like to clarify that.

Accel-PPP allows it. I have checked it on ipoe and it works.

set service ipoe-server authentication mode 'noauth' set service ipoe-server client-ip-pool TEST range '10.0.0.3-10.0.0.7' set service ipoe-server gateway-address '10.0.0.0/25'

On client side:

PC2> ip dhcp DORA IP 10.0.0.3/25 GW 10.0.0.0 PC2> show ip NAME : PC2[1] IP/MASK : 10.0.0.3/25 GATEWAY : 10.0.0.0 DNS : DHCP SERVER : 10.0.0.0 DHCP LEASE : 595, 600/300/525 MAC : 00:50:79:66:68:01 LPORT : 20025 RHOST:PORT : 127.0.0.1:20026 MTU : 1500 PC2> ping 1.1.1.1 84 bytes from 1.1.1.1 icmp_seq=1 ttl=64 time=0.833 ms 84 bytes from 1.1.1.1 icmp_seq=2 ttl=64 time=1.421 ms

I think it is a network engineer's responsibility to set a proper IP address.

python/vyos/accel_ppp_util.py

dmbaturin · 2023-11-22T13:21:22Z

src/validators/ipv4-range-mask

@@ -0,0 +1,59 @@
+#!/bin/bash


I believe I should add range option to ipaddrcheck. I'll look into it, if it takes a while, we can merge this first, then replace the validator part.

src/migration-scripts/ipoe-server/1-to-2

src/migration-scripts/pppoe-server/6-to-7

src/migration-scripts/pptp/2-to-3

src/migration-scripts/sstp/4-to-5

c-po · 2023-11-30T05:59:02Z

smoketest/scripts/cli/test_vpn_pptp.py

+    def test_accel_name_servers(self):
+        self.basic_config()
+        self.cli_commit()
+        self.assertTrue(True)


Whats the usecase of a smoketest asserting True all the time?

I have changed to skip this test.

I have created test.

c-po · 2023-11-30T05:59:08Z

smoketest/scripts/cli/test_vpn_pptp.py

+    def test_accel_local_authentication(self):
+        self.basic_config()
+        self.cli_commit()
+        self.assertTrue(True)


Whats the usecase of a smoketest asserting True all the time?

I have changed to skip this test.

I have created test.

c-po · 2023-11-30T05:59:20Z

smoketest/scripts/cli/test_vpn_l2tp.py

+    def test_accel_name_servers(self):
+        self.basic_config()
+        self.cli_commit()
+        self.assertTrue(True)


Whats the usecase of a smoketest asserting True all the time?

I have changed to skip this test.

I have created test.

Standardized pool configuration for all accel-ppp services. 1. Only named pools are used now. 2. Allows all services to use range in x.x.x.x/mask and x.x.x.x-x.x.x.y format 3. next-pool can be used in all services 2. Allows to use in ipoe gw-ip-address without pool configuration which allows to use Fraimed-IP-Address attribute by radius. 3. Default pool name should be explicidly configured with default-pool. 4. In ipoe netmask and range subnet can be different.

c-po · 2023-12-28T15:27:03Z

@Mergifyio backport sagitta

mergify · 2023-12-28T15:27:06Z

backport sagitta

✅ Backports have been created

#2700 accel-ppp: T5688: Standardized pool configuration in accel-ppp (backport #2501) has been created for branch sagitta

accel-ppp: T5688: Standardized pool configuration in accel-ppp (backport #2501)

github-actions bot assigned aapostoliuk Nov 17, 2023

github-actions bot added the current label Nov 17, 2023

vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team November 17, 2023 16:56

aapostoliuk force-pushed the T5688-current branch from 25ab253 to bf86c11 Compare November 17, 2023 16:58

aapostoliuk changed the title ~~accel-ppp:T5688:Standardized pool configuration in accel-ppp~~ accel-ppp: T5688: Standardized pool configuration in accel-ppp Nov 17, 2023

aapostoliuk force-pushed the T5688-current branch from bf86c11 to 75fd367 Compare November 17, 2023 17:02

dmbaturin requested changes Nov 22, 2023

View reviewed changes

aapostoliuk force-pushed the T5688-current branch from 75fd367 to ab2fb4b Compare November 22, 2023 15:25

dmbaturin approved these changes Nov 24, 2023

View reviewed changes

c-po approved these changes Nov 27, 2023

View reviewed changes

aapostoliuk force-pushed the T5688-current branch from ab2fb4b to 4c0bd3b Compare November 29, 2023 09:15

c-po requested changes Nov 30, 2023

View reviewed changes

aapostoliuk force-pushed the T5688-current branch from 4c0bd3b to a5ca24a Compare December 1, 2023 11:36

aapostoliuk force-pushed the T5688-current branch from a5ca24a to 422eb46 Compare December 4, 2023 16:12

c-po approved these changes Dec 4, 2023

View reviewed changes

c-po merged commit 8d3e287 into vyos:current Dec 4, 2023
7 checks passed

mergify bot mentioned this pull request Dec 28, 2023

accel-ppp: T5688: Standardized pool configuration in accel-ppp (backport #2501) #2700

Merged

dmbaturin added a commit that referenced this pull request Dec 28, 2023

Merge pull request #2700 from vyos/mergify/bp/sagitta/pr-2501

0e87990

accel-ppp: T5688: Standardized pool configuration in accel-ppp (backport #2501)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accel-ppp: T5688: Standardized pool configuration in accel-ppp #2501

accel-ppp: T5688: Standardized pool configuration in accel-ppp #2501

aapostoliuk commented Nov 17, 2023

dmbaturin left a comment

dmbaturin Nov 22, 2023

aapostoliuk Nov 22, 2023

dmbaturin Nov 22, 2023

aapostoliuk Nov 22, 2023

dmbaturin Nov 22, 2023

aapostoliuk Nov 22, 2023 •

edited

dmbaturin Nov 22, 2023

aapostoliuk Nov 22, 2023

dmbaturin Nov 22, 2023

c-po Nov 30, 2023

aapostoliuk Nov 30, 2023 •

edited

aapostoliuk Dec 4, 2023

c-po Nov 30, 2023

aapostoliuk Nov 30, 2023 •

edited

aapostoliuk Dec 4, 2023

c-po Nov 30, 2023

aapostoliuk Nov 30, 2023 •

edited

aapostoliuk Dec 4, 2023

c-po commented Dec 28, 2023

mergify bot commented Dec 28, 2023 •

edited

accel-ppp: T5688: Standardized pool configuration in accel-ppp #2501

accel-ppp: T5688: Standardized pool configuration in accel-ppp #2501

Conversation

aapostoliuk commented Nov 17, 2023

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

dmbaturin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapostoliuk Nov 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapostoliuk Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapostoliuk Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapostoliuk Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-po commented Dec 28, 2023

mergify bot commented Dec 28, 2023 • edited

✅ Backports have been created

aapostoliuk Nov 22, 2023 •

edited

aapostoliuk Nov 30, 2023 •

edited

aapostoliuk Nov 30, 2023 •

edited

aapostoliuk Nov 30, 2023 •

edited

mergify bot commented Dec 28, 2023 •

edited