245877 - Add an installation test suite

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P2)

Description

[Joel Peshkin]

From an IRC conversation with glob....

As the barriers to Bugzilla prerequisites are reduced, the team is being hit
with larger numbers of basic support questions.  Creating some tests to
troubleshoot installation issues may be a big help.

A test suite could be written in a reasonably portable fashion, particularly if
it is willing to use LWP, wget, or other alternatives interchangably.


APACHE CONFIG PROBLEMS....

The test suite could start by prompting the user for URLBASE and then fetching
URLBASE/ant.jpg and 127.0.0.1/URLBASE_without_hostname/test.cgi

test.cgi could dump the working directory and userid/groupid(s) used by the
webserver.  My inclination is to make test.cgi respond only to requests from
127.0.0.1 to avoid leaking valuable config information.  An alternative would be
to use the data/ directory to store a session-secret shared by test.cgi and the
test suite so that test.cgi can know that the request is not a remote attacker
on a fishing expedition.

If the cgis do not work, the script can do a "ps -oargs -e | grep httpd" to find
the webserver.  If it is not running, it can advise.  If it is running, it can
check to make sure that the user is using the same httpd.conf that they think
they are using.

Once httpd.conf is located, it might attempt to check the config for the
standard Alias, Addhandler, and Directory sections, but it is probably possible
only to request tha the user check those.  It could locate the User/Group lines
and check those against localconfig and against the parent directory permissions.

In the process of checking the webserver, it can also check to confirm that no
file with "localconfig" in its name is acessible ecxept for localconfig.js

Comment 1

[:glob ✱]

there's some checks that can't be performed from a cgi .. like checking apache :)

perhaps these tests could be integrated with checksetup.pl, which does the
initial apache sanity checks, then calls test.cgi and parses its output.

Comment 2

[Joel Peshkin]

My vision of this was to add a cgi only for the command-line script to use as a
vehicle to confirm some properties of the webserver.

The command-line utility is probably a distinct entity from checksetup.pl like
"verify.pl" or something.

I'm open to suggestion on the name of the utility.  Granted, if we didn't
already call the install/update tool "checksetup," it would be a better name for
this.

Comment 3

[Joel Peshkin]

Attachment: First draft

Comment 4

[Joel Peshkin]

The patch adds a new cgi, "testagent.cgi" that will only divulge information if
it is passed a CGI parameter matching the value it finds in data/secret. If the
secret does match, testagent can be assured that it was invoked by servertest.pl
running in the same bugzilla installation directory.  It then lists the groups
of which the webserver is a memeber.

servertest.pl is invoked with urlbase as an argument. [question - should it use
the urlbase in data/params??]

servertest.pl first ensures that it can fetch ant.jpg, then proceeds to invoke
testagent.cgi  it currently troubleshoots....
1) Webserver not serving even static content
2) Webserver serving a differrent bugzilla than the one we want
3) AddHandler messed up
4) webservergroup incorrect

todo....(or think about doing)
avoid hardcoding name of data directory
fall back to wget if LWP is missing
locate running httpd's httpd.conf file and run some checks on it
make sure .htaccess files are working

Comment 5

[Joel Peshkin]

Attachment: Servertest patch

Comment 6

[Joel Peshkin]

Comment on attachment 150652 [details] [diff] [review]
Servertest patch

I'd like to have someone try this on win32.  We may have to add some checks for
Win32 and bypass some of the tests.

I am targetting this for 2.18 since it is cloe to incapable of causing a
problem and I suspect that it will be critical in easing the support load as
newbies use 2.18

Comment 7

[Vlad Dascalu]

Comment on attachment 150652 [details] [diff] [review]
Servertest patch

I really like the idea!

+	 print "TEST-FAILED Webserver is not executing CGI files\n";

+    print "TEST-FAILED webserver is not executing cgi files\n"; 

Those 2 lines above are conflicting themselves (cgi versus CGI). I suspect you
would like the CGI version in both places since it's an abbreviation.

+	 print "Check the AddHandler statment in your httpd.conf file\n";

statment --> statement

I'd prefer to see a period at the end of the sentences; it keeps Bugzilla
professional. You have a period in some cases and you don't in others. This is
a nit.

Also, what happens if the specified URL as arg[0] has an ending slash? I know
"bugzilla//ant.jpg" works on most servers but I'd prefer to trim the trailing
slash if one exists.

+	 print "Check the AddHandler statment in your httpd.conf file\n";

IIS doesn't have httpd.conf; probably it should be taken in consideration at
some point. This one is more like a brainstorm/look-did-we-consider-this rather
then a reason for review-.

Comment 8

[Joel Peshkin]

Attachment: Patch - verifies webservergroup errors properly

It turns out that we can't even get testagent.cgi to run if webservergroup is
wrong.	This uses a form of ps that works on Linux and Solaris to attempt to
determine the group that httpd is using.  If that step fails, it continues
without that check.

Comment 9

[Joel Peshkin]

Attachment: oops - fix spelling too

Comment 10

[Joel Peshkin]

Attachment: Patch that should be OK with Win32 also

Thanks Glob

Comment 11

[Joel Peshkin]

Attachment: Covers OSX/BSD as well as Linux/Solaris/Win32

Comment 12

[Vlad Dascalu]

Comment on attachment 150696 [details] [diff] [review]
Patch that should be OK with Win32 also

Clearing review request on obsolete patch.

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 150717 [details] [diff] [review]
Covers OSX/BSD as well as Linux/Solaris/Win32

Excellent idea :-) But the feedback needs to be improved...

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.282
>diff -u -r1.282 checksetup.pl
>--- checksetup.pl	1 Jun 2004 20:29:04 -0000	1.282
>+++ checksetup.pl	14 Jun 2004 14:48:51 -0000
>@@ -1260,7 +1260,7 @@
> 
> # These are the files which need to be marked executable
> my @executable_files = ('whineatnews.pl', 'collectstats.pl',
>-   'checksetup.pl', 'importxml.pl', 'runtests.sh');
>+   'checksetup.pl', 'importxml.pl', 'runtests.sh', 'servertest.pl');

Other files of this sort are verb-noun rather than noun-verb; so that would
make it "testserver.pl".

>+# servertest.pl is involked with the baseurl of the bugzilla installation

Nit: Bugzilla.

>+# as its only argument.  It attempts to troubleshoot as many installation
>+# issues as possible.
>+# Currently, it attempts to ...

This list is dangerous, because it will inevitably get out of sync with the
script. Better to have block comments at the start of each section describing
the test it does.

>+use strict;
>+if ((@ARGV != 1) || ($ARGV[0] !~ /^https?:/))
>+{
>+    print "Usage: $0 url\n";

"Usage: $0 <URL to this Bugzilla installation>
 E.g.:	$0 http://www.mycompany.com/bugzilla/"

>+my @pscmds = ('ps -eo comm,gid', 'ps -acxo command,gid');
>+
>+# Try to determine if webservergroup is correct

More specific? "Try to determine if the administrator has set $webservergroup
to be the group that the webserver is actually running under."

>+my $sgid = 0;
>+if ($^O !~ /MSWin32/i) {
>+    foreach my $pscmd (@pscmds) {
>+        open PH, "$pscmd 2>/dev/null |";
>+        while (my $line = <PH>) {
>+            if ($line =~ /^\S*httpd\s+(\d+)$/) {
>+                $sgid = $1 if $1 > $sgid;
>+            }
>+        }
>+        close(PH);
>+    }
>+}
>+
>+use lib ".";
>+require "globals.pl";

Better at the top?

>+if ($sgid > 0) {
>+    if ($::webservergroup eq "") {
>+        print "WARNING \$webservergroup is set to an empty string.\n";
>+        print "That is a very insecure practice.\n";

In general, warnings should either give the complete problem and solution, or
reference a section number in the manual page. Which you go for is up to you -
the latter has syncing issues, and the former is repetition. Such is life.

>+    } elsif ($webgroupnum == $sgid) {
>+        print "TEST-OK Webserver is running under group id in \$webservergroup.\n";

IMO, the script should obey the Rule of Silence - when nothing's wrong, say
nothing.
http://www.faqs.org/docs/artu/ch01s06.html , Rule 11.

>+    } else {
>+        print "TEST-WARNING Webserver is running under group id not matching \$webservergroup.\n";
>+        print "This if the tests below fail, this is probably the problem.\n";

Say how to fix it :-)

>+        print "If you are using virtual hosts, this warning may not apply.\n";

Can we tell if they are by grepping httpd.conf? This last sentence causes
uncertainty.

>+# Check if LWP is available
>+eval "require LWP; require LWP::UserAgent;";
>+my $lwp = $@ ? 0 : 1;
>+if (!$lwp) {
>+    print "Servertest requires the LWP module to be installed.\n";
>+    print "Install LWP and try again.\n";

How? :-)

Can we fall back on wget, anyway?

>+    print "TEST-FAILED ant failed\n";
>+    print "Your webserver could not fetch $url.\n";
>+    print "Check your webserver configuration and try again.\n";

Again, not shockingly helpful... Can we do better?

>+if ($res->is_success) {
>+    my $response = $res->content;
>+    if ($response =~ /^000:/) {

Won't this test pass even if the webserver is writing out CGIs rather than
executing them?

Why "000", just out of interest?

>+        print "TEST-FAILED Webserver is fetching rather than executing CGI files.\n";
>+        print "Check the AddHandler statement in your httpd.conf file.\n";

Can we grep to see if it's there?

>+if (!$::localconfig) {
>+    print '$localconfig is not defined, servertest cannot continue.\n';

Even I don't understand this error...

Lots of comments, but let me reiterate - this is a fab idea :-) Do you need to
make it a bit more modular (at least by using functions) so it's a bit easier
to add more tests without ending up with a big ball of mud like checksetup.pl?

Gerv

Comment 14

[:glob ✱]

> Can we tell if they are by grepping httpd.conf?

the trick is to locate httpd.conf.

> Can we fall back on wget, anyway?

i've slapped together a version of servertest that falls back on wget (on
non-windows), and then on raw Socket stuff.  joel's playing with it.

>+    if ($response =~ /^000:/) {
> Won't this test pass even if the webserver is writing
> out CGIs rather than executing them?

no, $response will start with #! not 000

Comment 15

[Joel Peshkin]

Attachment: Revised to work even of LWP is missing

This covers most of Gerv's comments (I don't want to stay silent on pass) and
handles http: even when LWP is not present (thanks glob)

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 150772 [details] [diff] [review]
Revised to work even of LWP is missing

>+        print "TEST-WARNING Webserver is running under group id not matching \$webservergroup.\n";
>+        print "This if the tests below fail, this is probably the problem.\n";
>+        print "If you are using virtual hosts or suexec, this warning may not apply.\n";

I still think that for a diagnostic tool, we need to do much better in telling
people how to fix the problems they encounter - probably by referring them to
the manual. 

How about factoring out the error text into a format (i.e. $bad_gid = <<END;
... END) at the bottom, so it can be extended and edited without messing with
\ns and quotes and escaping? Then, we could explain in more detail more easily.

>+    }
>+} else {
>+   print "TEST-???? Could not identify group webserver is using.\n";
>+}
>+
>+
>+# Try to fetch a static file (ant.jpg)
>+$ARGV[0] =~ s/\/$//;
>+my $url = $ARGV[0] . "/ant.jpg";
>+if (fetch($url)) {
>+    print "TEST-OK got ant picture.\n";

Nit: be consistent about whether your output sentences start with a capital or
not. I suggest one of the following styles:

TEST-OK - got ant picture
or
TEST-OK Got ant picture

>+# Try to execute a cgi script
>+my $response = fetch($ARGV[0] . "/testagent.cgi");
>+if ($response =~ /^000:/) {
>+    print "TEST-OK Webserver is executing CGIs.\n";
>+} elsif ($response =~ /^#!/) {
>+    print "TEST-FAILED Webserver is fetching rather than executing CGI files.\n";
>+    print "Check the AddHandler statement in your httpd.conf file.\n";

So is the point that we can't reliably locate httpd.conf to check this?

>+sub fetch {
>+    my $url = shift;
>+    if ($lwp) {
>+        my $req = HTTP::Request->new(GET => $url);
>+        my $ua = LWP::UserAgent->new;
>+        my $res = $ua->request($req);
>+        return($res->is_success ? $res->content : undef);

Nit: mid-function returns are a bit nasty. Can you factor out fetch into two
subs, lwp_fetch and socket_fetch? 

>+++ testagent.cgi	2004-06-14 07:44:23.000000000 -0700
>+# This script is used by servertest.pl to confirm that cgi scripts

Nit: testserver.pl

>+# are being run instead of shown without relying on database access
>+# or correct params

This sentence could do with some punctuation; I only parsed it on the third
attempt :-)

>+
>+use strict;
>+print "content-type:text/plain\n\n";
>+print "000: OK\n";
>+exit;

Any reason not to just print "OK"? The "000" thing is a bit confusing - later
maintainers are going to spend time trying to work out what it means, when the
answer is "nothing".

Gerv

Comment 17

[Joel Peshkin]

Ideally, we would like to locate and check the httpd.conf file.  However, it is
easy to have more than one httpd somewhere on the system and there is no
consistent way to locate the httpd that is running and ask it what conf file it
is using.  Futhermore, the conf file might not be readable.  That is why I check
for the effective GID of the running httpd processes.

I'll move the message printing to some subs.  If you care to provide wording,
that would be great.

Comment 18

[Joel Peshkin]

Attachment: Cleaned up

Comment 19

[:glob ✱]

on windows, the following error will always be displayed:

  TEST-???? Could not identify group webserver is using.

as the webserver group isn't used on windows, not being able to identify it
shouldn't be treated as an error.

Comment 20

[Joel Peshkin]

Attachment: Supress irrelevent complaint on Win32

Comment 21

[Gervase Markham [:gerv]]

Wording for each error could just be something like:

TEST-ERROR: Your frobnicator is foozled. Bugzilla won't work correctly unless
you fix this. See section X.X of the Bugzilla Guide/FAQ for more details and
information on how to resolve the problem.

Gerv

Comment 22

[Gervase Markham [:gerv]]

joel, glob: are we trying to make this happen for 2.18? As you say, it would be
useful - and it's standalone, so fairly low risk.

Gerv

Comment 23

[:glob ✱]

if i get the time, i'd like to change this to use Test::Harness and add a few
more tests :

  - ensure cgi's have write access to data/
  - ensure cgi's have write access to the temp directories 
      File::Temp, File::Spec::tmpdir, and CGI.pm's temp dir
  - warn if maintainer and urlbase parameters haven't been changed from default
  - warn if specified url doesn't match urlbase
  - test email sending

hopefully that'll be this weekend.

Comment 24

[Joel Peshkin]

Adding the test sounds good.  Just make sure that you order them so that a
permission problem that keeps the web interface from working does not cause the
test to stop befoe it tells the user about the problem.

Comment 25

[Gervase Markham [:gerv]]

Comment on attachment 151427 [details] [diff] [review]
Supress irrelevent complaint on Win32

r=gerv. I have some issues with the messages, but it seems far more sensible to
check this in and fix them later.

Gerv

Comment 26

[Joel Peshkin]

checked in on both branches

Comment 27

[Frédéric Buclin]

Attachment: documentation patch, v1

Comment 28

[Frédéric Buclin]

tip:

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.133; previous revision: 1.132
done
Checking in docs/xml/security.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/security.xml,v  <--  security.xml
new revision: 1.16; previous revision: 1.15
done


2.22.1:

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.107.2.14; previous revision: 1.107.2.13
done
Checking in docs/xml/security.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/security.xml,v  <--  security.xml
new revision: 1.8.2.6; previous revision: 1.8.2.5
done


2.20.3:

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.98.2.20; previous revision: 1.98.2.19
done
Checking in docs/xml/security.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/security.xml,v  <--  security.xml
new revision: 1.6.2.6; previous revision: 1.6.2.5
done


2.18.6:

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.72.2.33; previous revision: 1.72.2.32
done
Checking in docs/xml/security.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/security.xml,v  <--  security.xml
new revision: 1.2.2.9; previous revision: 1.2.2.8
done