Skip to content

Drop the use of classids files #636

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.

Sign up for GitHub

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

Open
jaomaloy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Drop the use of classids files #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,6 @@ Both parameters must be specified to enable throttling. Rate formats supported:

For troubleshooting throttling issues:

* `LEARN_ADDRESS_DEBUG=1` - Enable detailed logging of tc operations
* `LEARN_ADDRESS_DEBUG=true` - Enable detailed logging of tc operations
* `LEARN_ADDRESS_STATE_DIR` - Custom state directory (default: `/var/lib/openvpn/tc-state`)
* `LEARN_ADDRESS_LOG_DIR` - Custom log directory (default: `/var/log/openvpn`)

### Production Considerations

- Set `LEARN_ADDRESS_DEBUG=0` in production to minimize logging overhead
- Monitor state directory growth (`/var/lib/openvpn/tc-state`)
- Consider log rotation for debug logs when enabled
6 changes: 0 additions & 6 deletions docker-compose.test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,3 @@ services:

/command/with-contenv npm run test-unit
/command/with-contenv npx mocha test/app.ts

# Test the learn-address script directly
env LEARN_ADDRESS_DEBUG=true /usr/src/app/openvpn/scripts/learn-address.sh 10mbit 5mbit add 10.0.0.100 test-client debug lo
# Check if log file was created and show its contents
ls -la /var/log/openvpn/
cat /var/log/openvpn/learn-address.log
50 changes: 24 additions & 26 deletions openvpn/scripts/learn-address.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,27 +89,31 @@ function trace() {
fi
}

function compute_classid() {
# Derives a tc classid (1–65534) deterministically from the client IP
# (last 2 octets), eliminating the need for persistent classid state files.
#
# Collision safety: VPN_INSTANCE_SUBNET_BITMASK is clamped to a minimum of
# /16 (see src/utils/config.ts), so oct1 and oct2 are always fixed per
# instance. The (oct3 * 256 + oct4) expression therefore produces a unique
# index for every client address in the pool. The only wrap-around at
# index 65534 maps to classid 1, which would only alias the network base
# address (e.g. 100.64.0.0) — never assigned to a client. In practice,
# max-clients 32768 keeps the pool well within half of this space.
local ip=$1
IFS='.' read -r oct1 oct2 oct3 oct4 <<< "$ip"
echo $(( (oct3 * 256 + oct4) % 65534 + 1 ))
}

function bwlimit-enable() {
ip=$1

trace "PRE-ENABLE" "$dev"
trace "PRE-ENABLE" "$dev"

# Disable if already enabled.
bwlimit-disable $ip

# Find unique classid.
if [ -f $statedir/$ip.classid ]; then
# Reuse this IP's classid
classid=`cat $statedir/$ip.classid`
else
if [ -f $statedir/last_classid ]; then
classid=`cat $statedir/last_classid`
classid=$((classid+1))
else
classid=1
fi
echo $classid > $statedir/last_classid
fi
classid=$(compute_classid "$ip")

# Limit traffic from VPN server to client (download)
if [[ $DEBUG -eq 1 ]]; then
Expand All @@ -134,29 +138,24 @@ function bwlimit-enable() {
echo "[ERROR] Failed to add tc ingress filter for client $ip" >&2
fi

# Store classid and dev for further use.
echo $classid > $statedir/$ip.classid
echo $dev > $statedir/$ip.dev

trace "POST-ENABLE" "$dev"
trace "POST-ENABLE" "$dev"
}

function bwlimit-disable() {
ip=$1

if [ ! -f $statedir/$ip.classid ]; then
return
fi
if [ ! -f $statedir/$ip.dev ]; then
return
fi

classid=`cat $statedir/$ip.classid`
classid=$(compute_classid "$ip")

local dev_from_state
dev_from_state=$(cat "$statedir/$ip.dev")
local dev_from_state
dev_from_state=$(cat "$statedir/$ip.dev")

trace "PRE-DISABLE" "$dev_from_state"
trace "PRE-DISABLE" "$dev_from_state"

# Remove tc rules with proper error handling
if [[ $DEBUG -eq 1 ]]; then
Expand All @@ -167,10 +166,9 @@ function bwlimit-disable() {
tc class del dev "$dev_from_state" classid "1:$classid" 2>/dev/null || true
tc filter del dev "$dev_from_state" parent ffff: protocol all prio 1 u32 match ip src "$ip/32" 2>/dev/null || true

# Remove .dev but keep .classid so it can be reused.
rm -f "$statedir/$ip.dev"

trace "POST-DISABLE" "$dev_from_state"
trace "POST-DISABLE" "$dev_from_state"
}

# Make sure queueing discipline is enabled on the device
Expand Down
153 changes: 125 additions & 28 deletions test/throttling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ export default () => {
// Script should succeed (note: tc commands may fail on loopback but script should handle gracefully)
expect(code).to.equal(0);

// Check that state files are created
// Check that .dev state file is created
const stateFiles = fs.readdirSync(tempDir);
expect(stateFiles).to.include('10.0.0.1.classid');
expect(stateFiles).to.include('10.0.0.1.dev');

done();
Expand Down Expand Up @@ -104,72 +103,170 @@ export default () => {
deleteChild.on('exit', (code) => {
expect(code).to.equal(0);

// Check that .dev file is removed but .classid remains for reuse
const stateFiles = fs.readdirSync(tempDir);
expect(stateFiles).to.include('10.0.0.1.classid');
expect(stateFiles).to.not.include('10.0.0.1.dev');

done();
});
});
});

it('should reuse classids for reconnecting clients', (done) => {
// Add client
const addChild = spawn(
it('should compute deterministic classid from IP address', (done) => {
// Test the compute_classid function indirectly by running the script
// and checking the classid in debug logs
const testCases = [
// classid = (oct3 * 256 + oct4) % 65534 + 1
{ ip: '10.0.0.1', expectedClassid: 2 },
{ ip: '10.0.0.255', expectedClassid: 256 },
{ ip: '10.0.1.0', expectedClassid: 257 },
{ ip: '10.0.1.1', expectedClassid: 258 },
{ ip: '10.0.255.255', expectedClassid: 2 },
];

let completed = 0;
testCases.forEach(({ ip, expectedClassid }, index) => {
// Create a separate log directory for each test case to avoid conflicts
const logDir = fs.mkdtempSync(
path.join(os.tmpdir(), `throttling-log-${index}-`),
);

const child = spawn(
'bash',
[scriptPath, '5mbit', '1mbit', 'add', ip, 'client1', 'debug', 'lo'],
{
env: {
...process.env,
LEARN_ADDRESS_STATE_DIR: tempDir,
LEARN_ADDRESS_LOG_DIR: logDir,
},
},
);

child.on('exit', () => {
const logFile = path.join(logDir, 'learn-address.log');
const logContent = fs.readFileSync(logFile, 'utf8');

// Look for the classid in the log (format: "classid=1:XXX")
const match = logContent.match(/classid=1:(\d+)/);
expect(match).to.not.be.null;

if (match) {
const classid = parseInt(match[1], 10);
expect(classid).to.equal(
expectedClassid,
`IP ${ip} should produce classid ${expectedClassid}`,
);
}

fs.rmSync(logDir, { recursive: true, force: true });

completed++;
if (completed === testCases.length) {
done();
}
});
});
});

it('should ensure classid is always in valid range (1-65535)', (done) => {
// Test edge cases to ensure classid never exceeds valid range for tc
const edgeCases = [
'10.0.0.0', // Minimum possible
'10.0.255.254', // Near maximum
'10.255.255.255', // Maximum possible in /8
];

let completed = 0;
edgeCases.forEach((ip, index) => {
// Create a separate log directory for each test case to avoid conflicts
const logDir = fs.mkdtempSync(
path.join(os.tmpdir(), `throttling-range-${index}-`),
);

const child = spawn(
'bash',
[scriptPath, '5mbit', '1mbit', 'add', ip, 'client1', 'debug', 'lo'],
{
env: {
...process.env,
LEARN_ADDRESS_STATE_DIR: tempDir,
LEARN_ADDRESS_LOG_DIR: logDir,
},
},
);

child.on('exit', () => {
const logFile = path.join(logDir, 'learn-address.log');
const logContent = fs.readFileSync(logFile, 'utf8');

// Look for the classid in the log (format: "classid=1:XXX")
const match = logContent.match(/classid=1:(\d+)/);
expect(match).to.not.be.null;

if (match) {
const classid = parseInt(match[1], 10);
expect(classid).to.be.at.least(1);
expect(classid).to.be.at.most(65535);
}

fs.rmSync(logDir, { recursive: true, force: true });

completed++;
if (completed === edgeCases.length) {
done();
}
});
});
});

it('should produce same classid for reconnecting client with same IP', (done) => {
const firstAdd = spawn(
'bash',
[scriptPath, '5mbit', '1mbit', 'add', '10.0.0.1', 'client1'],
[scriptPath, '5mbit', '1mbit', 'add', '10.0.2.50', 'client1'],
{
env: {
...process.env,
LEARN_ADDRESS_STATE_DIR: tempDir,
LEARN_ADDRESS_DEBUG: '0',
LEARN_ADDRESS_DEBUG: '1',
dev: 'lo',
},
},
);

addChild.on('exit', () => {
const classid1 = fs
.readFileSync(path.join(tempDir, '10.0.0.1.classid'), 'utf8')
.trim();

// Delete client
const deleteChild = spawn(
firstAdd.on('exit', () => {
const deleteClient = spawn(
'bash',
[scriptPath, '5mbit', '1mbit', 'delete', '10.0.0.1', 'client1'],
[scriptPath, '5mbit', '1mbit', 'delete', '10.0.2.50'],
{
env: {
...process.env,
LEARN_ADDRESS_STATE_DIR: tempDir,
LEARN_ADDRESS_DEBUG: '0',
dev: 'lo',
},
},
);

deleteChild.on('exit', () => {
// Add client again
const addAgainChild = spawn(
deleteClient.on('exit', () => {
// Reconnect with same IP
const secondAdd = spawn(
'bash',
[scriptPath, '5mbit', '1mbit', 'add', '10.0.0.1', 'client1'],
[scriptPath, '5mbit', '1mbit', 'add', '10.0.2.50', 'client1'],
{
env: {
...process.env,
LEARN_ADDRESS_STATE_DIR: tempDir,
LEARN_ADDRESS_DEBUG: '0',
LEARN_ADDRESS_DEBUG: '1',
dev: 'lo',
},
},
);

addAgainChild.on('exit', (code) => {
secondAdd.on('exit', (code) => {
expect(code).to.equal(0);

const classid2 = fs
.readFileSync(path.join(tempDir, '10.0.0.1.classid'), 'utf8')
.trim();
expect(classid1).to.equal(classid2);
// Calculate expected classid: (2 * 256 + 50) % 65534 + 1 = 563
const expectedClassid = ((2 * 256 + 50) % 65534) + 1;
expect(expectedClassid).to.equal(563);

done();
});
Expand Down
Loading