Merge lp://staging/~gary-wzl77/squid/snap_package into lp://staging/squid/3.5

Proposed by Gary.Wang
Status: Rejected
Rejected by: Amos Jeffries
Proposed branch: lp://staging/~gary-wzl77/squid/snap_package
Merge into: lp://staging/squid/3.5
Diff against target: 584 lines (+409/-8)
13 files modified
README.snap (+29/-0)
configure.ac (+10/-0)
snap/snapcraft.yaml (+83/-0)
snap/src/squid/conf/squid.conf.template (+116/-0)
snap/src/squid/script/run-squid (+52/-0)
snap/src/squid/script/settings (+39/-0)
src/cache_cf.cc (+6/-0)
src/cf_gen_defines (+1/-0)
src/errorpage.cc (+8/-2)
src/ipc/mem/Segment.cc (+16/-0)
src/main.cc (+20/-4)
src/mime.cc (+7/-1)
src/tools.cc (+22/-1)
To merge this branch: bzr merge lp://staging/~gary-wzl77/squid/snap_package
Reviewer Review Type Date Requested Status
Amos Jeffries Disapprove
Alex Rousskov Disapprove
Review via email: mp+318302@code.staging.launchpad.net

Commit message

Enable to package and compile squid in snap world.

1.Added conditional for snap packaging by testing "--enable-snap".
As all services run as root thanks to confinement in snap world,
so we need to get rid of uid, gid configured,
otherwise there will be bunch of apparmor DENIED issue when running
this snap in confined mode.
2.Fixed bunch of critical conf file reading path.
3.Make sure writeable path for some file reading. e.g pidfile
4.Make sure sem_open available inside snap. see
    https://bugs.launchpad.net/snappy/+bug/1653955

Description of the change

Enable to package and compile squid in snap world.

1.Added conditional for snap packaging by testing "--enable-snap".
As all services run as root thanks to confinement in snap world,
so we need to get rid of uid, gid configured,
otherwise there will be bunch of apparmor DENIED issue when running
this snap in confined mode.
2.Fixed bunch of critical conf file reading path.
3.Make sure writeable path for some file reading. e.g pidfile
4.Make sure sem_open available inside snap. see
    https://bugs.launchpad.net/snappy/+bug/1653955

This PR is still using the master branch as squid source in snapraft.yaml file.
As a PR, I don't change it as it targets to merge into master.
You can simply make a change as following to use my branch for testing purpose for the time being.
http://paste.ubuntu.com/24063880/

To post a comment you must log in.
Revision history for this message
Alex Rousskov (rousskov) wrote :

The patch or bundle should probably be posted and discussed on squid-dev mailing list rather than here.

FWIW, I am against the proposed "let's sprinkle the code with snap-only hacks and add a bunch of snap-only files that developers will have to somehow maintain" approach. I hope this work can be refactored into two pieces:

1. A stand-alone package with a custom Squid configuration file template (if really needed) and possibly snap-specific Squid build options. The Squid Project will not maintain this package but official Squid documentation can refer to it.

2. A _minimum_ set of generally-useful Squid changes that make #1 possible. These changes will be committed and maintained by the Squid Project, of course. This should be done without adding a single monolithic set of options tied to the snap environment.

review: Disapprove
Revision history for this message
Amos Jeffries (yadi) wrote :

Just so it is clear why I'm rejecting this outright:

* see review of previous proposal https://code.launchpad.net/~cprov/squid/snap/+merge/297663. All of Alex's and Robert's comments about packaging still apply.
 For the purposes of snap packaging your upstreams are in this order: the Ubuntu Server Team, the Debian pkg-squid Team, then Squid Project.

* SNAP does not change any of the legal restrictions that have long prevented Debian/Ubuntu distrbuting SSL/TLS enabled Squid packages. You are free to build your own binaries, but Debian/Ubuntu are forbidden from distribution. Please drop all the OpenSSL related parts, there is no chance of a merge while they remain.

review: Disapprove
Revision history for this message
Gary.Wang (gary-wzl77) wrote :

Thanks, Alex and Amos for your comments.

@Alex, about your suggestions, I think it makes sense to me.
But
"...This should be done without adding a single monolithic set of options tied to the snap environment..."
That's impossible if we're gonna release squid snap in stable channel of ubuntu store. Because the ultimate goal is to publish squid snap into the stable channel so that normal user can let it run with the strict security confinement.
And except custom Squid configuration file template, all the changes I made in squid3.5 are related to sth which let normal users run this snap in strict confinement mode.

Strict confinement gives you the following readable and/or writable paths:
1. /snap/<snap>/<revision> (read-only, snap install path)
    Hence I need to pre-append $SNAP to the following file path to make these files can be found and read by squid.
     *mime table file
     *error template file.
     *conf file
     *icon directory
2./var/snap/<snap>/<revision> (read/write, per-revision data)
    Hence I need to pre-append $SNAP_DATA to the pid file path to make sure it's in writable path.
3.As all services run as root due to confinement in snap world
    Hence I need to get rid of setresuid/setuid/setgid sys call otherwise appapmor denies occurs.
4. In order to make shm_open available in snap world, need to make share memory file name declared according to the required namespace
     Please see https://bugs.launchpad.net/snappy/+bug/1653955

If any above changes are not made, squid daemon failed to run in strict confinement mode.
Snap that can only read and write in its own namespace is recommended and enforced, if we wanna publish it into stable channel.
That's why I made this change here.

Revision history for this message
Alex Rousskov (rousskov) wrote :

>> should be done without adding a single monolithic set of options tied to the snap environment

> That's impossible

I obviously think it is possible. AFAICT, your "Because the ultimate goal is to publish squid snap into the stable channel" explanation does not prove that the only way to achieve this ultimate goal is to have a "monolithic set of options tied to the snap environment".

> Snap that can only read and write in its own namespace is recommended and enforced, if we wanna publish it into stable channel.

I am not against teaching Squid to obey some kind of a "namespace". We already have "./configure --prefix" and "squid -n". It is possible that more knobs like that are needed (but it is on you to prove that the existing knobs are insufficient or too awkward to use in a snap context -- I do not see that proof in your comments but please point me to it if I missed it).

To better understand why I do not like your implementation, take a step back and assume that there is not just one snap-like environment, but ten. It does not matter what they are called. You can call them Snap v1, Snap v2, ... or Docker, Snap, Chroot, ... or something else. Do we want to add --enable-snap-v1, --enable-snap-v2, --enable-snap-v3, ... options and then deal with all their weird combinations in the code, while being unable to test most of them? No. Does that "no" mean that we do not want to support Snap v1, Snap v2, ...? No! It only means that you need to (a) use existing configuration knobs and (b) generalize the missing knobs that you want to add.

For example, if --prefix and/or -n are enough to support snap "install path", use those existing configuration features. If they are not enough, describe what is missing in generalized terms and propose adding knobs for that generally useful support (using snap as an example).

Similarly, do you need a custom prefix for shared memory segment names? Does Squid already support a customer prefix for those names? If yes/no, then it would be OK to propose a ./configure or runtime option that adds such support, but that option is not going to be --enable-snap, it would be something like --ipc-prefix.

Hope this clarifies. To avoid doing a lot of work that is going to be rejected at the end, I recommend getting a preliminary Project approval in advance, based on a brief description of what Squid changes you want to make and _why_ they are necessary.

N.B. As for licensing conflicts, I do not yet understand why your proposal would create any new ones, but I will leave it for you and Amos to battle that out.

Unmerged revisions

14144. By Gary.Wang

Enable to package and compile squid in snap world.

1.Added conditional for snap packaging by testing "--enable-snap".
As all services run as root thanks to confinement in snap world,
so we need to get rid of uid, gid configured,
otherwise there will be bunch of apparmor DENIED issue when running
this snap in confined mode.
2.Fixed bunch of critical conf file reading path.
3.Make sure writeable path for some file reading. e.g pidfile
4.Make sure sem_open available inside snap. see
    https://bugs.launchpad.net/snappy/+bug/1653955

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: