Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 21 additions & 8 deletions apps/cli/src/command/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export const toKVConfiguration = (
'plugin_metadata',
'stream_routes',
'upstreams',
'custom_plugins',
].includes(resourceType) // ensure that you don't convert unexpected keys
) {
return [
Expand Down Expand Up @@ -131,7 +132,13 @@ export const filterConfiguration = (
): [ADCSDK.Configuration, ADCSDK.Configuration] => {
const removed: ADCSDK.Configuration = {};
Object.keys(configuration).forEach((resourceType) => {
if (resourceType === 'plugin_metadata' || resourceType === 'global_rules')
// Keyed-object and label-less resources are not subject to label filtering;
// custom plugins are global and must not be pruned by a label selector.
if (
resourceType === 'plugin_metadata' ||
resourceType === 'global_rules' ||
resourceType === 'custom_plugins'
)
return;
const result = labelFilter(configuration[resourceType], rules);
configuration[resourceType] = result.filtered;
Expand All @@ -146,11 +153,10 @@ const labelFilter = <T extends ADCSDK.Event['newValue']>(
rules: Record<string, string> = {},
) => {
const filtered = resources.filter((resource) => {
// Some resources (e.g. custom plugins) carry no labels.
const labels = (resource as { labels?: ADCSDK.Labels })?.labels;
return Object.entries(rules).every(
([key, value]) =>
resource?.labels &&
resource?.labels[key] &&
resource?.labels[key] === value,
([key, value]) => labels && labels[key] && labels[key] === value,
);
Comment on lines +156 to 160

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle array-valued labels in selector matching.

ADCSDK.Labels allows string | string[], but this predicate only matches scalar strings. A selector like { team: "blue" } will incorrectly drop resources whose label is team: ["blue", "prod"], which can cascade into unintended pruning during sync.

Proposed fix
   const filtered = resources.filter((resource) => {
     // Some resources (e.g. custom plugins) carry no labels.
     const labels = (resource as { labels?: ADCSDK.Labels })?.labels;
     return Object.entries(rules).every(
-      ([key, value]) => labels && labels[key] && labels[key] === value,
+      ([key, value]) => {
+        const actual = labels?.[key];
+        return Array.isArray(actual)
+          ? actual.includes(value)
+          : actual === value;
+      },
     );
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Some resources (e.g. custom plugins) carry no labels.
const labels = (resource as { labels?: ADCSDK.Labels })?.labels;
return Object.entries(rules).every(
([key, value]) =>
resource?.labels &&
resource?.labels[key] &&
resource?.labels[key] === value,
([key, value]) => labels && labels[key] && labels[key] === value,
);
// Some resources (e.g. custom plugins) carry no labels.
const labels = (resource as { labels?: ADCSDK.Labels })?.labels;
return Object.entries(rules).every(
([key, value]) => {
const actual = labels?.[key];
return Array.isArray(actual)
? actual.includes(value)
: actual === value;
},
);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/cli/src/command/utils.ts` around lines 156 - 160, The selector predicate
currently assumes labels are scalar strings; update the check inside the
Object.entries(rules).every(...) arrow function to handle ADCSDK.Labels values
that may be string or string[] by normalizing labels[key] to an array (if it's a
string make it [value]) and then checking that the selector value equals any
element (use includes) — preserve the existing behavior for missing labels (fail
the match) and for exact string matches; refer to the labels variable and the
rules matching arrow function to locate where to change.

});

Expand All @@ -170,10 +176,17 @@ export const fillLabels = (
: labels;

for (const resourceType in configuration) {
if (['global_rules', 'plugin_metadata'].includes(resourceType)) continue;
// global_rules/plugin_metadata are keyed objects; custom_plugins are
// label-less, so none of them participate in label selectors.
if (
['global_rules', 'plugin_metadata', 'custom_plugins'].includes(
resourceType,
)
)
continue;

(configuration[resourceType] as Array<ADCSDK.ResourceFor<any>>).forEach(
(resource) => {
(configuration[resourceType] as Array<{ labels?: ADCSDK.Labels }>).forEach(
(resource: any) => {
resource.labels = assignSelector(resource.labels as ADCSDK.Labels);

// Process the nested resources
Expand Down
56 changes: 50 additions & 6 deletions apps/cli/src/tasks/load_local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,16 @@ export const LoadLocalConfigurationTask = (
(await readFile(filePath, { encoding: 'utf-8' })) ?? '';

const ext = path.extname(filePath).toLowerCase();
if (ext === '.json') {
subCtx.configurations[filePath] =
JSON.parse(fileContent) ?? {};
return;
}
subCtx.configurations[filePath] = YAML.load(fileContent) ?? {};
const config: ADCSDK.Configuration =
ext === '.json'
? (JSON.parse(fileContent) ?? {})
: (YAML.load(fileContent) ?? {});

// Inline external Lua sources referenced by custom plugins and
// validate the declared name against the source.
await resolveCustomPluginSources(config, filePath);

subCtx.configurations[filePath] = config;
Comment on lines +55 to +64

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Expand env vars in custom_plugins[].path before resolving the file.

resolveCustomPluginSources() runs before the later "Resolve value variables" step, so a config like path: ${PLUGIN_DIR}/my.lua is treated literally and fails the Line 137 existence check. That makes custom plugin paths the only config strings that do not honor env substitution.

Proposed fix
 const resolveCustomPluginSources = async (
   config: ADCSDK.Configuration,
   configFilePath: string,
 ) => {
+  const expandEnv = (value: string) =>
+    value.replace(/\$\{(\w+)\}/g, (_, key) => process.env[key] ?? '');
+
   const customPlugins = config.custom_plugins;
   if (!Array.isArray(customPlugins) || customPlugins.length === 0) return;
 
   const baseDir = path.dirname(configFilePath);
@@
   for (const plugin of customPlugins) {
     if (plugin.path) {
-      const sourcePath = path.resolve(baseDir, plugin.path);
+      const sourcePath = path.resolve(baseDir, expandEnv(plugin.path));
       if (!existsSync(sourcePath))
         throw new Error(
           `Custom plugin "${plugin.name}" references a source file that does not exist: ${sourcePath}`,
         );

Also applies to: 135-143

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/cli/src/tasks/load_local.ts` around lines 55 - 64, The custom plugin
paths aren't getting environment variable substitution before existence checks;
run the same env-variable expansion used in the "Resolve value variables" step
on config.custom_plugins[].path (or on the whole config object) immediately
after parsing (before calling resolveCustomPluginSources) so entries like path:
${PLUGIN_DIR}/my.lua are expanded; modify the code around
resolveCustomPluginSources to perform expansion (reusing the project's existing
expand/resolve function) and then call resolveCustomPluginSources(config,
filePath), finally assign subCtx.configurations[filePath] = config.

},
};
}),
Expand Down Expand Up @@ -111,3 +115,43 @@ export const LoadLocalConfigurationTask = (
);
},
});

// Resolve each custom plugin's `path` reference into an inline `content`
// (read relative to the config file), and verify that the declared `name`
// actually appears in the (plaintext) Lua source so a typo is caught locally
// rather than rejected later by the control plane.
const resolveCustomPluginSources = async (
config: ADCSDK.Configuration,
configFilePath: string,
) => {
const customPlugins = config.custom_plugins;
if (!Array.isArray(customPlugins) || customPlugins.length === 0) return;

const baseDir = path.dirname(configFilePath);
const looksLikeLua = (source: string) =>
/\b(function|return|local)\b/.test(source);

for (const plugin of customPlugins) {
if (plugin.path) {
const sourcePath = path.resolve(baseDir, plugin.path);
if (!existsSync(sourcePath))
throw new Error(
`Custom plugin "${plugin.name}" references a source file that does not exist: ${sourcePath}`,
);
plugin.content =
(await readFile(sourcePath, { encoding: 'utf-8' })) ?? '';
delete plugin.path;
}

// Only validate when the source is plaintext Lua; obfuscated/bytecode
// uploads will not contain the name literally.
if (
plugin.content &&
looksLikeLua(plugin.content) &&
!plugin.content.includes(plugin.name)
)
throw new Error(
`Custom plugin name "${plugin.name}" was not found in its Lua source; the declared name must match the plugin's name in the source.`,
);
}
};
84 changes: 84 additions & 0 deletions libs/backend-api7/e2e/resources/custom-plugin.e2e-spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import * as ADCSDK from '@api7/adc-sdk';
import { globalAgent as httpAgent } from 'node:http';

import { BackendAPI7 } from '../../src';
import {
createEvent,
deleteEvent,
dumpConfiguration,
generateHTTPSAgent,
syncEvents,
updateEvent,
} from '../support/utils';

describe('Custom Plugin E2E', () => {
let backend: BackendAPI7;

beforeAll(() => {
backend = new BackendAPI7({
server: process.env.SERVER!,
token: process.env.TOKEN!,
tlsSkipVerify: true,
gatewayGroup: process.env.GATEWAY_GROUP,
cacheKey: 'default',
httpAgent,
httpsAgent: generateHTTPSAgent(),
});
Comment on lines +17 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast on missing E2E env vars (especially GATEWAY_GROUP).

Line 17–26 should validate required env vars before constructing BackendAPI7; otherwise failures are deferred and harder to diagnose, particularly since custom-plugin management requires a resolved gateway group.

Suggested guard
   beforeAll(() => {
+    const { SERVER, TOKEN, GATEWAY_GROUP } = process.env;
+    if (!SERVER || !TOKEN || !GATEWAY_GROUP) {
+      throw new Error(
+        'Missing required env vars: SERVER, TOKEN, and GATEWAY_GROUP must be set for Custom Plugin E2E',
+      );
+    }
+
     backend = new BackendAPI7({
-      server: process.env.SERVER!,
-      token: process.env.TOKEN!,
+      server: SERVER,
+      token: TOKEN,
       tlsSkipVerify: true,
-      gatewayGroup: process.env.GATEWAY_GROUP,
+      gatewayGroup: GATEWAY_GROUP,
       cacheKey: 'default',
       httpAgent,
       httpsAgent: generateHTTPSAgent(),
     });
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
beforeAll(() => {
backend = new BackendAPI7({
server: process.env.SERVER!,
token: process.env.TOKEN!,
tlsSkipVerify: true,
gatewayGroup: process.env.GATEWAY_GROUP,
cacheKey: 'default',
httpAgent,
httpsAgent: generateHTTPSAgent(),
});
beforeAll(() => {
const { SERVER, TOKEN, GATEWAY_GROUP } = process.env;
if (!SERVER || !TOKEN || !GATEWAY_GROUP) {
throw new Error(
'Missing required env vars: SERVER, TOKEN, and GATEWAY_GROUP must be set for Custom Plugin E2E',
);
}
backend = new BackendAPI7({
server: SERVER,
token: TOKEN,
tlsSkipVerify: true,
gatewayGroup: GATEWAY_GROUP,
cacheKey: 'default',
httpAgent,
httpsAgent: generateHTTPSAgent(),
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@libs/backend-api7/e2e/resources/custom-plugin.e2e-spec.ts` around lines 17 -
26, Before instantiating BackendAPI7, validate required E2E env vars (at least
process.env.SERVER, process.env.TOKEN and process.env.GATEWAY_GROUP) and fail
fast with a clear error if any are missing; update the beforeAll block that
currently calls new BackendAPI7(...) to first check those env vars and throw or
call a test fail with a descriptive message referencing GATEWAY_GROUP so
construction of BackendAPI7 (the BackendAPI7 constructor call and variables
server/token/gatewayGroup) never proceeds when the required env is absent.

});

describe('Sync and dump custom plugins', () => {
const pluginName = 'e2e-custom-plugin';
const pluginContent = [
'local core = require("apisix.core")',
'local schema = { type = "object", properties = {} }',
`local _M = { version = 0.1, priority = 0, name = "${pluginName}", schema = schema }`,
'function _M.check_schema(conf) return core.schema.check(schema, conf) end',
'function _M.access(conf, ctx) end',
'return _M',
].join('\n');
const plugin = {
name: pluginName,
content: pluginContent,
description: 'created by e2e',
} as ADCSDK.CustomPlugin;

it('Create custom plugin', async () =>
syncEvents(backend, [
createEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName, plugin),
]));

it('Dump', async () => {
const result = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
expect(result.custom_plugins).toHaveLength(1);
expect(result.custom_plugins?.[0]).toMatchObject({
name: pluginName,
content: pluginContent,
});
});

it('Update custom plugin (description changed)', async () => {
plugin.description = 'updated by e2e';
await syncEvents(backend, [
updateEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName, plugin),
]);
});

it('Dump again (description updated)', async () => {
const result = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
expect(result.custom_plugins?.[0]).toMatchObject({
name: pluginName,
description: 'updated by e2e',
});
});

it('Delete custom plugin', async () =>
syncEvents(backend, [
deleteEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName),
]));

it('Dump again (custom plugin should not exist)', async () => {
const result = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
expect(result.custom_plugins ?? []).toHaveLength(0);
});
});
Comment on lines +29 to +83

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify explicit inter-test dependency and shared mutable state in this spec file.
rg -n --type=ts -C2 'const plugin =|plugin\.description\s*=|it\(' libs/backend-api7/e2e/resources/custom-plugin.e2e-spec.ts

Repository: api7/adc

Length of output: 1440


Avoid order-coupled, stateful E2E tests in custom-plugin.e2e-spec.ts

  • The suite mutates a shared plugin object (plugin.description = ...) and relies on prior it(...) blocks (create → dump → update → dump → delete → dump). Reordering or parallel execution would break isolation and can cause flakiness.
Suggested refactor
-  describe('Sync and dump custom plugins', () => {
+  describe('Sync and dump custom plugins', () => {
@@
-    it('Create custom plugin', async () =>
-      syncEvents(backend, [
-        createEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName, plugin),
-      ]));
-
-    it('Dump', async () => {
+    it('creates, dumps, updates, and deletes a custom plugin', async () => {
+      await syncEvents(backend, [
+        createEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName, plugin),
+      ]);
+
       const result = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
       expect(result.custom_plugins).toHaveLength(1);
       expect(result.custom_plugins?.[0]).toMatchObject({
         name: pluginName,
         content: pluginContent,
       });
-    });
-
-    it('Update custom plugin (description changed)', async () => {
+
       plugin.description = 'updated by e2e';
       await syncEvents(backend, [
         updateEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName, plugin),
       ]);
-    });
-
-    it('Dump again (description updated)', async () => {
-      const result = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
-      expect(result.custom_plugins?.[0]).toMatchObject({
+
+      const updated = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
+      expect(updated.custom_plugins?.[0]).toMatchObject({
         name: pluginName,
         description: 'updated by e2e',
       });
-    });
-
-    it('Delete custom plugin', async () =>
-      syncEvents(backend, [
+
+      await syncEvents(backend, [
         deleteEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName),
-      ]));
-
-    it('Dump again (custom plugin should not exist)', async () => {
-      const result = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
-      expect(result.custom_plugins ?? []).toHaveLength(0);
+      ]);
+
+      const afterDelete = (await dumpConfiguration(backend)) as ADCSDK.Configuration;
+      expect(afterDelete.custom_plugins ?? []).toHaveLength(0);
     });
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@libs/backend-api7/e2e/resources/custom-plugin.e2e-spec.ts` around lines 29 -
83, Tests are order-coupled and mutate the shared plugin object (plugin,
pluginContent) causing stateful, flaky E2E behavior; fix by making each spec
independent: avoid mutating the shared plugin (remove plugin.description = ...),
create fresh plugin instances per test (or deep-clone plugin before modifying)
and ensure create/update/delete operations use isolated names or run
setup/teardown in beforeEach/afterEach (refer to the plugin variable,
pluginContent, and the "Create custom plugin"/"Update custom plugin (description
changed)"/"Delete custom plugin" test cases) so each it(...) is self-contained
and does not rely on prior tests.

Source: Coding guidelines

});
43 changes: 42 additions & 1 deletion libs/backend-api7/src/fetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { isEmpty } from 'lodash-es';
import {
Subject,
combineLatest,
filter,
from,
map,
mergeMap,
Expand Down Expand Up @@ -257,23 +258,63 @@ export class Fetcher extends ADCSDK.backend.BackendEventSource {
);
}

public listCustomPlugins() {
if (this.isSkip(ADCSDK.ResourceType.CUSTOM_PLUGIN))
return of<Array<ADCSDK.CustomPlugin>>([]);

const taskName = 'Fetch custom plugins';
const logger = this.getLogger(taskName);
const taskStateEvent = this.taskStateEvent(taskName);
logger(taskStateEvent('TASK_START'));
return from(
this.client.get<typing.ListResponse<typing.CustomPlugin>>(
'/api/custom_plugins',
),
).pipe(
tap((resp) => logger(this.debugLogEvent(resp))),
mergeMap((resp) =>
from(resp.data.list ?? []).pipe(
// Custom plugins are control-plane-global; only manage those deployed
// to the gateway group that this backend targets.
filter(
(plugin) =>
!this.opts.gatewayGroupId ||
(plugin.gateway_groups ?? []).includes(this.opts.gatewayGroupId),
),
Comment on lines +279 to +283

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't widen dump() to every custom plugin when the gateway group is unresolved.

!this.opts.gatewayGroupId currently lets every /api/custom_plugins item through. That breaks the per-gateway-group contract from this PR: dump() can emit plugins from unrelated groups, while Operator.operateCustomPlugin() explicitly refuses to manage them without a resolved group. Return [] or fail early here instead of broadening scope.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@libs/backend-api7/src/fetcher.ts` around lines 279 - 283, The current filter
in dump() uses !this.opts.gatewayGroupId which lets all custom plugins through
when the gateway group is unresolved; instead, in dump() check
this.opts.gatewayGroupId up front and, if it's falsy, return an empty array (or
throw early) so you don't broaden scope. Locate the dump() implementation in
fetcher.ts and replace the permissive filter logic that references
this.opts.gatewayGroupId with an early guard (use this.opts.gatewayGroupId to
gate execution) so dump() only emits plugins for the resolved gateway group and
stays consistent with Operator.operateCustomPlugin().

map((plugin) => this.toADC.transformCustomPlugin(plugin)),
),
),
toArray(),
tap(() => logger(taskStateEvent('TASK_DONE'))),
);
}

public dump() {
return combineLatest([
this.listServices(),
this.listConsumers(),
this.listSSLs(),
this.listGlobalRules(),
this.listMetadatas(),
this.listCustomPlugins(),
]).pipe(
takeLast(1),
map(
([services, consumers, ssls, global_rules, plugin_metadata]) =>
([
services,
consumers,
ssls,
global_rules,
plugin_metadata,
custom_plugins,
]) =>
({
services,
consumers,
ssls,
global_rules,
plugin_metadata,
custom_plugins,
}) as ADCSDK.Configuration,
),
);
Expand Down
Loading
Loading