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

SELinux: allow keepalived to cat haproxy pid #47

Merged
merged 2 commits into from Jul 10, 2017

Conversation

major
Copy link
Contributor

@major major commented Jul 3, 2017

On CentOS 7, an SELinux denial occurs when keepalived tries to read
haproxy's PID file. This patch adds SELinux policy to allow this
activity and breaks the SELinux compilation part of the role into
a reusable set of tasks.

On CentOS 7, an SELinux denial occurs when keepalived tries to read
haproxy's PID file. This patch adds SELinux policy to allow this
activity and breaks the SELinux compilation part of the role into
a reusable set of tasks.
@evrardjp
Copy link
Owner

evrardjp commented Jul 4, 2017

I will spin up my centos tests today.

@evrardjp
Copy link
Owner

evrardjp commented Jul 4, 2017

My tests don't do integration tests of keepalived + haproxy, merely keepalived.
This PR passed my upgrade tests. Doing the greenfield ones right now.
If they pass, this will be merged and a new bugfix tag will be issued.

@@ -0,0 +1,23 @@
# Copyright 2017, Rackspace US, Inc.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this should be shipped into keepalived.
It's a good example so we could keep it, but I think it should live in OSA

args:
creates: /etc/selinux/targeted/active/modules/400/keepalived_ping/cil
chdir: /tmp/ansible-keepalived-selinux
- include: keepalived_selinux_compile.yml
Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with keeping this feature, this way this keepalived role stays an "all bolts included" keepalived deployment tool.
However, we should avoid feature bloat, and stick only to bare minimum.... (see next comment)

state: absent
when:
- '"keepalived_ping" not in selinux_modules.stdout'
- "keepalived_haproxy_pid_file"
Copy link
Owner

Choose a reason for hiding this comment

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

Therefore, I think these items should be coming from a variable, named keepalived_selinux_policy_files, with a default value of either [] or [ 'keepalived_ping'].

I can't adapt your PR, so could you please update it?

This variable could then be updated in OSA (with a group var for example), to compile the files.
OSA can provide additional files, like the haproxy pid one.

@evrardjp
Copy link
Owner

evrardjp commented Jul 4, 2017

The code passed my tests, but I'd rather see some of these features be optional, by either using a role as dependency, or making these SELinux bits running optionally depending on a var (see review above)

Keepalived role doesn't need HAProxy pid SELinux permissions.
Therefore the keepalived_haproxy_pid_file was removed from
the list of rules to compile.

However, flexibility could be advisable, and the list of rules
to compile should not be static, but instead be an overridable
list. By default, the list will compile the ping rule, as it
was done before, for retro-compatibility.

But a deployer can now provide path to files to compile.
@evrardjp evrardjp dismissed their stale review July 7, 2017 17:35

Updated.

@evrardjp evrardjp merged commit 8e215fa into evrardjp:master Jul 10, 2017
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