feat: add ability to retrieve policy resources by id or name #1901
Conversation
update Makefile to show lint failures in all modules
| // string kas_name = 2; | ||
| // // Optional | ||
| // string kas_uri = 3; | ||
| oneof identifier { |
There was a problem hiding this comment.
I am not concerned about these breaking changes as we haven't released this new functionality yet.
| if req.Msg.GetId() != "" { //nolint:staticcheck // Id can still be used until removed | ||
| identifier = req.Msg.GetId() //nolint:staticcheck // Id can still be used until removed |
There was a problem hiding this comment.
I don't understand why we need this if it's only running in the platform. Is this related to ConnectRPC and legacy gRPC requests?
There was a problem hiding this comment.
No this is because I moved the fields to a oneof in the proto definition. So the id field would be removed in a later release.
The intention of the oneof was so a user could only set either x_id or fqn/name. But after syncing with @jakedoublev it might not be worth the extra effort to move to the oneof.
| @@ -86,10 +86,17 @@ message ListAttributesResponse { | |||
| } | |||
|
|
|||
| message GetAttributeRequest { | |||
| // Required | |||
| // Deprecated | |||
| string id = 1 [ | |||
There was a problem hiding this comment.
I'd like to discuss this breaking change and the pros/cons. An alternative could be allowing either id or fqn without oneof, then logically preferring id in the event both were provided.
| @@ -412,6 +428,14 @@ func TestGetAttributeValueRequest(t *testing.T) { | |||
| } | |||
| err = getValidator().Validate(req) | |||
| require.NoError(t, err) | |||
|
|
|||
| req = &attributes.GetAttributeValueRequest{ | |||
| Identifier: &attributes.GetAttributeValueRequest_ValueId{ | |||
There was a problem hiding this comment.
should we do an FQN test here across these proto test suites?
| case string: | ||
| av, err = c.Queries.GetAttributeValueById(ctx, i) | ||
| default: | ||
| // Hopefully this will never happen |
There was a problem hiding this comment.
If we keep a switch for the oneof / deprecation, can we defined a typed error for this then wrap it with the unknown type?
return nil, fmt.Errorf("type: %T, %w", ErrTypeIdentifierInvalid")
Proposed Changes
Allowing us to make Get request by policy resource names will open up the doors to using friendly names versus uuid in the cli. This will help with the new key mapping support in the cli.
Copilot Summary
This pull request includes several changes to the documentation and codebase to support new query parameters and mark certain fields as deprecated. The most important changes are grouped by theme below:
Documentation Updates:
docs/grpc/index.html: Added new fields (attribute_id,fqn,value_id,kas_id,name,uri,namespace_id) to multiple request types and marked theidfield as deprecated. [1] [2] [3] [4] [5] [6]docs/openapi/policy/attributes/attributes.swagger.json: Updated theidfield description to "Deprecated" and added new query parameters (valueId,fqn,attributeId). [1] [2]docs/openapi/policy/kasregistry/key_access_server_registry.swagger.json: Updated theidfield description to "Deprecated" and added new query parameters (kasId,name,uri).docs/openapi/policy/namespaces/namespaces.swagger.json: Updated theidfield description to "Deprecated" and added new query parameters (namespaceId,fqn).Code Updates:
protocol/go/policy/attributes/attributes.pb.gw.go: Added new filters forGetAttributeandGetAttributeValuerequests to handle new query parameters. [1] [2] [3] [4] [5] [6]protocol/go/policy/kasregistry/key_access_server_registry.pb.gw.go: Added new filters forGetKeyAccessServerrequests to handle new query parameters. [1] [2] [3]protocol/go/policy/namespaces/namespaces.pb.gw.go: Added new filters forGetNamespacerequests to handle new query parameters. [1] [2] [3]Checklist
Testing Instructions