feat(api): add DNSBL resource lifecycle - #124
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 No DnsblEntry schema in OpenAPI components
components.schemas defines Route but no DnsblEntry, so the DNSBL body $refs have no target. Completing the DNSBL contract requires adding a DnsblEntry schema (address, code, reason, source, ttl_seconds, optional prefix_len).
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| put: | ||
| summary: Create or conditionally replace one DNSBL entry | ||
| security: [{ AdminToken: [] }] | ||
| parameters: | ||
| - name: If-Match | ||
| in: header | ||
| required: false | ||
| schema: { type: string } | ||
| responses: | ||
| '200': { description: DNSBL entry replaced } | ||
| '201': { description: DNSBL entry created } | ||
| '400': { $ref: '#/components/responses/BadRequest' } | ||
| '401': { $ref: '#/components/responses/Unauthorized' } | ||
| '403': { $ref: '#/components/responses/Forbidden' } | ||
| '412': { description: ETag does not match } | ||
| '428': { description: If-Match required for an existing entry } |
There was a problem hiding this comment.
🟡 DNSBL PUT declares no request body
The put operation for /api/dnsbl/{address} declares no requestBody, unlike the route put at openapi.yaml. The handler requires a JSON DnsblEntry body, so a client generated from this contract sends no body and every create/replace fails.
| put: | |
| summary: Create or conditionally replace one DNSBL entry | |
| security: [{ AdminToken: [] }] | |
| parameters: | |
| - name: If-Match | |
| in: header | |
| required: false | |
| schema: { type: string } | |
| responses: | |
| '200': { description: DNSBL entry replaced } | |
| '201': { description: DNSBL entry created } | |
| '400': { $ref: '#/components/responses/BadRequest' } | |
| '401': { $ref: '#/components/responses/Unauthorized' } | |
| '403': { $ref: '#/components/responses/Forbidden' } | |
| '412': { description: ETag does not match } | |
| '428': { description: If-Match required for an existing entry } | |
| put: | |
| summary: Create or conditionally replace one DNSBL entry | |
| security: [{ AdminToken: [] }] | |
| parameters: | |
| - name: If-Match | |
| in: header | |
| required: false | |
| schema: { type: string } | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: { $ref: '#/components/schemas/DnsblEntry' } | |
| responses: | |
| '200': { description: DNSBL entry replaced } | |
| '201': { description: DNSBL entry created } | |
| '400': { $ref: '#/components/responses/BadRequest' } | |
| '401': { $ref: '#/components/responses/Unauthorized' } | |
| '403': { $ref: '#/components/responses/Forbidden' } | |
| '412': { description: ETag does not match } | |
| '428': { description: If-Match required for an existing entry } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| '200': | ||
| description: DNSBL entry | ||
| headers: | ||
| ETag: { schema: { type: string } } |
There was a problem hiding this comment.
🟡 DNSBL GET response omits its JSON body
The 200 response for GET /api/dnsbl/{address} documents only the ETag header and no content, unlike the route GET at openapi.yaml. The handler returns a JSON entry body, so the contract understates the actual response shape.
| '200': | |
| description: DNSBL entry | |
| headers: | |
| ETag: { schema: { type: string } } | |
| '200': | |
| description: DNSBL entry | |
| headers: | |
| ETag: { schema: { type: string } } | |
| content: | |
| application/json: | |
| schema: { $ref: '#/components/schemas/DnsblEntry' } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| async fn get_dnsbl( | ||
| State(state): State<AppState>, | ||
| PathParam(address): PathParam<IpAddr>, | ||
| ) -> Response { | ||
| let data = state.inner.read().await; | ||
| let Some(entry) = data.dnsbl.iter().find(|entry| entry.address == address) else { | ||
| return error(StatusCode::NOT_FOUND, "DNSBL entry not found"); | ||
| }; | ||
| dnsbl_response(StatusCode::OK, entry) | ||
| } | ||
|
|
||
| async fn replace_dnsbl( | ||
| State(state): State<AppState>, | ||
| PathParam(address): PathParam<IpAddr>, | ||
| headers: HeaderMap, | ||
| Json(entry): Json<DnsblEntry>, | ||
| ) -> Response { | ||
| if let Some(response) = management_write_denied(&state, &headers) { | ||
| return response; | ||
| } | ||
| if entry.address != address { | ||
| return error( | ||
| StatusCode::BAD_REQUEST, | ||
| "path address must match body address", | ||
| ); | ||
| } | ||
| if let Err(message) = validate_dnsbl(&entry) { | ||
| return error(StatusCode::BAD_REQUEST, message); | ||
| } | ||
| let expected = headers | ||
| .get(header::IF_MATCH) | ||
| .and_then(|value| value.to_str().ok()); | ||
| let actor = audit_actor(&state, &headers); | ||
| match state | ||
| .try_mutate_and_persist(|data| { | ||
| let existing = data.dnsbl.iter().find(|item| item.address == address); | ||
| let existed = existing.is_some(); | ||
| match existing { | ||
| None if expected.is_some() => Err(( | ||
| StatusCode::PRECONDITION_FAILED, | ||
| "If-Match requires an existing DNSBL entry".to_string(), | ||
| )), | ||
| Some(_) if expected.is_none() => Err(( | ||
| StatusCode::PRECONDITION_REQUIRED, | ||
| "If-Match is required when replacing an existing DNSBL entry".to_string(), | ||
| )), | ||
| Some(current) if !if_match_satisfied(expected, &dnsbl_etag(current)) => Err(( | ||
| StatusCode::PRECONDITION_FAILED, | ||
| "DNSBL entry changed; GET the latest representation and retry".to_string(), | ||
| )), | ||
| _ => { | ||
| let status = if existed { | ||
| StatusCode::OK | ||
| } else { | ||
| StatusCode::CREATED | ||
| }; | ||
| let saved = upsert_dnsbl(&mut data.dnsbl, entry.clone()); | ||
| record_successful_audit_log( | ||
| data, | ||
| actor, | ||
| "replace_dnsbl", | ||
| "dnsbl_entry", | ||
| saved.address.to_string(), | ||
| ); | ||
| Ok((status, saved)) | ||
| } | ||
| } | ||
| }) | ||
| .await | ||
| { | ||
| Ok(Ok((status, saved))) => dnsbl_response(status, &saved), | ||
| Ok(Err((status, message))) => error(status, message), | ||
| Err(message) => error(StatusCode::INTERNAL_SERVER_ERROR, message), | ||
| } | ||
| } | ||
|
|
||
| async fn delete_dnsbl( | ||
| State(state): State<AppState>, | ||
| PathParam(address): PathParam<IpAddr>, | ||
| headers: HeaderMap, | ||
| ) -> Response { | ||
| if let Some(response) = management_write_denied(&state, &headers) { | ||
| return response; | ||
| } | ||
| let expected = headers | ||
| .get(header::IF_MATCH) | ||
| .and_then(|value| value.to_str().ok()); | ||
| if expected.is_none() { | ||
| return error( | ||
| StatusCode::PRECONDITION_REQUIRED, | ||
| "If-Match is required when deleting a DNSBL entry", | ||
| ); | ||
| } | ||
| let actor = audit_actor(&state, &headers); | ||
| match state | ||
| .try_mutate_and_persist(|data| { | ||
| let Some(index) = data.dnsbl.iter().position(|entry| entry.address == address) else { | ||
| return Err(( | ||
| StatusCode::PRECONDITION_FAILED, | ||
| "If-Match requires an existing DNSBL entry".to_string(), | ||
| )); | ||
| }; | ||
| if !if_match_satisfied(expected, &dnsbl_etag(&data.dnsbl[index])) { | ||
| return Err(( | ||
| StatusCode::PRECONDITION_FAILED, | ||
| "DNSBL entry changed; GET the latest representation and retry".to_string(), | ||
| )); | ||
| } | ||
| data.dnsbl.remove(index); | ||
| record_successful_audit_log( | ||
| data, | ||
| actor, | ||
| "delete_dnsbl", | ||
| "dnsbl_entry", | ||
| address.to_string(), | ||
| ); | ||
| Ok(()) | ||
| }) | ||
| .await | ||
| { | ||
| Ok(Ok(())) => StatusCode::NO_CONTENT.into_response(), | ||
| Ok(Err((status, message))) => error(status, message), | ||
| Err(message) => error(StatusCode::INTERNAL_SERVER_ERROR, message), | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: DNSBL item handlers mirror route lifecycle
get_dnsbl, replace_dnsbl, and delete_dnsbl replicate the route lifecycle: write gate, path/body identity check, validation, If-Match precondition arms, transactional persist-with-rollback, and audit logging. ETag helpers match route_etag/route_response. Address equality uses parsed IpAddr, so IPv6 canonicalization is handled. No logic divergence found.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Stack
feat/operator-resource-api/ PR feat(api): complete gateway route lifecycle #112 so it reuses the reviewed route lifecycle transaction and precondition contractValidation
cargo fmt --checkcargo test --locked --workspace(all library/workspace suites passed; the binary SIGTERM test passed on immediate focused rerun after one timing failure)cargo test --locked dnsbl_item_api_enforces_identity_etag_rbac_and_audits_deletecargo clippy --locked --workspace --all-targets -- -D warningsgit diff --check