Repository navigation
Initialize OpenFGA server on ATE API Server bootstrap. - #1670
Benjamin Elder (BenTheElder) merged 2 commits into
Conversation
a0c0420 to
a2cd50a
Compare
|
Please review first commit. The other commit is vendor dependencies. |
e16c09b to
2de21b1
Compare
| server.WithDatastore(datastore), | ||
| ) | ||
| if err != nil { | ||
| datastore.Close() |
There was a problem hiding this comment.
IIUC, this also closes the provided *pgxpool.Pool. Maybe worth mentioning that in the NewServer() function comment and double checking to make sure that a dedicated pool is always provided here so this doesn't break anything (e.g. if it was a shared pool).
There was a problem hiding this comment.
Good catch, I actually created a dedicated pool for openfga connections.
There was a problem hiding this comment.
This is also because, my implementation for create atespace + auth check happens in 2 different transactions since openFGA always executes in it's own transaction.
2b993f4 to
470caca
Compare
Bowei Du (bowei)
added
kind/feature
| defer closer.Close() | ||
| } | ||
|
|
||
| if poolProvider, ok := persistence.(interface{ Pool() *pgxpool.Pool }); ok { |
There was a problem hiding this comment.
I don't think we should be creating a new connection pool for OpenFGA --- we should have one connection pool for both OpenFGA and the store (another connection pool just increases connection pressure on Postgres for no gain).
This looks like it might be a larger refactor, since we should probably be dialing the connection pools in main and injecting them into the store and OpenFGA modules. It can be done as a follow-up.
There was a problem hiding this comment.
working on this as followup task: #1755.
| "github.com/openfga/openfga/pkg/storage" | ||
| "github.com/openfga/openfga/pkg/storage/postgres" | ||
| "github.com/openfga/openfga/pkg/storage/sqlcommon" | ||
| "github.com/pressly/goose/v3" |
There was a problem hiding this comment.
Just to clarify, this is the same solution we are using for applying migrations for the main store?
There was a problem hiding this comment.
Yes its the same:
|
|
||
| // Server wraps an embedded OpenFGA server backed by PostgreSQL and | ||
| // initialized with Substrate's authorization model. | ||
| type Server struct { |
There was a problem hiding this comment.
I am not sure on whether or not we should have this wrapper struct or instead just return the *server.Server from NewServer.
Let's see some of the API call integrations, and we can revisit.
| storeID, modelID, err := ensureStoreAndModel(ctx, fgaServer) | ||
| unlock() | ||
| if err != nil { | ||
| fgaServer.Close() |
There was a problem hiding this comment.
Needing to write these lines points sort of points to that we should be creating the fgaServer and datastore in main (where they can have an unconditional defer close() after they are created).
The risk of doing this without defer is that a panic will leave them open. But obviously, it's complicated to write the defer() logic in the scope of NewServer. That's why constructor functions should usually accept their stateful dependencies as injected arguments, not try to create them inline.
I would reconsider whether this package should have a wrapper struct, or just provide utility functions to initialize the FGA server, datastore, and apply migrations.
470caca to
0038b5e
Compare
0038b5e to
be90ffc
Compare
b29f977
Working on #1563
Summary
Initializes an embedded OpenFGA server within
ateapibacked by PostgreSQL and updates the top-level authorization scope.Key Changes
global: Renamedtype clusterandparent_clustertotype globalandparent_global, addingcan_set_policyandcan_get_policytoglobal.internal/authz/server.go):model.fgainto OpenFGA's protobuf representation.goose_db_version).pg_advisory_lockto avoid multi-replica races.*pgxpool.Pool."substrate"store and authorization model.cmd/ateapi): ExposesPool()on*atepg.Persistenceand initializesauthz.NewServeron bootstrap.Verification
openfga model test: 103/103 checks passing (model_test.fga.yaml).go test -mod=mod ./internal/authz/...: Passes against PostgreSQL (migrations, tuple writes, permission checks, and restart idempotency).