Add --equal equivalent to sefcontext #5189

dithotxgh · 2022-08-25T12:30:47Z

SUMMARY

Add the ability to idempotently add and remove an fcontext
equivalence, mirroring the 'semanage fcontext --equal' operations.

This operation requires a bogus setype, or even a valid one for that
matter, as the value is unused, and I did not want to make the
conditional change to
AnsibleModule(... setype=dict(type='str', required=True), ...

New tests were added to
tests/integration/targets/sefcontext/tasks/sefcontext.yml.

During testing it was discovered that deletions using 'semanage
fcontext' do not work for an individual equivalence deletion, only
--deleteall, which blows out ALL local settings. This makes this
module an improvement over the command line tools in this case.

This should address #1193

Signed-off-by: Doug Maxey dithotxgh@github.com

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

community.general.sefcontext

Add the ability to idempotently add and remove an fcontext equivalence, mirroring the 'semanage fcontext --equal' operations. This operation requires a bogus setype, or even a valid one for that matter, as the value is unused, and I did not want to make the conditional change to AnsibleModule(... setype=dict(type='str', required=True), ... New tests were added to tests/integration/targets/sefcontext/tasks/sefcontext.yml. During testing it was discovered that deletions using 'semanage fcontext' do not work for an individual equivalence deletion, only --deleteall, which blows out ALL local settings. This makes this module an improvement over the command line tools in this case. Signed-off-by: Doug Maxey <dithotxgh@github.com>

ansibullbot · 2022-08-25T12:31:11Z

cc @dagwieers
click here for bot help

github-actions · 2022-08-25T12:42:09Z

Docs Build 📝

This PR is closed and any previously published docsite has been unpublished.

TMI: missed that default is needed only for selections. Signed-off-by: Doug Maxey <dithotxgh@github.com>

dithotxgh · 2022-08-26T04:53:10Z

Please hold off on moving this forward while I investigate an issue outside the testcase,

Re-ran the testcases to see if I missed it there, but all still pass.

Am getting a 'KeyError: equiv' traceback running this in production. Debugging it now.

dithotxgh · 2022-08-26T05:17:14Z

Please hold off on moving this forward while I investigate an issue outside the testcase,

Re-ran the testcases to see if I missed it there, but all still pass.

Am getting a 'KeyError: equiv' traceback running this in production. Debugging it now.

Aha. Due to pathing, was picking up an older modules version with the bug, fixed in the latest. So there is no error when using the correct current version.

So the hold is not needed.

dithotxgh · 2022-08-26T06:36:27Z

FYI: Testing environment is current ansible 2.12.2 on rhel 8 and 9.

felixfontein

I've added some first comments, mostly on the documentation part.

felixfontein · 2022-08-27T09:21:26Z

plugins/modules/system/sefcontext.py

@@ -22,6 +22,13 @@
    type: str
    required: yes
    aliases: [ path ]
+  equiv:
+    description:
+    - set target selinux equivalence to this path.


Please put proper sentences in each paragraph.

Suggested change

- set target selinux equivalence to this path.

- Set target selinux equivalence to this path.

Unresolving, as the issue remains.

felixfontein · 2022-08-27T09:21:59Z

plugins/modules/system/sefcontext.py

+  equiv:
+    description:
+    - set target selinux equivalence to this path.
+    - setype must be set, the bogus 'many_t' works.


Suggested change

- setype must be set, the bogus 'many_t' works.

- I(setype) must be set, the bogus C(many_t) works.

See https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#linking-and-other-format-macros-within-module-documentation for how to format things.

Feel free to make any doc changes you need.

@dithotxgh please take a look at the conventions used in the code and documentation - the PR will not be merged otherwise.

felixfontein · 2022-08-27T09:22:22Z

plugins/modules/system/sefcontext.py

+    - setype must be set, the bogus 'many_t' works.
+    - when set, ftype, seuser, selevel are ignored.
+    type: str
+    required: no


Use true/false for YAML booleans. (Check out the current main branch, it should do that for all options in this module.)

Suggested change

required: no

required: false

felixfontein · 2022-08-27T09:22:48Z

plugins/modules/system/sefcontext.py

+    - set target selinux equivalence to this path.
+    - setype must be set, the bogus 'many_t' works.
+    - when set, ftype, seuser, selevel are ignored.
+    type: str


The next minor release will be 5.6.0:

Suggested change

type: str

version_added: 5.6.0

type: str

felixfontein · 2022-08-27T09:23:20Z

plugins/modules/system/sefcontext.py

+    equiv: '/var/lib/pgsql'
+    path: '/tmp/foo'
+    setype: 'many_t'  # setype 'many_t' does not exist, but works with no conflicts.
+    reload: no


Suggested change

reload: no

reload: false

Same below.

russoz · 2022-09-05T11:19:44Z

Other than the adjustments pointed by @felixfontein , it LGTM

felixfontein · 2022-09-05T19:33:26Z

Please note this also requires a changelog fragment. Looks like I forgot to mention that in my first review :)

russoz

@dithotxgh Please see the comments and make the proper adjustments.

russoz · 2022-09-10T04:54:24Z

plugins/modules/system/sefcontext.py

@@ -22,6 +22,13 @@
    type: str
    required: yes
    aliases: [ path ]
+  equiv:
+    description:
+    - set target selinux equivalence to this path.


Unresolving, as the issue remains.

russoz · 2022-09-10T04:56:14Z

plugins/modules/system/sefcontext.py

+  equiv:
+    description:
+    - set target selinux equivalence to this path.
+    - setype must be set, the bogus 'many_t' works.


@dithotxgh please take a look at the conventions used in the code and documentation - the PR will not be merged otherwise.

russoz · 2022-09-10T04:58:16Z

plugins/modules/system/sefcontext.py

+    try:
+        # this is for the shipped equivs.
+        return sefcontext.equiv_dist[target]
+    except KeyError:
+        try:
+            # these are the local added items.
+            return sefcontext.equiv[target]
+        except KeyError:
+            return None


This can be simplified as:

Suggested change

try:

# this is for the shipped equivs.

return sefcontext.equiv_dist[target]

except KeyError:

try:

# these are the local added items.

return sefcontext.equiv[target]

except KeyError:

return None

return sefcontext.equiv_dist.get(target, sefcontext.equiv.get(target))

dithotxgh · 2022-09-10T05:57:10Z

I was making a contribution, that appears to have violated your idea of what makes a valid contribution.

Quibbles over style stopping the benefit of the contribution are on you.

I don't care that you make editorial changes if necessary to align with your worldview. My part is done.

russoz · 2022-09-10T08:33:23Z

Your contribution seems to be quite valid, however this community is keen to maintain the documentation up to date and consistent, and that includes nits like formatting, punctuation and what not. Other than that, we only suggested an improvement in the code that makes it simpler and more readable. You seem to have taken the comments as personal offenses, but we make that kind of adjustments to all contributions.

You are free to let this PR die as it is. I think it would be a shame, but if that's your choice, that's how it's going to be.

dithotxgh · 2022-09-10T19:49:42Z

Nope, nothing personal, Just an observation.

If your process does not allow minor editorial changes, it's broken.

russoz · 2022-09-10T23:46:44Z

The process allows it, of course. But this is a community: "our" community (you included), not "yours" or "mine" as your language points to. People volunteer time to this work, they don't have to do the editorial work dumped upon them, especially when it comes paired with passive-aggressive rhetoric. A different tone might have got you much further in gathering other people's help.

felixfontein · 2022-10-01T20:09:17Z

Please update the PR according to the review comments. Otherwise we will close it eventually. Thanks.

needs_info

ansibullbot · 2022-11-02T06:40:41Z

@dithotxgh This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

felixfontein · 2022-11-03T05:58:19Z

Please note that in #5461 the collection repository was restructured to remove the directory tree in plugins/modules/, and the corresponding tree in tests/unit/plugins/modules/. Your PR modifies files in this directory structure, and unfortunately now has some conflicts, potentially because of this. Please rebase with the current main branch to remove the conflicts.

ansibullbot · 2022-12-02T19:34:25Z

@dithotxgh You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.

click here for bot help

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Aug 25, 2022

drop default: None in doc block

9132ca6

TMI: missed that default is needed only for selections. Signed-off-by: Doug Maxey <dithotxgh@github.com>

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Aug 25, 2022

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Aug 26, 2022

felixfontein reviewed Aug 27, 2022

View reviewed changes

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Sep 4, 2022

russoz requested changes Sep 10, 2022

View reviewed changes

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Sep 10, 2022

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Oct 1, 2022

felixfontein removed the backport-5 label Oct 25, 2022

ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 3, 2022

felixfontein added the backport-6 label Nov 8, 2022

ansibullbot closed this Dec 2, 2022

bluikko mentioned this pull request Jan 13, 2023

sefcontext: add support for path substitutions #5830

Merged

patchback bot mentioned this pull request Feb 26, 2023

[PR #5830/c8a2ac3a backport][stable-6] sefcontext: add support for path substitutions #6098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --equal equivalent to sefcontext #5189

Add --equal equivalent to sefcontext #5189

dithotxgh commented Aug 25, 2022

ansibullbot commented Aug 25, 2022

This comment was marked as outdated.

github-actions bot commented Aug 25, 2022 •

edited

Loading

dithotxgh commented Aug 26, 2022

dithotxgh commented Aug 26, 2022

dithotxgh commented Aug 26, 2022

felixfontein left a comment

felixfontein Aug 27, 2022

russoz Sep 10, 2022

felixfontein Aug 27, 2022

dithotxgh Sep 10, 2022

russoz Sep 10, 2022

felixfontein Aug 27, 2022

felixfontein Aug 27, 2022

felixfontein Aug 27, 2022

russoz commented Sep 5, 2022

felixfontein commented Sep 5, 2022

russoz left a comment

russoz Sep 10, 2022

russoz Sep 10, 2022

russoz Sep 10, 2022

dithotxgh commented Sep 10, 2022

russoz commented Sep 10, 2022

dithotxgh commented Sep 10, 2022

russoz commented Sep 10, 2022

felixfontein commented Oct 1, 2022

ansibullbot commented Nov 2, 2022

felixfontein commented Nov 3, 2022

ansibullbot commented Dec 2, 2022

	- set target selinux equivalence to this path.
	- Set target selinux equivalence to this path.

	- setype must be set, the bogus 'many_t' works.
	- I(setype) must be set, the bogus C(many_t) works.

Add --equal equivalent to sefcontext #5189

Add --equal equivalent to sefcontext #5189

Conversation

dithotxgh commented Aug 25, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Aug 25, 2022

This comment was marked as outdated.

github-actions bot commented Aug 25, 2022 • edited Loading

Docs Build 📝

dithotxgh commented Aug 26, 2022

dithotxgh commented Aug 26, 2022

dithotxgh commented Aug 26, 2022

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russoz commented Sep 5, 2022

felixfontein commented Sep 5, 2022

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dithotxgh commented Sep 10, 2022

russoz commented Sep 10, 2022

dithotxgh commented Sep 10, 2022

russoz commented Sep 10, 2022

felixfontein commented Oct 1, 2022

ansibullbot commented Nov 2, 2022

felixfontein commented Nov 3, 2022

ansibullbot commented Dec 2, 2022

github-actions bot commented Aug 25, 2022 •

edited

Loading