feat(apisix): add validate command support for APISIX and standalone backends#434
feat(apisix): add validate command support for APISIX and standalone backends#434jarvis9443 wants to merge 4 commits intomainfrom
Conversation
Add server-side configuration validation for both APISIX and APISIX Standalone backends, reusing the same /apisix/admin/configs/validate endpoint. - Create shared Validator class in backend-apisix that handles transforming ADC events to APISIX native format and calling the validate API - backend-apisix-standalone imports and reuses the Validator with custom request config (baseURL + auth header) - Handle APISIX-specific transformService tuple return which produces separate upstream resources - Add e2e test suites for both backends
📝 WalkthroughWalkthroughIntroduces a Validator that builds APISIX validation requests from ADC events, exposes validate(events) on BackendAPISIX and BackendAPISIXStandalone (returning Observables), adds semver-gated E2E tests for validation scenarios, updates CI matrix versions, and adds a workspace dependency for the standalone backend. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant Backend as BackendAPISIX / Standalone
participant Validator as Validator
participant APISIX as APISIX Server
Test->>Backend: validate(events)
Backend->>Validator: new Validator({ client, eventSubject, requestConfig })
Backend->>Validator: validate(events)
Validator->>Validator: buildRequestBody(events)
Validator->>APISIX: POST /apisix/admin/configs/validate
APISIX-->>Validator: 200 OK or 400 Validation Error
Validator->>Validator: map errors to resource_name via nameIndex
Validator-->>Backend: BackendValidateResult(success, errors, errorMessage)
Backend->>Test: Observable<BackendValidateResult>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The configs/validate endpoint is not available in released APISIX versions yet (only on master). Skip validate e2e tests on APISIX versions below 3.15.0, and handle 404 responses gracefully with a user-friendly error message.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts (1)
12-17: Consider edge case:configToEventswith undefined/null input.The helper casts inputs directly without null checks. While the test suite controls all inputs, adding defensive handling would improve robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts` around lines 12 - 17, The helper configToEvents currently casts its input without null checks which can blow up if config is undefined/null; update the function (configToEvents) to first guard against null/undefined (e.g., if config == null return an empty Array<ADCSDK.Event>) and only call DifferV3.diff when config is present, preserving the current cast to ADCSDK.InternalConfiguration for the non-null path.
🤖 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-apisix-standalone/e2e/validate.e2e-spec.ts`:
- Around line 31-35: The test in validate.e2e-spec.ts fails because
backend.validate([]) calls a nonstandard /apisix/admin/configs/validate
endpoint; update the test or the backend.validate implementation to use the
correct APISIX schema validation endpoint (e.g.,
/apisix/admin/schema/validate/{resource}) or make the path configurable based on
APISIX version/image; specifically, change the call-site in the it block or
adjust the backend.validate method to resolve the endpoint dynamically (or mock
the validation call) so the E2E run uses the supported endpoint for the APISIX
instance under test.
In `@libs/backend-apisix/src/validator.ts`:
- Around line 110-121: The code mutates event.newValue by assigning id directly
(inside the ADCSDK.ResourceType.SERVICE case before calling
this.fromADC.transformService), which can produce unintended side effects; fix
it by creating a shallow copy of event.newValue (e.g., const svc = {
...(event.newValue as ADCSDK.Service), id: event.resourceId }) and pass that
copy to this.fromADC.transformService, then push the returned service/upstream
as before; apply the same pattern in the ROUTE, STREAM_ROUTE, and SSL branches
so you never modify event.newValue in-place.
- Around line 160-166: The pushed global rule is missing the required id
property and is using an unsafe cast; update the object in the
body.global_rules.push call to include id: event.resourceId and remove the "as
unknown as" cast so the object conforms to typing.GlobalRule (mirror how
SERVICE/ROUTE/STREAM_ROUTE/PLUGIN_METADATA set id); ensure
nameIndex.global_rules.push(event.resourceName) remains unchanged.
---
Nitpick comments:
In `@libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts`:
- Around line 12-17: The helper configToEvents currently casts its input without
null checks which can blow up if config is undefined/null; update the function
(configToEvents) to first guard against null/undefined (e.g., if config == null
return an empty Array<ADCSDK.Event>) and only call DifferV3.diff when config is
present, preserving the current cast to ADCSDK.InternalConfiguration for the
non-null path.
🪄 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: af799504-33c1-499d-8ee4-9c87356def22
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
libs/backend-apisix-standalone/e2e/validate.e2e-spec.tslibs/backend-apisix-standalone/package.jsonlibs/backend-apisix-standalone/src/index.tslibs/backend-apisix/e2e/validate.e2e-spec.tslibs/backend-apisix/src/index.tslibs/backend-apisix/src/validator.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/backend-apisix/src/validator.ts (1)
165-168:⚠️ Potential issue | 🔴 Critical
global_rulesentries still miss the requiredid.This branch is still building a non-conforming
typing.GlobalRuleand hiding it behindas unknown as. Addid: event.resourceIdand drop the cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix/src/validator.ts` around lines 165 - 168, The GLOBAL_RULE case is constructing a typing.GlobalRule without the required id and using an unsafe cast; update the ADCSDK.ResourceType.GLOBAL_RULE branch so the object pushed to body.global_rules includes id: event.resourceId and the plugins field set to { [event.resourceId]: event.newValue }, and remove the "as unknown as typing.GlobalRule" cast so a properly typed object is added (i.e., push { id: event.resourceId, plugins: { [event.resourceId]: event.newValue } }).
🧹 Nitpick comments (1)
libs/backend-apisix/e2e/validate.e2e-spec.ts (1)
17-18: The new 404 fallback still needs coverage somewhere.Because this suite is gated to
>= 3.15.0, the “validate not supported” branch can regress silently. Please add a small mocked test aroundValidator.validate()that asserts the thrown message on a 404.Based on learnings "E2E 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 `@libs/backend-apisix/e2e/validate.e2e-spec.ts` around lines 17 - 18, The suite is gated to >=3.15.0 so the new 404 fallback path in Validator.validate() can regress unnoticed; add a unit-style test that mocks/stubs Validator.validate() to simulate a 404 response and asserts the thrown/rejected error message matches the expected fallback text. Specifically, inside or alongside the existing conditionalDescribe block (or in a non-gated sibling test), create a small test that uses your test framework’s mocking (e.g., jest.spyOn or similar) to make Validator.validate() throw an error/object with status 404, then call the code path that invokes Validator.validate() and assert the error message is the fallback message; reference BackendAPISIX and Validator.validate() so the mock targets the right symbol and ensure the test runs regardless of semverCondition to protect the fallback behavior.
🤖 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-apisix/src/validator.ts`:
- Around line 46-49: The axios debug event currently forwards the raw response
(see subject.next publishing ADCSDK.BackendEventType.AXIOS_DEBUG with event: {
response: resp, ... }), which can leak auth headers and plugin payloads; before
calling this.subject.next in validate (and the similar block around lines
58-63), construct a sanitized payload that strips or masks sensitive fields from
resp (remove Authorization/X-API-KEY and other auth headers, redact
request.data/request.body and any plugin config fields, and keep only safe
primitives such as status, statusText, url/path, and optionally masked headers
and truncated timing info), then publish that sanitized object instead of the
raw resp. Ensure the masking logic is centralized or reused so both publish
sites use the same redact function.
---
Duplicate comments:
In `@libs/backend-apisix/src/validator.ts`:
- Around line 165-168: The GLOBAL_RULE case is constructing a typing.GlobalRule
without the required id and using an unsafe cast; update the
ADCSDK.ResourceType.GLOBAL_RULE branch so the object pushed to body.global_rules
includes id: event.resourceId and the plugins field set to { [event.resourceId]:
event.newValue }, and remove the "as unknown as typing.GlobalRule" cast so a
properly typed object is added (i.e., push { id: event.resourceId, plugins: {
[event.resourceId]: event.newValue } }).
---
Nitpick comments:
In `@libs/backend-apisix/e2e/validate.e2e-spec.ts`:
- Around line 17-18: The suite is gated to >=3.15.0 so the new 404 fallback path
in Validator.validate() can regress unnoticed; add a unit-style test that
mocks/stubs Validator.validate() to simulate a 404 response and asserts the
thrown/rejected error message matches the expected fallback text. Specifically,
inside or alongside the existing conditionalDescribe block (or in a non-gated
sibling test), create a small test that uses your test framework’s mocking
(e.g., jest.spyOn or similar) to make Validator.validate() throw an error/object
with status 404, then call the code path that invokes Validator.validate() and
assert the error message is the fallback message; reference BackendAPISIX and
Validator.validate() so the mock targets the right symbol and ensure the test
runs regardless of semverCondition to protect the fallback behavior.
🪄 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: 03a8bb10-160e-4b13-8dbc-a94a1a110d3d
📒 Files selected for processing (3)
libs/backend-apisix-standalone/e2e/validate.e2e-spec.tslibs/backend-apisix/e2e/validate.e2e-spec.tslibs/backend-apisix/src/validator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts
…p semver to 3.17.0 - Add missing id field to global_rules in validator (required by APISIX schema) - Add APISIX 3.15.0, 3.16.0, and dev to apisix backend CI matrix - Add APISIX 3.15.0, 3.16.0 to standalone CI matrix - Update validate test semver condition to gte 3.17.0 (validate endpoint not in any released version yet)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-apisix/src/validator.ts`:
- Around line 128-148: The code uses unsafe non-null assertions on
event.parentId when calling this.fromADC.transformRoute and transformStreamRoute
for ADCSDK.ResourceType.ROUTE and STREAM_ROUTE; add a defensive check that
event.parentId is defined before using it: if event.parentId is undefined, throw
or return a clear validation error that includes event.resourceName and
event.resourceId (or otherwise handle root-level routes if that is the intended
behavior), and only call this.fromADC.transformRoute(event.newValue as
ADCSDK.Route, event.parentId) / transformStreamRoute(...) after validating
parentId; also consider surfacing this check from validator.validate() or
documenting the required invariant.
🪄 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: a5c8a7aa-ff47-48af-8310-ac6b06df7f79
📒 Files selected for processing (4)
.github/workflows/e2e.yamllibs/backend-apisix-standalone/e2e/validate.e2e-spec.tslibs/backend-apisix/e2e/validate.e2e-spec.tslibs/backend-apisix/src/validator.ts
✅ Files skipped from review due to trivial changes (1)
- libs/backend-apisix/e2e/validate.e2e-spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts
QA: #434 —
|
| # | Scenario | apisix | apisix-standalone | Notes |
|---|---|---|---|---|
| 01 | empty config | ✅ success | ✅ success | no events sent, returns immediately |
| 02 | valid service+route+consumer | ✅ success | ✅ success | |
| 03 | invalid plugin (limit-count count=-1, time_window=-10) |
✅ correctly rejected | ✅ correctly rejected | error tagged with route name |
| 04 | invalid route (uris: [123]) |
✅ correctly rejected | ✅ correctly rejected | error tagged with route name |
| 05 | multi-error (3 bad routes) | ✅ all 3 returned, named | ✅ all 3 returned, named | confirms multi-error aggregation works |
| 06 | mixed (service + route + consumer + global_rule) | ✅ success | ✅ success | |
| 07 | dry-run check (validate then dump) |
✅ no resources persisted | ✅ no resources persisted | confirms validate is side-effect free |
| 08 | unsupported version (3.16.0) | ✅ user-friendly error: “Validate is not supported by the current APISIX version. Please upgrade to a newer version.” |
Sample error output (scenario 05, both backends)
Error: Configuration validation failed:
Configuration validation failed
- [routes, name="bad-route-a"]: invalid configuration: property "uris" validation failed: failed to validate item 1: wrong type: expected string, got number
- [routes, name="bad-route-b"]: failed to check the configuration of plugin limit-count err: property "count" validation failed: value should match only one schema, but matches none
- [routes, name="bad-route-c"]: invalid configuration: property "uris" validation failed: failed to validate item 1: wrong type: expected string, got number
resource_name mapping (nameIndex) works; users get a meaningful pointer back to the offending resource in their adc.yaml rather than just index 0.
Notable finding — version gating is overly conservative for standalone mode
The PR description and e2e gate (semverCondition(gte, '3.17.0')) say the validate endpoint exists only on master. That is true for etcd / admin mode:
apache/apisix:3.16.0-debianetcd mode →POST /apisix/admin/configs/validatereturns 404{"error_msg":"Unsupported resource type: configs"}→ ADC throws the expected user-friendly upgrade message ✅
But it is not true for standalone mode:
apache/apisix:3.16.0-ubuntustandalone mode →POST /apisix/admin/configs/validatereturns 200 and actually validates (rejectinguris: [123]correctly).
So the standalone backend validate() is functional starting from APISIX 3.16.0, not 3.17.0. The semver gate in libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts could be relaxed (or split per backend) so 3.16.0 standalone runs the suite in CI.
Test environment caveat
The committed e2e standalone compose (libs/backend-apisix-standalone/e2e/assets/docker-compose.yaml) declares restart: always and points to etcd:2379 but doesn’t define an etcd service. When the host’s docker DNS returns temporary failure in name resolution (instead of NXDOMAIN), init_etcd exits 1 and the container restart-loops. CI happens to work because GitHub runners’ DNS gives a fast NXDOMAIN that doesn’t trip the retry path. For local QA I had to add a dummy etcd service to the compose. Consider adding etcd to the compose or removing the deployment.etcd block from the conf to make local runs deterministic.
Conclusion
Both backends’ validate() work as designed: the shared Validator correctly builds the request body, surfaces multi-error responses with resource names, gracefully degrades with a clear upgrade message on 404, and has no observable side effects. LGTM.
The validate endpoint is available in standalone mode (config_provider: yaml) starting from APISIX 3.16.0, while etcd mode requires 3.17.0+. Split the version gating accordingly.
|
Thanks for the thorough QA! Addressed the version gate finding — relaxed standalone e2e to Re the docker-compose etcd issue — that's pre-existing in the standalone test setup, not introduced here. Happy to fix it in a follow-up if needed. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts (3)
24-31: Consider per-test backend setup to reduce hidden order coupling.Using one shared backend instance via
beforeAllis efficient, but it can couple tests if backend state/caching behavior changes.beforeEach(or explicit reset) would keep cases fully independent.Based on learnings: Applies to **/*.{test,spec,e2e,integration}.{js,ts,go} : Avoid explicit dependencies between tests and hidden execution order assumptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts` around lines 24 - 31, The tests create a single shared BackendAPISIXStandalone instance in beforeAll which can introduce hidden order coupling; change the setup to create a fresh backend per test (use beforeEach to instantiate BackendAPISIXStandalone into the backend variable with the same options including cacheKey and defaultBackendOptions) or add an explicit reset method call on BackendAPISIXStandalone between tests to clear state/caches; ensure references to BackendAPISIXStandalone, backend, cacheKey and defaultBackendOptions are updated so each spec runs with an isolated backend instance.
109-115: Strengthen invalid-case assertions to verify resource-name mapping and aggregation.These checks currently assert only count/type. Since this suite validates user-facing error mapping, assert expected
resource_namevalues for single-error and multi-error cases to lock in the intended behavior.Proposed assertion upgrade
expect(result.success).toBe(false); - expect(result.errors.length).toBeGreaterThan(0); - expect(result.errors[0].resource_type).toBe('routes'); + expect(result.errors).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + resource_type: 'routes', + resource_name: 'validate-bad-plugin-route', + }), + ]), + ); @@ expect(result.success).toBe(false); expect(result.errors.length).toBeGreaterThanOrEqual(2); + expect(result.errors.map((e) => e.resource_name)).toEqual( + expect.arrayContaining([ + 'validate-multi-err-route1', + 'validate-multi-err-route2', + ]), + );Also applies to: 172-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts` around lines 109 - 115, The test only asserts result.success, errors.length and errors[0].resource_type but must also verify that the validator maps and aggregates resource names correctly; update the assertions after calling backend.validate(configToEvents(config)) (variable result) to check expected result.errors[0].resource_name for the single-error case and, for the multi-error case (the other test around the same file), assert that the set/array of result.errors.resource_name values contains the expected resource names and that aggregation behavior (e.g., combined entries for the same resource) matches the expected mapping; use the existing result and result.errors references and add assertions that explicitly compare resource_name values to the expected strings.
14-18: Consider extractingconfigToEventshelper to a shared utility or documenting why theConfiguration→InternalConfigurationcast is safe.The cast at line 16 is used identically across all validate E2E tests (
backend-api7,backend-apisix, andbackend-apisix-standalone), suggesting it's a deliberate pattern. However, the type cast silently dropsconsumer_groups(present inConfiguration, absent inInternalConfiguration). Since the current test configs don't exerciseconsumer_groups, the impact is minimal, but making this pattern explicit (via a documented utility or inline comment) would improve clarity and prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts` around lines 14 - 18, Extract the configToEvents logic into a shared test utility (e.g., tests/utils/configToEvents) or add an inline comment above the configToEvents function explaining why the cast from ADCSDK.Configuration to ADCSDK.InternalConfiguration is safe in these E2E tests; specifically mention that consumer_groups is dropped by the cast, that current test fixtures don't include consumer_groups, and that the helper centralizes this behavior for reuse across backend-api7, backend-apisix, and backend-apisix-standalone. Update references to call the shared configToEvents helper (or keep the local function but include the explanatory comment) and ensure the helper uses DifferV3.diff with the same config as ADCSDK.InternalConfiguration to preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts`:
- Around line 24-31: The tests create a single shared BackendAPISIXStandalone
instance in beforeAll which can introduce hidden order coupling; change the
setup to create a fresh backend per test (use beforeEach to instantiate
BackendAPISIXStandalone into the backend variable with the same options
including cacheKey and defaultBackendOptions) or add an explicit reset method
call on BackendAPISIXStandalone between tests to clear state/caches; ensure
references to BackendAPISIXStandalone, backend, cacheKey and
defaultBackendOptions are updated so each spec runs with an isolated backend
instance.
- Around line 109-115: The test only asserts result.success, errors.length and
errors[0].resource_type but must also verify that the validator maps and
aggregates resource names correctly; update the assertions after calling
backend.validate(configToEvents(config)) (variable result) to check expected
result.errors[0].resource_name for the single-error case and, for the
multi-error case (the other test around the same file), assert that the
set/array of result.errors.resource_name values contains the expected resource
names and that aggregation behavior (e.g., combined entries for the same
resource) matches the expected mapping; use the existing result and
result.errors references and add assertions that explicitly compare
resource_name values to the expected strings.
- Around line 14-18: Extract the configToEvents logic into a shared test utility
(e.g., tests/utils/configToEvents) or add an inline comment above the
configToEvents function explaining why the cast from ADCSDK.Configuration to
ADCSDK.InternalConfiguration is safe in these E2E tests; specifically mention
that consumer_groups is dropped by the cast, that current test fixtures don't
include consumer_groups, and that the helper centralizes this behavior for reuse
across backend-api7, backend-apisix, and backend-apisix-standalone. Update
references to call the shared configToEvents helper (or keep the local function
but include the explanatory comment) and ensure the helper uses DifferV3.diff
with the same config as ADCSDK.InternalConfiguration to preserve existing
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e4d21a9-e69a-4bfd-b81e-94dc073dadd0
📒 Files selected for processing (1)
libs/backend-apisix-standalone/e2e/validate.e2e-spec.ts
Add
validate()support to both APISIX and APISIX Standalone backends, sharing a commonValidatorclass inbackend-apisix.Changes
Validatorclass inlibs/backend-apisix/src/validator.ts/apisix/admin/configs/validateand processes 200/400 responsesvalidate()method toBackendAPISIXvalidate()method toBackendAPISIXStandalone(importsValidatorfrom backend-apisix)semverCondition(gte, '3.17.0'))Notes
/apisix/admin/configs/validateendpoint exists on APISIX master but not in any released version (3.2.2–3.16.0). Tests are version-gated and will auto-enable when 3.17.0 releases.Validatorvia@api7/adc-backend-apisixworkspace dependency.Summary by CodeRabbit
New Features
Tests
Chores