ansible/ansible — pull request #8380 by astorije (2014-07-31 14:41:04+00:00)
Title: Refactoring and recursive support for the ACL module

This PR enables the support for recursive ACLs. Some refactoring was needed to make this work.

Other things were fixed along the way (and other things got probably broken, but that's what reviews are for, right?).

Note that during my work on this PR, I had a discussion on IRC with @bcoca and I realized that I am currently breaking BSD support. However, from this point, I arranged my refactoring so that a compatibility fix/extension would not be a major pain. So it might be worth to **wait before merging** this until I get feedback and apply fixes.

## Advantages of the refactoring

- Permissions do not need to be normalized anymore using the `--test` checking system

- `--test` does not check using `getfacl`, so the code is less verbose and less likely to introduce a parsing error (since there is actually - almost - no parsing)

- `--test` makes `--recursive` doable at no cost, and checking the `changed` status is neither overly complicated or slow

- `default` can now be used on acl queries as well

- [`X` (`capital-x` / *Special execute*)](https://en.wikipedia.org/wiki/Chmod#Symbolic_modes) is now supported in the permissions with no extra code

## Inconvenients of the refactoring

- `--test` does not seem to be supported by BSD's `setfacl` (I learned that afterwards). However, this can now be solved in the `acl_changed` function only without affecting the rest of the file (hopefully). Suggestions below.

- Performance-wise, there is one super small caveat:
- Before my changes, the steps of the module were the following:
- If there no changes on the ACLs of a file to be applied:
1. `a = getfacl`
2. compare `a` and the acls to be set
3. output `a` in JSON
- If there are some:
1. `a = getfacl`
2. compare `a` and the acls to be set
3. `setfacl`
4. `a_new = getfacl`
5. output `a_new` in JSON
- After my :changes
- If there no changes on the ACLs of a file to be applied:
1. `setfacl --test`
2. `a = getfacl`
3. output `a` in JSON
- If there are some:
1. `setfacl --test`
2. `setfacl`
3. `a_new = getfacl`
4. output `a_new` in JSON

Hence we can see that the cost (in terms of calls to `setfacl` and `getfacl`) is one extra `setfacl --test` when there are no changes. However, since there is no manual comparison which will take time in recursive mode.

## General notes (and motivations) to help for the review

- `normalize_permissions` was causing a bug when using entry: `permissions` was needed to avoid an exception (due to call to `normalize_permissions` in `main`) but `entry` and `permissions` were incompatible

- `normalize_permissions` did not support `X` (`capital-x`) and its support would cause problem with the former checking system (since `X` itself is not stateless)

- `_run_acl` was making the usage of `--test` impossible because for the last line returned when using `--test` is relevant and not empty. It now makes sure that the last line is empty before stripping it.

- 2 new builder helpers `build_entry` and `build_command` now factorize some recurring code

- `acl_changed` is a function meant to test if a command changes ACLs. The checking can be extended there rather than in the `main` function

- I removed the `get_acls`, `set_acl` and `rm_acl` wrappers for the following reasons:
- They were used only once each (at least in the context of this PR)
- They only contained one call to `build_command` and one call to `run_acl` in the end
- The command is already built when it is `--test`ed before so I reuse the command rather than rebuilding it

- `_run_acl` is now `run_acl` because it is not a hidden/private function anymore, it is used in the `main` function multiple times

## Suggestions for Linux / BSD support

- The `follow` argument sets `-h`, which is supported by BSD only and not by Linux. To my understanding, the corresponding argument is `-L`, `--logical`. If so, I can add a condition in `build_command`. Can you confirm?

- Before my PR, both BSD and Linux were supported, `--recursive` was not. My PR breaks support for BSD because of its lack of `--test`, but adds support for `--recursive` on Linux. I suggest that a condition checks if this is running on BSD and if so, that `changed` is **always** true when using `recursive=yes`. When not using `recursive=yes`, we can either do the same or replicate the old code for checking state. What I described earlier (notably the `X` bit) will still be a problem.

## TODOs before possible merging

- I didn't know whether to choose `recurse` or `recursive` as a keyword. The former one is used in the `file` module but I chose the latter one to stay close to the `setfacl` command. Any preference on that?

- Although I added a documentation bit, I did not specify the version as I prefer your review before. What should I set?

- The documentation should mention the differences BSD support if there is a difference (for example no support for `recursive` on BSD)

- `build_entry` can be simplified down to one line if you accept that the `entry` looks like `user:someone:` rather than `user:someone` when `state=absent`. Both are POSIX valid. Right now `user:someone:---` is displayed, which is not a valid `entry` with `-x`.

- `state=absent` and `permissions` should be incompatible (akin `entry` and `permissions`) since `setfacl` cannot work with the `permissions` bit set

- The formerly existing `run_acl(module, command, False)`, called when `state == 'absent'` was there (if I understood properly) to ignore the error when not setting an `entity`, and therefore this was reported as a success but did not do anything internally. This combination (no `entity` + `state=absent`) now produces an error. A requirement should be added so that this combination of arguments is not possible, since `setfacl` does not accept them.

- In the `main` function, there is:
```python
module = AnsibleModule(
argument_spec=dict(
# ...
entry=dict(required=False, etype='str'),
# ...
)
)
```
Shouldn't it be `etype='str'` instead? Why `etype`?