mirror of
https://github.com/fosrl/pangolin.git
synced 2026-03-11 13:16:38 +00:00
fix: simplify path encoding per review — inline utils, use single key scheme
Address PR review comments: - Remove pathUtils.ts and move sanitize/encodePath directly into utils.ts - Simplify dual-key approach to single key using encodePath for map keys - Remove backward-compat logic (not needed per reviewer) - Update tests to match simplified approach
This commit is contained in:
Shreyas Papinwar
@@ -1,14 +1,30 @@
|
||||
import { assertEquals } from "../../../test/assert";
|
||||
import { encodePath, sanitize } from "./pathUtils";
|
||||
|
||||
// ── Pure function copies (inlined to avoid pulling in server dependencies) ──
|
||||
|
||||
function sanitize(input: string | null | undefined): string | undefined {
|
||||
if (!input) return undefined;
|
||||
if (input.length > 50) {
|
||||
input = input.substring(0, 50);
|
||||
}
|
||||
return input
|
||||
.replace(/[^a-zA-Z0-9-]/g, "-")
|
||||
.replace(/-+/g, "-")
|
||||
.replace(/^-|-$/g, "");
|
||||
}
|
||||
|
||||
function encodePath(path: string | null | undefined): string {
|
||||
if (!path) return "";
|
||||
return path.replace(/[^a-zA-Z0-9]/g, (ch) => {
|
||||
return ch.charCodeAt(0).toString(16);
|
||||
});
|
||||
}
|
||||
|
||||
// ── Helpers ──────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Exact replica of the OLD key computation from upstream main.
|
||||
* This is what existing Pangolin deployments use today for both
|
||||
* map grouping AND Traefik router/service names.
|
||||
*
|
||||
* Source: origin/main server/lib/traefik/getTraefikConfig.ts lines 130-146
|
||||
* Uses sanitize() for paths — this is what had the collision bug.
|
||||
*/
|
||||
function oldKeyComputation(
|
||||
resourceId: number,
|
||||
@@ -27,11 +43,8 @@ function oldKeyComputation(
|
||||
}
|
||||
|
||||
/**
|
||||
* Replica of the NEW dual-key computation from our fix.
|
||||
* Returns both the internal map key (for grouping) and the
|
||||
* Traefik-facing key (for router/service names).
|
||||
*
|
||||
* Source: our getTraefikConfig.ts lines 135-163
|
||||
* Replica of the NEW key computation from our fix.
|
||||
* Uses encodePath() for paths — collision-free.
|
||||
*/
|
||||
function newKeyComputation(
|
||||
resourceId: number,
|
||||
@@ -39,49 +52,20 @@ function newKeyComputation(
|
||||
pathMatchType: string | null,
|
||||
rewritePath: string | null,
|
||||
rewritePathType: string | null
|
||||
): { internalMapKey: string; traefikKey: string } {
|
||||
): string {
|
||||
const targetPath = encodePath(path);
|
||||
const pmt = pathMatchType || "";
|
||||
const rp = rewritePath || "";
|
||||
const rpt = rewritePathType || "";
|
||||
|
||||
// Internal map key: uses encodePath (collision-free)
|
||||
const encodedPath = encodePath(path);
|
||||
const internalPathKey = [encodedPath, pmt, rp, rpt]
|
||||
.filter(Boolean)
|
||||
.join("-");
|
||||
const internalMapKey = [resourceId, internalPathKey]
|
||||
.filter(Boolean)
|
||||
.join("-");
|
||||
|
||||
// Traefik-facing key: uses sanitize (backward-compatible)
|
||||
const sanitizedPath = sanitize(path) || "";
|
||||
const traefikPathKey = [sanitizedPath, pmt, rp, rpt]
|
||||
.filter(Boolean)
|
||||
.join("-");
|
||||
const traefikKey = sanitize(
|
||||
[resourceId, traefikPathKey].filter(Boolean).join("-")
|
||||
);
|
||||
|
||||
return { internalMapKey, traefikKey: traefikKey || "" };
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the full Traefik router/service names the way getTraefikConfig does.
|
||||
*/
|
||||
function buildTraefikNames(key: string, resourceName: string) {
|
||||
const name = sanitize(resourceName) || "";
|
||||
return {
|
||||
routerName: `${key}-${name}-router`,
|
||||
serviceName: `${key}-${name}-service`,
|
||||
transportName: `${key}-transport`,
|
||||
headersMiddlewareName: `${key}-headers-middleware`
|
||||
};
|
||||
const pathKey = [targetPath, pmt, rp, rpt].filter(Boolean).join("-");
|
||||
const mapKey = [resourceId, pathKey].filter(Boolean).join("-");
|
||||
return sanitize(mapKey) || "";
|
||||
}
|
||||
|
||||
// ── Tests ────────────────────────────────────────────────────────────
|
||||
|
||||
function runTests() {
|
||||
console.log("Running path encoding & backward compatibility tests...\n");
|
||||
console.log("Running path encoding tests...\n");
|
||||
|
||||
let passed = 0;
|
||||
|
||||
@@ -200,267 +184,22 @@ function runTests() {
|
||||
passed++;
|
||||
}
|
||||
|
||||
// ── Backward compatibility: Traefik names must match old code ─────
|
||||
|
||||
// Test 8: simple resource, no path — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(1, null, null, null, null);
|
||||
const { traefikKey } = newKeyComputation(1, null, null, null, null);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"no-path resource: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — no path resource");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 9: resource with /api prefix — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(1, "/api", "prefix", null, null);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
1,
|
||||
"/api",
|
||||
"prefix",
|
||||
null,
|
||||
null
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"/api prefix: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — /api prefix");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 10: resource with exact path — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(5, "/health", "exact", null, null);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
5,
|
||||
"/health",
|
||||
"exact",
|
||||
null,
|
||||
null
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"/health exact: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — /health exact");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 11: resource with regex path — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(
|
||||
3,
|
||||
"^/api/v[0-9]+",
|
||||
"regex",
|
||||
null,
|
||||
null
|
||||
);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
3,
|
||||
"^/api/v[0-9]+",
|
||||
"regex",
|
||||
null,
|
||||
null
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"regex path: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — regex path");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 12: resource with path rewrite — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(
|
||||
10,
|
||||
"/api",
|
||||
"prefix",
|
||||
"/backend",
|
||||
"prefix"
|
||||
);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
10,
|
||||
"/api",
|
||||
"prefix",
|
||||
"/backend",
|
||||
"prefix"
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"path rewrite: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — path rewrite (prefix→prefix)");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 13: resource with stripPrefix rewrite — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(
|
||||
7,
|
||||
"/app",
|
||||
"prefix",
|
||||
null,
|
||||
"stripPrefix"
|
||||
);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
7,
|
||||
"/app",
|
||||
"prefix",
|
||||
null,
|
||||
"stripPrefix"
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"stripPrefix: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — stripPrefix rewrite");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 14: root path "/" — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(1, "/", "prefix", null, null);
|
||||
const { traefikKey } = newKeyComputation(1, "/", "prefix", null, null);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"root path: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — root path /");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 15: full Traefik router/service names unchanged for existing users
|
||||
{
|
||||
const scenarios = [
|
||||
{
|
||||
rid: 1,
|
||||
name: "my-webapp",
|
||||
path: "/api",
|
||||
pmt: "prefix" as const,
|
||||
rp: null,
|
||||
rpt: null
|
||||
},
|
||||
{
|
||||
rid: 2,
|
||||
name: "backend",
|
||||
path: "/",
|
||||
pmt: "prefix" as const,
|
||||
rp: null,
|
||||
rpt: null
|
||||
},
|
||||
{
|
||||
rid: 3,
|
||||
name: "docs",
|
||||
path: "/docs",
|
||||
pmt: "prefix" as const,
|
||||
rp: "/",
|
||||
rpt: "stripPrefix" as const
|
||||
},
|
||||
{
|
||||
rid: 42,
|
||||
name: "api-service",
|
||||
path: null,
|
||||
pmt: null,
|
||||
rp: null,
|
||||
rpt: null
|
||||
},
|
||||
{
|
||||
rid: 100,
|
||||
name: "grafana",
|
||||
path: "/grafana",
|
||||
pmt: "prefix" as const,
|
||||
rp: null,
|
||||
rpt: null
|
||||
}
|
||||
];
|
||||
for (const s of scenarios) {
|
||||
const oldKey = oldKeyComputation(s.rid, s.path, s.pmt, s.rp, s.rpt);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
s.rid,
|
||||
s.path,
|
||||
s.pmt,
|
||||
s.rp,
|
||||
s.rpt
|
||||
);
|
||||
const oldNames = buildTraefikNames(oldKey, s.name);
|
||||
const newNames = buildTraefikNames(traefikKey, s.name);
|
||||
assertEquals(
|
||||
newNames.routerName,
|
||||
oldNames.routerName,
|
||||
`router name mismatch for resource ${s.rid} ${s.name} path=${s.path}`
|
||||
);
|
||||
assertEquals(
|
||||
newNames.serviceName,
|
||||
oldNames.serviceName,
|
||||
`service name mismatch for resource ${s.rid} ${s.name} path=${s.path}`
|
||||
);
|
||||
assertEquals(
|
||||
newNames.transportName,
|
||||
oldNames.transportName,
|
||||
`transport name mismatch for resource ${s.rid} ${s.name} path=${s.path}`
|
||||
);
|
||||
}
|
||||
console.log(
|
||||
" PASS: backward compat — full router/service/transport names match old code for 5 scenarios"
|
||||
);
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 16: large resourceId — Traefik name unchanged
|
||||
{
|
||||
const oldKey = oldKeyComputation(
|
||||
99999,
|
||||
"/dashboard",
|
||||
"prefix",
|
||||
null,
|
||||
null
|
||||
);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
99999,
|
||||
"/dashboard",
|
||||
"prefix",
|
||||
null,
|
||||
null
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"large resourceId: Traefik key must match old"
|
||||
);
|
||||
console.log(" PASS: backward compat — large resourceId");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// ── Collision fix: the actual bug we're fixing ───────────────────
|
||||
|
||||
// Test 17: /a/b and /a-b now have different internal keys (THE BUG FIX)
|
||||
// Test 8: /a/b and /a-b now have different keys (THE BUG FIX)
|
||||
{
|
||||
const keysAB = newKeyComputation(1, "/a/b", "prefix", null, null);
|
||||
const keysDash = newKeyComputation(1, "/a-b", "prefix", null, null);
|
||||
const keyAB = newKeyComputation(1, "/a/b", "prefix", null, null);
|
||||
const keyDash = newKeyComputation(1, "/a-b", "prefix", null, null);
|
||||
assertEquals(
|
||||
keysAB.internalMapKey !== keysDash.internalMapKey,
|
||||
keyAB !== keyDash,
|
||||
true,
|
||||
"/a/b and /a-b MUST have different internal map keys"
|
||||
);
|
||||
console.log(
|
||||
" PASS: collision fix — /a/b vs /a-b have different internal keys"
|
||||
"/a/b and /a-b MUST have different keys"
|
||||
);
|
||||
console.log(" PASS: collision fix — /a/b vs /a-b have different keys");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 18: demonstrate the old bug — old code maps /a/b and /a-b to same key
|
||||
// Test 9: demonstrate the old bug — old code maps /a/b and /a-b to same key
|
||||
{
|
||||
const oldKeyAB = oldKeyComputation(1, "/a/b", "prefix", null, null);
|
||||
const oldKeyDash = oldKeyComputation(1, "/a-b", "prefix", null, null);
|
||||
@@ -473,7 +212,7 @@ function runTests() {
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 19: /api/v1 and /api-v1 — old code collision, new code fixes it
|
||||
// Test 10: /api/v1 and /api-v1 — old code collision, new code fixes it
|
||||
{
|
||||
const oldKey1 = oldKeyComputation(1, "/api/v1", "prefix", null, null);
|
||||
const oldKey2 = oldKeyComputation(1, "/api-v1", "prefix", null, null);
|
||||
@@ -483,10 +222,10 @@ function runTests() {
|
||||
"old code collision for /api/v1 vs /api-v1"
|
||||
);
|
||||
|
||||
const new1 = newKeyComputation(1, "/api/v1", "prefix", null, null);
|
||||
const new2 = newKeyComputation(1, "/api-v1", "prefix", null, null);
|
||||
const newKey1 = newKeyComputation(1, "/api/v1", "prefix", null, null);
|
||||
const newKey2 = newKeyComputation(1, "/api-v1", "prefix", null, null);
|
||||
assertEquals(
|
||||
new1.internalMapKey !== new2.internalMapKey,
|
||||
newKey1 !== newKey2,
|
||||
true,
|
||||
"new code must separate /api/v1 and /api-v1"
|
||||
);
|
||||
@@ -494,20 +233,16 @@ function runTests() {
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 20: /app.v2 and /app/v2 and /app-v2 — three-way collision fixed
|
||||
// Test 11: /app.v2 and /app/v2 and /app-v2 — three-way collision fixed
|
||||
{
|
||||
const a = newKeyComputation(1, "/app.v2", "prefix", null, null);
|
||||
const b = newKeyComputation(1, "/app/v2", "prefix", null, null);
|
||||
const c = newKeyComputation(1, "/app-v2", "prefix", null, null);
|
||||
const keys = new Set([
|
||||
a.internalMapKey,
|
||||
b.internalMapKey,
|
||||
c.internalMapKey
|
||||
]);
|
||||
const keys = new Set([a, b, c]);
|
||||
assertEquals(
|
||||
keys.size,
|
||||
3,
|
||||
"three paths must produce three unique internal keys"
|
||||
"three paths must produce three unique keys"
|
||||
);
|
||||
console.log(
|
||||
" PASS: collision fix — three-way /app.v2, /app/v2, /app-v2"
|
||||
@@ -517,38 +252,33 @@ function runTests() {
|
||||
|
||||
// ── Edge cases ───────────────────────────────────────────────────
|
||||
|
||||
// Test 21: same path in different resources — always separate
|
||||
// Test 12: same path in different resources — always separate
|
||||
{
|
||||
const res1 = newKeyComputation(1, "/api", "prefix", null, null);
|
||||
const res2 = newKeyComputation(2, "/api", "prefix", null, null);
|
||||
const key1 = newKeyComputation(1, "/api", "prefix", null, null);
|
||||
const key2 = newKeyComputation(2, "/api", "prefix", null, null);
|
||||
assertEquals(
|
||||
res1.internalMapKey !== res2.internalMapKey,
|
||||
key1 !== key2,
|
||||
true,
|
||||
"different resources with same path must have different keys"
|
||||
);
|
||||
assertEquals(
|
||||
res1.traefikKey !== res2.traefikKey,
|
||||
true,
|
||||
"different resources with same path must have different Traefik keys"
|
||||
);
|
||||
console.log(" PASS: edge case — same path, different resources");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 22: same resource, different pathMatchType — separate keys
|
||||
// Test 13: same resource, different pathMatchType — separate keys
|
||||
{
|
||||
const exact = newKeyComputation(1, "/api", "exact", null, null);
|
||||
const prefix = newKeyComputation(1, "/api", "prefix", null, null);
|
||||
assertEquals(
|
||||
exact.internalMapKey !== prefix.internalMapKey,
|
||||
exact !== prefix,
|
||||
true,
|
||||
"exact vs prefix must have different internal keys"
|
||||
"exact vs prefix must have different keys"
|
||||
);
|
||||
console.log(" PASS: edge case — same path, different match types");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 23: same resource and path, different rewrite config — separate keys
|
||||
// Test 14: same resource and path, different rewrite config — separate keys
|
||||
{
|
||||
const noRewrite = newKeyComputation(1, "/api", "prefix", null, null);
|
||||
const withRewrite = newKeyComputation(
|
||||
@@ -559,25 +289,22 @@ function runTests() {
|
||||
"prefix"
|
||||
);
|
||||
assertEquals(
|
||||
noRewrite.internalMapKey !== withRewrite.internalMapKey,
|
||||
noRewrite !== withRewrite,
|
||||
true,
|
||||
"with vs without rewrite must have different internal keys"
|
||||
"with vs without rewrite must have different keys"
|
||||
);
|
||||
console.log(" PASS: edge case — same path, different rewrite config");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 24: paths with special URL characters
|
||||
// Test 15: paths with special URL characters
|
||||
{
|
||||
const paths = ["/api?foo", "/api#bar", "/api%20baz", "/api+qux"];
|
||||
const internal = new Set(
|
||||
paths.map(
|
||||
(p) =>
|
||||
newKeyComputation(1, p, "prefix", null, null).internalMapKey
|
||||
)
|
||||
const keys = new Set(
|
||||
paths.map((p) => newKeyComputation(1, p, "prefix", null, null))
|
||||
);
|
||||
assertEquals(
|
||||
internal.size,
|
||||
keys.size,
|
||||
paths.length,
|
||||
"special URL chars must produce unique keys"
|
||||
);
|
||||
@@ -585,49 +312,6 @@ function runTests() {
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 25: very long path (sanitize truncates at 50 chars — verify consistency)
|
||||
{
|
||||
const longPath = "/" + "a".repeat(100);
|
||||
const oldKey = oldKeyComputation(1, longPath, "prefix", null, null);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
1,
|
||||
longPath,
|
||||
"prefix",
|
||||
null,
|
||||
null
|
||||
);
|
||||
assertEquals(
|
||||
traefikKey,
|
||||
oldKey,
|
||||
"long path: Traefik key must match old (both truncate)"
|
||||
);
|
||||
console.log(" PASS: edge case — very long path (50-char truncation)");
|
||||
passed++;
|
||||
}
|
||||
|
||||
// Test 26: sticky session cookie safety — service name doesn't change
|
||||
{
|
||||
// Sticky sessions use cookie name "p_sticky" tied to the service name.
|
||||
// If service name changes, existing cookies become invalid.
|
||||
const oldKey = oldKeyComputation(1, "/api", "prefix", null, null);
|
||||
const { traefikKey } = newKeyComputation(
|
||||
1,
|
||||
"/api",
|
||||
"prefix",
|
||||
null,
|
||||
null
|
||||
);
|
||||
const oldServiceName = `${oldKey}-my-app-service`;
|
||||
const newServiceName = `${traefikKey}-my-app-service`;
|
||||
assertEquals(
|
||||
newServiceName,
|
||||
oldServiceName,
|
||||
"service name must not change (would break sticky session cookies)"
|
||||
);
|
||||
console.log(" PASS: sticky session safety — service name preserved");
|
||||
passed++;
|
||||
}
|
||||
|
||||
console.log(`\nAll ${passed} tests passed!`);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user