[docker] Fix docker credential management

tests/common/system_utils/docker.py

@@ -5,7 +5,6 @@
 import collections
 import logging
 import os
-import yaml
 
 from tests.common import config_reload
 from tests.common.broadcom_data import is_broadcom_device
@@ -28,7 +27,7 @@ class DockerRegistryInfo(_DockerRegistryInfo):
     """
     pass
 
-def load_docker_registry_info(dut):
+def load_docker_registry_info(dut, creds):
     """
         Attempts to load Docker registry information.
 
@@ -47,31 +46,12 @@ def load_docker_registry_info(dut):
         Returns:
             DockerRegistryInfo: The registry information that was loaded.
     """
+    host = creds.get("docker_registry_host")
+    username = creds.get("docker_registry_username")
+    password = creds.get("docker_registry_password")
 
-    # FIXME: In Ansible we're able to load the facts regardless of where they're
-    # stored. We should figure out how to do this in pytest so the registry
-    # location isn't hard-coded.
-    registry_vars = dut.host.options['variable_manager'] \
-                            ._hostvars.get(dut.hostname, {}) \
-                            .get("secret_vars", {}) \
-                            .get("docker_registry")
-
-    if not registry_vars:
-        _LOGGER.warning("Registry info not found in inventory, falling back to registry file")
-
-        try:
-            with open(SONIC_DOCKER_REGISTRY) as contents:
-                registry_vars = yaml.safe_load(contents)
-        except IOError as err:
-            _LOGGER.error("Failed to parse registry file (%s)", err)
-            raise
-
-    host = registry_vars.get("docker_registry_host")
-    username = registry_vars.get("docker_registry_username")
-    password = registry_vars.get("docker_registry_password")
-
-    if not host or not username or not password:
-        error_message = "Missing registry hostname or login"
+    if not host:
+        error_message = "Missing registry hostname"
         _LOGGER.error(error_message)
         raise ValueError(error_message)
 
@@ -99,7 +79,9 @@ def download_image(dut, registry, image_name, image_version="latest"):
             image_version (str): The version of the image to download.
     """
 
-    dut.command("docker login {} -u {} -p {}".format(registry.host, registry.username, registry.password))
+    if registry.username and registry.password:
+        dut.command("docker login {} -u {} -p {}".format(registry.host, registry.username, registry.password))
+
     dut.command("docker pull {}/{}:{}".format(registry.host, image_name, image_version))
 
 def tag_image(dut, tag, image_name, image_version="latest"):
@@ -115,7 +97,7 @@ def tag_image(dut, tag, image_name, image_version="latest"):
 
     dut.command("docker tag {}:{} {}".format(image_name, image_version, tag))
 
-def swap_syncd(dut):
+def swap_syncd(dut, creds):
     """
         Replaces the running syncd container with the RPC version of it.
 
@@ -150,7 +132,7 @@ def swap_syncd(dut):
     output = dut.command("sonic-cfggen -y /etc/sonic/sonic_version.yml -v build_version")
     sonic_version = output["stdout_lines"][0].strip()
 
-    registry = load_docker_registry_info(dut)
+    registry = load_docker_registry_info(dut, creds)
     download_image(dut, registry, docker_rpc_image, sonic_version)
 
     tag_image(dut,
@@ -162,7 +144,7 @@ def swap_syncd(dut):
     config_reload(dut)
 
 
-def restore_default_syncd(dut):
+def restore_default_syncd(dut, creds):
     """
         Replaces the running syncd with the default syncd that comes with the image.
 
@@ -200,5 +182,5 @@ def restore_default_syncd(dut):
 
     # Remove the RPC image from the DUT
     docker_rpc_image = docker_syncd_name + "-rpc"
-    registry = load_docker_registry_info(dut)
+    registry = load_docker_registry_info(dut, creds)
     dut.command("docker rmi {}/{}:{}".format(registry.host, docker_rpc_image, sonic_version))

tests/conftest.py

@@ -333,6 +333,7 @@ def creds(duthost):
     groups.append("fanout")
     logger.info("dut {} belongs to groups {}".format(duthost.hostname, groups))
     files = glob.glob("../ansible/group_vars/all/*.yml")
+    files += glob.glob("../ansible/vars/*.yml")
     for group in groups:
         files += glob.glob("../ansible/group_vars/{}/*.yml".format(group))
     creds = {}
@@ -344,7 +345,13 @@ def creds(duthost):
             else:
                 logging.info("skip empty var file {}".format(f))
 
-    cred_vars = ["sonicadmin_user", "sonicadmin_password"]
+    cred_vars = [
+        "sonicadmin_user",
+        "sonicadmin_password",
+        "docker_registry_host",
+        "docker_registry_username",
+        "docker_registry_password"
+    ]
     hostvars = duthost.host.options['variable_manager']._hostvars[duthost.hostname]
     for cred_var in cred_vars:
         if cred_var in creds:

tests/copp/test_copp.py

@@ -102,7 +102,7 @@ def test_no_policer(self, protocol, duthost, ptfhost, copp_testbed):
                      copp_testbed)
 
 @pytest.fixture(scope="class")
-def copp_testbed(duthost, ptfhost, testbed, request):
+def copp_testbed(duthost, creds, ptfhost, testbed, request):
     """
         Pytest fixture to handle setup and cleanup for the COPP tests.
     """
@@ -111,9 +111,9 @@ def copp_testbed(duthost, ptfhost, testbed, request):
     if test_params.topo not in _SUPPORTED_TOPOS:
         pytest.skip("Topology not supported by COPP tests")
 
-    _setup_testbed(duthost, ptfhost, test_params)
+    _setup_testbed(duthost, creds, ptfhost, test_params)
     yield test_params
-    _teardown_testbed(duthost, ptfhost, test_params)
+    _teardown_testbed(duthost, creds, ptfhost, test_params)
 
 @pytest.fixture(autouse=True)
 def ignore_expected_loganalyzer_exceptions(duthost, loganalyzer):
@@ -180,7 +180,7 @@ def _gather_test_params(testbed, duthost, request):
                                topo=topo,
                                bgp_graph=bgp_graph)
 
-def _setup_testbed(dut, ptf, test_params):
+def _setup_testbed(dut, creds, ptf, test_params):
     """
         Sets up the testbed to run the COPP tests.
     """
@@ -197,7 +197,7 @@ def _setup_testbed(dut, ptf, test_params):
 
     if test_params.swap_syncd:
         logging.info("Swap out syncd to use RPC image...")
-        docker.swap_syncd(dut)
+        docker.swap_syncd(dut, creds)
     else:
         # NOTE: Even if the rpc syncd image is already installed, we need to restart
         # SWSS for the COPP changes to take effect.
@@ -207,7 +207,7 @@ def _setup_testbed(dut, ptf, test_params):
     logging.info("Configure syncd RPC for testing")
     copp_utils.configure_syncd(dut, test_params.nn_target_port)
 
-def _teardown_testbed(dut, ptf, test_params):
+def _teardown_testbed(dut, creds, ptf, test_params):
     """
         Tears down the testbed, returning it to its initial state.
     """
@@ -220,7 +220,7 @@ def _teardown_testbed(dut, ptf, test_params):
 
     if test_params.swap_syncd:
         logging.info("Restore default syncd docker...")
-        docker.restore_default_syncd(dut)
+        docker.restore_default_syncd(dut, creds)
     else:
         logging.info("Reloading config and restarting swss...")
         config_reload(dut)

tests/qos/qos_sai_base.py

@@ -295,7 +295,7 @@ def runPtfTest(self, ptfhost, testCase='', testParams={}):
         )["rc"] == 0, "Failed when running test '{0}'".format(testCase)
 
     @pytest.fixture(scope='class')
-    def swapSyncd(self, request, duthost):
+    def swapSyncd(self, request, duthost, creds):
         """
             Swap syncd on DUT host
 
@@ -308,12 +308,12 @@ def swapSyncd(self, request, duthost):
         """
         swapSyncd = request.config.getoption("--qos_swap_syncd")
         if swapSyncd:
-            docker.swap_syncd(duthost)
+            docker.swap_syncd(duthost, creds)
 
         yield
 
         if swapSyncd:
-            docker.restore_default_syncd(duthost)
+            docker.restore_default_syncd(duthost, creds)
 
     @pytest.fixture(scope='class', autouse=True)
     def dutConfig(self, request, duthost):