Included REST Spec Changes #2217

nathannaveen · 2024-10-25T15:21:14Z

Description of the PR

Second Part of Search REST via Purl #2089
All the OpenAPI Spec changes have been included
The /query/dependencies is no longer going to be used, instead, the /v0/package/{purl}/dependencies and /v0/artifact/{digest}/dependencies will be replacing it.
The code in retrieveDependencies.go has been updated to work for the new endpoints.
The error handling code has also been updated.

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 GraphQL schema is changed, GraphQL client updates/additions have been made
If OpenAPI spec is changed, make generate has been run
If ent schema 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

* Second Part of guacsec#2089 * All the OpenAPI Spec changes have been included * The `/query/dependencies` is no longer going to be used, instead, the `/v0/package/{purl}/dependencies` and `/v0/artifact/{digest}/dependencies` will be replacing it. * The code in retrieveDependencies.go has been updated to work for the new endpoints. Signed-off-by: nathannaveen <[email protected]>

nathannaveen · 2024-10-25T15:44:49Z

👀 @pxp928 @mdeicas @lumjjb

mdeicas

This is a great change, thanks @nathannaveen! Thank you as well for splitting the original PR up and adding documentation to the API spec. I added comments but overall it looks good.

mdeicas · 2024-10-29T11:55:24Z

pkg/guacrest/openapi.yaml

+        If a partial purl, pkg:foo/bar is passed in, then it would find all purls 
+        associated with the given purl, such as pkg:foo/[email protected], and pkg:foo/[email protected].


nit: how are qualifiers handled? For example, will searching for pkg:foo/bar@a=b return both pkg:foo/bar@a=b and pkg:foo/bar@c=d&a=b?

mdeicas · 2024-10-29T12:01:52Z

pkg/guacrest/openapi.yaml

+    get:
+      summary: Get dependencies for a specific Package URL (purl)
+      description: >
+        If a partial purl is passed in, then it will find all associated purls 


I don't think "associated purls" count as a dependency.

mdeicas · 2024-10-29T13:00:18Z

pkg/guacrest/openapi.yaml

    get:
-      summary: Identify the most important dependencies
-      operationId: analyzeDependencies
+      summary: Get vulnerabilities for a specific digest


nit: I would phrase it as Get vulnerabilities for an artifact, identified by a digest (also in /v0/artifact/{digest}/dependencies)

mdeicas · 2024-10-29T13:00:20Z

pkg/guacrest/openapi.yaml

+    get:
+      summary: Get dependencies for a specific Package URL (purl)
+      description: >
+        If a partial purl is passed in, then it will find all associated purls 


IMO these endpoints should operate on a single purl/package that is defined in Guac, instead of handling partial purls. Doing so hides information from the user, and it's not clear if what is returned is directly related to the input package or if it originates from a similar package. Supporting partial purls increases complexity, compounds the inaccuracy of finding the dependencies of a package (https://blog.deps.dev/zillions-of-sboms/), and may lead to drift between the different endpoints.

If a user wants to retrieve the dependencies/vulns of a partial purl, they can just compose /v0/package/{purl} and the dependencies endpoint or /v0/package/{purl} and the vuln endpoint.

mdeicas · 2024-10-29T13:03:10Z

pkg/guacrest/openapi.yaml

+    get:
+      summary: Get vulnerabilities for a Package URL (purl)
+      description: >
+        If a partial purl is passed in, then it will find all associated purls 


See the other comment on using partial vs precise purls in these endpoints.

mdeicas · 2024-10-29T13:39:07Z

pkg/guacrest/server/retrieveDependencies.go

-			BadRequestJSONResponse: gen.BadRequestJSONResponse{
-				Message: "Neither a purl or a digest argument was provided",
-			}}, nil
+	unescapedPurl, err := url.QueryUnescape(purl)


Thanks for noticing this, it was missing before! It should be PathUnescape though.

mdeicas · 2024-10-29T13:47:43Z

pkg/guacrest/server/retrieveDependencies_test.go

+			encodedPurl := url.QueryEscape(tt.purl)
+			encodedArtifact := url.QueryEscape(tt.digest)


See the other comments on escaping, but this shouldn't be needed here.

mdeicas · 2024-10-29T13:48:21Z

pkg/guacrest/server/retrieveDependencies_test.go

-	}
-}
-
-func Test_ClientErrors(t *testing.T) {


Any reason that these tests were removed?

mdeicas · 2024-10-29T13:51:32Z

pkg/guacrest/server/retrieveDependencies_test.go

-		// Only specify Purl or Digest. The test will set linkCondition, because both are tested
-		input api.RetrieveDependenciesParams
+		// Only specify a Purl or a Digest. But, both purls and digests are tested
+		purl   string
+		digest string


Now that the package and artifact functions are separate, I think it makes more sense to split this test suite into one for each.

mdeicas · 2024-10-29T13:51:56Z

pkg/guacrest/server/retrieveDependencies_test.go

-				if !cmp.Equal(v.PurlList, tt.expectedByName, cmpopts.EquateEmpty(), cmpopts.SortSlices(stdcmp.Less[string])) {
-					t.Errorf("RetrieveDependencies with byName returned %v, but wanted %v", v.PurlList, tt.expectedByName)
+			if tt.purl != "" {
+				/******** call the endpoint with byName link condition ********/


nit: this comment isn't relevant anymore

pull-request-size bot added the size/XL label Oct 25, 2024

nathannaveen mentioned this pull request Oct 25, 2024

Search REST via Purl #2089

Closed

9 tasks

nathannaveen force-pushed the nathan/updateApiSpec branch from 0c02fd0 to 0577c13 Compare October 25, 2024 15:43

lumjjb added the needs-review Needs writer LGTM label Oct 28, 2024

lumjjb self-requested a review October 28, 2024 15:08

mdeicas requested changes Oct 29, 2024

View reviewed changes

mdeicas mentioned this pull request Oct 29, 2024

Documentation of RestAPI integration from v0.4.0 - v0.6.0 guacsec/guac-docs#164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Included REST Spec Changes #2217

Included REST Spec Changes #2217

nathannaveen commented Oct 25, 2024

nathannaveen commented Oct 25, 2024

mdeicas left a comment

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

mdeicas Oct 29, 2024

		If a partial purl, pkg:foo/bar is passed in, then it would find all purls
		associated with the given purl, such as pkg:foo/[email protected], and pkg:foo/[email protected].

		encodedPurl := url.QueryEscape(tt.purl)
		encodedArtifact := url.QueryEscape(tt.digest)

Included REST Spec Changes #2217

Are you sure you want to change the base?

Included REST Spec Changes #2217

Conversation

nathannaveen commented Oct 25, 2024

Description of the PR

PR Checklist

nathannaveen commented Oct 25, 2024

mdeicas 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