Skip to content

Commit fcc3b29

Browse files
committed
Drop the use of classids files
We base the classid needed for tc rules off of IP (last 2 octet) hash instead of writing to a file everytime a device connects. Change-type: patch Fibery: https://balena.fibery.io/Work/Project/(Cloudlink)-Monitor-and-ready-Bandwidth-limiting-and-openVPN-exporter-for-production-1731 Signed-off-by: jaomaloy <jan.maloy@balena.io>
1 parent 5bb7da5 commit fcc3b29

File tree

4 files changed

+142
-67
lines changed

4 files changed

+142
-67
lines changed

README.md

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,12 +82,6 @@ Both parameters must be specified to enable throttling. Rate formats supported:
8282

8383
For troubleshooting throttling issues:
8484

85-
* `LEARN_ADDRESS_DEBUG=1` - Enable detailed logging of tc operations
85+
* `LEARN_ADDRESS_DEBUG=true` - Enable detailed logging of tc operations
8686
* `LEARN_ADDRESS_STATE_DIR` - Custom state directory (default: `/var/lib/openvpn/tc-state`)
8787
* `LEARN_ADDRESS_LOG_DIR` - Custom log directory (default: `/var/log/openvpn`)
88-
89-
### Production Considerations
90-
91-
- Set `LEARN_ADDRESS_DEBUG=0` in production to minimize logging overhead
92-
- Monitor state directory growth (`/var/lib/openvpn/tc-state`)
93-
- Consider log rotation for debug logs when enabled

docker-compose.test.yml

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,3 @@ services:
5959
6060
/command/with-contenv npm run test-unit
6161
/command/with-contenv npx mocha test/app.ts
62-
63-
# Test the learn-address script directly
64-
env LEARN_ADDRESS_DEBUG=true /usr/src/app/openvpn/scripts/learn-address.sh 10mbit 5mbit add 10.0.0.100 test-client debug lo
65-
# Check if log file was created and show its contents
66-
ls -la /var/log/openvpn/
67-
cat /var/log/openvpn/learn-address.log

openvpn/scripts/learn-address.sh

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -89,27 +89,23 @@ function trace() {
8989
fi
9090
}
9191

92+
function compute_classid() {
93+
# Compute unique classid from IP address (last 2 octets)
94+
# This ensures deterministic mapping from IP to classid
95+
local ip=$1
96+
IFS='.' read -r oct1 oct2 oct3 oct4 <<< "$ip"
97+
echo $(( (oct3 * 256 + oct4) % 65534 + 1 ))
98+
}
99+
92100
function bwlimit-enable() {
93101
ip=$1
94102

95-
trace "PRE-ENABLE" "$dev"
103+
trace "PRE-ENABLE" "$dev"
96104

97105
# Disable if already enabled.
98106
bwlimit-disable $ip
99107

100-
# Find unique classid.
101-
if [ -f $statedir/$ip.classid ]; then
102-
# Reuse this IP's classid
103-
classid=`cat $statedir/$ip.classid`
104-
else
105-
if [ -f $statedir/last_classid ]; then
106-
classid=`cat $statedir/last_classid`
107-
classid=$((classid+1))
108-
else
109-
classid=1
110-
fi
111-
echo $classid > $statedir/last_classid
112-
fi
108+
classid=$(compute_classid "$ip")
113109

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

137-
# Store classid and dev for further use.
138-
echo $classid > $statedir/$ip.classid
139133
echo $dev > $statedir/$ip.dev
140134

141-
trace "POST-ENABLE" "$dev"
135+
trace "POST-ENABLE" "$dev"
142136
}
143137

144138
function bwlimit-disable() {
145139
ip=$1
146140

147-
if [ ! -f $statedir/$ip.classid ]; then
148-
return
149-
fi
150141
if [ ! -f $statedir/$ip.dev ]; then
151142
return
152143
fi
153144

154-
classid=`cat $statedir/$ip.classid`
145+
classid=$(compute_classid "$ip")
155146

156-
local dev_from_state
157-
dev_from_state=$(cat "$statedir/$ip.dev")
147+
local dev_from_state
148+
dev_from_state=$(cat "$statedir/$ip.dev")
158149

159-
trace "PRE-DISABLE" "$dev_from_state"
150+
trace "PRE-DISABLE" "$dev_from_state"
160151

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

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

173-
trace "POST-DISABLE" "$dev_from_state"
163+
trace "POST-DISABLE" "$dev_from_state"
174164
}
175165

176166
# Make sure queueing discipline is enabled on the device

test/throttling.ts

Lines changed: 125 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,8 @@ export default () => {
6262
// Script should succeed (note: tc commands may fail on loopback but script should handle gracefully)
6363
expect(code).to.equal(0);
6464

65-
// Check that state files are created
65+
// Check that .dev state file is created
6666
const stateFiles = fs.readdirSync(tempDir);
67-
expect(stateFiles).to.include('10.0.0.1.classid');
6867
expect(stateFiles).to.include('10.0.0.1.dev');
6968

7069
done();
@@ -104,72 +103,170 @@ export default () => {
104103
deleteChild.on('exit', (code) => {
105104
expect(code).to.equal(0);
106105

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

112109
done();
113110
});
114111
});
115112
});
116113

117-
it('should reuse classids for reconnecting clients', (done) => {
118-
// Add client
119-
const addChild = spawn(
114+
it('should compute deterministic classid from IP address', (done) => {
115+
// Test the compute_classid function indirectly by running the script
116+
// and checking the classid in debug logs
117+
const testCases = [
118+
// classid = (oct3 * 256 + oct4) % 65534 + 1
119+
{ ip: '10.0.0.1', expectedClassid: 2 },
120+
{ ip: '10.0.0.255', expectedClassid: 256 },
121+
{ ip: '10.0.1.0', expectedClassid: 257 },
122+
{ ip: '10.0.1.1', expectedClassid: 258 },
123+
{ ip: '10.0.255.255', expectedClassid: 2 },
124+
];
125+
126+
let completed = 0;
127+
testCases.forEach(({ ip, expectedClassid }, index) => {
128+
// Create a separate log directory for each test case to avoid conflicts
129+
const logDir = fs.mkdtempSync(
130+
path.join(os.tmpdir(), `throttling-log-${index}-`),
131+
);
132+
133+
const child = spawn(
134+
'bash',
135+
[scriptPath, '5mbit', '1mbit', 'add', ip, 'client1', 'debug', 'lo'],
136+
{
137+
env: {
138+
...process.env,
139+
LEARN_ADDRESS_STATE_DIR: tempDir,
140+
LEARN_ADDRESS_LOG_DIR: logDir,
141+
},
142+
},
143+
);
144+
145+
child.on('exit', () => {
146+
const logFile = path.join(logDir, 'learn-address.log');
147+
const logContent = fs.readFileSync(logFile, 'utf8');
148+
149+
// Look for the classid in the log (format: "classid=1:XXX")
150+
const match = logContent.match(/classid=1:(\d+)/);
151+
expect(match).to.not.be.null;
152+
153+
if (match) {
154+
const classid = parseInt(match[1], 10);
155+
expect(classid).to.equal(
156+
expectedClassid,
157+
`IP ${ip} should produce classid ${expectedClassid}`,
158+
);
159+
}
160+
161+
fs.rmSync(logDir, { recursive: true, force: true });
162+
163+
completed++;
164+
if (completed === testCases.length) {
165+
done();
166+
}
167+
});
168+
});
169+
});
170+
171+
it('should ensure classid is always in valid range (1-65535)', (done) => {
172+
// Test edge cases to ensure classid never exceeds valid range for tc
173+
const edgeCases = [
174+
'10.0.0.0', // Minimum possible
175+
'10.0.255.254', // Near maximum
176+
'10.255.255.255', // Maximum possible in /8
177+
];
178+
179+
let completed = 0;
180+
edgeCases.forEach((ip, index) => {
181+
// Create a separate log directory for each test case to avoid conflicts
182+
const logDir = fs.mkdtempSync(
183+
path.join(os.tmpdir(), `throttling-range-${index}-`),
184+
);
185+
186+
const child = spawn(
187+
'bash',
188+
[scriptPath, '5mbit', '1mbit', 'add', ip, 'client1', 'debug', 'lo'],
189+
{
190+
env: {
191+
...process.env,
192+
LEARN_ADDRESS_STATE_DIR: tempDir,
193+
LEARN_ADDRESS_LOG_DIR: logDir,
194+
},
195+
},
196+
);
197+
198+
child.on('exit', () => {
199+
const logFile = path.join(logDir, 'learn-address.log');
200+
const logContent = fs.readFileSync(logFile, 'utf8');
201+
202+
// Look for the classid in the log (format: "classid=1:XXX")
203+
const match = logContent.match(/classid=1:(\d+)/);
204+
expect(match).to.not.be.null;
205+
206+
if (match) {
207+
const classid = parseInt(match[1], 10);
208+
expect(classid).to.be.at.least(1);
209+
expect(classid).to.be.at.most(65535);
210+
}
211+
212+
fs.rmSync(logDir, { recursive: true, force: true });
213+
214+
completed++;
215+
if (completed === edgeCases.length) {
216+
done();
217+
}
218+
});
219+
});
220+
});
221+
222+
it('should produce same classid for reconnecting client with same IP', (done) => {
223+
const firstAdd = spawn(
120224
'bash',
121-
[scriptPath, '5mbit', '1mbit', 'add', '10.0.0.1', 'client1'],
225+
[scriptPath, '5mbit', '1mbit', 'add', '10.0.2.50', 'client1'],
122226
{
123227
env: {
124228
...process.env,
125229
LEARN_ADDRESS_STATE_DIR: tempDir,
126-
LEARN_ADDRESS_DEBUG: '0',
230+
LEARN_ADDRESS_DEBUG: '1',
127231
dev: 'lo',
128232
},
129233
},
130234
);
131235

132-
addChild.on('exit', () => {
133-
const classid1 = fs
134-
.readFileSync(path.join(tempDir, '10.0.0.1.classid'), 'utf8')
135-
.trim();
136-
137-
// Delete client
138-
const deleteChild = spawn(
236+
firstAdd.on('exit', () => {
237+
const deleteClient = spawn(
139238
'bash',
140-
[scriptPath, '5mbit', '1mbit', 'delete', '10.0.0.1', 'client1'],
239+
[scriptPath, '5mbit', '1mbit', 'delete', '10.0.2.50'],
141240
{
142241
env: {
143242
...process.env,
144243
LEARN_ADDRESS_STATE_DIR: tempDir,
145-
LEARN_ADDRESS_DEBUG: '0',
146244
dev: 'lo',
147245
},
148246
},
149247
);
150248

151-
deleteChild.on('exit', () => {
152-
// Add client again
153-
const addAgainChild = spawn(
249+
deleteClient.on('exit', () => {
250+
// Reconnect with same IP
251+
const secondAdd = spawn(
154252
'bash',
155-
[scriptPath, '5mbit', '1mbit', 'add', '10.0.0.1', 'client1'],
253+
[scriptPath, '5mbit', '1mbit', 'add', '10.0.2.50', 'client1'],
156254
{
157255
env: {
158256
...process.env,
159257
LEARN_ADDRESS_STATE_DIR: tempDir,
160-
LEARN_ADDRESS_DEBUG: '0',
258+
LEARN_ADDRESS_DEBUG: '1',
161259
dev: 'lo',
162260
},
163261
},
164262
);
165263

166-
addAgainChild.on('exit', (code) => {
264+
secondAdd.on('exit', (code) => {
167265
expect(code).to.equal(0);
168266

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

174271
done();
175272
});

0 commit comments

Comments
 (0)