Merge pull request #667 from rjeffman/fix_service_principal_idempotent_issue

ipaservice: Fix idempotent behavior for principal aliases.

plugins/modules/ipaservice.py

@@ -224,7 +224,8 @@ RETURN = """
 
 from ansible.module_utils.ansible_freeipa_module import \
     IPAAnsibleModule, compare_args_ipa, encode_certificate, \
-    gen_add_del_lists, ipalib_errors
+    gen_add_del_lists, gen_add_list, gen_intersection_list, ipalib_errors, \
+    api_get_realm, to_text
 
 
 def find_service(module, name):
@@ -492,6 +493,30 @@ def main():
 
         for name in names:
             res_find = find_service(ansible_module, name)
+            res_principals = []
+
+            if principal and res_find:
+                # When comparing principals to the existing ones,
+                # the REALM is needded, and are added here for those
+                # that do not have it.
+                principal = [
+                    p if "@" in p
+                    else "%s@%s" % (p, api_get_realm())
+                    for p in principal
+                ]
+                principal = list(set(principal))
+
+                # Create list of existing principal aliases as strings
+                # to compare with provided ones.
+                canonicalname = {
+                    to_text(p)
+                    for p in res_find.get("krbcanonicalname", [])
+                }
+                res_principals = [
+                    to_text(elem)
+                    for elem in res_find.get("krbprincipalname", [])
+                ]
+                res_principals = list(set(res_principals) - canonicalname)
 
             if state == "present":
                 if action == "service":
@@ -576,8 +601,8 @@ def main():
                         host_add, host_del = gen_add_del_lists(
                             host, res_find.get('managedby_host', []))
 
-                        principal_add, principal_del = gen_add_del_lists(
+                        principal_add, principal_del = \
-                            principal, res_find.get("principal"))
+                            gen_add_del_lists(principal, res_principals)
 
                         (allow_create_keytab_user_add,
                          allow_create_keytab_user_del) = \
@@ -646,7 +671,7 @@ def main():
                     certificate_del = []
                     host_add = host or []
                     host_del = []
-                    principal_add = principal or []
+                    principal_add = gen_add_list(principal, res_principals)
                     principal_del = []
 
                     allow_create_keytab_user_add = \
@@ -674,21 +699,12 @@ def main():
                         allow_retrieve_keytab_hostgroup or []
                     allow_retrieve_keytab_hostgroup_del = []
 
-                # Add principals
+                if principal_add:
-                for _principal in principal_add:
                     commands.append([name, "service_add_principal",
-                                     {
+                                     {"krbprincipalname": principal_add}])
-                                         "krbprincipalname":
+                if principal_del:
-                                         _principal,
-                                     }])
-
-                # Remove principals
-                for _principal in principal_del:
                     commands.append([name, "service_remove_principal",
-                                     {
+                                     {"krbprincipalname": principal_del}])
-                                         "krbprincipalname":
-                                         _principal,
-                                     }])
 
                 for _certificate in certificate_add:
                     commands.append([name, "service_add_cert",
@@ -776,13 +792,12 @@ def main():
                         ansible_module.fail_json(msg="No service '%s'" % name)
 
                     # Remove principals
-                    if principal is not None:
+                    principal_del = gen_intersection_list(
-                        for _principal in principal:
+                        principal, res_principals)
-                            commands.append([name, "service_remove_principal",
+                    if principal_del:
-                                             {
+                        commands.append([name, "service_remove_principal",
-                                                 "krbprincipalname":
+                                         {"krbprincipalname": principal_del}])
-                                                 _principal,
+
-                                             }])
                     # Remove certificates
                     if certificate is not None:
                         existing = res_find.get('usercertificate', [])

tests/service/test_service_without_skip_host_check.yml

@@ -347,6 +347,118 @@
     register: result
     failed_when: result.changed or result.failed
 
+  # tests for upstream issue #663
+  - name: Ensure service is present with principal alias.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "asvc/{{ host1_fqdn }}"
+    register: result
+    failed_when: result.failed or not result.changed
+
+  - name: Ensure service is present with principal alias, again.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "asvc/{{ host1_fqdn }}"
+    register: result
+    failed_when: result.failed or result.changed
+
+  - name: Ensure service is present with different principal alias.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "HTTP/{{ host1_fqdn }}"
+      force: yes
+    register: result
+    failed_when: result.failed or not result.changed
+
+  - name: Ensure service is presennt with different principal alias, again.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "HTTP/{{ host1_fqdn }}"
+      force: yes
+    register: result
+    failed_when: result.failed or result.changed
+
+  - name: Ensure service member principal alias is present.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "asvc/{{ host1_fqdn }}"
+      action: member
+    register: result
+    failed_when: result.failed or not result.changed
+
+  - name: Ensure service member principal alias is present, again.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "asvc/{{ host1_fqdn }}"
+      action: member
+    register: result
+    failed_when: result.failed or result.changed
+
+  - name: Ensure service member principal alias is absent.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "asvc/{{ host1_fqdn }}"
+      action: member
+      state: absent
+    register: result
+    failed_when: result.failed or not result.changed
+
+  - name: Ensure service member principal alias is absent, again.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal: "asvc/{{ host1_fqdn }}"
+      action: member
+      state: absent
+    register: result
+    failed_when: result.failed or result.changed
+
+  - name: Ensure service is present with multiple principal aliases.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal:
+        - "HTTP/{{ host1_fqdn }}"
+        - "asvc/{{ host1_fqdn }}"
+    register: result
+    failed_when: result.failed or not result.changed
+
+  - name: Ensure service is present with multiple principal aliases, again.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      principal:
+        - "HTTP/{{ host1_fqdn }}"
+        - "asvc/{{ host1_fqdn }}"
+    register: result
+    failed_when: result.failed or result.changed
+
+  - name: Ensure service is with multiple principal aliases is absent.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      continue: yes
+      state: absent
+    register: result
+    failed_when: result.failed or not result.changed
+
+  - name: Ensure service is with multiple principal aliases is absent, again.
+    ipaservice:
+      ipaadmin_password: SomeADMINpassword
+      name: "mysvc/{{ host1_fqdn }}"
+      continue: yes
+      state: absent
+    register: result
+    failed_when: result.failed or result.changed
+  # end of tests for upstream issue #663
+
   # cleanup
   - name: Cleanup test environment
     include_tasks: env_cleanup.yml