Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRANSFORM_INVALID_GROUP_CHARS doesn't document valid group patterns #56930

Open
xarses opened this issue May 24, 2019 · 100 comments · May be fixed by #55474
Open

TRANSFORM_INVALID_GROUP_CHARS doesn't document valid group patterns #56930

xarses opened this issue May 24, 2019 · 100 comments · May be fixed by #55474

Comments

@xarses
Copy link

@xarses xarses commented May 24, 2019

SUMMARY

With the addition of the TRANSFORM_INVALID_GROUP_CHARS. Other than reading the source, it was not clear which characters must be avoided going forward, only that the warning (with -vvvv) points out which characters you currently use that are invalid.

Please clarify that you are pushing the names to be valid python vars. This is missing from the documentation for the cfg option, warning and online documentation

(d241794#diff-b77962b6b54a830ec373de0602918318R122)

There appears to be no mention of this on https://docs.ansible.com/ansible/latest/porting_guides/porting_guide_2.8.html either.

ISSUE TYPE
  • Documentation Report
COMPONENT NAME

group

ANSIBLE VERSION
ansible 2.8.0
  config file = /home/awoodward/ansible-skynet/ansible.cfg
  configured module search path = [u'/home/awoodward/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.5 (default, Apr  9 2019, 14:30:50) [GCC 4.8.5 20150623 (Red Hat 4.8.5-36)]
CONFIGURATION

n/a

OS / ENVIRONMENT

n/a

ADDITIONAL INFORMATION

n/a

@ansibot
Copy link
Contributor

@ansibot ansibot commented May 24, 2019

Files identified in the description:

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot
Copy link
Contributor

@ansibot ansibot commented May 24, 2019

@danielmotaleite
Copy link

@danielmotaleite danielmotaleite commented May 28, 2019

i started to get this warning but found no references in the porting guide and no references how or what to fix.

most of my warnings are from the ec2.py, where the instance_id used the - (eg: i-033f62b586143dff7) and the regions (eg: eu-central-1c) , so we have no real fix for this ones

Finally, this broke some of my playbooks, where i used when: ansible_hostname in groups['varnish'] and the ansible_hostname is varnish-eu-central-1c-001 .
In the past this worked fine, now i need to use inventory_hostname to get varnish_eu_central_1c_001 and get a match vs the groups['varnish']

So this needs at least and urgently a warning in the porting guide that inventory_hostname and groups[] may be returning different data

@bcoca bcoca self-assigned this May 28, 2019
@bcoca bcoca linked a pull request that will close this issue May 28, 2019
@ssbarnea
Copy link
Contributor

@ssbarnea ssbarnea commented May 31, 2019

What was the reasoning behind dropping the dash from group names? I really struggle to find good reason for that, especially as this will require lots of code refactoring.

@sivel
Copy link
Member

@sivel sivel commented May 31, 2019

@ssbarnea For one thing, we are making a push to only allow variable names, and other similar keys, that are valid python identifiers. To explain a little further about group names, it causes a problem for users trying to use "dot syntax" such as groups.foo-group, which doesn't do what the user expects. The number of issues and support requests caused by small problems like these has caused us to go down a path to safe guard names to ensure that problems like this do not occur.

For those wanting to keep what we consider invalid characters, can opt out of this functionality.

@caleb-s-cullen
Copy link

@caleb-s-cullen caleb-s-cullen commented May 31, 2019

What must we do to opt out of this functionality? Our local Ansible deployment scripts are littered with hyphen-containing group names. We don't use them with dot notation, of course. But changing all of them would be a truly monumental task. I would prefer to opt out and at the same time encourage my team to avoid using hyphens in the future and where possible to convert hyphens to underscores, tho' that last part is not always as straightforward as it might seem.

So, does one simply set force_valid_group_names = false in ansible.cfg ? That seems right based on d241794#diff-fd24ad93fbc32f454761746c1ac908f2

@jamescassell
Copy link
Contributor

@jamescassell jamescassell commented Jun 3, 2019

What must we do to opt out of this functionality? Our local Ansible deployment scripts are littered with hyphen-containing group names. We don't use them with dot notation, of course. But changing all of them would be a truly monumental task. I would prefer to opt out and at the same time encourage my team to avoid using hyphens in the future and where possible to convert hyphens to underscores, tho' that last part is not always as straightforward as it might seem.

So, does one simply set force_valid_group_names = false in ansible.cfg ? That seems right based on d241794#diff-fd24ad93fbc32f454761746c1ac908f2

export ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS=never or export ANSIBLE_TRANSFORM_INVALID_GROUP_CHARS=ignore -- the latter is not in the docs yet: #57318

@al-x
Copy link
Contributor

@al-x al-x commented Jun 4, 2019

Thanks, James. Since people are coming to this issue to follow-up on the warning message, I'm including information I think may be useful:

To disable the ≥2.10 group name auto-transformation more portably/permanently until such time as you may be ready to clear out invalid groups from your inventory, add force_valid_group_names = never to the [defaults] INI-section of ansible.cfg.

To see all groups and invalid characters which trigger the warning (perhaps so you can target them for phase-out), you can do something like this ansible CLI no-op:

ansible-inventory -vvvv --host=localhost 2>&1 | grep replacing

These invalid characters are (as of 2019-06-04) defined as a constant, INVALID_VARIABLE_NAMES, in:
https://github.com/ansible/ansible/blob/devel/lib/ansible/constants.py#L119
as '^[\d\W]|[^\w]',
that is: any leading non-alpha character OR any character other than alpha-numeric and underscore.
(I hope I got that right)

If you find deprecation warnings annoying, you may also disable permanently them for any ansible- command or the ansible ad-hoc command by adding deprecation_warnings = False to the same [defaults] section of ansible.cfg, but I would recommend against that (since you may miss important news), and instead use inline shell environment variables like this:
ANSIBLE_DEPRECATION_WARNINGS=False ansible-inventory --host=localhost

The inventory parsing [WARNING]s won't go away, however. There's no specific config or env var to turn off all warnings (yet?), but if it really bugs you, you can just send all stderr to /dev/null (insert "best practice" caveats here):

2>/dev/null ansible-inventory --host=localhost

Hope this helps someone, somewhere.

@ssbarnea
Copy link
Contributor

@ssbarnea ssbarnea commented Jun 4, 2019

I only find deprecation warning messages annoying when they do not provide a migration path. Considering that the space is limited and that the remediation is likely to need update I would find very useful to provide links to tickets which can document solutions, workarounds,...

An approach like this could save extra work needed for improving incomplete warnings messages as we would not have to update the message, backport it to few versions back.

PS. Disabling deprecations warnings is something I would not recommend to anyone, maybe only if a project is already facing its ultimate fate ;)

@bpleines
Copy link
Contributor

@bpleines bpleines commented Jun 5, 2019

i started to get this warning but found no references in the porting guide and no references how or what to fix.

most of my warnings are from the ec2.py, where the instance_id used the - (eg: i-033f62b586143dff7) and the regions (eg: eu-central-1c) , so we have no real fix for this ones

Finally, this broke some of my playbooks, where i used when: ansible_hostname in groups['varnish'] and the ansible_hostname is varnish-eu-central-1c-001 .
In the past this worked fine, now i need to use inventory_hostname to get varnish_eu_central_1c_001 and get a match vs the groups['varnish']

So this needs at least and urgently a warning in the porting guide that inventory_hostname and groups[] may be returning different data

I'd like to echo statement about the warning being generated by the EC2 dynamic inventory script. I noticed that there is an ec2.ini configuration setting to disable grouping hosts by instance_id (group_by_instance_id = False), but setting that didn't resolve the warning for me like I anticipated it would - I made sure I cleared the local inventory cache.

Any workarounds for EC2 dynamic inventory specifically?

@jamescassell
Copy link
Contributor

@jamescassell jamescassell commented Jun 5, 2019

These invalid characters are (as of 2019-06-04) defined as a constant, INVALID_VARIABLE_NAMES, in:
https://github.com/ansible/ansible/blob/devel/lib/ansible/constants.py#L119
as '^[\d\W]|[^\w]',
that is: any leading non-alpha character OR any character other than alpha-numeric and underscore.
(I hope I got that right)

Sounds accurate to me. You should submit a docs PR with that info.

If you find deprecation warnings annoying, you may also disable permanently them for any ansible- command or the ansible ad-hoc command by adding deprecation_warnings = False to the same [defaults] section of ansible.cfg, but I would recommend against that (since you may miss important news), and instead use inline shell environment variables like this:
ANSIBLE_DEPRECATION_WARNINGS=False ansible-inventory --host=localhost

The inventory parsing [WARNING]s won't go away, however. There's no specific config or env var to turn off all warnings (yet?), but if it really bugs you, you can just send all stderr to /dev/null (insert "best practice" caveats here):

The undocumented ignore option provides this functionality. Docs PR here: #57318

Starting in 2.8.2, this deprecation warning will be squashed if you explicitly set any of choices.

@maglub
Copy link

@maglub maglub commented Jun 5, 2019

Where does the ansible dev team discuss this type of decisions? It is very hard for us users to understand the reasoning to this. If it is pure "python style" reasoning, rather than a practical reasoning, it might be worth reconsidering? If a dash in group names break things in future releases of ansible, it might rather be a problem with the implementation, more than the naming of a group?

To me, this sounds more like a cosmetic change, than something that has been thought through properly.

A group name is not a variable name, it is the content of such. A hyphen/dash is just a character, that also happens to be an extremely popular way of grouping information in a naming convention. Compared to the exclamation point or the star, it does not have a special meaning in a limit clause.

The cost to mitigate this issue is enorm, given that thousands of sites has to not only change the group names in the inventories, but also go through all their playbooks and home grown roles, and test them all again.

If there is any way at all for "peasants" to make their voice heard, I would love to chip in my opinion and try and understand how this idea came to be.

@jamescassell
Copy link
Contributor

@jamescassell jamescassell commented Jun 5, 2019

I've come to understand that the change was made to ansible because users made mistakes such as trying to use groups.group-name rather than groups['group-name']. AIUI, it is a change purely for the purpose of reducing support issues. (I'm personally opposed to the change.)

The old behavior will not go away; it will just become unavailable without explicitly choosing the old behavior.

@maglub
Copy link

@maglub maglub commented Jun 5, 2019

Sad to hear.

My use case, is that I am embedding the command "ansible-inventory" in a Vagrant file, in a way where it is impolite to put things in ansible.cfg, and that it would help to be able to be able to override the behavior as a command line option (not environment variable).

Usually changes like this are due to good intentions, but might not always lead to the outcome one has in mind.

vitabaks added a commit to vitabaks/postgresql_cluster that referenced this issue Jun 5, 2019
…e (postgres-cluster)"

[DEPRECATION WARNING]: The TRANSFORM_INVALID_GROUP_CHARS settings is set to allow bad characters in group names by default, this will change, but still be user configurable on deprecation. This feature
will be removed in version 2.10.

ansible/ansible#56930
@MarkusTeufelberger
Copy link
Contributor

@MarkusTeufelberger MarkusTeufelberger commented Jun 7, 2019

My issue with this change is that group names now become somewhat "special" - dashes are allowed in host names, but not in group names which makes it a bit weird considering at the start of a playbook in the hosts: section I could write host and/or group names.

sukujgrg added a commit to sukujgrg/kubernetes-do-ansible that referenced this issue Dec 16, 2019
@radioactive73
Copy link

@radioactive73 radioactive73 commented Dec 23, 2019

My understanding is you can't use the dot notation for variables with spaces anyways, and while I never create variables with spaces there are unfortunately lots of vendors that return dictionary keys with spaces through json API responses. The sensible option to me seems to switch to the square bracket notation. Hopefully setting force_valid_group_names to ignore doesn't cause ill effects further down the road, who knows what else is planned in the future with this change.

@dvntstph
Copy link

@dvntstph dvntstph commented Dec 31, 2019

This is a pretty dreadful decision particularly when it comes to working with dynamic inventories as with Openstack (and AWS).
Instance names and metadata keys containing "forbidden characters" are often returned as inventory items and/or group variables from the underlying cloud. This is going to make life hell for a lot of Openstack (and AWS) admins trying to manage their fleets using meta tags and or instance ID's like:
instance-8ca09c33-f255-440f-9544-b0ab318c79d9
meta-os_ubuntu

@eightseventhreethree
Copy link

@eightseventhreethree eightseventhreethree commented Jan 13, 2020

Ansible devs should be taking @geerlingguy opinion seriously. He is one of the biggest contributors to Ansible Galaxy and his Roles are consumed by tons of people. I think this change is really bad for people who have thousands of hosts named like: $env-$role-[0..99]. Are we supposed to go rename everything to appease our Ansible overlords?

@nathonfowlie
Copy link

@nathonfowlie nathonfowlie commented Feb 13, 2020

@ssbarnea For one thing, we are making a push to only allow variable names, and other similar keys, that are valid python identifiers. To explain a little further about group names, it causes a problem for users trying to use "dot syntax" such as groups.foo-group, which doesn't do what the user expects. The number of issues and support requests caused by small problems like these has caused us to go down a path to safe guard names to ensure that problems like this do not occur.

For those wanting to keep what we consider invalid characters, can opt out of this functionality.

How long will users be allowed to opt out of this behaviour? Will there be a permanent configuration option to disable this behaviour in all ansible versions moving forward, or will it only be supported until 2.11? I'd be happy for the option to be turned on by default, as long as I always have the option to turn it off.

If this becomes a hard restriction in 2.11+, then you're likely going to find yourself losing customers that are bound by the constraints of their organisation (not all ansible users have the power to dictate the naming conventions used by their company). It seems this change also would introduce a significant challenge for those using ansible to manage cloud infrastructure, where dashes tend to be heavily used.

@Tronde
Copy link
Contributor

@Tronde Tronde commented Feb 17, 2020

Just to remind those who haven't read the whole thread here. There is also a thread on the devel mailling list: https://groups.google.com/forum/#!topic/ansible-devel/bjAcM9ferIw

IMHO this change was a really poor choice. Codebreaking syntax changes in minor release versions holds us back from extending the use of Ansible in our envirionment. Because I won't be able to update Ansible when it breaks the playbooks of my users.

@andyfeller
Copy link

@andyfeller andyfeller commented Mar 12, 2020

But as @bcoca stated above most of the developers don't look at these discussions here on a regular basis and this issue may not be the right place to discuss the change because it is about the correct documentation.

@Tronde : one would argue that contributors AND customers are consulted before stories are written to understand the impact and gather feedback well before someone codes a solution. As several here have mentioned, this is a product mgmt failing we've seen more than once.

@Aethylred
Copy link

@Aethylred Aethylred commented Mar 30, 2020

As an example of the situation @andyfeller describes about this change:

We have a problem with this on our site.

We use Red Hat Identity Manager as an external inventory, we do not control it, it contains many host groups with dashes instead of underscores. This will not be changed (because of all the other things that exist using those names).

So, we need:

  • To configure Ansible to maintain the current behaviour
  • Silence the deprecation warning
  • Do this for command line Ansible and Ansible Tower
@geerlingguy
Copy link
Contributor

@geerlingguy geerlingguy commented Apr 2, 2020

FYI PR #66650 (please no pitchforks there) is slated for 2.10 (as of the current moment), meaning anyone who currently sees this warning would (once they upgrade to 2.10, again assuming that PR is merged) start having playbook failures instead (until they set force_valid_group_names = ignore in an ansible.cfg).

Just posting for visibility. I'm still firmly behind my earlier assertion that this is a user-hostile default, as there are still many dynamic inventory scripts (some part of Ansible itself or now moved into 'official-ish' collections) that generate group names with dashes or other valid DNS characters.

Practically anyone who uses Ansible with AWS is going to have to override the default.

@apple4ever

This comment was marked as outdated.

@apple4ever
Copy link
Contributor

@apple4ever apple4ever commented Apr 2, 2020

FYI this was discussed at a Core Meeting here, starting at 19:06:55

@geerlingguy

This comment was marked as off-topic.

@bcoca
Copy link
Member

@bcoca bcoca commented Apr 2, 2020

So i've seen above many comments of things that were already answered/debunked/etc, so just going to link here my previous post .

#56930 (comment)

Please don't add new posts that don't add NEW items for discussion as they hide the previous ones that were already answered.

@MarkusTeufelberger
Copy link
Contributor

@MarkusTeufelberger MarkusTeufelberger commented Apr 2, 2020

By the way, where would be a good place to link to in the Python documentation about how valid variable names look like? There's https://docs.python.org/3/reference/lexical_analysis.html#grammar-token-identifier, but that's not really user friendly or readable for people without Computer Science backgrounds.

The reason for asking is that I'm not sure if the actual initial complaint has actually been dealt with. There's just a warning that there's something wrong, but it takes a lot of digging to find out what exactly and how one - if willing or able - could actually choose a valid group name. I would expect at least a clear "The group name foo-bar contains invalid characters (-). Valid group names should be valid Python identifiers (see https://docs.python.org/??? for more information)" message, not just "there are bad characters, check again with -vvvv to actually find out which ones!". Ideally this would also mention that this can be disabled, but could lead to other unexpected problems (like making it really hard for Ansible to distinguish the groups foo-bar, foo.bar and foo_bar).

Currently it's more of an "You did something wrong, fix it" message with no clear way laid out how to proceed which might also have contributed to the strong response here.

@Tronde
Copy link
Contributor

@Tronde Tronde commented Apr 3, 2020

@geerlingguy wrote in comment 56930:

(until they set force_valid_group_names = false in an ansible.cfg)

The documentation does not mention 'false' as a valid value for this key. I've set the value to 'ignore' which should do the trick. But is 'false' an invalid keyword or is it correct and the documentation is not complete here?

@Aethylred
Copy link

@Aethylred Aethylred commented Apr 7, 2020

@bcoca in a previous comment:

Just to say this once, clearly , YOU WILL ALWAYS BE ABLE TO USE DASHES IN GROUP NAMES also dots and other characters that are now considered 'invalid', just not by default. This 'default' is what is being deprecated, the default will be 'safe' in 2.11, but you will always have an option to 'opt in' to the old behaviour.

You've stated repeatedly that the current behaviour can be preserved, but what are the exact ansible.cfg settings required to do this now and squash the deprecation warning.

I've tried as @geerlingguy wrote in comment 56930:

(until they set force_valid_group_names = false in an ansible.cfg)

Which causes my playbooks to fail when it can't find hosts or groups with hyphens (they're coming in from an inventory plugin we wrote BTW, do theses have to do the transform too, or is that done when Ansible ingests the inventory from the plugin?)

@beenje
Copy link

@beenje beenje commented Apr 7, 2020

I've tried as @geerlingguy wrote in comment 56930:

(until they set force_valid_group_names = false in an ansible.cfg)

Which causes my playbooks to fail when it can't find hosts or groups with hyphens (they're coming in from an inventory plugin we wrote BTW, do theses have to do the transform too, or is that done when Ansible ingests the inventory from the plugin?)

This was mentioned in several comments and is in the documentation. You should use never or ignore.

@campee
Copy link

@campee campee commented Apr 14, 2020

So are we just not supposed to use the EC2 dynamic inventory script anymore since it groups everything by 'us-east-1', 'us-east-2', etc? Or is there a plan to update it? I just went to Ansible's documentation for the EC2 dynamic inventory script and the link to download it on Github doesn't work anymore, so that's interesting.

@mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Apr 14, 2020

I just went to Ansible's documentation for the EC2 dynamic inventory script and the link to download it on Github doesn't work anymore, so that's interesting.

See #68419

@trombik
Copy link

@trombik trombik commented Apr 20, 2020

for those of you who don't bother to read the IRC log, here is the decision, i.e. no decision:

19:15:40 <sivel> I've got to say, that brining this topic up all the time isn't a good use of time
19:15:52 <cyberpear> bcoca nominated it
19:16:07 <felixfontein> I think the aim was to solve this once and for all (like, again :) )
19:16:29 <cyberpear> since bcoca is not here, move on to next topic?
19:16:34 <sivel> honestly, I don't think this is going to be the right forum to make a decision on this
19:16:45 <jillr> +2 moving on
19:16:47 <cyberpear> sivel: what's the correct forum?
19:16:55 <felixfontein> sivel: what is the right forum for making that decision?
19:17:02 <cyberpear> "declaration from Red Hat On High"?
19:17:15 <sivel> I'm going to abstain on that, but this project is not a democracy
19:17:16 <cyberpear> -1 to "declaration from Red Hat On High"
19:17:24 <sivel> too many cooks in the kitchen distract
19:17:45 <sivel> We know the arguments at this point
19:17:59 <sivel> anywho, next topic

yes, someone wrote "feel free to drop by ML or IRC". no, "this project is not a democracy".

@sunshine69
Copy link

@sunshine69 sunshine69 commented Apr 20, 2020

yes, someone wrote "feel free to drop by ML or IRC". no, "this project is not a democracy".

To be honest this is wrong with opensource - If it leads to a not popular way - people can fork it, can it be forked?

I can see accepting PR in ansible is terribly slow. Patches looks obviously needed and simple change but it never gets in. Luckily ansible itself is flexible to allow people to use custom plugins however it seems stale will make myself lesser in contributions - or even more hassle to do so.

Feeling a bit sad, really ...

@Tronde
Copy link
Contributor

@Tronde Tronde commented Apr 21, 2020

@sunshine69 I feel your pain. But that is a discussion that should take place on IRC or the Google Group for Ansible Development.

This issue is not the right place for it. Because to few people read here.

@andyfeller
Copy link

@andyfeller andyfeller commented Apr 21, 2020

@sunshine69 I feel your pain. But that is a discussion that should take place on IRC or the Google Group for Ansible Development.

This issue is not the right place for it. Because to few people read here.

While the discussion might be more productive in those other channels, the transparency is appreciated for people following this issue specifically. IRC isn’t everyone’s preference after all.

@geerlingguy
Copy link
Contributor

@geerlingguy geerlingguy commented May 14, 2020

FYI: Remove deprecation for TRANSFORM_INVALID_GROUP_CHARS was just merged yesterday. There are backport PRs for 2.9 (#69487) and 2.8 (#69488) to remove the deprecation warnings there.

@ansibot
Copy link
Contributor

@ansibot ansibot commented May 14, 2020

Files identified in the description:

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.