-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Add toggle to control invalid character substitution in group names #52748
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
5fec53e
against my better judgement
bcoca ecf305f
handle bad regex
bcoca fefb4e2
ensure script plugin uses safe names
bcoca fca3275
make add_group return proper name
bcoca cafaa96
move to boolean with warning and deprecation
bcoca a34f5c8
forgot what to search
bcoca 9acf284
also allow for no matches
bcoca 63a18ae
ensure central transform/check
bcoca cca60dc
minor fixes
bcoca 303b9ad
fixed missing )
bcoca 3323273
fix indentation issues
bcoca 6ce3691
preserve orig error
bcoca 7ca08cd
restored behaviour
bcoca 0d53db3
fixes as per fb
bcoca ecf96f7
added 'silent' to avoid spamming current users
bcoca 15f6f3f
centralized valid var names
bcoca 83f546f
actually reverse, want invalid
bcoca 0475d14
fixed regex
bcoca e974abf
fix regex
bcoca 15d493e
just compile regex one time
bcoca d5d66e5
dont display dupes
bcoca 3cf6d0b
comment on regex
bcoca 439e3cb
added regex tests
bcoca b837daf
revert ini, script, foreman to prev behaviour
bcoca 6c7f222
more complete errormsg
bcoca 5269f28
move sanitization to avoid keyerror
bcoca File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
minor_changes: | ||
- add toggle to allow user to override invalid group character filter |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Make coding more python3-ish | ||
from __future__ import (absolute_import, division, print_function) | ||
__metaclass__ = type | ||
|
||
from units.compat import unittest | ||
|
||
from ansible import constants as C | ||
|
||
|
||
test_cases = (('not-valid', ['-'], 'not_valid'), ('not!valid@either', ['!', '@'], 'not_valid_either'), ('1_nor_This', ['1'], '__nor_This')) | ||
|
||
|
||
class TestInvalidVars(unittest.TestCase): | ||
|
||
def test_positive_matches(self): | ||
|
||
for name, invalid, sanitized in test_cases: | ||
self.assertEqual(C.INVALID_VARIABLE_NAMES.findall(name), invalid) | ||
|
||
def test_negative_matches(self): | ||
for name in ('this_is_valid', 'Also_1_valid', 'noproblem'): | ||
self.assertEqual(C.INVALID_VARIABLE_NAMES.findall(name), []) | ||
|
||
def test_get_setting(self): | ||
|
||
for name, invalid, sanitized in test_cases: | ||
self.assertEqual(C.INVALID_VARIABLE_NAMES.sub('_', name), sanitized) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a customer has any non "-" invalid characters in their inventory groups this won't provide backwards compatibility for the ec2/azure inventory plugins with the scripts, since the scripts would have replaced those non "-" invalid characters with underscores. Tags with invalid characters or IPv6 are possible examples.
The gce script doesn't sanitize, so a blanket toggle like this would keep the gce inventory plugin groups compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default is false, but will give users a deprecation notice now, once we toggle to 'true' it will 'autotransform' with a warning for those plugins that don't already do the transform themselves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like you are assuming that some scripts like
ec2.py
sanitize group names. It has a setting for sanitizing group names, yes. But that doesn't mean that it works. I talked about this here:#40581 (comment)
I can file a bug if you'd like. I have found the behavior to be highly reproducible, and have dived in a little what values it has in code. It appears that
self.replace_dash_in_groups
does take the "True" value (meaning it should sanitize), but I think that either the regex was wrong, or the methodto_safe
was never called.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, assuming the opposite, those using the script plugin will now get a deprecation warning when trying to pass invalid group names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-hertel I can safely say that in the
ec2.py
script,to_safe
is not called on the relevant group names, and that the group nameus-east-2
is returned with eitherreplace_dash_in_groups = True
orreplace_dash_in_groups = False
(with the config loaded correctly, that is ruled out). I can get you more information on this if you want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlanCoding I'm not sure if you're understanding me. EC2 script always sanitizes, just to varying degrees. Here's my test case. Try adding a bad character in a tag. For instance, host has tags: {'bad:idea': 'here:too'}. Using the script on devel that creates the group
tag_bad_idea_here_too
. Those:
are replaced by_
(entirely regardless of replace_dash_in_groups). A filter is being used for bad characters, just not necessarily-
. So now if you port to using the plugin, you're not going to be able to replicate that behavior while also creating groups with some invalid characters (i.e.-
) because it's all or nothing, no partial sanitization (the group will becometag_bad:idea:here:too
if you're using the toggle to create groups likeus-east-1
). Anyway, I don't use it and ripping off the bandaid is probably good. I'm not going to fight for it, just pointing out the deficiency and to have something to point to later if users run into the corner case while trying to replicate the behavior of the script via the plugin. Hopefully no one is doing that but... ¯_ツ_/¯There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, a global solution like this also poses problems for combining different plugins per playbook run as you may want some plugins to filter and not others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlanCoding Ah, btw, replace_dash_in_groups is a tag specific option. Poorly named, doesn't work on general groups but works great for tags.