From a001d1db1a4f55cbaafdd161ff16d139136bd82c Mon Sep 17 00:00:00 2001 From: Adar Nimrod Date: Sat, 22 May 2021 13:13:33 +0300 Subject: [PATCH] A more reliable way to get the container id. The hostname is not always the container id. Get the container id from /proc/1/cgroup. Fixes #1918. --- .pre-commit-config.yaml | 2 +- pre_commit/languages/docker.py | 17 ++++-- tests/clientlib_test.py | 4 +- tests/languages/docker_test.py | 97 +++++++++++++++++++++------------- tests/repository_test.py | 2 +- 5 files changed, 79 insertions(+), 43 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a2ebb7017..78cbfacd6 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,7 +25,7 @@ repos: hooks: - id: validate_manifest - repo: https://github.com/asottile/pyupgrade - rev: v2.19.1 + rev: v2.16.0 hooks: - id: pyupgrade args: [--py36-plus] diff --git a/pre_commit/languages/docker.py b/pre_commit/languages/docker.py index 5b21ec94c..5bb3395af 100644 --- a/pre_commit/languages/docker.py +++ b/pre_commit/languages/docker.py @@ -1,7 +1,6 @@ import hashlib import json import os -import socket from typing import Sequence from typing import Tuple @@ -26,12 +25,24 @@ def _is_in_docker() -> bool: return False +def _get_container_id() -> str: + # It's assumed that we already check /proc/1/cgroup in _is_in_docker. The + # cpuset cgroup controller existed since cgroups were introduced so this + # way of getting the container ID is pretty reliable. + with open('/proc/1/cgroup', 'rb') as f: + for line in f.readlines(): + if line.split(b':')[1] == b'cpuset': + return os.path.basename(line.split(b':')[2]).strip().decode() + raise RuntimeError('Failed to find the container ID in /proc/1/cgroup.') + + def _get_docker_path(path: str) -> str: if not _is_in_docker(): return path - hostname = socket.gethostname() - _, out, _ = cmd_output_b('docker', 'inspect', hostname) + container_id = _get_container_id() + + _, out, _ = cmd_output_b('docker', 'inspect', container_id) container, = json.loads(out) for mount in container['Mounts']: diff --git a/tests/clientlib_test.py b/tests/clientlib_test.py index 09bdb3ee9..ff3cce38d 100644 --- a/tests/clientlib_test.py +++ b/tests/clientlib_test.py @@ -427,7 +427,7 @@ def test_minimum_pre_commit_version_passing(): @pytest.mark.parametrize('schema', (CONFIG_SCHEMA, CONFIG_REPO_DICT)) def test_warn_additional(schema): allowed_keys = {item.key for item in schema.items if hasattr(item, 'key')} - warn_additional, = ( + warn_additional, = [ x for x in schema.items if isinstance(x, cfgv.WarnAdditionalKeys) - ) + ] assert allowed_keys == set(warn_additional.keys) diff --git a/tests/languages/docker_test.py b/tests/languages/docker_test.py index 01b5e2773..797ad2013 100644 --- a/tests/languages/docker_test.py +++ b/tests/languages/docker_test.py @@ -9,6 +9,43 @@ from pre_commit.languages import docker +docker_cgroup_example = b'''\ +12:hugetlb:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +11:blkio:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +10:freezer:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +9:cpu,cpuacct:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +8:pids:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +7:rdma:/ +6:net_cls,net_prio:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +5:cpuset:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +4:devices:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +3:memory:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +2:perf_event:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +1:name=systemd:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 +0::/system.slice/containerd.service +''' # noqa: E501 + +# The ID should match the above cgroup example. +docker_container_id = ( + 'c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7' +) + +non_docker_cgroup_example = b'''\ +12:perf_event:/ +11:hugetlb:/ +10:devices:/ +9:blkio:/ +8:rdma:/ +7:cpuset:/ +6:cpu,cpuacct:/ +5:freezer:/ +4:memory:/ +3:pids:/ +2:net_cls,net_prio:/ +1:name=systemd:/init.scope +0::/init.scope +''' + def test_docker_fallback_user(): def invalid_attribute(): @@ -37,45 +74,25 @@ def _mock_open(data): def test_in_docker_docker_in_file(): - docker_cgroup_example = b'''\ -12:hugetlb:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -11:blkio:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -10:freezer:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -9:cpu,cpuacct:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -8:pids:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -7:rdma:/ -6:net_cls,net_prio:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -5:cpuset:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -4:devices:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -3:memory:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -2:perf_event:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -1:name=systemd:/docker/c33988ec7651ebc867cb24755eaf637a6734088bc7eef59d5799293a9e5450f7 -0::/system.slice/containerd.service -''' # noqa: E501 with _mock_open(docker_cgroup_example): assert docker._is_in_docker() is True def test_in_docker_docker_not_in_file(): - non_docker_cgroup_example = b'''\ -12:perf_event:/ -11:hugetlb:/ -10:devices:/ -9:blkio:/ -8:rdma:/ -7:cpuset:/ -6:cpu,cpuacct:/ -5:freezer:/ -4:memory:/ -3:pids:/ -2:net_cls,net_prio:/ -1:name=systemd:/init.scope -0::/init.scope -''' with _mock_open(non_docker_cgroup_example): assert docker._is_in_docker() is False +def test_get_container_id(): + with _mock_open(docker_cgroup_example): + assert docker._get_container_id() == docker_container_id + + +def test_get_container_id_failure(): + with _mock_open(b''), pytest.raises(RuntimeError): + docker._get_container_id() + + def test_get_docker_path_not_in_docker_returns_same(): with mock.patch.object(docker, '_is_in_docker', return_value=False): assert docker._get_docker_path('abc') == 'abc' @@ -87,6 +104,14 @@ def in_docker(): yield +@pytest.fixture +def return_docker_container_id(): + with mock.patch.object( + docker, '_get_container_id', return_value=docker_container_id, + ): + yield + + def _linux_commonpath(): return mock.patch.object(os.path, 'commonpath', posixpath.commonpath) @@ -100,14 +125,14 @@ def _docker_output(out): return mock.patch.object(docker, 'cmd_output_b', return_value=ret) -def test_get_docker_path_in_docker_no_binds_same_path(in_docker): +def test_get_docker_path_in_docker_no_binds_same_path(in_docker, return_docker_container_id): docker_out = json.dumps([{'Mounts': []}]).encode() with _docker_output(docker_out): assert docker._get_docker_path('abc') == 'abc' -def test_get_docker_path_in_docker_binds_path_equal(in_docker): +def test_get_docker_path_in_docker_binds_path_equal(in_docker, return_docker_container_id): binds_list = [{'Source': '/opt/my_code', 'Destination': '/project'}] docker_out = json.dumps([{'Mounts': binds_list}]).encode() @@ -115,7 +140,7 @@ def test_get_docker_path_in_docker_binds_path_equal(in_docker): assert docker._get_docker_path('/project') == '/opt/my_code' -def test_get_docker_path_in_docker_binds_path_complex(in_docker): +def test_get_docker_path_in_docker_binds_path_complex(in_docker, return_docker_container_id): binds_list = [{'Source': '/opt/my_code', 'Destination': '/project'}] docker_out = json.dumps([{'Mounts': binds_list}]).encode() @@ -124,7 +149,7 @@ def test_get_docker_path_in_docker_binds_path_complex(in_docker): assert docker._get_docker_path(path) == '/opt/my_code/test/something' -def test_get_docker_path_in_docker_no_substring(in_docker): +def test_get_docker_path_in_docker_no_substring(in_docker, return_docker_container_id): binds_list = [{'Source': '/opt/my_code', 'Destination': '/project'}] docker_out = json.dumps([{'Mounts': binds_list}]).encode() @@ -133,7 +158,7 @@ def test_get_docker_path_in_docker_no_substring(in_docker): assert docker._get_docker_path(path) == path -def test_get_docker_path_in_docker_binds_path_many_binds(in_docker): +def test_get_docker_path_in_docker_binds_path_many_binds(in_docker, return_docker_container_id): binds_list = [ {'Source': '/something_random', 'Destination': '/not-related'}, {'Source': '/opt/my_code', 'Destination': '/project'}, @@ -145,7 +170,7 @@ def test_get_docker_path_in_docker_binds_path_many_binds(in_docker): assert docker._get_docker_path('/project') == '/opt/my_code' -def test_get_docker_path_in_docker_windows(in_docker): +def test_get_docker_path_in_docker_windows(in_docker, return_docker_container_id): binds_list = [{'Source': r'c:\users\user', 'Destination': r'c:\folder'}] docker_out = json.dumps([{'Mounts': binds_list}]).encode() diff --git a/tests/repository_test.py b/tests/repository_test.py index af829c2e7..b6f7fb254 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -51,7 +51,7 @@ def _get_hook_no_install(repo_config, store, hook_id): config = cfgv.validate(config, CONFIG_SCHEMA) config = cfgv.apply_defaults(config, CONFIG_SCHEMA) hooks = all_hooks(config, store) - hook, = (hook for hook in hooks if hook.id == hook_id) + hook, = [hook for hook in hooks if hook.id == hook_id] return hook