Made the Timeout in LicenseInformationService configurable via CLI ar…

…gument (#584) (#773) * Made the Timeout in LicenseInformationService configurable via CLI argument (#584) * Added recommended fixes for PR #773 - Renamed variables to specify Seconds - Added new CLI arg -lto to docs - Added support for negative values for -lto - Added tests for limits of -lto - Fixed breaking API change by creating new Interfaces - Changed some magic numbers to constants instead - Added some Warning statements * Added additional recommended fixes for PR #773 - Fixed a couple of minor bugs - Reduced valid input range for -lto to 1-86400 - Moved MaxTimeout and DefaultTimeout to Constants class --------- Co-authored-by: Dave Tryon <[email protected]>
microsoft · Dec 9, 2024 · 4d0ce83 · 4d0ce83
1 parent 970a9e0
commit 4d0ce83
Show file tree

Hide file tree

Showing 15 changed files with 180 additions and 6 deletions.
diff --git a/docs/sbom-tool-arguments.md b/docs/sbom-tool-arguments.md
@@ -66,6 +66,9 @@ Actions
     DeleteManifestDirIfPresent (-D)           If set to true, we will delete any previous manifest directories that are already present in the ManifestDirPath without asking the user for confirmation. The new
                                               manifest directory will then be created at this location and the generated SBOM will be stored there.
     FetchLicenseInformation (-li)             If set to true, we will attempt to fetch license information of packages detected in the SBOM from the ClearlyDefinedApi.
+    LicenseInformationTimeoutInSeconds (-lto) Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if
+                                              FetchLicenseInformation (-li) argument is false or not provided. Valid values are from 1 to 86400. Negative values use the default
+                                              value and Values exceeding the maximum are truncated to the maximum. 
     EnablePackageMetadataParsing (-pm)        If set to true, we will attempt to parse license and supplier info from the packages metadata file (RubyGems, NuGet, Maven, Npm).
     Verbosity (-V)                            Display this amount of detail in the logging output.
                                               Verbose

diff --git a/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs b/src/Microsoft.Sbom.Api/Config/Args/GenerationArgs.cs
@@ -116,6 +116,16 @@ public class GenerationArgs : GenerationAndValidationCommonArgs
     [ArgDescription("If set to true, we will attempt to fetch license information of packages detected in the SBOM from the ClearlyDefinedApi.")]
     public bool? FetchLicenseInformation { get; set; }
 
+    /// <summary>
+    /// Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. Has no effect if
+    /// FetchLicenseInformation (li) argument is false or not provided. A negative value corresponds to an infinite timeout.
+    /// </summary>
+    [ArgShortcut("lto")]
+    [ArgDescription("Specifies the timeout in seconds for fetching the license information. Defaults to 30 seconds. " +
+        "Has no effect if the FetchLicenseInformation (li) argument is false or not provided. A negative value corresponds" +
+        "to an infinite timeout.")]
+    public int? LicenseInformationTimeoutInSeconds { get; set; }
+
     /// <summary>
     /// If set to true, we will attempt to parse license and supplier info from the packages metadata file.
     /// </summary>

diff --git a/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs b/src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs
@@ -6,6 +6,7 @@
 using System.IO;
 using System.Linq;
 using System.Reflection;
+using Microsoft.Sbom.Api.Executors;
 using Microsoft.Sbom.Api.Hashing;
 using Microsoft.Sbom.Api.Utils;
 using Microsoft.Sbom.Common;
@@ -81,6 +82,30 @@ public IConfiguration SanitizeConfig(IConfiguration configuration)
         // Set default package supplier if not provided in configuration.
         configuration.PackageSupplier = GetPackageSupplierFromAssembly(configuration, logger);
 
+        // Prevent null value for LicenseInformationTimeoutInSeconds.
+        // Values of (0, Constants.MaxLicenseFetchTimeoutInSeconds] are allowed. Negative values are replaced with the default, and
+        // the higher values are truncated to the maximum of Common.Constants.MaxLicenseFetchTimeoutInSeconds
+        if (configuration.LicenseInformationTimeoutInSeconds is null)
+        {
+            configuration.LicenseInformationTimeoutInSeconds = new(Common.Constants.DefaultLicenseFetchTimeoutInSeconds, SettingSource.Default);
+        }
+        else if (configuration.LicenseInformationTimeoutInSeconds.Value <= 0)
+        {
+            logger.Warning($"Negative and Zero Values not allowed for timeout. Using the default {Common.Constants.DefaultLicenseFetchTimeoutInSeconds} seconds instead.");
+            configuration.LicenseInformationTimeoutInSeconds.Value = Common.Constants.DefaultLicenseFetchTimeoutInSeconds;
+        }
+        else if (configuration.LicenseInformationTimeoutInSeconds.Value > Common.Constants.MaxLicenseFetchTimeoutInSeconds)
+        {
+            logger.Warning($"Specified timeout exceeds maximum allowed. Truncating the timeout to {Common.Constants.MaxLicenseFetchTimeoutInSeconds} seconds.");
+            configuration.LicenseInformationTimeoutInSeconds.Value = Common.Constants.MaxLicenseFetchTimeoutInSeconds;
+        }
+
+        // Check if arg -lto is specified but -li is not
+        if (configuration.FetchLicenseInformation?.Value != true && !configuration.LicenseInformationTimeoutInSeconds.IsDefaultSource)
+        {
+            logger.Warning("A license fetching timeout is specified (argument -lto), but this has no effect when FetchLicenseInfo is unspecified or false (argument -li)");
+        }
+
         // Replace backslashes in directory paths with the OS-sepcific directory separator character.
         PathUtils.ConvertToOSSpecificPathSeparators(configuration);
 

diff --git a/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs b/src/Microsoft.Sbom.Api/Executors/ComponentDetectionBaseWalker.cs
@@ -137,7 +137,21 @@ async Task Scan(string path)
                 {
                     licenseInformationRetrieved = true;
 
-                    var apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi);
+                    List<string> apiResponses;
+                    var licenseInformationFetcher2 = licenseInformationFetcher as ILicenseInformationFetcher2;
+                    if (licenseInformationFetcher2 is null && (bool)!configuration.LicenseInformationTimeoutInSeconds?.IsDefaultSource)
+                    {
+                        log.Warning("Timeout value is specified, but ILicenseInformationFetcher2 is not implemented for the licenseInformationFetcher");
+                    }
+
+                    if (licenseInformationFetcher2 is null || configuration.LicenseInformationTimeoutInSeconds is null)
+                    {
+                        apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi);
+                    }
+                    else
+                    {
+                        apiResponses = await licenseInformationFetcher2.FetchLicenseInformationAsync(listOfComponentsForApi, configuration.LicenseInformationTimeoutInSeconds.Value);
+                    }
 
                     foreach (var response in apiResponses)
                     {

diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher.cs
@@ -18,7 +18,7 @@ public interface ILicenseInformationFetcher
     List<string> ConvertComponentsToListForApi(IEnumerable<ScannedComponent> scannedComponents);
 
     /// <summary>
-    /// Calls the ClearlyDefined API to get the license information for the list of components.
+    /// Calls the ClearlyDefined API to get the license information for the list of components. Uses a default timeout specified in implementation
     /// </summary>
     /// <param name="listOfComponentsForApi"> A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API.</param>
     /// <returns></returns>

diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher2.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationFetcher2.cs
@@ -0,0 +1,20 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Threading.Tasks;
+using Microsoft.ComponentDetection.Contracts.BcdeModels;
+
+namespace Microsoft.Sbom.Api.Executors;
+
+public interface ILicenseInformationFetcher2: ILicenseInformationFetcher
+{
+    /// <summary>
+    /// Calls the ClearlyDefined API to get the license information for the list of components.
+    /// </summary>
+    /// <param name="listOfComponentsForApi"> A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API.</param>
+    /// <param name="timeoutInSeconds">Timeout in seconds to use when making web requests</param>
+    /// <returns></returns>
+    Task<List<string>> FetchLicenseInformationAsync(List<string> listOfComponentsForApi, int timeoutInSeconds);
+}
diff --git a/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService2.cs b/src/Microsoft.Sbom.Api/Executors/ILicenseInformationService2.cs
@@ -0,0 +1,12 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Collections.Generic;
+using System.Threading.Tasks;
+
+namespace Microsoft.Sbom.Api.Executors;
+
+public interface ILicenseInformationService2 : ILicenseInformationService
+{
+    public Task<List<string>> FetchLicenseInformationFromAPI(List<string> listOfComponentsForApi, int timeoutInSeconds);
+}
diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationFetcher.cs
@@ -14,7 +14,7 @@
 
 namespace Microsoft.Sbom.Api.Executors;
 
-public class LicenseInformationFetcher : ILicenseInformationFetcher
+public class LicenseInformationFetcher : ILicenseInformationFetcher2
 {
     private readonly ILogger log;
     private readonly IRecorder recorder;
@@ -87,6 +87,20 @@ public async Task<List<string>> FetchLicenseInformationAsync(List<string> listOf
         return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi);
     }
 
+    public async Task<List<string>> FetchLicenseInformationAsync(List<string> listOfComponentsForApi, int timeoutInSeconds)
+    {
+        var licenseInformationService2 = licenseInformationService as ILicenseInformationService2;
+        if (licenseInformationService2 is null)
+        {
+            log.Warning("Timeout is specified in License Fetcher, but licenseInformationService does not implement ILicenseInformationService2");
+            return await licenseInformationService.FetchLicenseInformationFromAPI(listOfComponentsForApi);
+        }
+        else
+        {
+            return await licenseInformationService2.FetchLicenseInformationFromAPI(listOfComponentsForApi, timeoutInSeconds);
+        }
+    }
+
     // Will attempt to extract license information from a clearlyDefined batch API response. Will always return a dictionary which may be empty depending on the response.
     public Dictionary<string, string> ConvertClearlyDefinedApiResponseToList(string httpResponseContent)
     {

diff --git a/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs b/src/Microsoft.Sbom.Api/Executors/LicenseInformationService.cs
@@ -15,12 +15,11 @@
 
 namespace Microsoft.Sbom.Api.Executors;
 
-public class LicenseInformationService : ILicenseInformationService
+public class LicenseInformationService : ILicenseInformationService2
 {
     private readonly ILogger log;
     private readonly IRecorder recorder;
     private readonly HttpClient httpClient;
-    private const int ClientTimeoutSeconds = 30;
 
     public LicenseInformationService(ILogger log, IRecorder recorder, HttpClient httpClient)
     {
@@ -30,6 +29,11 @@ public LicenseInformationService(ILogger log, IRecorder recorder, HttpClient htt
     }
 
     public async Task<List<string>> FetchLicenseInformationFromAPI(List<string> listOfComponentsForApi)
+    {
+        return await FetchLicenseInformationFromAPI(listOfComponentsForApi, Common.Constants.MaxLicenseFetchTimeoutInSeconds);
+    }
+
+    public async Task<List<string>> FetchLicenseInformationFromAPI(List<string> listOfComponentsForApi, int timeoutInSeconds)
     {
         var batchSize = 500;
         var responses = new List<HttpResponseMessage>();
@@ -38,7 +42,10 @@ public async Task<List<string>> FetchLicenseInformationFromAPI(List<string> list
         var uri = new Uri("https://api.clearlydefined.io/definitions?expand=-files");
 
         httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
-        httpClient.Timeout = TimeSpan.FromSeconds(ClientTimeoutSeconds);
+        if (timeoutInSeconds > 0)
+        {
+            httpClient.Timeout = TimeSpan.FromSeconds(timeoutInSeconds);
+        } // The else cases should be sanitized in Config Sanitizer, and even if not, it'll just use httpClient's default timeout
 
         for (var i = 0; i < listOfComponentsForApi.Count; i += batchSize)
         {

diff --git a/src/Microsoft.Sbom.Common/Config/Configuration.cs b/src/Microsoft.Sbom.Common/Config/Configuration.cs
@@ -47,6 +47,7 @@ public class Configuration : IConfiguration
     private static readonly AsyncLocal<ConfigurationSetting<string>> generationTimestamp = new();
     private static readonly AsyncLocal<ConfigurationSetting<bool>> followSymlinks = new();
     private static readonly AsyncLocal<ConfigurationSetting<bool>> fetchLicenseInformation = new();
+    private static readonly AsyncLocal<ConfigurationSetting<int>> licenseInformationTimeout = new();
     private static readonly AsyncLocal<ConfigurationSetting<bool>> enablePackageMetadataParsing = new();
     private static readonly AsyncLocal<ConfigurationSetting<bool>> deleteManifestDirIfPresent = new();
     private static readonly AsyncLocal<ConfigurationSetting<bool>> failIfNoPackages = new();
@@ -309,6 +310,14 @@ public ConfigurationSetting<bool> FetchLicenseInformation
         set => fetchLicenseInformation.Value = value;
     }
 
+    /// <inheritdoc cref="IConfiguration.LicenseInformationTimeoutInSeconds" />
+    [DefaultValue(Constants.DefaultLicenseFetchTimeoutInSeconds)]
+    public ConfigurationSetting<int> LicenseInformationTimeoutInSeconds
+    {
+        get => licenseInformationTimeout.Value;
+        set => licenseInformationTimeout.Value = value;
+    }
+
     /// <inheritdoc cref="IConfiguration.EnablePackageMetadataParsing" />
     [DefaultValue(false)]
     public ConfigurationSetting<bool> EnablePackageMetadataParsing

diff --git a/src/Microsoft.Sbom.Common/Config/IConfiguration.cs b/src/Microsoft.Sbom.Common/Config/IConfiguration.cs
@@ -194,6 +194,13 @@ public interface IConfiguration
     /// </summary>
     ConfigurationSetting<bool> FetchLicenseInformation { get; set; }
 
+    /// <summary>
+    /// Specifies the timeout in seconds for fetching the license information. Defaults to <see cref="Constants.DefaultLicenseFetchTimeoutInSeconds"/>.
+    /// Has no effect if FetchLicenseInformation (li) argument is false or not provided. Negative values are set to the default and values exceeding the
+    /// maximum are truncated to <see cref="Constants.MaxLicenseFetchTimeoutInSeconds"/>
+    /// </summary>
+    ConfigurationSetting<int> LicenseInformationTimeoutInSeconds { get; set; }
+
     /// <summary>
     /// If set to true, we will attempt to locate and parse package metadata files for additional information to include in the SBOM such as .nuspec/.pom files in the local package cache.
     /// </summary>

diff --git a/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs b/src/Microsoft.Sbom.Common/Config/InputConfiguration.cs
@@ -141,6 +141,10 @@ public class InputConfiguration : IConfiguration
     [DefaultValue(false)]
     public ConfigurationSetting<bool> FetchLicenseInformation { get; set; }
 
+    /// <inheritdoc cref="IConfiguration.LicenseInformationTimeoutInSeconds" />
+    [DefaultValue(Constants.DefaultLicenseFetchTimeoutInSeconds)]
+    public ConfigurationSetting<int> LicenseInformationTimeoutInSeconds { get; set; }
+
     [DefaultValue(false)]
     public ConfigurationSetting<bool> EnablePackageMetadataParsing { get; set; }
 

diff --git a/src/Microsoft.Sbom.Common/Constants.cs b/src/Microsoft.Sbom.Common/Constants.cs
@@ -13,5 +13,8 @@ public static class Constants
     public const int DefaultParallelism = 8;
     public const int MaxParallelism = 48;
 
+    public const int DefaultLicenseFetchTimeoutInSeconds = 30;
+    public const int MaxLicenseFetchTimeoutInSeconds = 86400;
+
     public const LogEventLevel DefaultLogLevel = LogEventLevel.Warning;
 }
diff --git a/src/Microsoft.Sbom.Targets/README.md b/src/Microsoft.Sbom.Targets/README.md
@@ -36,6 +36,7 @@ The custom MSBuild task accepts most of the arguments available for the [SBOM CL
 | `<SbomGenerationNamespaceUriUniquePart>`            | N/A | No |
 | `<SbomGenerationExternalDocumentReferenceListFile>` | N/A | No |
 | `<SbomGenerationFetchLicenseInformation>`           | `false` | No |
+| `<SbomGenerationLicenseInformationTimeoutInSeconds>`| `30` | No |
 | `<SbomGenerationEnablePackageMetadataParsing>`      | `false` | No |
 | `<SbomGenerationVerbosity>`                         | `Information` | No |
 | `<SbomGenerationManifestInfo>`                      | `SPDX:2.2` | No |

diff --git a/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs b/test/Microsoft.Sbom.Api.Tests/Config/ConfigSanitizerTests.cs
@@ -9,6 +9,7 @@
 using System.Security.Cryptography;
 using Microsoft.Sbom.Api.Config;
 using Microsoft.Sbom.Api.Exceptions;
+using Microsoft.Sbom.Api.Executors;
 using Microsoft.Sbom.Api.Hashing;
 using Microsoft.Sbom.Api.Utils;
 using Microsoft.Sbom.Common;
@@ -351,4 +352,48 @@ public void ConfigSantizer_Validate_ReplacesBackslashes_Linux(ManifestToolAction
             Assert.IsTrue(config.TelemetryFilePath.Value.StartsWith($"/{nameof(config.TelemetryFilePath)}/", StringComparison.Ordinal));
         }
     }
+
+    [TestMethod]
+    [DataRow(1, DisplayName = "Minimum value of 1")]
+    [DataRow(Common.Constants.MaxLicenseFetchTimeoutInSeconds, DisplayName = "Maximum Value of 86400")]
+    public void LicenseInformationTimeoutInSeconds_SanitizeMakesNoChanges(int value)
+    {
+        var config = GetConfigurationBaseObject();
+        config.LicenseInformationTimeoutInSeconds = new(value, SettingSource.CommandLine);
+
+        configSanitizer.SanitizeConfig(config);
+
+        Assert.AreEqual(value, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should remain the same through the sanitization process");
+    }
+
+    [TestMethod]
+    [DataRow(int.MinValue, Common.Constants.DefaultLicenseFetchTimeoutInSeconds, DisplayName = "Negative Value is changed to Default")]
+    [DataRow(0, Common.Constants.DefaultLicenseFetchTimeoutInSeconds, DisplayName = "Zero is changed to Default")]
+    [DataRow(Common.Constants.MaxLicenseFetchTimeoutInSeconds + 1, Common.Constants.MaxLicenseFetchTimeoutInSeconds, DisplayName = "Max Value + 1 is truncated")]
+    [DataRow(int.MaxValue, Common.Constants.MaxLicenseFetchTimeoutInSeconds, DisplayName = "int.MaxValue is truncated")]
+    public void LicenseInformationTimeoutInSeconds_SanitizeExceedsLimits(int value, int expected)
+    {
+        var config = GetConfigurationBaseObject();
+        config.LicenseInformationTimeoutInSeconds = new(value, SettingSource.CommandLine);
+
+        configSanitizer.SanitizeConfig(config);
+
+        Assert.AreEqual(expected, config.LicenseInformationTimeoutInSeconds.Value, "The value of LicenseInformationTimeoutInSeconds should be sanitized to a valid value");
+    }
+
+    [TestMethod]
+    public void LicenseInformationTimeoutInSeconds_SanitizeNull()
+    {
+        var config = GetConfigurationBaseObject();
+        config.LicenseInformationTimeoutInSeconds = null;
+
+        configSanitizer.SanitizeConfig(config);
+
+        Assert.AreEqual(
+            Common.Constants.DefaultLicenseFetchTimeoutInSeconds,
+            config.LicenseInformationTimeoutInSeconds.Value,
+            $"The value of LicenseInformationTimeoutInSeconds should be set to {Common.Constants.DefaultLicenseFetchTimeoutInSeconds}s when null");
+
+        Assert.AreEqual(SettingSource.Default, config.LicenseInformationTimeoutInSeconds.Source, "The source of LicenseInformationTimeoutInSeconds should be set to Default when null");
+    }
 }