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

ini_file - support optional spaces around section names #8075

Merged
merged 2 commits into from
Mar 24, 2024

Conversation

utoddl
Copy link
Contributor

@utoddl utoddl commented Mar 9, 2024

ini_file - Support optional spaces between section names and their surrounding brackets

SUMMARY

Some ini files have spaces between some of their section names and the brackets that enclose them. This is documented in the openssl.cnf(5) man page. In order to manage files such as openssl.cnf with ini_file before now, one would have to include spaces in the section name like this:

    section: ' crypto_policy '
    option: Options
    value: UnsafeLegacyRenegotiation

This change implements matching section headers with such optional spaces, removing the need to include those spaces in the task's section: parameter. Existing tasks using the workaround above will continue to work, even in cases where spaces in section headers are subsequently removed.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ini_file

ADDITIONAL INFORMATION

Here's an actual "legacy" production ini_file task. Note the spaces in the section name. These spaces have been necessary because the target file has these spaces already.

    - name: Configure UnsafeLegacyRenegotiation in /etc/pki/tls/openssl.cnf
      community.general.ini_file:
        path: /etc/pki/tls/openssl.cnf
        section: ' crypto_policy '
        option: Options
        value: UnsafeLegacyRenegotiation
        state: present
        exclusive: false
        create: false
        backup: true

After this change, those spaces are no longer necessary. Furthermore, the above task will continue to work even if someone removes the spaces from the target file's crypto_policy section header.

…rrounding brackets

Some ini files have spaces between some of their section names and the
brackets that enclose them. This is documented in the 'openssl.cnf(5)' man
page. In order to manage files such as /etc/ssl/openssl.cnf with ini_file
before now, one would have to include spaces in the section name like this:
    section: ' crypto_policy '
    option: Options
    value: UnsafeLegacyRenegotiation

This change implements matching section headers with such optional spaces.
Existing tasks using the workaround above will continue to work, even in
cases where spaces in section headers are subsequently removed.
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-8 Automatically create a backport for the stable-8 branch labels Mar 9, 2024
@felixfontein
Copy link
Collaborator

I wonder whether this should better be configurable, since there could be INI parsers which consider [foo] and [ foo ] to be two distinct sections (and maybe in some cases you even need both in the same file?). If that would be the case, this change would be breaking for users which rely on the existing behavior.

@felixfontein
Copy link
Collaborator

(I have zero idea whether that's actually the case. It's just that there are so many flavors of INI files that I woudn't be surprised if that's the case...)

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Mar 9, 2024
@utoddl
Copy link
Contributor Author

utoddl commented Mar 9, 2024

I admit I had not considered the possibility that someone / something may be using outer spaces as a discriminator between otherwise identically named sections. But I would be extremely surprised.

The point of ini files is to have a format that is

  • easily read by machines
  • easily maintained by humans

The latter point seems to exclude "significant leading/trailing spaces" shenanigans. Adding a flag later if necessary could be considered, but without evidence of such cases in the wild I don't think it's worthwhile bloating ini_file's options with something the almost certainly will never be used.

@utoddl
Copy link
Contributor Author

utoddl commented Mar 9, 2024

I should add that, with this change, section: ' foo ' would match existing section headers like:

[foo]
[ foo]
[foo ]
[ foo ]
[    foo     ]

However, if it caused the creation of a section, that header would be exactly as specified:

[ foo ]

in this case.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

I think there could be a slight improvement in the tests contents formatting, for readability's sake, but that is certainly not a show-stopper.

- name: Test-section_name_spaces 1 (does legacy workaround still work) - create test file
ansible.builtin.copy: # noqa risky-file-permissions
dest: "{{ output_file }}"
content: "[ foo ]\n; bar=baz\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

for the sake of readability, I would suggest using:

Suggested change
content: "[ foo ]\n; bar=baz\n"
content: |
[ foo ]
; bar=baz

- name: Test-section_name_spaces 1 - verify results
vars:
actual_content: "{{ output_content.content | b64decode }}"
expected_content: "[ foo ]\nbar = frelt\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

conversely:

Suggested change
expected_content: "[ foo ]\nbar = frelt\n"
expected_content: |
[ foo ]
bar = frelt

same in the similar situations below

@felixfontein
Copy link
Collaborator

I admit I had not considered the possibility that someone / something may be using outer spaces as a discriminator between otherwise identically named sections. But I would be extremely surprised.

The point of ini files is to have a format that is

* easily read by machines

* easily maintained by humans

The latter point seems to exclude "significant leading/trailing spaces" shenanigans. Adding a flag later if necessary could be considered, but without evidence of such cases in the wild I don't think it's worthwhile bloating ini_file's options with something the almost certainly will never be used.

I agree it's not very likely (and I hope nobody ever does that - but then, you never know ;) ). I think it's OK to merge this without making it configurable; in case someone actually needs this we can add a flag later to toggle the behavior.

@russoz
Copy link
Collaborator

russoz commented Mar 17, 2024

hi @utoddl

I left a formatting suggestion on the test script. It's a minor thing but it would help with readability. Other than that, I think this is good to go.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Mar 24, 2024
@felixfontein felixfontein merged commit 4363f87 into ansible-collections:main Mar 24, 2024
124 checks passed
Copy link

patchback bot commented Mar 24, 2024

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/4363f8764b44a6bf62f671954642f60f9e76186c/pr-8075

Backported as #8135

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Mar 24, 2024
* ini_file - support optional spaces between section names and their surrounding brackets

Some ini files have spaces between some of their section names and the
brackets that enclose them. This is documented in the 'openssl.cnf(5)' man
page. In order to manage files such as /etc/ssl/openssl.cnf with ini_file
before now, one would have to include spaces in the section name like this:
    section: ' crypto_policy '
    option: Options
    value: UnsafeLegacyRenegotiation

This change implements matching section headers with such optional spaces.
Existing tasks using the workaround above will continue to work, even in
cases where spaces in section headers are subsequently removed.

* readability improvement in the test content expressions

---------

Co-authored-by: Todd Lewis <[email protected]>
(cherry picked from commit 4363f87)
@felixfontein
Copy link
Collaborator

@utoddl thanks for your contribution!
@russoz thanks for reviewing this!

felixfontein pushed a commit that referenced this pull request Mar 24, 2024
…aces around section names (#8135)

ini_file - support optional spaces around section names (#8075)

* ini_file - support optional spaces between section names and their surrounding brackets

Some ini files have spaces between some of their section names and the
brackets that enclose them. This is documented in the 'openssl.cnf(5)' man
page. In order to manage files such as /etc/ssl/openssl.cnf with ini_file
before now, one would have to include spaces in the section name like this:
    section: ' crypto_policy '
    option: Options
    value: UnsafeLegacyRenegotiation

This change implements matching section headers with such optional spaces.
Existing tasks using the workaround above will continue to work, even in
cases where spaces in section headers are subsequently removed.

* readability improvement in the test content expressions

---------

Co-authored-by: Todd Lewis <[email protected]>
(cherry picked from commit 4363f87)

Co-authored-by: Todd Lewis <[email protected]>
@utoddl utoddl deleted the ini_file_sections branch March 24, 2024 17:40
Massl123 pushed a commit to Massl123/community.general that referenced this pull request Feb 7, 2025
…ections#8075)

* ini_file - support optional spaces between section names and their surrounding brackets

Some ini files have spaces between some of their section names and the
brackets that enclose them. This is documented in the 'openssl.cnf(5)' man
page. In order to manage files such as /etc/ssl/openssl.cnf with ini_file
before now, one would have to include spaces in the section name like this:
    section: ' crypto_policy '
    option: Options
    value: UnsafeLegacyRenegotiation

This change implements matching section headers with such optional spaces.
Existing tasks using the workaround above will continue to work, even in
cases where spaces in section headers are subsequently removed.

* readability improvement in the test content expressions

---------

Co-authored-by: Todd Lewis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8 Automatically create a backport for the stable-8 branch bug This issue/PR relates to a bug integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants