feat: rust backend apisix - #550
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds a Rust APISIX backend. It introduces resource models, conversion logic, fetching, synchronization, validation, live integration tests, mTLS fixtures, and Rust CI workflows. ChangesAPISIX backend integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend
participant Fetcher
participant Operator
participant Validator
participant APISIX
Client->>Backend: Request ping, dump, sync, or validate
Backend->>APISIX: Probe server and read version
Backend->>Fetcher: Dump configuration
Fetcher->>APISIX: Fetch APISIX resources
APISIX-->>Fetcher: Return resource data
Fetcher-->>Backend: Return ADC configuration
Backend->>Operator: Synchronize events
Operator->>APISIX: Apply ordered resource requests
APISIX-->>Operator: Return synchronization results
Backend->>Validator: Validate events
Validator->>APISIX: Submit validation payload
APISIX-->>Validator: Return validation results
Possibly related PRs
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)
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: 9
🧹 Nitpick comments (16)
.github/workflows/unit.yaml (1)
51-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant full recompile between
Build,Clippy, andRun unit tests.
cargo build,cargo clippy, andcargo testeach use different compiler flags, so each step recompiles the whole workspace from scratch even in an ideal setup. The standaloneBuildstep (lines 51-53) does not speed up the followingClippyorRun unit testssteps; consider dropping it (Clippy alone reports compile errors) and adding a caching action, tying into the same caching gap flagged on thee2e.yamlRust step.🤖 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 @.github/workflows/unit.yaml around lines 51 - 59, The Rust workflow redundantly runs a standalone build before Clippy and tests without reusing its artifacts. Remove the Build step from the workflow, retain Clippy and unit tests, and add the repository’s Rust dependency/build caching action or configuration consistently with the e2e workflow..github/workflows/e2e.yaml (1)
84-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo Rust build cache; step rebuilds the crate from scratch in every matrix job.
This step runs once per matrix entry (17 versions). Without a cache for
~/.cargoandtarget/, each entry does a fullcargo buildbefore running the ignored tests. Add a Rust caching step (for exampleSwatinem/rust-cache) to cut redundant compile time across the matrix.🤖 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 @.github/workflows/e2e.yaml around lines 84 - 91, Add a Rust dependency and build-artifact cache step before “Run Rust E2E tests” in the workflow, such as Swatinem/rust-cache, configured for the ./rust working directory and matrix jobs. Ensure it caches ~/.cargo and the Rust target directory so cargo test -p adc-backend-apisix can reuse compiled artifacts across runs.rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs (2)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
SERVER/TOKENconstants and client-construction boilerplate across five e2e test files. Each new e2e test file independently redeclares the sameSERVER/TOKENconstants and abackend()/client()helper that builds an identicalHttpClientConfig. Extract this into one shared test-support module (for exampletests/common/mod.rs) that the individual test files import.
rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs#L13-L19: replace the localSERVER/TOKENconstants andbackend()with the shared helper.rust/crates/adc-backend-apisix/tests/e2e_misc.rs#L15-L21: replace the localSERVER/TOKENconstants andbackend()with the shared helper.rust/crates/adc-backend-apisix/tests/e2e_ping.rs#L17-L27: replace the localTOKENconstant andbackend()with the shared helper, keeping its server/TLS parameters.rust/crates/adc-backend-apisix/tests/e2e_resource_service.rs#L13-L22: replace the localSERVER/TOKENconstants,client(), andbackend()with the shared helper.rust/crates/adc-backend-apisix/tests/e2e_resource_upstream.rs#L12-L18: replace the localSERVER/TOKENconstants andbackend()with the shared helper.🤖 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 `@rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs` around lines 13 - 19, Duplicate APISIX e2e client setup should be centralized in shared test support. Add a common module exposing the shared SERVER/TOKEN values and backend/client construction, then update rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs#L13-L19, rust/crates/adc-backend-apisix/tests/e2e_misc.rs#L15-L21, and rust/crates/adc-backend-apisix/tests/e2e_resource_upstream.rs#L12-L18 to use it; update rust/crates/adc-backend-apisix/tests/e2e_ping.rs#L17-L27 while preserving its server/TLS parameters, and rust/crates/adc-backend-apisix/tests/e2e_resource_service.rs#L13-L22 to replace its constants, client(), and backend() with the shared helper.
105-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a test-specific service ID seed.
generate_iddeterministically hashes its input, so this test ande2e_resource_service.rstarget the same service ID on the same APISIX server. If a run panics before cleanup, stale state can affect a later run. Use a unique service name consistently forservice_id, the service"name", and the child parent arguments.🤖 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 `@rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs` around lines 105 - 119, Update service_named_upstreams_lifecycle to use a test-specific seed when calling generate_id, and reuse the resulting unique service name consistently for service_id, the service payload’s "name", and each create_child parent argument instead of the shared "test" value.rust/crates/adc-backend-apisix/tests/e2e_apisix.rs (2)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider cleanup that survives a failed assertion.
sync_okand each test delete resources only on the success path. If an assertion between create and delete panics, the resources stay in the shared APISIX instance. The next run of the same test then starts from a dirty state, and the "should have been deleted" assertions can fail for the wrong reason.A scope guard that issues the deletes on drop would make each test self-cleaning.
🤖 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 `@rust/crates/adc-backend-apisix/tests/e2e_apisix.rs` around lines 59 - 64, Update sync_ok and the affected tests to register resource cleanup with a scope guard so APISIX deletes execute during unwinding as well as normal completion. Ensure all resources created between assertions are deleted on guard drop, while preserving the existing success-path deletion and “should have been deleted” assertions.
17-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe same test scaffolding is copied into five e2e files.
SERVER,TOKEN,client(),apisix_version()and thecreate/deleteevent helpers are redefined in each integration-test binary because there is no shared test-support module. A change to the admin key or the port needs five edits.
apisix_version()also carries a defect in every copy.std::env::var(...).ok().and_then(|v| Version::parse(&v).ok()).unwrap_or(Version::new(999, 999, 999))maps an unparsable value to the highest version. If CI setsBACKEND_APISIX_VERSIONto a non-semver string such as3.9ordev, every version gate opens and the tests run against a server that lacks the feature. Panic on a value that is present but unparsable, and keep the999.999.999fallback only for the unset case.Move the shared items into
rust/crates/adc-backend-apisix/tests/common/mod.rsand fixapisix_version()once there.
rust/crates/adc-backend-apisix/tests/e2e_apisix.rs#L17-L43: moveSERVER,TOKEN,client(),apisix_version(),create,deleteanddelete_childinto the shared module, and fix the unparsable-version fallback there.rust/crates/adc-backend-apisix/tests/e2e_operator.rs#L34-L39: replace the localSERVER,TOKENandclient()with the shared module.rust/crates/adc-backend-apisix/tests/e2e_resource_consumer.rs#L13-L26: replace the localSERVER,TOKEN,client()andapisix_version()with the shared module.rust/crates/adc-backend-apisix/tests/e2e_sync_and_dump.rs#L23-L33: replace the localSERVER,TOKEN,backend()client construction andapisix_version()with the shared module.rust/crates/adc-backend-apisix/tests/e2e_validate.rs#L17-L30: replace the localSERVER,TOKEN,apisix_version()and the client construction insidevalidator()with the shared module, and extract the repeated 3.17.0 gate into one helper.🤖 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 `@rust/crates/adc-backend-apisix/tests/e2e_apisix.rs` around lines 17 - 43, Centralize the duplicated APISIX e2e scaffolding in rust/crates/adc-backend-apisix/tests/common/mod.rs: move SERVER, TOKEN, client(), apisix_version(), create, delete, and delete_child from rust/crates/adc-backend-apisix/tests/e2e_apisix.rs#L17-L43, and make apisix_version() panic when BACKEND_APISIX_VERSION is present but unparsable while retaining the 999.999.999 fallback only when unset. Update rust/crates/adc-backend-apisix/tests/e2e_operator.rs#L34-L39, e2e_resource_consumer.rs#L13-L26, e2e_sync_and_dump.rs#L23-L33, and e2e_validate.rs#L17-L30 to use the shared module; replace each local client/version setup, including backend() and validator(), and extract the repeated 3.17.0 gate in e2e_validate.rs into one shared helper.libs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.key (1)
2-27: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffPrivate keys are committed although
generate-mtls.shcan produce them. The directory already holdslibs/backend-apisix/e2e/assets/apisix_conf/mtls/generate-mtls.sh, so the keys are reproducible at setup time. Committing them keeps long-lived private keys in git history and forces a new commit on every rotation. These are local test fixtures, so this is a posture concern, not an exploitable defect.
libs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.key#L2-L27: generate the CA key fromgenerate-mtls.shduring e2e setup and remove the committed file.libs/backend-apisix/e2e/assets/apisix_conf/mtls/client.key#L2-L27: generate the client key in the same setup step and remove the committed file.libs/backend-apisix/e2e/assets/apisix_conf/mtls/server.key#L2-L27: generate the server key in the same setup step and remove the committed file.If the compose stack needs the files before the tests start, keep them but add a short README note stating that they are throwaway fixtures and must never be reused outside e2e.
🤖 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-apisix/e2e/assets/apisix_conf/mtls/ca.key` around lines 2 - 27, Remove the committed private keys and update generate-mtls.sh/e2e setup to generate ca.key, client.key, and server.key before the compose stack or tests start; apply this to libs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.key lines 2-27, client.key lines 2-27, and server.key lines 2-27. If setup cannot generate them before stack startup, retain the files and add a README note identifying them as throwaway e2e fixtures that must not be reused outside e2e.rust/crates/adc-backend-apisix/tests/e2e_operator.rs (1)
49-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one
HttpClientinupdate_time.
HttpClient::executeaccepts a request built by another instance, so this is not a correctness issue. However,client()creates a new connection pool for each call. Reuse one client for bothrequestandexecute. Preserve the error context instead of converting transport errors toNone.🤖 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 `@rust/crates/adc-backend-apisix/tests/e2e_operator.rs` around lines 49 - 57, Update update_time to create a single HttpClient instance and reuse it for both request construction and execute. Preserve transport error context by avoiding the current immediate conversion of client/request/response errors to None, while retaining the existing unsuccessful-status and response parsing behavior.rust/crates/adc-backend-apisix/src/transformer.rs (2)
328-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the
__ADC_NAMElabel key into a shared constant.The literal
"__ADC_NAME"appears in three places: lines 328, 333, and 587. The related keyADC_UPSTREAM_SERVICE_ID_LABELis already a constant intyping.rs. Move this key next to it. A single constant prevents a typo in one of the three sites from breaking the write-then-read name recovery silently.♻️ Proposed refactor
In
rust/crates/adc-backend-apisix/src/typing.rs:pub const ADC_UPSTREAM_SERVICE_ID_LABEL: &str = "__ADC_UPSTREAM_SERVICE_ID"; +pub const ADC_NAME_LABEL: &str = "__ADC_NAME";In this file:
- let name = extract_name_label(&route.labels, "__ADC_NAME") + let name = extract_name_label(&route.labels, typing::ADC_NAME_LABEL) .unwrap_or_else(|| route.id.clone().unwrap_or_default()); let labels = route .labels .map(|mut labels| { - labels.remove("__ADC_NAME"); + labels.remove(typing::ADC_NAME_LABEL); labels })if inject_name { - labels.insert("__ADC_NAME".to_string(), LabelValue::Single(route.name)); + labels.insert(typing::ADC_NAME_LABEL.to_string(), LabelValue::Single(route.name)); }Also applies to: 586-588
🤖 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 `@rust/crates/adc-backend-apisix/src/transformer.rs` around lines 328 - 336, Define a shared constant for the "__ADC_NAME" label key alongside ADC_UPSTREAM_SERVICE_ID_LABEL in typing.rs, then replace all three literal occurrences in transformer.rs, including the uses in the route transformation and name-recovery logic, with that constant.
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the HTTP method string with serde.
adc::HttpMethodserde names match the manual table. Useserde_json::to_valueinhttp_method_to_stringto keep both directions aligned. Handle the impossible non-string result explicitly instead of returning an empty string.🤖 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 `@rust/crates/adc-backend-apisix/src/transformer.rs` around lines 42 - 45, Update http_method_to_string to derive the method string via serde_json::to_value, keeping serialization aligned with parse_http_method and adc::HttpMethod serde names. Explicitly handle the unexpected non-string serialized value rather than returning an empty string.rust/crates/adc-backend-apisix/src/operator.rs (1)
243-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelect the service body by request kind, not by a substring of the path.
Line 247 tests
path.contains("/upstreams/")to decide which of the two bodies to build. The path is an implementation detail produced two functions earlier. A future change to the upstream path, or a service id that itself contains the substring, silently selects the wrong body.Carry the intent explicitly. Let
build_requestsemit a small enum or a boolean alongside each path, and pass that torequest_body.🤖 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 `@rust/crates/adc-backend-apisix/src/operator.rs` around lines 243 - 255, Update the request-building flow so `build_requests` carries an explicit request-kind flag or enum alongside each path and passes it into `request_body`. In the `ResourceType::Service` branch, use that value to choose between `wire_upstream` and `wire_service` instead of checking `path.contains("/upstreams/")`; preserve the existing missing-default-upstream error.rust/crates/adc-backend-apisix/src/fetcher.rs (1)
218-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the routes dropped for a missing owning service.
Lines 224-229 and 237-242 drop a route or a stream route whose
service_idis absent from this dump, with no record. The comment states this should not normally happen. If it does happen, the dumped configuration omits those routes, and a later diff against that dump plans a delete for each of them on the server. A warning log makes the cause visible in that case.🤖 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 `@rust/crates/adc-backend-apisix/src/fetcher.rs` around lines 218 - 247, Update the route-bucketing logic in the `routes_by_service` and `stream_routes_by_service` loops to emit a warning whenever a route or stream route has a `service_id` not present in `services`, immediately before dropping it. Include enough route identity and owning service information to diagnose the omitted configuration while preserving the existing skip behavior.rust/crates/adc-backend-apisix/src/lib.rs (1)
18-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating the
testsmodule behind a feature and hiding it from docs.
pub mod testscompiles into every build, including release builds of downstream consumers, and it appears in published documentation. Integration tests intests/link against the crate as an external consumer, so#[cfg(test)]is not an option. Atest-utilsfeature plus#[doc(hidden)]keeps the surface out of docs and out of normal builds while the crate's own test targets enable the feature.♻️ Proposed change
+#[cfg(feature = "test-utils")] +#[doc(hidden)] /// Internal building blocks, exposed only for tests — see the crate-level /// doc comment. Not part of the supported API. pub mod tests {Add
test-utils = []to[features]andadc-backend-apisix = { path = ".", features = ["test-utils"] }under[dev-dependencies].🤖 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 `@rust/crates/adc-backend-apisix/src/lib.rs` around lines 18 - 31, Gate the public `tests` module in `adc-backend-apisix` with a `test-utils` feature and mark it with `#[doc(hidden)]`. Add the empty `test-utils` feature to the crate’s feature definitions, and enable it on the self-referencing `adc-backend-apisix` dev-dependency so integration tests retain access while normal and release builds do not expose the module.rust/crates/adc-backend-core/src/client.rs (1)
66-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument or preserve
serverpath prefixes.
serveraccepts path-prefixed URLs, but APISIX callers pass root-anchored paths such as/apisix/admin/routes.Url::joindrops a prefix such as/gateway/. Preserve the prefix or document that path-prefixedserverURLs are unsupported.🤖 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 `@rust/crates/adc-backend-core/src/client.rs` around lines 66 - 71, Update Client::request to preserve any path prefix from base_url when joining root-anchored request paths such as /apisix/admin/routes, ensuring prefixed server URLs like /gateway/ remain intact; alternatively, explicitly document that such path-prefixed server URLs are unsupported.rust/crates/adc-backend-apisix/src/utils.rs (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the unsupported
ConsumerCredentialcase explicit.Current callers do not pass
ConsumerCredentialto this function. However, the branch returns the literal"consumers/%s/credentials", which produces an invalid path because Rust does not interpolate%s. ReturnOption<String>and useNoneforConsumerCredential, or accept the consumer ID and construct the nested path here.🤖 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 `@rust/crates/adc-backend-apisix/src/utils.rs` around lines 7 - 13, Update resource_type_to_api_name to make ConsumerCredential unsupported explicitly: change its return type to Option<String>, return None for that variant, and wrap the existing valid API names in Some while preserving the current PluginMetadata and pluralized-resource behavior.rust/crates/adc-backend-apisix/src/backend.rs (1)
68-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a valid APISIX page size for the routes probe.
Use
page=1&page_size=10;page_size=1is invalid. The version probe is cached and runs only once.pingdrops the response body, so ADC does not download the full JSON.🤖 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 `@rust/crates/adc-backend-apisix/src/backend.rs` around lines 68 - 72, Update the request path in Backend::ping to query the APISIX routes endpoint with the valid page=1&page_size=10 parameters instead of relying on the current request URL. Keep the existing send-and-discard response flow unchanged so the probe remains lightweight.
🤖 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 @.github/workflows/unit.yaml:
- Line 45: Update the checkout step using actions/checkout in the unit workflow
to set persist-credentials to false. Keep the existing pinned action version and
all other job behavior unchanged.
In `@rust/crates/adc-backend-apisix/src/fetcher.rs`:
- Around line 98-117: Update Fetcher::list_stream_routes to treat only HTTP 404
as an unsupported endpoint and return an empty list; pass all other responses
through require_success, preserving existing response parsing for successful
requests. Revise the method’s doc comment to state that only 404 is tolerated,
preventing authentication, authorization, and server errors from being
interpreted as no stream routes.
- Around line 124-134: Update list_consumers to use concurrent_map_until_err
instead of concurrent_map, passing an explicit concurrency limit of
CREDENTIAL_FETCH_CONCURRENCY. Define that constant near the top of the file with
the requested value of 16 and documentation, and return the helper’s Result
directly so scheduling stops after the first credential-fetch error.
- Around line 287-302: Update resolve_plugin_config_refs so matching plugin
configurations are cloned into a new plugin map, then extend it with the
existing route.plugins entries; preserve route entries when plugin names overlap
and assign the merged map back to route.plugins.
In `@rust/crates/adc-backend-apisix/src/operator.rs`:
- Around line 178-196: Update the Service branch in build_requests so Create
only adds the default upstream path when the service has an upstream, reusing
transform_service’s existing Option or checking new_value before inserting it.
Preserve current request ordering and behavior for services with a default
upstream, and avoid generating an upstream request when it is absent.
- Around line 216-224: Update the ConsumerGroup handling in the request-body
path so both the serialized wire body ID and each member’s group_id use
event.resource_id, matching main_path, instead of relying on
transform_consumer_group’s name-derived ID. Preserve the explicit
ConsumerGroup.id through transformation or override the transformed values
before to_request_body, and add a request-body test covering differing group
names and resource IDs.
- Around line 156-162: Update main_path to encode parent_id and resource_id as
URL path segments before constructing APISIX URLs, preventing user-provided
separators, query, fragment, and traversal values from altering resolution.
Apply the same path-segment encoding to the service upstream_path. Explicitly
reject "." and ".." values when the encoder leaves them unchanged.
- Around line 183-193: Update the EventType::Update handling in operator.rs to
reject absent or empty event.kind.diff() values for Service updates before
mutating paths, including before paths.pop(). Return an appropriate error
instead of proceeding with no request; preserve the existing path handling for
non-empty diffs.
In `@rust/crates/adc-backend-apisix/tests/transformer.rs`:
- Around line 147-166: Update parse_discovery_map_nodes to recognize bracketed
IPv6 node keys such as "[::1]:9000", extracting the full IPv6 address without
brackets and the explicit port while preserving scheme-based defaults for keys
without a port. Extend the upstream discovery map-node tests with a regression
case asserting the parsed host and port.
---
Nitpick comments:
In @.github/workflows/e2e.yaml:
- Around line 84-91: Add a Rust dependency and build-artifact cache step before
“Run Rust E2E tests” in the workflow, such as Swatinem/rust-cache, configured
for the ./rust working directory and matrix jobs. Ensure it caches ~/.cargo and
the Rust target directory so cargo test -p adc-backend-apisix can reuse compiled
artifacts across runs.
In @.github/workflows/unit.yaml:
- Around line 51-59: The Rust workflow redundantly runs a standalone build
before Clippy and tests without reusing its artifacts. Remove the Build step
from the workflow, retain Clippy and unit tests, and add the repository’s Rust
dependency/build caching action or configuration consistently with the e2e
workflow.
In `@libs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.key`:
- Around line 2-27: Remove the committed private keys and update
generate-mtls.sh/e2e setup to generate ca.key, client.key, and server.key before
the compose stack or tests start; apply this to
libs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.key lines 2-27, client.key
lines 2-27, and server.key lines 2-27. If setup cannot generate them before
stack startup, retain the files and add a README note identifying them as
throwaway e2e fixtures that must not be reused outside e2e.
In `@rust/crates/adc-backend-apisix/src/backend.rs`:
- Around line 68-72: Update the request path in Backend::ping to query the
APISIX routes endpoint with the valid page=1&page_size=10 parameters instead of
relying on the current request URL. Keep the existing send-and-discard response
flow unchanged so the probe remains lightweight.
In `@rust/crates/adc-backend-apisix/src/fetcher.rs`:
- Around line 218-247: Update the route-bucketing logic in the
`routes_by_service` and `stream_routes_by_service` loops to emit a warning
whenever a route or stream route has a `service_id` not present in `services`,
immediately before dropping it. Include enough route identity and owning service
information to diagnose the omitted configuration while preserving the existing
skip behavior.
In `@rust/crates/adc-backend-apisix/src/lib.rs`:
- Around line 18-31: Gate the public `tests` module in `adc-backend-apisix` with
a `test-utils` feature and mark it with `#[doc(hidden)]`. Add the empty
`test-utils` feature to the crate’s feature definitions, and enable it on the
self-referencing `adc-backend-apisix` dev-dependency so integration tests retain
access while normal and release builds do not expose the module.
In `@rust/crates/adc-backend-apisix/src/operator.rs`:
- Around line 243-255: Update the request-building flow so `build_requests`
carries an explicit request-kind flag or enum alongside each path and passes it
into `request_body`. In the `ResourceType::Service` branch, use that value to
choose between `wire_upstream` and `wire_service` instead of checking
`path.contains("/upstreams/")`; preserve the existing missing-default-upstream
error.
In `@rust/crates/adc-backend-apisix/src/transformer.rs`:
- Around line 328-336: Define a shared constant for the "__ADC_NAME" label key
alongside ADC_UPSTREAM_SERVICE_ID_LABEL in typing.rs, then replace all three
literal occurrences in transformer.rs, including the uses in the route
transformation and name-recovery logic, with that constant.
- Around line 42-45: Update http_method_to_string to derive the method string
via serde_json::to_value, keeping serialization aligned with parse_http_method
and adc::HttpMethod serde names. Explicitly handle the unexpected non-string
serialized value rather than returning an empty string.
In `@rust/crates/adc-backend-apisix/src/utils.rs`:
- Around line 7-13: Update resource_type_to_api_name to make ConsumerCredential
unsupported explicitly: change its return type to Option<String>, return None
for that variant, and wrap the existing valid API names in Some while preserving
the current PluginMetadata and pluralized-resource behavior.
In `@rust/crates/adc-backend-apisix/tests/e2e_apisix.rs`:
- Around line 59-64: Update sync_ok and the affected tests to register resource
cleanup with a scope guard so APISIX deletes execute during unwinding as well as
normal completion. Ensure all resources created between assertions are deleted
on guard drop, while preserving the existing success-path deletion and “should
have been deleted” assertions.
- Around line 17-43: Centralize the duplicated APISIX e2e scaffolding in
rust/crates/adc-backend-apisix/tests/common/mod.rs: move SERVER, TOKEN,
client(), apisix_version(), create, delete, and delete_child from
rust/crates/adc-backend-apisix/tests/e2e_apisix.rs#L17-L43, and make
apisix_version() panic when BACKEND_APISIX_VERSION is present but unparsable
while retaining the 999.999.999 fallback only when unset. Update
rust/crates/adc-backend-apisix/tests/e2e_operator.rs#L34-L39,
e2e_resource_consumer.rs#L13-L26, e2e_sync_and_dump.rs#L23-L33, and
e2e_validate.rs#L17-L30 to use the shared module; replace each local
client/version setup, including backend() and validator(), and extract the
repeated 3.17.0 gate in e2e_validate.rs into one shared helper.
In `@rust/crates/adc-backend-apisix/tests/e2e_operator.rs`:
- Around line 49-57: Update update_time to create a single HttpClient instance
and reuse it for both request construction and execute. Preserve transport error
context by avoiding the current immediate conversion of client/request/response
errors to None, while retaining the existing unsuccessful-status and response
parsing behavior.
In `@rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs`:
- Around line 13-19: Duplicate APISIX e2e client setup should be centralized in
shared test support. Add a common module exposing the shared SERVER/TOKEN values
and backend/client construction, then update
rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs#L13-L19,
rust/crates/adc-backend-apisix/tests/e2e_misc.rs#L15-L21, and
rust/crates/adc-backend-apisix/tests/e2e_resource_upstream.rs#L12-L18 to use it;
update rust/crates/adc-backend-apisix/tests/e2e_ping.rs#L17-L27 while preserving
its server/TLS parameters, and
rust/crates/adc-backend-apisix/tests/e2e_resource_service.rs#L13-L22 to replace
its constants, client(), and backend() with the shared helper.
- Around line 105-119: Update service_named_upstreams_lifecycle to use a
test-specific seed when calling generate_id, and reuse the resulting unique
service name consistently for service_id, the service payload’s "name", and each
create_child parent argument instead of the shared "test" value.
In `@rust/crates/adc-backend-core/src/client.rs`:
- Around line 66-71: Update Client::request to preserve any path prefix from
base_url when joining root-anchored request paths such as /apisix/admin/routes,
ensuring prefixed server URLs like /gateway/ remain intact; alternatively,
explicitly document that such path-prefixed server URLs are unsupported.
🪄 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: 678ded25-d44d-463d-aa3c-8533ce360fba
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/e2e.yaml.github/workflows/unit.yamllibs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.cerlibs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.csrlibs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.keylibs/backend-apisix/e2e/assets/apisix_conf/mtls/client.cerlibs/backend-apisix/e2e/assets/apisix_conf/mtls/client.csrlibs/backend-apisix/e2e/assets/apisix_conf/mtls/client.keylibs/backend-apisix/e2e/assets/apisix_conf/mtls/generate-mtls.shlibs/backend-apisix/e2e/assets/apisix_conf/mtls/server.cerlibs/backend-apisix/e2e/assets/apisix_conf/mtls/server.csrlibs/backend-apisix/e2e/assets/apisix_conf/mtls/server.keyrust/Cargo.tomlrust/crates/adc-backend-apisix/Cargo.tomlrust/crates/adc-backend-apisix/src/backend.rsrust/crates/adc-backend-apisix/src/fetcher.rsrust/crates/adc-backend-apisix/src/lib.rsrust/crates/adc-backend-apisix/src/operator.rsrust/crates/adc-backend-apisix/src/transformer.rsrust/crates/adc-backend-apisix/src/typing.rsrust/crates/adc-backend-apisix/src/utils.rsrust/crates/adc-backend-apisix/src/validator.rsrust/crates/adc-backend-apisix/tests/e2e_apisix.rsrust/crates/adc-backend-apisix/tests/e2e_misc.rsrust/crates/adc-backend-apisix/tests/e2e_operator.rsrust/crates/adc-backend-apisix/tests/e2e_ping.rsrust/crates/adc-backend-apisix/tests/e2e_resource_consumer.rsrust/crates/adc-backend-apisix/tests/e2e_resource_service.rsrust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rsrust/crates/adc-backend-apisix/tests/e2e_resource_upstream.rsrust/crates/adc-backend-apisix/tests/e2e_sync_and_dump.rsrust/crates/adc-backend-apisix/tests/e2e_validate.rsrust/crates/adc-backend-apisix/tests/transformer.rsrust/crates/adc-backend-core/Cargo.tomlrust/crates/adc-backend-core/src/client.rsrust/crates/adc-backend-core/src/concurrency.rsrust/crates/adc-backend-core/src/lib.rsrust/crates/adc-backend-core/src/retry.rsrust/crates/adc-backend-core/tests/concurrency.rsrust/crates/adc-differ/Cargo.tomlrust/crates/adc-mock-server/Cargo.tomlrust/crates/adc-sdk/Cargo.tomlrust/crates/adc-sdk/src/backend/mod.rsrust/crates/adc-sdk/src/utils.rsrust/crates/adc-sync-bench/Cargo.toml
💤 Files with no reviewable changes (1)
- libs/backend-apisix/e2e/assets/apisix_conf/mtls/ca.csr
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/crates/adc-backend-core/src/client.rs (1)
89-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject server URLs with a query or fragment.
A server URL such as
https://host/gateway?tenant=xpassesUrl::parse. This code then buildshttps://host/gateway?tenant=x/apisix/admin/routes. The request targets/gateway, not/gateway/apisix/admin/routes.Reject configured server URLs that contain a query or fragment before this concatenation. Add tests for both invalid forms.
Proposed fix
let base_url = Url::parse(&config.server).map_err(|e| { BackendError::Other(format!("invalid server URL {:?}: {e}", config.server).into()) })?; +if base_url.query().is_some() || base_url.fragment().is_some() { + return Err(BackendError::Other( + format!("server URL {:?} must not contain a query or fragment", config.server).into(), + )); +}🤖 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 `@rust/crates/adc-backend-core/src/client.rs` around lines 89 - 107, Update the client URL validation around request so configured base URLs containing a query or fragment are rejected before path concatenation, returning the existing BackendError form rather than constructing an incorrect request URL. Add coverage for both query-bearing and fragment-bearing server URLs while preserving valid path-prefix concatenation.
🤖 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 `@rust/crates/adc-backend-apisix/src/operator.rs`:
- Around line 191-194: Update the EventType::Delete branch in the service
handling within the event path-building logic to add the upstream deletion path
only when event.old_value indicates the service had an inline upstream. Leave
deletion of other service resources unchanged and avoid scheduling an APISIX
upstream request when old_value is absent or contains no upstream.
---
Outside diff comments:
In `@rust/crates/adc-backend-core/src/client.rs`:
- Around line 89-107: Update the client URL validation around request so
configured base URLs containing a query or fragment are rejected before path
concatenation, returning the existing BackendError form rather than constructing
an incorrect request URL. Add coverage for both query-bearing and
fragment-bearing server URLs while preserving valid path-prefix concatenation.
🪄 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: 483a80ed-fb65-42f6-a99c-c08bc650f3fd
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.github/workflows/e2e.yaml.github/workflows/unit.yamlrust/crates/adc-backend-apisix/Cargo.tomlrust/crates/adc-backend-apisix/src/backend.rsrust/crates/adc-backend-apisix/src/fetcher.rsrust/crates/adc-backend-apisix/src/lib.rsrust/crates/adc-backend-apisix/src/operator.rsrust/crates/adc-backend-apisix/src/transformer.rsrust/crates/adc-backend-apisix/src/typing.rsrust/crates/adc-backend-apisix/src/utils.rsrust/crates/adc-backend-apisix/tests/common/mod.rsrust/crates/adc-backend-apisix/tests/e2e_apisix.rsrust/crates/adc-backend-apisix/tests/e2e_misc.rsrust/crates/adc-backend-apisix/tests/e2e_operator.rsrust/crates/adc-backend-apisix/tests/e2e_ping.rsrust/crates/adc-backend-apisix/tests/e2e_resource_consumer.rsrust/crates/adc-backend-apisix/tests/e2e_resource_service.rsrust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rsrust/crates/adc-backend-apisix/tests/e2e_resource_upstream.rsrust/crates/adc-backend-apisix/tests/e2e_sync_and_dump.rsrust/crates/adc-backend-apisix/tests/e2e_validate.rsrust/crates/adc-backend-apisix/tests/transformer.rsrust/crates/adc-backend-core/Cargo.tomlrust/crates/adc-backend-core/src/client.rsrust/crates/adc-backend-core/src/lib.rsrust/crates/adc-backend-core/tests/http_client.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- .github/workflows/unit.yaml
- rust/crates/adc-backend-apisix/tests/e2e_resource_upstream.rs
- rust/crates/adc-backend-apisix/tests/e2e_ping.rs
- rust/crates/adc-backend-core/Cargo.toml
- rust/crates/adc-backend-apisix/tests/e2e_resource_service_upstream.rs
- rust/crates/adc-backend-apisix/tests/e2e_resource_consumer.rs
- rust/crates/adc-backend-apisix/src/backend.rs
- rust/crates/adc-backend-apisix/tests/e2e_validate.rs
- rust/crates/adc-backend-apisix/tests/e2e_sync_and_dump.rs
- rust/crates/adc-backend-apisix/tests/e2e_misc.rs
- rust/crates/adc-backend-apisix/src/lib.rs
- rust/crates/adc-backend-apisix/tests/transformer.rs
- .github/workflows/e2e.yaml
- rust/crates/adc-backend-apisix/src/typing.rs
- rust/crates/adc-backend-apisix/src/transformer.rs
1 participant
Description
Backend for APISIX.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests