Repository: ansible/ansible
Pull request: Bugfix of 23976: mysql_replication - allow to pass empty values to the module's parameters
##### SUMMARY
Bugfix of #23976: mysql_replication - allow to pass empty values to the module's parameters
fixes #23976

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
```lib/ansible/modules/database/mysql/mysql_replication.py```


Problem statement:
mysql_replication does not allow setting blank values
##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
Module: mysql_replication

##### ANSIBLE VERSION
```
ansible 2.3.0.0
  config file = /home/XXX/..../ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.12+ (default, Sep 17 2016, 12:08:02) [GCC 6.2.0 20160914]
```

##### CONFIGURATION
N/A

##### OS / ENVIRONMENT
N/A

##### SUMMARY
When setting properties to an empty string, the module treats those properties as not being specified at all, so they won't be blanked out on the server. For example, if I want to remove SSL certificates because the connection no longer requires SSL, setting the various ssl properties to an empty string does not result in any change to the slave's configuration. The module ignores the properties with empty strings entirely.

I looked in the source and it appears that the module checks each property for truthiness before making a change, and since empty strings are falsy, properties with them fail the checks for updates.

##### STEPS TO REPRODUCE
```yaml
- name: Change master (Slave)
  mysql_replication:
    mode: changemaster
    master_host: '{{ DATABASE_MASTER_IP_ADDRESS }}'
    master_port: '{{ DATABASE_MASTER_PORT }}'
    master_user: '{{ DATABASE_REPLICATION_USERNAME }}'
    master_password: '{{ DATABASE_REPLICATION_PASSWORD }}'
    master_ssl: "{{ 1 if DATABASE_REQUIRES_SSL else 0 }}"
    master_ssl_ca: ''
    master_ssl_capath: ''
    master_ssl_cert: "{{ DATABASE_SERVER_SSL_CERTIFICATE_DIRECTORY + '/' + DATABASE_CLIENT_SSL_CERTIFICATES.cert.filename if DATABASE_REQUIRES_SSL else '' }}"
    master_ssl_key: "{{ DATABASE_SERVER_SSL_CERTIFICATE_DIRECTORY + '/' + DATABASE_CLIENT_SSL_CERTIFICATES.key.filename if DATABASE_REQUIRES_SSL else '' }}"
    master_log_file: '{{ _master_status.File }}'
    master_log_pos: '{{ _master_status.Position }}'
  become: yes
```

This is not a complete example, but illustrates the issue. If SSL is no longer enabled, then `master_ssl_cert` and `master_ssl_key` will evaluate to empty strings, but the module will not actually change these properties. Instead, they remain at their old values, which can be verified by running `SHOW SLAVE STATUS\G` on the slave.

Hints from issue discussion:
I've been able to work around this by using strings that consist of a single space. This works okay for the SSL stuff, but I'm not sure that it would work for other fields.
cc @banyek
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @Xyon @bmalynovytch @michaelcoburn @oneiroi @tolland
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @bmildren
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @Alexander198961
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @Andersson007
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @kurtdavis
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
resolved_by_pr: https://github.com/ansible/ansible/pull/63546
@JoelFeiner hello, thanks for reporting this!
would be cool to know your opinion about https://github.com/ansible/ansible/pull/63546

Commit message: Bugfix of 23976: mysql_replication - allow to pass blank lines, fix sanity

Patch to review:
diff --git a/changelogs/fragments/63546-mysql_replication_allow_to_pass_empty_values.yml b/changelogs/fragments/63546-mysql_replication_allow_to_pass_empty_values.yml
new file mode 100644
index 00000000000000..801bda2f9e912f
--- /dev/null
+++ b/changelogs/fragments/63546-mysql_replication_allow_to_pass_empty_values.yml
@@ -0,0 +1,2 @@
+bugfixes:
+- mysql_replication - allow to pass empty values to parameters (https://github.com/ansible/ansible/issues/23976).
diff --git a/lib/ansible/modules/database/mysql/mysql_replication.py b/lib/ansible/modules/database/mysql/mysql_replication.py
index 9c261a435083a6..2787870dbdcc85 100644
--- a/lib/ansible/modules/database/mysql/mysql_replication.py
+++ b/lib/ansible/modules/database/mysql/mysql_replication.py
@@ -147,6 +147,10 @@
     type: str
     version_added: '2.10'
 
+notes:
+- If an empty value for the parameter of string type is needed,
+  use string with a single space.
+
 extends_documentation_fragment:
 - mysql
 
@@ -333,6 +337,11 @@ def start_slave(cursor, connection_name='', channel=''):
 
 
 def changemaster(cursor, chm, connection_name='', channel=''):
+    # Params ransformation for issue #23976:
+    for num, param in enumerate(chm):
+        if "=' '" in param:
+            chm[num] = chm[num].replace("=' '", "=''")
+
     if connection_name:
         query = "CHANGE MASTER '%s' TO %s" % (connection_name, ','.join(chm))
     else:
diff --git a/test/integration/targets/mysql_replication/tasks/mysql_replication_initial.yml b/test/integration/targets/mysql_replication/tasks/mysql_replication_initial.yml
index 59d03c08d75b0f..2c799b82598b52 100644
--- a/test/integration/targets/mysql_replication/tasks/mysql_replication_initial.yml
+++ b/test/integration/targets/mysql_replication/tasks/mysql_replication_initial.yml
@@ -33,6 +33,8 @@
     - master_status is not changed
 
 # Test changemaster mode:
+# master_ssl_ca will be set as ' ' to check the module's behaviour for #23976,
+# must be converted to an empty string
 - name: Run replication
   mysql_replication:
     login_host: 127.0.0.1
@@ -44,12 +46,13 @@
     master_password: "{{ replication_pass }}"
     master_log_file: "{{ master_status.File }}"
     master_log_pos: "{{ master_status.Position }}"
+    master_ssl_ca: ' '
   register: result
 
 - assert:
     that:
     - result is changed
-    - result.queries == ["CHANGE MASTER TO MASTER_HOST='127.0.0.1',MASTER_USER='replication_user',MASTER_PASSWORD='********',MASTER_PORT=3306,MASTER_LOG_FILE='{{ master_status.File }}',MASTER_LOG_POS={{ master_status.Position }}"]
+    - result.queries == ["CHANGE MASTER TO MASTER_HOST='127.0.0.1',MASTER_USER='replication_user',MASTER_PASSWORD='********',MASTER_PORT=3306,MASTER_LOG_FILE='{{ master_status.File }}',MASTER_LOG_POS={{ master_status.Position }},MASTER_SSL_CA=''"]
 
 # Test startslave mode:
 - name: Start slave


Code review comments:
diff_hunk: @@ -44,12 +46,13 @@
     master_password: "{{ replication_pass }}"
     master_log_file: "{{ master_status.File }}"
     master_log_pos: "{{ master_status.Position }}"
+    master_ssl_ca: ' '
path: test/integration/targets/mysql_replication/tasks/mysql_replication_initial.yml
text: @user1:
Requiring a string of exactly one space is a bit fragile. I would suggest using `None` or an empty string in the code as an indicator that it should be an empty string to make the interface a bit more flexible. Then the module interface would look like this:

```yaml
master_ssl_ca: 
master_ssl_ca: ~
master_ssl_ca: null
master_ssl_ca: ''
```

You would then probably want to change the default value to something else to give yourself an indication as to whether a value was passed into the playbook on not.

```python
master_ssl_ca=dict(type='str', default='__unset__')
```

@author:
@user1 good suggestion! will do

@author:
Done, @user1 , PTAL (i chose an underscore because there are different checks in the code - a part for None, others for True/False)