feat(api7): add server-side configuration validator#432
Conversation
Add a new 'validate' subcommand that validates local ADC configuration against the server-side /apisix/admin/configs/validate API. This performs comprehensive server-side validation (JSON Schema, plugin check_schema, duplicate ID detection) as a dry-run without persisting any changes. Changes: - SDK: Add BackendValidateResult, BackendValidationError interfaces and optional validate()/supportValidate() methods to Backend interface - backend-api7: Implement Validator class that transforms ADC hierarchical configuration to flat backend-native format and POSTs to validate endpoint - backend-api7: Add supportValidate() with version gating (>= 3.9.10) - CLI: Add ValidateTask with version checking and error formatting - CLI: Add validate command with -f, --no-lint options - E2E: Add validate test suite with valid/invalid/multi-error/dry-run cases APISIX and standalone backend support will follow in a separate PR.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end backend validation: a new CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Task as Validate Task
participant Backend as BackendAPI7
participant Validator as Validator Class
participant API as APISIX Admin API
CLI->>Task: run validate with options
Task->>Backend: supportValidate()
Backend->>Backend: version() & semver check
Backend-->>Task: support true/false
alt supported
Task->>Backend: validate(config)
Backend->>Validator: instantiate and call validate(config)
Validator->>API: POST /apisix/admin/configs/validate (rgba(52,152,219,0.5))
alt 200 OK
API-->>Validator: 200 {…}
Validator-->>Backend: { success: true, errors: [] }
else 400 Bad Request
API-->>Validator: 400 { error_msg, errors }
Validator-->>Backend: { success: false, errorMessage, errors }
end
Backend-->>Task: BackendValidateResult
Task->>Task: format errors & throw if !success
else not supported
Task-->>Task: throw unsupported error
end
Task-->>CLI: return success or exit with error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/backend-api7/e2e/validate.e2e-spec.ts (1)
10-13: Unused imports:syncEvents,createEvent,deleteEvent.These utilities are imported but not used in any test case.
♻️ Proposed fix
import { conditionalDescribe, generateHTTPSAgent, semverCondition, - syncEvents, - createEvent, - deleteEvent, } from './support/utils';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-api7/e2e/validate.e2e-spec.ts` around lines 10 - 13, The import list includes unused symbols syncEvents, createEvent, and deleteEvent; remove these three identifiers from the import statement (or, if they were intended to be used, add the appropriate test cases that call syncEvents, createEvent, and deleteEvent) so the test file no longer contains unused imports—update the import line where syncEvents, createEvent, deleteEvent are listed to only import the utilities actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/backend-api7/src/validator.ts`:
- Around line 113-118: The current mapping for config.ssls computes sslId as
ssl.id ?? ADCSDK.utils.generateId(ssl.snis?.[0] ?? '') which will call
generateId('') when snis is undefined or an empty array; change the logic in the
block that builds body.ssls (the map that sets sslId and calls
fromADC.transformSSL) to explicitly detect missing/empty ssl.snis and handle it
safely: either require/validate presence of snis (log a warning and skip or
throw), or derive a non-empty id using a safer seed (e.g., use a random/unique
value or serialize the ssl object) instead of ''. Ensure you reference
config.ssls, ssl.snis, ADCSDK.utils.generateId, sslId and fromADC.transformSSL
when making the change so no empty-string seed is passed to generateId.
---
Nitpick comments:
In `@libs/backend-api7/e2e/validate.e2e-spec.ts`:
- Around line 10-13: The import list includes unused symbols syncEvents,
createEvent, and deleteEvent; remove these three identifiers from the import
statement (or, if they were intended to be used, add the appropriate test cases
that call syncEvents, createEvent, and deleteEvent) so the test file no longer
contains unused imports—update the import line where syncEvents, createEvent,
deleteEvent are listed to only import the utilities actually used.
🪄 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: ccc2e6a2-01a4-4230-8913-b42d44c6bdbe
📒 Files selected for processing (8)
apps/cli/src/command/index.tsapps/cli/src/command/validate.command.tsapps/cli/src/tasks/index.tsapps/cli/src/tasks/validate.tslibs/backend-api7/e2e/validate.e2e-spec.tslibs/backend-api7/src/index.tslibs/backend-api7/src/validator.tslibs/sdk/src/backend/index.ts
|
|
jarvis9443
force-pushed
the
feat/validate-api7
branch
from
4a13509 to
b5030ed
Compare
|
Usage example demonstrating the batched error output with resource names. Config with two routes both missing required fields for services:
- name: qa-multi-err-svc
upstream:
scheme: http
nodes:
- host: httpbin.org
port: 80
weight: 100
routes:
- name: qa-multi-err-r1
uris:
- /qa-multi-err-1
plugins:
limit-count: {}
- name: qa-multi-err-r2
uris:
- /qa-multi-err-2
plugins:
limit-count: {}Output: All violations across all resources are reported in a single pass, each tagged with |
| validate?: ( | ||
| config: ADCSDK.Configuration, | ||
| ) => Promise<BackendValidateResult>; |
There was a problem hiding this comment.
This API signature implies (and in fact does): validation applies to all resources, even those that already exist remotely. I'm not sure if this is a good approach.
I suggest having it accept an Array<ADCSDK.Event>, so we can control at the higher level whether to perform a diff.
All we need to do is pass an empty ADCSDK.Configuration to ctx in the DiffResourceTask. This assumes that there are no resources on the remote side, so the CLI will emit events for the creation of all configurations. This is essentially consistent with the current implementation.
Later, in the API7 backend, we can convert and construct the data structures needed for API calls from these events.
| validate?: ( | |
| config: ADCSDK.Configuration, | |
| ) => Promise<BackendValidateResult>; | |
| validate?: ( | |
| events: Array<ADCSDK.Event>, | |
| ) => Observable<BackendValidateResult>; |
There was a problem hiding this comment.
Done in a98878b. Changed validate() to accept Array<ADCSDK.Event> and return Observable<BackendValidateResult>. The validate command now runs DiffResourceTask (with empty remote) before ValidateTask, so events flow through the same pipeline as sync.
jarvis9443
force-pushed
the
feat/validate-api7
branch
from
b5030ed to
01eae10
Compare
Per reviewer feedback, change validate() to accept Array<ADCSDK.Event> instead of ADCSDK.Configuration. This aligns with the existing sync() pattern and allows future diff-then-validate workflows. The validate command now runs DiffResourceTask (with empty remote config) before ValidateTask, generating CREATE events for all local resources. The Validator rebuilds the request body from flattened events using the same fromADC transformation logic as the Operator.
bzp2010
left a comment
bzp2010
left a comment
There was a problem hiding this comment.
Overall, this PR achieved its intended goals, but I feel the code quality is subpar; perhaps the agent isn't sufficiently familiar with this codebase.
I'll merge it first and then perform some minor refactoring.
| return semver.gte(version, MINIMUM_VALIDATE_VERSION); | ||
| } | ||
|
|
||
| public validate(events: Array<ADCSDK.Event>) { |
There was a problem hiding this comment.
| public validate(events: Array<ADCSDK.Event>) { | |
| public validate(events: Array<ADCSDK.Event>): Observable<BackendValidateResult> { |
| public async supportValidate(): Promise<boolean> { | ||
| const version = await this.version(); | ||
| return semver.gte(version, MINIMUM_VALIDATE_VERSION); | ||
| } |
There was a problem hiding this comment.
I’m wondering if we really need to implement this API. Perhaps it would be sufficient to simply call the validate function unconditionally in the CLI and perform the validation based on the version number within each backend’s validate function.
For backends that don’t currently support this, they can choose not to implement this API, since it’s optional in the Backend interface.
| const supported = await ctx.backend.supportValidate(); | ||
| if (!supported) { | ||
| const version = await ctx.backend.version(); | ||
| throw new Error( | ||
| `Validate is not supported by the current backend version (${version}). Please upgrade to a newer version.`, | ||
| ); | ||
| } | ||
|
|
||
| const result = await lastValueFrom(ctx.backend.validate!(ctx.diff)); |
There was a problem hiding this comment.
| const supported = await ctx.backend.supportValidate(); | |
| if (!supported) { | |
| const version = await ctx.backend.version(); | |
| throw new Error( | |
| `Validate is not supported by the current backend version (${version}). Please upgrade to a newer version.`, | |
| ); | |
| } | |
| const result = await lastValueFrom(ctx.backend.validate!(ctx.diff)); | |
| if (!ctx.backend.validate) | |
| throw new Error(`Validate is not supported by the current backend.`); | |
| const result = await lastValueFrom(ctx.backend.validate(ctx.diff)); |
ref: https://github.com/api7/adc/pull/432/changes#r3104831632
bzp2010
changed the title
4 participants
Summary
Add a
validatesubcommand that validates local ADC configuration against the server-side/apisix/admin/configs/validateAPI (dry-run, no side effects).Changes
SDK (
libs/sdk/)BackendValidationError,BackendValidateResultinterfacesvalidate?(events: Array<ADCSDK.Event>) => Observable<BackendValidateResult>andsupportValidate?()toBackendinterfaceBackend API7 (
libs/backend-api7/)Validatorclass that:FromADCtransformers (same asOperator)/apisix/admin/configs/validatewithgateway_group_idsupportValidate()with version gating (>= 3.9.10)validate()returningObservable<BackendValidateResult>CLI (
apps/cli/)ValidateTaskthat checks backend support, subscribes to validate observable, and formats errors with resource name/id/indexValidateCommandwith pipeline: InitBackend → LoadLocal → Lint → DiffResource (empty remote) → ValidateE2E Tests (
libs/backend-api7/e2e/)conditionalDescribe(semverCondition(gte, '3.9.10'))DifferV3.diff(config, {})to generate events from config, matching the CLI pipelineUsage