[PATCH consfigurator v2 1/2] GPG: batch mode and return *ERROR-OUTPUT* in conditions

[Russell Sim]

Sean Whitton <spwhitton at spwhitton.name> writes:

> Thanks for this, it will be a nice improvement.
>
> On Wed 21 Sep 2022 at 10:06PM +02, Russell Sim wrote:
>
>> diff --git a/src/data/util.lisp b/src/data/util.lisp
>> index aa0451f..b6c8685 100644
>> --- a/src/data/util.lisp
>> +++ b/src/data/util.lisp
>> @@ -19,6 +19,15 @@
>>  (in-package :consfigurator.data.util)
>>  (named-readtables:in-readtable :consfigurator)
>>
>> +(define-condition gpg-error (subprocess-error)
>> +  ((output :initform nil :initarg :output :reader gpg-error-output))
>> +  (:report (lambda (condition stream)
>> +             (format stream "Subprocess ~@[~S~% ~]~@[with command ~S~% ~]exited with error~@[ code ~D~%~]~@[~% with error output:~% ~S~]"
>> +                     (subprocess-error-process condition)
>> +                     (subprocess-error-command condition)
>> +                     (subprocess-error-code condition)
>> +                     (gpg-error-output condition)))))
>> +
>
> How about using the RUN-FAILED condition, instead of adding a new one?

I think this is a good idea.  I was unaware of that condition, my slime
apropos foo is lacking.

Looking at all the other datasources, they all have RUN-PROGRAM calls
that will also swallow stdout/stderr when they fail.  Should those be
updated, because it's starting to look like we want a utility to wrap
RUN-PROGRAM and create capture stdout/stderr 

> Please reformat to avoid overly long lines here.

Apologies, I did see that requirement in the contributions, but I only
set it up in Emacs at the last second, and it looks like I set it to 80
by accident. 

>>  (defun gpg-file-as-string (location)
>>    "Decrypt the contents of a gpg encrypted file at LOCATION, return as a
>>  string."
>>    (handler-case
>>        (gpg (list "--decrypt" (unix-namestring location)))
>> -    (subprocess-error (error)
>> -      (missing-data-source "While attempt to decrypt ~A, gpg exited with ~A"
>> -			   location (uiop:subprocess-error-code error)))))
>> +    (gpg-error (error)
>> +      (missing-data-source "While attempt to decrypt ~A, gpg exited with ~A~@[~% with gpg error output:~% ~S~]"
>> +			   location (subprocess-error-code error) (gpg-error-output error)))))
>
> Likewise, lines are too long here.  Can you use the same format string
> as RUN-FAILED, too, for consistency?  Like this:
>
>     "~&'gpg' failed, exit code ~A~%~%stderr was:~%~A"

Can do, but this is somewhat addressed by explicitly detecting if any
secret file exists. in the second patch.  I think I'll just submit that
independently as an initial patch, it's small.

>> diff --git a/src/package.lisp b/src/package.lisp
>> index 4b51b70..55b2d1a 100644
>> --- a/src/package.lisp
>> +++ b/src/package.lisp
>> @@ -1022,7 +1022,11 @@
>>                               (#:lxc  #:consfigurator.property.lxc)))
>>
>>    (package :consfigurator.data.util
>> -           (:export #:literal-data-pathname #:gpg-file-as-string #:gpg))
>> +           (:import-from #:uiop
>> +                         #:subprocess-error-process
>> +                         #:subprocess-error-command
>> +                         #:subprocess-error-code)
>> +           (:export #:literal-data-pathname #:gpg-file-as-string #:gpg #:gpg-error))
>>
>>    (package :consfigurator.data.asdf)
>
> Please just import them unconditionally into CONSFIGURATOR, just as is
> already done for SUBPROCESS-ERROR itself.

Will do

>
>> diff --git a/tests/data/pgp.lisp b/tests/data/pgp.lisp
>> index 21ba60c..0eb7749 100644
>> --- a/tests/data/pgp.lisp
>> +++ b/tests/data/pgp.lisp
>> @@ -19,3 +19,12 @@
>>  (deftest data.pgp.3
>>      (get-data-string "host.example.com" "/etc/foo.conf")
>>    "secret file content")
>> +
>> +(deftest data.pgp.4
>> +    (handler-case (data.pgp:get-data "/dev/null" "_secrets" "test")
>> +      (missing-data-source (error)
>> +        (princ-to-string error)))
>> +  "While attempt to decrypt /dev/null, gpg exited with 2
>> + with gpg error output:
>> + \"gpg: decrypt_message failed: Unknown system error
>> +\"")
>
> How about just searching for the substring "decrypt_message failed", to
> make this a bit less fragile?

Yep, or just the text "failed", like you say, we really just want to
ensure that the error was recorded.  This was a bit of a rookie error I
made while learning this RT package, it seems that it can only compare
literal values.  So lots of the newer tests I'm writing use ASSERT
internally and return T when they succeed, and then the final form is T,
it's a bit weird TBH, but it does work.  I can make this better



Regarding the patch as a whole, while reviewing the rest of the packages
in the DATA section, it looks like RUN-PROGRAM is used a lot and in all
cases stderr is discarded.

Would it be reasonable to expand the scope of this work to 

Make a utility called RUN-PROGRAM that internally calls
UIOP:RUN-PROGRAM, but will capture any STDOUT/STDERR if no streams are
passed in.  Handle the SUBPROCESS-ERROR and convert it to a RUN-FAILED
condition for all cases.  There is only one function
TRY-GET-FILE-MIME-TYPE that deals directly with the SUBPROCESS-ERROR
condition so I can update it to handle RUN-FAILED.  There will probably
be a few places then where we need to explicitly pass in *ERROR-OUTPUT*
or *STANDARD-OUTPUT* rather than that being the default output location?
(or perhaps call the UIOP:RUN-PROGRAM directly) Does this make sense?
then we can expose RUN-PROGRAM to other packages to consume, giving them
all the same behaviour.

Sorry this patch keeps changing, I'm trying to avoid refactoring large
amounts of code if it's not needed.  But I have learned a lot since I
started down this path.