particle-os/debian-forge-composer
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

UploadJobArtifact(): return 400 if not accepting artifacts

Browse source 
The worker server API handler `UploadJobArtifact()` was previously
silently discarding artifacts uploaded by the worker, if the server was
configured to not accept artifacts.

Change the behavior to return HTTP error "Bad Request" (`400`) to the
worker, in case it tries to upload artifact to the server, but the
server is configured to not accept any artifacts.

Add a new unit test testing the new behavior and adjust existing unit
tests, which were relying on the artifact being previously silently
discarded.
This commit is contained in:
Tomas Hozza 2022-06-16 16:17:55 +02:00 • committed by Achilleas Koutsou
parent fd82174469
commit bdf009f800
2 changed files with 62 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

8
internal/worker/server.go
View file

@ -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))

78
internal/worker/server_test.go
View file

@ -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)