DependencyTrack / dependency-track

Dependency-Track is an intelligent Component Analysis platform that allows organizations to identify and reduce risk in the software supply chain.
https://dependencytrack.org/
Apache License 2.0
2.72k stars 580 forks source link

metrics ossindex-api - failed with retry / without retry is always 0 #3999

Open tsimas opened 4 months ago

tsimas commented 4 months ago

Current Behavior

In OssIndexAnalysisTask.java - submit method the implementation swallow http exception so configs on RETRY is never used and from this reason any http call is successful

try (final CloseableHttpResponse response = RETRY.executeCheckedSupplier(() -> HttpClientPool.getClient().execute(request))) { HttpEntity responseEntity = response.getEntity(); String responseString = EntityUtils.toString(responseEntity); if (response.getStatusLine().getStatusCode() == HttpStatus.SC_OK) { final OssIndexParser parser = new OssIndexParser(); return parser.parse(responseString); } else { handleUnexpectedHttpResponse(LOGGER, apiBaseUrl, response.getStatusLine().getStatusCode(), response.getStatusLine().getReasonPhrase()); // you should throw Exception in handle method } }

Steps to Reproduce

  1. Use invalid ApiKey, do a BOmUpload

Expected Behavior

Metrics show failed ossindex call

Dependency-Track Version

4.11.5

Dependency-Track Distribution

Container Image

Database Server

PostgreSQL

Database Server Version

No response

Browser

Google Chrome

Checklist

nscuro commented 4 months ago

The retry behavior is configured here: https://github.com/DependencyTrack/dependency-track/blob/76eaad1ac6f8b5dbb5d55a0439b57512a5de5802/src/main/java/org/dependencytrack/tasks/scanners/OssIndexAnalysisTask.java#L102-L113

Note withTransientErrorCode which checks if the response code is any of the following:

https://github.com/DependencyTrack/dependency-track/blob/76eaad1ac6f8b5dbb5d55a0439b57512a5de5802/src/main/java/org/dependencytrack/util/RetryUtil.java#L54-L59

and withTransientCause, which checks if the thrown exception is any of the following:

https://github.com/DependencyTrack/dependency-track/blob/76eaad1ac6f8b5dbb5d55a0439b57512a5de5802/src/main/java/org/dependencytrack/util/RetryUtil.java#L49-L53

The circumstances that can trigger a retry are chosen conservatively on purpose. In fact, in your case, a retry should not be performed. An Unauthorized or Forbidden response due to invalid API token is not transient, and won't resolve itself. It needs manual intervention (you providing a new token), hence no retry is performed.

There is a test that demonstrates retries working as expected here: https://github.com/DependencyTrack/dependency-track/blob/76eaad1ac6f8b5dbb5d55a0439b57512a5de5802/src/test/java/org/dependencytrack/tasks/scanners/OssIndexAnalysisTaskTest.java#L96-L190

valentijnscholten commented 4 months ago

Are the metrics for those failures (401/403) also working? That seems the main reason this issue was created.

tsimas commented 4 months ago

thx @nscuro for yourr response. you are right, at authn/authz error doesn't make sense to retry the request, but in that case - i think - failed_without_retry counter could be increase and not the successful_without_retry. I would like alert if http call fails based on metrics and currently authn/authz exceptions are hidden.

nscuro commented 4 months ago

Thanks for clarifying! I misunderstood the original post. You are absolutely correct in your observation.