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 <arorasoham9@gmail.com>

…th the comparing path, not sure which to use Signed-off-by: Soham Arora <arorasoham9@gmail.com>

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

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.

Signed-off-by: arorasoham9 <68672198+arorasoham9@users.noreply.github.com>

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 <arorasoham9@gmail.com>

fixing mod sum

0b56fe0

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

lint fixes

62f2fa5

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

lint fixes

a36daed

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

mod fixes

9689179

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix cli name

0bc691b

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

changes to work with test cases

d7f7503

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

adding tests and more fixes in analyze.go to support

728dfd7

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fix for highlight analysis

096894c

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

changes requested

9a0c88e

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

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 <arorasoham9@gmail.com>

moving print functions to new file, fixing lint

067803b

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

moving print functions to new file, fixing lint

df6d68a

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

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

2505185

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

removing hardcoded values

c8f6392

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixing flag names

c04e02c

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixed repetitive path difference detection

dd2ebb7

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

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 <arorasoham9@gmail.com>

diffpatchmatch output for guacdiff

5bc9ed3

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

removing edges from nodes

24b2ac5

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

Merge branch 'guacsec:main' into guacdiff

b217cec

print fixes and mod

a01cd3e

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

fixing output to show diffpatchmatch

4092bce

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

print format

d7f2658

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

lint

bd1f6fa

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

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

lint

e334803

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

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 <arorasoham9@gmail.com>

implementing termui

81c653c

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

changing tablewriter

f718aa8

Signed-off-by: Soham Arora <arorasoham9@gmail.com>

arorasoham9 added 6 commits November 17, 2024 19:38

Merge branch 'main' into guacdiff

17d5cf4

some more term ui fixes

08986ef

Signed-off-by: arorasoham9 <68672198+arorasoham9@users.noreply.github.com>

fixing multi line output, line wrapping and interactive ui

446e327

Signed-off-by: arorasoham9 <68672198+arorasoham9@users.noreply.github.com>

Merge branch 'guacsec:main' into guacdiff

d90d0ec

clean up

cf714a3

Signed-off-by: arorasoham9 <68672198+arorasoham9@users.noreply.github.com>

Merge branch 'main' into guacdiff

d9f3e5f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Analyze command to guacone #1809

Adding Analyze command to guacone #1809

arorasoham9 commented Apr 4, 2024 •

edited

Loading

arorasoham9 commented Apr 4, 2024

pxp928 commented Apr 4, 2024

pxp928 Apr 4, 2024

arorasoham9 Apr 4, 2024

arorasoham9 Apr 5, 2024

arorasoham9 commented Apr 4, 2024 •

edited

Loading

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

pxp928 Apr 5, 2024

jeffmendoza left a comment

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

jeffmendoza Apr 5, 2024

pxp928 commented Apr 18, 2024

lumjjb commented Oct 28, 2024


		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

lumjjb commented Oct 28, 2024

arorasoham9 commented Apr 4, 2024 •

edited

Loading

arorasoham9 commented Apr 4, 2024 •

edited

Loading