Bug 16520 - Per-virtualhost SetEnvs don't work with Plack
Summary: Per-virtualhost SetEnvs don't work with Plack
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jesse Weaver
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-13 22:24 UTC by Jesse Weaver
Modified: 2018-02-09 08:53 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16520: Allow per-VirtualHost environment variables with Plack (6.06 KB, patch)
2016-05-13 22:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16520: Initialize the sysprefs Koha::Cache on demand (1.41 KB, patch)
2016-05-13 22:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files (2.04 KB, patch)
2016-05-13 22:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16520: Fix the Apache Plack config to load the main page through Plack (1.94 KB, patch)
2016-05-13 22:50 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16520: Allow per-VirtualHost environment variables with Plack (6.12 KB, patch)
2016-05-17 15:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16520: Initialize the sysprefs Koha::Cache on demand (1.47 KB, patch)
2016-05-17 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files (2.09 KB, patch)
2016-05-17 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16520: Fix the Apache Plack config to load the main page through Plack (1.99 KB, patch)
2016-05-17 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16520: (followup) use __MEMCACHED_NAMESPACE__ where appropriate (1.52 KB, patch)
2016-05-17 16:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16520: Allow per-VirtualHost environment variables with Plack (6.09 KB, patch)
2016-10-13 08:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16520: Initialize the sysprefs Koha::Caches on demand (1.23 KB, patch)
2016-10-13 08:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files (970 bytes, patch)
2016-10-13 08:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16520 (Follow up) Rebase fixes (2.06 KB, patch)
2016-10-13 08:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16520: Allow per-VirtualHost environment variables with Plack (6.15 KB, patch)
2016-10-13 13:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16520: Allow per-VirtualHost environment variables with Plack (6.15 KB, patch)
2016-10-13 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16520: Initialize the sysprefs Koha::Caches on demand (1.28 KB, patch)
2016-10-13 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files (1.00 KB, patch)
2016-10-13 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16520 (Follow up) Rebase fixes (2.11 KB, patch)
2016-10-13 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16520 - (Follow up) Restore retrieval of syspref_cache (625 bytes, patch)
2016-11-09 11:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16520 - (Follow up) Restore retrieval of syspref_cache (687 bytes, patch)
2016-11-09 11:27 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Weaver 2016-05-13 22:24:37 UTC
The various SetEnvs that can be used in Koha to configure memcached, change search limits, override sysprefs and add custom stylesheets don't work in Plack. It is likely impossible to fix this with the current HTTP proxy setup, so we need a workaround.
Comment 1 Jesse Weaver 2016-05-13 22:50:13 UTC Comment hidden (obsolete)
Comment 2 Jesse Weaver 2016-05-13 22:50:18 UTC Comment hidden (obsolete)
Comment 3 Jesse Weaver 2016-05-13 22:50:21 UTC Comment hidden (obsolete)
Comment 4 Jesse Weaver 2016-05-13 22:50:25 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2016-05-17 15:59:56 UTC
Created attachment 51576 [details] [review]
Bug 16520: Allow per-VirtualHost environment variables with Plack

This allows MEMCACHED_*, OVERRIDE_SYSPREF_* and others to work properly.

Test plan:

  1) Add the following line to your plack.psgi (near the bottom, just
     above "mount ..."):
       enable "+Koha::Middleware::Plack";
  2) Load the OPAC advanced search page (under Plack). The title should
    read "Koha online catalog" (or whatever your LibraryName syspref
    contains).
  3) Add the following to your Apache configuration:
       RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato"
  4) Restart Apache.
  5) Refresh. The title should now read "Potato, Potato online catalog".

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2016-05-17 16:00:04 UTC
Created attachment 51577 [details] [review]
Bug 16520: Initialize the sysprefs Koha::Cache on demand

This is needed so that the cache singleton doesn't get created before
the MEMCACHED_* environment variables are set.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2016-05-17 16:00:10 UTC
Created attachment 51578 [details] [review]
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files

This was not added to the old etc/koha-httpd.conf because that file has
no infrastructure for Plack anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2016-05-17 16:00:17 UTC
Created attachment 51579 [details] [review]
Bug 16520: Fix the Apache Plack config to load the main page through Plack

Previously, the ProxyPass would not pass the OPAC/staff homepage to
Plack. This fixes this problem (which is necessary for the other changes
on this bug to show on the main page).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2016-05-17 16:00:23 UTC
Created attachment 51580 [details] [review]
Bug 16520: (followup) use __MEMCACHED_NAMESPACE__ where appropriate

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2016-05-17 16:07:10 UTC
I tested this patches and I verified they correctly make the ENV variables reach the plack backend. Thus, the OVERRIVE_SYS* work again, and the memcached variables are correctly set.
To verify the latter, I wrote a small patch that would go to another bug to display memcached configuration/status in about.pl.
Comment 11 Jonathan Druart 2016-05-21 11:30:25 UTC
Jesse,
It does not work for me, could you please have a look at the test plan and make sure all tests are listed?
For instance, it seems that debian/templates/apache-shared-intranet-plack.conf and debian/templates/apache-shared-opac-plack.conf need to be copied (I did but still does not work).

Code review:
Should not we test if the headers mod is enabled in debian/templates/apache-site.conf.in?
Comment 12 Jonathan Druart 2016-05-21 11:30:41 UTC
(In reply to Jonathan Druart from comment #11)
> Jesse,
> It does not work for me, could you please have a look at the test plan and
> make sure all tests are listed?

s/tests/steps
Comment 13 Tomás Cohen Arazi 2016-05-21 11:35:56 UTC
Jonathan, what i did for testing was applying the changes manually to my kohadevbox instance. I agree the headers module should be correctly checked. 
The patched config files worked for me.
Comment 14 Jonathan Druart 2016-05-21 11:58:12 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> Jonathan, what i did for testing was applying the changes manually to my
> kohadevbox instance. I agree the headers module should be correctly checked. 
> The patched config files worked for me.

Yep I have double checked everything and it now works.
Comment 15 Jonathan Druart 2016-05-21 12:00:08 UTC
I have to admit I am a bit lost with all the apache config files, but should not we move the RequestHeader add X-Koha-SetEnv lines to apache-shared-[intranet|opac]-plack.conf files?
Comment 16 Jonathan Druart 2016-05-21 12:02:31 UTC
In C4::Context->preference, the lc $var has been moved after the OVERRIDE_SYSPREF_* check, I don't think it's expected.
Comment 17 Jonathan Druart 2016-05-21 12:05:52 UTC
108     # We also add the MEMCACHED_ settings to the actual environment, to make sure any early
109     # initialization of Koha::Cache correctly sets up a memcached connection.
110     foreach my $special_var ( qw( MEMCACHED_SERVERS MEMCACHED_NAMESPACE ) ) {
111         $ENV{$special_var} = $setenvs{$special_var} if defined $setenvs{$special_var};
112     }

Jesse, could you explain in which cases this is useful?
Comment 18 Jesse Weaver 2016-05-25 08:53:00 UTC
(In reply to Jonathan Druart from comment #15)
> I have to admit I am a bit lost with all the apache config files, but should
> not we move the RequestHeader add X-Koha-SetEnv lines to
> apache-shared-[intranet|opac]-plack.conf files?

I'm inclined to agree; at first I was thinking to have an example of the new syntax in the configuration files, but given that this should be wrapped in an <IfModule>, it should be moved there.

(In reply to Jonathan Druart from comment #16)
> In C4::Context->preference, the lc $var has been moved after the
> OVERRIDE_SYSPREF_* check, I don't think it's expected.

This is in tune with the documentation and how this feature has been historically used; see http://manual.koha-community.org/3.22/en/administration.html#globalsysprefs .

(In reply to Jonathan Druart from comment #17)
> 108     # We also add the MEMCACHED_ settings to the actual environment, to
> make sure any early
> 109     # initialization of Koha::Cache correctly sets up a memcached
> connection.
> 110     foreach my $special_var ( qw( MEMCACHED_SERVERS MEMCACHED_NAMESPACE
> ) ) {
> 111         $ENV{$special_var} = $setenvs{$special_var} if defined
> $setenvs{$special_var};
> 112     }
> 
> Jesse, could you explain in which cases this is useful?

C4::Auth, in my testing, loads up some system preferences early on that cause Koha::Cache to be initialized before the Plack environment is copied to %ENV (presumably by the Plack CGI wrapper?).
Comment 19 Jesse Weaver 2016-07-06 17:12:58 UTC
I was poking at this, and one issue with moving those sysprefs is that the apache-*-plack.conf files are not .in files, so __MEMCACHED_NAMESPACE__ is not replaced in them. I could rejigger that, but any strenuous objections to leaving the RequestHeader lines where I currently have them?
Comment 20 Nick Clemens 2016-10-13 08:38:57 UTC
Created attachment 56309 [details] [review]
Bug 16520: Allow per-VirtualHost environment variables with Plack

This allows OVERRIDE_SYSPREF_* and others to work properly.

Test plan:

  1) Add the following line to your plack.psgi (near the bottom, just
     above "mount ..."):
       enable "+Koha::Middleware::Plack";
  2) Load the OPAC advanced search page (under Plack). The title should
    read "Koha online catalog" (or whatever your LibraryName syspref
    contains).
  3) Add the following to your Apache configuration:
       RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato"
  4) Restart Apache.
  5) Refresh. The title should now read "Potato, Potato online catalog".

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Nick Clemens 2016-10-13 08:39:03 UTC
Created attachment 56310 [details] [review]
Bug 16520: Initialize the sysprefs Koha::Caches on demand

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Nick Clemens 2016-10-13 08:39:16 UTC
Created attachment 56311 [details] [review]
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files

This was not added to the old etc/koha-httpd.conf because that file has
no infrastructure for Plack anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Nick Clemens 2016-10-13 08:39:23 UTC
Created attachment 56312 [details] [review]
Bug 16520 (Follow up) Rebase fixes
Comment 24 Kyle M Hall 2016-10-13 13:24:01 UTC
Created attachment 56397 [details] [review]
Bug 16520: Allow per-VirtualHost environment variables with Plack

This allows OVERRIDE_SYSPREF_* and others to work properly.

Test plan:

  1) Add the following line to your plack.psgi (near the bottom, just
     above "mount ..."):
       enable "+Koha::Middleware::Plack";
  2) Load the OPAC advanced search page (under Plack). The title should
    read "Koha online catalog" (or whatever your LibraryName syspref
    contains).
  3) Add the following to your Apache configuration:
       RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato"
  4) Restart Apache.
  5) Refresh. The title should now read "Potato, Potato online catalog".

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Brendan Gallagher 2016-10-13 15:00:51 UTC
Pushed to Master - Should be in the November 2016 release.  Thanks!
Comment 26 Kyle M Hall 2016-10-13 15:08:55 UTC
Created attachment 56410 [details] [review]
Bug 16520: Allow per-VirtualHost environment variables with Plack

This allows OVERRIDE_SYSPREF_* and others to work properly.

Test plan:

  1) Add the following line to your plack.psgi (near the bottom, just
     above "mount ..."):
       enable "+Koha::Middleware::Plack";
  2) Load the OPAC advanced search page (under Plack). The title should
    read "Koha online catalog" (or whatever your LibraryName syspref
    contains).
  3) Add the following to your Apache configuration:
       RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_LibraryName Potato\, Potato"
  4) Restart Apache.
  5) Refresh. The title should now read "Potato, Potato online catalog".

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2016-10-13 15:09:06 UTC
Created attachment 56411 [details] [review]
Bug 16520: Initialize the sysprefs Koha::Caches on demand

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2016-10-13 15:09:11 UTC
Created attachment 56412 [details] [review]
Bug 16520: Add new Plack-SetEnv syntax to Apache configuration files

This was not added to the old etc/koha-httpd.conf because that file has
no infrastructure for Plack anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2016-10-13 15:09:16 UTC
Created attachment 56413 [details] [review]
Bug 16520 (Follow up) Rebase fixes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Jonathan Druart 2016-10-14 07:38:48 UTC
I would have liked to take a look at this one before pushing it. Especially because it modifies the syspref cache code.
Comment 31 Jonathan Druart 2016-11-09 10:04:02 UTC
(In reply to Jonathan Druart from comment #30)
> I would have liked to take a look at this one before pushing it. Especially
> because it modifies the syspref cache code.

Would have been good to launch the tests:
prove t/db_dependent/Context.t
t/db_dependent/Context.t .. 1/? Can't call method "set_in_cache" on an undefined value at /home/vagrant/kohaclone/C4/Context.pm line 533.

Caused by commit 174769e382dfd5205595a5b9ae7b58c8ebf5d850
    Bug 16520 (Follow up) Rebase fixes


Please fix it ASAP
Comment 32 Nick Clemens 2016-11-09 11:14:17 UTC
Created attachment 57372 [details] [review]
Bug 16520 - (Follow up) Restore retrieval of syspref_cache
Comment 33 Jonathan Druart 2016-11-09 11:27:55 UTC
Created attachment 57383 [details] [review]
Bug 16520 - (Follow up) Restore retrieval of syspref_cache

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 34 Kyle M Hall 2016-11-15 14:09:31 UTC
Followup pushed to master thanks Nick!
Comment 35 Magnus Enger 2018-02-09 08:53:29 UTC
Reading this bug, I thought I had to do thos step in order to be able to set per-OPAC variables in my Apache config: 

Add the following line to your plack.psgi (near the bottom, just above "mount ..."): enable "+Koha::Middleware::Plack";

But there is no such thing as Koha::Middleware::Plack, so adding the line made this error show up in the Plack error log: 

Error while loading /etc/koha/sites/x/plack.psgi: Can't locate Koha/Middleware/Plack.pm in @INC (you may need to install the Koha::Middleware::Plack module) (@INC contains: /usr/share/koha/lib /usr/share/koha/installer /usr/share/koha/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl .) at /usr/share/perl5/Plack/Util.pm line 23, <DATA> line 309.

Turns out Koha::Middleware::Plack is not needed, all you need is already in the out-of-the-box .psgi file. Not sure why. Maybe Koha::Middleware::Plack was  just a typo for Koha::Middleware::SetEnv all along? 

http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=1e6df24d6cbf44c865805712e6a15ecc601273aa