Init script fails test on reload/restart because of faulty regex
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
| squid3 (Ubuntu) |
Undecided
|
Unassigned | ||
| Xenial |
Medium
|
Andreas Hasenack | ||
| Zesty |
Medium
|
Unassigned |
Bug Description
[Impact]
The squid initscript has a guard against configuration mistakes that prevents the service from being disrupted if the current config has an invalid setting.
This guard relies on the "squid -k parse" command which analyzes the configuration and, in the case of a fatal problem, outputs the string "FATAL: <reason>". The initscript parses that output to catch such errors before further action is taken.
There is a mistake in the expression that is looked for, though: instead of "FATAL: ", the initscript is looking for "FATAL " (i.e., no ":"). The consequence is that actions that would reload or restart the service end up shutting the service down in the case of a configuration error.
The change fixes the expression that is looked for, restoring the functionality of the guard.
[Test Case]
* install squid:
sudo apt update && sudo apt install squid -y
* confirm the reload action is quick to return and doesn't change the pid of squid, i.e., it's not restarted:
ubuntu@
2684
ubuntu@
ubuntu@
2684
ubuntu@
* add an invalid setting to the config file:
echo "acl nonsense nonsense nonsense" | sudo tee -a /etc/squid/
* reload squid one more time:
sudo service squid reload
* After about 30s, squid should be dead and service squid status should show errors:
ubuntu@
ubuntu@
ubuntu@
● squid.service - LSB: Squid HTTP Proxy version 3.x
(...)
Oct 29 19:56:26 xenial-
Oct 29 19:56:26 xenial-
Oct 29 19:56:26 xenial-
Oct 29 19:56:26 xenial-
With the fixed package, a reload action (for example) will flag the error, and keep the service running:
ubuntu@
3801
ubuntu@
Job for squid.service failed because the control process exited with error code. See "systemctl status squid.service" and "journalctl -xe" for details.
ubuntu@
1
ubuntu@
3801
And the status message will be much clearer:
ubuntu@
● squid.service - LSB: Squid HTTP Proxy version 3.x
...
Oct 29 20:13:26 xenial-
Oct 29 20:13:26 xenial-
Oct 29 20:13:26 xenial-
Oct 29 20:13:26 xenial-
Oct 29 20:13:26 xenial-
[Regression Potential]
This changes not only the reload action, but also start and consequently restart. The fix makes an existing safety net around those actions actually work, instead of being ignored.
Due to the lack of an explicit restart action in systemd, however, the restart guard in the SysV initscript doesn't take effect. If there is a bad config, and squid is restarted, the service will be in a stopped state at the end:
- stop will succeed
- start will fail: service remains stopped
There is an upstream bug about it: https:/
This is not a regression, however, as it's the same behavior as before, but it will have an interesting consequence in package upgrades.
With the old package, admins upgrading squid to a newer version while having a syntax error in their configs will end up with a stopped service, but no notification of that, since the restart postinst action will exit 0.
With the new package, the same scenario will trigger an upgrade failure like this:
dpkg: error processing package squid (--configure):
subprocess installed post-installation script returned error exit status 1
Processing triggers for systemd (229-4ubuntu21.5) ...
Processing triggers for ureadahead (0.100.0-19) ...
Errors were encountered while processing:
squid
E: Sub-process /usr/bin/dpkg returned an error code (1)
So the end result is the same as before, in the sense that squid won't be running after the upgrade, but now it will be clear. I expect this can trigger some apport bug reports, though.
[Other Info]
Not at this time.
[Original Description]
This is a very serious issue that got fixed upstream in:
https:/
It is also logged in the Ubuntu changelog as fixed in:
squid3 (3.5.12-1) unstable; urgency=medium
[ Mathieu Parent ]
* Fix FATAL parsing before start/reload/
But is in fact not fixed.
When I look in the source package I find two init scripts:
squid3.rc and squid.rc. squid3.rc has the patch while squid.rc does not.
The one being included in the package and deployed is the one that does not have the fix.
I'm including a patch to fix this issue.
Please push this out ASAP.
Related branches
- Christian Ehrhardt : Approve on 2018-10-31
- Canonical Server Team: Pending requested 2018-10-31
- Canonical Server packageset reviewers: Pending requested 2018-10-30
-
Diff: 110 lines (+25/-14)5 files modifieddebian/changelog (+12/-0)
debian/squid.rc (+3/-3)
debian/tests/control (+1/-1)
debian/tests/squid (+5/-0)
debian/tests/test-squid.py (+4/-10)
Tommy Nevtelen (dal) wrote : | #1 |
Tommy Nevtelen (dal) wrote : | #3 |
This appears to be fixed in artful+
Christian Ehrhardt (paelzer) wrote : | #4 |
I can confirm that this is missing in Xenial&Zesty but Fixed in Artful+ as mentioned in comment #3.
The fix came in by:
https:/
That is in 3.15.12 and Xenial has 3.5.12-1ubuntu7.
But due to the different times the renaming squid3/squid happened Xenial and Zesty are affected.
One would need to apply above linked patch to debian/squid.rc (without the 3).
@Rbasak - you might have some extra context on that renaming history, I'm subscribing you and Ubuntu-server would you mind to take a look?
Christian Ehrhardt (paelzer) wrote : | #5 |
Note: the attached patch is incomplete, but the linked change should be good.
Robie Basak (racb) wrote : | #6 |
Tommy, thank you for bringing our attention to this.
It looks like this issue *is* fixed in the current stable Ubuntu release. Perhaps you were looking at the changelog against a newer release of Ubuntu than the one you are using?
> This is a very serious issue...
Why is this "a very serious issue"? It seems to me that it only affects invalid configurations so should only affect development and testing of deployments rather than in production? The workaround (if it can be called that) is to fix the invalid configuration.
The fix does look reasonable to SRU though. I wonder if https:/
Changed in squid3 (Ubuntu): | |
status: | New → Fix Released |
Changed in squid3 (Ubuntu Xenial): | |
status: | New → Triaged |
Changed in squid3 (Ubuntu Zesty): | |
status: | New → Triaged |
Changed in squid3 (Ubuntu Xenial): | |
importance: | Undecided → Medium |
Changed in squid3 (Ubuntu Zesty): | |
importance: | Undecided → Medium |
Tommy Nevtelen (dal) wrote : Re: [Bug 1738412] Re: Init script fails test on reload/restart because of faulty regex | #7 |
On 2018-01-02 15:38, Robie Basak wrote:
> Tommy, thank you for bringing our attention to this.
>
> It looks like this issue *is* fixed in the current stable Ubuntu
> release. Perhaps you were looking at the changelog against a newer
> release of Ubuntu than the one you are using?
>> This is a very serious issue...
> Why is this "a very serious issue"? It seems to me that it only affects
> invalid configurations so should only affect development and testing of
> deployments rather than in production? The workaround (if it can be
> called that) is to fix the invalid configuration.
I kind of hope that this is a joke :)
You know sometimes things don't go as intended, that's why the check is
there.
> The fix does look reasonable to SRU though. I wonder if
> https:/
> squid.git/
> separate bug though, or we may need an additional test case for it.
So it has been a while now. What's the next step to get this in?
The fix is really trivial. Can I help with something to get it rolling?
--
TN
Robie Basak (racb) wrote : | #8 |
On Thu, Mar 15, 2018 at 11:42:49PM -0000, Tommy Nevtelen wrote:
> On 2018-01-02 15:38, Robie Basak wrote:
> >> This is a very serious issue...
> > Why is this "a very serious issue"? It seems to me that it only affects
> > invalid configurations so should only affect development and testing of
> > deployments rather than in production? The workaround (if it can be
> > called that) is to fix the invalid configuration.
> I kind of hope that this is a joke :)
Sorry, no, it wasn't a joke.
> You know sometimes things don't go as intended, that's why the check is
> there.
I don't disagree. It is a bug and as I said I have no objection to
getting it fixed in older stable releases if somebody wants to drive
that. But I maintain it is not a serious issue, because as I explained
it only causes some inconvenience to users during development and
testing and does not affect production systems. Claiming it is a serious
issue dilutes attention from issues that really are serious, which is
why I objected to that statement in this case.
> So it has been a while now. What's the next step to get this in?
> The fix is really trivial. Can I help with something to get it rolling?
Please see https:/
fix may be trivial, but unfortunately updating the stable release is
not. The risk of regressing users who expect stability means that we
have to take considerable care. For example, an unrelated latent bug
could be exposed by a rebuild. See
https:/
Andreas Hasenack (ahasenack) wrote : | #9 |
Zesty is EOL.
Changed in squid3 (Ubuntu Zesty): | |
status: | Triaged → Won't Fix |
Changed in squid3 (Ubuntu Xenial): | |
assignee: | nobody → Andreas Hasenack (ahasenack) |
status: | Triaged → In Progress |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
description: | updated |
Hello Tommy, or anyone else affected,
Accepted squid3 into xenial-proposed. The package will build now and be available at https:/
Please help us by testing this new package. See https:/
If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-
Further information regarding the verification process can be found at https:/
Changed in squid3 (Ubuntu Xenial): | |
status: | In Progress → Fix Committed |
tags: | added: verification-needed verification-needed-xenial |
Andreas Hasenack (ahasenack) wrote : | #11 |
The DEP8 tests in http://
For example, see https:/
badpkg: Test dependencies are unsatisfiable. A common reason is that your testbed is out of date with respect to the archive, and you need to use a current testbed or run apt-get update or use -U.
That and a few other things is what was fixed. The tests now run locally at least, with autopkgtest. The current failure in the autopkgtest infrastructure from Ubuntu is firewall related, as some tests try to reach out to public facing websites like www.ubuntu.com:
FAIL: test_http_proxy (__main_
Test http
-------
Traceback (most recent call last):
File "/tmp/autopkgte
self.
File "/tmp/autopkgte
self.
File "/tmp/autopkgte
self.
AssertionError: Could not find "Canonical"
The requested URL could not be retrieved
----
The following error was encountered while trying to retrieve the URL:
[1]http://
Connection to 91.189.89.118 failed.
The system returned: (110) Connection timed out
The remote host or network may be down. Please try the request again.
Your cache administrator is [2]webmaster.
----
Generated Wed, 31 Oct 2018 19:09:52 GMT by autopkgtest (squid/3.5.12)
I don't think the above is worth fixing at the moment, because:
a) it's a long process: reject package from proposed, prepare new one, upload, get sru team to approve, then let it lose in the autopkgtest infrastructure to see if it works
b) bileto isn't working for this for some reason, I can't get test results from it. It just says "test running (always failed)", and no logs.
c) package that is currently in proposed is already an improvement, as the test now can be run locally (and pass).
Andreas Hasenack (ahasenack) wrote : | #12 |
xenial verification
confirming the bug:
root@xenia-
squid:
Installed: 3.5.12-1ubuntu7.5
Candidate: 3.5.12-1ubuntu7.5
Version table:
*** 3.5.12-1ubuntu7.5 500
500 http://
# confirming reload doesn't restart or kill the service
ubuntu@
4280
ubuntu@
ubuntu@
4280
# corrupting the config file and trying one more time
ubuntu@
acl nonsense nonsense nonsense
ubuntu@
ubuntu@
ubuntu@
4280
ubuntu@
ubuntu@
Indeed after a while squid isn't running anymore.
Status shows it failed:
ubuntu@
● squid.service - LSB: Squid HTTP Proxy version 3.x
Loaded: loaded (/etc/init.d/squid; bad; vendor preset: enabled)
Active: active (exited) since Mon 2018-11-12 18:49:18 UTC; 13min ago
...
Nov 12 18:51:56 xenia-squid-
Nov 12 18:51:56 xenia-squid-
Now retrying with the new package from proposed:
ubuntu@
squid:
Installed: 3.5.12-1ubuntu7.6
Candidate: 3.5.12-1ubuntu7.6
Version table:
*** 3.5.12-1ubuntu7.6 500
500 http://
The upgrade failed as expected, because the config file is corrupted and now it's properly detected and it shows in the output:
Setting up squid-common (3.5.12-1ubuntu7.6) ...
Setting up squid (3.5.12-1ubuntu7.6) ...
Installing new version of config file /etc/init.d/squid ...
Job for squid.service failed because the control process exited with error code. See "systemctl status squid.service" and "journalctl -xe" for details.
invoke-rc.d: initscript squid, action "restart" failed.
● squid.service - LSB: Squid HTTP Proxy version 3.x
Loaded: loaded (/etc/init.d/squid; bad; vendor preset: enabled)
Active: failed (Result: exit-code) since Mon 2018-11-12 19:04:20 UTC; 4ms ago
Docs: man:systemd-
Process: 5293 ExecStop=
Process: 5306 ExecStart=
Nov 12 19:04:20 xenia-squid-
Nov 12 19:04:20 xenia-squid-
Nov 12 19:04:20 xenia-squid-
(...)
Retrying the original test steps, starting with a fixed config file and then mangling it:
ubuntu@
tags: |
added: verification-done-xenial removed: verification-needed-xenial |
The verification of the Stable Release Update for squid3 has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.
Launchpad Janitor (janitor) wrote : | #14 |
This bug was fixed in the package squid3 - 3.5.12-1ubuntu7.6
---------------
squid3 (3.5.12-1ubuntu7.6) xenial; urgency=medium
* d/squid.rc: fix regexp for catching FATAL errors (LP: #1738412)
* d/t/test-squid.py: in xenial, initscript, apparmor profile, pidfile and
process are named squid, not squid3. Get rid of the multiple distro
logic since these tests will be only run on xenial.
* d/t/control: drop uneeded dependency on python-unit.
* d/t/squid: use a shorter shutdown timeout for the tests, so they
run faster
-- Andreas Hasenack <email address hidden> Wed, 31 Oct 2018 09:22:14 -0300
Changed in squid3 (Ubuntu Xenial): | |
status: | Fix Committed → Fix Released |
The attachment "Fixing it" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.
[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]