workerapi: serialize koji errors as strings

Serializing an interface does not work, let us simply use the string
representation and treat the empty string as no error. This is
compatible with the current API in the success case, and fixes the
error case, which is currently broken.

Also extend the test matrix for the kojiapi to ensure that all the
different kinds of errors can be serialized correctly and leads to
the correct status being returned.

Fixes #1079 and #1080.

cmd/osbuild-worker/jobimpl-koji-finalize.go

@@ -105,7 +105,7 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error {
 		return err
 	}
 
-	if initArgs.KojiError != nil {
+	if initArgs.KojiError != "" {
 		failure = true
 	}
 
@@ -127,7 +127,7 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error {
 		if err != nil {
 			return err
 		}
-		if !buildArgs.OSBuildOutput.Success || buildArgs.KojiError != nil {
+		if !buildArgs.OSBuildOutput.Success || buildArgs.KojiError != "" {
 			failure = true
 			break
 		}
@@ -167,9 +167,15 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error {
 
 	var result worker.KojiFinalizeJobResult
 	if failure {
-		result.KojiError = impl.kojiFail(args.Server, int(initArgs.BuildID), initArgs.Token)
+		err = impl.kojiFail(args.Server, int(initArgs.BuildID), initArgs.Token)
+		if err != nil {
+			result.KojiError = err.Error()
+		}
 	} else {
-		result.KojiError = impl.kojiImport(args.Server, build, buildRoots, images, args.KojiDirectory, initArgs.Token)
+		err = impl.kojiImport(args.Server, build, buildRoots, images, args.KojiDirectory, initArgs.Token)
+		if err != nil {
+			result.KojiError = err.Error()
+		}
 	}
 
 	err = job.Update(&result)

cmd/osbuild-worker/jobimpl-koji-init.go

@@ -60,7 +60,10 @@ func (impl *KojiInitJobImpl) Run(job worker.Job) error {
 	}
 
 	var result worker.KojiInitJobResult
-	result.Token, result.BuildID, result.KojiError = impl.kojiInit(args.Server, args.Name, args.Version, args.Release)
+	result.Token, result.BuildID, err = impl.kojiInit(args.Server, args.Name, args.Version, args.Release)
+	if err != nil {
+		result.KojiError = err.Error()
+	}
 
 	err = job.Update(&result)
 	if err != nil {

cmd/osbuild-worker/jobimpl-osbuild-koji.go

@@ -84,7 +84,7 @@ func (impl *OSBuildKojiJobImpl) Run(job worker.Job) error {
 		return err
 	}
 
-	if initArgs.KojiError == nil {
+	if initArgs.KojiError == "" {
 		result.OSBuildOutput, err = RunOSBuild(args.Manifest, impl.Store, outputDirectory, os.Stderr)
 		if err != nil {
 			return err
@@ -95,7 +95,10 @@ func (impl *OSBuildKojiJobImpl) Run(job worker.Job) error {
 			if err != nil {
 				return err
 			}
-			result.ImageHash, result.ImageSize, result.KojiError = impl.kojiUpload(f, args.KojiServer, args.KojiDirectory, args.KojiFilename)
+			result.ImageHash, result.ImageSize, err = impl.kojiUpload(f, args.KojiServer, args.KojiDirectory, args.KojiFilename)
+			if err != nil {
+				result.KojiError = err.Error()
+			}
 		}
 	}
 

internal/kojiapi/server.go

@@ -190,7 +190,7 @@ func (h *apiHandlers) PostCompose(ctx echo.Context) error {
 		}
 		time.Sleep(500 * time.Millisecond)
 	}
-	if initResult.KojiError != nil {
+	if initResult.KojiError != "" {
 		return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Could not initialize build with koji: %v", initResult.KojiError))
 	}
 
@@ -224,7 +224,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni
 		return "failure"
 	}
 
-	if initResult.KojiError != nil {
+	if initResult.KojiError != "" {
 		return "failure"
 	}
 
@@ -232,7 +232,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni
 		if buildResult.OSBuildOutput != nil && !buildResult.OSBuildOutput.Success {
 			return "failure"
 		}
-		if buildResult.KojiError != nil {
+		if buildResult.KojiError != "" {
 			return "failure"
 		}
 	}
@@ -241,7 +241,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni
 		return "pending"
 	}
 
-	if result.KojiError == nil {
+	if result.KojiError == "" {
 		return "success"
 	}
 
@@ -253,7 +253,7 @@ func imageStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiInitJ
 		return "failure"
 	}
 
-	if initResult.KojiError != nil {
+	if initResult.KojiError != "" {
 		return "failure"
 	}
 
@@ -265,7 +265,7 @@ func imageStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiInitJ
 		return "building"
 	}
 
-	if buildResult.OSBuildOutput != nil && buildResult.OSBuildOutput.Success && buildResult.KojiError == nil {
+	if buildResult.OSBuildOutput != nil && buildResult.OSBuildOutput.Success && buildResult.KojiError == "" {
 		return "success"
 	}
 

internal/kojiapi/server_test.go

@@ -17,6 +17,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/kojiapi/api"
 	distro_mock "github.com/osbuild/osbuild-composer/internal/mocks/distro"
 	rpmmd_mock "github.com/osbuild/osbuild-composer/internal/mocks/rpmmd"
+	"github.com/osbuild/osbuild-composer/internal/osbuild"
 	"github.com/osbuild/osbuild-composer/internal/test"
 	"github.com/osbuild/osbuild-composer/internal/worker"
 	"github.com/stretchr/testify/require"
@@ -55,6 +56,10 @@ func TestStatus(t *testing.T) {
 	test.TestRoute(t, handler, false, "GET", "/api/composer-koji/v1/status", ``, http.StatusOK, `{"status":"OK"}`, "message")
 }
 
+type jobResult struct {
+	Result interface{} `json:"result"`
+}
+
 func TestCompose(t *testing.T) {
 	dir, err := ioutil.TempDir("", "osbuild-composer-test-kojiapi-")
 	if err != nil {
@@ -64,139 +69,282 @@ func TestCompose(t *testing.T) {
 
 	kojiServer, workerServer := newTestKojiServer(t, dir)
 	handler := kojiServer.Handler("/api/composer-koji/v1")
-	var wg sync.WaitGroup
-	wg.Add(1)
 
-	go func() {
+	type kojiCase struct {
-		token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-init"})
+		initResult       worker.KojiInitJobResult
-		require.NoError(t, err)
+		buildResult      worker.OSBuildKojiJobResult
-		require.Equal(t, "koji-init", jobType)
+		finalizeResult   worker.KojiFinalizeJobResult
+		composeReplyCode int
+		composeReply     string
+		composeStatus    string
+	}
 
-		var initJob worker.KojiInitJob
+	var cases = []kojiCase{
-		err = json.Unmarshal(rawJob, &initJob)
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "success"
+					},
+					{
+						"status": "success"
+					}
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "success"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				KojiError: "failure",
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+			},
+			composeReplyCode: http.StatusBadRequest,
+			composeReply:     `{"message":"Could not initialize build with koji: failure"}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "failure"
+					},
+					{
+						"status": "failure"
+					}
+				],
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: false,
+				},
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "failure"
+					},
+					{
+						"status": "success"
+					}
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+				KojiError: "failure",
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "failure"
+					},
+					{
+						"status": "success"
+					}
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+			},
+			finalizeResult: worker.KojiFinalizeJobResult{
+				KojiError: "failure",
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "success"
+					},
+					{
+						"status": "success"
+					}
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+	}
+	for _, c := range cases {
+		var wg sync.WaitGroup
+		wg.Add(1)
+
+		go func(t *testing.T, result worker.KojiInitJobResult) {
+			token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-init"})
+			require.NoError(t, err)
+			require.Equal(t, "koji-init", jobType)
+
+			var initJob worker.KojiInitJob
+			err = json.Unmarshal(rawJob, &initJob)
+			require.NoError(t, err)
+			require.Equal(t, "koji.example.com", initJob.Server)
+			require.Equal(t, "foo", initJob.Name)
+			require.Equal(t, "1", initJob.Version)
+			require.Equal(t, "2", initJob.Release)
+
+			initJobResult, err := json.Marshal(&jobResult{Result: result})
+			require.NoError(t, err)
+			test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(initJobResult), http.StatusOK, `{}`)
+
+			wg.Done()
+		}(t, c.initResult)
+
+		test.TestRoute(t, handler, false, "POST", "/api/composer-koji/v1/compose", `
+		{
+			"name":"foo",
+			"version":"1",
+			"release":"2",
+			"distribution":"fedora-30",
+			"image_requests": [
+				{
+					"architecture": "x86_64",
+					"image_type": "qcow2",
+					"repositories": [
+						{
+							"baseurl": "https://repo.example.com/"
+						}
+					]
+				},
+				{
+					"architecture": "x86_64",
+					"image_type": "qcow2",
+					"repositories": [
+						{
+							"baseurl": "https://repo.example.com/"
+						}
+					]
+				}
+			],
+			"koji": {
+				"server": "koji.example.com"
+			}
+		}`, c.composeReplyCode, c.composeReply, "id")
+		wg.Wait()
+
+		token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
 		require.NoError(t, err)
-		require.Equal(t, "koji.example.com", initJob.Server)
+		require.Equal(t, "osbuild-koji", jobType)
-		require.Equal(t, "foo", initJob.Name)
+
-		require.Equal(t, "1", initJob.Version)
+		var osbuildJob worker.OSBuildKojiJob
-		require.Equal(t, "2", initJob.Release)
+		err = json.Unmarshal(rawJob, &osbuildJob)
+		require.NoError(t, err)
+		require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
+		require.Equal(t, "test.img", osbuildJob.ImageName)
+		require.NotEmpty(t, osbuildJob.KojiDirectory)
+
+		buildJobResult, err := json.Marshal(&jobResult{Result: c.buildResult})
+		require.NoError(t, err)
+		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(buildJobResult), http.StatusOK, `{}`)
+
+		token, _, jobType, rawJob, _, err = workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
+		require.NoError(t, err)
+		require.Equal(t, "osbuild-koji", jobType)
+
+		err = json.Unmarshal(rawJob, &osbuildJob)
+		require.NoError(t, err)
+		require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
+		require.Equal(t, "test.img", osbuildJob.ImageName)
+		require.NotEmpty(t, osbuildJob.KojiDirectory)
 
 		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
+			"result": {
-			"build_id": 42,
+				"arch": "x86_64",
-			"token": "foobar"
+				"host_os": "fedora-30",
-		}
+				"image_hash": "browns",
-	}`, http.StatusOK, `{}`)
+				"image_size": 42,
-		wg.Done()
+				"osbuild_output": {
-	}()
+					"success": true
-
+				}
-	test.TestRoute(t, handler, false, "POST", "/api/composer-koji/v1/compose", `
-	{
-		"name":"foo",
-		"version":"1",
-		"release":"2",
-		"distribution":"fedora-30",
-		"image_requests": [
-			{
-				"architecture": "x86_64",
-				"image_type": "qcow2",
-				"repositories": [
-					{
-						"baseurl": "https://repo.example.com/"
-					}
-				]
-			},
-			{
-				"architecture": "x86_64",
-				"image_type": "qcow2",
-				"repositories": [
-					{
-						"baseurl": "https://repo.example.com/"
-					}
-				]
 			}
-		],
+		}`, http.StatusOK, `{}`)
-		"koji": {
-			"server": "koji.example.com"
-		}
-	}`, http.StatusCreated, `{"koji_build_id":42}`, "id")
-	wg.Wait()
 
-	token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
+		token, finalizeID, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-finalize"})
-	require.NoError(t, err)
+		require.NoError(t, err)
-	require.Equal(t, "osbuild-koji", jobType)
+		require.Equal(t, "koji-finalize", jobType)
 
-	var osbuildJob worker.OSBuildKojiJob
+		var kojiFinalizeJob worker.KojiFinalizeJob
-	err = json.Unmarshal(rawJob, &osbuildJob)
+		err = json.Unmarshal(rawJob, &kojiFinalizeJob)
-	require.NoError(t, err)
+		require.NoError(t, err)
-	require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
+		require.Equal(t, "koji.example.com", kojiFinalizeJob.Server)
-	require.Equal(t, "test.img", osbuildJob.ImageName)
+		require.Equal(t, "1", kojiFinalizeJob.Version)
-	require.NotEmpty(t, osbuildJob.KojiDirectory)
+		require.Equal(t, "2", kojiFinalizeJob.Release)
+		require.ElementsMatch(t, []string{"foo-1-2.x86_64.img", "foo-1-2.x86_64.img"}, kojiFinalizeJob.KojiFilenames)
+		require.NotEmpty(t, kojiFinalizeJob.KojiDirectory)
 
-	test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
+		finalizeResult, err := json.Marshal(&jobResult{Result: c.finalizeResult})
-		"result": {
+		require.NoError(t, err)
-			"arch": "x86_64",
+		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(finalizeResult), http.StatusOK, `{}`)
-			"host_os": "fedora-30",
-			"image_hash": "browns",
-			"image_size": 42,
-			"osbuild_output": {
-				"success": true
-			}
-		}
-	}`, http.StatusOK, `{}`)
 
-	token, _, jobType, rawJob, _, err = workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
+		test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/composer-koji/v1/compose/%v", finalizeID), ``, http.StatusOK, c.composeStatus)
-	require.NoError(t, err)
+	}
-	require.Equal(t, "osbuild-koji", jobType)
-
-	err = json.Unmarshal(rawJob, &osbuildJob)
-	require.NoError(t, err)
-	require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
-	require.Equal(t, "test.img", osbuildJob.ImageName)
-	require.NotEmpty(t, osbuildJob.KojiDirectory)
-
-	test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
-			"arch": "x86_64",
-			"host_os": "fedora-30",
-			"image_hash": "browns",
-			"image_size": 42,
-			"osbuild_output": {
-				"success": true
-			}
-		}
-	}`, http.StatusOK, `{}`)
-
-	token, finalizeID, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-finalize"})
-	require.NoError(t, err)
-	require.Equal(t, "koji-finalize", jobType)
-
-	var kojiFinalizeJob worker.KojiFinalizeJob
-	err = json.Unmarshal(rawJob, &kojiFinalizeJob)
-	require.NoError(t, err)
-	require.Equal(t, "koji.example.com", kojiFinalizeJob.Server)
-	require.Equal(t, "1", kojiFinalizeJob.Version)
-	require.Equal(t, "2", kojiFinalizeJob.Release)
-	require.ElementsMatch(t, []string{"foo-1-2.x86_64.img", "foo-1-2.x86_64.img"}, kojiFinalizeJob.KojiFilenames)
-	require.NotEmpty(t, kojiFinalizeJob.KojiDirectory)
-
-	test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
-		}
-	}`, http.StatusOK, `{}`)
-
-	test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/composer-koji/v1/compose/%v", finalizeID), ``, http.StatusOK, `{
-		"image_statuses": [
-			{
-				"status": "success"
-			},
-			{
-				"status": "success"
-			}
-		],
-		"koji_build_id": 42,
-		"koji_task_id": 0,
-		"status": "success"
-	}`)
 }
 
 func TestRequest(t *testing.T) {

internal/worker/json.go

@@ -36,7 +36,7 @@ type KojiInitJob struct {
 type KojiInitJobResult struct {
 	BuildID   uint64 `json:"build_id"`
 	Token     string `json:"token"`
-	KojiError error  `json:"koji_error"`
+	KojiError string `json:"koji_error"`
 }
 
 type OSBuildKojiJob struct {
@@ -53,7 +53,7 @@ type OSBuildKojiJobResult struct {
 	OSBuildOutput *osbuild.Result `json:"osbuild_output"`
 	ImageHash     string          `json:"image_hash"`
 	ImageSize     uint64          `json:"image_size"`
-	KojiError     error           `json:"koji_error"`
+	KojiError     string          `json:"koji_error"`
 }
 
 type KojiFinalizeJob struct {
@@ -68,7 +68,7 @@ type KojiFinalizeJob struct {
 }
 
 type KojiFinalizeJobResult struct {
-	KojiError error `json:"koji_error"`
+	KojiError string `json:"koji_error"`
 }
 
 //