feat: add orchestrator/common package#293
Open
jason-lynch wants to merge 1 commit intofeat/PLAT-417/postgres-database-resourcefrom
Open
feat: add orchestrator/common package#293jason-lynch wants to merge 1 commit intofeat/PLAT-417/postgres-database-resourcefrom
jason-lynch wants to merge 1 commit intofeat/PLAT-417/postgres-database-resourcefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive orchestration framework for managing Patroni/PostgreSQL clusters with etcd integration. It adds resource types for credentials, certificates, and configuration management, implements Patroni config generation with multiple deployment strategies, and registers these resources globally via a new common module. Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
298c58f to
887b1a8
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
2 times, most recently
from
9bfda29 to
3bd613f
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
887b1a8 to
daeb238
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
2 times, most recently
from
2000707 to
321ebff
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
20b19ea to
b768409
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
3 times, most recently
from
7213a0b to
fbd7177
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
e47f053 to
26aa05d
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
fbd7177 to
818b6cd
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
26aa05d to
a856583
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
818b6cd to
db06d7c
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
a856583 to
57a6663
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
db06d7c to
94c2dbe
Compare
This was referenced Mar 11, 2026
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
57a6663 to
e656f49
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
94c2dbe to
1de4ace
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
e656f49 to
4e0f475
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
1de4ace to
187c000
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
4e0f475 to
3b3cfbb
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
187c000 to
a6f44f6
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
server/internal/orchestrator/common/paths.go (1)
45-77: Well-designed auto-injection of--target-action.The logic correctly identifies restore types (
immediate,lsn,name,time,xid) that require--target-actionand defaults topromotewhen not explicitly provided.Minor nit: The comment at line 51 ("skip the next arg since it's the value") is slightly misleading since
continuedoesn't actually incrementi. The code still works correctly because the value arg won't match any prefix checks and will hitcontinueat line 60. Consider updating the comment to clarify this:if strings.HasPrefix(arg, "--target-action") { hasTargetAction = true - continue // skip the next arg since it's the value of --target-action no further checks needed + continue // no further checks needed for this flag }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/common/paths.go` around lines 45 - 77, The comment inside InstancePaths.PgBackRestRestoreCmd's arg-parsing loop is misleading: when detecting "--target-action" the code sets hasTargetAction and uses continue but does not increment i, so the following value token is not explicitly skipped by i++; update the comment to accurately state that continue leaves i unchanged and that the next token will be seen but won't match any "--type" checks (so it is effectively ignored), and similarly clarify the other comment where i++ is used for "--type" to avoid confusion about which branch advances the index.server/internal/orchestrator/common/pg_service_conf.go (1)
22-25: RenamenodeNameparameter to match actual identifier semantics.This helper currently receives instance IDs in
Identifier(). Renaming the parameter (e.g.,idorinstanceID) will reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/common/pg_service_conf.go` around lines 22 - 25, Rename the PgServiceConfResourceIdentifier function parameter from nodeName to a name that reflects it holds the instance identifier (e.g., id or instanceID) so the signature reads PgServiceConfResourceIdentifier(instanceID string) and update all references inside the function (resource.Identifier{ ID: instanceID, Type: ResourceTypePgServiceConf }) and any callers to use the new parameter name to avoid semantic confusion.server/internal/patroni/utils.go (1)
16-27: Return a deterministic unique host list to avoid config churn.Consider deduping and sorting before return, so callers get stable output across runs.
♻️ Proposed refactor
import ( "context" "fmt" "net/url" + "sort" clientv3 "go.etcd.io/etcd/client/v3" ) @@ - var hosts []string + hostSet := map[string]struct{}{} for _, member := range members.Members { for _, endpoint := range member.GetClientURLs() { u, err := url.Parse(endpoint) if err != nil { return nil, fmt.Errorf("failed to parse etcd client url '%s': %w", endpoint, err) } - hosts = append(hosts, u.Host) + hostSet[u.Host] = struct{}{} } } + hosts := make([]string, 0, len(hostSet)) + for host := range hostSet { + hosts = append(hosts, host) + } + sort.Strings(hosts) return hosts, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/patroni/utils.go` around lines 16 - 27, The returned hosts slice built from members.Members and member.GetClientURLs() should be deduplicated and sorted to ensure deterministic output; update the code that constructs the hosts slice (the loop over members.Members and member.GetClientURLs()) to collect unique host strings (e.g., via a map[string]struct{}), convert the map keys back to a slice, sort it (e.g., sort.Strings), and then return that sorted, deduped hosts slice so callers receive stable output across runs.server/internal/orchestrator/common/patroni_cluster.go (1)
3-12: Derive the Patroni prefix fromDatabaseID/NodeNameinstead of storing it.
server/internal/patroni/config.goalready definesClusterPrefix(databaseID, nodeName)as the canonical DCS namespace. KeepingPatroniClusterPrefixas separate serialized state makes the delete path depend on a derived value that can drift from the struct’s real identity.♻️ Suggested simplification
import ( "context" "fmt" "github.com/samber/do" clientv3 "go.etcd.io/etcd/client/v3" + "github.com/pgEdge/control-plane/server/internal/patroni" "github.com/pgEdge/control-plane/server/internal/resource" "github.com/pgEdge/control-plane/server/internal/storage" ) type PatroniCluster struct { DatabaseID string `json:"database_id"` NodeName string `json:"node_name"` - PatroniClusterPrefix string `json:"patroni_namespace"` } ... - _, err = storage.NewDeletePrefixOp(client, p.PatroniClusterPrefix).Exec(ctx) + _, err = storage.NewDeletePrefixOp(client, patroni.ClusterPrefix(p.DatabaseID, p.NodeName)).Exec(ctx)Also applies to: 25-29, 67-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/common/patroni_cluster.go` around lines 3 - 12, The struct currently persists PatroniClusterPrefix separately causing drift; instead compute the DCS namespace on the fly using the existing ClusterPrefix(databaseID, nodeName) helper from server/internal/patroni/config.go. Remove the PatroniClusterPrefix field usage in the struct and replace all reads/writes (including delete paths) to call ClusterPrefix(p.DatabaseID, p.NodeName) (or equivalent variables) where PatroniClusterPrefix is referenced; update any constructors/serializers to stop setting PatroniClusterPrefix and ensure code paths that previously relied on PatroniClusterPrefix use the derived value so stored state no longer diverges from the canonical identity.server/internal/orchestrator/common/patroni_config_generator.go (2)
70-140: Note the TODO for PatroniAllowlist.The TODO at line 137 indicates that
PatroniAllowlistis not yet wired to the instance/database types. The field exists onPatroniConfigGeneratorbut will always be nil/empty until this is addressed.Would you like me to open an issue to track adding the allowlist field to instance and database types?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/common/patroni_config_generator.go` around lines 70 - 140, The PatroniAllowlist is currently not propagated from the instance/database types into PatroniConfigGenerator, so add a PatroniAllowlist field to the InstanceSpec (and/or Database type if global) and to PatroniConfigGeneratorOptions, then wire it through in NewPatroniConfigGenerator by setting PatroniAllowlist: opts.Instance.PatroniAllowlist (or opts.<Database>.PatroniAllowlist) when constructing the PatroniConfigGenerator; update any constructor signatures, serialization/parsing of instance/database specs, and relevant tests to ensure the allowlist is persisted and populated instead of remaining nil as the TODO indicates.
400-419: Consider the ordering ofextraEntriesrelative to reject rules.Extra HBA entries are appended after the reject rules for system users (lines 400-415). If any
extraEntriesneed to allow connections forpgedgeorpatroni_replicatorfrom additional addresses, they will be ineffective because the reject rules are evaluated first.If this is intentional (extra entries should only apply to non-system users), consider documenting this constraint in the
GenerateOptions.ExtraHbaEntriesfield comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/common/patroni_config_generator.go` around lines 400 - 419, The current code appends the system reject hba.Entry rules before appending GenerateOptions.ExtraHbaEntries (extraEntries), which makes any extra entries ineffective for pgedge/patroni_replicator because rejects are evaluated first; to fix, either move the loop that appends extraEntries to before the block that appends the two reject hba.Entry objects so extraEntries can allow connections for those users, or if the intended behavior is to keep rejects taking precedence, document this in the GenerateOptions.ExtraHbaEntries comment stating that extra entries are applied after system reject rules and cannot override them; refer to the extraEntries variable, entries slice, the hba.Entry reject entries, and GenerateOptions.ExtraHbaEntries when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/orchestrator/common/paths_test.go`:
- Line 89: The test currently only verifies the pgbackrest binary path, so
update the assertion to compare the entire pgbackrest.Cmd value (including
args/target/action) instead of a single field: replace the one-field check with
an equality check between tc.expected.PgBackrestCmd and result.PgBackrestCmd
(e.g., use assert.Equal or require.Equal on the whole pgbackrest.Cmd) so the
test validates args and target-action as well.
In `@server/internal/orchestrator/common/paths.go`:
- Around line 79-157: Add a samber/do provider function for this package so the
injector can construct the Paths utility; implement a Provide() function (e.g.,
Provide) in the same package that returns *Paths (or a constructor like
NewPaths) and registers it with do.New or do.Provide so callers can inject Paths
via the samber/do container; ensure the function creates/returns &Paths{BaseDir:
...} or accepts configuration params if needed and export the constructor name
(Provide or NewPaths) to satisfy the package-level DI requirement referenced in
the review.
In `@server/internal/orchestrator/common/patroni_config_generator.go`:
- Around line 421-431: The MD5 host rule only adds an IPv4 catch-all
("0.0.0.0/0") so IPv6 clients are not covered; update the logic that appends the
non-system-user rule (the entries slice where you create hba.Entry with Type
hba.EntryTypeHost, Database "all", User "all", AuthMethod hba.AuthMethodMD5) to
also append a second hba.Entry with Address "::/0" (same
Database/User/AuthMethod) before returning &entries so both IPv4 and IPv6 are
allowed.
In `@server/internal/orchestrator/common/pg_service_conf.go`:
- Line 147: Update the wrapped error message to reference the correct filename:
replace the incorrect "patroni.yaml" text in the fmt.Errorf call with
"pg_service.conf" so the error returned when failing to remove the
pg_service.conf file accurately reflects the operation (i.e., change the message
in the fmt.Errorf that currently reads "failed to remove patroni.yaml: %w" to
"failed to remove pg_service.conf: %w").
---
Nitpick comments:
In `@server/internal/orchestrator/common/paths.go`:
- Around line 45-77: The comment inside InstancePaths.PgBackRestRestoreCmd's
arg-parsing loop is misleading: when detecting "--target-action" the code sets
hasTargetAction and uses continue but does not increment i, so the following
value token is not explicitly skipped by i++; update the comment to accurately
state that continue leaves i unchanged and that the next token will be seen but
won't match any "--type" checks (so it is effectively ignored), and similarly
clarify the other comment where i++ is used for "--type" to avoid confusion
about which branch advances the index.
In `@server/internal/orchestrator/common/patroni_cluster.go`:
- Around line 3-12: The struct currently persists PatroniClusterPrefix
separately causing drift; instead compute the DCS namespace on the fly using the
existing ClusterPrefix(databaseID, nodeName) helper from
server/internal/patroni/config.go. Remove the PatroniClusterPrefix field usage
in the struct and replace all reads/writes (including delete paths) to call
ClusterPrefix(p.DatabaseID, p.NodeName) (or equivalent variables) where
PatroniClusterPrefix is referenced; update any constructors/serializers to stop
setting PatroniClusterPrefix and ensure code paths that previously relied on
PatroniClusterPrefix use the derived value so stored state no longer diverges
from the canonical identity.
In `@server/internal/orchestrator/common/patroni_config_generator.go`:
- Around line 70-140: The PatroniAllowlist is currently not propagated from the
instance/database types into PatroniConfigGenerator, so add a PatroniAllowlist
field to the InstanceSpec (and/or Database type if global) and to
PatroniConfigGeneratorOptions, then wire it through in NewPatroniConfigGenerator
by setting PatroniAllowlist: opts.Instance.PatroniAllowlist (or
opts.<Database>.PatroniAllowlist) when constructing the PatroniConfigGenerator;
update any constructor signatures, serialization/parsing of instance/database
specs, and relevant tests to ensure the allowlist is persisted and populated
instead of remaining nil as the TODO indicates.
- Around line 400-419: The current code appends the system reject hba.Entry
rules before appending GenerateOptions.ExtraHbaEntries (extraEntries), which
makes any extra entries ineffective for pgedge/patroni_replicator because
rejects are evaluated first; to fix, either move the loop that appends
extraEntries to before the block that appends the two reject hba.Entry objects
so extraEntries can allow connections for those users, or if the intended
behavior is to keep rejects taking precedence, document this in the
GenerateOptions.ExtraHbaEntries comment stating that extra entries are applied
after system reject rules and cannot override them; refer to the extraEntries
variable, entries slice, the hba.Entry reject entries, and
GenerateOptions.ExtraHbaEntries when making the change.
In `@server/internal/orchestrator/common/pg_service_conf.go`:
- Around line 22-25: Rename the PgServiceConfResourceIdentifier function
parameter from nodeName to a name that reflects it holds the instance identifier
(e.g., id or instanceID) so the signature reads
PgServiceConfResourceIdentifier(instanceID string) and update all references
inside the function (resource.Identifier{ ID: instanceID, Type:
ResourceTypePgServiceConf }) and any callers to use the new parameter name to
avoid semantic confusion.
In `@server/internal/patroni/utils.go`:
- Around line 16-27: The returned hosts slice built from members.Members and
member.GetClientURLs() should be deduplicated and sorted to ensure deterministic
output; update the code that constructs the hosts slice (the loop over
members.Members and member.GetClientURLs()) to collect unique host strings
(e.g., via a map[string]struct{}), convert the map keys back to a slice, sort it
(e.g., sort.Strings), and then return that sorted, deduped hosts slice so
callers receive stable output across runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08e9173a-f36f-4419-abbc-fcb188374101
📒 Files selected for processing (28)
server/cmd/root.goserver/internal/database/orchestrator.goserver/internal/orchestrator/common/etcd_creds.goserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/enable_fast_basebackup.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/in-place_restore.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/minimal_swarm.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/minimal_systemd.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/with_backup_config.yamlserver/internal/orchestrator/common/golden_test/TestPatroniConfigGenerator/with_restore_config.yamlserver/internal/orchestrator/common/main_test.goserver/internal/orchestrator/common/paths.goserver/internal/orchestrator/common/paths_test.goserver/internal/orchestrator/common/patroni_cluster.goserver/internal/orchestrator/common/patroni_config.goserver/internal/orchestrator/common/patroni_config_generator.goserver/internal/orchestrator/common/patroni_config_generator_test.goserver/internal/orchestrator/common/patroni_member.goserver/internal/orchestrator/common/pg_service_conf.goserver/internal/orchestrator/common/pgbackrest_config.goserver/internal/orchestrator/common/pgbackrest_stanza.goserver/internal/orchestrator/common/postgres_certs.goserver/internal/orchestrator/common/resources.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/patroni_config.goserver/internal/patroni/utils.goserver/internal/pgbackrest/config.goserver/internal/postgres/gucs.goserver/internal/postgres/pg_service_conf.go
| PatroniPath: "/usr/bin/patroni", | ||
| } | ||
| result := paths.PgBackRestRestoreCmd(tc.command, tc.args...) | ||
| assert.Equal(t, tc.expected.PgBackrestCmd, result.PgBackrestCmd) |
There was a problem hiding this comment.
Assert the full pgbackrest.Cmd, not just binary path.
This assertion only checks one field, so cases intended to validate args/target-action behavior can pass incorrectly.
✅ Proposed fix
- assert.Equal(t, tc.expected.PgBackrestCmd, result.PgBackrestCmd)
+ assert.Equal(t, tc.expected, result.PgBackrestCmd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/orchestrator/common/paths_test.go` at line 89, The test
currently only verifies the pgbackrest binary path, so update the assertion to
compare the entire pgbackrest.Cmd value (including args/target/action) instead
of a single field: replace the one-field check with an equality check between
tc.expected.PgBackrestCmd and result.PgBackrestCmd (e.g., use assert.Equal or
require.Equal on the whole pgbackrest.Cmd) so the test validates args and
target-action as well.
Comment on lines
+79
to
+157
| type Paths struct { | ||
| BaseDir string `json:"base_dir"` | ||
| } | ||
|
|
||
| func (p *Paths) Data() string { | ||
| return filepath.Join(p.BaseDir, "data") | ||
| } | ||
|
|
||
| func (p *Paths) Configs() string { | ||
| return filepath.Join(p.BaseDir, "configs") | ||
| } | ||
|
|
||
| func (p *Paths) Certificates() string { | ||
| return filepath.Join(p.BaseDir, "certificates") | ||
| } | ||
|
|
||
| func (p *Paths) PgData() string { | ||
| return filepath.Join(p.Data(), "pgdata") | ||
| } | ||
|
|
||
| func (p *Paths) PgDataRestore() string { | ||
| return filepath.Join(p.Data(), "pgdata-restore") | ||
| } | ||
|
|
||
| func (p *Paths) PatroniConfig() string { | ||
| return filepath.Join(p.Configs(), "patroni.yaml") | ||
| } | ||
|
|
||
| func (p *Paths) PgBackRestConfig(confType PgBackRestConfigType) string { | ||
| return filepath.Join(p.Configs(), fmt.Sprintf("pgbackrest.%s.conf", confType)) | ||
| } | ||
|
|
||
| func (p *Paths) EtcdCertificates() string { | ||
| return filepath.Join(p.Certificates(), "etcd") | ||
| } | ||
|
|
||
| func (p *Paths) EtcdCaCert() string { | ||
| return filepath.Join(p.EtcdCertificates(), etcdCaCertName) | ||
| } | ||
|
|
||
| func (p *Paths) EtcdClientCert() string { | ||
| return filepath.Join(p.EtcdCertificates(), etcdClientCertName) | ||
| } | ||
|
|
||
| func (p *Paths) EtcdClientKey() string { | ||
| return filepath.Join(p.EtcdCertificates(), etcdClientKeyName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresCertificates() string { | ||
| return filepath.Join(p.Certificates(), "postgres") | ||
| } | ||
|
|
||
| func (p *Paths) PostgresCaCert() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresCaCertName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresServerCert() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresServerCertName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresServerKey() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresServerKeyName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresSuperuserCert() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresSuperuserCertName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresSuperuserKey() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresSuperuserKeyName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresReplicatorCert() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresReplicatorCertName) | ||
| } | ||
|
|
||
| func (p *Paths) PostgresReplicatorKey() string { | ||
| return filepath.Join(p.PostgresCertificates(), postgresReplicatorKeyName) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the certificate name constants are defined in the common package
rg -n 'etcdCaCertName|etcdClientCertName|etcdClientKeyName|postgresCaCertName|postgresServerCertName|postgresServerKeyName|postgresSuperuserCertName|postgresSuperuserKeyName|postgresReplicatorCertName|postgresReplicatorKeyName' server/internal/orchestrator/common/Repository: pgEdge/control-plane
Length of output: 6508
🏁 Script executed:
#!/bin/bash
# Read the complete paths.go file to check for Provide() function and other guidelines compliance
cat -n server/internal/orchestrator/common/paths.goRepository: pgEdge/control-plane
Length of output: 5354
Verify dependency injection setup for this utility module.
The certificate name constants are properly defined in the common package, and the path construction methods use filepath.Join correctly for cross-platform compatibility. However, the coding guidelines require each package under server/**/*.go to have a Provide() function for dependency injection using the samber/do injector. While this utility module may not require external dependencies, ensure this requirement is addressed either here or at the package level in server/internal/orchestrator/common/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/orchestrator/common/paths.go` around lines 79 - 157, Add a
samber/do provider function for this package so the injector can construct the
Paths utility; implement a Provide() function (e.g., Provide) in the same
package that returns *Paths (or a constructor like NewPaths) and registers it
with do.New or do.Provide so callers can inject Paths via the samber/do
container; ensure the function creates/returns &Paths{BaseDir: ...} or accepts
configuration params if needed and export the constructor name (Provide or
NewPaths) to satisfy the package-level DI requirement referenced in the review.
Comment on lines
+421
to
+431
| // Use MD5 for non-system users from all other connections | ||
| // TODO: Can we safely upgrade this to scram-sha-256? | ||
| entries = append(entries, hba.Entry{ | ||
| Type: hba.EntryTypeHost, | ||
| Database: "all", | ||
| User: "all", | ||
| Address: "0.0.0.0/0", | ||
| AuthMethod: hba.AuthMethodMD5, | ||
| }.String()) | ||
|
|
||
| return &entries |
There was a problem hiding this comment.
Missing IPv6 catch-all for non-system user authentication.
The MD5 authentication rule only covers IPv4 (0.0.0.0/0) but not IPv6 (::0/0). Non-system users connecting via IPv6 would not match any rule and could be rejected. The reject rules for system users correctly include both address families.
🐛 Proposed fix to add IPv6 support
// Use MD5 for non-system users from all other connections
// TODO: Can we safely upgrade this to scram-sha-256?
entries = append(entries, hba.Entry{
Type: hba.EntryTypeHost,
Database: "all",
User: "all",
Address: "0.0.0.0/0",
AuthMethod: hba.AuthMethodMD5,
- }.String())
+ }.String(),
+ hba.Entry{
+ Type: hba.EntryTypeHost,
+ Database: "all",
+ User: "all",
+ Address: "::0/0",
+ AuthMethod: hba.AuthMethodMD5,
+ }.String())
return &entries
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Use MD5 for non-system users from all other connections | |
| // TODO: Can we safely upgrade this to scram-sha-256? | |
| entries = append(entries, hba.Entry{ | |
| Type: hba.EntryTypeHost, | |
| Database: "all", | |
| User: "all", | |
| Address: "0.0.0.0/0", | |
| AuthMethod: hba.AuthMethodMD5, | |
| }.String()) | |
| return &entries | |
| // Use MD5 for non-system users from all other connections | |
| // TODO: Can we safely upgrade this to scram-sha-256? | |
| entries = append(entries, hba.Entry{ | |
| Type: hba.EntryTypeHost, | |
| Database: "all", | |
| User: "all", | |
| Address: "0.0.0.0/0", | |
| AuthMethod: hba.AuthMethodMD5, | |
| }.String(), | |
| hba.Entry{ | |
| Type: hba.EntryTypeHost, | |
| Database: "all", | |
| User: "all", | |
| Address: "::0/0", | |
| AuthMethod: hba.AuthMethodMD5, | |
| }.String()) | |
| return &entries |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/orchestrator/common/patroni_config_generator.go` around lines
421 - 431, The MD5 host rule only adds an IPv4 catch-all ("0.0.0.0/0") so IPv6
clients are not covered; update the logic that appends the non-system-user rule
(the entries slice where you create hba.Entry with Type hba.EntryTypeHost,
Database "all", User "all", AuthMethod hba.AuthMethodMD5) to also append a
second hba.Entry with Address "::/0" (same Database/User/AuthMethod) before
returning &entries so both IPv4 and IPv6 are allowed.
| if errors.Is(err, afero.ErrFileNotFound) { | ||
| return nil | ||
| } else if err != nil { | ||
| return fmt.Errorf("failed to remove patroni.yaml: %w", err) |
There was a problem hiding this comment.
Fix stale filename in delete error message.
The delete path is pg_service.conf, but the wrapped error references patroni.yaml.
🛠️ Proposed fix
- return fmt.Errorf("failed to remove patroni.yaml: %w", err)
+ return fmt.Errorf("failed to remove pg_service.conf: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return fmt.Errorf("failed to remove patroni.yaml: %w", err) | |
| return fmt.Errorf("failed to remove pg_service.conf: %w", err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/orchestrator/common/pg_service_conf.go` at line 147, Update
the wrapped error message to reference the correct filename: replace the
incorrect "patroni.yaml" text in the fmt.Errorf call with "pg_service.conf" so
the error returned when failing to remove the pg_service.conf file accurately
reflects the operation (i.e., change the message in the fmt.Errorf that
currently reads "failed to remove patroni.yaml: %w" to "failed to remove
pg_service.conf: %w").
Adds a package with common resources that can be shared between more than one orchestrator implementation. This is conceptually distinct from the common resources in the `database` package, which are shared across all current and future orchestrator implementations. PLAT-417
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
3b3cfbb to
42e1c4c
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/orchestrator-common-resources
branch
from
a6f44f6 to
2f99484
Compare
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.
Summary
Adds a package with common resources that can be shared between more than one orchestrator implementation. This is conceptually distinct from the common resources in the
databasepackage, which are shared across all current and future orchestrator implementations.Testing
There are no user-facing functionality changes in this PR.
PLAT-417