Closed Bug 370241 Opened 13 years ago Closed 13 years ago

Buildbot patch uploader

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: bhearsum)

References

Details

Attachments

(3 files, 12 obsolete files)

2.64 KB, text/plain
rhelmer
: review+
Details
5.35 KB, text/plain
preed
: review+
Details
8.90 KB, patch
Details | Diff | Splinter Review
The attached file adds support for patches to be uploaded to Buildbot through a web interface.
Attachment #254903 - Flags: review?(rhelmer)
Assignee: build → bhearsum
Attachment #254903 - Attachment is patch: false
Attachment #254903 - Attachment mime type: text/plain → application/x-tar
Comment on attachment 254903 [details]
web upload cgi/html + custom patch step

looks good! we may want to simplify it as per bug 370224 (separate the upload from the "buildbot sendchange" run). However, this is a good starting point and we know it works.
Attachment #254903 - Flags: review?(rhelmer) → review+
Attached file buildbot patch uploader v2 (obsolete) —
Here's an updated version of my buildbot patch uploader. I tried doing the
file transfer to the slave and the patching in one buildstep but I wasn't able
to get that working. So I've created a MozillaPatchDownload step that tweaks
the FileDownload slightly.

MozillaPatchDownload takes an argument of 'patchDir'. This must be the
directory where the CGI script drops the patches off. The patches are sent to
the BuildSlave's builder root (ie. /home/buildslave/mybuildslave/test-project)
 and read it from there by the MozillaCustomPatch'er.

The CGI script was modified slightly to only send the filename of the patch
rather than the full path.
Attachment #254903 - Attachment is obsolete: true
Attachment #256043 - Flags: review?(rhelmer)
Comment on attachment 256043 [details]
buildbot patch uploader v2

Sorry, thought I had gone back and reverted my previous review.. 

We should make the CGI safer. It should only allow A-z, 0-9, - and _. Right now it disallows slashes and dots, but malicious users could still do backticks, pipes, and lots of other shell tricks.

Also, there should be a filesize limit, so a user can't upload a giant file and DOS the service.

For the production version of this we'd like to separate the machine that receives the patches from the actual buildbot master (which is more heavily firewalled).

Can you please make it so the CGI just drops the file somewhere?

You can assume that the CGI will be dropping the uploaded file somewhere web-accessible to the buildbot master; the location should be configurable.
Attachment #256043 - Flags: review?(rhelmer) → review-
Attached file Patch uploader CGI (obsolete) —
Patch uploader CGI script -- all html is contained in the cgi now.
A description field was added. Both it and the filename are checked to make sure they don't contain any backticks or other potentially nasty things. The name field was removed and the REMOTE_USER is used in it's place.
Attachment #256043 - Attachment is obsolete: true
Attached file Patch Downloader shell script (obsolete) —
This shell script downloads the patch files uploaded with uploadpatch.cgi and generates a 'sendchange' command for each of them.
Attached file MozillaCustomPatch buildstep (obsolete) —
Updated version of the MozillaCustomPatch buildstep.
Attached file MozillaPatchDownload buildstep. (obsolete) —
Updated version of the MozillaPatchDownload buildstep.
Attachment #256401 - Flags: review?(rhelmer)
Attachment #256402 - Flags: review?(rhelmer)
Attachment #256403 - Flags: review?(rhelmer)
Attachment #256404 - Flags: review?(rhelmer)
Okay. I've uploaded the updated version of all the buildbot patch uploader files. More in-depth descriptions can be found in the files themselves.

I've tested the entire setup with downloadpatches.sh in a cronjob and it's working great for me.

Here is the master.cfg I've been using with buildbot: http://foxybanana.com/~bhearsum/vault/master.cfg

Note that the for the PatchDownload step the patchDir is a directory on the buildmaster relative to the root of the project (ie. /home/master/myproject), and the patchDir on the CustomPath is relative to the buildslave's builder dir (ie. /home/slave/myproject/builderfoo). This is also documented in the .py files.
Comment on attachment 256401 [details]
Patch uploader CGI

Looks good, only nitpick is that I'd use HTML4 not XHTML :)

preed, what do you think? This addresses the security concerns on the filename (and imposes a size limit), and drops the file to the local disk with a .info file instead of calling "buildbot sendchange" directly. Also, uses REMOTE_USER (to get the LDAP username) instead of  depending on the user to input it.
Attachment #256401 - Flags: review?(rhelmer)
Attachment #256401 - Flags: review?(preed)
Attachment #256401 - Flags: review+
Comment on attachment 256401 [details]
Patch uploader CGI

><form action="/cgi-bin/uploadpatch.cgi" method="post"
>      enctype="multipart/form-data">

I think I would rather this be relative. Now that you've combined the HTML and the CGI, you could just use |action="uploadpatch.cgi"|.

>    # only allow alphanumeric, hyphens, and single dots
>    if ($patchfile !~ /^([\w-]|\.[\w-])+$/) {

I just think this is a bit harsh on the user and could be improved, but it'll work for now.
(In reply to comment #10)
> (From update of attachment 256401 [details])
> ><form action="/cgi-bin/uploadpatch.cgi" method="post"
> >      enctype="multipart/form-data">
> 
> I think I would rather this be relative. Now that you've combined the HTML and
> the CGI, you could just use |action="uploadpatch.cgi"|.
> 

I moved it over to the CGI because it's much much easier to display error messages this way.
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 256401 [details] [details])
> > ><form action="/cgi-bin/uploadpatch.cgi" method="post"
> > >      enctype="multipart/form-data">
> > 
> > I think I would rather this be relative. Now that you've combined the HTML and
> > the CGI, you could just use |action="uploadpatch.cgi"|.
> > 
> 
> I moved it over to the CGI because it's much much easier to display error
> messages this way.
> 

Oops. Please ignore that comment. I misread.
Comment on attachment 256401 [details]
Patch uploader CGI

I've been playing with this a bit; needs better error handling for the upload; I had a typo in the PATCH_DIR but didn't get any feedback (just "patch uploaded successfully").

Would be good to have more info on the success page, like a link to the patch maybe? Not sure.

Clarification on the XHTML nitpick from before - you're setting the 
Content-Type to "text/html" and then sending XHTML (which should be XML). It's better to just use HTML4 and send text/html IMHO.
Comment on attachment 256401 [details]
Patch uploader CGI

bhearsum: Can you want to just point me at the next version that does the error checking?

I'll cancel review for now.

I did skim, and it looks much better than the first version; good work!
Attachment #256401 - Flags: review?(preed)
Comment on attachment 256402 [details]
Patch Downloader shell script

This is kind of complicated as shell; I'd probably go for python or perl here.

># only look at the info files; it's not necessary to look at both
>files=$(ls -l $PATCHDIR | grep "\.info$" | awk '{print $8}')


This only works for me as $9, otherwise I get part of the date.
Attachment #256402 - Flags: review?(rhelmer) → review-
Comment on attachment 256404 [details]
MozillaPatchDownload buildstep.

>        self.mastersrc = "%s/%s" % (self.patchDir, file)
>        self.slavedest = self.mastersrc

Need to use workdir here, because we don't want the patch to have to be in the same place on both the master and client. Ben, I'll attach a patch because I made a minor modification since we last spoke about it.
Attachment #256404 - Attachment is obsolete: true
Attachment #256404 - Flags: review?(rhelmer)
Hey Ben,

This is the same as the patch you worked up on the test box, but we don't need to pass workdir on line 40:

self.slavedest = "%s" % (file)

The parent class FileDownload will use self.workdir so it doesn't need to be passed there (or else you'll have the workdir specified twice!).
Attachment #257110 - Flags: review?(bhearsum)
Attachment #257110 - Flags: review?(bhearsum) → review+
Attachment #257110 - Flags: review+ → review?(bhearsum)
Attachment #257110 - Flags: review?(bhearsum) → review+
Sorry for all the edits, still learning about Bugzilla attachments/reviews
Added checks for zero-length file uploads and if the script can open the destination file for writing.

The success page now includes a link to the buildbot waterfall page.
Attachment #256401 - Attachment is obsolete: true
Attachment #257155 - Flags: review?(preed)
The only thing logged from this script now is an error message when no files are available

And I fixed the ls bug that you mentioned, Rob.

I tested this along with the latest versions of the Buildsteps on test1-linux-buildbot.
Attachment #256402 - Attachment is obsolete: true
Attachment #257160 - Flags: review?(rhelmer)
Attached file removed patchDir option (obsolete) —
This is the latest version that was on test1-linux-buildbot. I re-tested it with the latest version of downloadpatches.sh and it worked without issue.
Attachment #256403 - Attachment is obsolete: true
Attachment #257161 - Flags: review?(rhelmer)
Attachment #256403 - Flags: review?(rhelmer)
Attachment #257161 - Flags: review?(rhelmer) → review+
Attachment #257160 - Flags: review?(rhelmer) → review+
Comment on attachment 257155 [details]
added error checking, updated success page


In general, it may be useful to look over these:

https://intranet.mozilla.org/Build:PerlStyleGuide

(Most notably for captilization/indentation and function call semantics.)

># the size limit for the file, in bytes
>my $SIZELIMIT = 10*1024*1024;

Might add to the comment that this is 10 megs.

>sub write_success_page {
>    print "Content-type: text/html\n\n";
>    print <<ENDOFHTML;

I tend to write these as:

print <<__END_OF_HTML__; so they're easier to pick out from the code.

>sub process {
>    my $time = time();
>    # get the parameters
>    my $name = "";
>    $name = $ENV{'REMOTE_USER'};
>    if ($name eq "") {
>        $name = "Noname";
>    }

Isn't it a requirement that people log in? Or are we just doing that for the trial? If it's a requirement, we should error out if $name eq "".

But I can't remember which we decided.

>    # only allow alphanumeric, hyphens, and single dots
>    if ($patchfile !~ /^([\w-]|\.[\w-])+$/) {
>        # ends the script

This is in the filename, right? Why do we allow single dots? So people can name them patch.foo.bar?

>    # write the patch
>    my $filename = "$PATCH_DIR/$patchfile";
>    open PATCH, ">$filename" or write_page($description,
>      "Server error - Could not open file for writing.") and return;
>    binmode PATCH;

We probably don't want to blindly overwrite files; there's a race-condition if you use -e; hrm... something to think about.

For the architectural stuff, I'm mostly ok with the patch as is; I'd like to see some of the formatting stuff fixed, though.

In general, r=preed. Good job on seeing this through, Ben!
Attachment #257155 - Flags: review?(preed) → review+
(In reply to comment #22)
> (From update of attachment 257155 [details])
> 
> In general, it may be useful to look over these:
> 
> https://intranet.mozilla.org/Build:PerlStyleGuide
> 

Will do. Do I need to file another bug to get access to the intranet? My username and password aren't being accepted for that page.

> Isn't it a requirement that people log in? Or are we just doing that for the
> trial? If it's a requirement, we should error out if $name eq "".
> 

I assumed that unless Apache accepts the login they will never be able to get to this point.

> 
> >    # only allow alphanumeric, hyphens, and single dots
> >    if ($patchfile !~ /^([\w-]|\.[\w-])+$/) {
> >        # ends the script
> 
> This is in the filename, right? Why do we allow single dots? So people can name
> them patch.foo.bar?
> 

Yep. I can't see any harm in allowing this.

> 
> In general, r=preed. Good job on seeing this through, Ben!
> 

Thanks :)
Priority: -- → P3
This fell off my radar, sorry about that. As I'm waiting on feedback for the VMware team I'm going to do some work on this, this week.
This version removes usage of extended regular expression with sed to make it more compatible. It turns out that BSD and GNU sed use different switches for it.
Attachment #257160 - Attachment is obsolete: true
Attachment #270962 - Flags: review?(rhelmer)
Attached file uploadpatch.cgi update (obsolete) —
I've addressed all of the suggestions in your previous commented except the last two. I commented on the single dots in the comment above.

With regard to file overwriting, I don't think that is something we need to worry about here. All of the files being written out are perpended by the current unix time. The likelihood of a conflict is negligible IMHO.
Attachment #257155 - Attachment is obsolete: true
Attachment #270964 - Flags: review?(preed)
Comment on attachment 270962 [details]
downloadpatches.sh bugfixes

Looks good; I don't have time/space to test now it but based on my previous testing and comments I think this looks right :)
Attachment #270962 - Flags: review?(rhelmer) → review+
Comment on attachment 270964 [details]
uploadpatch.cgi update

Looks good; a couple of nits:

># where the patches will go after being submitted
>my $PATCH_DIR = '/Users/bhearsum/Sites/www/buildbot/patches';

Is this where you're wanting the patches to go in production? :-)

>if (param()) {
>    process();
>} else {
>    WritePage();
>}

I honestly can't remember what the style guide says (in function are camel case or not; I think they *are*); if they're CamelCase, then process -> Process(); if they're not, then WritePage() should be writePage().
Attachment #270964 - Flags: review?(preed) → review+
Attached file uploadpatch.cgi final
The style guide says functions should start uppercase, so I've done that.
Thanks for catching those.
Attachment #270964 - Attachment is obsolete: true
Attachment #272085 - Flags: review?(preed)
Blocks: 387958
Attachment #272085 - Flags: review?(preed) → review+
Can we get these files checked in somewhere?
(In reply to comment #30)
> Can we get these files checked in somewhere?

I'd suggest something like mozilla/webtools/buildbot-try or something like that.

But mozilla/webtools is morgamic's area, so I'd want his suggestions/r+.
Attached file Custom BuildSteps, in one file. (obsolete) —
Moved both of the BuildSteps to a mozbuild.py file.
Attachment #257110 - Attachment is obsolete: true
Attachment #257161 - Attachment is obsolete: true
Attachment #272246 - Flags: review?(rhelmer)
Comment on attachment 272246 [details]
Custom BuildSteps, in one file.

The mozbuild.py/master.cfg for this is in bug 387958
Attachment #272246 - Attachment is obsolete: true
Attachment #272246 - Flags: review?(rhelmer)
(In reply to comment #31)
> But mozilla/webtools is morgamic's area, so I'd want his suggestions/r+.
+r=morgamic!

RCS file: /cvsroot/mozilla/webtools/buildbot-try/downloadpatches.sh,v
done
Checking in downloadpatches.sh;
/cvsroot/mozilla/webtools/buildbot-try/downloadpatches.sh,v  <--  downloadpatches.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/buildbot-try/uploadpatch.cgi,v
done
Checking in uploadpatch.cgi;
/cvsroot/mozilla/webtools/buildbot-try/uploadpatch.cgi,v  <--  uploadpatch.cgi
initial revision: 1.1
done
Resolving.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.