feat: add pg_hba_conf/pg_ident_conf to db spec#399
Merged
moizpgedge merged 2 commits intoJun 4, 2026
Merged
Conversation
Add user-managed `pg_hba_conf` and `pg_ident_conf` fields to the database spec, at both the database and per-node level, as arrays of strings (matching Patroni's `pg_hba`/`pg_ident` contract). - Per-node entries are **prepended** to database-level entries, giving node rules first-match priority while keeping the database-level list as the baseline. Empty/unset is identical to today's behavior. - Add a pg_hba/pg_ident line parser (`postgres/hba`) and minimal spec-time validation: reject `hba_file`/`ident_file` in `postgresql_conf` (they would make the entries ineffective) and lines that fail to parse, surfacing the offending line. No enum-style restriction on auth methods. Scope: spec contract, parser, and validation only. The orchestrator generators do not yet emit these entries into the generated `pg_hba.conf`/`pg_ident.conf`; that lands in a follow-up. See internal-design-docs: pg_hba / pg_ident User Configuration. PLAT-615
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds pg_hba.conf and pg_ident.conf support: new parsers, model fields on Node/Spec/Instance, API schema and conversion, instance-level merging (node-first), and validation with tests for parsing and configuration errors. ChangesHBA/Ident Configuration Pipeline
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 2 medium |
🟢 Metrics 35 complexity · 4 duplication
Metric Results Complexity 35 Duplication 4
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/database/spec_test.go (1)
597-609: ⚡ Quick winAdd prepend-order assertion for
PgIdentConfin the same override scenario.This subtest verifies prepend priority for
PgHbaConfbut not forPgIdentConf. Since both are merged independently, this should be asserted too.Suggested test update
t.Run("node entries are prepended ahead of database-level entries", func(t *testing.T) { nodeHba := "host example myapp_user 10.0.0.0/8 scram-sha-256" + nodeIdent := "ssl_users CN=bob bob" s := spec( - &database.Node{Name: "n1", HostIDs: []string{"host-1"}, PgHbaConf: []string{nodeHba}}, + &database.Node{ + Name: "n1", + HostIDs: []string{"host-1"}, + PgHbaConf: []string{nodeHba}, + PgIdentConf: []string{nodeIdent}, + }, &database.Node{Name: "n2", HostIDs: []string{"host-2"}}, ) nodes, err := s.NodeInstances() assert.NoError(t, err) // n1: node entry first, then database-level entry (first-match priority). assert.Equal(t, append([]string{nodeHba}, dbHba...), nodes[0].Instances[0].PgHbaConf) + assert.Equal(t, append([]string{nodeIdent}, dbIdent...), nodes[0].Instances[0].PgIdentConf) // n2: unchanged, inherits database-level list only. assert.Equal(t, dbHba, nodes[1].Instances[0].PgHbaConf) + assert.Equal(t, dbIdent, nodes[1].Instances[0].PgIdentConf) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/database/spec_test.go` around lines 597 - 609, The test adds an assertion ensuring node-level PgHbaConf entries are prepended before database-level entries but misses the equivalent check for PgIdentConf; update the subtest in spec_test.go (the anonymous t.Run that calls spec(...), s.NodeInstances(), and inspects nodes) to also assert that for node "n1" the Instances[0].PgIdentConf equals append([]string{<nodeIdentEntry>}, dbIdent...) (and that node "n2" still equals dbIdent), i.e. mirror the PgHbaConf checks for PgIdentConf using the same setup of database.Node with a node-level PgIdentConf entry so both merge behaviors are validated. Ensure you reference the same variables used in the test (spec, database.Node, NodeInstances, nodes, dbHba/dbIdent) when adding the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/internal/database/spec_test.go`:
- Around line 597-609: The test adds an assertion ensuring node-level PgHbaConf
entries are prepended before database-level entries but misses the equivalent
check for PgIdentConf; update the subtest in spec_test.go (the anonymous t.Run
that calls spec(...), s.NodeInstances(), and inspects nodes) to also assert that
for node "n1" the Instances[0].PgIdentConf equals
append([]string{<nodeIdentEntry>}, dbIdent...) (and that node "n2" still equals
dbIdent), i.e. mirror the PgHbaConf checks for PgIdentConf using the same setup
of database.Node with a node-level PgIdentConf entry so both merge behaviors are
validated. Ensure you reference the same variables used in the test (spec,
database.Node, NodeInstances, nodes, dbHba/dbIdent) when adding the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab00641f-10a7-4c97-9723-da29e377d6bd
⛔ Files ignored due to path filters (9)
api/apiv1/gen/control_plane/service.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/encode_decode.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (8)
api/apiv1/design/database.goserver/internal/api/apiv1/convert.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/database/spec.goserver/internal/database/spec_test.goserver/internal/postgres/hba/parse.goserver/internal/postgres/hba/parse_test.go
jason-lynch
reviewed
Jun 4, 2026
jason-lynch
left a comment
jason-lynch
left a comment
Member
There was a problem hiding this comment.
Nice work! All of my comments are really minor, and the parser implementation is neat!
Since you're adding the API fields here we'll want to hold off on the release until this feature is complete. I'm on board with that since we have a few other in-flight changes we want to include in the release as well.
Code-review cleanups for the pg_hba_conf/pg_ident_conf spec work. No
behavior change.
- Merge node-level and database-level entries with `slices.Concat`
instead of a custom helper. Node entries come first to keep
first-match priority.
- Use `ds.Set` for the pg_hba connection-type set instead of a plain
`map[EntryType]struct{}`.
- Drop the redundant `strings.Contains` guard in `isBareIP`;
`net.ParseIP` already returns nil for CIDR strings.
- Simplify the tokenizer to flush on `buf.Len() > 0` instead of a
separate `hasToken` flag.
- Convert the parser tests to testify, asserting specific error
messages via `require.ErrorContains`.
PLAT-615
jason-lynch
approved these changes
Jun 4, 2026
jason-lynch
left a comment
jason-lynch
left a comment
Member
There was a problem hiding this comment.
Awesome! Thank you for making those changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR: feat: add pg_hba_conf/pg_ident_conf to db spec
Add user-managed
pg_hba_confandpg_ident_conffields to the database spec, at both the database and per-node level, as arrays of strings (matching Patroni'spg_hba/pg_identcontract).postgres/hba) and minimal spec-time validation: rejecthba_file/ident_fileinpostgresql_conf(they would make the entries ineffective) and lines that fail to parse, surfacing the offending line. No enum-style restriction on auth methods.Scope: spec contract, parser, and validation only. The orchestrator generators do not yet emit these entries into the generated
pg_hba.conf/pg_ident.conf; that lands in a follow-up.See internal-design-docs: pg_hba / pg_ident User Configuration.
PLAT-615
Summary
Adds
pg_hba_confandpg_ident_confto the database spec (database-level and per-node) so operators can supply their own authentication rules. This PR covers the spec contract, a pg_hba/pg_ident parser, and validation only — the entries are accepted, validated, stored, and round-tripped, but are not yet emitted into the generated config (follow-up ticket).Changes
api/apiv1/design/database.go): addpg_hba_confandpg_ident_conf(ArrayOf(String)) toDatabaseSpecandDatabaseNodeSpec; regenerated Goa code and OpenAPI.server/internal/database/spec.go): add both fields toSpec,Node, andInstanceSpec(and theirClone()helpers); prepend-merge node-level entries ahead of database-level entries inNodeInstances().server/internal/api/apiv1/convert.go): map the new fields in both directions, at the database and node level.server/internal/postgres/hba/parse.go): newParseEntry(pg_hba) andParseIdent(pg_ident) with a quote-aware tokenizer; structural validation only (connection type + field count), no auth-method enum restriction.server/internal/api/apiv1/validate.go): rejecthba_file/ident_fileGUCs and unparseable lines (surfacing the offending line); wired into bothvalidateDatabaseSpecandvalidateNode, so it runs on create and update.Testing
Manual verification against a local dev cluster (restish):
pg_hba_conf/pg_ident_conf(db-level + node-level) → accepted;get-databaseround-trips the exact entries.modifying→available); changes round-trip.hba_file/ident_fileGUC each return400 invalid_inputwith the offending line/key; a non-standard auth method (ldap2) is accepted.pg_hba.confis unchanged (entries not yet emitted — expected for this scope).Checklist
Notes for Reviewers
swarm/common) do not yet write them intopg_hba.conf/pg_ident.conf, and there is no reload yet. That is the follow-up ticket.api/apiv1/gen/http/openapi3.*shows a large diff because goa's example faker reseeds and reshuffles every placeholder when the design changes. It is deterministic and expected (a clean design regenerates to zero diff). Happy to split the regeneratedgen/**into its own commit if that helps review.hba_file/ident_fileand unparseable lines are rejected — no auth-method enum, notrust-on-broad-CIDR block, no duplicate ormap=cross-ref checks. PostgreSQL surfaces deeper errors at reload time.omitempty; golden tests are unchanged).Result:-