Optimize evolinux-users #78

Open
pmarchand wants to merge 9 commits from simplify-evolinux-users into unstable
Owner

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 pmarchand 2 years ago
pmarchand self-assigned this 2 years ago
pmarchand added the
enhancement
label 2 years ago

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 2 years ago
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 ?
Poster
Owner

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.

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.
Poster
Owner

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.
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.
Poster
Owner

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

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

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.

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.
Poster
Owner

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.
Poster
Owner

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

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

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 ?
This pull request has changes conflicting with the target branch.
evolinux-users/tasks/main.yml
evolinux-users/tasks/ssh.yml
evolinux-users/tasks/sudo.yml
evolinux-users/tasks/sudo_jessie.yml
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.