feat: emit user pg_hba/pg_ident entries#400
Open
moizpgedge wants to merge 1 commit into
Open
Conversation
Wire pg_hba_conf and pg_ident_conf from the instance spec into both the Swarm and common/systemd Patroni config generators, so the entries now reach the generated `pg_hba.conf` / `pg_ident.conf`. The previous change only accepted, validated, and stored them. - User entries form a zone after the CP's system-user and bridge-isolation rules and before the catch-all, so they cannot affect control-plane-internal connectivity. Node-level entries are already prepended ahead of database-level entries by `NodeInstances()`. - The Block 3 catch-all auth method now follows `password_encryption` (defaults to md5 when unset), so user passwords and the fallback stay in the same auth landscape. - Swarm: add the IPv6 (`::/0`) system-user reject the common path already had, so a permissive user rule can't reach a system user over IPv6 now that the user zone sits below the reject. - Populate `PgIdent` (previously always nil); it is purely user-supplied. - Reload is unchanged (SIGHUP); the generator just emits the entries. Adds golden tests for both generators to guard against divergence. PLAT-628
📝 WalkthroughWalkthroughBoth Patroni config generators now support user-provided ChangesUser-provided pg_hba and pg_ident configuration with password-aware authentication
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 |
![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
🤖 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.
Inline comments:
In `@server/internal/orchestrator/swarm/patroni_config.go`:
- Around line 522-528: The IPv6 catch-all is missing: when appending the IPv4
catch-all to cfg.Postgresql.PgHba using hba.Entry{Type: hba.EntryTypeHost,
Database: "all", User: "all", Address: "0.0.0.0/0", AuthMethod:
passwordAuthMethod}.String(), also append a corresponding IPv6 entry with
Address set to "::/0" (same Type, Database, User and AuthMethod) so
cfg.Postgresql.PgHba contains both IPv4 and IPv6 catch-all rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c48cac0-3d74-4ada-b13a-389f29a36d6f
📒 Files selected for processing (8)
server/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/user_pg_hba_pg_ident_and_scram.yamlserver/internal/orchestrator/common/patroni_config_generator.goserver/internal/orchestrator/common/patroni_config_generator_test.goserver/internal/orchestrator/swarm/golden_test/TestGeneratePatroniConfig/no_user_entries_defaults_to_md5.yamlserver/internal/orchestrator/swarm/golden_test/TestGeneratePatroniConfig/user_pg_hba_pg_ident_and_scram.yamlserver/internal/orchestrator/swarm/main_test.goserver/internal/orchestrator/swarm/patroni_config.goserver/internal/orchestrator/swarm/patroni_config_golden_test.go
Comment on lines
+522
to
+528
| *cfg.Postgresql.PgHba = append(*cfg.Postgresql.PgHba, hba.Entry{ | ||
| Type: hba.EntryTypeHost, | ||
| Database: "all", | ||
| User: "all", | ||
| Address: "0.0.0.0/0", | ||
| AuthMethod: passwordAuthMethod, | ||
| }.String()) |
There was a problem hiding this comment.
Missing IPv6 catch-all rule for non-system users.
The common package emits both IPv4 (0.0.0.0/0) and IPv6 (::/0) catch-all rules for non-system users (see server/internal/orchestrator/common/patroni_config_generator.go lines 455-470), but this swarm implementation only appends the IPv4 catch-all. Without the IPv6 catch-all, connections from IPv6 addresses that don't match any user-supplied rule will fall through to PostgreSQL's default deny, which may be intentional but differs from the common path.
🔧 Proposed fix to add IPv6 catch-all
// Catch-all for non-system users; the auth method follows password_encryption.
*cfg.Postgresql.PgHba = append(*cfg.Postgresql.PgHba, hba.Entry{
Type: hba.EntryTypeHost,
Database: "all",
User: "all",
Address: "0.0.0.0/0",
AuthMethod: passwordAuthMethod,
- }.String())
+ }.String(), hba.Entry{
+ Type: hba.EntryTypeHost,
+ Database: "all",
+ User: "all",
+ Address: "::/0",
+ AuthMethod: passwordAuthMethod,
+ }.String())🤖 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/orchestrator/swarm/patroni_config.go` around lines 522 - 528,
The IPv6 catch-all is missing: when appending the IPv4 catch-all to
cfg.Postgresql.PgHba using hba.Entry{Type: hba.EntryTypeHost, Database: "all",
User: "all", Address: "0.0.0.0/0", AuthMethod: passwordAuthMethod}.String(),
also append a corresponding IPv6 entry with Address set to "::/0" (same Type,
Database, User and AuthMethod) so cfg.Postgresql.PgHba contains both IPv4 and
IPv6 catch-all rules.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| Duplication | 2 |
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.
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
1 participant
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: emit user pg_hba/pg_ident entries
Wire pg_hba_conf and pg_ident_conf from the instance spec into both the Swarm and common/systemd Patroni config generators, so the entries now reach the generated
pg_hba.conf/pg_ident.conf. The previous change only accepted, validated, and stored them.NodeInstances().password_encryption(defaults to md5 when unset), so user passwords and the fallback stay in the same auth landscape.::/0) system-user reject the common path already had, so a permissive user rule can't reach a system user over IPv6 now that the user zone sits below the reject.PgIdent(previously always nil); it is purely user-supplied.Adds golden tests for both generators to guard against divergence.
PLAT-628
Summary
Makes the
pg_hba_conf/pg_ident_confdatabase-spec fields take effect: both Patroni config generators (Swarm and common/systemd) now emit the user entries into the generatedpg_hba.conf/pg_ident.conf, the catch-all auth method followspassword_encryption, and the Swarm path gains the IPv6 system-user reject. The prior PR added the spec contract, parser, and validation only.Changes
server/internal/orchestrator/swarm/patroni_config.go): insert the user zone between the bridge-subnet reject and the catch-all; add the::/0system-user reject; the catch-all auth method followspassword_encryption(default md5); populatePgIdentfrom the spec. Final order:system reject (v4+v6) → gateway md5 → bridge-subnet reject → user zone → catch-all.server/internal/orchestrator/common/patroni_config_generator.go): addPgHbaConf/PgIdentConfto the generator (wired fromopts.Instance, likeSpecParameters); emit the user zone at the existingextraEntriesslot; both v4/v6 catch-alls followpassword_encryption; populatePgIdentvia apgIdent()helper.patroni_config_golden_test.go+main_test.gofor the-updateflag; common: newuser pg_hba pg_ident and scramcase), covering user-zone position, IPv6 reject, scram-vs-md5 catch-all, andpg_ident.No behavior change when the fields are empty/unset — the new struct fields use
omitemptyand the user-zone append is a no-op, so generated output is byte-identical to today.Testing
Manual verification against a local dev cluster (restish +
docker exec), withpassword_encryption: scram-sha-256and a cert rule usingmap=:pg_hba_conf, apg_ident_confmapping, andpassword_encryption. Confirmed in the running container'spg_hba.conf:::/0system-user reject present (alongside0.0.0.0/0);scram-sha-256;pg_ident.confcontains the mapping, referenced by themap=ssl_userscert rule.StartedAt— SIGHUP reload).Checklist
feat/PLAT-628/...)Notes for Reviewers
md5(design §1). The Swarmhost all all <gateway>/32 md5rule is left as md5 to match the design ("bridge gateway MD5"); only the catch-all followspassword_encryption. Heads-up: underpassword_encryption=scram-sha-256, host-local clients hitting the gateway rule would fail md5 auth. Conformant with the merged design — flagging as a possible follow-up rather than changing it here.PgHbaConf/PgIdentConfthrough the generator'sopts.Instance(consistent with howSpecParameters/PostgreSQLConfalready flow) rather than the[]hba.EntryExtraHbaEntriesparam. This emits the user lines verbatim (no lossy re-serialization throughhba.Entry.String()) and lands them at the same position.ExtraHbaEntriesstays available for orchestrator-internal structured entries. Easy to switch to the literal param route if preferred.PatroniConfigGeneratoris serialized into resource state (json:"generator"); the new fields areomitempty, so existing databases serialize identically (no migration, no spurious diff), and databases with entries carry them in state — which correctly drives the Update → SIGHUP reload.