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.
Contributor
Author
There was a problem hiding this comment.
Leaving this as is for now. The swarm catch-all has always been IPv4 only since the whole swarm pg_hba is IPv4 centric (the gateway and bridge subnets are all IPv4), and the IPv6 scope for this PR was just the ::/0 system user reject
There was a problem hiding this comment.
@moizpgedge, understood — since the swarm gateway and bridge subnets are all IPv4, the catch-all only needing to cover 0.0.0.0/0 makes sense. The ::/0 system-user reject was added specifically to prevent system users from slipping through a user-supplied IPv6 rule, not to open up general IPv6 access. Thanks for the context!
✏️ Learnings added
Learnt from: moizpgedge
Repo: pgEdge/control-plane PR: 400
File: server/internal/orchestrator/swarm/patroni_config.go:522-528
Timestamp: 2026-06-07T18:06:05.879Z
Learning: In pgEdge/control-plane, the swarm Patroni config pg_hba (server/internal/orchestrator/swarm/patroni_config.go) is intentionally IPv4-centric: the gateway and bridge subnets used in the swarm stack are all IPv4, so the non-system-user catch-all rule only covers 0.0.0.0/0 (not ::/0). The ::/0 system-user reject rule was added specifically to prevent system users (pgedge, patroni_replicator) from being matched by a later permissive user-supplied IPv6 rule, not to enable general IPv6 connectivity. Do not flag the missing IPv6 catch-all for non-system users in the swarm pg_hba as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 295
File: server/internal/orchestrator/systemd/orchestrator.go:482-489
Timestamp: 2026-04-02T15:53:45.848Z
Learning: In `server/internal/api/apiv1/validate.go`, the `validatePorts` function (around line 469) already rejects specs where the postgres port and patroni port are identical and non-zero, returning `"postgres and patroni ports must not conflict"`. It is invoked at both the database-spec level and the per-node level (after inheriting database-level port defaults into the effective node port). Therefore, `ValidateInstanceSpecs` in `server/internal/orchestrator/systemd/orchestrator.go` does not need an additional same-port check — the API validation layer enforces this upstream before the spec reaches the orchestrator.
Learnt from: rshoemaker
Repo: pgEdge/control-plane PR: 266
File: server/internal/workflows/update_database.go:122-153
Timestamp: 2026-02-09T21:47:46.904Z
Learning: In server/internal/workflows/update_database.go and related Go workflow files (e.g., provision_services.go), implement idempotent provisioning semantics: re-running ProvisionServices on already-provisioned service instances should be safe and non-destructive. Upsert operations (e.g., StoreServiceInstance) may temporarily set state to 'creating' but should recover back to 'running' via monitoring. Use SQL-level idempotence for user/service roles (e.g., CREATE ROLE IF NOT EXISTS / ALTER ROLE) and ensure deployment diffs compare desired vs. current state. These patterns support recovery when a user retries with corrected specs after a failure, and should be considered in similar Go workflow files across the repository.
Learnt from: rshoemaker
Repo: pgEdge/control-plane PR: 280
File: server/internal/orchestrator/swarm/mcp_config_resource.go:90-94
Timestamp: 2026-03-02T20:15:14.804Z
Learning: In MCP config resources (e.g., server/internal/orchestrator/swarm/mcp_config_resource.go and similar tokens.yaml/users.yaml), Refresh should only verify the existence of config.yaml. Tokens.yaml and users.yaml are owned and managed by the MCP server at runtime and should not be rewritten on Update. Do not trigger recreation that would overwrite runtime state. This ownership model is consistent with the comment indicating Do NOT touch tokens.yaml or users.yaml. Apply this guidance to all MCP config resource files in the swarm directory.
Learnt from: rshoemaker
Repo: pgEdge/control-plane PR: 280
File: server/internal/orchestrator/swarm/mcp_config.go:95-97
Timestamp: 2026-03-02T20:17:36.231Z
Learning: In the pgEdge/control-plane repository, internal (unexported) functions that are called through controlled paths by the resource framework should assume validated input from their invocation boundaries. Do not add defensive nil guards inside such internal helpers; rely on validation at the resource construction/invocation boundaries (e.g., MCPConfigResource.Create/Update) to guarantee data integrity. This guideline applies to all Go files under server/internal where internal helpers serve as implementation details behind controlled APIs.
Learnt from: rshoemaker
Repo: pgEdge/control-plane PR: 287
File: server/internal/orchestrator/swarm/service_user_role.go:59-60
Timestamp: 2026-03-11T16:05:27.239Z
Learning: In server/internal/orchestrator/swarm/service_user_role.go, treat a ResourceVersion bump (e.g., 2 -> 3) as a documentation/safety marker only. When the resource Identifier key changes (e.g., ServiceInstanceID to ServiceID), the system will automatically recreate resources due to the Type/ID key mismatch. Do not flag a ResourceVersion bump as a missing recreate trigger if an Identifier change is already causing a full recreate for that resource. This guidance is file-specific and should be applied when reviewing changes to service_user_role.go.
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 293
File: server/internal/orchestrator/common/paths.go:79-157
Timestamp: 2026-03-24T13:24:39.229Z
Learning: In pgEdge/control-plane, when reviewing Go packages under server/ that use the samber/do injector, apply the `Provide()` requirement only to packages that expose singleton components which are consumed as dependencies by other singleton components via the injector. Do NOT require `Provide()` for pure utility/value-type packages (e.g., path helpers, data structures) that are not injected as singleton dependencies.
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 312
File: server/internal/workflows/backend/etcd/store.go:52-58
Timestamp: 2026-03-24T19:00:21.971Z
Learning: In pgEdge/control-plane, treat errors returned from application startup/initialization functions as fatal: a startup failure should trigger the normal application shutdown path, which then calls each component’s `Shutdown`/`Stop` for cleanup. Therefore, when reviewing code in startup/initialization paths (e.g., cache/etcd initialization like `StartCaches`), do not require explicit rollback/cleanup of partially-initialized resources inside the startup function based on its error return. Avoid review suggestions that push intermediate rollback patterns for these fatal startup errors, since the existing shutdown sequence is responsible for teardown.
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 353
File: server/internal/orchestrator/common/pgbackrest_stanza.go:26-29
Timestamp: 2026-04-20T13:57:55.700Z
Learning: In the pgEdge control-plane codebase, treat `ResourceVersion()` as a cache-busting/diff-forcing mechanism for resources. Increment `ResourceVersion()` only when making a behavioral change that requires consumers to re-fetch/re-evaluate the resource, but where no serialized fields are changed. Do not bump `ResourceVersion()` solely because a field was removed—removing serialized fields should already produce a diff on its own.
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 353
File: server/internal/orchestrator/common/pgbackrest_stanza.go:26-29
Timestamp: 2026-04-20T13:57:55.700Z
Learning: In the pgEdge control-plane codebase, avoid using `DiffIgnore()` with JSON paths for fields that have been removed. `DiffIgnore()` suppresses diffs for the specified JSON field, which prevents the resource from being re-evaluated after a field is removed. For removed fields, do not add them to `DiffIgnore()`; instead let the diff occur naturally, relying on the idempotency of Update methods to handle the change safely.
Learnt from: jason-lynch
Repo: pgEdge/control-plane PR: 377
File: server/internal/monitor/provide.go:60-61
Timestamp: 2026-05-05T12:30:06.386Z
Learning: In pgEdge/control-plane, the storage layer (e.g., WatchOp) uses deferred execution: etcd connections are not created in constructors/initializers. When reviewing Go code, do not report “etcd resource leak” for code paths that merely construct candidate/watch/election objects (e.g., via electionSvc.NewCandidate) as long as the relevant execution method (Exec/Start or equivalent) is not called afterward. Only consider it a potential leak if the code actually starts/executes the watcher/candidate and fails to shut it down (if the project has a corresponding stop/close path).
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.