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

Support absolute paths for SELinux modules #264

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

cosandr
Copy link
Contributor

@cosandr cosandr commented Jan 12, 2023

Hi,

I like to keep forks of roles to a minimum and often refer to files (like your keepalived_selinux_compile_rules list) by absolute paths, usually based on inventory_dir or similar. This isn't supported currently as it assumes each item in that list is a filename. This PR adds a new var for the basename which is used in most places. It has no effect on the default values and shouldn't be a breaking change for existing users as I expect it currently fails with relative paths like it does with absolute ones.

I've also changed the check used for installing SELinux packages to check against the first element in keepalived_selinux_compile_rules instead of hardcoding keepalived_ping, which might be removed by a user overriding the list and thus missing the requirements for compiling modules.

@evrardjp
Copy link
Owner

I sadly don't have the time to check this in the next weeks.

As selinux coverage was never big, I can't ensure test this myself. Can you confirm it's been tested okay ?

When I will be available, I'll be happy to review and merge as I don't have anything against the idea.

Or, in other words: Looks good at first sight, but need to spend more time on it.

@cosandr
Copy link
Contributor Author

cosandr commented Jan 13, 2023

No worries, I'm not in a rush.

I've tried it by removing all modules it installed and it worked with the default values as well as my own custom policy which is using an absolute path.

Copy link
Owner

@evrardjp evrardjp left a comment

Choose a reason for hiding this comment

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

That's better indeed.

@evrardjp evrardjp merged commit baf17b0 into evrardjp:master Feb 27, 2023
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.

None yet

2 participants