Add new type 'mode' for Unix permissions/umask #50035

dagwieers · 2018-12-17T17:26:23Z

SUMMARY

To avoid any dangerous situations with octal or integer values as Unix mode, it is better to always rely on using strings for Unix permissions or umasks. It adds clarity and uniformity, as the current ambiguous situation leads to unforeseen consequences (e.g. with indirected values being converted to integers). It would have been better if we always had only accepted string octal representation IMO.

This implements a deprecation warning if a string was not provided, but by Ansible v2.12 we will only accept string values for type mode. As a temporary measure we introduced a mode_str type to cover old str-type modes (whereas mode is using raw-type modes instead).

This PR also:

fix all examples doing it wrong

This fixes #43256

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

apt_repository, copy, file, get_url, htpasswd, ini_file, java_keystore, jenkins_plugin, lineinfile, openssl_pkcs12, replace, stat, template, udm_share

ansibot · 2018-12-17T17:28:19Z

cc @ptux
click here for bot help

ansibot · 2018-12-17T17:43:22Z

cc @EvanK @Mogztter @Spredzy @ahtik @bpennypacker @dhozac @felixfontein @gdelpierre @gundalow @jpmens @jtyr @mattclay @noseka1 @samdoran @sashka @sivel
click here for bot help

ansibot · 2018-12-17T18:12:02Z

cc @Lujeni @mattupstate @webknjaz
click here for bot help

felixfontein · 2019-02-26T21:40:10Z

Why not simply changing https://github.com/ansible/ansible/blob/devel/docs/bin/plugin_formatter.py#L150-L163 to output mode when it encounters mode_str? Then the modules don't have to end up on the ignore list.

mattclay · 2019-02-26T21:42:59Z

@sivel Do you have any thoughts on #50035 (comment) ?

dagwieers · 2019-02-26T21:45:06Z

@felixfontein A possibility as well. It's only ~~two~~ three files so I didn't want to make it more complex than it already is for ~~two~~ three files.

This PR alse fixes existing modules and examples.

thaumos · 2019-02-26T22:46:46Z

I want to make sure I understand this correctly. Please forgive me for any misunderstanding up front.

This change will be introducing a warning to a user who may be using an int in their playbooks for mode. So we suggest moving to a string as the new recommendation?

I'd also like to see a porting guide addition for this change. Also, @kustodian, we do need a lint rule change for this.

dagwieers · 2019-02-28T08:28:14Z

This change will be introducing a warning to a user who may be using an int in their playbooks for mode. So we suggest moving to a string as the new recommendation?

The documentation of the mode-parameter usually recommends using strings for various reasons. Rather than giving users insights in how YAML and Ansible work together to make this a big problem, requiring a string takes away most of the pain.

This is what we have in the documentation currently:

For those used to /usr/bin/chmod remember that modes are actually octal numbers. You must either add a leading zero so that Ansible's YAML parser knows it is an octal number (like 0644 or 01777) or quote it (like '644' or '1777') so Ansible receives a string and can do its own conversion from string into number. Giving Ansible a number without following one of these rules will end up with a decimal number which will have unexpected results

acozine

Overall I think this change promotes consistency and clarity. I did find a couple of points of confusion, noted below.

acozine · 2019-04-10T14:38:46Z

lib/ansible/modules/files/copy.py

@@ -477,7 +477,7 @@ def main():
            backup=dict(type='bool', default=False),
            force=dict(type='bool', default=True, aliases=['thirsty']),
            validate=dict(type='str'),
-            directory_mode=dict(type='raw'),
+            directory_mode=dict(type='mode'),


Why isn't the parameter mode also defined here in the argspec? It's listed in the docs but not here.

AnsibleModule() has an argument add_file_common_args, which will add common parameters such as mode to the argspec. The parametes which are added are the ones in FILE_COMMON_ARGUMENTS defined in module_utils/basic.py, which includes this change.

acozine · 2019-04-10T14:43:18Z

lib/ansible/modules/files/stat.py

@@ -155,8 +155,8 @@
        mode:
            description: Unix permissions of the file in octal


This needs to change as well.

acozine · 2019-04-10T14:44:22Z

lib/ansible/modules/packaging/os/apt_repository.py

@@ -36,6 +36,7 @@
    mode:
        description:
            - The octal mode for newly created files in sources.list.d


This one too.

gundalow · 2019-04-11T12:03:53Z

We've passed Core feature freeze for 2.8.
So how about we get this early in 2.9 and get feedback?

jamescassell · 2019-04-12T13:25:07Z

https://meetbot.fedoraproject.org/ansible-meeting/2019-02-26/ansible_core_irc_public_meeting.2019-02-26-19.02.log.html#1-28

ansibot · 2019-05-30T16:43:10Z

cc @rsmontero @xorel
click here for bot help

ansibot · 2019-09-17T16:35:43Z

cc @meerkampdvv
click here for bot help

jborean93 · 2022-01-26T19:52:25Z

When discussing this in the backlog meeting it seems like these changes here have been implemented by various other features over time and the changes here may not be needed anymore (or at least aren't needed to the same extent). Due to the age and the conflicts in the files we are going to close this PR.

dagwieers requested review from abadger and acozine December 17, 2018 17:26

dagwieers force-pushed the mode-type branch 2 times, most recently from 5f74734 to 22d01bc Compare December 17, 2018 17:42

dagwieers force-pushed the mode-type branch from 22d01bc to 13fe9f4 Compare December 17, 2018 17:46

Xaroth approved these changes Dec 17, 2018

View reviewed changes

dagwieers force-pushed the mode-type branch from 13fe9f4 to 59c66d3 Compare December 17, 2018 17:47

dagwieers mentioned this pull request Dec 17, 2018

Do not document buggy behavior. #49325

Merged

This comment has been minimized.

Sign in to view

ansibot added cloud and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 17, 2018

dagwieers force-pushed the mode-type branch from 59c66d3 to 729c231 Compare December 17, 2018 17:57

dagwieers force-pushed the mode-type branch 3 times, most recently from e6c81de to 42fe72f Compare December 17, 2018 18:31

Add new type 'mode' for Unix permissions/umask

546648d

This PR alse fixes existing modules and examples.

dagwieers force-pushed the mode-type branch from bd29227 to 546648d Compare February 26, 2019 22:05

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 27, 2019

quulah mentioned this pull request Feb 28, 2019

Octal mode incorrect when used with default filter #53104

Closed

FixItDad mentioned this pull request Mar 7, 2019

String mode parameter for openssl_pkcs12 #53476

Closed

ansibot added files Files category stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 8, 2019

acozine reviewed Apr 10, 2019

View reviewed changes

felixfontein mentioned this pull request Apr 22, 2019

docker_swarm_service secrets with gid/uid/mode #55581

Closed

ansibot added the has_issue label Jul 25, 2019

ansibot added collection Related to Ansible Collections work collection:community.general labels Apr 29, 2020

ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 6, 2020

jborean93 closed this Jan 26, 2022

ansible locked and limited conversation to collaborators Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new type 'mode' for Unix permissions/umask #50035

Add new type 'mode' for Unix permissions/umask #50035

dagwieers commented Dec 17, 2018 •

edited

Loading

ansibot commented Dec 17, 2018

ansibot commented Dec 17, 2018

This comment has been minimized.

ansibot commented Dec 17, 2018

felixfontein commented Feb 26, 2019

mattclay commented Feb 26, 2019

dagwieers commented Feb 26, 2019 •

edited

Loading

thaumos commented Feb 26, 2019

dagwieers commented Feb 28, 2019

acozine left a comment

acozine Apr 10, 2019

felixfontein Apr 10, 2019

acozine Apr 10, 2019

acozine Apr 10, 2019

gundalow commented Apr 11, 2019

jamescassell commented Apr 12, 2019

ansibot commented May 30, 2019

ansibot commented Sep 17, 2019

jborean93 commented Jan 26, 2022

		@@ -155,8 +155,8 @@
		mode:
		description: Unix permissions of the file in octal

Add new type 'mode' for Unix permissions/umask #50035

Add new type 'mode' for Unix permissions/umask #50035

Conversation

dagwieers commented Dec 17, 2018 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibot commented Dec 17, 2018

ansibot commented Dec 17, 2018

This comment has been minimized.

ansibot commented Dec 17, 2018

felixfontein commented Feb 26, 2019

mattclay commented Feb 26, 2019

dagwieers commented Feb 26, 2019 • edited Loading

thaumos commented Feb 26, 2019

dagwieers commented Feb 28, 2019

acozine left a comment

Choose a reason for hiding this comment

acozine Apr 10, 2019

Choose a reason for hiding this comment

felixfontein Apr 10, 2019

Choose a reason for hiding this comment

acozine Apr 10, 2019

Choose a reason for hiding this comment

acozine Apr 10, 2019

Choose a reason for hiding this comment

gundalow commented Apr 11, 2019

jamescassell commented Apr 12, 2019

ansibot commented May 30, 2019

ansibot commented Sep 17, 2019

jborean93 commented Jan 26, 2022

dagwieers commented Dec 17, 2018 •

edited

Loading

dagwieers commented Feb 26, 2019 •

edited

Loading