Skip to content
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.

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

pg_hba module refactoring #772

Merged
merged 6 commits into from
Nov 28, 2024

Conversation

toydarian
Copy link
Collaborator

SUMMARY

This is a refactoring-only PR and another step on the refactoring and improvement-journey of that module.
There is a minor change which is, that internally auth-options are now represented with dicts and not with strings anymore. This allows us to verify that the same key doesn't appear multiple times. The integration-tests have been changed to account for that.

There is one more change in the tests, which is due to ordering of rules. I don't know how or why this was the way it was. I'm going with how ipaddress sorts those networks, which I think makes sense:

In [6]: ipaddress.ip_network('::1/128') < ipaddress.ip_network('2001:db8::1/128')
Out[6]: True

Otherwise this doesn't change the modules behavior.

More changes:

  • adds unit-tests for the refactored rule-class
  • adds comments

In upcoming PRs, I will add

  • handling of include
  • taking regexes, groups and inline-includes into account when sorting
  • using standard functions for writes
  • aligning backups with how they are handled elsewhere
ISSUE TYPE
  • Refactoring Pull Request
COMPONENT NAME

postgresql_pg_hba

@toydarian toydarian force-pushed the pg_hba-module-refactoring branch from 1a1e0ce to 6b26705 Compare November 19, 2024 08:34
@toydarian toydarian marked this pull request as ready for review November 19, 2024 09:03
Copy link
Collaborator

@hunleyd hunleyd left a comment

Choose a reason for hiding this comment

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

LGTM .. do we need a fragment for refactors @Andersson007 ?

@Andersson007
Copy link
Collaborator

LGTM .. do we need a fragment for refactors @Andersson007 ?

@hunleyd thanks for pinging!

@toydarian thanks, great improvement! Sorry for the late reply, was on PTO last week.
Important question: do I understand correctly that the module changes the return values format as we can see in the integration tests? If yes, it's actually a breaking change, so it needs a major release because the interface changed and resp users who rely on them in their playbooks, say, in conditions, can get their stuff broken.
On the other hand I'm as not the module's user not sure how critical it is.
In a perfect world we should avoid introducing breaking changes in minor releases (so we would make sure the module still returns the lines as strings as they were) but if it's something you think will unlikely cause broken playbooks, we could imo proceed.
Thoughts folks?

@hunleyd
Copy link
Collaborator

hunleyd commented Nov 25, 2024

Thoughts folks?

i'm ok with either option

@toydarian
Copy link
Collaborator Author

Thoughts folks?

You never know, there could be somebody who parses the output and then it would break for them. It also make it easier to parse the output, though. I can change that and we can save it for the next major release.
But I also order the options alphabetically, so it is deterministic in which order they are rendered. I wouldn't consider this a breaking change, even though if somebody compared the options-string it would change the behavior. But in this case I assign greater importance to being deterministic.
Is that okay @Andersson007?

@betanummeric
Copy link
Member

Hi, I currently don't have time for an in-depth review.
So only the rule ordering changes? Only in the module output or also in the written pg_hba.conf?
As far as I see the documentation only defines the return value to be the "List of the pg_hba rules as they are configured in the specified hba file." I would interpret this to match the order of the rules in the file (after potential modification).
I think the output rule order is (currently) not a defined feature, so this does not require a major release.
Changing the order in the pg_hba.conf on the other hand might break something, as noted here.

@Andersson007
Copy link
Collaborator

@betanummeric mentioned important points:

  • The order in pg_hba file shouldn't change
  • I think the order of rows returned by the module are also important, it should be imo the same as in pg_hba, i.e. as it was

@toydarian thoughts?

I would discuss another module for fetching rules like pg_hba_info or something but it's another discussion. Let's make sure now that the module works as it did and merge the PR ASAP

@toydarian
Copy link
Collaborator Author

The order in pg_hba file shouldn't change

The change is insignificant in this case. The order only changes for networks with the same size and it adheres to how the ipaddress module orders them. For example, it sorts '::1/128' before instead of after '2001:db8::1/128'. I can add some code that reverses the order only if the suffix is the same, but I don't think that is justified. It also cannot break anything, as you can't have overlapping networks of the same size.
The other thing that changes, is that the options are now sorted alphabetically, so ldapserver comes before ldapport before ldapprefix, which also doesn't break anything.

I think the order of rows returned by the module are also important, it should be imo the same as in pg_hba, i.e. as it was

I'll add some code to make it work as before. We can talk about the info module later, though I think that would probably not get used a lot.

@Andersson007
Copy link
Collaborator

@toydarian thanks for explaining! OK, please ping us explicitly when it's ready for another round of review, thanks

@toydarian
Copy link
Collaborator Author

@Andersson007 @hunleyd changed as discussed, added changelog fragment
I would have liked to get betanummeric's input, as he gave valuable feedback in the past, but if he doesn't have time, we need to make due without his review

@hunleyd hunleyd merged commit 359ca4b into ansible-collections:main Nov 28, 2024
29 checks passed
@Andersson007
Copy link
Collaborator

@toydarian thanks for the contribution!
@hunleyd @betanummeric thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants