RunCommandLogs: identify and access via uuid
Using a sha1 of the command combined with the build ID is not a particularly good or unique identifier: * A build could fail, be restarted, and then succeed -- assuming no configuration changes, the sha1 hash of the command as well as the build ID will be the same. This would lead to an overwritten log file. * Allowing user input to influence filenames is not the best of ideas.
This commit is contained in:
		| @@ -137,10 +137,10 @@ sub view_log : Chained('buildChain') PathPart('log') { | ||||
| } | ||||
|  | ||||
|  | ||||
| sub view_runcommand_log : Chained('buildChain') PathPart('runcommand-log') { | ||||
|     my ($self, $c, $sha) = @_; | ||||
| sub view_runcommandlog : Chained('buildChain') PathPart('runcommandlog') { | ||||
|     my ($self, $c, $uuid) = @_; | ||||
|  | ||||
|     $c->stash->{log_uri} = $c->uri_for($c->controller('Root')->action_for("runcommandlog"), constructRunCommandLogFilename($sha, $c->stash->{build}->id)); | ||||
|     $c->stash->{log_uri} = $c->uri_for($c->controller('Root')->action_for("runcommandlog"), $uuid); | ||||
|     $c->stash->{template} = 'runcommand-log.tt'; | ||||
| } | ||||
|  | ||||
|   | ||||
| @@ -531,13 +531,13 @@ sub log :Local :Args(1) { | ||||
| } | ||||
|  | ||||
| sub runcommandlog :Local :Args(1) { | ||||
|     my ($self, $c, $filename) = @_; | ||||
|     my ($self, $c, $uuid) = @_; | ||||
|  | ||||
|     my $tail = $c->request->params->{"tail"}; | ||||
|  | ||||
|     die if defined $tail && $tail !~ /^[0-9]+$/; | ||||
|  | ||||
|     my $logFile = constructRunCommandLogPath($filename); | ||||
|     my $logFile = constructRunCommandLogPath($uuid); | ||||
|     if (-f $logFile) { | ||||
|         serveLogFile($c, $logFile, $tail); | ||||
|         return; | ||||
|   | ||||
| @@ -19,7 +19,6 @@ our @EXPORT = qw( | ||||
|     cancelBuilds | ||||
|     captureStdoutStderr | ||||
|     captureStdoutStderrWithStdin | ||||
|     constructRunCommandLogFilename | ||||
|     constructRunCommandLogPath | ||||
|     findLog | ||||
|     gcRootFor | ||||
| @@ -591,13 +590,6 @@ sub isLocalStore { | ||||
| } | ||||
|  | ||||
|  | ||||
| sub constructRunCommandLogFilename { | ||||
|     my ($sha, $build_id) = @_; | ||||
|     my $filename = "$sha-$build_id"; | ||||
|     return $filename; | ||||
| } | ||||
|  | ||||
|  | ||||
| sub constructRunCommandLogPath { | ||||
|     my ($filename) = @_; | ||||
|  | ||||
|   | ||||
| @@ -163,8 +163,7 @@ sub buildFinished { | ||||
|  | ||||
|         $runlog->started(); | ||||
|  | ||||
|         my $filename = Hydra::Helper::Nix::constructRunCommandLogFilename(sha1_hex($command), $build->get_column('id')); | ||||
|         my $logPath = Hydra::Helper::Nix::constructRunCommandLogPath($filename); | ||||
|         my $logPath = Hydra::Helper::Nix::constructRunCommandLogPath($runlog->uuid); | ||||
|         my $dir = dirname($logPath); | ||||
|         my $oldUmask = umask(); | ||||
|  | ||||
|   | ||||
| @@ -175,8 +175,6 @@ __PACKAGE__->belongs_to( | ||||
|  | ||||
| use POSIX qw(WEXITSTATUS WIFEXITED WIFSIGNALED WTERMSIG); | ||||
| use UUID4::Tiny qw(create_uuid_string); | ||||
| use Digest::SHA1 qw(sha1_hex); | ||||
| use Hydra::Model::DB; | ||||
|  | ||||
|  | ||||
| =head2 new | ||||
| @@ -366,29 +364,3 @@ sub did_fail_with_exec_error { | ||||
| } | ||||
|  | ||||
| 1; | ||||
|  | ||||
| =head2 log_relative_url | ||||
|  | ||||
| Returns the URL to the log file relative to the build it belongs to. | ||||
|  | ||||
| Return: | ||||
|  | ||||
| * The relative URL if a log file exists | ||||
| * An empty string otherwise | ||||
| =cut | ||||
| sub log_relative_url() { | ||||
|     my ($self) = @_; | ||||
|  | ||||
|     # Do not return a URL when there is no build yet | ||||
|     if (not defined($self->start_time)) { | ||||
|       return ""; | ||||
|     } | ||||
|  | ||||
|     my $sha = sha1_hex($self->command); | ||||
|     # Do not return a URL when there is no log file yet | ||||
|     if (not -f Hydra::Model::DB::getHydraPath . "/runcommand-logs/" . substr($sha, 0, 2) . "/$sha-" . $self->build_id) { | ||||
|         return ""; | ||||
|     } | ||||
|  | ||||
|     return "runcommand-log/$sha"; | ||||
| } | ||||
|   | ||||
| @@ -526,14 +526,14 @@ END; | ||||
|               <div>Started at [% INCLUDE renderDateTime timestamp = runcommandlog.start_time; %]</div> | ||||
|               [% IF runcommandlog.end_time != undef %] | ||||
|                 <div>Ran for [% INCLUDE renderDuration duration = runcommandlog.end_time - runcommandlog.start_time %] | ||||
|                   [% IF runcommandlog.log_relative_url() %] | ||||
|                      — <a href="[% c.uri_for('/build', build.id, runcommandlog.log_relative_url()) %]">Logs</a> | ||||
|                   [% IF runcommandlog.uuid != undef %] | ||||
|                      — <a href="[% c.uri_for('/build', build.id, 'runcommandlog', runcommandlog.uuid) %]">Logs</a> | ||||
|                   [% END %] | ||||
|                 </div> | ||||
|               [% ELSE %] | ||||
|                 <div>Running for [% INCLUDE renderDuration duration = curTime - runcommandlog.start_time %] | ||||
|                   [% IF runcommandlog.log_relative_url() %] | ||||
|                      — <a href="[% c.uri_for('/build', build.id, runcommandlog.log_relative_url()) %]">Logs</a> | ||||
|                   [% IF runcommandlog.uuid != undef %] | ||||
|                      — <a href="[% c.uri_for('/build', build.id, 'runcommandlog', runcommandlog.uuid) %]">Logs</a> | ||||
|                   [% END %] | ||||
|                 </div> | ||||
|               [% END %] | ||||
|   | ||||
		Reference in New Issue
	
	Block a user