Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not crash when /etc/resolv.conf is immutable (bsc#1096142) #103

Merged
merged 6 commits into from
Aug 21, 2018

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Jun 14, 2018

  • See https://bugzilla.suse.com/show_bug.cgi?id=1096142
  • According to the reporter the system was gradually upgraded from 42.2 and uses some non-default settings (OpenDNS servers) so the bug should not affect many users => master should be enough
  • 4.1.0

@coveralls
Copy link

coveralls commented Jun 14, 2018

Coverage Status

Coverage increased (+0.09%) to 18.384% when pulling c61b6ad on eperm_fix into 0ade9a2 on master.

@@ -1718,7 +1718,13 @@ def create_backup
# the network connection works for the chrooted scripts
def inject_intsys_files
# the original file is backed up and restored later
::FileUtils.cp(RESOLV_CONF, File.join(Installation.destdir, RESOLV_CONF)) if File.exist?(RESOLV_CONF)
target = File.join(Installation.destdir, RESOLV_CONF)
::FileUtils.cp(RESOLV_CONF, target) if File.exist?(RESOLV_CONF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we are touching it we should also check if resolv.conf is not just symlink like new network manager or systemd-resolver doing and in such case also do not touch it.

Copy link
Member Author

@lslezak lslezak Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ummm, and will then the name resolution work in the /mnt chroot?

I mean when using standard wicked configration the resolv.conf looks like this if the system is not up:

### /etc/resolv.conf file autogenerated by netconfig!
#
# Before you change this file manually, consider to define the
# static DNS configuration using the following variables in the
# /etc/sysconfig/network/config file:
#     NETCONFIG_DNS_STATIC_SEARCHLIST
#     NETCONFIG_DNS_STATIC_SERVERS
#     NETCONFIG_DNS_FORWARDER
# or disable DNS configuration updates via netconfig by setting:
#     NETCONFIG_DNS_POLICY=''
#
# See also the netconfig(8) manual page and other documentation.
#
# Note: Manual change of this file disables netconfig too, but
# may get lost when this file contains comments or empty lines
# only, the netconfig settings are same with settings in this
# file and in case of a "netconfig update -f" call.
#
### Please remove (at least) this line when you modify the file!

So it's only an empty template and the name resolution will not work!

The question is how the other tools behave and whether the name resolution will work if we skip copying the file if a symlink is detected.

We need the name resolution for the registration rollback, we basically run chroot /mnt SUSEConnect --rollback. With empty or invalid resolv.conf this won't work... 😟

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand when you overwrite file, but I have to be sure that when you do backup it respect symlink and also in this case that cp never write to symlink target.
try this in shell ( scenario we have to avoid ):

jreidinger@linux-vvcf:/tmp> touch a
jreidinger@linux-vvcf:/tmp> ln -s a b
jreidinger@linux-vvcf:/tmp> cat a
jreidinger@linux-vvcf:/tmp> cat b
jreidinger@linux-vvcf:/tmp> echo "test" > c
jreidinger@linux-vvcf:/tmp> cp c b
jreidinger@linux-vvcf:/tmp> cat a
test
jreidinger@linux-vvcf:/tmp> cat b
test

end
end

context "resolf.conf does not exist in inst-sys" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolv.conf

allow(Yast::Installation).to receive(:destdir).and_return("/mnt")
end

context "resolf.conf exists in inst-sys" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolv.conf

allow(FileUtils).to receive(:cp)
end

it "copies the resolf.conf from inst-sys to the target" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolv.conf

expect(File).to receive(:exist?).with("/etc/resolv.conf").and_return(false)
end

it "does not copy the resolf.conf" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolv.conf

Copy link
Contributor

@teclator teclator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me looks good, just a NP typo and please fix the test failing in Travis

@jreidinger jreidinger merged commit 10ca38a into master Aug 21, 2018
@jreidinger jreidinger deleted the eperm_fix branch August 21, 2018 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants