Repository: osbuild/osbuild-composer
Pull request #2758 (2022-06-16 14:30:18)

Consolidate Koji target options values meaning
When the Koji target support was added to the osbuild job, based on the osbuild-koji job, the meaning of target option values got messed up.

This PR is ensuring, that the Koji target interprets values set in target options in the same way as any other target.

In addition, ensure that the worker server returns "Bad Request" if the worker tries to upload an artifact to it, but the server is configured to not accept any artifacts from workers.

Please not that this change is backward incompatible. This was done intentionally after a discussion with @ondrejbudai, because the Koji composes via Cloud API are currently not used anywhere in production.

This pull request includes:

- [ ] adequate testing for the new functionality or fixed issue
- [ ] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`

Information regarding our GitLab pipeline (Schutzbot):

CI will not be ran automatically if WIP label is applied or the PR is in DRAFT state, instead only
a link will be provided to the pipeline which can then be triggered manually if desired. To run the
CI automatically either switch the PR to ready or apply WIP+test label.

Outside contributors need manual approval from one of the osbuild-composer maintainers.

Schutzbot will only be triggered if all Tests jobs in GitHub workflow succeed.
-->



Patch:
diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go
index b24db25d3..b48020fbc 100644
--- a/cmd/osbuild-worker/jobimpl-osbuild.go
+++ b/cmd/osbuild-worker/jobimpl-osbuild.go
@@ -748,7 +748,8 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 					logWithId.Warnf("[Koji] logout failed: %v", err)
 				}
 			}()
-			file, err := os.Open(path.Join(outputDirectory, exportPath, args.ImageName))
+
+			file, err := os.Open(path.Join(outputDirectory, exportPath, options.Filename))
 			if err != nil {
 				osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorKojiBuild, fmt.Sprintf("failed to open the image for reading: %v", err))
 				return nil
@@ -756,7 +757,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 			defer file.Close()
 
 			logWithId.Info("[Koji] ⬆ Uploading the image")
-			imageHash, imageSize, err := kojiAPI.Upload(file, options.UploadDirectory, options.Filename)
+			imageHash, imageSize, err := kojiAPI.Upload(file, options.UploadDirectory, args.Targets[0].ImageName)
 			if err != nil {
 				osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorUploadingImage, err.Error())
 				return nil
diff --git a/internal/cloudapi/v2/server.go b/internal/cloudapi/v2/server.go
index 672a5b876..40ca19ed6 100644
--- a/internal/cloudapi/v2/server.go
+++ b/internal/cloudapi/v2/server.go
@@ -177,18 +177,21 @@ func (s *Server) enqueueKojiCompose(taskID uint64, server, name, version, releas
 			ir.arch.Name(),
 			splitExtension(ir.imageType.Filename()),
 		)
+
+		kojiTarget := target.NewKojiTarget(&target.KojiTargetOptions{
+			Server:          server,
+			UploadDirectory: kojiDirectory,
+			Filename:        ir.imageType.Filename(),
+		})
+		kojiTarget.ImageName = kojiFilename
+
 		buildID, err := s.workers.EnqueueOSBuildAsDependency(ir.arch.Name(), &worker.OSBuildJob{
-			ImageName: ir.imageType.Filename(),
-			Exports:   ir.imageType.Exports(),
+			Exports: ir.imageType.Exports(),
 			PipelineNames: &worker.PipelineNames{
 				Build:   ir.imageType.BuildPipelines(),
 				Payload: ir.imageType.PayloadPipelines(),
 			},
-			Targets: []*target.Target{target.NewKojiTarget(&target.KojiTargetOptions{
-				Server:          server,
-				UploadDirectory: kojiDirectory,
-				Filename:        kojiFilename,
-			})},
+			Targets:            []*target.Target{kojiTarget},
 			ManifestDynArgsIdx: common.IntToPtr(1),
 		}, []uuid.UUID{initID, manifestJobID}, channel)
 		if err != nil {
diff --git a/internal/target/koji_target.go b/internal/target/koji_target.go
index a46c744e2..f4e0586b1 100644
--- a/internal/target/koji_target.go
+++ b/internal/target/koji_target.go
@@ -1,6 +1,7 @@
 package target
 
 type KojiTargetOptions struct {
+	// Filename of the image as produced by osbuild for a given export
 	Filename        string `json:"filename"`
 	UploadDirectory string `json:"upload_directory"`
 	Server          string `json:"server"`
diff --git a/internal/worker/server.go b/internal/worker/server.go
index 0ad2386bb..0cc2f241e 100644
--- a/internal/worker/server.go
+++ b/internal/worker/server.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"log"
 	"net/http"
 	"os"
@@ -794,11 +793,8 @@ func (h *apiHandlers) UploadJobArtifact(ctx echo.Context, tokenstr string, name
 	request := ctx.Request()
 
 	if h.server.config.ArtifactsDir == "" {
-		_, err := io.Copy(ioutil.Discard, request.Body)
-		if err != nil {
-			return api.HTTPErrorWithInternal(api.ErrorDiscardingArtifact, err)
-		}
-		return ctx.NoContent(http.StatusOK)
+		// indicate to the worker that the server is not accepting any artifacts
+		return ctx.NoContent(http.StatusBadRequest)
 	}
 
 	f, err := os.Create(path.Join(h.server.config.ArtifactsDir, "tmp", token.String(), name))


Test patch:
diff --git a/internal/cloudapi/v2/v2_koji_test.go b/internal/cloudapi/v2/v2_koji_test.go
index 4d4e1df18..ad5361116 100644
--- a/internal/cloudapi/v2/v2_koji_test.go
+++ b/internal/cloudapi/v2/v2_koji_test.go
@@ -398,6 +398,7 @@ func TestKojiCompose(t *testing.T) {
 		},
 	}
 	for idx, c := range cases {
+		name, version, release := "foo", "1", "2"
 		t.Run(fmt.Sprintf("Test case #%d", idx), func(t *testing.T) {
 			composeRawReply := test.TestRouteWithReply(t, handler, false, "POST", "/api/image-builder-composer/v2/compose", fmt.Sprintf(`
 		{
@@ -424,12 +425,12 @@ func TestKojiCompose(t *testing.T) {
 			],
 			"koji": {
 				"server": "koji.example.com",
-				"name":"foo",
-				"version":"1",
-				"release":"2",
+				"name":"%[4]s",
+				"version":"%[5]s",
+				"release":"%[6]s",
 				"task_id": 42
 			}
-		}`, test_distro.TestDistroName, test_distro.TestArch3Name, string(v2.ImageTypesGuestImage)),
+		}`, test_distro.TestDistroName, test_distro.TestArch3Name, string(v2.ImageTypesGuestImage), name, version, release),
 				c.composeReplyCode, c.composeReply, "id", "operation_id")
 
 			// determine the compose ID from the reply
@@ -476,7 +477,9 @@ func TestKojiCompose(t *testing.T) {
 				require.NoError(t, err)
 				jobTarget := osbuildJob.Targets[0].Options.(*target.KojiTargetOptions)
 				require.Equal(t, "koji.example.com", jobTarget.Server)
-				require.Equal(t, "test.img", osbuildJob.ImageName)
+				require.Equal(t, "test.img", jobTarget.Filename)
+				require.Equal(t, fmt.Sprintf("%s-%s-%s.%s.img", name, version, release, test_distro.TestArch3Name),
+					osbuildJob.Targets[0].ImageName)
 				require.NotEmpty(t, jobTarget.UploadDirectory)
 
 				var buildJobResult string
diff --git a/internal/worker/server_test.go b/internal/worker/server_test.go
index f692bb7a6..c855b0630 100644
--- a/internal/worker/server_test.go
+++ b/internal/worker/server_test.go
@@ -5,6 +5,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"os"
+	"path"
 	"testing"
 	"time"
 
@@ -23,7 +25,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/worker/clienterrors"
 )
 
-func newTestServer(t *testing.T, tempdir string, jobRequestTimeout time.Duration, basePath string) *worker.Server {
+func newTestServer(t *testing.T, tempdir string, jobRequestTimeout time.Duration, basePath string, acceptArtifacts bool) *worker.Server {
 	q, err := fsjobqueue.New(tempdir)
 	if err != nil {
 		t.Fatalf("error creating fsjobqueue: %v", err)
@@ -33,12 +35,22 @@ func newTestServer(t *testing.T, tempdir string, jobRequestTimeout time.Duration
 		RequestJobTimeout: jobRequestTimeout,
 		BasePath:          basePath,
 	}
+
+	if acceptArtifacts {
+		artifactsDir := path.Join(tempdir, "artifacts")
+		err := os.Mkdir(artifactsDir, 0755)
+		if err != nil && !os.IsExist(err) {
+			t.Fatalf("cannot create state directory %s: %v", artifactsDir, err)
+		}
+		config.ArtifactsDir = artifactsDir
+	}
+
 	return worker.NewServer(nil, q, config)
 }
 
 // Ensure that the status request returns OK.
 func TestStatus(t *testing.T) {
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 	handler := server.Handler()
 	test.TestRoute(t, handler, false, "GET", "/api/worker/v1/status", ``, http.StatusOK, `{"status":"OK", "href": "/api/worker/v1/status", "kind":"Status"}`, "message", "id")
 }
@@ -67,7 +79,7 @@ func TestErrors(t *testing.T) {
 	tempdir := t.TempDir()
 
 	for _, c := range cases {
-		server := newTestServer(t, tempdir, time.Duration(0), "/api/worker/v1")
+		server := newTestServer(t, tempdir, time.Duration(0), "/api/worker/v1", false)
 		handler := server.Handler()
 		test.TestRoute(t, handler, false, c.Method, c.Path, c.Body, c.ExpectedStatus, `{"kind":"Error"}`, "message", "href", "operation_id", "reason", "id", "code")
 	}
@@ -97,7 +109,7 @@ func TestErrorsAlteredBasePath(t *testing.T) {
 	tempdir := t.TempDir()
 
 	for _, c := range cases {
-		server := newTestServer(t, tempdir, time.Duration(0), "/api/image-builder-worker/v1")
+		server := newTestServer(t, tempdir, time.Duration(0), "/api/image-builder-worker/v1", false)
 		handler := server.Handler()
 		test.TestRoute(t, handler, false, c.Method, c.Path, c.Body, c.ExpectedStatus, `{"kind":"Error"}`, "message", "href", "operation_id", "reason", "id", "code")
 	}
@@ -117,7 +129,7 @@ func TestCreate(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error creating osbuild manifest: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 	handler := server.Handler()
 
 	_, err = server.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
@@ -142,7 +154,7 @@ func TestCancel(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error creating osbuild manifest: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 	handler := server.Handler()
 
 	jobId, err := server.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
@@ -179,7 +191,7 @@ func TestUpdate(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error creating osbuild manifest: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 	handler := server.Handler()
 
 	jobId, err := server.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
@@ -208,7 +220,7 @@ func TestArgs(t *testing.T) {
 	manifest, err := imageType.Manifest(nil, distro.ImageOptions{Size: imageType.Size(0)}, nil, nil, 0)
 	require.NoError(t, err)
 
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 
 	job := worker.OSBuildJob{
 		Manifest:  manifest,
@@ -245,7 +257,7 @@ func TestUpload(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error creating osbuild manifest: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", true)
 	handler := server.Handler()
 
 	jobID, err := server.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
@@ -261,6 +273,36 @@ func TestUpload(t *testing.T) {
 	test.TestRoute(t, handler, false, "PUT", fmt.Sprintf("/api/worker/v1/jobs/%s/artifacts/foobar", token), `this is my artifact`, http.StatusOK, `?`)
 }
 
+func TestUploadNotAcceptingArtifacts(t *testing.T) {
+	distroStruct := test_distro.New()
+	arch, err := distroStruct.GetArch(test_distro.TestArchName)
+	if err != nil {
+		t.Fatalf("error getting arch from distro: %v", err)
+	}
+	imageType, err := arch.GetImageType(test_distro.TestImageTypeName)
+	if err != nil {
+		t.Fatalf("error getting image type from arch: %v", err)
+	}
+	manifest, err := imageType.Manifest(nil, distro.ImageOptions{Size: imageType.Size(0)}, nil, nil, 0)
+	if err != nil {
+		t.Fatalf("error creating osbuild manifest: %v", err)
+	}
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
+	handler := server.Handler()
+
+	jobID, err := server.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
+	require.NoError(t, err)
+
+	j, token, typ, args, dynamicArgs, err := server.RequestJob(context.Background(), arch.Name(), []string{worker.JobTypeOSBuild}, []string{""})
+	require.NoError(t, err)
+	require.Equal(t, jobID, j)
+	require.Equal(t, worker.JobTypeOSBuild, typ)
+	require.NotNil(t, args)
+	require.Nil(t, dynamicArgs)
+
+	test.TestRoute(t, handler, false, "PUT", fmt.Sprintf("/api/worker/v1/jobs/%s/artifacts/foobar", token), `this is my artifact`, http.StatusBadRequest, `?`)
+}
+
 func TestUploadAlteredBasePath(t *testing.T) {
 	distroStruct := test_distro.New()
 	arch, err := distroStruct.GetArch(test_distro.TestArchName)
@@ -275,7 +317,7 @@ func TestUploadAlteredBasePath(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error creating osbuild manifest: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/image-builder-worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/image-builder-worker/v1", true)
 	handler := server.Handler()
 
 	jobID, err := server.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
@@ -297,7 +339,7 @@ func TestTimeout(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error getting arch from distro: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Millisecond*10, "/api/image-builder-worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Millisecond*10, "/api/image-builder-worker/v1", false)
 
 	_, _, _, _, _, err = server.RequestJob(context.Background(), arch.Name(), []string{worker.JobTypeOSBuild}, []string{""})
 	require.Equal(t, jobqueue.ErrDequeueTimeout, err)
@@ -312,7 +354,7 @@ func TestRequestJobById(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error getting arch from distro: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 	handler := server.Handler()
 
 	depsolveJobId, err := server.EnqueueDepsolve(&worker.DepsolveJob{}, "")
@@ -353,7 +395,7 @@ func TestMixedOSBuildJob(t *testing.T) {
 	require := require.New(t)
 
 	emptyManifestV2 := distro.Manifest(`{"version":"2","pipelines":{}}`)
-	server := newTestServer(t, t.TempDir(), time.Millisecond*10, "/")
+	server := newTestServer(t, t.TempDir(), time.Millisecond*10, "/", false)
 	fbPipelines := &worker.PipelineNames{Build: distro.BuildPipelinesFallback(), Payload: distro.PayloadPipelinesFallback()}
 
 	oldJob := worker.OSBuildJob{
@@ -492,7 +534,7 @@ func TestMixedOSBuildKojiJob(t *testing.T) {
 	require := require.New(t)
 
 	emptyManifestV2 := distro.Manifest(`{"version":"2","pipelines":{}}`)
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 	fbPipelines := &worker.PipelineNames{Build: distro.BuildPipelinesFallback(), Payload: distro.PayloadPipelinesFallback()}
 
 	enqueueKojiJob := func(job *worker.OSBuildKojiJob) uuid.UUID {
@@ -648,7 +690,7 @@ func TestDepsolveLegacyErrorConversion(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error getting arch from distro: %v", err)
 	}
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 
 	depsolveJobId, err := server.EnqueueDepsolve(&worker.DepsolveJob{}, "")
 	require.NoError(t, err)
@@ -684,7 +726,7 @@ func TestMixedOSBuildJobErrors(t *testing.T) {
 	require := require.New(t)
 
 	emptyManifestV2 := distro.Manifest(`{"version":"2","pipelines":{}}`)
-	server := newTestServer(t, t.TempDir(), time.Millisecond*10, "/")
+	server := newTestServer(t, t.TempDir(), time.Millisecond*10, "/", false)
 
 	oldJob := worker.OSBuildJob{
 		Manifest:  emptyManifestV2,
@@ -788,7 +830,7 @@ func TestMixedOSBuildKojiJobErrors(t *testing.T) {
 	require := require.New(t)
 
 	emptyManifestV2 := distro.Manifest(`{"version":"2","pipelines":{}}`)
-	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+	server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 
 	enqueueKojiJob := func(job *worker.OSBuildKojiJob) uuid.UUID {
 		initJob := new(worker.KojiInitJob)
@@ -1705,7 +1747,7 @@ func TestJobDependencyChainErrors(t *testing.T) {
 
 	for idx, c := range cases {
 		t.Logf("Test case #%d", idx)
-		server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1")
+		server := newTestServer(t, t.TempDir(), time.Duration(0), "/api/worker/v1", false)
 		ids, err := enqueueAndFinishTestJobDependencies(server, []testJob{c.job})
 		require.Nil(t, err)
 		require.Len(t, ids, 1)