Skip to content

WIP: Add ability to define custom entries in pg_ident.conf #753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP: Add ability to define custom entries in pg_ident.conf #753

wants to merge 1 commit into from

Conversation

@dineshba
Copy link
Contributor

@dineshba dineshba commented Jan 22, 2020 •
edited
Loading

Trying to fix #633

Tasks:

  • Able to give pg_ident content in clusterSpec
  • Update pg_ident.conf file whenever there is a change in pg_ident in clusterSpec

How to use in clusterSpec:

stolonctl update -p '{"pgIdent": {"omicron": [{"systemUsername": "bryanh", "databaseUsername": "bryanh"}, {"systemUsername": "robert", "databaseUsername": "bob"}]}}'

Note: Raising WIP PR to get faster feedback on the approach.

@dineshba dineshba changed the title [WIP] add ability to define custom entries in pg_ident.conf Add ability to define custom entries in pg_ident.conf Jan 23, 2020
@sgotti
Copy link
Member

sgotti commented Jan 28, 2020

@dineshba Thanks for the PR!

  • Is it ready for review?
  • Do you think we need some integration tests for this?
  • We should probably document the new parameter in the cluster spec.

@dineshba
Copy link
Contributor Author

Hi @sgotti

It is better to add some integration tests. So I will add integration test and update the document about the new parameter. Thanks for the suggestion

So, it is not ready for review. I will mark the PR as WIP.

@dineshba dineshba changed the title Add ability to define custom entries in pg_ident.conf WIP: Add ability to define custom entries in pg_ident.conf Jan 30, 2020
porty
porty reviewed Jun 7, 2023
View reviewed changes
internal/postgresql/postgresql.go
if _, err := f.Write([]byte("# MAPNAME\tSYSTEM-USERNAME\tPG-USERNAME" + "\n")); err != nil {
return err
}
for key, value := range p.ident {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entries will likely be written out in an undefined order (https://go.dev/blog/maps#iteration-order) - sorting the entries before writing them out might be beneficial

porty
porty reviewed Jun 7, 2023
View reviewed changes
internal/postgresql/postgresql.go
}
for key, value := range p.ident {
for _, v := range value {
if _, err := f.Write([]byte(fmt.Sprintf("%s\t%s\t%s", key, v.SystemUsername, v.DBUsername) + "\n")); err != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f is an os.File which doesn't guarantee full writes

@porty
Copy link

porty commented Jun 12, 2023 •
edited
Loading

I found that attempting to replace an entry for one of the ident maps fails due to apimachinery unable to merge map entries (or something along those lines). Attempting to do so gives this error:

$ bin/stolonctl update \
    --cluster-name stolon-cluster \
    --store-backend=etcdv3 \
    -p '{ "pgIdent": { "map1": [{ "systemUsername": "sysuser1", "databaseUsername": "dbuser1" }, { "systemUsername": "sysuser2", "databaseUsername": "dbuser2" }] } }'
failed to patch cluster spec: failed to merge patch cluster spec: merging an object in json but data type is not struct, instead is: map

(where map1 already contained some entries).

Overall I think structuring the pg_ident.conf entries in to a nice JSON representation, only to put them back as lines in a file is overkill, and confusing - I've found that using []string instead (similar to pgHBA) is simpler and less error prone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@porty porty porty left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add ability to defice custom entries in pg_ident.conf

3 participants

@dineshba @sgotti @porty