feat: add plugin configuration validation to lint#426
feat: add plugin configuration validation to lint#426jarvis9443 wants to merge 6 commits intomainfrom
Conversation
Add two-phase validation to ADC's lint functionality: Phase 1 (local): Existing core resource Zod schema validation Phase 2 (optional, backend-connected): Plugin config validation using JSON Schema fetched from APISIX/API7 EE Admin API Key changes: - Add PluginSchemaEntry/PluginSchemaMap types to SDK Backend interface - Implement fetchPluginSchemas() in both BackendAPISIX and BackendAPI7 - Create plugin-validator.ts with AJV-based draft-04 JSON Schema validation - Introduce LintError/LintResult types for unified error reporting - Upgrade lint command with optional --backend/--server/--token/--gateway-group - Integrate FetchPluginSchemasTask into diff/sync commands - Add 20 new test cases covering all plugin locations and edge cases Plugin validation traverses all config locations: - services[*].plugins, routes[*].plugins, stream_routes[*].plugins - consumers[*].plugins, consumer_groups[*].plugins - consumers[*].credentials[*].config (consumerSchema) - global_rules, plugin_metadata (metadataSchema) The lint command remains a BaseCommand (no default backend connection). Backend params are optional - without them, only core validation runs. APISIX Standalone backend gracefully skips plugin validation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds backend-driven plugin schema fetching and integrates plugin-schema validation into the CLI linter flow, updating types, tasks, commands, tests, and backend implementations to fetch, cache, and use plugin JSON Schemas during lint/diff/sync operations. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Command
participant Fetch as FetchPluginSchemasTask
participant Backend as Backend (APISIX/API7)
participant Lint as LintTask
participant Validator as Plugin Validator
User->>CLI: run lint/diff/sync with --backend
CLI->>Fetch: enable & execute task
Fetch->>Backend: fetchPluginSchemas()
Backend->>Backend: request plugin list
Backend->>Backend: concurrently fetch each plugin schema (config, optional consumer)
Backend-->>Fetch: return PluginSchemaMap (cached)
Fetch-->>CLI: store pluginSchemas in context
CLI->>Lint: run lint with local config + pluginSchemas
Lint->>Validator: validatePlugins(config, pluginSchemas)
Validator-->>Lint: return LintError[]
Lint-->>CLI: return LintResult { success, errors, data }
CLI-->>User: display results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Change LintError.path type from (string | number)[] to PropertyKey[] to match Zod's issue path type (which can include symbols) - Remove unused direct 'ajv' dependency from CLI package (ajv-draft-04 includes it as a peer dependency)
- Remove accidentally committed out-tsc/ directories from libs/ - Change .gitignore from '/out-tsc' to 'out-tsc' to cover all nested dirs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cli/src/command/sync.command.ts (1)
84-91:⚠️ Potential issue | 🟠 MajorRespect
--no-lint: schema fetch should be gated with lint execution.Line 84 currently runs
FetchPluginSchemasTask()even when lint is disabled (Line 91). If schema fetch fails,synccan fail in--no-lintmode for a non-lint concern.Suggested adjustment
- FetchPluginSchemasTask(), + opts.lint ? FetchPluginSchemasTask() : { task: () => undefined },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/command/sync.command.ts` around lines 84 - 91, The FetchPluginSchemasTask is being executed regardless of the --no-lint flag and should only run when linting is enabled; modify the task list in sync.command.ts so FetchPluginSchemasTask() is included only if opts.lint is true (similar to how LintTask() is conditionally selected), for example wrap or replace FetchPluginSchemasTask() with a conditional expression using opts.lint so that when lint is disabled it returns a no-op task (matching the existing { task: () => undefined } pattern).apps/cli/src/command/diff.command.ts (1)
105-107:⚠️ Potential issue | 🟠 MajorDon’t silently swallow the caught error.
At Line 105,
erris captured but never handled, which also causes the reported ESLint warning. Please at least log the error before exiting.Suggested fix
- } catch (err) { - process.exit(1); + } catch (err) { + console.error(err); + process.exit(1); }As per coding guidelines
**/*.{js,ts,jsx,tsx}: Every function return value must be checked for errors (if applicable); errors must be properly handled, not ignored or silently swallowed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/command/diff.command.ts` around lines 105 - 107, The catch block that currently swallows the error (catch (err) { process.exit(1); }) should log the caught error before exiting: update the catch to log the error (e.g., using console.error or the module's logger) with a clear message referencing the failing operation and the err object, then call process.exit(1); ensure you modify the catch surrounding the code in diff.command.ts so that err is not ignored and the error details are emitted.
🧹 Nitpick comments (8)
apps/cli/src/command/lint.command.ts (1)
67-82: Consider validating required backend options when--backendis provided.When
--backendis specified,--serverand--tokenare likely required for the backend to function. Currently, if a user provides--backend apisixwithout--server, the error will occur deep inInitializeBackendTaskor during the first API call, which may be confusing.💡 Suggested validation before task execution
.action(async () => { const opts = LintCommand.optsWithGlobals(); const useBackend = !!opts.backend; + + if (useBackend && (!opts.server || !opts.token)) { + console.error('Error: --server and --token are required when --backend is specified'); + process.exit(1); + } const tasks = new Listr(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/command/lint.command.ts` around lines 67 - 82, When opts.backend is provided (useBackend true) validate required backend options (e.g., opts.server and opts.token) before creating the Listr tasks so users get a clear early error instead of a failure inside InitializeBackendTask or later API calls; update the code around useBackend/InitializeBackendTask to perform a pre-check (or add a small ValidateBackendOptionsTask) that ensures opts.server and opts.token are present and throw or log a descriptive error if missing, referencing opts.backend, InitializeBackendTask, and LintTask so the validation runs prior to task execution.apps/cli/src/linter/plugin-validator.ts (2)
134-149: Inconsistent handling of unknown plugins in consumer credentials.When a credential references an unknown plugin type (line 136-137), validation is silently skipped. This differs from
validatePluginsMapwhich reports anunknown_pluginerror. Consider reporting unknown credential plugin types for consistency.💡 Suggested fix for consistency
consumer.credentials?.forEach((credential, cri) => { const pluginName = credential.type; const schema = schemas[pluginName]; - if (schema) { + if (!schema) { + errors.push({ + path: ['consumers', ci, 'credentials', cri, 'type'], + message: `Unknown credential plugin type "${pluginName}"`, + code: 'unknown_plugin', + }); + } else { errors.push( ...validateSinglePlugin(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/linter/plugin-validator.ts` around lines 134 - 149, The consumer credential loop currently skips validation when schemas[pluginName] is missing, silently ignoring unknown credential plugin types; update the loop in plugin-validator.ts (inside consumer.credentials?.forEach) to mirror validatePluginsMap by pushing an error when credential.type isn't found: construct and push an 'unknown_plugin' error referencing the path ['consumers', ci, 'credentials', cri] (or similar) and include pluginName, so unknown credential plugin types are reported instead of silently ignored; keep using the same error shape/fields validatePluginsMap uses for consistency with validateSinglePlugin and the rest of the validator.
25-26: Consider handling potentialajv.compile()exceptions.
ajv.compile()can throw if the JSON schema is malformed (e.g., invalid$ref, unsupported keywords). Since schemas are fetched from the backend and may vary across versions, a try-catch would provide more graceful error handling.💡 Suggested defensive handling
const validateSinglePlugin = ( ajv: Ajv, pluginName: string, pluginConfig: Record<string, unknown>, schema: PluginSchemaEntry, path: Array<string | number>, schemaType: 'configSchema' | 'consumerSchema' | 'metadataSchema', ): LintError[] => { const errors: LintError[] = []; const jsonSchema = schema[schemaType]; if (!jsonSchema) return errors; + let validate; + try { + validate = ajv.compile(jsonSchema); + } catch (e) { + errors.push({ + path: [...path, pluginName], + message: `Failed to compile schema for plugin: ${e instanceof Error ? e.message : 'unknown error'}`, + code: 'schema_compile_error', + }); + return errors; + } - const validate = ajv.compile(jsonSchema); if (!validate(pluginConfig)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/linter/plugin-validator.ts` around lines 25 - 26, ajv.compile(jsonSchema) can throw on malformed schemas; wrap the compilation in a try-catch around the ajv.compile call (the code creating the validate function) so compilation errors are caught and handled gracefully instead of propagating. In the catch, log or surface the compilation error (including the error message and the offending jsonSchema context) and return/throw a controlled validation failure for the calling code that uses validate and pluginConfig, ensuring the rest of plugin-validator.ts behaves predictably when schema compilation fails.libs/backend-apisix/src/index.ts (1)
110-163: Same concerns asBackendAPI7.fetchPluginSchemas()apply here.This implementation has the same issues noted in the API7 backend:
- Missing
metadataSchemafetch (if needed for plugin metadata validation)- Silent error handling in catch blocks
- Minor race condition on concurrent calls
Additionally, there's significant code duplication between
BackendAPI7andBackendAPISIXforfetchPluginSchemas(). Consider extracting the common logic to a shared utility function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix/src/index.ts` around lines 110 - 163, fetchPluginSchemas in BackendAPISIX duplicates BackendAPI7 logic, silently swallows errors, omits fetching metadataSchema, and has a race on concurrent callers; refactor by extracting the shared fetching logic into a common utility (e.g., a new fetchPluginSchemasShared function) used by both BackendAPISIX.fetchPluginSchemas and BackendAPI7.fetchPluginSchemas, add a memoized in-flight promise property (e.g., _pluginSchemasPromise) to prevent concurrent duplicate fetches, fetch the metadataSchema alongside configSchema and consumerSchema (call the same endpoint with params schema_type='metadata' and attach to the PluginSchemaEntry as metadataSchema), and replace empty catch blocks with explicit error handling that emits/logs the error via this.subject (use ADCSDK.BackendEventType.AXIOS_DEBUG or a suitable ERROR type and include the error/response and plugin name) so failures are visible while still skipping unavailable schemas.libs/backend-api7/src/index.ts (1)
231-284: MissingmetadataSchemafetch and silent error handling.
The
PluginSchemaEntryinterface (fromlibs/sdk/src/backend/index.ts:75-79) includes an optionalmetadataSchema, but this implementation never fetches it. If plugin metadata validation is intended (per the PR objectives mentioningmetadataSchema), you'll need to add a fetch forschema_type=metadatasimilar to the consumer schema fetch.The empty
catchblocks (lines 271-273, 276-278) silently swallow errors without any logging, making debugging difficult when plugin schemas fail to load. Consider emitting a debug event or logging a warning.Minor race condition: if
fetchPluginSchemas()is called concurrently before the first call completes, both calls will proceed past the cache check (line 232) and fetch redundantly. This is a minor optimization issue but worth noting.Suggested improvements
// Fetch consumer schema if applicable try { const consumerResp = await this.client.get<JSONSchema4>( `/apisix/admin/schema/plugins/${name}`, { params: { schema_type: 'consumer' } }, ); + this.subject.next({ + type: ADCSDK.BackendEventType.AXIOS_DEBUG, + event: { + response: consumerResp, + description: `Get plugin consumer schema: ${name}`, + }, + }); if (consumerResp.data && Object.keys(consumerResp.data).length > 0) entry.consumerSchema = consumerResp.data; } catch { - // Plugin doesn't have a consumer schema — skip + // Plugin doesn't have a consumer schema — skip (expected for most plugins) } schemas[name] = entry; } catch { - // Skip plugins whose schema cannot be fetched + // Skip plugins whose schema cannot be fetched + this.subject.next({ + type: ADCSDK.BackendEventType.AXIOS_DEBUG, + event: { response: null, description: `Failed to fetch schema for plugin: ${name}` }, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-api7/src/index.ts` around lines 231 - 284, fetchPluginSchemas currently never fetches PluginSchemaEntry.metadataSchema, swallows errors silently in two empty catch blocks, and can run twice concurrently; update fetchPluginSchemas to also request `/apisix/admin/schema/plugins/${name}` with params { schema_type: 'metadata' } and assign it to entry.metadataSchema if present (same pattern as consumerSchema), replace empty catch blocks with debug/warn emits using this.subject.next (e.g., BackendEventType.AXIOS_DEBUG or a warning event) including the caught error and context (plugin name and schema type), and add a simple concurrency guard (introduce a private _pluginSchemasPromise checked/assigned at the start of fetchPluginSchemas so concurrent callers await the same promise) to prevent duplicate fetches; reference fetchPluginSchemas, PluginSchemaEntry, this.client.get, this.subject.next, this._pluginSchemas and introduce _pluginSchemasPromise.apps/cli/src/linter/specs/plugin.spec.ts (3)
152-230: Add explicit min-boundary tests forrate-limitingschema constraints.
countandtime_windowdefineminimum: 1, but there’s no boundary coverage for invalid0and valid1.Suggested boundary test additions
+ it('should reject values below minimum in rate-limiting config', () => { + const result = check( + { + services: [ + { + name: 'test-svc', + plugins: { 'rate-limiting': { count: 0, time_window: 1 } }, + routes: [{ name: 'test-route', uris: ['/test'] }], + }, + ], + }, + { pluginSchemas: mockPluginSchemas }, + ); + expect(result.success).toBe(false); + expect(result.errors).toContainEqual( + expect.objectContaining({ + path: ['services', 0, 'plugins', 'rate-limiting', 'count'], + code: 'plugin_schema_violation', + }), + ); + }); + + it('should accept minimum boundary values in rate-limiting config', () => { + const result = check( + { + services: [ + { + name: 'test-svc', + plugins: { 'rate-limiting': { count: 1, time_window: 1 } }, + routes: [{ name: 'test-route', uris: ['/test'] }], + }, + ], + }, + { pluginSchemas: mockPluginSchemas }, + ); + expect(result.success).toBe(true); + });As per coding guidelines: "Tests must cover boundary cases (empty values, min/max), invalid inputs, combination scenarios, and extreme cases (high load, failures)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/linter/specs/plugin.spec.ts` around lines 152 - 230, Add explicit boundary tests to the existing "plugin config validation" suite that exercise the rate-limiting plugin's minimum constraints: create tests (using the same check(...) helper and mockPluginSchemas) that assert a failing result when count is 0 (with time_window valid), failing when time_window is 0 (with count valid), and succeeding when both count and time_window are 1; reference the existing test cases (e.g., the "should report type mismatch" and "should report multiple errors at once" patterns) to mirror structure and assertions (expect(result.success).toBe(...), expect(result.errors) checks) and ensure paths point to ['services', 0, 'plugins', 'rate-limiting', 'count'] or ['...','time_window'] as appropriate.
247-395: Use order-independent assertions instead ofresult.errors[0].These assertions are brittle because they rely on error ordering. Prefer
toContainEqual/arrayContaining+objectContainingso tests validate content, not position.Suggested assertion pattern
- expect(result.errors[0].path).toEqual([ - 'services', - 0, - 'plugins', - 'key-auth', - ]); + expect(result.errors).toContainEqual( + expect.objectContaining({ + path: ['services', 0, 'plugins', 'key-auth'], + code: 'plugin_schema_violation', + }), + );As per coding guidelines: "Tests must not have hidden execution order assumptions; explicit dependencies between tests should be clear."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/linter/specs/plugin.spec.ts` around lines 247 - 395, The tests in plugin.spec.ts assert error positions by checking result.errors[0].path (e.g., in the tests that validate plugins under services/routes/stream_routes/consumers/consumer_groups/global_rules and plugin_metadata), which is brittle; update each assertion that uses result.errors[0] to an order-independent check by asserting the errors array contains an element with the expected path and properties (use toContainEqual or expect.arrayContaining + expect.objectContaining against result.errors and the path arrays like ['services',0,'routes',0,'plugins','key-auth'], ['consumers',0,'credentials',0,'config','key-auth'], ['plugin_metadata', ...], etc.), so tests verify content not ordering while still referencing the same result.errors and path shapes produced by check().
440-464: Strengthen the “core validation first” assertion with a structured core-error check.Current check proves
unknown_pluginis absent, but it doesn’t explicitly verify that the returned failure is the expected core-route validation error.As per coding guidelines: "Assertions must be readable; avoid complex regex, hard-to-understand logic, and prefer structured assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/linter/specs/plugin.spec.ts` around lines 440 - 464, The test "should still do core validation first" currently only asserts that no 'unknown_plugin' error exists; instead, strengthen it by asserting that the failure is the expected core route validation error: after calling check(...) and verifying result.success is false, assert that result.errors contains a structured core error pointing at the missing 'uris' on the route (e.g., check result.errors.some(e => e.path && e.path.includes('uris') && e.code === 'required' or otherwise matches the core validation shape), rather than just checking absence of 'unknown_plugin'); reference the same symbols used in the test (check, mockPluginSchemas, result, result.errors) so the assertion clearly verifies the core-route validation error is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/cli/src/linter/index.ts`:
- Around line 10-16: The mapped object in zodIssuesToLintErrors returns extra
fields from z.ZodIssue via the rest spread that conflict with the LintError
type, causing TS2322; fix it by returning an object that exactly matches the
LintError shape (do not spread unknown properties), e.g. explicitly pick/assign
only the LintError fields inside zodIssuesToLintErrors or cast the remainder to
a compatible type (e.g. Record<string, any>) before merging; refer to the
zodIssuesToLintErrors function, the z.ZodIssue type and the LintError type when
making the change so the returned object strictly conforms to LintError.
In `@package.json`:
- Around line 41-43: Remove the unused dependencies "ajv" and "ajv-draft-04"
from the dependencies section of package.json (they are the entries under
"dependencies" with keys ajv and ajv-draft-04) because `@nx/dependency-checks`
reports no imports; after removal, run your package manager to update the
lockfile (e.g., npm/yarn/pnpm install) so CI won't be blocked by the stale
entries.
---
Outside diff comments:
In `@apps/cli/src/command/diff.command.ts`:
- Around line 105-107: The catch block that currently swallows the error (catch
(err) { process.exit(1); }) should log the caught error before exiting: update
the catch to log the error (e.g., using console.error or the module's logger)
with a clear message referencing the failing operation and the err object, then
call process.exit(1); ensure you modify the catch surrounding the code in
diff.command.ts so that err is not ignored and the error details are emitted.
In `@apps/cli/src/command/sync.command.ts`:
- Around line 84-91: The FetchPluginSchemasTask is being executed regardless of
the --no-lint flag and should only run when linting is enabled; modify the task
list in sync.command.ts so FetchPluginSchemasTask() is included only if
opts.lint is true (similar to how LintTask() is conditionally selected), for
example wrap or replace FetchPluginSchemasTask() with a conditional expression
using opts.lint so that when lint is disabled it returns a no-op task (matching
the existing { task: () => undefined } pattern).
---
Nitpick comments:
In `@apps/cli/src/command/lint.command.ts`:
- Around line 67-82: When opts.backend is provided (useBackend true) validate
required backend options (e.g., opts.server and opts.token) before creating the
Listr tasks so users get a clear early error instead of a failure inside
InitializeBackendTask or later API calls; update the code around
useBackend/InitializeBackendTask to perform a pre-check (or add a small
ValidateBackendOptionsTask) that ensures opts.server and opts.token are present
and throw or log a descriptive error if missing, referencing opts.backend,
InitializeBackendTask, and LintTask so the validation runs prior to task
execution.
In `@apps/cli/src/linter/plugin-validator.ts`:
- Around line 134-149: The consumer credential loop currently skips validation
when schemas[pluginName] is missing, silently ignoring unknown credential plugin
types; update the loop in plugin-validator.ts (inside
consumer.credentials?.forEach) to mirror validatePluginsMap by pushing an error
when credential.type isn't found: construct and push an 'unknown_plugin' error
referencing the path ['consumers', ci, 'credentials', cri] (or similar) and
include pluginName, so unknown credential plugin types are reported instead of
silently ignored; keep using the same error shape/fields validatePluginsMap uses
for consistency with validateSinglePlugin and the rest of the validator.
- Around line 25-26: ajv.compile(jsonSchema) can throw on malformed schemas;
wrap the compilation in a try-catch around the ajv.compile call (the code
creating the validate function) so compilation errors are caught and handled
gracefully instead of propagating. In the catch, log or surface the compilation
error (including the error message and the offending jsonSchema context) and
return/throw a controlled validation failure for the calling code that uses
validate and pluginConfig, ensuring the rest of plugin-validator.ts behaves
predictably when schema compilation fails.
In `@apps/cli/src/linter/specs/plugin.spec.ts`:
- Around line 152-230: Add explicit boundary tests to the existing "plugin
config validation" suite that exercise the rate-limiting plugin's minimum
constraints: create tests (using the same check(...) helper and
mockPluginSchemas) that assert a failing result when count is 0 (with
time_window valid), failing when time_window is 0 (with count valid), and
succeeding when both count and time_window are 1; reference the existing test
cases (e.g., the "should report type mismatch" and "should report multiple
errors at once" patterns) to mirror structure and assertions
(expect(result.success).toBe(...), expect(result.errors) checks) and ensure
paths point to ['services', 0, 'plugins', 'rate-limiting', 'count'] or
['...','time_window'] as appropriate.
- Around line 247-395: The tests in plugin.spec.ts assert error positions by
checking result.errors[0].path (e.g., in the tests that validate plugins under
services/routes/stream_routes/consumers/consumer_groups/global_rules and
plugin_metadata), which is brittle; update each assertion that uses
result.errors[0] to an order-independent check by asserting the errors array
contains an element with the expected path and properties (use toContainEqual or
expect.arrayContaining + expect.objectContaining against result.errors and the
path arrays like ['services',0,'routes',0,'plugins','key-auth'],
['consumers',0,'credentials',0,'config','key-auth'], ['plugin_metadata', ...],
etc.), so tests verify content not ordering while still referencing the same
result.errors and path shapes produced by check().
- Around line 440-464: The test "should still do core validation first"
currently only asserts that no 'unknown_plugin' error exists; instead,
strengthen it by asserting that the failure is the expected core route
validation error: after calling check(...) and verifying result.success is
false, assert that result.errors contains a structured core error pointing at
the missing 'uris' on the route (e.g., check result.errors.some(e => e.path &&
e.path.includes('uris') && e.code === 'required' or otherwise matches the core
validation shape), rather than just checking absence of 'unknown_plugin');
reference the same symbols used in the test (check, mockPluginSchemas, result,
result.errors) so the assertion clearly verifies the core-route validation error
is present.
In `@libs/backend-api7/src/index.ts`:
- Around line 231-284: fetchPluginSchemas currently never fetches
PluginSchemaEntry.metadataSchema, swallows errors silently in two empty catch
blocks, and can run twice concurrently; update fetchPluginSchemas to also
request `/apisix/admin/schema/plugins/${name}` with params { schema_type:
'metadata' } and assign it to entry.metadataSchema if present (same pattern as
consumerSchema), replace empty catch blocks with debug/warn emits using
this.subject.next (e.g., BackendEventType.AXIOS_DEBUG or a warning event)
including the caught error and context (plugin name and schema type), and add a
simple concurrency guard (introduce a private _pluginSchemasPromise
checked/assigned at the start of fetchPluginSchemas so concurrent callers await
the same promise) to prevent duplicate fetches; reference fetchPluginSchemas,
PluginSchemaEntry, this.client.get, this.subject.next, this._pluginSchemas and
introduce _pluginSchemasPromise.
In `@libs/backend-apisix/src/index.ts`:
- Around line 110-163: fetchPluginSchemas in BackendAPISIX duplicates
BackendAPI7 logic, silently swallows errors, omits fetching metadataSchema, and
has a race on concurrent callers; refactor by extracting the shared fetching
logic into a common utility (e.g., a new fetchPluginSchemasShared function) used
by both BackendAPISIX.fetchPluginSchemas and BackendAPI7.fetchPluginSchemas, add
a memoized in-flight promise property (e.g., _pluginSchemasPromise) to prevent
concurrent duplicate fetches, fetch the metadataSchema alongside configSchema
and consumerSchema (call the same endpoint with params schema_type='metadata'
and attach to the PluginSchemaEntry as metadataSchema), and replace empty catch
blocks with explicit error handling that emits/logs the error via this.subject
(use ADCSDK.BackendEventType.AXIOS_DEBUG or a suitable ERROR type and include
the error/response and plugin name) so failures are visible while still skipping
unavailable schemas.
🪄 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: 09e2cbc9-ddd0-44af-a497-d541a016ed34
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
apps/cli/package.jsonapps/cli/src/command/diff.command.tsapps/cli/src/command/lint.command.tsapps/cli/src/command/sync.command.tsapps/cli/src/linter/index.tsapps/cli/src/linter/plugin-validator.tsapps/cli/src/linter/specs/common.spec.tsapps/cli/src/linter/specs/consumer.spec.tsapps/cli/src/linter/specs/plugin.spec.tsapps/cli/src/linter/specs/route.spec.tsapps/cli/src/linter/specs/ssl.spec.tsapps/cli/src/linter/specs/upstream.spec.tsapps/cli/src/linter/types.tsapps/cli/src/server/sync.tsapps/cli/src/tasks/fetch_plugin_schemas.tsapps/cli/src/tasks/index.tsapps/cli/src/tasks/lint.tslibs/backend-api7/package.jsonlibs/backend-api7/src/index.tslibs/backend-apisix/package.jsonlibs/backend-apisix/src/index.tslibs/sdk/package.jsonlibs/sdk/src/backend/index.tspackage.jsonpnpm-workspace.yaml
These belong only in pnpm-workspace.yaml catalog and apps/cli/package.json, not in the root workspace package.json.
When --no-lint is used, skip FetchPluginSchemasTask to avoid unnecessary network requests and potential failures for a non-lint concern.
|
Addressed CodeRabbit review findings:
Nitpick comments noted but not applied to keep the PR scope focused on the core plugin validation feature. |
|
This does not solve 90% of the problems, because configuration errors outside of any plugins cannot be checked. As a result, even if the advanced mode of the linter passes, the false negatives it reports are meaningless, since the server will reject other configuration errors. Furthermore, even if we take a step back and assume that plugin-only validation is acceptable, the current approach cannot verify whether the plugin configuration is truly, completely correct. This is because many plugins include not only JSON Schema validation but also code-driven configuration validation, which the current method cannot check. A prime example is the CORS plugin. And without a once-and-for-all solution, I am unwilling to endorse the functionality claimed by the lint tool. Even if this PR doesn’t exist, we can still prevent configuration errors through workflow design. Once the PR is merged or a tag is pushed to the main branch, we can then initiate a sync to the production environment—all this can be achieved simply by loading different environment variables at runtime. Any configuration errors are detected and fixed during the PR before the PR is merged. As long as all CI checks pass, the configuration can be deployed to the production environment without issues. In fact, the |
|
Overall, I’m not optimistic about the content added in this PR. |
Two bugs fixed:
1. FetchPluginSchemasTask destructured fetchPluginSchemas from ctx.backend,
which detaches the method from its class instance and loses `this` binding.
Changed to call ctx.backend.fetchPluginSchemas() directly.
2. Lint command options used .env('ADC_BACKEND') etc, which caused env vars
(e.g. ADC_BACKEND=api7ee) to implicitly activate backend plugin validation
even when running plain `adc lint -f file.yaml`. Removed .env() from lint
options so backend validation is only activated via explicit CLI flags.
This keeps lint as a local-first command that doesn't require network access
by default.
bzp2010
added
test/api7
This comment was marked as outdated.
This comment was marked as outdated.
@bzp2010 Why is that? Even though some plugins need to be validated through Lua code, most plugin configurations are validated via json schema. What exactly do you mean by the 90% of the problems you mentioned? If you consider those plugins that need to be verified through Lua code, we can provide an API on the backend for the ADC to perform remote verification. Would this solve the problem 100%? |
Any issue will cause the synchronization process to be interrupted. Local lint is useless against the latter two issues. Consequently, "false negatives" in the This introduces unnecessary complexity.
If the solution doesn’t fully resolve (validation passed = server unconditionally accepts the configuration during synchronization) the issue, I’d rather not do it. Engineering solutions can resolve the issue hacky. |
|
@bzp2010 Is there any linter tool that claims to solve all production problems? All the user needs is a configuration checking capability, so why include all so-called synchronization failure issues in the current requirement? |
|
If we don’t do it, the sync process might be interrupted by an error during synchronization. |
|
Why is there a lint function now? Following this reasoning, wouldn't the lint function be completely worthless? But in fact, users are using it and hoping it will be more powerful. |
|
You are right. The fundamental issue isn’t whether an ADC can detect and flag errors locally, but rather that we can never fully align client-side and server-side checks. UPDATE: The Zod schema does not allow any undefined fields to pass validation, so the spell check is entirely valid. |
|
adc/apps/cli/src/command/lint.command.ts Lines 7 to 11 in b3ef55f Furthermore, if you read the code carefully, you’ll discover the original design intent behind the THAT IS It was never intended to ensure that the configuration passes server-side validation, but rather to verify that the input in the local configuration file meets ADC’s own formatting requirements—such as mandatory fields, correct data types, valid data ranges and spelling errors. To elaborate, this is primarily to ensure that the subsequent ADC-to-Backend types conversion does not fail due to dirty data. A predefined Zod schema handles this process quite well; while I cannot guarantee its absolute effectiveness, it is sufficient for most scenarios. Users can choose to opt out of this (by Yes, it's mainly to prevent program crashes. |
3 participants
Summary
Add plugin configuration validation to ADC's lint functionality. Previously, lint only validated core resource structure (routes, services, upstreams, etc.) via Zod schemas, but accepted any plugin configuration without validation.
Changes
Two-Phase Validation
GET /apisix/admin/schema/plugins/{name}New Features
adc lintnow accepts optional--backend,--server,--token,--gateway-groupflags to enable plugin validationadc diffandadc syncautomatically validate plugin configs using the connected backendPlugin Validation Coverage
All plugin locations in the configuration are validated:
services[*].plugins,routes[*].plugins,stream_routes[*].pluginsconsumers[*].plugins,consumer_groups[*].pluginsconsumers[*].credentials[*].config(usingconsumerSchema)global_rules,plugin_metadata(usingmetadataSchema)Backend Support
Implementation Details
PluginSchemaEntry,PluginSchemaMaptypes andfetchPluginSchemas()to Backend interfacefetchPluginSchemas()in both BackendAPISIX and BackendAPI7LintError/LintResulttypes for both Zod and AJV errorsBaseCommand(no default backend connection).gitignorefix: Changed/out-tsctoout-tscto ignore build artifacts in all subdirectoriesTests
Added 20 new test cases covering:
All 45 tests pass (25 existing + 20 new).
Summary by CodeRabbit
New Features
Improvements