Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split the sbom.Format interface by encode and decode use cases #2186

Merged
merged 24 commits into from Oct 25, 2023

Conversation

wagoodman
Copy link
Contributor

@wagoodman wagoodman commented Sep 28, 2023

Today there is a single unified sbom.Format interface allowing for identification, encoding, and decoding use cases. This PR splits this interface down the middle into two use cases: encoding and decoding (where encoding would subsume the identification use case).

type FormatEncoder interface {
	ID() FormatID
	Aliases() []string
	Version() string
	Encode(io.Writer, SBOM) error
}

type FormatDecoder interface {
	Decode(by []byte) (*SBOM, FormatID, string, error)
	Identify(by []byte) (FormatID, string)
}

Why do this?

Primarily, there is one core problem with how formats are constructed today: they do not allow for injection of configuration. This means that if there are any specific behavioral differences that are desired for how an SBOM is presented then the abstraction must be broken to allow for injecting that information (this is done today for templating).

Secondarily, the abstraction can lead to bad assumptions for the decoding use case. The encoding use case requires a format ID and specific version, however, when decoding neither is really needed. That is, given a pile of bytes an object should be able to produce and SBOM or not. Having a single object where you have a strong ID and version coupled with the decoder makes it seem like you need a format object per-version-per-format-id in order to work, but this is not the case.

Changes made in this PR:

  • Split the sbom.Format interface into sbom.FormatEncoder and sbom.FormatDecoder interfaces.
  • Replaces the Validate format functions with Identify, which more closely tracks the reason for this functions existence.
  • Removes the existing sbom.Decoder and sbom.Encoder definitions. The names are not reused to prevent consumer confusion on this breaking change.
  • Remove the top-level format objects (Closes Remove deprecated syft.Format functions #1344). Otherwise these functions (which are about to be imminently removed anyway) would need to be refactored to continue to function.
  • Add sbom.FormatEncoder construction in the option.Output object, allowing for application configuration to be injected into the objects that are used to encode SBOMs (the main driver behind this change).
  • Combines option.MultiOutput and option.SingleOutput into a single option.Output object. Both original objects have a need to craft encoders when making sbom.Writer objects. This refactor was done to make this addition simpler.
  • Rename the formats package to format to be consistent with other lib packages.
  • While adding a test case it became necessary to add an additional golden image fixture. Instead of adding an additional fixture golden file, I refactored testutil to be able to reuse the same fixture file for all format tests.
  • Keeps the logic for cyclonedx and spdx more centralized so encoders for the same SBOM format (cyclone vs spdx) but different on-disk formats (xml vs tag value vs json) still have the same basic functionalities (such as supporting the same spec versions). This has lead to the additional versions support for cyclonedx-json (v1.5) thus the schema was updated.

Note for reviewers

the bump to the cyclonedx schemas make the PR diff look much worse than it is:

 schema/cyclonedx/cyclonedx.json                                                         | 2352 ++++++++++++++++++++++--
 schema/cyclonedx/cyclonedx.xsd                                                          | 4021 ++++++++++++++++++++++++++++++++++++-----

Fixes #2112

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman added the breaking-change Change is not backwards compatible label Sep 28, 2023
@wagoodman wagoodman changed the title Split format interface Split the sbom.Format interface by encode and decode use cases Sep 28, 2023
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman marked this pull request as ready for review September 29, 2023 21:10
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman self-assigned this Oct 1, 2023
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman added this to the Stabilize user surfaces milestone Oct 4, 2023
@wagoodman wagoodman marked this pull request as draft October 6, 2023 19:09
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman marked this pull request as ready for review October 9, 2023 14:23
kzantow
kzantow previously approved these changes Oct 13, 2023
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this looks good. 🎉

The one general comment I had is that I don't think we should have nearly as many Default functions for this stuff. I think it should be either: a) a New constructor, or b) another constructor function that takes a configuration. Providing multiple ways to get default config AND default format encoders confuses library consumers, when they should be using the configs and constructors, not any of the default functions (aside from getting syft lib config defaults).

Lastly, I would avoid the static defaults on the decoders, too, so this doesn't have to also later get refactored to get configuration passed in.

cmd/syft/cli/options/output.go Show resolved Hide resolved
cmd/syft/cli/options/output.go Outdated Show resolved Hide resolved
cmd/syft/cli/options/writer.go Outdated Show resolved Hide resolved
schema/cyclonedx/Makefile Outdated Show resolved Hide resolved
syft/format/internal/cyclonedxutil/versions.go Outdated Show resolved Hide resolved
syft/format/cyclonedxxml/encoder.go Outdated Show resolved Hide resolved
syft/format/cyclonedxxml/encoder_test.go Outdated Show resolved Hide resolved
syft/format/cyclonedxxml/encoder_test.go Show resolved Hide resolved
syft/format/decoders.go Show resolved Hide resolved
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman dismissed kzantow’s stale review October 13, 2023 21:18

agreed to sync later next week about more changes

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 🎉

syft/format/spdxtagvalue/decoder.go Show resolved Hide resolved
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman enabled auto-merge (squash) October 25, 2023 13:02
@github-actions github-actions bot removed the breaking-change Change is not backwards compatible label Oct 25, 2023
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman merged commit 7392d60 into main Oct 25, 2023
10 checks passed
@wagoodman wagoodman deleted the split-format-interface branch October 25, 2023 13:43
@spiffcs spiffcs added the enhancement New feature or request label Oct 31, 2023
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
…re#2186)

* split up sbom.Format into encode and decode ops

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* update cmd pkg to inject format configs

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* bump cyclonedx schema to 1.5

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* redact image metadata from github encoder tests

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* add more testing around format decoder identify

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* add test case for format version options

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix cli tests

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix CLI test

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* [wip] - review comments

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* keep encoder creation out of post load function

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* keep decider and identify functions

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* add a few more doc comments

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* remove format encoder default function helpers

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* address PR feedback

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* move back to streaming based decode functions

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* with common convention for encoder constructors

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix tests and allow for encoders to be created from cli options

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix cli tests

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* fix linting

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

* buffer reads from stdin to support seeking

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

---------

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPDX-2.3 is misidentified as SPDX-2.2 Remove deprecated syft.Format functions
3 participants