Merge ~sergiodj/ubuntu/+source/memcached:sasl-configuration-wrong-path-bug1878721-focal into ubuntu/+source/memcached:ubuntu/focal-devel
Status: | Merged |
---|---|
Approved by: | Bryce Harrington |
Approved revision: | 3cdd6b530852570c87fdbbd4a902e5f3c37636a1 |
Merged at revision: | 3cdd6b530852570c87fdbbd4a902e5f3c37636a1 |
Proposed branch: | ~sergiodj/ubuntu/+source/memcached:sasl-configuration-wrong-path-bug1878721-focal |
Merge into: | ubuntu/+source/memcached:ubuntu/focal-devel |
Diff against target: |
158 lines (+124/-1) 4 files modified
debian/changelog (+18/-0) debian/control (+2/-1) debian/patches/fix-bug-where-sasl-will-load-config-the-wrong-path.patch (+103/-0) debian/patches/series (+1/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Bryce Harrington (community) | Approve | ||
Anders Kaseorg (community) | Approve | ||
Canonical Server Core Reviewers | Pending | ||
Review via email: mp+384764@code.launchpad.net |
Description of the change
This is a simple regression that happens on memcached. The SASL configuration is being read from an erroneous file, '/etc/sasl2/
The reporter was thorough and provided the debdiff along with an SRU template, so one could say I'm filing this MP for him. I kept his authorship intact.
There is a PPA with the proposed change here:
https:/
autopkgtest is still happy:
autopkgtest [08:50:52]: test client.pl: -------
autopkgtest [08:50:53]: test client.pl: - - - - - - - - - - results - - - - - - - - - -
client.pl PASS
autopkgtest [08:50:54]: @@@@@@@
daemon PASS
client.pl PASS
* Changelog:
- [√] old content and logical tag match as expected
- [√] changelog entry correct version and targeted codename
- [√] changelog entries correct
- [x] update-maintainer has been run
* Actual changes:
- [-] no upstream changes to consider
- [-] no further upstream version to consider
- [√] debian changes look safe
* Old Delta:
- [-] dropped changes are ok to be dropped
- [-] nothing else to drop
- [√] changes forwarded upstream/debian (if appropriate)
* New Delta: patches/ series
- [-] no new patches added
- [√] patches match what was proposed upstream
- [√] patches correctly included in debian/
- [√] patches have correct DEP3 metadata
* Build/Test:
- [√] build is ok
- [√] verified PPA package installs/uninstalls
- [√] autopkgtest against the PPA package passes
- [x] sanity checks test fine
A few questions / things to fix. First, the packaging work itself looks correct, except for one detail: Make sure to run update-maintainer, since this is the first ubuntu version after debian's. The eoan package is ok since it had a ubuntu version previously.
Just a minor nitpick, but I noticed a typo in a comment in the patch itself:
...actual configue file...
In a fresh lxc container I ran the commands verbatim as given in the SRU bug report, on both focal and eoan, and got the error message:
root@triage- focal:/ home/bryce# systemctl restart memcached focal:/ home/bryce# memcping --servers=127.0.0.1 --binary --username=foo --password=bar
root@triage-
Failed to ping 127.0.0.1:11211 UNKNOWN READ FAILURE
I experimented with some different username/password combos, and tried the config file in a few different places but didn't get a different result. Could you verify that the test case works for you in lxc? Maybe the setup directions are missing a step?
I wonder if the patch and test case may deserve to be expanded on a bit. As I understand it, normally the config file should be stored in a file named /etc/sasl2/ memcached. conf, but the problem is that due to a bug /etc/sasl2/ memcached. conf/ may have inadvertently been created as a subdirectory, and memcached.conf placed inside of that. So the purpose of the patch is to make memcached check the incorrect location just in case. Is that understanding correct?
Assuming it is, I would suggest saying basically this in the patch description. It might be worth also explaining in the debian changelog entry, at least mentioning the bad path by name and that we're working around it. Just saying "Fix the path" is omitting some pertinent details. ;-)
Another thing I notice in the patch is that it is also checking for if the config file was copied to /etc/sasl or /etc/sasl2. I.e. where those are supposed to be directories they've been created as files. I can see how this could be a common error, however the bug report doesn't describe this case. I wonder under what circumstances this issue was cropping up - do we have any ubuntu bug reports about this happening in the wild? Or can we point to a commit upstream that introduced the problem originally?
For the test case in the SRU bug, I notice it is testing the case where the config file is at /etc/s...