Repository: ansible/ansible
Pull request: docker_swarm_service: Make secret_id and config_id optional
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR removes the requirement of parameters `secret_id` for `secrets` and `config_id` for `configs`. If these parameters are not set the `*_name` will be used to look up the `*_id`.

Not really sure if something should be added to make the documentation bit clearer.

The lookups is made in the `DockerServiceManager` and then passed to `DockerService` through `from_ansible_params` instead of directly in `DockerService` . This is because `DockerService` is written to not have a dependency on the docker client. 

It should be fine to not have any version checks in `get_missing_secret_ids` and `get_missing_config_ids` as it will exit early if `secrets` and `configs` are not set and this will be the case if a user is running a docker version not supporting configs / secrets.

<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
Fixes #55009

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
docker_swarm_service


Problem statement:
docker_swarm_service: don't require secret_id
<!--- Verify first that your feature was not already discussed on GitHub -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
<!--- Describe the new feature/improvement briefly below -->
Match docker compose's short syntax which just requires the secret name.

##### ISSUE TYPE
- Feature Idea

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below, use your best guess if unsure -->
docker_swarm_service
##### ADDITIONAL INFORMATION
<!--- Describe how the feature would be used, why it is needed and what it would solve -->
Obtaining the secret ids for several secrets and passing them to docker_swarm_service is clunky and involves a lot of repetition. The secret's name should be all that's needed to reference an already defined secret, this is supported by compose. The filename could default to the secret name.
<!--- Paste example playbooks or commands between quotes below -->
```yaml

```

<!--- HINT: You can also paste gist.github.com links for larger files -->

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/cloud/docker/docker_swarm_service.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/docker/docker_swarm_service.py)

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](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @DBendit @WojciechowskiPiotr @akshay196 @danihodovic @dariko @felixfontein @hannseman @jwitko @kassiansun @tbouvet
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Thanks for your feature idea! I agree that it's clunky to require passing the secret id. The reason why it's built this way is because the Docker API requires both ID and Name when adding secrets through the `/services/` API endpoint. 

Looks like the way `docker stack` implements this is to make an API call to `/secrets/{id}` with passing Name as id. This will then return the relevant secret along with the ID and it can be used when passed to the `/services/` API endpoint. Passing Name as id to `/secrets/{id}` is not a documented feature.

Here's the actual secrets implementation in `docker stack deploy`: https://github.com/docker/cli/blob/master/cli/command/stack/swarm/deploy_composefile.go#L112
https://github.com/moby/moby/blob/master/client/secret_inspect.go#L13

If `/secrets/{id}` really accepts name as opposed to id it should be pretty easy to implement by loosing the requirement of `secret_id` and looking it up if missing.

For the record `configs` has the same issue.
It looks like the `/secrets/{x}` API method accepts both the name and the ID:
`````
$ docker secret ls
ID                          NAME                DRIVER              CREATED             UPDATED
p0v9wlmwss93duevq1ox1lu5r   test                                    6 minutes ago       6 minutes ago
$ curl --unix-socket /var/run/docker.sock http:/v1.39/secrets/test
{"ID":"p0v9wlmwss93duevq1ox1lu5r","Version":{"Index":204},"CreatedAt":"2019-04-09T09:22:24.906100326Z","UpdatedAt":"2019-04-09T09:22:24.906100326Z","Spec":{"Name":"test","Labels":{}}}
$ curl --unix-socket /var/run/docker.sock http:/v1.39/secrets/p0v9wlmwss93duevq1ox1lu5r
{"ID":"p0v9wlmwss93duevq1ox1lu5r","Version":{"Index":204},"CreatedAt":"2019-04-09T09:22:24.906100326Z","UpdatedAt":"2019-04-09T09:22:24.906100326Z","Spec":{"Name":"test","Labels":{}}}
 `````
but the [docs](https://docs.docker.com/engine/api/v1.39/#operation/SecretInspect) describes this method as only accepting the ID

Commit message: Python 2.6 compat

Patch to review:
diff --git a/changelogs/fragments/58299-docker_swarm_service-remove-required-config-secret-id.yml b/changelogs/fragments/58299-docker_swarm_service-remove-required-config-secret-id.yml
new file mode 100644
index 00000000000000..c9c965e369eaf0
--- /dev/null
+++ b/changelogs/fragments/58299-docker_swarm_service-remove-required-config-secret-id.yml
@@ -0,0 +1,2 @@
+minor_changes:
+- "docker_swarm_service - Remove requirement of ``secret_id`` on ``secrets`` and ``config_id`` on ``configs``."
diff --git a/lib/ansible/modules/cloud/docker/docker_swarm_service.py b/lib/ansible/modules/cloud/docker/docker_swarm_service.py
index e33fba7a3143de..2825c4cf473cd2 100644
--- a/lib/ansible/modules/cloud/docker/docker_swarm_service.py
+++ b/lib/ansible/modules/cloud/docker/docker_swarm_service.py
@@ -44,7 +44,6 @@
         description:
           - Config's ID.
         type: str
-        required: yes
       config_name:
         description:
           - Config's name as defined at its creation.
@@ -598,7 +597,6 @@
         description:
           - Secret's ID.
         type: str
-        required: yes
       secret_name:
         description:
           - Secret's name as defined at its creation.
@@ -993,8 +991,7 @@
     name: myservice
     image: alpine:edge
     configs:
-      - config_id: myconfig_id
-        config_name: myconfig_name
+      - config_name: myconfig_name
         filename: "/tmp/config.txt"
 
 - name: Set networks
@@ -1009,8 +1006,7 @@
     name: myservice
     image: alpine:edge
     secrets:
-      - secret_id: mysecret_id
-        secret_name: mysecret_name
+      - secret_name: mysecret_name
         filename: "/run/secrets/secret.txt"
 
 - name: Start service with healthcheck
@@ -1480,7 +1476,9 @@ def get_placement_from_ansible_params(params):
         }
 
     @classmethod
-    def from_ansible_params(cls, ap, old_service, image_digest, can_update_networks):
+    def from_ansible_params(
+        cls, ap, old_service, image_digest, can_update_networks, secret_ids, config_ids
+    ):
         s = DockerService()
         s.image = image_digest
         s.can_update_networks = can_update_networks
@@ -1616,9 +1614,10 @@ def from_ansible_params(cls, ap, old_service, image_digest, can_update_networks)
             s.configs = []
             for param_m in ap['configs']:
                 service_c = {}
-                service_c['config_id'] = param_m['config_id']
-                service_c['config_name'] = param_m['config_name']
-                service_c['filename'] = param_m['filename'] or service_c['config_name']
+                config_name = param_m['config_name']
+                service_c['config_id'] = param_m['config_id'] or config_ids[config_name]
+                service_c['config_name'] = config_name
+                service_c['filename'] = param_m['filename'] or config_name
                 service_c['uid'] = param_m['uid']
                 service_c['gid'] = param_m['gid']
                 service_c['mode'] = param_m['mode']
@@ -1628,9 +1627,10 @@ def from_ansible_params(cls, ap, old_service, image_digest, can_update_networks)
             s.secrets = []
             for param_m in ap['secrets']:
                 service_s = {}
-                service_s['secret_id'] = param_m['secret_id']
-                service_s['secret_name'] = param_m['secret_name']
-                service_s['filename'] = param_m['filename'] or service_s['secret_name']
+                secret_name = param_m['secret_name']
+                service_s['secret_id'] = param_m['secret_id'] or secret_ids[secret_name]
+                service_s['secret_name'] = secret_name
+                service_s['filename'] = param_m['filename'] or secret_name
                 service_s['uid'] = param_m['uid']
                 service_s['gid'] = param_m['gid']
                 service_s['mode'] = param_m['mode']
@@ -2326,6 +2326,54 @@ def can_update_networks(self):
             self.client.docker_py_version >= LooseVersion('2.7')
         )
 
+    def get_missing_secret_ids(self):
+        """
+        Resolve missing secret ids by looking them up by name
+        """
+        secret_names = [
+            secret['secret_name']
+            for secret in self.client.module.params.get('secrets') or []
+            if secret['secret_id'] is None
+        ]
+        if not secret_names:
+            return {}
+        secrets = self.client.secrets(filters={'name': secret_names})
+        secrets = dict(
+            (secret['Spec']['Name'], secret['ID'])
+            for secret in secrets
+            if secret['Spec']['Name'] in secret_names
+        )
+        for secret_name in secret_names:
+            if secret_name not in secrets:
+                self.client.fail(
+                    'Could not find a secret named "%s"' % secret_name
+                )
+        return secrets
+
+    def get_missing_config_ids(self):
+        """
+        Resolve missing config ids by looking them up by name
+        """
+        config_names = [
+            config['config_name']
+            for config in self.client.module.params.get('configs') or []
+            if config['config_id'] is None
+        ]
+        if not config_names:
+            return {}
+        configs = self.client.configs(filters={'name': config_names})
+        configs = dict(
+            (config['Spec']['Name'], config['ID'])
+            for config in configs
+            if config['Spec']['Name'] in config_names
+        )
+        for config_name in config_names:
+            if config_name not in configs:
+                self.client.fail(
+                    'Could not find a config named "%s"' % config_name
+                )
+        return configs
+
     def run(self):
         self.diff_tracker = DifferenceTracker()
         module = self.client.module
@@ -2351,11 +2399,15 @@ def run(self):
             )
         try:
             can_update_networks = self.can_update_networks()
+            secret_ids = self.get_missing_secret_ids()
+            config_ids = self.get_missing_config_ids()
             new_service = DockerService.from_ansible_params(
                 module.params,
                 current_service,
                 image_digest,
-                can_update_networks
+                can_update_networks,
+                secret_ids,
+                config_ids
             )
         except Exception as e:
             return self.client.fail(
@@ -2375,7 +2427,9 @@ def run(self):
                 msg = 'Service removed'
                 changed = True
             else:
-                changed, differences, need_rebuild, force_update = new_service.compare(current_service)
+                changed, differences, need_rebuild, force_update = new_service.compare(
+                    current_service
+                )
                 if changed:
                     self.diff_tracker.merge(differences)
                     if need_rebuild:
@@ -2504,7 +2558,7 @@ def main():
             tmpfs_mode=dict(type='int')
         )),
         configs=dict(type='list', elements='dict', options=dict(
-            config_id=dict(type='str', required=True),
+            config_id=dict(type='str'),
             config_name=dict(type='str', required=True),
             filename=dict(type='str'),
             uid=dict(type='str'),
@@ -2512,7 +2566,7 @@ def main():
             mode=dict(type='int'),
         )),
         secrets=dict(type='list', elements='dict', options=dict(
-            secret_id=dict(type='str', required=True),
+            secret_id=dict(type='str'),
             secret_name=dict(type='str', required=True),
             filename=dict(type='str'),
             uid=dict(type='str'),
diff --git a/test/integration/targets/docker_swarm_service/tasks/tests/configs.yml b/test/integration/targets/docker_swarm_service/tasks/tests/configs.yml
index e995841d188abc..77d078002fc58b 100644
--- a/test/integration/targets/docker_swarm_service/tasks/tests/configs.yml
+++ b/test/integration/targets/docker_swarm_service/tasks/tests/configs.yml
@@ -48,8 +48,7 @@
     resolve_image: no
     command: '/bin/sh -v -c "sleep 10m"'
     configs:
-      - config_id: "{{ config_result_1.config_id|default('') }}"
-        config_name: "{{ config_name_1 }}"
+      - config_name: "{{ config_name_1 }}"
         filename: "/tmp/{{ config_name_1 }}.txt"
   register: configs_2
   ignore_errors: yes
@@ -64,12 +63,25 @@
       - config_id: "{{ config_result_1.config_id|default('') }}"
         config_name: "{{ config_name_1 }}"
         filename: "/tmp/{{ config_name_1 }}.txt"
-      - config_id: "{{ config_result_2.config_id|default('') }}"
-        config_name: "{{ config_name_2 }}"
+      - config_name: "{{ config_name_2 }}"
         filename: "/tmp/{{ config_name_2 }}.txt"
   register: configs_3
   ignore_errors: yes
 
+- name: configs (add idempotency)
+  docker_swarm_service:
+    name: "{{ service_name }}"
+    image: alpine:3.8
+    resolve_image: no
+    command: '/bin/sh -v -c "sleep 10m"'
+    configs:
+      - config_name: "{{ config_name_1 }}"
+        filename: "/tmp/{{ config_name_1 }}.txt"
+      - config_name: "{{ config_name_2 }}"
+        filename: "/tmp/{{ config_name_2 }}.txt"
+  register: configs_4
+  ignore_errors: yes
+
 - name: configs (empty)
   docker_swarm_service:
     name: "{{ service_name }}"
@@ -77,7 +89,7 @@
     resolve_image: no
     command: '/bin/sh -v -c "sleep 10m"'
     configs: []
-  register: configs_4
+  register: configs_5
   ignore_errors: yes
 
 - name: configs (empty idempotency)
@@ -87,7 +99,7 @@
     resolve_image: no
     command: '/bin/sh -v -c "sleep 10m"'
     configs: []
-  register: configs_5
+  register: configs_6
   ignore_errors: yes
 
 - name: cleanup
@@ -101,8 +113,9 @@
     - configs_1 is changed
     - configs_2 is not changed
     - configs_3 is changed
-    - configs_4 is changed
-    - configs_5 is not changed
+    - configs_4 is not changed
+    - configs_5 is changed
+    - configs_6 is not changed
   when: docker_api_version is version('1.30', '>=') and docker_py_version is version('2.6.0', '>=')
 
 - assert:
diff --git a/test/integration/targets/docker_swarm_service/tasks/tests/secrets.yml b/test/integration/targets/docker_swarm_service/tasks/tests/secrets.yml
index 7ac2b4aa6ef5bf..da1135cd98ae0c 100644
--- a/test/integration/targets/docker_swarm_service/tasks/tests/secrets.yml
+++ b/test/integration/targets/docker_swarm_service/tasks/tests/secrets.yml
@@ -48,8 +48,7 @@
     resolve_image: no
     command: '/bin/sh -v -c "sleep 10m"'
     secrets:
-      - secret_id: "{{ secret_result_1.secret_id|default('') }}"
-        secret_name: "{{ secret_name_1 }}"
+      - secret_name: "{{ secret_name_1 }}"
         filename: "/run/secrets/{{ secret_name_1 }}.txt"
   register: secrets_2
   ignore_errors: yes
@@ -64,12 +63,25 @@
       - secret_id: "{{ secret_result_1.secret_id|default('') }}"
         secret_name: "{{ secret_name_1 }}"
         filename: "/run/secrets/{{ secret_name_1 }}.txt"
-      - secret_id: "{{ secret_result_2.secret_id|default('') }}"
-        secret_name: "{{ secret_name_2 }}"
+      - secret_name: "{{ secret_name_2 }}"
         filename: "/run/secrets/{{ secret_name_2 }}.txt"
   register: secrets_3
   ignore_errors: yes
 
+- name: secrets (add idempotency)
+  docker_swarm_service:
+    name: "{{ service_name }}"
+    image: alpine:3.8
+    resolve_image: no
+    command: '/bin/sh -v -c "sleep 10m"'
+    secrets:
+      - secret_name: "{{ secret_name_1 }}"
+        filename: "/run/secrets/{{ secret_name_1 }}.txt"
+      - secret_name: "{{ secret_name_2 }}"
+        filename: "/run/secrets/{{ secret_name_2 }}.txt"
+  register: secrets_4
+  ignore_errors: yes
+
 - name: secrets (empty)
   docker_swarm_service:
     name: "{{ service_name }}"
@@ -77,7 +89,7 @@
     resolve_image: no
     command: '/bin/sh -v -c "sleep 10m"'
     secrets: []
-  register: secrets_4
+  register: secrets_5
   ignore_errors: yes
 
 - name: secrets (empty idempotency)
@@ -87,7 +99,7 @@
     resolve_image: no
     command: '/bin/sh -v -c "sleep 10m"'
     secrets: []
-  register: secrets_5
+  register: secrets_6
   ignore_errors: yes
 
 - name: cleanup
@@ -101,8 +113,9 @@
       - secrets_1 is changed
       - secrets_2 is not changed
       - secrets_3 is changed
-      - secrets_4 is changed
-      - secrets_5 is not changed
+      - secrets_4 is not changed
+      - secrets_5 is changed
+      - secrets_6 is not changed
   when: docker_api_version is version('1.25', '>=') and docker_py_version is version('2.4.0', '>=')
 - assert:
     that:


Code review comments:
diff_hunk: @@ -64,20 +63,33 @@
       - config_id: "{{ config_result_1.config_id|default('') }}"
         config_name: "{{ config_name_1 }}"
         filename: "/tmp/{{ config_name_1 }}.txt"
-      - config_id: "{{ config_result_2.config_id|default('') }}"
-        config_name: "{{ config_name_2 }}"
+      - config_name: "{{ config_name_2 }}"
         filename: "/tmp/{{ config_name_2 }}.txt"
   register: configs_3
   ignore_errors: yes
 
+- name: configs (add idempotency)
+  docker_swarm_service:
+    name: "{{ service_name }}"
+    image: alpine:3.8
+    resolve_image: no
+    command: '/bin/sh -v -c "sleep 10m"'
+    configs:
+      - config_name: "{{ config_name_1 }}"
+        filename: "/tmp/{{ config_name_1 }}.txt"
+      - config_name: "{{ config_name_2 }}"
path: test/integration/targets/docker_swarm_service/tasks/tests/configs.yml
text: @user1:
Maybe add the ID here? Then in one of the add/idempotency pairs, you once have the ID in the first but not in the second, and once you have it in the second but not in the first.

@author:
That's a good idea. I wanted the test to run with two non id configs / secrets so we test passing multiple items as filters to the Docker API. I added a new test task for this.