Adding Analyze command to guacone #1809

arorasoham9 · 2024-04-04T08:54:55Z

Description of the PR

Analyze command for guacone
Comparing SBOMs with only patch version differences
guacone analyze diff --uri --sboms=https://anchore.com/syft/image/k8s.gcr.io/kube-apiserver-v1.24.1-583a02ce-8f7e-4794-91af-35f27ffeb73d,https://anchore.com/syft/image/k8s.gcr.io/kube-apiserver-v1.24.2-ee7e0a81-87de-4761-9689-4f7162d81e44

The diff is a 4 step process:

Create graph for SBOMs
Find all paths in both graphs
Find the paths that are present in one graph and not in the other, do this for both graphs.
Pair paths from one graph to the other that are closest, then find the difference between these pairs.

Each step has tests to ensure that it is stable. Meaning, if the steps are repeated for the same Input SBOM, the results are the same. The JSON you see are the test SBOMS I use to carry out the tests for each step of the diff. These JSON files are marshalled HasSBOM nodes for SBOMs I ingested into GUAC, pulled using the Node function.

PR Checklist

All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
All new changes are covered by tests
If GraphQL schema is changed, make generate has been run
If OpenAPI spec is changed, make generate has been run
If collectsub protobuf has been changed, make proto has been run
All CI checks are passing (tests and formatting)
All dependent PRs have already been merged

arorasoham9 · 2024-04-04T18:16:00Z

@pxp928 ready for review.

pxp928 · 2024-04-04T18:19:45Z

Cool, @arorasoham9 can you add some screenshots to what the output will look like for the CLI?

pxp928 · 2024-04-04T18:21:58Z

pkg/analyzer/analyze.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package guacanalyze


one major task would be to add unit tests to this package. Some things like the graph, getting data from graphQL can be abstracted away but do you have some tests you can add here to test the functionality?

I don't have any at the moment. I can write a few. Could they come in a future PR?

Added unit tests as requested.

arorasoham9 · 2024-04-04T19:00:47Z

Cool, @arorasoham9 can you add some screenshots to what the output will look like for the CLI?

Added some screenshots. The Prettify function errored out at many instances so I improvised to print the same details that function does but taken from the graph DS itself, not graphQL.

pxp928 · 2024-04-05T19:37:26Z

pkg/analyzer/analyze.go

+    node *Node
+  )
+  if node, err = g.Vertex(ID); err !=  nil {
+    fmt.Println("Error setting node attribute", err)


need to return this as an error.

pxp928 · 2024-04-05T19:37:52Z

pkg/analyzer/analyze.go

+  )
+  if node, err = g.Vertex(ID); err !=  nil {
+    fmt.Println("Error setting node attribute", err)
+    os.Exit(1)


don't use os.Exit(1) should return error

pxp928 · 2024-04-05T19:38:36Z

pkg/analyzer/analyze.go

+
+func GetNodeAttribute(g graph.Graph[string, *Node],ID, key string) interface{} {
+  var (
+    err error


no need to instantiate this here, can just it on line 72?

pxp928 · 2024-04-05T19:39:10Z

pkg/analyzer/analyze.go

+  node.Attributes[key] = value
+}
+
+func GetNodeAttribute(g graph.Graph[string, *Node],ID, key string) interface{} {


why is this returning an interface{}

pxp928 · 2024-04-05T19:41:10Z

pkg/analyzer/analyze.go

+    foundHasSBOMPkg, err = model.HasSBOMs(ctx, gqlclient, model.HasSBOMSpec{Subject: &model.PackageOrArtifactSpec{Package: &model.PkgSpec{Id: &pkgResponse.Packages[0].Namespaces[0].Names[0].Versions[0].Id}},
+      })
+    if err != nil {
+      fmt.Printf("(purl)failed getting hasSBOM with error :%v", err)


dont use fmt.Printf, you should return an error via fmt.Errorf

pxp928 · 2024-04-05T19:41:32Z

pkg/analyzer/analyze.go

+func verifyAnalyzeFlags(slsas, sboms []string,  errSlsa, errSbom error, uri, purl, id bool) {
+
+  if (errSlsa != nil && errSbom != nil) || (len(slsas) ==0  && len(sboms) == 0 ){
+    fmt.Println("Must specify slsa or sboms ")


return a proper error and dont use os.Exit

pxp928 · 2024-04-05T19:43:01Z

pkg/analyzer/analyze.go

+      namespaceTwo, okTwo := GetNodeAttribute(analysisGraph,diffList.MissingAddedRemovedLinks[i][1], "Namespace[0]").(string)
+
+      if !okOne || !okTwo {
+        fmt.Println("Error getting node namespace attribute")


clean up and pass out proper error

pxp928 · 2024-04-05T19:44:39Z

pkg/analyzer/analyze.go

+  table.SetAlignment(tablewriter.ALIGN_LEFT)
+  table.Render()
+  if (!all && max > maxprint){
+    fmt.Println("Run with --all to see full list")


should user logger here: https://github.com/guacsec/guac/blob/main/pkg/handler/processor/process/process.go#L117

It should be able to be pulled from the context:

logger := logging.FromContext(ctx)

pxp928 · 2024-04-05T19:45:42Z

pkg/analyzer/analyze.go

+}
+
+func HasSBOMToGraph(cmd *cobra.Command, ctx context.Context, gqlclient graphql.Client) ( []graph.Graph[string, *Node]){
+  slsas, errSlsa := cmd.Flags().GetStringSlice("slsa")


pass in options or something similar and not the actual cmd. See: https://github.com/guacsec/guac/blob/main/cmd/guacone/cmd/s3.go#L80-L93

pxp928 · 2024-04-05T19:46:02Z

pkg/analyzer/analyze.go

+  if uri {
+    hasSBOMResponseOne, err = FindHasSBOMBy(model.HasSBOMSpec{} ,sboms[0],"", "", ctx, gqlclient)
+    if err != nil {
+      fmt.Println("(uri)failed to lookup sbom:", sboms[0], err)


pxp928 · 2024-04-05T19:49:23Z

pkg/analyzer/analyze_test.go

@@ -0,0 +1,66 @@
+package guacanalyze_test


missing header

jeffmendoza

Mostly code organization and naming comments.

jeffmendoza · 2024-04-05T19:37:18Z

cmd/guacone/cmd/analyze.go

+  "github.com/guacsec/guac/pkg/logging"
+  "github.com/spf13/cobra"
+  "github.com/spf13/viper"
+  analysis "github.com/guacsec/guac/pkg/analyzer"


Any reason for the import rename here?

jeffmendoza · 2024-04-05T19:38:11Z

cmd/guacone/cmd/analyze.go

+    if diff && intersect && union || diff && intersect || diff && union || intersect && union {
+      fmt.Println("Must specify only one of --diff, --intersect, --union")
+      return
+    }


This should be a positional argument if required, not a flag/option.

jeffmendoza · 2024-04-05T19:39:10Z

cmd/guacone/cmd/analyze.go

+    }
+
+    //create graphs
+    graphs := analysis.HasSBOMToGraph(cmd, ctx, gqlclient)


Don't pass cmd to a package under pkg/. Those shouldn't have a dependency on cli commands or cobra. Explicitly pass the parameters this needs.

jeffmendoza · 2024-04-05T19:40:52Z

cmd/guacone/cmd/analyze.go

+
+func init() {
+
+  rootCmd.PersistentFlags().Bool("intersect", false, "compute intesection of given sboms")


Don't add flags to rootCmd, this will show up on all guacone cmds if so. Use this analyzeCmd you are creating.
Also, cli options are normally under pkg/cli/store.go but this command has so many it might make sense to just leave them here.

jeffmendoza · 2024-04-05T19:41:19Z

cmd/guacone/cmd/analyze.go

+  rootCmd.PersistentFlags().Bool("purl", false, "input is a pURL")
+  rootCmd.PersistentFlags().Bool("id", false, "input is an Id")
+  rootCmd.PersistentFlags().Bool("metadata", false, "Compare SBOM metadata")
+  rootCmd.PersistentFlags().Bool("inclSoft", false, "Compare Included Softwares")


change to incl-soft and similar for others. All cli options use hyphen-case.

jeffmendoza · 2024-04-05T19:42:31Z

cmd/guacone/cmd/analyze.go

+  rootCmd.PersistentFlags().Int("maxprint", PRINT_MAX, "max number of similar sboms to print")
+  rootCmd.AddCommand(analyzeCmd)
+
+}


Add a newline to the end of the file

jeffmendoza · 2024-04-05T19:44:29Z

pkg/analyzer/analyze.go

+  )
+  if node, err = g.Vertex(ID); err !=  nil {
+    fmt.Println("Error setting node attribute", err)
+    os.Exit(1)


Shouldn't have any os.Exit in a package under pkg/, assume this is a library that can be called from anywhere. Propagate errors back to the caller.

jeffmendoza · 2024-04-05T19:45:36Z

pkg/analyzer/analyze.go

+  return foundHasSBOMPkg, nil
+}
+
+func verifyAnalyzeFlags(slsas, sboms []string,  errSlsa, errSbom error, uri, purl, id bool) {


All this should be under the file in cmd/, shouldn't have any flag validation here in pkg/.

jeffmendoza · 2024-04-05T19:46:15Z

pkg/analyzer/analyze.go

+  //print to stdout
+  printHighlightedAnalysis(dot, diffList, all, maxprint, action, analysisGraph )
+}
+func max(nums []int) int {


add an empty line between functions.

jeffmendoza · 2024-04-05T19:50:01Z

pkg/analyzer/analyze.go

+  //Create dot file
+  createGraphDotFile(dot, analysisGraph)
+  //print to stdout
+  printHighlightedAnalysis(dot, diffList, all, maxprint, action, analysisGraph )


Anything under pkg/ should not print anything to stdout. Any logic should result in a datastructure that returns back to cmd/ and is then printed. This helps for testability. Looks like much of this may need to be moved back to cmd/

pxp928 · 2024-04-18T12:06:56Z

@arorasoham9 did the node functionality get fixed?

Signed-off-by: Soham Arora <[email protected]>

pxp928 · 2024-05-24T16:02:11Z

@arorasoham9 any luck in improving the output? It is very hard to understand. Like if you can show less information and just show only what is relevant, that would be better.

arorasoham9 · 2024-05-24T16:25:06Z

@pxp928 I am still thinking of ways to show only the relevant information. This is a better explanation for why you see so much even with just a small difference between two sboms. The diff works at the granularity of a path, not a node, so when it finds even one node different it classifies all paths with that participating node as different (and prints them). I believe the original aim was to find missing/different paths bw two sboms was to also offer a graph "structural" difference as well. I am trying to think how we can still offer the same differences in an easier to understand manner.

lumjjb

Thanks @arorasoham9 for working on this big feature!! definitely going to be very useful, would you be able to help break this up into a few separate PRs, that would be very helpful in getting this merged in faster!

maybe start with the analyzer package, and then add a table formatter pkg, and then the CLI options and the CLI itself?

lumjjb · 2024-05-24T20:37:20Z

cmd/guacone/cmd/analyze.go

+		uri, _ := cmd.Flags().GetBool("uri")
+		purl, _ := cmd.Flags().GetBool("purl")
+
+		metadata, _ := cmd.Flags().GetBool("metadata")


these flags should be initialized in https://github.com/guacsec/guac/blob/main/pkg/cli/store.go, and preferably prefixed in a way that shows its for guacanalyze

lumjjb · 2024-05-24T20:38:14Z

cmd/guacone/cmd/analyze.go

+
+		if test == "" {
+			if err = verifyAnalyzeFlags(slsas, sboms, errSlsa, errSbom, uri, purl, id); err != nil {
+				fmt.Fprintf(os.Stderr, "Error: %s", err)


nit: golang errors style guide is lowercase first characters.

lumjjb · 2024-05-24T20:39:02Z

cmd/guacone/cmd/analyze.go

+
+			//create graphs
+			graphs, err = hasSBOMToGraph(ctx, gqlclient, sboms, AnalyzeOpts{
+				Metadata: metadata, InclSoft: inclSoft, InclDeps: inclDeps, InclOccur: inclOccur, Namespaces: namespaces, URI: uri, PURL: purl, ID: id})


make it into different lines for readability.

lumjjb · 2024-05-24T20:39:51Z

cmd/guacone/cmd/analyze.go

+				os.Exit(1)
+			}
+
+			//create graphs


nit: the comment doesn't add much context to the code, either remove or expand.

lumjjb · 2024-05-24T20:40:33Z

cmd/guacone/cmd/analyze.go

+			}
+		}
+
+		if args[0] == "diff" {


would it make sense for these to be subcommands? otherwise this should be a flag maybe?

lumjjb · 2024-05-24T20:52:26Z

pkg/analyzer/analyzer.go

+	return n.ID
+}
+
+func SetNodeAttribute(g graph.Graph[string, *Node], ID, key string, value interface{}) bool {


can you provide additional context and documentation around the functions? it is not clear what should be in value, unless there's a good reason, i would highly recommend not having interface{} as the type of value. If anything it can be a hidden type that can be minted by the analyzer but must provide sufficient constructors to make sense of it.

lumjjb · 2024-05-24T20:52:45Z

pkg/analyzer/analyzer.go

+
+	switch action {
+	//0 is diff
+	case 0:


use an enum :)

lumjjb · 2024-05-24T20:55:13Z

pkg/analyzer/analyzer.go

+	return hex.EncodeToString(hash[:])
+}
+
+func AddGraphNode(g graph.Graph[string, *Node], _ID, color string) {


what is color used for? Is it the identity of an edge of just for analysis purpose? can it be passed it as an option on the node for extensibility?

_ID should be id to follow golang style?

lumjjb · 2024-05-24T20:56:11Z

cmd/guacone/cmd/analyze.go

+	},
+}
+
+func printDiffedPathTable(diffs []analyzer.DiffedPath) error {


would be helpful to move all printing stuff to either a different package or a different file

lumjjb · 2024-05-24T20:56:43Z

cmd/guacone/cmd/analyze.go

+	return nil
+}
+
+func readTwoSBOM(filename string) ([]graph.Graph[string, *analyzer.Node], error) {


why read two sboms from a file? i don't think that's a common pattern?

Signed-off-by: Soham Arora <[email protected]>

…th the comparing path, not sure which to use Signed-off-by: Soham Arora <[email protected]>

Signed-off-by: Soham Arora <[email protected]>

lumjjb · 2024-10-28T15:11:32Z

Hey @arorasoham9 , would you be able to chat at bit more amount helping to turn the corner to complete the PR.

arorasoham9 requested a review from jeffmendoza as a code owner April 4, 2024 08:54

pull-request-size bot added the size/XL label Apr 4, 2024

arorasoham9 force-pushed the guacdiff branch from 383ff7d to 853c095 Compare April 4, 2024 17:59

pxp928 reviewed Apr 4, 2024

View reviewed changes

pxp928 reviewed Apr 5, 2024

View reviewed changes

pkg/analyzer/analyze_test.go Outdated

@@ -0,0 +1,66 @@

package guacanalyze_test

Copy link

Collaborator

pxp928 Apr 5, 2024

Choose a reason for hiding this comment

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

missing header

jeffmendoza reviewed Apr 5, 2024

View reviewed changes

pull-request-size bot added size/XXL size/XL and removed size/XL size/XXL labels Apr 10, 2024

arorasoham9 force-pushed the guacdiff branch from ccb7778 to 93255cc Compare April 29, 2024 14:28

pull-request-size bot added size/XXL and removed size/XL labels Apr 29, 2024

arorasoham9 force-pushed the guacdiff branch from 63a3593 to 3ac17a0 Compare May 17, 2024 05:21

arorasoham9 requested a review from mihaimaruseac as a code owner May 17, 2024 05:21

arorasoham9 added 10 commits May 17, 2024 11:02

adding files

70a37cd

Signed-off-by: Soham Arora <[email protected]>

fixing mod sum

0b56fe0

Signed-off-by: Soham Arora <[email protected]>

lint fixes

62f2fa5

Signed-off-by: Soham Arora <[email protected]>

lint fixes

a36daed

Signed-off-by: Soham Arora <[email protected]>

mod fixes

9689179

Signed-off-by: Soham Arora <[email protected]>

fix cli name

0bc691b

Signed-off-by: Soham Arora <[email protected]>

changes to work with test cases

d7f7503

Signed-off-by: Soham Arora <[email protected]>

adding tests and more fixes in analyze.go to support

728dfd7

Signed-off-by: Soham Arora <[email protected]>

fix for highlight analysis

096894c

Signed-off-by: Soham Arora <[email protected]>

changes requested

9a0c88e

Signed-off-by: Soham Arora <[email protected]>

Merge branch 'guacsec:main' into guacdiff

7682412

lumjjb requested changes May 24, 2024

View reviewed changes

arorasoham9 and others added 2 commits May 31, 2024 03:49

Merge branch 'guacsec:main' into guacdiff

57c1b2c

changes requested

f4b5590

Signed-off-by: Soham Arora <[email protected]>

arorasoham9 force-pushed the guacdiff branch from 9dbac7e to f4b5590 Compare June 24, 2024 11:16

arorasoham9 and others added 15 commits July 5, 2024 14:47

moving flag init

167f6f1

Signed-off-by: Soham Arora <[email protected]>

moving print functions to new file, fixing lint

067803b

Signed-off-by: Soham Arora <[email protected]>

moving print functions to new file, fixing lint

df6d68a

Signed-off-by: Soham Arora <[email protected]>

moving print functions to new file, removing interface{} type in Node

2505185

Signed-off-by: Soham Arora <[email protected]>

removing hardcoded values

c8f6392

Signed-off-by: Soham Arora <[email protected]>

fixing flag names

c04e02c

Signed-off-by: Soham Arora <[email protected]>

fixed repetitive path difference detection

dd2ebb7

Signed-off-by: Soham Arora <[email protected]>

added TODO for bug where multiple paths may have equal differences wi…

5795cfc

…th the comparing path, not sure which to use Signed-off-by: Soham Arora <[email protected]>

diffpatchmatch output for guacdiff

5bc9ed3

Signed-off-by: Soham Arora <[email protected]>

removing edges from nodes

24b2ac5

Signed-off-by: Soham Arora <[email protected]>

Merge branch 'guacsec:main' into guacdiff

b217cec

print fixes and mod

a01cd3e

Signed-off-by: Soham Arora <[email protected]>

fixing output to show diffpatchmatch

4092bce

Signed-off-by: Soham Arora <[email protected]>

print format

d7f2658

Signed-off-by: Soham Arora <[email protected]>

lint

bd1f6fa

Signed-off-by: Soham Arora <[email protected]>

arorasoham9 force-pushed the guacdiff branch from a719d30 to bd1f6fa Compare August 15, 2024 23:23

lint

e334803

Signed-off-by: Soham Arora <[email protected]>

arorasoham9 force-pushed the guacdiff branch from 547537c to e334803 Compare August 15, 2024 23:31

arorasoham9 and others added 4 commits August 16, 2024 05:02

Merge branch 'main' into guacdiff

b85e676

mod

1bbb1b1

Signed-off-by: Soham Arora <[email protected]>

implementing termui

81c653c

Signed-off-by: Soham Arora <[email protected]>

changing tablewriter

f718aa8

Signed-off-by: Soham Arora <[email protected]>


		func init() {

		rootCmd.PersistentFlags().Bool("intersect", false, "compute intesection of given sboms")

Adding Analyze command to guacone #1809

Are you sure you want to change the base?

Adding Analyze command to guacone #1809

Conversation

arorasoham9 commented Apr 4, 2024 • edited Loading

Description of the PR

PR Checklist

arorasoham9 commented Apr 4, 2024

pxp928 commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arorasoham9 commented Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffmendoza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxp928 commented Apr 18, 2024

pxp928 commented May 24, 2024 • edited Loading

arorasoham9 commented May 24, 2024 • edited Loading

lumjjb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lumjjb commented Oct 28, 2024

arorasoham9 commented Apr 4, 2024 •

edited

Loading

arorasoham9 commented Apr 4, 2024 •

edited

Loading

pxp928 commented May 24, 2024 •

edited

Loading

arorasoham9 commented May 24, 2024 •

edited

Loading