feat(api): add private networks contracts#150
Conversation
Test & Lint Summary
Test statistics: 3 passed / 0 failed / 0 skipped. |
|
The latest Buf updates on your PR. Results from workflow buf-pr / buf (pull_request).
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for laying out the contract foundation. I found several contract-level issues that should be fixed before this becomes the stable API surface: missing org scoping for group membership lookups, missing provisioning state on access grants, and ambiguous proto3 presence semantics for repeated/map update fields.
| optional string target_host = 4; | ||
| // Numeric target ports. Each value must be in the range 1..65535. | ||
| repeated int32 target_ports = 5; | ||
| optional string intercept_host = 6; |
There was a problem hiding this comment.
[major] This update request cannot distinguish “field omitted” from “clear the list” for target_ports and intercept_ports. In proto3, repeated fields have no presence, so an update that only changes name will deserialize these as empty lists and looks identical to a request that intentionally clears the ports. Because private resources require explicit 1..65535 port lists with target/intercept cardinality preserved, this makes partial updates unsafe. Use an explicit presence wrapper/message for the port lists, or an update_mask, so implementations can tell when the client actually wants to replace these lists.
| string private_resource_id = 2; | ||
| PrivateResourceAccessPrincipalType principal_type = 3; | ||
| string principal_id = 4; | ||
| } |
There was a problem hiding this comment.
[major] PrivateResourceAccess is missing provisioning_state, even though the architecture defines each grant as materializing a Dial policy and tracking whether that OpenZiti policy is active/failed/removing. Without this field the read/list APIs cannot surface failed grant provisioning or reconciliation state, so callers will see a grant as present even when its backing Dial policy was not created. Add the same ProvisioningState field here that Network, TunnelCredential, and PrivateResource expose.
| message ListMemberGroupsRequest { | ||
| GroupMemberType member_type = 1; | ||
| string member_id = 2; | ||
| int32 page_size = 3; |
There was a problem hiding this comment.
[major] ListMemberGroups needs an organization_id (or equivalent org-scoping input). The linked architecture authorizes this RPC differently for self vs. other-identity lookups, and downstream consumers such as Networks need to resolve group memberships within the relevant organization. With only member_type and member_id, the service cannot enforce the “member on organization” check or disambiguate memberships for identities that can participate in multiple organizations. Please add the org scope to this request (and consider whether the batch entry/request should carry the same scope).
| repeated string role_attributes = 2; | ||
| // Tags attached to the OpenZiti identity. | ||
| map<string, string> tags = 5; | ||
| } |
There was a problem hiding this comment.
[major] Adding tags at field number 5 leaves a gap at numbers 3 and 4 without reserving or documenting them. Those numbers were previously used and are reserved in CreateRunnerIdentityResponse, not in this request message, so future contributors could accidentally reuse them here and create avoidable wire-format confusion. Either use the next available field number (3) for this additive request field, or explicitly reserved 3, 4; with names/comments explaining why they must remain unavailable.
| optional HostV1Config host_v1_config = 2; | ||
| optional InterceptV1Config intercept_v1_config = 3; | ||
| map<string, string> tags = 4; | ||
| } |
There was a problem hiding this comment.
[major] This update has the same presence problem as UpdatePrivateResourceRequest: map fields do not have presence in proto3. A caller updating only the configs will send an empty tags map, which is indistinguishable from intentionally clearing all tags. Because reconciliation relies on tags as the ownership marker, accidental tag replacement/clearing would break orphan detection and cleanup. Use an explicit patch/replace message or update mask for tags, or omit tags from this RPC if service updates should not mutate ownership tags.
3 participants
Summary
Closes #149
Out of scope
No service implementation, Gateway handlers, DB migrations, NATS deployment, OpenFGA changes, or generated downstream client updates are included.
Test & Lint Summary
buf lint— passed with no errors.buf breaking --against '.git#branch=origin/main'— passed with no breaking changes.git diff --check— passed with no whitespace errors.Test statistics: 3 passed / 0 failed / 0 skipped.
Linting passed with no errors.