feat: support custom plugins as a declarative resource#455
Open
shreemaan-abhishek wants to merge 1 commit into
Open
feat: support custom plugins as a declarative resource#455shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
Adds custom_plugins as a first-class resource managed by ADC on the API7 EE (api7ee) backend. - sdk: new ResourceType.CUSTOM_PLUGIN and customPluginSchema (name plus either inline "content" or an external "path"), wired into the configuration schemas and ResourceFor. - backend-api7: fetch/sync against /api/custom_plugins. Custom plugins are control-plane-global objects carrying a gateway_groups membership, so: dump only surfaces plugins deployed to the targeted gateway group; create/update ensures the group is in the membership; prune removes only the targeted group and deletes the plugin entirely solely when it was the last member. - differ: order custom plugin create/update before referencing resources (routes, etc.) and prune after all references are removed. - cli: resolve a custom plugin's "path" into inline "content" at load time and validate that the declared name appears in the plaintext Lua source. Regenerates schema.json and adds differ, fetcher, and e2e coverage.
|
|
📝 WalkthroughWalkthroughThis PR adds custom plugins as a first-class resource type to the API7 Declarative Configuration system. It introduces plugin schema validation, backend reconciliation with gateway group membership management, configuration diffing, and CLI file resolution with Lua source validation. ChangesCustom Plugin Resource Support
🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
libs/backend-api7/test/fetcher.spec.ts (1)
80-95: ⚡ Quick winAdd a behavioral test for
listCustomPlugins()scoping.These cases only verify
isSkip(). They do not exercise the new/api/custom_pluginsmembership filter, so a gateway-group scoping regression indump()would still pass this file. A focused test aroundlistCustomPlugins()would pin the new behavior down.🤖 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/test/fetcher.spec.ts` around lines 80 - 95, The tests only assert isSkip() behavior and miss verifying that listCustomPlugins() uses the gateway-group membership filter, so add a focused test that constructs a fetcher via newFetcher(...) with includeResourceType/excludeResourceType set to CUSTOM_PLUGIN and then calls fetcher.listCustomPlugins() (or triggers fetcher.dump() if listCustomPlugins is internal) to assert the returned plugin set is correctly scoped by membership; mock or stub the underlying API response for /api/custom_plugins to contain plugins from multiple gateway-groups and verify only the expected group(s) are returned when scoping is applied, referencing the existing newFetcher, listCustomPlugins (or dump), and isSkip symbols to locate the code to test.libs/differ/src/test/custom-plugin.spec.ts (1)
58-69: ⚡ Quick winAdd the symmetric delete-order assertion.
This only pins
CUSTOM_PLUGIN.CREATEbefore dependent resources. The backend executor explicitly depends on event order, so a regression that emitsCUSTOM_PLUGIN.DELETEbefore route/global-rule deletes would still pass this suite and break prune behavior at runtime. Add a case that removes a referencing resource and the custom plugin together, then assert the custom plugin delete is ordered last.🤖 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/differ/src/test/custom-plugin.spec.ts` around lines 58 - 69, Add a symmetric delete-order assertion: create a test that starts with a state containing the custom plugin and a referencing resource (use DifferV3.diff with { custom_plugins: [{ name, content }], routes: [{ name: 'r1', uris: ['/'], plugins: { [name]: {} } }] } as the "from" state and {} as the "to" state), collect the diff events, filter for delete actions, map resourceType, and assert that ADCSDK.ResourceType.CUSTOM_PLUGIN delete appears after the dependent resource delete (e.g., expect(indexOf(CUSTOM_PLUGIN) toBeGreaterThan indexOf(ROUTE))); reference DifferV3.diff, ADCSDK.ResourceType.CUSTOM_PLUGIN and ADCSDK.ResourceType.ROUTE to locate where to add this new test.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/cli/src/command/utils.ts`:
- Around line 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.
In `@apps/cli/src/tasks/load_local.ts`:
- Around line 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.
In `@libs/backend-api7/e2e/resources/custom-plugin.e2e-spec.ts`:
- Around line 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.
- Around line 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.
In `@libs/backend-api7/src/fetcher.ts`:
- Around line 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().
In `@libs/backend-api7/src/operator.ts`:
- Around line 321-343: The delete-path currently uses
fromADC.transformCustomPlugin(event.oldValue as ADCSDK.CustomPlugin) which can
be undefined and may overwrite newer data; instead, build the PUT request body
from the existing object: use the existing custom plugin record (existing) as
the base, copy/transform whatever server-side representation is required (or
call fromADC.transformCustomPlugin on existing if appropriate), then set
gateway_groups to the filtered remaining array and send that in
this.client.request for the PUT; update the branch handling
ADCSDK.EventType.DELETE (the block that computes remaining and issues the PUT)
to derive the prune-body from existing rather than event.oldValue, keeping the
same URL/existing.id and only replacing gateway_groups.
- Around line 290-365: The current operateCustomPlugin implementation (using
getCustomPluginList and client.request PUT/DELETE) can lose concurrent updates
to shared gateway_groups; change it to perform an optimistic-concurrency retry:
before any modifying PUT/DELETE, fetch the fresh plugin state for existing.id
(call the API to GET /api/custom_plugins/{id} instead of relying on the cached
list), compute the new gateway_groups from that fresh state, attempt the
PUT/DELETE and if the server returns a conflict (e.g., 409) or the returned
state differs, retry the read-modify-write cycle a bounded number of times
(e.g., 3) and then fail; update the code paths in operateCustomPlugin that build
the PUT/DELETE payloads to use the freshly fetched plugin, and prefer using
If-Match/ETag headers if the API supports them when issuing client.request to
make this compare-and-set robust.
In `@libs/sdk/src/core/schema.ts`:
- Around line 359-387: Replace the runtime .refine checks with a structural
union so the generated JSON Schema preserves the exclusivity: remove content and
path from the base strict object (the existing customPluginSchema base that
references idSchema and descriptionSchema), then create two variants and union
them: one variant extends the base with content: z.string().min(1) and path:
z.undefined(), the other extends the base with path: z.string().min(1) and
content: z.undefined(), and finally set customPluginSchema =
z.union([contentVariant, pathVariant]); keep the same name/id/metadata fields
(idSchema, descriptionSchema, catalog, documentation_link, author, logo)
unchanged.
---
Nitpick comments:
In `@libs/backend-api7/test/fetcher.spec.ts`:
- Around line 80-95: The tests only assert isSkip() behavior and miss verifying
that listCustomPlugins() uses the gateway-group membership filter, so add a
focused test that constructs a fetcher via newFetcher(...) with
includeResourceType/excludeResourceType set to CUSTOM_PLUGIN and then calls
fetcher.listCustomPlugins() (or triggers fetcher.dump() if listCustomPlugins is
internal) to assert the returned plugin set is correctly scoped by membership;
mock or stub the underlying API response for /api/custom_plugins to contain
plugins from multiple gateway-groups and verify only the expected group(s) are
returned when scoping is applied, referencing the existing newFetcher,
listCustomPlugins (or dump), and isSkip symbols to locate the code to test.
In `@libs/differ/src/test/custom-plugin.spec.ts`:
- Around line 58-69: Add a symmetric delete-order assertion: create a test that
starts with a state containing the custom plugin and a referencing resource (use
DifferV3.diff with { custom_plugins: [{ name, content }], routes: [{ name: 'r1',
uris: ['/'], plugins: { [name]: {} } }] } as the "from" state and {} as the "to"
state), collect the diff events, filter for delete actions, map resourceType,
and assert that ADCSDK.ResourceType.CUSTOM_PLUGIN delete appears after the
dependent resource delete (e.g., expect(indexOf(CUSTOM_PLUGIN) toBeGreaterThan
indexOf(ROUTE))); reference DifferV3.diff, ADCSDK.ResourceType.CUSTOM_PLUGIN and
ADCSDK.ResourceType.ROUTE to locate where to add this new test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45e06528-f5e9-4413-a536-a6ba8e460d78
📒 Files selected for processing (14)
apps/cli/src/command/utils.tsapps/cli/src/tasks/load_local.tslibs/backend-api7/e2e/resources/custom-plugin.e2e-spec.tslibs/backend-api7/src/fetcher.tslibs/backend-api7/src/operator.tslibs/backend-api7/src/transformer.tslibs/backend-api7/src/typing.tslibs/backend-api7/test/fetcher.spec.tslibs/differ/src/differv3.tslibs/differ/src/test/custom-plugin.spec.tslibs/sdk/src/core/index.tslibs/sdk/src/core/resource.tslibs/sdk/src/core/schema.tsschema.json
Comment on lines
+156
to
160
| // 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, | ||
| ); |
There was a problem hiding this comment.
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.
Comment on lines
+55
to
+64
| 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; |
There was a problem hiding this comment.
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.
Comment on lines
+17
to
+26
| beforeAll(() => { | ||
| backend = new BackendAPI7({ | ||
| server: process.env.SERVER!, | ||
| token: process.env.TOKEN!, | ||
| tlsSkipVerify: true, | ||
| gatewayGroup: process.env.GATEWAY_GROUP, | ||
| cacheKey: 'default', | ||
| httpAgent, | ||
| httpsAgent: generateHTTPSAgent(), | ||
| }); |
There was a problem hiding this comment.
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.
Comment on lines
+29
to
+83
| 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); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 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.tsRepository: api7/adc
Length of output: 1440
Avoid order-coupled, stateful E2E tests in custom-plugin.e2e-spec.ts
- The suite mutates a shared
pluginobject (plugin.description = ...) and relies on priorit(...)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
Comment on lines
+279
to
+283
| filter( | ||
| (plugin) => | ||
| !this.opts.gatewayGroupId || | ||
| (plugin.gateway_groups ?? []).includes(this.opts.gatewayGroupId), | ||
| ), |
There was a problem hiding this comment.
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().
Comment on lines
+290
to
+365
| private getCustomPluginList(): Promise<Array<typing.CustomPlugin>> { | ||
| if (!this.customPluginListCache) | ||
| this.customPluginListCache = this.client | ||
| .get<typing.ListResponse<typing.CustomPlugin>>('/api/custom_plugins') | ||
| .then((resp) => resp.data?.list ?? []); | ||
| return this.customPluginListCache; | ||
| } | ||
|
|
||
| // Reconciles a custom plugin against the control plane while only touching | ||
| // the gateway group this backend targets: | ||
| // - create/update: ensure this group is in the plugin's membership (POST when | ||
| // the plugin does not exist yet, otherwise PUT and append the group). | ||
| // - delete (prune): drop this group from the membership; the plugin is only | ||
| // removed entirely when no other group still references it. | ||
| // Because a custom plugin is shared across its member groups, the uploaded | ||
| // source/metadata in the local config is authoritative for every group. | ||
| private async operateCustomPlugin( | ||
| event: ADCSDK.Event, | ||
| ): Promise<AxiosResponse> { | ||
| const groupId = this.opts.gatewayGroupId; | ||
| if (!groupId) | ||
| throw new Error( | ||
| 'Managing custom plugins requires a resolved gateway group, but none is available for the current backend.', | ||
| ); | ||
|
|
||
| const name = event.resourceName; | ||
| const existing = (await this.getCustomPluginList()).find( | ||
| (plugin) => plugin.name === name, | ||
| ); | ||
| const fromADC = new FromADC(); | ||
|
|
||
| if (event.type === ADCSDK.EventType.DELETE) { | ||
| if (!existing) | ||
| return this.syntheticResponse('delete', 'custom plugin already absent'); | ||
|
|
||
| const remaining = (existing.gateway_groups ?? []).filter( | ||
| (id) => id !== groupId, | ||
| ); | ||
| if (remaining.length === 0) | ||
| return this.client.request({ | ||
| method: 'DELETE', | ||
| url: `/api/custom_plugins/${existing.id}`, | ||
| }); | ||
|
|
||
| return this.client.request({ | ||
| method: 'PUT', | ||
| url: `/api/custom_plugins/${existing.id}`, | ||
| data: { | ||
| ...fromADC.transformCustomPlugin( | ||
| event.oldValue as ADCSDK.CustomPlugin, | ||
| ), | ||
| gateway_groups: remaining, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const body = fromADC.transformCustomPlugin( | ||
| event.newValue as ADCSDK.CustomPlugin, | ||
| ); | ||
| if (existing) | ||
| return this.client.request({ | ||
| method: 'PUT', | ||
| url: `/api/custom_plugins/${existing.id}`, | ||
| data: { | ||
| ...body, | ||
| gateway_groups: Array.from( | ||
| new Set([...(existing.gateway_groups ?? []), groupId]), | ||
| ), | ||
| }, | ||
| }); | ||
|
|
||
| return this.client.request({ | ||
| method: 'POST', | ||
| url: '/api/custom_plugins', | ||
| data: { ...body, gateway_groups: [groupId] }, | ||
| }); |
There was a problem hiding this comment.
This reconciliation has a lost-update window on shared gateway_groups.
getCustomPluginList() reads the global plugin, then create/update/delete locally merges or filters gateway_groups, then PUTs the full array back. Two syncs touching the same custom plugin from different gateway groups can race and drop each other's membership changes on the last write. Please add optimistic concurrency/retry against fresh server state, or another compare-and-set mechanism if the API supports it. As per coding guidelines, **/*.{go,java,ts,js} changes should be checked for race conditions and shared resource access.
🤖 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/operator.ts` around lines 290 - 365, The current
operateCustomPlugin implementation (using getCustomPluginList and client.request
PUT/DELETE) can lose concurrent updates to shared gateway_groups; change it to
perform an optimistic-concurrency retry: before any modifying PUT/DELETE, fetch
the fresh plugin state for existing.id (call the API to GET
/api/custom_plugins/{id} instead of relying on the cached list), compute the new
gateway_groups from that fresh state, attempt the PUT/DELETE and if the server
returns a conflict (e.g., 409) or the returned state differs, retry the
read-modify-write cycle a bounded number of times (e.g., 3) and then fail;
update the code paths in operateCustomPlugin that build the PUT/DELETE payloads
to use the freshly fetched plugin, and prefer using If-Match/ETag headers if the
API supports them when issuing client.request to make this compare-and-set
robust.
Source: Coding guidelines
Comment on lines
+321
to
+343
| if (event.type === ADCSDK.EventType.DELETE) { | ||
| if (!existing) | ||
| return this.syntheticResponse('delete', 'custom plugin already absent'); | ||
|
|
||
| const remaining = (existing.gateway_groups ?? []).filter( | ||
| (id) => id !== groupId, | ||
| ); | ||
| if (remaining.length === 0) | ||
| return this.client.request({ | ||
| method: 'DELETE', | ||
| url: `/api/custom_plugins/${existing.id}`, | ||
| }); | ||
|
|
||
| return this.client.request({ | ||
| method: 'PUT', | ||
| url: `/api/custom_plugins/${existing.id}`, | ||
| data: { | ||
| ...fromADC.transformCustomPlugin( | ||
| event.oldValue as ADCSDK.CustomPlugin, | ||
| ), | ||
| gateway_groups: remaining, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Build the prune-body from existing, not event.oldValue.
When a delete only removes this gateway group, this PUT reconstructs the full plugin payload from event.oldValue. The provided e2e call pattern deletes with deleteEvent(ADCSDK.ResourceType.CUSTOM_PLUGIN, pluginName), so oldValue is not guaranteed to exist here; transformCustomPlugin(event.oldValue as ADCSDK.CustomPlugin) can therefore blow up before the request is sent. Even when oldValue is present, it can overwrite newer shared metadata/source on the control plane. Rebuild the body from existing and only replace gateway_groups.
🤖 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/operator.ts` around lines 321 - 343, The delete-path
currently uses fromADC.transformCustomPlugin(event.oldValue as
ADCSDK.CustomPlugin) which can be undefined and may overwrite newer data;
instead, build the PUT request body from the existing object: use the existing
custom plugin record (existing) as the base, copy/transform whatever server-side
representation is required (or call fromADC.transformCustomPlugin on existing if
appropriate), then set gateway_groups to the filtered remaining array and send
that in this.client.request for the PUT; update the branch handling
ADCSDK.EventType.DELETE (the block that computes remaining and issues the PUT)
to derive the prune-body from existing rather than event.oldValue, keeping the
same URL/existing.id and only replacing gateway_groups.
Comment on lines
+359
to
+387
| const customPluginSchema = z | ||
| .strictObject({ | ||
| id: idSchema.optional(), | ||
| // The plugin name must match the one declared inside the Lua source. It is | ||
| // declared explicitly so the diff can be keyed without parsing the source. | ||
| name: z | ||
| .string() | ||
| .min(1) | ||
| .max(256) | ||
| .regex(/^[a-zA-Z0-9-_.]+$/), | ||
| description: descriptionSchema.optional(), | ||
|
|
||
| // The Lua source. Either provided inline via "content" or referenced from | ||
| // an external file via "path" (resolved into "content" at load time). | ||
| content: z.string().min(1).optional(), | ||
| path: z.string().min(1).optional(), | ||
|
|
||
| // Display-only metadata stored on the control plane. | ||
| catalog: z.string().optional(), | ||
| documentation_link: z.string().optional(), | ||
| author: z.string().optional(), | ||
| logo: z.string().optional(), | ||
| }) | ||
| .refine((val) => !isNil(val.content) || !isNil(val.path), { | ||
| error: 'Custom plugin must set either "content" or "path"', | ||
| }) | ||
| .refine((val) => !(!isNil(val.content) && !isNil(val.path)), { | ||
| error: 'Custom plugin "content" and "path" are mutually exclusive', | ||
| }); |
There was a problem hiding this comment.
Model the content/path exclusivity structurally, not with refine().
Lines 382-387 enforce the invariant at runtime, but the generated schema.json drops it entirely, so { name } and { name, content, path } validate against the published schema and then fail later in Zod parsing. That breaks the SDK's external config contract for custom_plugins.
Suggested shape that preserves the constraint in generated JSON Schema
-const customPluginSchema = z
- .strictObject({
- id: idSchema.optional(),
- // The plugin name must match the one declared inside the Lua source. It is
- // declared explicitly so the diff can be keyed without parsing the source.
- name: z
- .string()
- .min(1)
- .max(256)
- .regex(/^[a-zA-Z0-9-_.]+$/),
- description: descriptionSchema.optional(),
-
- // The Lua source. Either provided inline via "content" or referenced from
- // an external file via "path" (resolved into "content" at load time).
- content: z.string().min(1).optional(),
- path: z.string().min(1).optional(),
-
- // Display-only metadata stored on the control plane.
- catalog: z.string().optional(),
- documentation_link: z.string().optional(),
- author: z.string().optional(),
- logo: z.string().optional(),
- })
- .refine((val) => !isNil(val.content) || !isNil(val.path), {
- error: 'Custom plugin must set either "content" or "path"',
- })
- .refine((val) => !(!isNil(val.content) && !isNil(val.path)), {
- error: 'Custom plugin "content" and "path" are mutually exclusive',
- });
+const customPluginBaseSchema = z.strictObject({
+ id: idSchema.optional(),
+ name: z.string().min(1).max(256).regex(/^[a-zA-Z0-9-_.]+$/),
+ description: descriptionSchema.optional(),
+ catalog: z.string().optional(),
+ documentation_link: z.string().optional(),
+ author: z.string().optional(),
+ logo: z.string().optional(),
+});
+
+const customPluginSchema = z.union([
+ customPluginBaseSchema.extend({
+ content: z.string().min(1),
+ path: z.never().optional(),
+ }),
+ customPluginBaseSchema.extend({
+ path: z.string().min(1),
+ content: z.never().optional(),
+ }),
+]);📝 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
| const customPluginSchema = z | |
| .strictObject({ | |
| id: idSchema.optional(), | |
| // The plugin name must match the one declared inside the Lua source. It is | |
| // declared explicitly so the diff can be keyed without parsing the source. | |
| name: z | |
| .string() | |
| .min(1) | |
| .max(256) | |
| .regex(/^[a-zA-Z0-9-_.]+$/), | |
| description: descriptionSchema.optional(), | |
| // The Lua source. Either provided inline via "content" or referenced from | |
| // an external file via "path" (resolved into "content" at load time). | |
| content: z.string().min(1).optional(), | |
| path: z.string().min(1).optional(), | |
| // Display-only metadata stored on the control plane. | |
| catalog: z.string().optional(), | |
| documentation_link: z.string().optional(), | |
| author: z.string().optional(), | |
| logo: z.string().optional(), | |
| }) | |
| .refine((val) => !isNil(val.content) || !isNil(val.path), { | |
| error: 'Custom plugin must set either "content" or "path"', | |
| }) | |
| .refine((val) => !(!isNil(val.content) && !isNil(val.path)), { | |
| error: 'Custom plugin "content" and "path" are mutually exclusive', | |
| }); | |
| const customPluginBaseSchema = z.strictObject({ | |
| id: idSchema.optional(), | |
| // The plugin name must match the one declared inside the Lua source. It is | |
| // declared explicitly so the diff can be keyed without parsing the source. | |
| name: z | |
| .string() | |
| .min(1) | |
| .max(256) | |
| .regex(/^[a-zA-Z0-9-_.]+$/), | |
| description: descriptionSchema.optional(), | |
| // Display-only metadata stored on the control plane. | |
| catalog: z.string().optional(), | |
| documentation_link: z.string().optional(), | |
| author: z.string().optional(), | |
| logo: z.string().optional(), | |
| }); | |
| const customPluginSchema = z.union([ | |
| customPluginBaseSchema.extend({ | |
| // The Lua source. Either provided inline via "content" or referenced from | |
| // an external file via "path" (resolved into "content" at load time). | |
| content: z.string().min(1), | |
| path: z.never().optional(), | |
| }), | |
| customPluginBaseSchema.extend({ | |
| content: z.never().optional(), | |
| path: z.string().min(1), | |
| }), | |
| ]); |
🤖 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/sdk/src/core/schema.ts` around lines 359 - 387, Replace the runtime
.refine checks with a structural union so the generated JSON Schema preserves
the exclusivity: remove content and path from the base strict object (the
existing customPluginSchema base that references idSchema and
descriptionSchema), then create two variants and union them: one variant extends
the base with content: z.string().min(1) and path: z.undefined(), the other
extends the base with path: z.string().min(1) and content: z.undefined(), and
finally set customPluginSchema = z.union([contentVariant, pathVariant]); keep
the same name/id/metadata fields (idSchema, descriptionSchema, catalog,
documentation_link, author, logo) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
custom_pluginsas a first-class, declaratively-managed resource in ADC on the API7 EE (api7ee) backend, so user-defined Lua plugins can be created, updated, and pruned throughadc sync/adc dumplike any other resource.Why: custom plugins were the one API7 EE resource with no ADC representation. They are not addressable via the gateway-group-scoped admin API; they live on the control plane at
/api/custom_pluginsas global objects carrying agateway_groupsmembership list, which is why they need bespoke handling rather than the generic resource path.What changed:
ResourceType.CUSTOM_PLUGINandcustomPluginSchema(anameplus either an inlinecontentor an externalpath, mutually exclusive), wired intoConfiguration/InternalConfigurationandResourceFor./api/custom_plugins. Because a custom plugin is control-plane-global but a sync targets a single gateway group:pathinto inlinecontentat load time (relative to the config file) and validates that the declarednameappears in the plaintext Lua source, catching typos locally.Example:
Known limitations (by design): obfuscated/bytecode plugins won't round-trip cleanly (their returned source won't match local plaintext) and are expected to be managed outside the declarative flow; an update pushes the local source to all of the plugin's member groups, since it is a single shared object; managing custom plugins requires a resolved gateway group.
Checklist
Tested locally:
nx run-many -t typecheck(all projects), unit tests forsdk/differ/backend-api7/cli(all green, including the regeneratedschema.jsonconsistency test), plus lint and prettier. The backend-api7 e2e CRUD spec requires a live API7 EE backend and runs in CI. The "documentation" box is unchecked because the README does not enumerate supported resource types, so there is no doc table to update.Summary by CodeRabbit
Release Notes
New Features
Tests