Various fixes

- Improved log messages (and don't log multiple times), fix exception stack traces
- Don't remote-fetch twice at startup (load + timer that immediately fired)
- Moved options bind and remote-fetch from constructor to Load, fix combination with placeholder

Author: bart-vmware

src/Configuration/src/Abstractions/CompositeConfigurationProvider.cs

@@ -53,10 +53,9 @@ private void Load(bool isReload)
 
     public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath)
     {
+        ArgumentNullException.ThrowIfNull(earlierKeys);
+
         string[] earlierKeysArray = earlierKeys as string[] ?? earlierKeys.ToArray();
-#pragma warning disable S3236 // Caller information arguments should not be provided explicitly
-        ArgumentNullException.ThrowIfNull(earlierKeysArray, nameof(earlierKeys));
-#pragma warning restore S3236 // Caller information arguments should not be provided explicitly
 
         if (_logger.IsEnabled(LogLevel.Trace))
         {

src/Configuration/src/ConfigServer/ConfigServerClientOptions.cs

@@ -133,7 +133,7 @@ public bool ValidateCertificatesAlt
     public int TokenTtl { get; set; } = 300_000;
 
     /// <summary>
-    /// Gets or sets the vault token renew rate (in milliseconds). Default value: 60_000 (1 minute).
+    /// Gets or sets the Vault token renew rate (in milliseconds). Default value: 60_000 (1 minute).
     /// </summary>
     public int TokenRenewRate { get; set; } = 60_000;
 

src/Configuration/src/ConfigServer/ConfigServerConfigurationBuilderExtensions.cs

@@ -133,10 +133,7 @@ internal static IConfigurationBuilder AddConfigServer(this IConfigurationBuilder
             builder.AddCloudFoundry();
             builder.AddKubernetesServiceBindings();
 
-            ConfigServerConfigurationSource source = builder is IConfiguration configuration
-                ? new ConfigServerConfigurationSource(options, configuration, configure, createHttpClientHandler, loggerFactory)
-                : new ConfigServerConfigurationSource(options, builder.Sources, builder.Properties, configure, createHttpClientHandler, loggerFactory);
-
+            var source = new ConfigServerConfigurationSource(options, builder.Sources, builder.Properties, configure, createHttpClientHandler, loggerFactory);
             builder.Add(source);
         }
 

src/Configuration/src/ConfigServer/ConfigServerConfigurationProvider.cs

@@ -12,8 +12,8 @@ internal sealed class ConfigServerConfigurationSource : IConfigurationSource
 {
     private readonly ILoggerFactory _loggerFactory;
 
-    internal List<IConfigurationSource> Sources { get; } = [];
-    internal Dictionary<string, object> Properties { get; } = [];
+    internal List<IConfigurationSource> Sources { get; }
+    internal Dictionary<string, object> Properties { get; }
 
     /// <summary>
     /// Gets the initial options the client uses to contact Config Server.
@@ -36,39 +36,6 @@ internal sealed class ConfigServerConfigurationSource : IConfigurationSource
     /// </summary>
     public Func<HttpClientHandler>? CreateHttpClientHandler { get; }
 
-    /// <summary>
-    /// Initializes a new instance of the <see cref="ConfigServerConfigurationSource" /> class.
-    /// </summary>
-    /// <param name="defaultOptions">
-    /// The initial options the client uses to contact Config Server.
-    /// </param>
-    /// <param name="configuration">
-    /// The configuration the client uses to contact Config Server. Entries overrule <paramref name="defaultOptions" />.
-    /// </param>
-    /// <param name="configure">
-    /// An optional delegate that further configures options from code, after settings from <paramref name="configuration" /> have been applied.
-    /// </param>
-    /// <param name="createHttpClientHandler">
-    /// An optional factory to create the HTTP client handler, used to mock HTTP requests to Config Server in tests. When provided, the caller is responsible
-    /// for handler disposal.
-    /// </param>
-    /// <param name="loggerFactory">
-    /// Used for internal logging. Pass <see cref="NullLoggerFactory.Instance" /> to disable logging.
-    /// </param>
-    public ConfigServerConfigurationSource(ConfigServerClientOptions defaultOptions, IConfiguration configuration, Action<ConfigServerClientOptions>? configure,
-        Func<HttpClientHandler>? createHttpClientHandler, ILoggerFactory loggerFactory)
-    {
-        ArgumentNullException.ThrowIfNull(defaultOptions);
-        ArgumentNullException.ThrowIfNull(configuration);
-        ArgumentNullException.ThrowIfNull(loggerFactory);
-
-        DefaultOptions = defaultOptions;
-        Configure = configure;
-        CreateHttpClientHandler = createHttpClientHandler;
-        Configuration = configuration;
-        _loggerFactory = loggerFactory;
-    }
-
     /// <summary>
     /// Initializes a new instance of the <see cref="ConfigServerConfigurationSource" /> class.
     /// </summary>
@@ -102,11 +69,7 @@ public ConfigServerConfigurationSource(ConfigServerClientOptions defaultOptions,
         ArgumentNullException.ThrowIfNull(loggerFactory);
 
         Sources = sources.ToList();
-
-        if (properties != null)
-        {
-            Properties = new Dictionary<string, object>(properties);
-        }
+        Properties = properties != null ? new Dictionary<string, object>(properties) : [];
 
         DefaultOptions = defaultOptions;
         Configure = configure;
@@ -125,26 +88,19 @@ public ConfigServerConfigurationSource(ConfigServerClientOptions defaultOptions,
     /// </returns>
     public IConfigurationProvider Build(IConfigurationBuilder builder)
     {
-        if (Configuration == null)
-        {
-            // Create our own builder to build sources
-            var configurationBuilder = new ConfigurationBuilder();
+        var configurationBuilder = new ConfigurationBuilder();
 
-            foreach (IConfigurationSource source in Sources)
-            {
-                configurationBuilder.Add(source);
-            }
-
-            // Use properties provided
-            foreach (KeyValuePair<string, object> pair in Properties)
-            {
-                configurationBuilder.Properties.Add(pair);
-            }
+        foreach (IConfigurationSource source in Sources)
+        {
+            configurationBuilder.Add(source);
+        }
 
-            // Create configuration
-            Configuration = configurationBuilder.Build();
+        foreach (KeyValuePair<string, object> pair in Properties)
+        {
+            configurationBuilder.Properties.Add(pair);
         }
 
+        Configuration = configurationBuilder.Build();
         return new ConfigServerConfigurationProvider(this, _loggerFactory);
     }
 }

src/Configuration/src/ConfigServer/ConfigServerConfigurationSource.cs

@@ -76,10 +76,10 @@ internal void UpdateHealth(HealthCheckResult health, IList<PropertySource> sourc
 
         foreach (PropertySource source in sources)
         {
-            LogReturningPropertySource(source.Name);
             names.Add(source.Name);
         }
 
+        LogReturningPropertySources(string.Join(", ", names));
         health.Details.Add("propertySources", names);
     }
 
@@ -92,7 +92,17 @@ internal void UpdateHealth(HealthCheckResult health, IList<PropertySource> sourc
         {
             LastAccess = currentTime;
             LogCacheStale();
-            Cached = await provider.LoadInternalAsync(optionsSnapshot, false, cancellationToken);
+
+            try
+            {
+                Cached = await provider.LoadInternalAsync(optionsSnapshot, false, cancellationToken);
+            }
+            catch (ConfigServerException exception)
+            {
+                LogFetchFailed(exception);
+                Cached = null;
+                return null;
+            }
         }
 
         return Cached?.PropertySources;
@@ -114,14 +124,17 @@ internal bool IsCacheStale(long accessTime, ConfigServerClientOptions optionsSna
     [LoggerMessage(Level = LogLevel.Debug, Message = "No Config Server provider found.")]
     private partial void LogNoProviderFound();
 
+    [LoggerMessage(Level = LogLevel.Debug, Message = "Failed fetching remote configuration from server(s).")]
+    private partial void LogFetchFailed(Exception exception);
+
     [LoggerMessage(Level = LogLevel.Debug, Message = "No property sources found.")]
     private partial void LogNoPropertySourcesFound();
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Config Server health check returning UP.")]
     private partial void LogHealthCheckReturningUp();
 
-    [LoggerMessage(Level = LogLevel.Debug, Message = "Returning property source {PropertySource}.")]
-    private partial void LogReturningPropertySource(string? propertySource);
+    [LoggerMessage(Level = LogLevel.Debug, Message = "Returning property sources: {PropertySources}.")]
+    private partial void LogReturningPropertySources(string propertySources);
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Cache stale, fetching config server health.")]
     private partial void LogCacheStale();

src/Configuration/src/ConfigServer/ConfigServerHealthContributor.cs

@@ -173,7 +173,7 @@
                 },
                 "TokenRenewRate": {
                   "type": "integer",
-                  "description": "Gets or sets the vault token renew rate (in milliseconds). Default value: 60_000 (1 minute)."
+                  "description": "Gets or sets the Vault token renew rate (in milliseconds). Default value: 60_000 (1 minute)."
                 },
                 "TokenTtl": {
                   "type": "integer",

src/Configuration/src/ConfigServer/ConfigurationSchema.json

@@ -10,6 +10,7 @@
 using Microsoft.Extensions.Options;
 using RichardSzalay.MockHttp;
 using Steeltoe.Common.TestResources;
+using Steeltoe.Configuration.Placeholder;
 
 namespace Steeltoe.Configuration.ConfigServer.Test;
 
@@ -333,12 +334,15 @@ public void Changes_in_IConfiguration_update_provider_options_and_injected_optio
 
         fileProvider.IncludeAppSettingsJsonFile("""
             {
+              "custom": {
+                "profileName": "example-profile"
+              },
               "spring": {
                 "cloud": {
                   "config": {
                     "uri": "https://config.server.com:9999",
                     "name": "example-app-name",
-                    "env": "example-profile",
+                    "env": "${custom:profileName}",
                     "timeout": 30000
                   }
                 }
@@ -361,6 +365,7 @@ public void Changes_in_IConfiguration_update_provider_options_and_injected_optio
 
         var configurationBuilder = new ConfigurationBuilder();
         configurationBuilder.AddInMemoryAppSettingsJsonFile(fileProvider);
+        configurationBuilder.AddPlaceholderResolver();
         // ReSharper disable once AccessToDisposedClosure
         configurationBuilder.AddConfigServer(initialOptions, configureOptions, () => handler, NullLoggerFactory.Instance);
         IConfigurationRoot configuration = configurationBuilder.Build();
@@ -395,12 +400,15 @@ public void Changes_in_IConfiguration_update_provider_options_and_injected_optio
 
         fileProvider.ReplaceAppSettingsJsonFile("""
             {
-              "spring": {
+              "custom": {
+              "profileName": "example-profile"
+            },
+            "spring": {
                 "cloud": {
                   "config": {
                     "uri": "https://alternate-config.server.com:7777",
                     "name": "alternate-name",
-                    "env": "example-profile",
+                    "env": "${custom:profileName}",
                     "timeout": 15000,
                     "label": "alternate-label"
                   }

src/Configuration/test/ConfigServer.Test/ConfigServerClientOptionsTest.cs

@@ -42,7 +42,8 @@ public void AddConfigServer_WithLoggerFactorySucceeds()
 
         IList<string> logMessages = loggerProvider.GetAll();
 
-        logMessages.Should().Contain("DBUG Steeltoe.Configuration.ConfigServer.ConfigServerConfigurationProvider: Fetching configuration from server(s).");
+        logMessages.Should().Contain(
+            "DBUG Steeltoe.Configuration.ConfigServer.ConfigServerConfigurationProvider: Fetching remote configuration from server(s).");
     }
 
     [Fact]