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

Adds support for Rocky Linux #40

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

digiserg
Copy link

No description provided.

@fauust
Copy link
Owner

fauust commented Jan 8, 2024

Hi @digiserg! Thanks for the PR.
I see that there are quite a lot's of changes in this PR but before reviewing it, I need to understand what this PR fixes/adds, at least the title seems confusing to me since Rocky Linux seems already supported to me.

Can you please explain shortly why you think this PR is needed and what it brings to the role?

- Support for setting the root user password
- It is possible to install additional packages required by MariaDB
- Configure MariaDB using `mariadb_options`
- Fixes users creation for servers requiring authentication
- Adds support for SSL
@digiserg
Copy link
Author

digiserg commented Jan 11, 2024

Hi @fauust

You've done a very good job with this role but after I started using I found a few things I require. I understand these are quite a lot of changes and I'll maintain my own fork if you're not happy with them but I thought I should contribute.

  • In Rocky Linux 9, ansible_os_family is set as Rocky and not RedHat. Hence the installation did not work for me
  • Adds support for setting the root user password
  • It is now possible to install additional packages required by MariaDB (in my case, I need mariadb-pam)
  • Configure MariaDB using the mariadb_options dictionary
  • Fixes users' creation for servers requiring authentication
  • Adds support for SSL

Thank you for the great work.

@fauust
Copy link
Owner

fauust commented Jan 11, 2024

You've done a very good job with this role but after I started using I found a few things I require. I understand these are quite a lot of changes and I'll maintain my own fork if you're not happy with them but I thought I should contribute.

The more people use and contribute to this role, the better it will become. So, if we can make it work for your needs, let's do it!

  • In Rocky Linux 9, ansible_os_family is set as Rocky and not RedHat. Hence the installation did not work for me

This surprises me, first because the role testing should fail in the CI, then because I have just tested and ansible_os_family is set as RedHat (but it may be related to a different version or installation method), see bellow:

podman run -it rockylinux:9 bash -c "dnf install ansible-core -y && ansible -m setup localhost | grep ansible_os_family"
[...]
        "ansible_os_family": "RedHat",
  • Adds support for setting the root user password

I am not against adding this feature to the role, but just for curiosity, and since the default root authentication method is unix socket (since MariaDB 10.4), why do you need it in your deployments? In any case, I would like that any new feature comes with the corresponding testing if possible.

  • It is now possible to install additional packages required by MariaDB (in my case, I need mariadb-pam)

What prevents you to use the mariadb_package var with something like (not tested):

mariadb_package:
  - mariadb-server
  - mariadb-pam

Adding a new var for extra package could be cleaner though.

  • Configure MariaDB using the mariadb_options dictionary

When possible, I am strongly against using dictionaries for variables in ansible, in my experience using raw variables is much easier and maintainable. But if you have good reason doing so, I could consider it.

  • Fixes users' creation for servers requiring authentication

Same comment as before, you could (and probably should) use unix socket here but you may have a very good reason not to.

  • Adds support for SSL

That could be a great feature to add, I am not sure how we could test this in the CI but I can probably help you. In any case and as said, I prefer that we use raw variable. Here I am not against adding a specific SSL block with a var like mariadb_ssl_raw.

Thank you for the great work.

🙏

@digiserg
Copy link
Author

Interestingly, my Rocky instances are in AWS using the AWS Marketplace:

❯ansible -m setup -u rocky -i 10.x.x.x all  | grep ansible_os_family
        "ansible_os_family": "Rocky",

I'm deploying to secure environments such as banking. root password is a must.

@fauust
Copy link
Owner

fauust commented Jan 11, 2024

Interestingly, my Rocky instances are in AWS using the AWS Marketplace:

❯ansible -m setup -u rocky -i 10.x.x.x all  | grep ansible_os_family
        "ansible_os_family": "Rocky",

If possible, can you give more information on the setup so I could test?

I'm deploying to secure environments such as banking. root password is a must.

Can you develop? root authentication via password does not bring any extra security, it's the contrary actually. See also https://mariadb.org/authentication-in-mariadb-10-4/

@grooverdan
Copy link

I'm deploying to secure environments such as banking. root password is a must.

The lack of a password doesn't mean its an empty password. So a password set is another exploit avenue.

https://mariadb.com/kb/en/authentication-plugin-unix-socket/#security

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.

3 participants