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.

Sign up for GitHub

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

Allow the characters _:./+-@= to be used in tag keys during deploy #1798

Merged
merged 6 commits into from
Feb 18, 2020
Merged

Allow the characters _:./+-@= to be used in tag keys during deploy #1798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
988a84c
Allow the characters _:./+- to be used in tag keys
RobReus Jan 15, 2020
53c2f68
fix: tag keys and values use the same regex.
sriram-mv Feb 17, 2020
3c94914
unit: additional unit test for tags
sriram-mv Feb 17, 2020
8155f61
fix: add docstrings for `_generate_match_regex` method
sriram-mv Feb 18, 2020
8838421
refactor: change function `_unquote` name to be more precise
sriram-mv Feb 18, 2020
fc890c3
chore: rename constant names and change scoping
sriram-mv Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 62 additions & 42 deletions samcli/cli/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,57 @@

import click

PARAM_AND_METADATA_KEY_REGEX = '([A-Za-z0-9\\"]+)'

def _value_regex(delim):
return f'(\\"(?:\\\\.|[^\\"\\\\]+)*\\"|(?:\\\\.|[^{delim}\\"\\\\]+)+)'

def _generate_match_regex(match_pattern, delim):

KEY_REGEX = '([A-Za-z0-9\\"]+)'
# Use this regex when you have space as delimiter Ex: "KeyName1=string KeyName2=string"
VALUE_REGEX_SPACE_DELIM = _value_regex(" ")
# Use this regex when you have comma as delimiter Ex: "KeyName1=string,KeyName2=string"
VALUE_REGEX_COMMA_DELIM = _value_regex(",")
"""
Creates a regex string based on a match pattern (also a regex) that is to be
run on a string (which may contain escaped quotes) that is separated by delimiters.

Parameters
----------
match_pattern: (str) regex pattern to match
delim: (str) delimiter that is respected when identifying matching groups with generated regex.

Returns
-------
str: regex expression

"""

# Non capturing groups reduces duplicates in groups, but does not reduce matches.
return f'(\\"(?:\\\\{match_pattern}|[^\\"\\\\]+)*\\"|(?:\\\\{match_pattern}|[^{delim}\\"\\\\]+)+)'


def _unquote_wrapped_quotes(value):
r"""
Removes wrapping double quotes and any '\ ' characters. They are usually added to preserve spaces when passing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You say "They are usually added". So does that mean there is a case that we wouldn't want to replace the double or '\ ' characters from the input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think so, we will want to replace it. "Usually added" maybe a misnomer, preserving spaces inside quotes or even escaping quotes within quotes requires a \ (escape) character.

value thru shell.

Examples
--------
>>> _unquote_wrapped_quotes('val\ ue')
value

>>> _unquote_wrapped_quotes("hel\ lo")
hello

Parameters
----------
value : str
Input to unquote

Returns
-------
Unquoted string
"""
if value and (value[0] == value[-1] == '"'):
# Remove quotes only if the string is wrapped in quotes
value = value.strip('"')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here is to only remove the first and last quotes right? Do we need to worry about the case where there are two " in the beginning or end of the string input?

Below was run in Python3.8

>>> value = '"a\""'
>>> value.strip('"')
'a'
>>> value = '""a""'
>>> value.strip('"')
'a'

Doing a slice might be better here:

>>> value = '"a\""'
>>> value[1:-1]
'a"'
>>> value = '""a""'
>>> value[1:-1]
'"a"'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only cares about wrapping quotes and not the entire nesting, can change the function name to reflect that.


return value.replace("\\ ", " ").replace('\\"', '"')


class CfnParameterOverridesType(click.ParamType):
Expand All @@ -36,8 +77,12 @@ class CfnParameterOverridesType(click.ParamType):
# If Both ParameterKey pattern and KeyPairName=MyKey should not be present
# while adding parameter overrides, if they are, it
# can result in unpredicatable behavior.
_pattern_1 = r"(?:ParameterKey={key},ParameterValue={value})".format(key=KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)
_pattern_2 = r"(?:(?: ){key}={value})".format(key=KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)
# Use this regex when you have space as delimiter Ex: "KeyName1=string KeyName2=string"
VALUE_REGEX_SPACE_DELIM = _generate_match_regex(match_pattern=".", delim=" ")
_pattern_1 = r"(?:ParameterKey={key},ParameterValue={value})".format(
key=PARAM_AND_METADATA_KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM
)
_pattern_2 = r"(?:(?: ){key}={value})".format(key=PARAM_AND_METADATA_KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)

ordered_pattern_match = [_pattern_1, _pattern_2]

Expand Down Expand Up @@ -80,39 +125,10 @@ def convert(self, value, param, ctx):

# 'groups' variable is a list of tuples ex: [(key1, value1), (key2, value2)]
for key, param_value in groups:
result[self._unquote(key)] = self._unquote(param_value)
result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value)

return result

@staticmethod
def _unquote(value):
r"""
Removes wrapping double quotes and any '\ ' characters. They are usually added to preserve spaces when passing
value thru shell.

Examples
--------
>>> _unquote('val\ ue')
value

>>> _unquote("hel\ lo")
hello

Parameters
----------
value : str
Input to unquote

Returns
-------
Unquoted string
"""
if value and (value[0] == value[-1] == '"'):
# Remove quotes only if the string is wrapped in quotes
value = value.strip('"')

return value.replace("\\ ", " ").replace('\\"', '"')


class CfnMetadataType(click.ParamType):
"""
Expand All @@ -121,8 +137,10 @@ class CfnMetadataType(click.ParamType):
"""

_EXAMPLE = 'KeyName1=string,KeyName2=string or {"string":"string"}'
# Use this regex when you have comma as delimiter Ex: "KeyName1=string,KeyName2=string"
VALUE_REGEX_COMMA_DELIM = _generate_match_regex(match_pattern=".", delim=",")

_pattern = r"(?:{key}={value})".format(key=KEY_REGEX, value=VALUE_REGEX_COMMA_DELIM)
_pattern = r"(?:{key}={value})".format(key=PARAM_AND_METADATA_KEY_REGEX, value=VALUE_REGEX_COMMA_DELIM)

# NOTE(TheSriram): name needs to be added to click.ParamType requires it.
name = ""
Expand Down Expand Up @@ -167,8 +185,10 @@ class CfnTags(click.ParamType):
"""

_EXAMPLE = "KeyName1=string KeyName2=string"
# Tags have additional constraints and they allow "+ - = . _ : / @" apart from alpha-numerics.
TAG_REGEX = '[A-Za-z0-9\\"_:\\.\\/\\+-\\@=]'

_pattern = r"{key}={value}".format(key=KEY_REGEX, value=VALUE_REGEX_SPACE_DELIM)
_pattern = r"{tag}={tag}".format(tag=_generate_match_regex(match_pattern=TAG_REGEX, delim=" "))

# NOTE(TheSriram): name needs to be added to click.ParamType requires it.
name = ""
Expand All @@ -191,7 +211,7 @@ def convert(self, value, param, ctx):
for group in groups:
key, v = group
# assign to result['KeyName1'] = string and so on.
result[key] = v
result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(v)

if fail:
return self.fail(
Expand Down
24 changes: 12 additions & 12 deletions tests/integration/deploy/test_deploy_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def test_package_and_deploy_no_s3_bucket_all_args(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=True,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
)

deploy_process_no_execute = Popen(deploy_command_list_no_execute, stdout=PIPE)
Expand All @@ -91,7 +91,7 @@ def test_package_and_deploy_no_s3_bucket_all_args(self, template_file):
notification_arns=self.sns_arn,
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
)

deploy_process = Popen(deploy_command_list_execute, stdout=PIPE)
Expand Down Expand Up @@ -121,7 +121,7 @@ def test_no_package_and_deploy_with_s3_bucket_all_args(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
)

Expand Down Expand Up @@ -212,7 +212,7 @@ def test_no_package_and_deploy_with_s3_bucket_all_args_confirm_changeset(self, t
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=True,
)

Expand All @@ -237,7 +237,7 @@ def test_deploy_without_s3_bucket(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
)

Expand Down Expand Up @@ -272,7 +272,7 @@ def test_deploy_without_stack_name(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
)

Expand Down Expand Up @@ -301,7 +301,7 @@ def test_deploy_without_capabilities(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
)

Expand All @@ -327,7 +327,7 @@ def test_deploy_without_template_file(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
)

Expand Down Expand Up @@ -359,7 +359,7 @@ def test_deploy_with_s3_bucket_switch_region(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
)

Expand All @@ -384,7 +384,7 @@ def test_deploy_with_s3_bucket_switch_region(self, template_file):
parameter_overrides="Parameter=Clarity",
kms_key_id=self.kms_key,
no_execute_changeset=False,
tags="integ=true clarity=yes",
tags="integ=true clarity=yes foo_bar=baz",
confirm_changeset=False,
region="eu-west-2",
)
Expand Down Expand Up @@ -425,7 +425,7 @@ def test_deploy_twice_with_no_fail_on_empty_changeset(self, template_file):
"parameter_overrides": "Parameter=Clarity",
"kms_key_id": self.kms_key,
"no_execute_changeset": False,
"tags": "integ=true clarity=yes",
"tags": "integ=true clarity=yes foo_bar=baz",
"confirm_changeset": False,
}
# Package and Deploy in one go without confirming change set.
Expand Down Expand Up @@ -473,7 +473,7 @@ def test_deploy_twice_with_fail_on_empty_changeset(self, template_file):
"parameter_overrides": "Parameter=Clarity",
"kms_key_id": self.kms_key,
"no_execute_changeset": False,
"tags": "integ=true clarity=yes",
"tags": "integ=true clarity=yes foo_bar=baz",
"confirm_changeset": False,
}
deploy_command_list = self.get_deploy_command_list(**kwargs)
Expand Down
10 changes: 9 additions & 1 deletion tests/unit/cli/test_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,15 @@ def test_must_fail_on_invalid_format(self, input):

self.param_type.fail.assert_called_with(ANY, "param", "ctx")

@parameterized.expand([(("a=b",), {"a": "b"}), (("a=b", "c=d"), {"a": "b", "c": "d"}), (("",), {})])
@parameterized.expand(
[
(("a=b",), {"a": "b"}),
(("a=b", "c=d"), {"a": "b", "c": "d"}),
(('"a+-=._:/@"="b+-=._:/@" "--c="="=d/"',), {"a+-=._:/@": "b+-=._:/@", "--c=": "=d/"}),
(('owner:name="son of anton"',), {"owner:name": "son of anton"}),
(("",), {}),
]
)
def test_successful_parsing(self, input, expected):
result = self.param_type.convert(input, None, None)
self.assertEqual(result, expected, msg="Failed with Input = " + str(input))
4 changes: 2 additions & 2 deletions tests/unit/commands/samconfig/test_samconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ def test_deploy(self, do_cli_mock):
["notify1", "notify2"],
True,
True,
{"a": "tag1", "b": '"tag with spaces"'},
{"a": "tag1", "b": "tag with spaces"},
{"m1": "value1", "m2": "value2"},
True,
True,
Expand Down Expand Up @@ -452,7 +452,7 @@ def test_deploy_different_parameter_override_format(self, do_cli_mock):
["notify1", "notify2"],
True,
True,
{"a": "tag1", "b": '"tag with spaces"'},
{"a": "tag1", "b": "tag with spaces"},
{"m1": "value1", "m2": "value2"},
True,
True,
Expand Down