From efbaa93eef3ebb18bc6009b10b1f0bcae07cf33e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Hozza?= Date: Fri, 25 Aug 2023 17:03:07 +0200 Subject: [PATCH] Target/Koji: rework result options to contain more than just image info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `KojiTargetResultOptions` previously contained information only about the uploaded image file. And even then, some information, such as the filename, were scattered in other structures such as `KojiFinalizeJob` struct. Since the plan is to start uploading also osbuild manifest and osbuild build log to Koji, we need to extend the result options structure to hold more information and also make it specific to which file is the information related. Rework the `KojiTargetResultOptions` to contain information about: - the built image - build log - osbuild manifest Information about each file contains: - filename - checksum type - file checksum - file size For now, only the built image information is set and consumed by the worker. Add custom JSON (un)marshaler for `KojiTargetResultOptions` to handle backward compatibility when old version of worker or composer server interact with each other. Cover them with unit tests. Signed-off-by: Tomáš Hozza --- cmd/osbuild-worker/jobimpl-koji-finalize.go | 19 ++- cmd/osbuild-worker/jobimpl-osbuild.go | 8 +- internal/cloudapi/v2/v2_koji_test.go | 56 +++++-- internal/target/koji_target.go | 101 +++++++++++- internal/target/koji_target_test.go | 163 ++++++++++++++++++++ internal/target/targetresult_test.go | 10 +- internal/worker/json.go | 9 +- 7 files changed, 336 insertions(+), 30 deletions(-) create mode 100644 internal/target/koji_target_test.go diff --git a/cmd/osbuild-worker/jobimpl-koji-finalize.go b/cmd/osbuild-worker/jobimpl-koji-finalize.go index ab4850fa3..70abe8dbc 100644 --- a/cmd/osbuild-worker/jobimpl-koji-finalize.go +++ b/cmd/osbuild-worker/jobimpl-koji-finalize.go @@ -187,15 +187,24 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error { Arch: buildArgs.Arch, BootMode: buildArgs.ImageBootMode, } - imgOutputsExtraInfo[args.KojiFilenames[i]] = imgOutputExtraInfo + + // The image filename is now set in the KojiTargetResultOptions. + // For backward compatibility, if the filename is not set in the + // options, use the filename from the KojiTargetOptions. + imageFilename := kojiTargetOptions.Image.Filename + if imageFilename == "" { + imageFilename = args.KojiFilenames[i] + } + + imgOutputsExtraInfo[imageFilename] = imgOutputExtraInfo outputs = append(outputs, koji.BuildOutput{ BuildRootID: uint64(i), - Filename: args.KojiFilenames[i], - FileSize: kojiTargetOptions.ImageSize, + Filename: imageFilename, + FileSize: kojiTargetOptions.Image.Size, Arch: buildArgs.Arch, - ChecksumType: koji.ChecksumTypeMD5, - Checksum: kojiTargetOptions.ImageMD5, + ChecksumType: koji.ChecksumType(kojiTargetOptions.Image.ChecksumType), + Checksum: kojiTargetOptions.Image.Checksum, Type: koji.BuildOutputTypeImage, RPMs: imageRPMs, Extra: &koji.BuildOutputExtra{ diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go index 3625fc0b8..c9128f73b 100644 --- a/cmd/osbuild-worker/jobimpl-osbuild.go +++ b/cmd/osbuild-worker/jobimpl-osbuild.go @@ -872,8 +872,12 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error { } logWithId.Info("[Koji] 🎉 Image successfully uploaded") targetResult.Options = &target.KojiTargetResultOptions{ - ImageMD5: imageHash, - ImageSize: imageSize, + Image: &target.KojiOutputInfo{ + Filename: jobTarget.ImageName, + ChecksumType: target.ChecksumTypeMD5, + Checksum: imageHash, + Size: imageSize, + }, } case *target.OCITargetOptions: diff --git a/internal/cloudapi/v2/v2_koji_test.go b/internal/cloudapi/v2/v2_koji_test.go index 69d5ce31c..c4463457b 100644 --- a/internal/cloudapi/v2/v2_koji_test.go +++ b/internal/cloudapi/v2/v2_koji_test.go @@ -50,8 +50,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: true, @@ -87,8 +91,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: true, @@ -124,8 +132,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: true, @@ -160,8 +172,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: false, @@ -198,8 +214,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: true, @@ -247,8 +267,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: true, @@ -288,8 +312,12 @@ func TestKojiCompose(t *testing.T) { Arch: test_distro.TestArchName, HostOS: test_distro.TestDistroName, TargetResults: []*target.TargetResult{target.NewKojiTargetResult(&target.KojiTargetResultOptions{ - ImageMD5: "browns", - ImageSize: 42, + Image: &target.KojiOutputInfo{ + Filename: "test.img", + ChecksumType: target.ChecksumTypeMD5, + Checksum: "browns", + Size: 42, + }, })}, OSBuildOutput: &osbuild.Result{ Success: true, diff --git a/internal/target/koji_target.go b/internal/target/koji_target.go index 72e58de4d..6145e29b3 100644 --- a/internal/target/koji_target.go +++ b/internal/target/koji_target.go @@ -1,5 +1,10 @@ package target +import ( + "encoding/json" + "fmt" +) + const TargetNameKoji TargetName = "org.osbuild.koji" type KojiTargetOptions struct { @@ -13,9 +18,101 @@ func NewKojiTarget(options *KojiTargetOptions) *Target { return newTarget(TargetNameKoji, options) } +// ChecksumType represents the type of a checksum used for a KojiOutputInfo. +type ChecksumType string + +const ( + ChecksumTypeMD5 ChecksumType = "md5" + + // Only MD5 is supported for now to enable backwards compatibility. + // The reason is tha the old KojiTargetOptions contained only + // ImageMD5 and ImageSize fields, which mandates the use of MD5. + // TODO: uncomment the lines below when the backwards compatibility is no longer needed. + //ChecksumTypeAdler32 ChecksumType = "adler32" + //ChecksumTypeSHA256 ChecksumType = "sha256" +) + +// KojiOutputInfo represents the information about any output file uploaded to Koji +// as part of the OSBuild job. This information is then used by the KojiFinalize +// job when importing files into Koji. +type KojiOutputInfo struct { + Filename string `json:"filename"` + ChecksumType ChecksumType `json:"checksum_type"` + Checksum string `json:"checksum"` + Size uint64 `json:"size"` +} + type KojiTargetResultOptions struct { - ImageMD5 string `json:"image_md5"` - ImageSize uint64 `json:"image_size"` + Image *KojiOutputInfo `json:"image"` + Log *KojiOutputInfo `json:"log,omitempty"` + OSBuildManifest *KojiOutputInfo `json:"osbuild_manifest,omitempty"` +} + +func (o *KojiTargetResultOptions) UnmarshalJSON(data []byte) error { + type aliasType KojiTargetResultOptions + if err := json.Unmarshal(data, (*aliasType)(o)); err != nil { + return err + } + + // compatType contains deprecated fields, which are being checked + // for backwards compatibility. + type compatType struct { + // Deprecated: Use Image in KojiTargetOptions instead. + // Kept for backwards compatibility. + ImageMD5 string `json:"image_md5"` + ImageSize uint64 `json:"image_size"` + } + + var compat compatType + if err := json.Unmarshal(data, &compat); err != nil { + return err + } + + // Check if the Image data in the new struct format are set. + // If not, then the data are coming from an old composer. + if o.Image == nil { + // o.Image.Filename is kept empty, because the filename was previously + // not set as there was always only the Image file. The KojiFinalize job + // handles this case and takes the Image filename from the KojiFinalizeJob + // options. + + o.Image = &KojiOutputInfo{ + ChecksumType: ChecksumTypeMD5, + Checksum: compat.ImageMD5, + Size: compat.ImageSize, + } + } + + return nil +} + +func (o KojiTargetResultOptions) MarshalJSON() ([]byte, error) { + type alias KojiTargetResultOptions + // compatType is a super-set of the current KojiTargetResultOptions and + // old version of it. It contains deprecated fields, which are being set + // for backwards compatibility. + type compatType struct { + alias + + // Deprecated: Use Image in KojiTargetOptions instead. + // Kept for backwards compatibility. + ImageMD5 string `json:"image_md5"` + ImageSize uint64 `json:"image_size"` + } + + // Only MD5 is supported for now to enable backwards compatibility. + // TODO: remove this block when the backwards compatibility is no longer needed. + if o.Image.ChecksumType != ChecksumTypeMD5 { + return nil, fmt.Errorf("unsupported checksum type: %s", o.Image.ChecksumType) + } + + compat := compatType{ + alias: (alias)(o), + ImageMD5: o.Image.Checksum, + ImageSize: o.Image.Size, + } + + return json.Marshal(compat) } func (KojiTargetResultOptions) isTargetResultOptions() {} diff --git a/internal/target/koji_target_test.go b/internal/target/koji_target_test.go new file mode 100644 index 000000000..41b03e025 --- /dev/null +++ b/internal/target/koji_target_test.go @@ -0,0 +1,163 @@ +package target + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestKojiTargetOptionsUnmarshalJSON tests that the resulting struct +// has appropriate fields set when legacy JSON is used. +func TestKojiTargetResultOptionsUnmarshalJSON(t *testing.T) { + type testCase struct { + name string + JSON []byte + expectedResult *KojiTargetResultOptions + err bool + } + + testCases := []testCase{ + { + name: "new format", + JSON: []byte(`{"image":{"checksum_type":"md5","checksum":"hash","filename":"image.raw","size":123456}}`), + expectedResult: &KojiTargetResultOptions{ + Image: &KojiOutputInfo{ + Filename: "image.raw", + ChecksumType: "md5", + Checksum: "hash", + Size: 123456, + }, + }, + }, + { + name: "old format", + JSON: []byte(`{"image_md5":"hash","image_size":123456}`), + expectedResult: &KojiTargetResultOptions{ + Image: &KojiOutputInfo{ + ChecksumType: "md5", + Checksum: "hash", + Size: 123456, + }, + }, + }, + { + name: "full format", + JSON: []byte(`{"image":{"checksum_type":"md5","checksum":"hash","filename":"image.raw","size":123456},"log":{"checksum_type":"md5","checksum":"hash","filename":"log.txt","size":123456},"osbuild_manifest":{"checksum_type":"md5","checksum":"hash","filename":"manifest.json","size":123456}}`), + expectedResult: &KojiTargetResultOptions{ + Image: &KojiOutputInfo{ + Filename: "image.raw", + ChecksumType: "md5", + Checksum: "hash", + Size: 123456, + }, + Log: &KojiOutputInfo{ + Filename: "log.txt", + ChecksumType: "md5", + Checksum: "hash", + Size: 123456, + }, + OSBuildManifest: &KojiOutputInfo{ + Filename: "manifest.json", + ChecksumType: "md5", + Checksum: "hash", + Size: 123456, + }, + }, + }, + { + name: "invalid JSON", + JSON: []byte(`{"image_md5":"hash","image_size":123456`), + err: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var result KojiTargetResultOptions + err := json.Unmarshal(tc.JSON, &result) + if tc.err { + assert.Error(t, err) + return + } else { + assert.NoError(t, err) + } + assert.Equal(t, tc.expectedResult, &result) + }) + } +} + +// TestKojiTargetResultOptionsMarshalJSON tests that the resulting JSON +// has the legacy fields set for backwards compatibility. +func TestKojiTargetResultOptionsMarshalJSON(t *testing.T) { + type testCase struct { + name string + results *KojiTargetResultOptions + expectedJSON []byte + err bool + } + + testCases := []testCase{ + { + name: "backwards compatibility", + results: &KojiTargetResultOptions{ + Image: &KojiOutputInfo{ + Filename: "image.raw", + ChecksumType: ChecksumTypeMD5, + Checksum: "hash", + Size: 123456, + }, + }, + expectedJSON: []byte(`{"image":{"filename":"image.raw","checksum_type":"md5","checksum":"hash","size":123456},"image_md5":"hash","image_size":123456}`), + }, + { + name: "full format", + results: &KojiTargetResultOptions{ + Image: &KojiOutputInfo{ + Filename: "image.raw", + ChecksumType: ChecksumTypeMD5, + Checksum: "hash", + Size: 123456, + }, + Log: &KojiOutputInfo{ + Filename: "log.txt", + ChecksumType: ChecksumTypeMD5, + Checksum: "hash", + Size: 654321, + }, + OSBuildManifest: &KojiOutputInfo{ + Filename: "manifest.json", + ChecksumType: ChecksumTypeMD5, + Checksum: "hash", + Size: 123321, + }, + }, + expectedJSON: []byte(`{"image":{"filename":"image.raw","checksum_type":"md5","checksum":"hash","size":123456},"log":{"filename":"log.txt","checksum_type":"md5","checksum":"hash","size":654321},"osbuild_manifest":{"filename":"manifest.json","checksum_type":"md5","checksum":"hash","size":123321},"image_md5":"hash","image_size":123456}`), + }, + { + name: "invalid checksum type", + results: &KojiTargetResultOptions{ + Image: &KojiOutputInfo{ + Filename: "image.raw", + ChecksumType: "sha256", + Checksum: "hash", + Size: 123456, + }, + }, + err: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := json.Marshal(tc.results) + if tc.err { + assert.Error(t, err) + return + } else { + assert.NoError(t, err) + } + assert.Equal(t, tc.expectedJSON, result) + }) + } +} diff --git a/internal/target/targetresult_test.go b/internal/target/targetresult_test.go index dcc7a884f..b92bf9785 100644 --- a/internal/target/targetresult_test.go +++ b/internal/target/targetresult_test.go @@ -58,12 +58,16 @@ func TestTargetResultUnmarshal(t *testing.T) { }, }, { - resultJSON: []byte(`{"name":"org.osbuild.koji","options":{"image_md5":"hash","image_size":123456}}`), + resultJSON: []byte(`{"name":"org.osbuild.koji","options":{"image":{"checksum_type":"md5","checksum":"hash","filename":"image.raw","size":123456}}}`), expectedResult: &TargetResult{ Name: TargetNameKoji, Options: &KojiTargetResultOptions{ - ImageMD5: "hash", - ImageSize: 123456, + Image: &KojiOutputInfo{ + Filename: "image.raw", + ChecksumType: ChecksumTypeMD5, + Checksum: "hash", + Size: 123456, + }, }, }, }, diff --git a/internal/worker/json.go b/internal/worker/json.go index ae8305f52..8b5feddad 100644 --- a/internal/worker/json.go +++ b/internal/worker/json.go @@ -124,10 +124,11 @@ type KojiInitJobResult struct { } type KojiFinalizeJob struct { - Server string `json:"server"` - Name string `json:"name"` - Version string `json:"version"` - Release string `json:"release"` + Server string `json:"server"` + Name string `json:"name"` + Version string `json:"version"` + Release string `json:"release"` + // TODO: eventually deprecate and remove KojiFilenames, since the image filenames are now set in the KojiTargetResultOptions. KojiFilenames []string `json:"koji_filenames"` KojiDirectory string `json:"koji_directory"` TaskID uint64 `json:"task_id"` /* https://pagure.io/koji/issue/215 */