Optimize evolinux-users

Ghost commented

2019-08-07 00:11:16 +02:00

First-time contributor

Even a simple check can require more memory and time from an ansible role.

I moved around some tasks to turn 3 tasks * (length evolinux-users) into tasks that only execute once. I ignored some comments that made no sense to me, so I'll need some validation that the comments were indeed wrong.

Even a simple check can require more memory and time from an ansible role. I moved around some tasks to turn 3 tasks * (length evolinux-users) into tasks that only execute once. I ignored some comments that made no sense to me, so I'll need some validation that the comments were indeed wrong.

benpro was assigned by Ghost

2019-08-07 00:11:16 +02:00

Ghost added the

enhancement

label 2019-08-07 00:11:16 +02:00

benpro commented

2019-08-19 16:13:30 +02:00

Contributor

I'm not sure why you assigned me.
I cannot review this code.
Maybe @jlecour can check this?

I'm not sure why you assigned me. I cannot review this code. Maybe @jlecour can check this?

benpro removed their assignment 2019-08-19 16:13:34 +02:00

jlecour commented

2019-09-07 13:31:12 +02:00

Owner

I didn't run tests yet, but the "sudo" part looks good to me. The sudoers template doesn't need to be run for each user so we'll save a bunch of executions.

I'm much more skeptical regarding the SSH part. The verify AllowGroups directive and verify AllowUsers directive task seem to have disappeared. And the comment stating that the check needs to be run for each user might not be there by mistake. I vaguely remember that I've put it there intentionally.

Could you clarify what kind of tests you've run to verify that there is no regression ?

I didn't run tests yet, but the "sudo" part looks good to me. The sudoers template doesn't need to be run for each user so we'll save a bunch of executions. I'm much more skeptical regarding the SSH part. The `verify AllowGroups directive` and `verify AllowUsers directive` task seem to have disappeared. And the comment stating that the check needs to be run for each user might not be there by mistake. I vaguely remember that I've put it there intentionally. Could you clarify what kind of tests you've run to verify that there is no regression ?

Ghost commented

2019-09-11 16:20:42 +02:00

Author

First-time contributor

I removed them from the loop after a lot of consideration.

The verify allow groups and verify allow users checks are done in the ssh.yml file.
The second verify allow groups in ssh_allow_groups.yml is clearly useless, as the file is not in a loop and just straight up included. It could easily be inlined.
If you check for the allow users / match users existence in ssh.yml, then you know it is not present for the first user in the loop, but you must still rerun the check to see if it is there in subsequent loops. By making the add allow users / add match users task register a variable that says it has been done previously in the run, we can turn those superfluous tasks into when conditions.

This has not been extensively tested, but I believe my rational sound, the main thing I'm unsure about is testing on added_allow_user.changed. Perhaps there exists a better condition.

I removed them from the loop after a lot of consideration. 1. The verify allow groups and verify allow users checks are done in the ssh.yml file. 2. The second verify allow groups in ssh_allow_groups.yml is clearly useless, as the file is not in a loop and just straight up included. It could easily be inlined. 3. If you check for the allow users / match users existence in ssh.yml, then you know it is not present for the first user in the loop, but you must still rerun the check to see if it is there in subsequent loops. By making the add allow users / add match users task register a variable that says it has been done previously in the run, we can turn those superfluous tasks into when conditions. This has not been extensively tested, but I believe my rational sound, the main thing I'm unsure about is testing on added_allow_user.changed. Perhaps there exists a better condition.

Ghost commented

2019-09-11 16:33:36 +02:00

Author

First-time contributor

I'm fairly certain we could actually make this much faster and simpler by using map and join, but I'm not sure how we could make sure it does not overwrite manually added allow users.

{{ users|map(attribute='name')|join(',') }}

Doing this would allow us to reduce the allow users and match users from 2*n tasks to 2 tasks.

I'm fairly certain we could actually make this much faster and simpler by using map and join, but I'm not sure how we could make sure it does not overwrite manually added allow users. ``` {{ users|map(attribute='name')|join(',') }} ``` Doing this would allow us to reduce the allow users and match users from 2*n tasks to 2 tasks.

jlecour commented

2019-09-11 17:04:45 +02:00

Owner

OK, so I need to take a more global look to it, and not just your diff.
Thanks for your explanation.

Regarding the map/join, as you figured this is not possible because users are often added beyong evolinux users.

OK, so I need to take a more global look to it, and not just your diff. Thanks for your explanation. Regarding the map/join, as you figured this is not possible because users are often added beyong evolinux users.

Ghost commented

2019-09-11 17:55:27 +02:00

Author

First-time contributor

Map / join works if you're adding the statement though. See my two latest commits.

Ghost commented

2019-09-11 20:55:34 +02:00

Author

First-time contributor

Further optimised the sudo tasks in the case of debian 9 or later, you only need to create the group once. I also inlined the jessie file as there was only one task in it.

Ghost commented

2019-09-20 15:36:19 +02:00

Author

First-time contributor

I realise now that we are missing one case:

Servers with AllowGroups activated should also use Match Group instead of Match User. I do not believe we need as much stringent verification as with AllowGroups / AllowUsers, since multiple statements can coexist.

I realise now that we are missing one case: Servers with AllowGroups activated should also use Match Group instead of Match User. I do not believe we need as much stringent verification as with AllowGroups / AllowUsers, since multiple statements can coexist.

Ghost commented

2019-09-24 17:18:50 +02:00

Author

First-time contributor

I made a fix to the Match User stuff, but the Match Group stuff is not supported yet.

Ghost commented

2019-09-24 17:22:24 +02:00

Author

First-time contributor

I've meeting an error with map|join, for some reason it is not working. If somebody has time to check it out before me, here is the error:

The task includes an option with an undefined variable. The error was: 'str object' has no attribute 'name'\n\nThe error appears to be in '/home/pmarchand/ansible-roles/evolinux-users/tasks/ssh.yml': line 43, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: \"Add AllowUsers sshd directive with all users\"\n  ^ here\n

The syntax for my join operation seems to be correct, but maybe I misunderstood where it could be used ?

I've meeting an error with map|join, for some reason it is not working. If somebody has time to check it out before me, here is the error: ``` The task includes an option with an undefined variable. The error was: 'str object' has no attribute 'name'\n\nThe error appears to be in '/home/pmarchand/ansible-roles/evolinux-users/tasks/ssh.yml': line 43, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: \"Add AllowUsers sshd directive with all users\"\n ^ here\n ``` The syntax for my join operation seems to be correct, but maybe I misunderstood where it could be used ?

jlecour commented

2022-06-06 18:27:33 +02:00

Owner

I'm pretty sure this has been done elsewere and this PR is obsolete.

jlecour closed this pull request

2022-06-06 18:27:33 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Optimize evolinux-users #78

Pull request closed