Allow configuring timeout for external sources #1812

pouyan021 · 2024-04-18T09:31:49Z

This pull request closes #1624. It adds and enforces the ability to set a new property abort-after to external sources. As discussed in the issue, it supports both a global prop and a maven property that overrides the global if it is set.

README.md

spiffcs · 2024-04-22T17:35:34Z

@pouyan021 Approved and running checks now - Thank you so much for the contribution!

spiffcs · 2024-04-22T17:36:29Z

@pouyan021 looks like there is a small change needed where go mod tidy is run

pouyan021 · 2024-04-22T17:49:12Z

@spiffcs Appreciate your support, thanks a lot! The problem with go.mod is addressed but I forgot to sign-off my commit! I fixed that and pushed again. Could you kindly approve the checks once more?

spiffcs

Everything else LGTM - Just waiting for @wagoodman and his final say on the config direction he wants to go here

spiffcs · 2024-04-25T18:31:48Z

README.md

 base-url: https://search.maven.org/solrsearch/select
+ abort-after: 5m #override the global config


cc @wagoodman - I know he's pretty sensitive to duplicate fields that override each other so I'd like him to chime in on where he sees this going or what his preference would be

stale review and waiting for IC input

wagoodman

Overall the functionality looks good, but have some comments on testing and configuration. I'll push up some changes shortly to help out.

I do think that RequestTimeout is a better name for this config item -- what do you think @pouyan021 ?

wagoodman · 2024-05-16T13:27:58Z

cmd/grype/cli/options/datasources.go

- BaseURL string `yaml:"base-url" json:"baseUrl" mapstructure:"base-url"`
+ SearchUpstreamBySha1 bool `yaml:"search-upstream" json:"searchUpstreamBySha1" mapstructure:"search-maven-upstream"`
+ BaseURL string `yaml:"base-url" json:"baseUrl" mapstructure:"base-url"`
+ AbortAfter *string `yaml:"abort-after" json:"abortAfter" mapstructure:"abort-after"`


Using time.Duration natively here should work, and also allows for invalid values to be a concern of application configuration processing, which is already done and is much earlier in execution, thus will error earlier upon a misconfiguration:

GRYPE_EXTERNAL_SOURCES_ABORT_AFTER=blurb grype alpine:latest invalid application config: 1 error(s) decoding: * error decoding 'external-sources.abort-after': time: invalid duration "blurb" exit status 1

FYI, no action needed, I made the adjustment to use the time.Duration in the application configuration.

wagoodman · 2024-05-16T13:29:25Z

grype/matcher/java/matcher_mocks_test.go

- return &m.pkg, nil
+func (m mockMavenSearcher) GetMavenPackageBySha(ctx context.Context, sha1 string) (*pkg.Package, error) {
+ deadline, ok := ctx.Deadline()
+ fmt.Println("GetMavenPackageBySha called with deadline:", deadline, "deadline set:", ok)


test should not have a print, but if output is helpful this should use t.Log instead

FYI, no action needed, I made the mock to take testing.TB for logging.

wagoodman · 2024-05-16T13:59:02Z

grype/matcher/java/matcher_mocks_test.go

+ fmt.Println("GetMavenPackageBySha called with deadline:", deadline, "deadline set:", ok)
+ // Sleep for a duration longer than the context's deadline
+ select {
+ case <-time.After(10 * time.Second):


this will inadvertently cause existing tests (TestMatcherJava_matchUpstreamMavenPackage) to take 10 seconds. This mock could probably be refactored to account for this, where there is a configurable work duration and error behavior.

FYI, no action needed, I made the adjustment to change the mock to not require sleeping in problematic cases.

wagoodman · 2024-05-16T14:20:45Z

README.md

@@ -278,9 +278,11 @@ feature is currently disabled by default. To enable this feature add the followi
 ```yaml
 external-sources:
 enable: true
+ abort-after: 10m


I think we can clarify what this means by changing the name some. This could be interpreted as either:
a. aborting looking up from external sources in general after the duration elapses
b. aborting a single request to an external source after the duration elapses

From the functionality implemented b is implied.

Regarding naming and the above context, request-timeout feels like a more descriptive name.

wagoodman · 2024-05-16T15:04:01Z

cmd/grype/cli/options/datasources.go

 func defaultExternalSources() externalSources {
 return externalSources{
+ AbortAfter: defaultAbortAfter,
 Maven: maven{
 SearchUpstreamBySha1: true,
 BaseURL: defaultMavenBaseURL,


We want the following quality to multi-teir config items like this:

a user specifying top-level fields overrides UNspecified nested fields

a user specifying top-level fields does NOT override specified nested fields

a user specifying nested fields does NOT top-level fields

Where:

specified: a user explicitly provided a value

unspecified: a user did NOT explicitly provide a value. This might mean no value should be set or a default value should be set instead.

The implementation does follow this, however, the rendered configuration unintentionally conveys that there is no maven abort-after value:

# running: grype -vv ... external-sources: abort-after: 10m maven: abort-after: null

Here from a user perspective it appears as if an override has been set at the top level, but has not been inherited. This is because we're conflating inheriting default values vs overriding nested values.

What we really want is to convey that the user did not specify overriding values in top-level fields, and that the individual sources have sane defaults:

# running: grype -vv ... external-sources: abort-after: null maven: abort-after: 10m0s

This has the added benefit that different sources can have different default values (since the default value for nested properties is not inheriting a top-level property).

We've dealt with multi-level properties in syft, see the pretty fields for an example: https://github.com/anchore/syft/blob/v1.4.1/cmd/syft/internal/options/format.go . We end up making both fields optional to show what is specified vs unspecified, then reason about the final values in a PostLoad function, which is invoked while rendering the configuration.

Specifying an override then appears in both places correctly:

# run: GRYPE_EXTERNAL_SOURCES_ABORT_AFTER=40m grype -vv external-sources: abort-after: 40m0s maven: abort-after: 40m0s

Instead of no value being set:

# run: GRYPE_EXTERNAL_SOURCES_ABORT_AFTER=40m grype -vv external-sources: abort-after: 40m maven: abort-after: null

FYI, no action needed, I made the adjustment to use the PostLoad in a follow up commit.

Signed-off-by: Pouyan Khodabakhsh <[email protected]>

… functionality Signed-off-by: Pouyan Khodabakhsh <[email protected]>

…-after functionality Signed-off-by: Pouyan Khodabakhsh <[email protected]>

Signed-off-by: Pouyan Khodabakhsh <[email protected]>

Signed-off-by: Alex Goodman <[email protected]>

wagoodman · 2024-05-16T15:30:05Z

note: I force pushed to get this branch rebased onto the latest commit on main

pouyan021 · 2024-05-17T03:31:22Z

Overall the functionality looks good, but have some comments on testing and configuration. I'll push up some changes shortly to help out.

I do think that RequestTimeout is a better name for this config item -- what do you think @pouyan021 ?

Hey @wagoodman thanks a lot for the thorough review and your additional changes. The name was chosen as abort-after based on the suggestion by one of the contributors here. I agree with you that RequestTimeout is more self-explanatory.

spiffcs previously approved these changes Apr 22, 2024

View reviewed changes

README.md Show resolved Hide resolved

pouyan021 force-pushed the feat/allow-configuring-timeout-for-external-sources branch 2 times, most recently from 5cc6f4b to e5ed88d Compare April 22, 2024 17:46

pouyan021 requested a review from spiffcs April 25, 2024 06:19

spiffcs reviewed Apr 25, 2024

View reviewed changes

spiffcs self-requested a review April 25, 2024 18:34

wagoodman changed the title ~~Feat/allow configuring timeout for external sources~~ Allow configuring timeout for external sources May 16, 2024

wagoodman reviewed May 16, 2024

View reviewed changes

pouyan021 and others added 7 commits May 16, 2024 11:27

adding abort-after property to external-sources

4cbb4f0

Signed-off-by: Pouyan Khodabakhsh <[email protected]>

considering the timeout in java matcher

b8fe080

Signed-off-by: Pouyan Khodabakhsh <[email protected]>

make the maven search method respect the context time out

49ae4c9

Signed-off-by: Pouyan Khodabakhsh <[email protected]>

adding a number of additional tests for the external-sources time-out…

7043535

… functionality Signed-off-by: Pouyan Khodabakhsh <[email protected]>

facilitating java matcher mocks to imitate the external-sources abort…

db426d2

…-after functionality Signed-off-by: Pouyan Khodabakhsh <[email protected]>

updating readme to reflect the new changes in external-sources

1e947b6

Signed-off-by: Pouyan Khodabakhsh <[email protected]>

adjust multi level configuration + faster tests

0bf64dd

Signed-off-by: Alex Goodman <[email protected]>

wagoodman force-pushed the feat/allow-configuring-timeout-for-external-sources branch from ca5d50c to 0bf64dd Compare May 16, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring timeout for external sources #1812

Allow configuring timeout for external sources #1812

pouyan021 commented Apr 18, 2024 •

edited

spiffcs commented Apr 22, 2024

spiffcs commented Apr 22, 2024

pouyan021 commented Apr 22, 2024 •

edited

spiffcs left a comment

spiffcs Apr 25, 2024

wagoodman left a comment

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman May 16, 2024

wagoodman commented May 16, 2024

pouyan021 commented May 17, 2024 •

edited

		base-url: https://search.maven.org/solrsearch/select
		abort-after: 5m #override the global config

Allow configuring timeout for external sources #1812

Are you sure you want to change the base?

Allow configuring timeout for external sources #1812

Conversation

pouyan021 commented Apr 18, 2024 • edited

spiffcs commented Apr 22, 2024

spiffcs commented Apr 22, 2024

pouyan021 commented Apr 22, 2024 • edited

spiffcs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagoodman 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

wagoodman commented May 16, 2024

pouyan021 commented May 17, 2024 • edited

pouyan021 commented Apr 18, 2024 •

edited

pouyan021 commented Apr 22, 2024 •

edited

pouyan021 commented May 17, 2024 •

edited