feat: add secondary datastore option (closes #260)#287
feat: add secondary datastore option (closes #260)#287xvirgov wants to merge 3 commits intoopenfga:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds secondary datastore support to OpenFGA Helm charts by introducing template helpers for environment configuration and secret name resolution, updating deployment specifications to include secondary datastore environment variables, creating conditional secrets resources, and defining configuration schema with new secondary datastore options. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@charts/openfga/templates/secrets.yaml`:
- Around line 19-20: The unconditional YAML document separator '---' is always
emitted which can create an empty document; change the template so the '---' is
only rendered when the secondary secret block will be produced — i.e., wrap the
separator inside the existing conditional that checks
.Values.datastore.secondary.enabled and ( .Values.datastore.secondary.username
|| .Values.datastore.secondary.password || .Values.datastore.secondary.uri ), or
alternatively emit the separator only when either the primary or secondary
secret condition is true so that '---' appears only when a secret resource is
actually rendered.
There was a problem hiding this comment.
Pull request overview
This PR adds support for configuring a secondary (read-only) datastore for OpenFGA, which is useful for PostgreSQL read replica setups. The implementation follows the same pattern as the primary datastore configuration, supporting three methods: direct values (creates a new secret), using an existing secret, or using a URI secret.
Changes:
- Added
datastore.secondaryconfiguration block in values.yaml with all necessary fields - Created template helper for secondary datastore secret name and environment configuration
- Updated deployment to inject secondary datastore environment variables
- Added secret creation logic for secondary datastore credentials
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| charts/openfga/values.yaml | Adds secondary datastore configuration fields (enabled, uri, uriSecret, username, password, existingSecret, secretKeys) |
| charts/openfga/templates/secrets.yaml | Adds conditional secret creation for secondary datastore credentials |
| charts/openfga/templates/deployment.yaml | Injects secondary datastore environment variables into the container |
| charts/openfga/templates/_helpers.tpl | Adds helper templates for secondary datastore secret name and environment variable configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
charts/openfga/values.yaml
Outdated
| # Optional read datastore configuration (for PostgreSQL) | ||
| secondary: | ||
| enabled: false | ||
| uri: | ||
| uriSecret: | ||
| username: | ||
| password: | ||
| existingSecret: | ||
| secretKeys: | ||
| uriKey: | ||
| usernameKey: | ||
| passwordKey: |
There was a problem hiding this comment.
The README.md should be updated to document the new secondary datastore configuration feature. Currently, the README documents the primary datastore configuration with examples. Consider adding a section explaining how to configure a secondary (read-only) datastore for PostgreSQL, including examples of the three configuration methods (direct values, existing secret, and uriSecret) similar to the existing datastore documentation.
There was a problem hiding this comment.
The readme doesn't contain doc for other values either, I don't think it's necessary to add it there either
charts/openfga/values.yaml
Outdated
| pullPolicy: Always | ||
| tag: "v2.0" | ||
|
|
||
| # Optional read datastore configuration (for PostgreSQL) |
There was a problem hiding this comment.
Consider adding validation or documentation to clarify the behavior when datastore.secondary.enabled is true but no configuration method is provided (no uri/uriSecret/existingSecret). Currently, this would result in the secondary datastore being marked as enabled but with no actual configuration, which might cause runtime errors in OpenFGA. Either add a Helm validation that ensures at least one configuration method is provided when enabled is true, or document that enabled is a feature flag that still requires one of the configuration methods to be set.
| # Optional read datastore configuration (for PostgreSQL) | |
| # Optional read datastore configuration (for PostgreSQL) | |
| # NOTE: | |
| # - `enabled` acts as a feature flag to turn on the secondary (read) datastore. | |
| # - When `enabled` is set to true, you must configure at least ONE of: | |
| # * `uri` – direct connection string | |
| # * `uriSecret` – name of a Secret containing the connection string | |
| # * `existingSecret`– name of a Secret containing credentials referenced via `secretKeys` | |
| # Otherwise, OpenFGA may start with secondary datastore marked as enabled but without | |
| # any usable configuration, which can lead to runtime errors. |
There was a problem hiding this comment.
I think that once reading up on the docs about secondary, it's clear that either of these options needs to be specified but I can add it if you think it's a good idea anyway
SoulPancake
changed the title
chadbirch
left a comment
chadbirch
left a comment
There was a problem hiding this comment.
It looks good to me. Just the minor linting error that lint-test is observing, but nothing that looks functionally significant.
2 participants
Description
Adding secondary datastore configuration in the chart
What problem is being solved?
How is it being solved?
Following same way the primary configuration is provided:
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit