ANXS / postgresql

Fairly full featured Ansible role for Postgresql.
http://anxs.io/
MIT License
850 stars 573 forks source link

Become root when installing dependencies #376

Closed Ichimonji10 closed 1 year ago

aoyawale commented 6 years ago

as far as I know this whole role is run as root first then changes to the pgsql user when needed then returns as root so this is not needed.

Ichimonji10 commented 6 years ago

The entire ANXS.postgresql role is supposed to be run as root? If so, isn't that a design bug? Or do you mean something else?

FWIW, this PR was created because I ran ANXS.postgresql against a remote host, where the user used for SSH connections was non-root and had sudo privileges.

bmbouter commented 6 years ago

I think being able to run as non-root would be an improvement. It would be good if in places where the playbook needs root, it would specifically becomes it there. Not having to specify to run the entire role as root is one less requirement on users. This is backwards compatible too.

I didn't see it specified in the docs that it needs root or not. Either way, that might also be helpful too.

aoyawale commented 6 years ago

yes, it does. Your doing a lot of system level changes. This means either ssh in as root or as another user that can escalate privileges. So if you look at the tasks, you're installing packages, changing systemd, init files etc. Adding become root/sudo in every tasks is a waste of time and unnecessary. The parts that are not run as root/sudo are the postgres parts where it becomes pgsql to handle pgsql tasks since you can't be root/sudo on postgres. This is also how ansible works.

aoyawale commented 6 years ago

you don't need to specify anywhere that you need root. You should know what you need by reading the ansible documents and decide if you need to escalate privileges or not. So if you are running this from ansible core or awx, you can easily escalate your privileges when you run the playbook. There is no need to do it on every tasks. When it comes to this PR this is not needed.

bmbouter commented 6 years ago

@jlozadad I hear you; all of your environments work that way so I can see how this isn't useful to you. I'm a user of this role, and I see value in it.

In terms of documenting the root requirement, I agree the Ansible docs do guide the role author to make a choice for the users if it should be run as root. What I'm confused about is how users could know what choice you made without it being documented.

The PR is posted so if you accept it that would be cool. If you don't accept it that is your choice. Thanks for the role either way. It's really helpful.

Ichimonji10 commented 6 years ago

Adding become root/sudo in every tasks is a waste of time and unnecessary.

This role already specifies become in a number of places. If "adding become root/sudo in every tasks is a waste of time and unnecessary," it would be good to drop these directives. Doing so would let the code base be self-consistent, with the possible side-effects of improving user-friendliness and squashing some latent bugs.

$ git grep become:
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: yes
tasks/configure.yml:  become: true
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/databases.yml:  become: yes
tasks/install_dnf.yml:    become: yes
tasks/install_yum.yml:    become: yes
tasks/schemas.yml:  become: yes
tasks/users.yml:  become: yes
tasks/users_privileges.yml:  become: yes
tests/docker/site.yml:  become: false
tests/docker/site.yml:  become: false
tests/playbook.yml:  become: yes

That said, I disagree with the notion that explicitly escalating privileges on specific tasks is a harmful design pattern. There are two benefits to escalating privileges only when needed:

gclough commented 5 years ago

Hi @Ichimonji10 , I notice this has stalled... and I'm wondering if there's a fault, or this is just making things safer?

I've always run the role as a standard user which has "sudo" privs, and it seems to work OK.

Ichimonji10 commented 5 years ago

My goal in filing this issue is to make this role safer. Better to fix an issue before it bites someone than afterwards.

I don't recall whether I was able to run this role as a user who merely had sudo privileges. It's been quite some time since I've had to use this role, as I no longer work on a project which uses this.

nchudleigh commented 5 years ago

This seems like a fine change to me I agree with the sentiments put forward by @Ichimonji10 and @bmbouter

maglub commented 5 years ago

The whole role is meant to be run as non root, that is why there are "become" statemens in other tasks.

In my view, the lack of thereof (hence this pull request) is a bug. Most likely this has not been addressed before as other users that need "yum" probably ran the whole role as root.

In my view, we should merge this into master.

maglub commented 5 years ago

Hi @Ichimonji10 , I notice this has stalled... and I'm wondering if there's a fault, or this is just making things safer?

I've always run the role as a standard user which has "sudo" privs, and it seems to work OK.

Did you run it on Fedora or CentOs? If you, as I, are on Ubuntu or Debian, you would not be affected by the tasks in this pull request.

Ichimonji10 commented 5 years ago

Did you run it on Fedora or CentOs? If you, as I, are on Ubuntu or Debian, you would not be affected by the tasks in this pull request.

At the time I wrote this pull request, I would have tested it against Fedora and/or RHEL 7.

maglub commented 5 years ago

Sorry Ichimonji10, my question was actually directed to @gclough. I suspect that he has not ran into your elevated rights issue that you are correcting with this pull request, as he might not have been running on Fedora and/or RHEL.

gclough commented 5 years ago

Is this related to issue #434