Repository: osbuild/osbuild-composer
Pull request #4145 (2024-05-13 16:09:41)

osbuild-worker: do not use `error` in clienterror.Error.Details
This is an alternative/complementary fix for https://github.com/osbuild/osbuild-composer/pull/4137. It is very simple so should be uncontroversial.

It fixes an issue that @schuellerf discovered, i.e. that when an error interface is passed into clienterrors.Error.Details the details get lost because the json.Marshaler will not know how to handler an error interface.

To find the problematic uses of `error` a custom vet checker was build in https://github.com/mvo5/osbuild-cvet. With that the result is:
```
$ go run github.com/mvo5/osbuild-cvet@latest ./...
/home/mvogt/devel/osbuild/osbuild-composer/cmd/osbuild-worker/jobimpl-depsolve.go:93:26: do not pass 'error' to WorkerClientError() details, use error.Error() instead
/home/mvogt/devel/osbuild/osbuild-composer/cmd/osbuild-worker/jobimpl-osbuild.go:404:31: do not pass 'error' to WorkerClientError() details, use error.Error() instead
/home/mvogt/devel/osbuild/osbuild-composer/cmd/osbuild-worker/jobimpl-osbuild.go:519:31: do not pass 'error' to WorkerClientError() details, use error.Error() instead
/home/mvogt/devel/osbuild/osbuild-composer/cmd/osbuild-worker/jobimpl-osbuild.go:556:31: do not pass '[]error' to WorkerClientError() details, use []string instead
```
and once this commit is in no more errors.

Just like PR#4137 this is not perfect because it will not do a recursive check for the passed argument.

If we are happy with this approach we should add something like
```yaml
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index 635b1af0b..88f3d831c 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -146,6 +146,9 @@ jobs:
           version: v1.54.2
           args: --verbose --timeout 5m0s
 
+      - name: Run osbuild-cvet
+        run: go run github.com/mvo5/osbuild-cvet@latest ./...
+
   packit-config-lint:
     name: "📦 Packit config lint"
     runs-on: ubuntu-latest
```
(and move to the vet to the osbuild org)


Patch:
diff --git a/cmd/osbuild-worker-executor/handler_build.go b/cmd/osbuild-worker-executor/handler_build.go
index a790ae523..096876ab5 100644
--- a/cmd/osbuild-worker-executor/handler_build.go
+++ b/cmd/osbuild-worker-executor/handler_build.go
@@ -39,7 +39,7 @@ func (wf *writeFlusher) Write(p []byte) (n int, err error) {
 	return n, err
 }
 
-func runOsbuild(buildDir string, control *controlJSON, output io.Writer) (string, error) {
+func runOsbuild(logger *logrus.Logger, buildDir string, control *controlJSON, output io.Writer) (string, error) {
 	flusher, ok := output.(http.Flusher)
 	if !ok {
 		return "", fmt.Errorf("cannot stream the output")
@@ -92,11 +92,11 @@ func runOsbuild(buildDir string, control *controlJSON, output io.Writer) (string
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		err = fmt.Errorf("cannot tar output directory: %w, output:\n%s", err, out)
-		logrus.Errorf(err.Error())
+		logger.Errorf(err.Error())
 		_, _ = mw.Write([]byte(err.Error()))
 		return "", err
 	}
-	logrus.Infof("tar output:\n%s", out)
+	logger.Infof("tar output:\n%s", out)
 	return outputDir, nil
 }
 
@@ -286,7 +286,7 @@ func handleBuild(logger *logrus.Logger, config *Config) http.Handler {
 
 			// run osbuild and stream the output to the client
 			buildResult := newBuildResult(config)
-			_, err = runOsbuild(buildDir, control, w)
+			_, err = runOsbuild(logger, buildDir, control, w)
 			if werr := buildResult.Mark(err); werr != nil {
 				logger.Errorf("cannot write result file %v", werr)
 			}
diff --git a/cmd/osbuild-worker-executor/main.go b/cmd/osbuild-worker-executor/main.go
index 59edfcbc9..2330ea9c8 100644
--- a/cmd/osbuild-worker-executor/main.go
+++ b/cmd/osbuild-worker-executor/main.go
@@ -16,8 +16,6 @@ import (
 // based on the excellent post
 // https://grafana.com/blog/2024/02/09/how-i-write-http-services-in-go-after-13-years/
 
-var logrusNew = logrus.New
-
 func newServer(logger *logrus.Logger, config *Config) http.Handler {
 	mux := http.NewServeMux()
 	addRoutes(mux, logger, config)
@@ -27,11 +25,10 @@ func newServer(logger *logrus.Logger, config *Config) http.Handler {
 	return handler
 }
 
-func run(ctx context.Context, args []string, getenv func(string) string) error {
+func run(ctx context.Context, args []string, getenv func(string) string, logger *logrus.Logger) error {
 	ctx, cancel := signal.NotifyContext(ctx, os.Interrupt)
 	defer cancel()
 
-	logger := logrusNew()
 	config, err := newConfigFromCmdline(args)
 	if err != nil {
 		return err
@@ -75,8 +72,9 @@ func run(ctx context.Context, args []string, getenv func(string) string) error {
 }
 
 func main() {
+	logger := logrus.New()
 	ctx := context.Background()
-	if err := run(ctx, os.Args, os.Getenv); err != nil {
+	if err := run(ctx, os.Args, os.Getenv, logger); err != nil {
 		fmt.Fprintf(os.Stderr, "%s\n", err)
 		os.Exit(1)
 	}
diff --git a/cmd/osbuild-worker/jobimpl-depsolve.go b/cmd/osbuild-worker/jobimpl-depsolve.go
index f50bd209a..4dbc5495e 100644
--- a/cmd/osbuild-worker/jobimpl-depsolve.go
+++ b/cmd/osbuild-worker/jobimpl-depsolve.go
@@ -90,7 +90,7 @@ func (impl *DepsolveJobImpl) Run(job worker.Job) error {
 					for _, baseurlstr := range repo.BaseURLs {
 						match, err := impl.RepositoryMTLSConfig.CompareBaseURL(baseurlstr)
 						if err != nil {
-							result.JobError = clienterrors.WorkerClientError(clienterrors.ErrorInvalidRepositoryURL, "Repository URL is malformed", err)
+							result.JobError = clienterrors.WorkerClientError(clienterrors.ErrorInvalidRepositoryURL, "Repository URL is malformed", err.Error())
 							return err
 						}
 						if match {
diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go
index 4dde72ce0..6f3930cbb 100644
--- a/cmd/osbuild-worker/jobimpl-osbuild.go
+++ b/cmd/osbuild-worker/jobimpl-osbuild.go
@@ -401,7 +401,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 
 	osbuildVersion, err := osbuild.OSBuildVersion()
 	if err != nil {
-		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "Error getting osbuild binary version", err)
+		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "Error getting osbuild binary version", err.Error())
 		return err
 	}
 	osbuildJobResult.OSBuildVersion = osbuildVersion
@@ -516,7 +516,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 	osbuildJobResult.OSBuildOutput, err = executor.RunOSBuild(jobArgs.Manifest, impl.Store, outputDirectory, exports, exportPaths, nil, extraEnv, true, os.Stderr)
 	// First handle the case when "running" osbuild failed
 	if err != nil {
-		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", err)
+		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", err.Error())
 		return err
 	}
 
@@ -544,13 +544,13 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 
 	// Second handle the case when the build failed, but osbuild finished successfully
 	if !osbuildJobResult.OSBuildOutput.Success {
-		var osbErrors []error
+		var osbErrors []string
 		if osbuildJobResult.OSBuildOutput.Error != nil {
-			osbErrors = append(osbErrors, fmt.Errorf("osbuild error: %s", string(osbuildJobResult.OSBuildOutput.Error)))
+			osbErrors = append(osbErrors, fmt.Sprintf("osbuild error: %s", string(osbuildJobResult.OSBuildOutput.Error)))
 		}
 		if osbuildJobResult.OSBuildOutput.Errors != nil {
 			for _, err := range osbuildJobResult.OSBuildOutput.Errors {
-				osbErrors = append(osbErrors, fmt.Errorf("manifest validation error: %v", err))
+				osbErrors = append(osbErrors, fmt.Sprintf("manifest validation error: %v", err))
 			}
 		}
 		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", osbErrors)


Test patch:
diff --git a/cmd/osbuild-worker-executor/export_test.go b/cmd/osbuild-worker-executor/export_test.go
index cf9b430d4..9698623c8 100644
--- a/cmd/osbuild-worker-executor/export_test.go
+++ b/cmd/osbuild-worker-executor/export_test.go
@@ -4,9 +4,6 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
-
-	"github.com/sirupsen/logrus"
-	logrusTest "github.com/sirupsen/logrus/hooks/test"
 )
 
 var (
@@ -15,19 +12,6 @@ var (
 	HandleIncludedSources = handleIncludedSources
 )
 
-func MockLogger() (hook *logrusTest.Hook, restore func()) {
-	saved := logrusNew
-	logger, hook := logrusTest.NewNullLogger()
-	logrusNew = func() *logrus.Logger {
-		return logger
-	}
-	logger.SetLevel(logrus.DebugLevel)
-
-	return hook, func() {
-		logrusNew = saved
-	}
-}
-
 func MockOsbuildBinary(t *testing.T, new string) (restore func()) {
 	t.Helper()
 
diff --git a/cmd/osbuild-worker-executor/main_test.go b/cmd/osbuild-worker-executor/main_test.go
index 50ff90446..97aca273f 100644
--- a/cmd/osbuild-worker-executor/main_test.go
+++ b/cmd/osbuild-worker-executor/main_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/sirupsen/logrus"
 	logrusTest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 
@@ -57,8 +58,8 @@ func runTestServer(t *testing.T) (baseURL, buildBaseDir string, loggerHook *logr
 	ctx, cancel := context.WithCancel(context.Background())
 	t.Cleanup(cancel)
 
-	loggerHook, restore := main.MockLogger()
-	defer restore()
+	logger, loggerHook := logrusTest.NewNullLogger()
+	logger.SetLevel(logrus.DebugLevel)
 
 	args := []string{
 		"-host", host,
@@ -66,7 +67,7 @@ func runTestServer(t *testing.T) (baseURL, buildBaseDir string, loggerHook *logr
 		"-build-path", buildBaseDir,
 	}
 	go func() {
-		_ = main.Run(ctx, args, os.Getenv)
+		_ = main.Run(ctx, args, os.Getenv, logger)
 	}()
 
 	err := waitReady(ctx, defaultTimeout, baseURL)