Repository: ansible/ansible
Pull request: elasticache_parameter_group: fix KeyError bug #24475
##### SUMMARY
Some parameters don't have a list of allowed values or a current value. This wasn't taken into account, so when the key didn't exist would get KeyError exception. Also fixed yes/no values for parameters being interpreted as truthy so changed doesn't always show as True when modifying those. Used to_text. Updated the description parameter to be an empty string by default.
Fixes #24475

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py

##### ANSIBLE VERSION
```
2.4.0
```

Problem statement:
elasticache_parameter_group fails with KeyError: 'AllowedValues'
##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
elasticache_parameter_group

##### ANSIBLE VERSION
```
$ ansible --version
ansible 2.3.0.0
  config file = /home/tessa/.ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609]
```

##### CONFIGURATION

##### OS / ENVIRONMENT
N/A

##### SUMMARY
when attempting to create/update a parameter group using the new elasticache_parameter_group, it fails with a KeyError on 'AllowedValues'. Without knowing the code, this makes me think it's filtering on values allowed for the upstream API, but doesn't have a correct list of what is and isn't allowed.

##### STEPS TO REPRODUCE
```yaml
    - name: create EC parameter group
      elasticache_parameter_group:
        name: mainapp-28
        group_family: redis2.8
        description: This is managed by Ansible.
        state: present
        values:
          notify-keyspace-events: Egx
```

##### EXPECTED RESULTS
Parameter group created / modified.

##### ACTUAL RESULTS
```
The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_wAJvSx/ansible_module_elasticache_parameter_group.py", line 330, in <module>
    main()
  File "/tmp/ansible_wAJvSx/ansible_module_elasticache_parameter_group.py", line 305, in main
    modifiable_params = make_current_modifiable_param_dict(module, connection, parameter_group_name)
  File "/tmp/ansible_wAJvSx/ansible_module_elasticache_parameter_group.py", line 156, in make_current_modifiable_param_dict
    modifiable_params[param["ParameterName"]] = [param["AllowedValues"], param["DataType"], param["ParameterValue"]]
KeyError: 'AllowedValues'

fatal: [localhost]: FAILED! => {
    "changed": false, 
    "failed": true, 
    "module_stderr": "Traceback (most recent call last):\n  File \"/tmp/ansible_wAJvSx/ansible_module_elasticache_parameter_group.py\", line 330, in <module>\n    main()\n  File \"/tmp/ansible_wAJvSx/ansible_module_elasticache_parameter_group.py\", line 305, in main\n    modifiable_params = make_current_modifiable_param_dict(module, connection, parameter_group_name)\n  File \"/tmp/ansible_wAJvSx/ansible_module_elasticache_parameter_group.py\", line 156, in make_current_modifiable_param_dict\n    modifiable_params[param[\"ParameterName\"]] = [param[\"AllowedValues\"], param[\"DataType\"], param[\"ParameterValue\"]]\nKeyError: 'AllowedValues'\n", 
    "module_stdout": "", 
    "msg": "MODULE FAILURE", 
    "rc": 0
}
```

Hints from issue discussion:
cc @s-hertel
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Fix KeyError bug by appending None if key doesn't exist

ensure value is the expected type; if if expecting something parsed as truthy try to turn it back into the desired value - fixes result showing always changed since bool compared to str

use to_text

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py b/lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
index 46a73ef3396d70..cbc7b5434e7be3 100644
--- a/lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
+++ b/lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
@@ -114,7 +114,7 @@
 # import module snippets
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.ec2 import boto3_conn, get_aws_connection_info, ec2_argument_spec, camel_dict_to_snake_dict
-from ansible.module_utils.six import text_type
+from ansible.module_utils._text import to_text
 import traceback
 
 try:
@@ -156,8 +156,10 @@ def make_current_modifiable_param_dict(module, conn, name):
     modifiable_params = {}
 
     for param in parameters:
-        if param["IsModifiable"] and ("AllowedValues" and "ParameterValue") in param:
-            modifiable_params[param["ParameterName"]] = [param["AllowedValues"], param["DataType"], param["ParameterValue"]]
+        if param["IsModifiable"]:
+            modifiable_params[param["ParameterName"]] = [param["AllowedValues"]] if "AllowedValues" in param else [None]
+            modifiable_params[param["ParameterName"]].append(param["DataType"])
+            modifiable_params[param["ParameterName"]].append(param["ParameterValue"] if "ParameterValue" in param else None)
     return modifiable_params
 
 
@@ -173,21 +175,36 @@ def check_valid_modification(module, values, modifiable_params):
             module.fail_json(msg="%s is not a modifiable parameter. Valid parameters to modify are: %s." % (parameter, modifiable_params.keys()))
 
         # check allowed datatype for modified parameters
-        str_to_type = {"integer": int, "string": text_type}
-        if not isinstance(new_value, str_to_type[modifiable_params[parameter][1]]):
-            module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
-                             (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
+        str_to_type = {"integer": int, "string": str}
+        expected_type = str_to_type[modifiable_params[parameter][1]]
+        if not isinstance(new_value, expected_type):
+            if expected_type == str:
+                if isinstance(new_value, bool):
+                    values[parameter] = "yes" if new_value else "no"
+                else:
+                    values[parameter] = to_text(new_value)
+            elif expected_type == int:
+                if isinstance(new_value, bool):
+                    values[parameter] = 1 if new_value else 0
+                else:
+                    module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
+                                     (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
+            else:
+                module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
+                                 (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
 
         # check allowed values for modifiable parameters
-        if text_type(new_value) not in modifiable_params[parameter][0] and not isinstance(new_value, int):
-            module.fail_json(msg="%s is not an allowed value for the parameter %s. Valid parameters are: %s." %
-                                 (new_value, parameter, modifiable_params[parameter][0]))
+        choices = modifiable_params[parameter][0]
+        if choices:
+            if to_text(new_value) not in choices and not isinstance(new_value, int):
+                module.fail_json(msg="%s is not an allowed value for the parameter %s. Valid parameters are: %s." %
+                                     (new_value, parameter, choices))
 
         # check if a new value is different from current value
-        if text_type(new_value) != modifiable_params[parameter][2]:
+        if to_text(values[parameter]) != modifiable_params[parameter][2]:
             changed_with_update = True
 
-    return changed_with_update
+    return changed_with_update, values
 
 
 def check_changed_parameter_values(values, old_parameters, new_parameters):
@@ -215,7 +232,7 @@ def modify(module, conn, name, values):
     # compares current group parameters with the parameters we've specified to to a value to see if this will change the group
     format_parameters = []
     for key in values:
-        value = text_type(values[key])
+        value = to_text(values[key])
         format_parameters.append({'ParameterName': key, 'ParameterValue': value})
     try:
         response = conn.modify_cache_parameter_group(CacheParameterGroupName=name, ParameterNameValues=format_parameters)
@@ -236,7 +253,7 @@ def reset(module, conn, name, values):
         all_parameters = False
         format_parameters = []
         for key in values:
-            value = text_type(values[key])
+            value = to_text(values[key])
             format_parameters.append({'ParameterName': key, 'ParameterValue': value})
     else:
         all_parameters = True
@@ -268,7 +285,7 @@ def main():
         dict(
             group_family=dict(type='str', choices=['memcached1.4', 'redis2.6', 'redis2.8', 'redis3.2']),
             name=dict(required=True, type='str'),
-            description=dict(type='str'),
+            description=dict(default='', type='str'),
             state=dict(required=True),
             values=dict(type='dict'),
         )
@@ -296,8 +313,8 @@ def main():
     exists = get_info(connection, parameter_group_name)
 
     # check that the needed requirements are available
-    if state == 'present' and not (exists and parameter_group_family and group_description):
-        module.fail_json(msg="Creating a group requires a family group and a description.")
+    if state == 'present' and not exists and not parameter_group_family:
+        module.fail_json(msg="Creating a group requires a family group.")
     elif state == 'reset' and not exists:
         module.fail_json(msg="No group %s to reset. Please create the group before using the state 'reset'." % parameter_group_name)
 
@@ -312,14 +329,14 @@ def main():
             # modify existing group
             else:
                 modifiable_params = make_current_modifiable_param_dict(module, connection, parameter_group_name)
-                changed = check_valid_modification(module, values, modifiable_params)
+                changed, values = check_valid_modification(module, values, modifiable_params)
                 response = modify(module, connection, parameter_group_name, values)
         # create group
         else:
             response, changed = create(module, connection, parameter_group_name, parameter_group_family, group_description)
             if values:
                 modifiable_params = make_current_modifiable_param_dict(module, connection, parameter_group_name)
-                changed = check_valid_modification(module, values, modifiable_params)
+                changed, values = check_valid_modification(module, values, modifiable_params)
                 response = modify(module, connection, parameter_group_name, values)
     elif state == 'absent':
         if exists:


Code review comments:
diff_hunk: @@ -156,8 +156,10 @@ def make_current_modifiable_param_dict(module, conn, name):
     modifiable_params = {}
 
     for param in parameters:
-        if param["IsModifiable"] and ("AllowedValues" and "ParameterValue") in param:
-            modifiable_params[param["ParameterName"]] = [param["AllowedValues"], param["DataType"], param["ParameterValue"]]
+        if param["IsModifiable"]:
+            modifiable_params[param["ParameterName"]] = [param["AllowedValues"]] if "AllowedValues" in param else [None]
path: lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
text: @user1:
Instead of inline ifs, you can use `[param.get("AllowedValues")]` instead and that will give you a list containing `None` if there are no allowed values.

diff_hunk: @@ -173,21 +175,36 @@ def check_valid_modification(module, values, modifiable_params):
             module.fail_json(msg="%s is not a modifiable parameter. Valid parameters to modify are: %s." % (parameter, modifiable_params.keys()))
 
         # check allowed datatype for modified parameters
-        str_to_type = {"integer": int, "string": text_type}
-        if not isinstance(new_value, str_to_type[modifiable_params[parameter][1]]):
-            module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
-                             (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
+        str_to_type = {"integer": int, "string": str}
+        expected_type = str_to_type[modifiable_params[parameter][1]]
+        if not isinstance(new_value, expected_type):
+            if expected_type == str:
+                if isinstance(new_value, bool):
+                    values[parameter] = "yes" if new_value else "no"
+                else:
+                    values[parameter] = to_text(new_value)
+            elif expected_type == int:
+                if isinstance(new_value, bool):
+                    values[parameter] = 1 if new_value else 0
+                else:
+                    module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
+                                     (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
+            else:
+                module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
+                                 (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
 
         # check allowed values for modifiable parameters
-        if text_type(new_value) not in modifiable_params[parameter][0] and not isinstance(new_value, int):
-            module.fail_json(msg="%s is not an allowed value for the parameter %s. Valid parameters are: %s." %
-                                 (new_value, parameter, modifiable_params[parameter][0]))
+        choices = modifiable_params[parameter][0]
+        if choices:
+            if to_text(new_value) not in choices and not isinstance(new_value, int):
path: lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
text: @user1:
boolean tip: `if not (to_text(new_value) in choices or isinstance(new_value, int)):`

diff_hunk: @@ -173,21 +175,36 @@ def check_valid_modification(module, values, modifiable_params):
             module.fail_json(msg="%s is not a modifiable parameter. Valid parameters to modify are: %s." % (parameter, modifiable_params.keys()))
 
         # check allowed datatype for modified parameters
-        str_to_type = {"integer": int, "string": text_type}
-        if not isinstance(new_value, str_to_type[modifiable_params[parameter][1]]):
-            module.fail_json(msg="%s (type %s) is not an allowed value for the parameter %s. Expected a type %s." %
-                             (new_value, type(new_value), parameter, modifiable_params[parameter][1]))
+        str_to_type = {"integer": int, "string": str}
path: lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
text: @user1:
I don't think this will work the way you expect with Python 2/3. I don't *think* we'll ever get `unicode` inputs, but if someone did a `!!python/unicode sometext` in their play it would show as unicode.

diff_hunk: @@ -296,8 +313,8 @@ def main():
     exists = get_info(connection, parameter_group_name)
 
     # check that the needed requirements are available
-    if state == 'present' and not (exists and parameter_group_family and group_description):
-        module.fail_json(msg="Creating a group requires a family group and a description.")
+    if state == 'present' and not exists and not parameter_group_family:
path: lib/ansible/modules/cloud/amazon/elasticache_parameter_group.py
text: @user1:
This can also be written as `if state == 'present' and not (exists or parameter_group_family):`