pkg: add new manifestgen package
#1153
Conversation
mvo5
force-pushed
the
new-pkg-manifestgen
branch
from
043ee78 to
605348a
Compare
| } | ||
| if len(warnings) > 0 { | ||
| // XXX: what can we do here? for things like json output? | ||
| // what are these warnings? |
There was a problem hiding this comment.
There's only a couple of warnings that can happen here and they're both related to blueprint options.
- Adding a user to an edge- or iot-commit returns a deprecation warning.
- Building a FIPS-enabled image on a non-FIPS-enabled host returns this warning.
Sidenote: The first one should probably be dropped since the plan was to deprecate users in some version of RHEL 9, but with edge going away in RHEL 10, we might as well stop warning about it and keep the feature around as long as we have ostree commits.
There was a problem hiding this comment.
As for what we can do, I think printing them to stderr should be enough. But then, if you're only doing machine-readable output, they might be missed right?
Should we add a warnings field to our output?
There was a problem hiding this comment.
Sorry, I should update this. As part of the work in https://github.com/osbuild/images/compare/main...mvo5:new-pkg-manifestgen-used-in-build?expand=1 I added code that adds a "manifestgen: add new manifestgen.Options.WarningsOutput" which should cover this. So the user can decide if warnings are fatal or surfaced (via the WarningsOutput(Writer).
There was a problem hiding this comment.
First of all, I like this new interface for generating manifests. ❤️
A bit of a weird part is that the initial commits work with a code version from before #1142. I expect that as a result, those commits would fail to compile if merged to main. The end result is fine, but maybe it would make sense to squash some commits together, so that every commit of this PR compiles on top of the current implementation?
There is a bug in how SBOMs are written, which I missed when looking at a draft version of osbuild/image-builder-cli#69. This should be fixed, because the SBOM documents written by the code are not valid SPDX documents.
I also suggest to remove some of the XXX comments from the code that discussess moving it to the images repository, since this PRs now does that.
pkg/manifestgen/manifestgen.go
Outdated
|
|
||
| var buf bytes.Buffer | ||
| enc := json.NewEncoder(&buf) | ||
| if err := enc.Encode(depsolvedPipeline.SBOM); err != nil { |
There was a problem hiding this comment.
This is not correct. We should serialize just depsolvedPipeline.SBOM.Document, which is the actual SBOM document.
There was a problem hiding this comment.
Oh, excellent catch, thank you. I will fix this.
There was a problem hiding this comment.
This made me wonder, how would you feel about a potential followup with something like:
diff --git a/pkg/sbom/document.go b/pkg/sbom/document.go
index 8d4dd2c96..6cc02e240 100644
--- a/pkg/sbom/document.go
+++ b/pkg/sbom/document.go
@@ -3,6 +3,7 @@ package sbom
import (
"encoding/json"
"fmt"
+ "io"
)
type StandardType uint64
@@ -68,3 +69,15 @@ func NewDocument(docType StandardType, doc json.RawMessage) (*Document, error) {
Document: doc,
}, nil
}
+
+// Export exports the given writer
+func (doc *Document) Export(w io.Writer) error {
+ switch doc.DocType {
+ case StandardTypeNone:
+ return fmt.Errorf("cannot export empty SBOM documents")
+ case StandardTypeSpdx:
+ return json.NewEncoder(w).Encode(doc.Document)
+ default:
+ return fmt.Errorf("internal error: unsupported export for %v", doc.DocType)
+ }
+}Alternatively the signature could be func (doc *Document) Export() (content []byte, fileExt string, err error) which would be easier to handle from the caller, eliminate the need for a "FileExt()" helper and not cause more memory because the document is already a []byte so we would just pass this pointer (at least currently, for the future the io.Writer might be beneficial but it make the manifestgen code slightly more complicated as we now need an "io.Pipe" there to forward the document.Export() to the manifestgen.SBOMWriter(). But most likely followup material (if we do it at all, the exiting code is fine of course).
There was a problem hiding this comment.
I like it 👍
EDIT: specifically the version in the code snippet
There was a problem hiding this comment.
Are you okay with doing the sbom changes in a followup? I plan one anyway with some extras like "warnings" support but I did not want to include them to avoid the PR getting too big and what we have here is enough to remove the version in image-builder-cli. But happy to also add this here if you prefer to see this in the "bigger context".
pkg/manifestgen/manifestgen.go
Outdated
| } | ||
| // XXX: sync with image-builder-cli:build.go name generation - can we have a shared helper? | ||
| imageName := fmt.Sprintf("%s-%s-%s", dist.Name(), imgType.Name(), a.Name()) | ||
| sbomDocOutputFilename := fmt.Sprintf("%s.%s-%s.spdx.json", imageName, pipelinePurpose, plName) |
There was a problem hiding this comment.
It may make sense to determine the extension based on depsolvedPipeline.SBOM.DocType value.
There was a problem hiding this comment.
What do you think about moving this into the sbom/document.go itself? This way we don't need to duplicate the logic across the callers. Something like:
diff --git a/pkg/sbom/document.go b/pkg/sbom/document.go
index 8d4dd2c96..68567f568 100644
--- a/pkg/sbom/document.go
+++ b/pkg/sbom/document.go
@@ -68,3 +68,12 @@ func NewDocument(docType StandardType, doc json.RawMessage) (*Document, error) {
Document: doc,
}, nil
}
+
+func (doc *Document) FileExt() string {
+ switch doc.DocType {
+ case StandardTypeSpdx:
+ return "spdx.json"
+ default:
+ return ""
+ }
+}?
This commit adds a new `osbuild/manifesttest` that helps sharing code when testing generated osbuild manifests.
This commit adds a new generic `manifestgen` package that can be used to generate osbuild manifests. It works on a higher level then the low-level `manifest` package from `images` and provides plugable resolvers and a streamlined API. Thanks to Tomáš Hozza for the comment around `imgType.Manifest()` in `Generate()`.
This commit creates a temporary directory for the defaultDepvolver() if no cacheDir is given. This ensures that we do not clutter the current working directory with `platform:foo` cache directories that `solver.Depvolve()` creates. Note that this is only tested indirectly via the integration test in `test_container.py:test_container_builds_image()` that checks that the output directory is clean.
This commit allows passing an osbuild.RpmDownloader via the `manifestgen.Options` struct. This allows us to switch manifests to use librepo instead of curl for downloading.
mvo5
force-pushed
the
new-pkg-manifestgen
branch
from
605348a to
33edd18
Compare
This commit adds an `SBOMWriter()` callback to the `manifestgen.Options` struct. This allows to export the SBOMs when the manifest is generated. Note that usually at least two files are generated, one for the "image" payload and one for the "buildroot", but it can be an arbitrary number. They typcially look something like: ``` centos-9-qcow2-x86_64.buildroot-build.spdx.json centos-9-qcow2-x86_64.image-os.spdx.json ``` The callback will not write the files directly but just provide the suggested filename and the data so that the caller is flexible and can also put it in e.g. a database.
This commit adds support for custom seed arguments via the `manifesten.Options` to support use-cases like `cmd/gen-manifests`.
For some of our more specialised use-cases (like gen-manifests) it is useful to give them access to the default implementaions of the solvers so that they can e.g. use them but also collect the results.
This commit moves `gen-manifests` to use the manifestgen module
to remove some code duplication.
It does not move to use the actual `manifestgen.Generator` because
it needs access to all results of the "solvers" to save() them.
While this is easy enough to do by just having something like:
```go
var depsolvedSet map[string]dnfjson.DepsolveResult
opts := &manifestgen.Options{
Output: &mf,
Cachedir: cacheDir,
CustomSeed: &seedArg,
Depsolver: func(cacheDir string, packageSets map[string][]rpmmd.PackageSet, d distro.Distro, arch string) (map[string]dnfjson.DepsolveResult, error) {
depsolvedSet, err = manifestgen.DefaultDepsolver(cacheDir, packageSets, d, arch)
return depsolvedSet, err
},
}
...
if err := mg.Generate(&bp, distribution, imgType, distroArch, &imgOptions); err != nil {
```
it would make the code here about the same size as before and
potentially more confusing. We can still do it if desired.
This commit makes the `cmd/build` command use the manifestgen module to remove some code duplication. It does not move to use the actual `manifestgen.Generator` because the way the repositories are used is not supported by manifestgen right now: `build` supports loading a single repository file but manifestgen expects a reporegistry so that it can map "distroname" to a "repository". We could add support for loading a repo json directly in `manifestgen.Generator` but I do feel uneasy about it because it seems a good property that there is some connection between the distroname and the repo in the general case.
This commit changes the depsolve cache to a persistent location to speed up the resolving. Thanks to Achilleas for the suggestion.
mvo5
force-pushed
the
new-pkg-manifestgen
branch
from
33edd18 to
76b3327
Compare
This commit drops the internal `manifestgen` package in favor of using the version in `images` now that osbuild/images#1153 is merged.
This commit drops the internal `manifestgen` package in favor of using the version in `images` now that osbuild/images#1153 is merged.
This commit drops the internal `manifestgen` package in favor of using the version in `images` now that osbuild/images#1153 is merged.
This commit adds a new generic
manifestgenpackage that can beused to generate osbuild manifests. It works on a higher level
then the low-level
manifestpackage fromimagesand providesplugable resolvers and a streamlined API.
The rational is that we currently have duplicated implementations
of e.g.
depsolve,containerResolver,ostreeCommitResolver.This helper provides default implementations for the common case
but also provide the ability to override the defaults.
This is a cleaned up version of https://github.com/osbuild/image-builder-cli/tree/main/internal/manifestgen
It is used in image-builder-cli like this: https://github.com/osbuild/image-builder-cli/blob/main/cmd/image-builder/manifest.go#L39
I did not include the full port of the "cmd/build" in this PR but I did do it in https://github.com/osbuild/images/compare/main...mvo5:new-pkg-manifestgen-used-in-build?expand=1 - this is what build would looks like https://github.com/mvo5/images/blob/30aa2cf16355d2ba5f4289b70c4ff4b15eed0a9e/cmd/build/main.go#L119 (still some warts and it will increase the size of this PR even more but I could pull it in here).