-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: initial OpenFGA v2beta1
protobuf API development
#127
base: main
Are you sure you want to change the base?
Conversation
|
|
||
import "google/protobuf/struct.proto"; | ||
|
||
message RelationshipTuple { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WHat do you think about naming them RelationshipTuple
and RelationshipTupleWithCondition
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So invert them?
I think the API more unanymously operates on a RelationshipTuple which can optionally have a condition. For example the WriteRelationshipTuples
API, the ReadRelationshipTuples
API, etc.. The exception are those APIs that do not need a condition in the tuple representation whatsoever. So I think I lean toward leaving as is.
// restriction allows it. | ||
message TypeRestriction { | ||
oneof type_reference { | ||
UnconditionedObjectTypeRestriction unconditioned_object_type_reference = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the separation? also all can be conditioned not just object type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the API more strongly typed. A type restriction can reference a direct object type with no condition or one explicitly with a condition. The later requires the condition to be defined, the former does not. To uniquely distinguish these cases we make the protobuf API strongly typed to represent the difference.
|
||
rpc WriteModel(WriteModelRequest) returns (WriteModelResponse); | ||
rpc ReadModel(ReadModelRequest) returns (ReadModelResponse); | ||
rpc ListModels(ListModelsRequest) returns (ListModelsResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReadChanges?
Read?
BatchCheck?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More to come 😄 . Just started with some.
string model_id = 2; | ||
openfga.v2beta1.Subject subject = 3; | ||
string relation = 4; | ||
string object_type = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, maybe:
object: {
type: ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would imply another message definition of the form
message ObjectWithoutId {
string type = 1;
}
That's a bit overloaded and adds message level indirection for little to no gain. string object_type
points to the Object.Type
field. Accepting the string is more straightforward.
Description
Start the development branch for the
v2beta1
protobuf API definitions for OpenFGA. This is to get things started with that new API development effort as we go into the new year.As we work through these changes in the scope of this PR, lets try to focus on a few guiding principles:
Well typed and strongly typed protobuf API contract - prefer objects over string encoded entities where you can (e.g.
Object
instead ofstring object
)Separation of input types vs output types and a clear definition of required vs non-required fields within them- this really helps other development in our tooling such as in the SDKs etc..
Think about the gRPC experience, not just the HTTP mapping - the protobuf API is the definitive source of truth. The HTTP API is derived off of it. When in doubt, the protobuf API should take precedence.
Give a shit about naming things - if you've had a beef with the API names, speak up and fight for the cause 😄
Encourage strong alignment with
buf lint
lint style guidelines so that we have a standard - we shouldn't have to make exceptions regularly for our protobuf API linting behavior. Stick to the community standard of usingbuf lint
and the default rules for it.Consider Google API Improvement Proposals (AIPs) as a good starting point for inspiration behind certain API changes and design - we don't have to copy Google API guidelines, but they've clearly given a lot of API experience some thought and since they are the primary driving force behind gRPC and RPC APIs in general, their guidelines are a good reference point for ideas of achieving certain design goals. In particular, consider the AIPs around standard methods.
A few notable changes to get started:
The new API has been designed with an emphasis on a strongly typed protobuf API. For example, instead of
string object
fields we have explicitObject object
fields.User
fields have been renamed toSubject
to avoid confusion with the often defined type nameuser
.Subject
is a strongly typed entity that can be one_ofObject
(a direct object),TypedWildcard
(to represent the public wildcard), or aSubjectSet
(to represent the set of subjects that expand to one or more concrete objects). ASubjectSet
models Usersets in Zanzibar nomenclature.TupleKey
has been more aptly namedRelationshipTuple
- this coincides with our official documentation Concepts better (see Relationship Tuple)Userset
, which previously represented a relation rewrite rule, has been refactored to a more aptly namedRelationRewrite
and the model of it is much more uniform.The relations defined in an AuthorizationModel has changed from a direct
Userset rewrite
field to a map of Relation structuresmap<string, Relation>
. This will make things more uniform by far in all application code that deals with an AuthorizationModel structure and will more easily allow for indexing into the model's relation map to grab a particular relation.Relation
now uniformly represents a relation definition, and the constructs associated with it such as the type restrictions that apply to it etc..The
ReadChanges
API has been renamed toWatchChanges
and has been relocated to an auxillary gRPC service definitionWatchService
. This will allow for a separate watch server to implement the WatchService and allow that service to be hosted and scaled independently of the mainOpenFGAService
which services Checks, ListObjects, etc.. The ability to serve and scale this service independently will be more important especially as we start building other features around changelog metadata such as the indexer andExpandedWatchChanges
endpoints etc.. These workloads will be different and need not be served from the same server in general..References
Review Checklist
main