Merge pull request #5559 from mgugino-upstream-stage/readd-ent-registry

Automatic merge from submit-queue.

Migrate enterprise registry logic to docker role

Currently, the enterprise registry to forcefully added
in openshift_facts.  Recently, the docker role has
been modified to consume registry variables directly,
bypassing openshift_facts.

This commit cleans up unused code in openshift_facts,
and migrates enterprise registry logic to the
docker role.

Fixes: https://github.com/openshift/openshift-ansible/issues/5557

9 files changed, 25 insertions, 57 deletions

diff --git a/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml b/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml
index ac3c702a0..dab17aaa9 100644
--- a/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml
+++ b/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml
@@ -5,15 +5,6 @@
   - always
   tasks:
   - set_fact:
-      openshift_docker_additional_registries: "{{ lookup('oo_option', 'docker_additional_registries') }}"
-    when: openshift_docker_additional_registries is not defined
-  - set_fact:
-      openshift_docker_insecure_registries: "{{ lookup('oo_option',  'docker_insecure_registries') }}"
-    when: openshift_docker_insecure_registries is not defined
-  - set_fact:
-      openshift_docker_blocked_registries: "{{ lookup('oo_option', 'docker_blocked_registries') }}"
-    when: openshift_docker_blocked_registries is not defined
-  - set_fact:
       openshift_docker_options: "{{ lookup('oo_option', 'docker_options') }}"
     when: openshift_docker_options is not defined
   - set_fact:
diff --git a/roles/docker/defaults/main.yml b/roles/docker/defaults/main.yml
index 274fd8603..e36dfa7b9 100644
--- a/roles/docker/defaults/main.yml
+++ b/roles/docker/defaults/main.yml
@@ -9,6 +9,8 @@ openshift_docker_additional_registries: []
 openshift_docker_blocked_registries: []
 openshift_docker_insecure_registries: []
 
+openshift_docker_ent_reg: 'registry.access.redhat.com'
+
 # The l2_docker_* variables convert csv strings to lists, if
 # necessary.  These variables should be used in place of their respective
 # openshift_docker_* counterparts to ensure the properly formatted lists are
diff --git a/roles/docker/tasks/package_docker.yml b/roles/docker/tasks/package_docker.yml
index 0c5621259..4215dc5bd 100644
--- a/roles/docker/tasks/package_docker.yml
+++ b/roles/docker/tasks/package_docker.yml
@@ -50,6 +50,14 @@
       src: custom.conf.j2
   when: not os_firewall_use_firewalld | default(False) | bool
 
+- name: Add enterprise registry, if necessary
+  set_fact:
+    l2_docker_additional_registries: "{{ l2_docker_additional_registries + [openshift_docker_ent_reg] }}"
+  when:
+  - openshift.common.deployment_type == 'openshift-enterprise'
+  - openshift_docker_ent_reg != ''
+  - openshift_docker_ent_reg not in l2_docker_additional_registries
+
 - stat: path=/etc/sysconfig/docker
   register: docker_check
 
diff --git a/roles/docker/tasks/systemcontainer_crio.yml b/roles/docker/tasks/systemcontainer_crio.yml
index 5b02b72be..66ce475e1 100644
--- a/roles/docker/tasks/systemcontainer_crio.yml
+++ b/roles/docker/tasks/systemcontainer_crio.yml
@@ -1,17 +1,17 @@
 ---
 # TODO: Much of this file is shared with container engine tasks
 - set_fact:
-    l_insecure_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(openshift.docker.insecure_registries)) }}"
-  when: openshift.docker.insecure_registries
+    l_insecure_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(l2_docker_insecure_registries)) }}"
+  when: l2_docker_insecure_registries
 - set_fact:
-    l_crio_registries: "{{ openshift.docker.additional_registries + ['docker.io'] }}"
-  when: openshift.docker.additional_registries
+    l_crio_registries: "{{ l2_docker_additional_registries + ['docker.io'] }}"
+  when: l2_docker_additional_registries
 - set_fact:
     l_crio_registries: "{{ ['docker.io'] }}"
-  when: not openshift.docker.additional_registries
+  when: not l2_docker_additional_registries
 - set_fact:
     l_additional_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(l_crio_registries)) }}"
-  when: openshift.docker.additional_registries
+  when: l2_docker_additional_registries
 
 - name: Ensure container-selinux is installed
   package:
diff --git a/roles/docker/tasks/systemcontainer_docker.yml b/roles/docker/tasks/systemcontainer_docker.yml
index 146e5f430..8b43393cb 100644
--- a/roles/docker/tasks/systemcontainer_docker.yml
+++ b/roles/docker/tasks/systemcontainer_docker.yml
@@ -148,10 +148,10 @@
 # Set local versions of facts that must be in json format for container-daemon.json
 # NOTE: When jinja2.9+ is used the container-daemon.json file can move to using tojson
 - set_fact:
-    l_docker_insecure_registries: "{{ docker_insecure_registries | default([]) | to_json }}"
+    l_docker_insecure_registries: "{{ l2_docker_insecure_registries | default([]) | to_json }}"
     l_docker_log_options: "{{ docker_log_options | default({}) | to_json }}"
-    l_docker_additional_registries: "{{ docker_additional_registries | default([]) | to_json }}"
-    l_docker_blocked_registries: "{{ docker_blocked_registries | default([]) | to_json }}"
+    l_docker_additional_registries: "{{ l2_docker_additional_registries | default([]) | to_json }}"
+    l_docker_blocked_registries: "{{ l2_docker_blocked_registries | default([]) | to_json }}"
     l_docker_selinux_enabled: "{{ docker_selinux_enabled | default(true) | to_json }}"
 
 # Configure container-engine using the container-daemon.json file
diff --git a/roles/openshift_docker_facts/tasks/main.yml b/roles/openshift_docker_facts/tasks/main.yml
index 334150f63..5a3e50678 100644
--- a/roles/openshift_docker_facts/tasks/main.yml
+++ b/roles/openshift_docker_facts/tasks/main.yml
@@ -6,9 +6,6 @@
   with_items:
   - role: docker
     local_facts:
-      additional_registries: "{{ openshift_docker_additional_registries | default(None) }}"
-      blocked_registries: "{{ openshift_docker_blocked_registries | default(None) }}"
-      insecure_registries: "{{ openshift_docker_insecure_registries | default(None) }}"
       selinux_enabled: "{{ openshift_docker_selinux_enabled | default(None) }}"
       log_driver: "{{ openshift_docker_log_driver | default(None) }}"
       log_options: "{{ openshift_docker_log_options | default(None) }}"
@@ -23,12 +20,6 @@
       sdn_mtu: "{{ openshift_node_sdn_mtu | default(None) }}"
 
 - set_fact:
-    docker_additional_registries: "{{ openshift.docker.additional_registries
-                                      | default(omit) }}"
-    docker_blocked_registries: "{{ openshift.docker.blocked_registries
-                                   | default(omit) }}"
-    docker_insecure_registries: "{{ openshift.docker.insecure_registries
-                                    | default(omit) }}"
     docker_selinux_enabled: "{{ openshift.docker.selinux_enabled | default(omit) }}"
     docker_log_driver: "{{ openshift.docker.log_driver | default(omit) }}"
     docker_log_options: "{{ openshift.docker.log_options | default(omit) }}"
diff --git a/roles/openshift_facts/library/openshift_facts.py b/roles/openshift_facts/library/openshift_facts.py
index 1c2c91a5a..11ef9fa97 100755
--- a/roles/openshift_facts/library/openshift_facts.py
+++ b/roles/openshift_facts/library/openshift_facts.py
@@ -55,9 +55,6 @@ def migrate_docker_facts(facts):
     """ Apply migrations for docker facts """
     params = {
         'common': (
-            'additional_registries',
-            'insecure_registries',
-            'blocked_registries',
             'options'
         ),
         'node': (
@@ -768,14 +765,6 @@ def set_deployment_facts_if_unset(facts):
                 service_type = 'origin'
             facts['common']['service_type'] = service_type
 
-    if 'docker' in facts:
-        deployment_type = facts['common']['deployment_type']
-        if deployment_type == 'openshift-enterprise':
-            addtl_regs = facts['docker'].get('additional_registries', [])
-            ent_reg = 'registry.access.redhat.com'
-            if ent_reg not in addtl_regs:
-                facts['docker']['additional_registries'] = addtl_regs + [ent_reg]
-
     for role in ('master', 'node'):
         if role in facts:
             deployment_type = facts['common']['deployment_type']
@@ -2248,19 +2237,6 @@ class OpenShiftFacts(object):
                                       protected_facts_to_overwrite)
 
         if 'docker' in new_local_facts:
-            # remove duplicate and empty strings from registry lists, preserving order
-            for cat in ['additional', 'blocked', 'insecure']:
-                key = '{0}_registries'.format(cat)
-                if key in new_local_facts['docker']:
-                    val = new_local_facts['docker'][key]
-                    if isinstance(val, string_types):
-                        val = [x.strip() for x in val.split(',')]
-                    seen = set()
-                    new_local_facts['docker'][key] = list()
-                    for registry in val:
-                        if registry not in seen and registry != '':
-                            seen.add(registry)
-                            new_local_facts['docker'][key].append(registry)
             # Convert legacy log_options comma sep string to a list if present:
             if 'log_options' in new_local_facts['docker'] and \
                     isinstance(new_local_facts['docker']['log_options'], string_types):
diff --git a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
index 98372d979..93a5973d4 100644
--- a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
+++ b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py
@@ -153,7 +153,7 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck):
 
     def known_docker_registries(self):
         """Build a list of docker registries available according to inventory vars."""
-        regs = list(self.get_var("openshift.docker.additional_registries", default=[]))
+        regs = list(self.get_var("openshift_docker_additional_registries", default=[]))
 
         deployment_type = self.get_var("openshift_deployment_type")
         if deployment_type == "origin" and "docker.io" not in regs:
diff --git a/roles/openshift_health_checker/test/docker_image_availability_test.py b/roles/openshift_health_checker/test/docker_image_availability_test.py
index 952fa9aa6..c523ffd5c 100644
--- a/roles/openshift_health_checker/test/docker_image_availability_test.py
+++ b/roles/openshift_health_checker/test/docker_image_availability_test.py
@@ -72,7 +72,7 @@ def test_all_images_available_remotely(task_vars, available_locally):
             return {'images': [], 'failed': available_locally}
         return {}
 
-    task_vars['openshift']['docker']['additional_registries'] = ["docker.io", "registry.access.redhat.com"]
+    task_vars['openshift_docker_additional_registries'] = ["docker.io", "registry.access.redhat.com"]
     task_vars['openshift_image_tag'] = 'v3.4'
     check = DockerImageAvailability(execute_module, task_vars)
     check._module_retry_interval = 0
@@ -90,7 +90,7 @@ def test_all_images_unavailable(task_vars):
 
         return {}  # docker_image_facts failure
 
-    task_vars['openshift']['docker']['additional_registries'] = ["docker.io"]
+    task_vars['openshift_docker_additional_registries'] = ["docker.io"]
     task_vars['openshift_deployment_type'] = "openshift-enterprise"
     task_vars['openshift_image_tag'] = 'latest'
     check = DockerImageAvailability(execute_module, task_vars)
@@ -121,9 +121,9 @@ def test_no_known_registries():
                 service_type='origin',
                 is_containerized=False,
                 is_atomic=False,
-            ),
-            docker=dict(additional_registries=["docker.io"]),
+            )
         ),
+        openshift_docker_additional_registries=["docker.io"],
         openshift_deployment_type="openshift-enterprise",
         openshift_image_tag='latest',
         group_names=['nodes', 'masters'],
@@ -154,7 +154,7 @@ def test_skopeo_update_failure(task_vars, message, extra_words):
 
         return {}
 
-    task_vars['openshift']['docker']['additional_registries'] = ["unknown.io"]
+    task_vars['openshift_docker_additional_registries'] = ["unknown.io"]
     task_vars['openshift_deployment_type'] = "openshift-enterprise"
     check = DockerImageAvailability(execute_module, task_vars)
     check._module_retry_interval = 0