diff --git a/internal/cloudapi/v2/export_test.go b/internal/cloudapi/v2/export_test.go new file mode 100644 index 0000000000..d79344c20e --- /dev/null +++ b/internal/cloudapi/v2/export_test.go @@ -0,0 +1,18 @@ +package v2 + +import ( + "context" + + "github.com/google/uuid" + "github.com/osbuild/images/pkg/manifest" + "github.com/osbuild/osbuild-composer/internal/worker" +) + +// OverrideSerializeManifestFunc overrides the serializeManifestFunc for testing +func OverrideSerializeManifestFunc(f func(ctx context.Context, manifestSource *manifest.Manifest, workers *worker.Server, depsolveJobID, containerResolveJobID, ostreeResolveJobID, manifestJobID uuid.UUID, seed int64)) func() { + originalSerializeManifestFunc := serializeManifestFunc + serializeManifestFunc = f + return func() { + serializeManifestFunc = originalSerializeManifestFunc + } +} diff --git a/internal/cloudapi/v2/handler.go b/internal/cloudapi/v2/handler.go index ccece5011d..23b043e5c5 100644 --- a/internal/cloudapi/v2/handler.go +++ b/internal/cloudapi/v2/handler.go @@ -878,6 +878,9 @@ func (h *apiHandlers) getComposeManifestsImpl(ctx echo.Context, jobId uuid.UUID) if err != nil { return HTTPErrorWithInternal(ErrorComposeNotFound, fmt.Errorf("job %q: %v", jobId, err)) } + if manifestResult.JobError != nil { + return HTTPErrorWithInternal(ErrorFailedToMakeManifest, fmt.Errorf("job %q: %v", jobId, manifestResult.JobError)) + } mf = manifestResult.Manifest } @@ -907,6 +910,9 @@ func (h *apiHandlers) getComposeManifestsImpl(ctx echo.Context, jobId uuid.UUID) if err != nil { return HTTPErrorWithInternal(ErrorComposeNotFound, fmt.Errorf("job %q: %v", jobId, err)) } + if manifestResult.JobError != nil { + return HTTPErrorWithInternal(ErrorFailedToMakeManifest, fmt.Errorf("job %q: %v", jobId, manifestResult.JobError)) + } mf = manifestResult.Manifest } manifestBlobs = append(manifestBlobs, mf) diff --git a/internal/cloudapi/v2/server.go b/internal/cloudapi/v2/server.go index bbb92d6f54..86528db6f2 100644 --- a/internal/cloudapi/v2/server.go +++ b/internal/cloudapi/v2/server.go @@ -40,6 +40,10 @@ import ( // How long to wait for a depsolve job to finish const depsolveTimeoutMin = 5 +// serializeManifestFunc is used to serialize the manifest +// it can be overridden for testing +var serializeManifestFunc = serializeManifest + // Server represents the state of the cloud Server type Server struct { workers *worker.Server @@ -270,7 +274,7 @@ func (s *Server) enqueueCompose(irs []imageRequest, channel string) (uuid.UUID, s.goroutinesGroup.Add(1) go func() { - serializeManifest(s.goroutinesCtx, manifestSource, s.workers, depsolveJobID, containerResolveJobID, ostreeResolveJobID, manifestJobID, ir.manifestSeed) + serializeManifestFunc(s.goroutinesCtx, manifestSource, s.workers, depsolveJobID, containerResolveJobID, ostreeResolveJobID, manifestJobID, ir.manifestSeed) defer s.goroutinesGroup.Done() }() @@ -433,7 +437,7 @@ func (s *Server) enqueueKojiCompose(taskID uint64, server, name, version, releas // copy the image request while passing it into the goroutine to prevent data races s.goroutinesGroup.Add(1) go func(ir imageRequest) { - serializeManifest(s.goroutinesCtx, manifestSource, s.workers, depsolveJobID, containerResolveJobID, ostreeResolveJobID, manifestJobID, ir.manifestSeed) + serializeManifestFunc(s.goroutinesCtx, manifestSource, s.workers, depsolveJobID, containerResolveJobID, ostreeResolveJobID, manifestJobID, ir.manifestSeed) defer s.goroutinesGroup.Done() }(ir) } @@ -690,7 +694,7 @@ func serializeManifest(ctx context.Context, manifestSource *manifest.Manifest, w ms, err := manifestSource.Serialize(depsolveResultsInTheRightFormat, containerSpecs, ostreeCommitSpecs, nil) if err != nil { reason := "Error serializing manifest" - jobResult.JobError = clienterrors.New(clienterrors.ErrorManifestGeneration, reason, nil) + jobResult.JobError = clienterrors.New(clienterrors.ErrorManifestGeneration, reason, err.Error()) return } diff --git a/internal/cloudapi/v2/v2_koji_test.go b/internal/cloudapi/v2/v2_koji_test.go index a23df728f2..eb29e75e57 100644 --- a/internal/cloudapi/v2/v2_koji_test.go +++ b/internal/cloudapi/v2/v2_koji_test.go @@ -400,7 +400,7 @@ func TestKojiCompose(t *testing.T) { }, } - emptyManifest := `{"version":"2","pipelines":[{"name":"build"},{"name":"os"}],"sources":{"org.osbuild.curl":{"items":{"sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe":{"url":""}}}}}` + emptyManifest := `{"version":"2","pipelines":[{"name":"build"},{"name":"os"}],"sources":{"org.osbuild.curl":{"items":{"sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe":{"url":"https://pkg1.example.com/1.33-2.fc30.x86_64.rpm"}}}}}` expectedManifests := `{"manifests":[` + emptyManifest + `,` + emptyManifest + `],"kind":"ComposeManifests"}` for idx, c := range cases { name, version, release := "foo", "1", "2" diff --git a/internal/cloudapi/v2/v2_multi_tenancy_test.go b/internal/cloudapi/v2/v2_multi_tenancy_test.go index 89dafd5530..b62c39cb87 100644 --- a/internal/cloudapi/v2/v2_multi_tenancy_test.go +++ b/internal/cloudapi/v2/v2_multi_tenancy_test.go @@ -16,9 +16,11 @@ import ( "github.com/osbuild/osbuild-composer/pkg/jobqueue" "github.com/osbuild/images/pkg/distro/test_distro" + "github.com/osbuild/images/pkg/rpmmd" v2 "github.com/osbuild/osbuild-composer/internal/cloudapi/v2" "github.com/osbuild/osbuild-composer/internal/test" "github.com/osbuild/osbuild-composer/internal/worker" + "github.com/osbuild/osbuild-composer/internal/worker/api" ) func kojiRequest() string { @@ -136,13 +138,13 @@ func jobRequest() string { test_distro.TestArch3Name) } -func runNextJob(t *testing.T, jobs []uuid.UUID, workerHandler http.Handler, orgID string) { +func runNextJob(t *testing.T, jobs []uuid.UUID, workerServer *worker.Server, orgID string) { // test that a different tenant doesn't get any job // 100ms ought to be enough 🤞 ctx, cancel := context.WithDeadline(reqContext("987"), time.Now().Add(time.Millisecond*100)) defer cancel() test.APICall{ - Handler: workerHandler, + Handler: workerServer.Handler(), Method: http.MethodPost, Path: "/api/worker/v1/jobs", RequestBody: test.JSONRequestBody(jobRequest()), @@ -152,7 +154,7 @@ func runNextJob(t *testing.T, jobs []uuid.UUID, workerHandler http.Handler, orgI // get a job using the right tenant resp := test.APICall{ - Handler: workerHandler, + Handler: workerServer.Handler(), Method: http.MethodPost, Path: "/api/worker/v1/jobs", RequestBody: test.JSONRequestBody(jobRequest()), @@ -169,12 +171,48 @@ func runNextJob(t *testing.T, jobs []uuid.UUID, workerHandler http.Handler, orgI jobID := uuid.MustParse(job.ID) require.Contains(t, jobs, jobID) + jobType, err := workerServer.JobType(jobID) + require.NoError(t, err) + + var requestBody []byte + switch jobType { + // We need to set dummy values for the depsolve job result, because otherwise + // the manifest generation job would fail on empty depsolved package list. + // This would make the ComposeManifests endpoint return an error. + case worker.JobTypeDepsolve: + dummyPackage := rpmmd.PackageSpec{ + Name: "pkg1", + Version: "1.33", + Release: "2.fc30", + Arch: "x86_64", + Checksum: "sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe", + RemoteLocation: "https://pkg1.example.com/1.33-2.fc30.x86_64.rpm", + } + depsolveJobResult := &worker.DepsolveJobResult{ + PackageSpecs: map[string][]rpmmd.PackageSpec{ + // Used when depsolving a manifest + "build": {dummyPackage}, + "os": {dummyPackage}, + }, + } + rawDepsolveJobResult, err := json.Marshal(depsolveJobResult) + require.NoError(t, err) + result := api.UpdateJobRequest{ + Result: json.RawMessage(rawDepsolveJobResult), + } + requestBody, err = json.Marshal(result) + require.NoError(t, err) + // For the purpose of the test, other job types results are not important + default: + requestBody = []byte(`{"result": {"job_result":{}}}`) + } + // finish the job test.APICall{ - Handler: workerHandler, + Handler: workerServer.Handler(), Method: http.MethodPatch, Path: job.Location, - RequestBody: test.JSONRequestBody(`{"result": {"job_result":{}}}`), + RequestBody: test.JSONRequestBody(requestBody), Context: reqContext(orgID), ExpectedStatus: http.StatusOK, }.Do(t) @@ -257,7 +295,7 @@ func TestMultitenancy(t *testing.T) { // Run all jobs for j := 0; j < numjobs; j++ { - runNextJob(t, c.jobIDs, workerServer.Handler(), c.orgID) + runNextJob(t, c.jobIDs, workerServer, c.orgID) } // Make sure that the compose is not pending (i.e. all jobs did run) diff --git a/internal/cloudapi/v2/v2_test.go b/internal/cloudapi/v2/v2_test.go index 3f8233b3fe..51d669f5be 100644 --- a/internal/cloudapi/v2/v2_test.go +++ b/internal/cloudapi/v2/v2_test.go @@ -3,6 +3,7 @@ package v2_test import ( "context" "encoding/json" + "errors" "fmt" "net/http" "os" @@ -18,6 +19,7 @@ import ( "github.com/osbuild/images/pkg/distro/test_distro" "github.com/osbuild/images/pkg/distrofactory" + "github.com/osbuild/images/pkg/manifest" "github.com/osbuild/images/pkg/osbuild" "github.com/osbuild/images/pkg/ostree/mock_ostree_repo" "github.com/osbuild/images/pkg/reporegistry" @@ -90,24 +92,19 @@ func mockDepsolve(t *testing.T, workerServer *worker.Server, wg *sync.WaitGroup, if err != nil { continue } + dummyPackage := rpmmd.PackageSpec{ + Name: "pkg1", + Version: "1.33", + Release: "2.fc30", + Arch: "x86_64", + Checksum: "sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe", + RemoteLocation: "https://pkg1.example.com/1.33-2.fc30.x86_64.rpm", + } dJR := &worker.DepsolveJobResult{ PackageSpecs: map[string][]rpmmd.PackageSpec{ // Used when depsolving a manifest - "build": { - { - Name: "pkg1", - Checksum: "sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe", - }, - }, - "os": { - { - Name: "pkg1", - Version: "1.33", - Release: "2.fc30", - Arch: "x86_64", - Checksum: "sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe", - }, - }, + "build": {dummyPackage}, + "os": {dummyPackage}, }, SbomDocs: map[string]worker.SbomDoc{ "build": { @@ -837,7 +834,7 @@ func TestComposeStatusSuccess(t *testing.T) { ] }`, jobId, jobId)) - emptyManifest := `{"version":"2","pipelines":[{"name":"build"},{"name":"os"}],"sources":{"org.osbuild.curl":{"items":{"sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe":{"url":""}}}}}` + emptyManifest := `{"version":"2","pipelines":[{"name":"build"},{"name":"os"}],"sources":{"org.osbuild.curl":{"items":{"sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe":{"url":"https://pkg1.example.com/1.33-2.fc30.x86_64.rpm"}}}}}` test.TestRoute(t, srv.Handler("/api/image-builder-composer/v2"), false, "GET", fmt.Sprintf("/api/image-builder-composer/v2/composes/%v/manifests", jobId), ``, http.StatusOK, fmt.Sprintf(` { "href": "/api/image-builder-composer/v2/composes/%v/manifests", @@ -872,6 +869,143 @@ func TestComposeStatusSuccess(t *testing.T) { }`, jobId, jobId, sbomDoc, v2.ImageSBOMSbomType(v2.Spdx)), "details") } +func TestComposeManifests(t *testing.T) { + testCases := []struct { + name string + jobResult worker.ManifestJobByIDResult + expectedError *clienterrors.Error + }{ + { + name: "success", + jobResult: worker.ManifestJobByIDResult{ + Manifest: manifest.OSBuildManifest([]byte(`{"version":"2","pipelines":[{"name":"build"},{"name":"os"}],"sources":{"org.osbuild.curl":{"items":{"sha256:e50ddb78a37f5851d1a5c37a4c77d59123153c156e628e064b9daa378f45a2fe":{"url":"https://pkg1.example.com/1.33-2.fc30.x86_64.rpm"}}}}}`)), + }}, + { + name: "failure", + jobResult: worker.ManifestJobByIDResult{ + JobResult: worker.JobResult{ + JobError: clienterrors.New(clienterrors.ErrorManifestDependency, "Manifest generation test error", "Package XYZ does not have a RemoteLocation"), + }, + }, + expectedError: clienterrors.New(clienterrors.ErrorManifestDependency, "Manifest generation test error", "Package XYZ does not have a RemoteLocation"), + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + + // Override the serialize manifest func to allow simulating various job states + // This is the only way to do it, because of the way the manifest job is handled. + serializeManifestFunc := func(ctx context.Context, manifestSource *manifest.Manifest, workers *worker.Server, depsolveJobID, containerResolveJobID, ostreeResolveJobID, manifestJobID uuid.UUID, seed int64) { + var token uuid.UUID + var err error + // wait until job is in a pending state + for { + _, token, _, _, _, err = workers.RequestJobById(ctx, "", manifestJobID) + if errors.Is(err, jobqueue.ErrNotPending) { + time.Sleep(time.Millisecond * 50) + select { + case <-ctx.Done(): + t.Fatalf("Context done") + default: + continue + } + } + if err != nil { + t.Fatalf("Error requesting manifest job: %v", err) + return + } + break + } + + result, err := json.Marshal(testCase.jobResult) + require.NoError(t, err) + err = workers.FinishJob(token, result) + require.NoError(t, err) + } + defer v2.OverrideSerializeManifestFunc(serializeManifestFunc)() + + srv, wrksrv, _, cancel := newV2Server(t, t.TempDir(), false, false) + defer cancel() + + test.TestRoute(t, srv.Handler("/api/image-builder-composer/v2"), false, "POST", "/api/image-builder-composer/v2/compose", fmt.Sprintf(` + { + "distribution": "%s", + "image_request":{ + "architecture": "%s", + "image_type": "aws", + "repositories": [{ + "baseurl": "somerepo.org", + "rhsm": false + }], + "upload_options": { + "region": "eu-central-1" + } + } + }`, test_distro.TestDistro1Name, test_distro.TestArch3Name), http.StatusCreated, ` + { + "href": "/api/image-builder-composer/v2/compose", + "kind": "ComposeId" + }`, "id") + + // Handle osbuild job + jobId, token, jobType, _, _, err := wrksrv.RequestJob(context.Background(), test_distro.TestArch3Name, []string{worker.JobTypeOSBuild}, []string{""}, uuid.Nil) + require.NoError(t, err) + require.Equal(t, worker.JobTypeOSBuild, jobType) + + osbuildJobResult, err := json.Marshal(worker.OSBuildJobResult{ + Success: true, + OSBuildOutput: &osbuild.Result{Success: true}, + PipelineNames: &worker.PipelineNames{ + Build: []string{"build"}, + Payload: []string{"os"}, + }, + }) + require.NoError(t, err) + err = wrksrv.FinishJob(token, osbuildJobResult) + require.NoError(t, err) + + // Verify the compose status + test.TestRoute(t, srv.Handler("/api/image-builder-composer/v2"), false, "GET", fmt.Sprintf("/api/image-builder-composer/v2/composes/%v", jobId), ``, http.StatusOK, fmt.Sprintf(` + { + "href": "/api/image-builder-composer/v2/composes/%v", + "kind": "ComposeStatus", + "id": "%v", + "image_status": { + "status": "success" + }, + "status": "success" + }`, jobId, jobId)) + + // Verify the compose manifests + var expectedManifestsResponse string + var expectedStatusCode int + if testCase.expectedError != nil { + expectedManifestsResponse = fmt.Sprintf(` + { + "code": "IMAGE-BUILDER-COMPOSER-11", + "details": "job \"%s\": %s", + "href": "/api/image-builder-composer/v2/errors/11", + "id": "11", + "kind": "Error", + "reason": "Failed to get manifest" + }`, jobId, testCase.expectedError) + expectedStatusCode = http.StatusBadRequest + } else { + expectedManifestsResponse = fmt.Sprintf(` + { + "href": "/api/image-builder-composer/v2/composes/%v/manifests", + "id": "%v", + "kind": "ComposeManifests", + "manifests": [%s] + }`, jobId, jobId, testCase.jobResult.Manifest) + expectedStatusCode = http.StatusOK + } + test.TestRoute(t, srv.Handler("/api/image-builder-composer/v2"), false, "GET", fmt.Sprintf("/api/image-builder-composer/v2/composes/%v/manifests", jobId), ``, expectedStatusCode, expectedManifestsResponse, "operation_id") + }) + } +} + func TestComposeStatusFailure(t *testing.T) { srv, wrksrv, _, cancel := newV2Server(t, t.TempDir(), false, false) defer cancel() diff --git a/internal/test/apicall.go b/internal/test/apicall.go index bd1fa70ce3..ad4bcfcce7 100644 --- a/internal/test/apicall.go +++ b/internal/test/apicall.go @@ -75,7 +75,7 @@ func (a APICall) Do(t *testing.T) APICallResult { require.NoErrorf(t, err, "%s: could not read response body", a.Path) if a.ExpectedStatus != 0 { - assert.Equalf(t, a.ExpectedStatus, resp.StatusCode, "%s: SendHTTP failed for path", a.Path) + assert.Equalf(t, a.ExpectedStatus, resp.StatusCode, "%s: SendHTTP failed for path; body: %s", a.Path, string(body)) } if a.ExpectedBody != nil { err = a.ExpectedBody.Validate(body)