From 5d1af1685e047339aadd0f888b0c425862c00791 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Mon, 8 May 2023 14:20:03 -0400 Subject: [PATCH 1/5] Lower log level in unknown relationship type An unknown relationship type doesn't mean things are incorrect; it probably means that the version of syft that generated the SBOM is ahead of the version of syft that's parsing it. Signed-off-by: Will Murphy --- syft/formats/syftjson/to_syft_model.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syft/formats/syftjson/to_syft_model.go b/syft/formats/syftjson/to_syft_model.go index cbc03726f58..260566083f9 100644 --- a/syft/formats/syftjson/to_syft_model.go +++ b/syft/formats/syftjson/to_syft_model.go @@ -194,7 +194,7 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio case artifact.OwnershipByFileOverlapRelationship, artifact.ContainsRelationship, artifact.DependencyOfRelationship, artifact.EvidentByRelationship: default: if !strings.Contains(string(typ), "dependency-of") { - log.Warnf("unknown relationship type: %s", typ) + log.Tracef("unknown relationship type: %s", typ) return nil } // lets try to stay as compatible as possible with similar relationship types without dropping the relationship From 506c15cef91c9a954b0ba2ca938c85a457a09a82 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Mon, 8 May 2023 15:01:02 -0400 Subject: [PATCH 2/5] Use debug instead of trace Knowing that the event-type was unexpected seems like useful information while debugging. Signed-off-by: Will Murphy --- syft/formats/syftjson/to_syft_model.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syft/formats/syftjson/to_syft_model.go b/syft/formats/syftjson/to_syft_model.go index 260566083f9..ca7743b8844 100644 --- a/syft/formats/syftjson/to_syft_model.go +++ b/syft/formats/syftjson/to_syft_model.go @@ -194,7 +194,7 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio case artifact.OwnershipByFileOverlapRelationship, artifact.ContainsRelationship, artifact.DependencyOfRelationship, artifact.EvidentByRelationship: default: if !strings.Contains(string(typ), "dependency-of") { - log.Tracef("unknown relationship type: %s", typ) + log.Debugf("unknown relationship type: %s", typ) return nil } // lets try to stay as compatible as possible with similar relationship types without dropping the relationship From 205aa08056b0af4ea9899ae3405232730fcca2e5 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Tue, 9 May 2023 09:26:23 -0400 Subject: [PATCH 3/5] Warn only once for all unknwon relationship types Signed-off-by: Will Murphy --- syft/formats/syftjson/to_syft_model.go | 61 +++++++++++++++++---- syft/formats/syftjson/to_syft_model_test.go | 33 +++++++++++ 2 files changed, 84 insertions(+), 10 deletions(-) diff --git a/syft/formats/syftjson/to_syft_model.go b/syft/formats/syftjson/to_syft_model.go index ca7743b8844..e43be4bd5fd 100644 --- a/syft/formats/syftjson/to_syft_model.go +++ b/syft/formats/syftjson/to_syft_model.go @@ -1,7 +1,9 @@ package syftjson import ( + "fmt" "os" + "sort" "strconv" "strings" @@ -149,14 +151,42 @@ func toSyftRelationships(doc *model.Document, catalog *pkg.Collection, relations idMap[f.ID] = f.Location } + out, warnings := toSyftRelationshipsWithWarnings(idMap, relationships, idAliases) + for _, warning := range warnings { + log.Warn(warning) + } + + return out +} + +func toSyftRelationshipsWithWarnings(idMap map[string]interface{}, relationships []model.Relationship, idAliases map[string]string) ([]artifact.Relationship, []string) { var out []artifact.Relationship + var warnings []string + omittedRelationshipTypes := make(map[string]int) + omittedRelatinshipCount := 0 for _, r := range relationships { - syftRelationship := toSyftRelationship(idMap, r, idAliases) + syftRelationship, err := toSyftRelationship(idMap, r, idAliases) + if err != nil { + if castErr, ok := err.(*unknownRelationshipTypeError); ok { + omittedRelationshipTypes[castErr.UnknownType()] = 1 + omittedRelatinshipCount++ + } else { + warnings = append(warnings, err.Error()) + } + } if syftRelationship != nil { out = append(out, *syftRelationship) } } - return out + if len(omittedRelationshipTypes) > 0 { + keys := make([]string, 0) + for key := range omittedRelationshipTypes { + keys = append(keys, key) + } + sort.Strings(keys) + warnings = append(warnings, fmt.Sprintf("Omitted %d relationship(s) of these unknown types: %v", omittedRelatinshipCount, strings.Join(keys, ", "))) + } + return out, warnings } func toSyftSource(s model.Source) *source.Source { @@ -167,7 +197,7 @@ func toSyftSource(s model.Source) *source.Source { return newSrc } -func toSyftRelationship(idMap map[string]interface{}, relationship model.Relationship, idAliases map[string]string) *artifact.Relationship { +func toSyftRelationship(idMap map[string]interface{}, relationship model.Relationship, idAliases map[string]string) (*artifact.Relationship, error) { id := func(id string) string { aliased, ok := idAliases[id] if ok { @@ -178,14 +208,12 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio from, ok := idMap[id(relationship.Parent)].(artifact.Identifiable) if !ok { - log.Warnf("relationship mapping from key %s is not a valid artifact.Identifiable type: %+v", relationship.Parent, idMap[relationship.Parent]) - return nil + return nil, fmt.Errorf("relationship mapping from key %s is not a valid artifact.Identifiable type: %+v", relationship.Parent, idMap[relationship.Parent]) } to, ok := idMap[id(relationship.Child)].(artifact.Identifiable) if !ok { - log.Warnf("relationship mapping to key %s is not a valid artifact.Identifiable type: %+v", relationship.Child, idMap[relationship.Child]) - return nil + return nil, fmt.Errorf("relationship mapping to key %s is not a valid artifact.Identifiable type: %+v", relationship.Child, idMap[relationship.Child]) } typ := artifact.RelationshipType(relationship.Type) @@ -194,8 +222,9 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio case artifact.OwnershipByFileOverlapRelationship, artifact.ContainsRelationship, artifact.DependencyOfRelationship, artifact.EvidentByRelationship: default: if !strings.Contains(string(typ), "dependency-of") { - log.Debugf("unknown relationship type: %s", typ) - return nil + return nil, &unknownRelationshipTypeError{ + relationshipType: string(typ), + } } // lets try to stay as compatible as possible with similar relationship types without dropping the relationship log.Warnf("assuming %q for relationship type %q", artifact.DependencyOfRelationship, typ) @@ -206,7 +235,19 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio To: to, Type: typ, Data: relationship.Metadata, - } + }, nil +} + +type unknownRelationshipTypeError struct { + relationshipType string +} + +func (u *unknownRelationshipTypeError) Error() string { + return fmt.Sprintf("unknown relationship type: %s", u.relationshipType) +} + +func (u *unknownRelationshipTypeError) UnknownType() string { + return u.relationshipType } func toSyftDescriptor(d model.Descriptor) sbom.Descriptor { diff --git a/syft/formats/syftjson/to_syft_model_test.go b/syft/formats/syftjson/to_syft_model_test.go index 6a42d468a42..74f7096d349 100644 --- a/syft/formats/syftjson/to_syft_model_test.go +++ b/syft/formats/syftjson/to_syft_model_test.go @@ -228,3 +228,36 @@ func Test_toSyftFiles(t *testing.T) { }) } } + +func Test_toSyfRelationship(t *testing.T) { + emptyIdMap := make(map[string]interface{}) + emptyAliasMap := make(map[string]string) + type relationships struct { + relationship model.Relationship + want *artifact.Relationship + } + tests := []struct { + name string + idMap map[string]interface{} + idAliases map[string]string + args []relationships + wantUnknownRelTypes map[string]int + }{{ + name: "normal relationship", + idMap: emptyIdMap, + idAliases: emptyAliasMap, + args: []relationships{}, + wantUnknownRelTypes: map[string]int{}, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotRelTypes := make(map[string]int) + for _, rel := range tt.args { + got, _ := toSyftRelationship(tt.idMap, rel.relationship, tt.idAliases) + assert.Equal(t, rel.want, got) + } + assert.Equal(t, tt.wantUnknownRelTypes, gotRelTypes) + }) + } +} From cad77e0f579abec261e44fa1aeae108fa7041032 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Tue, 9 May 2023 16:11:38 -0400 Subject: [PATCH 4/5] add unit tests for warn only once Signed-off-by: Will Murphy --- syft/formats/syftjson/to_syft_model_test.go | 107 +++++++++++++++----- 1 file changed, 83 insertions(+), 24 deletions(-) diff --git a/syft/formats/syftjson/to_syft_model_test.go b/syft/formats/syftjson/to_syft_model_test.go index 74f7096d349..0ac762b9871 100644 --- a/syft/formats/syftjson/to_syft_model_test.go +++ b/syft/formats/syftjson/to_syft_model_test.go @@ -10,6 +10,7 @@ import ( "github.com/anchore/syft/syft/artifact" "github.com/anchore/syft/syft/file" "github.com/anchore/syft/syft/formats/syftjson/model" + "github.com/anchore/syft/syft/pkg" "github.com/anchore/syft/syft/sbom" "github.com/anchore/syft/syft/source" ) @@ -229,35 +230,93 @@ func Test_toSyftFiles(t *testing.T) { } } -func Test_toSyfRelationship(t *testing.T) { - emptyIdMap := make(map[string]interface{}) - emptyAliasMap := make(map[string]string) - type relationships struct { - relationship model.Relationship - want *artifact.Relationship +func Test_toSyfRelationshipsWithWarnings(t *testing.T) { + packageWithId := func(id string) *pkg.Package { + p := &pkg.Package{} + p.OverrideID(artifact.ID(id)) + return p } + childPackage := packageWithId("some-child-id") + parentPackage := packageWithId("some-parent-id") tests := []struct { - name string - idMap map[string]interface{} - idAliases map[string]string - args []relationships - wantUnknownRelTypes map[string]int - }{{ - name: "normal relationship", - idMap: emptyIdMap, - idAliases: emptyAliasMap, - args: []relationships{}, - wantUnknownRelTypes: map[string]int{}, - }} + name string + idMap map[string]interface{} + idAliases map[string]string + relationships []model.Relationship + want []artifact.Relationship + wantWarnings []string + }{ + { + name: "one relationship no warnings", + idMap: map[string]interface{}{ + "some-child-id": childPackage, + "some-parent-id": parentPackage, + }, + idAliases: map[string]string{}, + relationships: []model.Relationship{{ + Parent: "some-parent-id", + Child: "some-child-id", + Type: string(artifact.ContainsRelationship), + }}, + want: []artifact.Relationship{{ + To: childPackage, + From: parentPackage, + Type: artifact.ContainsRelationship, + }}, + }, + { + name: "relationship unknown type one warning", + idMap: map[string]interface{}{ + "some-child-id": childPackage, + "some-parent-id": parentPackage, + }, + idAliases: map[string]string{}, + relationships: []model.Relationship{{ + Parent: "some-parent-id", + Child: "some-child-id", + Type: "some-unknown-relationship-type", + }}, + wantWarnings: []string{ + "Omitted 1 relationship(s) of these unknown types: some-unknown-relationship-type", + }, + }, + { + name: "relationship missing child ID one warning", + idMap: map[string]interface{}{ + "some-parent-id": parentPackage, + }, + idAliases: map[string]string{}, + relationships: []model.Relationship{{ + Parent: "some-parent-id", + Child: "some-child-id", + Type: string(artifact.ContainsRelationship), + }}, + wantWarnings: []string{ + "relationship mapping to key some-child-id is not a valid artifact.Identifiable type: ", + }, + }, + { + name: "relationship missing parent ID one warning", + idMap: map[string]interface{}{ + "some-child-id": childPackage, + }, + idAliases: map[string]string{}, + relationships: []model.Relationship{{ + Parent: "some-parent-id", + Child: "some-child-id", + Type: string(artifact.ContainsRelationship), + }}, + wantWarnings: []string{ + "relationship mapping from key some-parent-id is not a valid artifact.Identifiable type: ", + }, + }, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotRelTypes := make(map[string]int) - for _, rel := range tt.args { - got, _ := toSyftRelationship(tt.idMap, rel.relationship, tt.idAliases) - assert.Equal(t, rel.want, got) - } - assert.Equal(t, tt.wantUnknownRelTypes, gotRelTypes) + got, gotWarnings := toSyftRelationshipsWithWarnings(tt.idMap, tt.relationships, tt.idAliases) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.wantWarnings, gotWarnings) }) } } From deeabd547b9f770a923df9d47ea43d6c8ff5f537 Mon Sep 17 00:00:00 2001 From: Will Murphy Date: Wed, 10 May 2023 05:17:03 -0400 Subject: [PATCH 5/5] Deduplicate conversion errors generically Signed-off-by: Will Murphy --- syft/formats/syftjson/to_syft_model.go | 72 ++++++++------------- syft/formats/syftjson/to_syft_model_test.go | 71 +++++++++++++------- 2 files changed, 76 insertions(+), 67 deletions(-) diff --git a/syft/formats/syftjson/to_syft_model.go b/syft/formats/syftjson/to_syft_model.go index e43be4bd5fd..93bc561d956 100644 --- a/syft/formats/syftjson/to_syft_model.go +++ b/syft/formats/syftjson/to_syft_model.go @@ -3,7 +3,6 @@ package syftjson import ( "fmt" "os" - "sort" "strconv" "strings" @@ -37,10 +36,30 @@ func toSyftModel(doc model.Document) (*sbom.SBOM, error) { }, Source: *toSyftSourceData(doc.Source), Descriptor: toSyftDescriptor(doc.Descriptor), - Relationships: toSyftRelationships(&doc, catalog, doc.ArtifactRelationships, idAliases), + Relationships: warnConversionErrors(toSyftRelationships(&doc, catalog, doc.ArtifactRelationships, idAliases)), }, nil } +func warnConversionErrors[T any](converted []T, errors []error) []T { + errorMessages := deduplicateErrors(errors) + for _, msg := range errorMessages { + log.Warn(msg) + } + return converted +} + +func deduplicateErrors(errors []error) []string { + errorCounts := make(map[string]int) + var errorMessages []string + for _, e := range errors { + errorCounts[e.Error()] = errorCounts[e.Error()] + 1 + } + for msg, count := range errorCounts { + errorMessages = append(errorMessages, fmt.Sprintf("%q occurred %d time(s)", msg, count)) + } + return errorMessages +} + func toSyftFiles(files []model.File) sbom.Artifacts { ret := sbom.Artifacts{ FileMetadata: make(map[source.Coordinates]source.FileMetadata), @@ -133,7 +152,7 @@ func toSyftLinuxRelease(d model.LinuxRelease) *linux.Release { } } -func toSyftRelationships(doc *model.Document, catalog *pkg.Collection, relationships []model.Relationship, idAliases map[string]string) []artifact.Relationship { +func toSyftRelationships(doc *model.Document, catalog *pkg.Collection, relationships []model.Relationship, idAliases map[string]string) ([]artifact.Relationship, []error) { idMap := make(map[string]interface{}) for _, p := range catalog.Sorted() { @@ -151,42 +170,19 @@ func toSyftRelationships(doc *model.Document, catalog *pkg.Collection, relations idMap[f.ID] = f.Location } - out, warnings := toSyftRelationshipsWithWarnings(idMap, relationships, idAliases) - for _, warning := range warnings { - log.Warn(warning) - } - - return out -} - -func toSyftRelationshipsWithWarnings(idMap map[string]interface{}, relationships []model.Relationship, idAliases map[string]string) ([]artifact.Relationship, []string) { var out []artifact.Relationship - var warnings []string - omittedRelationshipTypes := make(map[string]int) - omittedRelatinshipCount := 0 + var conversionErrors []error for _, r := range relationships { syftRelationship, err := toSyftRelationship(idMap, r, idAliases) if err != nil { - if castErr, ok := err.(*unknownRelationshipTypeError); ok { - omittedRelationshipTypes[castErr.UnknownType()] = 1 - omittedRelatinshipCount++ - } else { - warnings = append(warnings, err.Error()) - } + conversionErrors = append(conversionErrors, err) } if syftRelationship != nil { out = append(out, *syftRelationship) } } - if len(omittedRelationshipTypes) > 0 { - keys := make([]string, 0) - for key := range omittedRelationshipTypes { - keys = append(keys, key) - } - sort.Strings(keys) - warnings = append(warnings, fmt.Sprintf("Omitted %d relationship(s) of these unknown types: %v", omittedRelatinshipCount, strings.Join(keys, ", "))) - } - return out, warnings + + return out, conversionErrors } func toSyftSource(s model.Source) *source.Source { @@ -222,9 +218,7 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio case artifact.OwnershipByFileOverlapRelationship, artifact.ContainsRelationship, artifact.DependencyOfRelationship, artifact.EvidentByRelationship: default: if !strings.Contains(string(typ), "dependency-of") { - return nil, &unknownRelationshipTypeError{ - relationshipType: string(typ), - } + return nil, fmt.Errorf("unknown relationship type: %s", string(typ)) } // lets try to stay as compatible as possible with similar relationship types without dropping the relationship log.Warnf("assuming %q for relationship type %q", artifact.DependencyOfRelationship, typ) @@ -238,18 +232,6 @@ func toSyftRelationship(idMap map[string]interface{}, relationship model.Relatio }, nil } -type unknownRelationshipTypeError struct { - relationshipType string -} - -func (u *unknownRelationshipTypeError) Error() string { - return fmt.Sprintf("unknown relationship type: %s", u.relationshipType) -} - -func (u *unknownRelationshipTypeError) UnknownType() string { - return u.relationshipType -} - func toSyftDescriptor(d model.Descriptor) sbom.Descriptor { return sbom.Descriptor{ Name: d.Name, diff --git a/syft/formats/syftjson/to_syft_model_test.go b/syft/formats/syftjson/to_syft_model_test.go index 0ac762b9871..de96667f14c 100644 --- a/syft/formats/syftjson/to_syft_model_test.go +++ b/syft/formats/syftjson/to_syft_model_test.go @@ -1,6 +1,7 @@ package syftjson import ( + "errors" "testing" "github.com/scylladb/go-set/strset" @@ -230,7 +231,7 @@ func Test_toSyftFiles(t *testing.T) { } } -func Test_toSyfRelationshipsWithWarnings(t *testing.T) { +func Test_toSyfRelationship(t *testing.T) { packageWithId := func(id string) *pkg.Package { p := &pkg.Package{} p.OverrideID(artifact.ID(id)) @@ -242,9 +243,9 @@ func Test_toSyfRelationshipsWithWarnings(t *testing.T) { name string idMap map[string]interface{} idAliases map[string]string - relationships []model.Relationship - want []artifact.Relationship - wantWarnings []string + relationships model.Relationship + want *artifact.Relationship + wantError error }{ { name: "one relationship no warnings", @@ -253,16 +254,16 @@ func Test_toSyfRelationshipsWithWarnings(t *testing.T) { "some-parent-id": parentPackage, }, idAliases: map[string]string{}, - relationships: []model.Relationship{{ + relationships: model.Relationship{ Parent: "some-parent-id", Child: "some-child-id", Type: string(artifact.ContainsRelationship), - }}, - want: []artifact.Relationship{{ + }, + want: &artifact.Relationship{ To: childPackage, From: parentPackage, Type: artifact.ContainsRelationship, - }}, + }, }, { name: "relationship unknown type one warning", @@ -271,14 +272,14 @@ func Test_toSyfRelationshipsWithWarnings(t *testing.T) { "some-parent-id": parentPackage, }, idAliases: map[string]string{}, - relationships: []model.Relationship{{ + relationships: model.Relationship{ Parent: "some-parent-id", Child: "some-child-id", Type: "some-unknown-relationship-type", - }}, - wantWarnings: []string{ - "Omitted 1 relationship(s) of these unknown types: some-unknown-relationship-type", }, + wantError: errors.New( + "unknown relationship type: some-unknown-relationship-type", + ), }, { name: "relationship missing child ID one warning", @@ -286,14 +287,14 @@ func Test_toSyfRelationshipsWithWarnings(t *testing.T) { "some-parent-id": parentPackage, }, idAliases: map[string]string{}, - relationships: []model.Relationship{{ + relationships: model.Relationship{ Parent: "some-parent-id", Child: "some-child-id", Type: string(artifact.ContainsRelationship), - }}, - wantWarnings: []string{ - "relationship mapping to key some-child-id is not a valid artifact.Identifiable type: ", }, + wantError: errors.New( + "relationship mapping to key some-child-id is not a valid artifact.Identifiable type: ", + ), }, { name: "relationship missing parent ID one warning", @@ -301,22 +302,48 @@ func Test_toSyfRelationshipsWithWarnings(t *testing.T) { "some-child-id": childPackage, }, idAliases: map[string]string{}, - relationships: []model.Relationship{{ + relationships: model.Relationship{ Parent: "some-parent-id", Child: "some-child-id", Type: string(artifact.ContainsRelationship), - }}, - wantWarnings: []string{ - "relationship mapping from key some-parent-id is not a valid artifact.Identifiable type: ", }, + wantError: errors.New("relationship mapping from key some-parent-id is not a valid artifact.Identifiable type: "), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, gotWarnings := toSyftRelationshipsWithWarnings(tt.idMap, tt.relationships, tt.idAliases) + got, gotErr := toSyftRelationship(tt.idMap, tt.relationships, tt.idAliases) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.wantError, gotErr) + }) + } +} + +func Test_deduplicateErrors(t *testing.T) { + tests := []struct { + name string + errors []error + want []string + }{ + { + name: "no errors, nil slice", + }, + { + name: "deduplicates errors", + errors: []error{ + errors.New("some error"), + errors.New("some error"), + }, + want: []string{ + `"some error" occurred 2 time(s)`, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := deduplicateErrors(tt.errors) assert.Equal(t, tt.want, got) - assert.Equal(t, tt.wantWarnings, gotWarnings) }) } }