Merge ~raharper/curtin:fix/chrootable-allow-resolvconf-missing into curtin:master

Proposed by Ryan Harper on 2019-07-02
Status: Merged
Approved by: Dan Watkins on 2019-07-23
Approved revision: 3084bb211521a1ea1adbfeb1874d5dfa1b072b06
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/curtin:fix/chrootable-allow-resolvconf-missing
Merge into: curtin:master
Diff against target: 133 lines (+89/-3)
2 files modified
curtin/util.py (+11/-3)
tests/unittests/test_util.py (+78/-0)
Reviewer Review Type Date Requested Status
Chad Smith 2019-07-02 Approve on 2019-07-08
Server Team CI bot continuous-integration Approve on 2019-07-05
Dan Watkins Approve on 2019-07-02
Lee Trager (community) Approve on 2019-07-02
Review via email: mp+369601@code.launchpad.net

Commit message

util.ChrootableTarget: skip rename of resolv.conf if not present in target

A target OS may not include an /etc/resolv.conf. ChrootableTarget attempts
to move the in-chroot resolv.conf out of the way and copy in the host
resolv.conf. If the target image does not have /etc/resolv.conf then we
fail when we call os.rename. Avoid this error by only invoking the
rename if the target image has a resolv.conf.

LP: #1834382

To post a comment you must log in.
Lee Trager (ltrager) wrote :

LGTM! Thanks for fixing this!

review: Approve
Ryan Harper (raharper) wrote :

CI found an error that I need to look at.

Ryan Harper (raharper) wrote :

In the ubuntu images, etc/resolv.conf is a symlink into /run which isn't present; the os.path.exists() check failed and we didn't rename. Fix this by accepting either file or symlink as need for rename.

Dan Watkins (daniel-thewatkins) wrote :

Just one inline question about simplifying things, but +1 regardless.

review: Approve
Ryan Harper (raharper) wrote :

Yes, that looks like just the thing.

db84b2e... by Ryan Harper on 2019-07-02

Refactor to use os.path.lexist

Chad Smith (chad.smith) :
Chad Smith (chad.smith) :
3084bb2... by Ryan Harper on 2019-07-05

ChrootableTarget: restore resolv.conf after rename if copy fails

If the copy of resolv.conf after a rename of an in-target resolv.conf
fails we will end up removing the in-target resolv.conf. Fix this by
checking if we renamed and if so restore the file before we remove
the temporary directory that was holding the original resolv.conf.

Chad Smith (chad.smith) wrote :

Thanks for the fix. LGTM!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/util.py b/curtin/util.py
2index 1ef17c0..ebe219d 100644
3--- a/curtin/util.py
4+++ b/curtin/util.py
5@@ -638,6 +638,7 @@ class ChrootableTarget(object):
6 self.allow_daemons = allow_daemons
7 self.sys_resolvconf = sys_resolvconf
8 self.rconf_d = None
9+ self.rc_tmp = None
10
11 def __enter__(self):
12 for p in self.mounts:
13@@ -656,14 +657,20 @@ class ChrootableTarget(object):
14 rtd = None
15 try:
16 rtd = tempfile.mkdtemp(dir=target_etc)
17- tmp = os.path.join(rtd, "resolv.conf")
18- os.rename(rconf, tmp)
19+ if os.path.lexists(rconf):
20+ self.rc_tmp = os.path.join(rtd, "resolv.conf")
21+ os.rename(rconf, self.rc_tmp)
22 self.rconf_d = rtd
23 shutil.copy("/etc/resolv.conf", rconf)
24 except Exception:
25 if rtd:
26+ # if we renamed, but failed later we need to restore
27+ if self.rc_tmp and os.path.lexists(self.rc_tmp):
28+ os.rename(os.path.join(self.rconf_d, "resolv.conf"),
29+ rconf)
30 shutil.rmtree(rtd)
31 self.rconf_d = None
32+ self.rc_tmp = None
33 raise
34
35 return self
36@@ -681,7 +688,8 @@ class ChrootableTarget(object):
37
38 rconf = paths.target_path(self.target, "/etc/resolv.conf")
39 if self.sys_resolvconf and self.rconf_d:
40- os.rename(os.path.join(self.rconf_d, "resolv.conf"), rconf)
41+ if self.rc_tmp and os.path.lexists(self.rc_tmp):
42+ os.rename(os.path.join(self.rconf_d, "resolv.conf"), rconf)
43 shutil.rmtree(self.rconf_d)
44
45 def subp(self, *args, **kwargs):
46diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
47index 7cf69cc..9960b34 100644
48--- a/tests/unittests/test_util.py
49+++ b/tests/unittests/test_util.py
50@@ -604,6 +604,84 @@ class TestChrootableTargetMounts(CiTestCase):
51 self.assertEqual(sorted(my_mounts), sorted(in_chroot.mounts))
52
53
54+class TestChrootableTargetResolvConf(CiTestCase):
55+ """Test ChrootableTargets handles target /etc/resolv.conf gracefully"""
56+
57+ def setUp(self):
58+ super(TestChrootableTargetResolvConf, self).setUp()
59+ self.target = self.tmp_dir()
60+ self.rconf = os.path.join(self.target, 'etc/resolv.conf')
61+ os.makedirs(os.path.dirname(self.rconf))
62+ self.add_patch('curtin.util.shutil', 'm_shutil')
63+ self.add_patch('curtin.util.do_mount', 'm_do_mount')
64+ self.add_patch('curtin.util.do_umount', 'm_do_umount')
65+ self.host_content = "host_resolvconf"
66+
67+ def tearDown(self):
68+ # manually remove resolv.conf since we're patching shutil
69+ if os.path.exists(self.rconf):
70+ os.unlink(self.rconf)
71+
72+ def mycopy(self, src, dst):
73+ print('mycopy(src=%s dst=%s)' % (src, dst))
74+ util.write_file(dst, self.host_content)
75+
76+ def mydel(self, tdir):
77+ print('mydel(tdir=%s)' % tdir)
78+ print(os.listdir(tdir))
79+
80+ def test_chrootable_target_renames_and_copies_resolvconf(self):
81+ content = "target_resolvconf"
82+ util.write_file(os.path.join(self.target, 'etc/resolv.conf'), content)
83+
84+ self.m_shutil.copy.side_effect = self.mycopy
85+ self.m_shutil.rmtree.side_effect = self.mydel
86+
87+ with util.ChrootableTarget(self.target):
88+ target_conf = util.load_file(
89+ paths.target_path(self.target, path='/etc/resolv.conf'))
90+ self.assertEqual(self.host_content, target_conf)
91+
92+ def test_chrootable_target_renames_and_copies_resolvconf_if_symlink(self):
93+ target_rconf = os.path.join(self.target, 'etc/resolv.conf')
94+ os.symlink('../run/foobar/wark.conf', target_rconf)
95+
96+ self.m_shutil.copy.side_effect = self.mycopy
97+ self.m_shutil.rmtree.side_effect = self.mydel
98+ with util.ChrootableTarget(self.target):
99+ target_conf = util.load_file(
100+ paths.target_path(self.target, path='/etc/resolv.conf'))
101+ self.assertEqual(self.host_content, target_conf)
102+
103+ @mock.patch('curtin.util.os.rename')
104+ def test_skip_rename_resolvconf_gone(self, m_rename):
105+ self.m_shutil.copy.side_effect = self.mycopy
106+ self.m_shutil.rmtree.side_effect = self.mydel
107+ with util.ChrootableTarget(self.target):
108+ tp = paths.target_path(self.target, path='/etc/resolv.conf')
109+ target_conf = util.load_file(tp)
110+ self.assertEqual(self.host_content, target_conf)
111+
112+ self.assertEqual(0, m_rename.call_count)
113+
114+ def test_chrootable_target_restores_resolvconf_on_copy_fail(self):
115+ content = "target_resolvconf"
116+ tconf = os.path.join(self.target, 'etc/resolv.conf')
117+ util.write_file(tconf, content)
118+
119+ self.m_shutil.copy.side_effect = OSError('Failed to copy')
120+ self.m_shutil.rmtree.side_effect = self.mydel
121+
122+ try:
123+ with util.ChrootableTarget(self.target):
124+ pass
125+ except OSError:
126+ pass
127+
128+ target_conf = util.load_file(tconf)
129+ self.assertEqual(content, target_conf)
130+
131+
132 class TestLoadFile(CiTestCase):
133 """Test utility 'load_file'"""
134

Subscribers

People subscribed via source and target branches