Skip to content

[KeyManager] End-to-end FFI error handling standardization#722

Merged
NilanjanDaw merged 8 commits intogoogle:mainfrom
NilanjanDaw:error-handling-pt2-rust-go
Mar 23, 2026
Merged

[KeyManager] End-to-end FFI error handling standardization#722
NilanjanDaw merged 8 commits intogoogle:mainfrom
NilanjanDaw:error-handling-pt2-rust-go

Conversation

@NilanjanDaw
Copy link
Collaborator

@NilanjanDaw NilanjanDaw commented Mar 18, 2026

This PR performs a comprehensive refactoring of the KeyManager component to utilize the standardized keymanager.Status Protobuf enum across all layers of the system. It replaces legacy negative i32 error codes and ad-hoc error enums with a unified, type-safe Status reporting mechanism that spans Rust, C, and Go.

Key Changes

  • Rust FFI & Core Updates:
    • Refactored KeyCustodyCore in both key_protection_service and workload_service to return the Status enum directly.
    • Simplified FFI implementation by utilizing the ffi_call and ffi_call_i32 helpers introduced in Part 1.
    • Updated internal logic to map crate-specific errors to the standardized Status proto.
  • C-Binding & Header Standardization:
    • Modified C function signatures in kps_key_custody_core.h and ws_key_custody_core.h to return Status instead of int32_t.
  • Go CGO & Service Layer Integration:
    • Updated CGO wrappers to interpret the new Status return codes and convert them into idiomatic Go errors using theToStatus() helper.
    • HTTP Status Mapping: Implemented httpStatusFromError in the Go server to automatically map standardized FFI errors to appropriate HTTP status codes (e.g., ERROR_NOT_FOUND404 Not Found, ERROR_INVALID_ARGUMENT400 Bad Request).
    • Removed redundant error aliases in types.go to maintain a single source of truth.
  • Test Alignment: Updated integration tests in Go and unit tests in Rust to verify the new error reporting behavior and
    ensure consistent error messages (e.g., "FFI error: ERROR_NOT_FOUND").

@NilanjanDaw NilanjanDaw force-pushed the error-handling-pt2-rust-go branch 7 times, most recently from 2345191 to 53e2346 Compare March 22, 2026 18:26
@NilanjanDaw NilanjanDaw self-assigned this Mar 22, 2026
Replaces negative `i32` error codes and ad-hoc error enums with the standardized `keymanager.Error` protobuf enum across the Rust backend, C-bindings, and Go HTTP server.

- Rust FFI explicitly returns `Error` enum.
- Updates `cbindgen.toml` configurations and C headers to export `Error`.
- Go CGO wrappers map `Error` to standard errors or `FFIError`.
- `server.go` translates these standardized FFI errors to standard HTTP status codes.
- Removes redundant `types.go` error aliases in the Go packages.
@NilanjanDaw NilanjanDaw requested review from pgonda March 23, 2026 15:21
pubkeyLen,
); rc != 0 {
return uuid.Nil, nil, fmt.Errorf("key_manager_generate_kem_keypair failed with code %d", rc)
); keymanager.Status(rc) != keymanager.Status_STATUS_SUCCESS {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gonna get old.

Should you add a .ok() or similar method for:

if !keymanager.Status(rc).ok() {
...
}

case errors.Is(err, keymanager.Status_STATUS_UNAUTHENTICATED):
return http.StatusUnauthorized
case errors.Is(err, keymanager.Status_STATUS_ALREADY_EXISTS):
return http.StatusConflict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. Once you update to gRPC service this gets much easier

Copy link
Collaborator

@pgonda pgonda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval but please have someone give the rust code a quick look

@NilanjanDaw NilanjanDaw force-pushed the error-handling-pt2-rust-go branch from 53e2346 to 7fea15e Compare March 23, 2026 16:46
@NilanjanDaw NilanjanDaw merged commit d92c8e8 into google:main Mar 23, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants