Add umask option for mount module #209

satken2 · 2021-06-12T10:47:00Z

SUMMARY

I added mode option for mount module.
When this module creates new directories for mount point, this value is used as permission of the directory.

Fixes #163, #178

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

mount

ADDITIONAL INFORMATION

When we want to set specific permission to the mount point directory, we can use file module.
However once the filesystem is mounted, the underlying directory is nearly inaccessible, and cannot be modified by file module.
This is a bit inconvenient, so I added mode option so that permission can be set before actual mount operation.

In #163, author requests to add umask option but when the mount point does not exist, this module creates directories, not files. So I think mode is more appropriate than umask.

changelogs/fragments/209_add_mode_for_mount.yml

plugins/modules/mount.py

Akasurde · 2021-06-14T07:03:04Z

cc @quidame @aminvakil @justjais

Co-authored-by: Abhijeet Kasurde <[email protected]>

…nto issue163

aminvakil

Thanks for adding this feature!

aminvakil · 2021-06-14T14:14:54Z

plugins/modules/mount.py

+            # Set permissions to the newly created mount point.
+            if mode is not None:
+                try:
+                    changed = module.set_mode_if_different(name, mode, changed)


This does not work on non-Linux operating systems? I've seen you run integration tests only on Linux systems when it was using chmod, I don't know about this function.
If so, please also mention this on parameter notes, and maybe warn user when the target os is not Linux?

aminvakil · 2021-06-14T14:15:46Z

tests/integration/targets/mount/tasks/main.yml

@@ -332,3 +332,44 @@
    - /tmp/myfs.img
    - /tmp/myfs
  when: ansible_system in ('Linux')
+


Please add a integration test which uses something like u+rw,g-wx,o-rwx or u=rw,g=r,o=r.

quidame · 2021-06-14T18:42:36Z

Hi @satken2, thanks for your contribution !

IMHO, this PR doesn't fix #163, as said in its description: the new parameter only applies to the mountpoint itself, not its parents, even if they're created by the module, that means that if /srv is empty and the user wants to mount some device at /srv/foo/bar/public with a public access to it (say mode: u=rwx,go=rx), this mountpoint will still be unreachable by most users because /srv/foo and /srv/foo/bar got mode 0700.

quidame · 2021-06-15T20:31:37Z

In #163, author requests to add umask option but when the mount point does not exist, this module creates directories, not files. So I think mode is more appropriate than umask.

I think there is a confusion here. umask applies to both files and directories. To directories, with the exact value of the mask; to files, with the value of the mask and by zeroing all executable bits.

satken2 · 2021-06-20T07:25:09Z

@quidame Thank you for your advice. Certainly, when creating directories recursively, I have to set permissions on all directories to resolve #163, as you point out. So I will fix my code.

I think there is a confusion here. umask applies to both files and directories. To directories, with the exact value of the mask; to files, with the value of the mask and by zeroing all executable bits.

The mount module only creates directories, so I don't think it is necessary to consider creating files, I guess.
So I added mode option.

quidame · 2021-06-21T01:07:56Z

tests/integration/targets/mount/tasks/main.yml

+    loop:
+    - /tmp/myfs.img
+    - /tmp/myfs
+  when: ansible_system in ('Linux')


why this restriction ? Does the new mode parameter not support other OSes ?

quidame · 2021-06-21T01:57:20Z

The mount module only creates directories, so I don't think it is necessary to consider creating files, I guess.
So I added mode option.

There is no need to create files to use umask. It applies to almost any file (regular files, directories, unix sockets, named pipes or fifos, character devices...) except symlinks, at creation time. Even if at the end it doesn't change anything to your code, please consider umask as a plausible option. It seems that you have discarded it on a misunderstanding.

For me, mode sounds more like something I (user) would have control over, at any time, not only at creation time. (ok, in this case, not at any time, but this could be every time the underlying directory used as mountpoint is not mounted, and exposed/unhidden). At least 2 modules make use of umask (https://github.com/ansible/ansible/blob/ec408a69f19052190f1766a357c882fc86cfeada/lib/ansible/modules/git.py#L136-L141, https://github.com/ansible/ansible/blob/ec408a69f19052190f1766a357c882fc86cfeada/lib/ansible/modules/pip.py#L106-L114). I don't know if there are modules using mode in a one-shot way, that is the purpose of umask, and if there are, I would probably consider them as confusing, and still suggest: please consider umask for this new option. If a contributor needs mode to implement mode of the root of the mounted filesystem, and mode is already used for something else that behaves like umask rather than mode, what to do ?

quidame

please update the changelog fragment - at least the name of the new parameter :); also, I don't understand the consecutive calls to os.umask(umask). All the rest looks good to me.

changelogs/fragments/209_add_mode_for_mount.yml

quidame · 2021-06-27T21:22:43Z

plugins/modules/mount.py

+                    except ValueError as e:
+                        module.fail_json(msg="umask must be an octal integer: %s" % (to_native(e)))
+                old_umask = os.umask(umask)
+                os.umask(umask)


what's the purpose of calling os.umask(umask) twice ?

Sorry, it's my mistake.

Co-authored-by: quidame <[email protected]>

satken2 · 2021-06-28T03:40:59Z

@quidame Thank you for your polite explanation and suggestion.
I have understood your intentions and changed my code to use umask instead of mode.

…nto issue163

verdurin · 2021-11-11T13:02:11Z

I think this will solve the problem I recently saw when creating CephFS mounts with posix.mount, so I will be happy to see it merged.

bcoca · 2024-07-12T16:10:07Z

plugins/modules/mount.py

+        If the mount point already exists, this parameter is not used.
+      - Note that after running this task and the device being successfully mounted,
+        the mode of the original directory will be hidden by the target device.
+    type: raw


I would change this to a 'strict string' to avoid all the 'octal/int' issues as we had to go through with mode in other modules.

satken2 added 2 commits June 12, 2021 19:24

Add mode option for mount module

52d040c

Add mode option for mount module

dce9f57

satken2 marked this pull request as ready for review June 13, 2021 01:06

Akasurde requested changes Jun 14, 2021

View reviewed changes

changelogs/fragments/209_add_mode_for_mount.yml Outdated Show resolved Hide resolved

plugins/modules/mount.py Outdated Show resolved Hide resolved

plugins/modules/mount.py Outdated Show resolved Hide resolved

plugins/modules/mount.py Outdated Show resolved Hide resolved

satken2 and others added 5 commits June 14, 2021 19:38

Fix description of the parameter

d7735d3

Co-authored-by: Abhijeet Kasurde <[email protected]>

correct changelog fragment

30c7360

Co-authored-by: Abhijeet Kasurde <[email protected]>

Add version_added parameter to documentation

f919496

Merge branch 'issue163' of https://github.com/satken2/ansible.posix i…

6663bf5

…nto issue163

Add mode option for mount module

d9f85d3

aminvakil reviewed Jun 14, 2021

View reviewed changes

quidame reviewed Jun 21, 2021

View reviewed changes

satken2 added 2 commits June 27, 2021 19:56

Add umask option for mount module

c14f4d7

Add umask option for mount module

d6ae098

satken2 force-pushed the issue163 branch from 009ba45 to d6ae098 Compare June 27, 2021 14:56

satken2 added 2 commits June 28, 2021 00:02

Add umask option for mount module

d2c5f21

Add umask option for mount module

f765706

satken2 changed the title ~~Add mode option for mount module~~ Add umask option for mount module Jun 27, 2021

quidame suggested changes Jun 27, 2021

View reviewed changes

Add umask option for mount module

b7424f4

Co-authored-by: quidame <[email protected]>

satken2 added 2 commits June 28, 2021 12:44

Add umask option for mount module

5845277

Merge branch 'issue163' of https://github.com/satken2/ansible.posix i…

3dd7712

…nto issue163

quidame approved these changes Jun 28, 2021

View reviewed changes

bcoca reviewed Jul 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add umask option for mount module #209

Add umask option for mount module #209

satken2 commented Jun 12, 2021

Akasurde commented Jun 14, 2021

aminvakil left a comment

aminvakil Jun 14, 2021

aminvakil Jun 14, 2021

quidame commented Jun 14, 2021 •

edited

Loading

quidame commented Jun 15, 2021 •

edited

Loading

satken2 commented Jun 20, 2021

quidame Jun 21, 2021

quidame commented Jun 21, 2021 •

edited

Loading

quidame left a comment

quidame Jun 27, 2021

satken2 Jun 28, 2021

satken2 commented Jun 28, 2021

verdurin commented Nov 11, 2021

bcoca Jul 12, 2024

Add umask option for mount module #209

Are you sure you want to change the base?

Add umask option for mount module #209

Conversation

satken2 commented Jun 12, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Akasurde commented Jun 14, 2021

aminvakil left a comment

Choose a reason for hiding this comment

aminvakil Jun 14, 2021

Choose a reason for hiding this comment

aminvakil Jun 14, 2021

Choose a reason for hiding this comment

quidame commented Jun 14, 2021 • edited Loading

quidame commented Jun 15, 2021 • edited Loading

satken2 commented Jun 20, 2021

quidame Jun 21, 2021

Choose a reason for hiding this comment

quidame commented Jun 21, 2021 • edited Loading

quidame left a comment

Choose a reason for hiding this comment

quidame Jun 27, 2021

Choose a reason for hiding this comment

satken2 Jun 28, 2021

Choose a reason for hiding this comment

satken2 commented Jun 28, 2021

verdurin commented Nov 11, 2021

bcoca Jul 12, 2024

Choose a reason for hiding this comment

quidame commented Jun 14, 2021 •

edited

Loading

quidame commented Jun 15, 2021 •

edited

Loading

quidame commented Jun 21, 2021 •

edited

Loading