Add support for identity-based connections to Diagnostic Events (#10438)

cjaliaga · liliankasem · web-flow · commit 5bac7614d792 · 2024-10-18T19:45:40.000+02:00
* Add support for identity-based connections to Diagnostic Events

* Updated release notes

* Removed unused namespace

* Ensure that the factory takes into account a configuration section and not only the connection string

* Logging failure to create the client as a warning and adapted message

* Update release_notes

Co-authored-by: Lilian Kasem &lt;likasem@microsoft.com&gt;

* Address PR feedback

* Update release notes

---------

Co-authored-by: Lilian Kasem &lt;likasem@microsoft.com&gt;
diff --git a/release_notes.md b/release_notes.md
@@ -11,3 +11,4 @@
 - Implement host configuration property to allow configuration of the metadata provider timeout period (#10526)
   - The value can be set via `metadataProviderTimeout` in host.json and defaults to "00:00:30" (30 seconds).
   - For logic apps, unless configured via the host.json, the timeout is disabled by default.
+- Added support for identity-based connections to Diagnostic Events (#10438)
diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventRepositoryFactory.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventRepositoryFactory.cs
@@ -4,7 +4,6 @@
 using System;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
-using Microsoft.Extensions.Logging;
 
 namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics
 {
@@ -21,11 +20,12 @@ public DiagnosticEventRepositoryFactory(IServiceProvider serviceProvider, IConfi
 
         public IDiagnosticEventRepository Create()
         {
-            // Using this to break ciruclar dependency for ILoggers. Typically you cannot log errors within the logging pipeline because it creates infinte loop.
+            // Using this to break circular dependency for ILoggers. Typically you cannot log errors within the logging pipeline because it creates infinite loop.
             // However in this case that loop is broken because of the filtering in the DiagnosticEventLogger
 
-            string storageConnectionString = _configuration.GetWebJobsConnectionString(ConnectionStringNames.Storage);
-            if (string.IsNullOrEmpty(storageConnectionString))
+            IConfigurationSection storageConnectionSection = _configuration.GetWebJobsConnectionSection(ConnectionStringNames.Storage);
+
+            if (storageConnectionSection is null || !storageConnectionSection.Exists())
             {
                 return new DiagnosticEventNullRepository();
             }
diff --git a/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs b/src/WebJobs.Script.WebHost/Diagnostics/DiagnosticEventTableStorageRepository.cs
@@ -12,7 +12,6 @@
 using Microsoft.Azure.WebJobs.Hosting;
 using Microsoft.Azure.WebJobs.Logging;
 using Microsoft.Azure.WebJobs.Script.WebHost.Helpers;
-using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 
@@ -25,9 +24,9 @@ public class DiagnosticEventTableStorageRepository : IDiagnosticEventRepository,
         private const int TableCreationMaxRetryCount = 5;
 
         private readonly Timer _flushLogsTimer;
-        private readonly IConfiguration _configuration;
         private readonly IHostIdProvider _hostIdProvider;
         private readonly IEnvironment _environment;
+        private readonly IAzureTableStorageProvider _azureTableStorageProvider;
         private readonly ILogger<DiagnosticEventTableStorageRepository> _logger;
         private readonly IServiceProvider _serviceProvider;
         private readonly object _syncLock = new object();
@@ -41,43 +40,30 @@ public class DiagnosticEventTableStorageRepository : IDiagnosticEventRepository,
         private string _tableName;
         private bool _isEnabled = true;
 
-        internal DiagnosticEventTableStorageRepository(IConfiguration configuration, IHostIdProvider hostIdProvider, IEnvironment environment, IScriptHostManager scriptHostManager,
-            ILogger<DiagnosticEventTableStorageRepository> logger, int logFlushInterval)
+        internal DiagnosticEventTableStorageRepository(IHostIdProvider hostIdProvider, IEnvironment environment, IScriptHostManager scriptHostManager,
+            IAzureTableStorageProvider azureTableStorageProvider, ILogger<DiagnosticEventTableStorageRepository> logger, int logFlushInterval)
         {
-            _configuration = configuration;
             _hostIdProvider = hostIdProvider;
             _environment = environment;
             _serviceProvider = scriptHostManager as IServiceProvider;
             _logger = logger;
             _flushLogsTimer = new Timer(OnFlushLogs, null, logFlushInterval, logFlushInterval);
+            _azureTableStorageProvider = azureTableStorageProvider;
         }
 
-        public DiagnosticEventTableStorageRepository(IConfiguration configuration, IHostIdProvider hostIdProvider, IEnvironment environment, IScriptHostManager scriptHost,
-            ILogger<DiagnosticEventTableStorageRepository> logger)
-            : this(configuration, hostIdProvider, environment, scriptHost, logger, LogFlushInterval) { }
+        public DiagnosticEventTableStorageRepository(IHostIdProvider hostIdProvider, IEnvironment environment, IScriptHostManager scriptHost,
+            IAzureTableStorageProvider azureTableStorageProvider, ILogger<DiagnosticEventTableStorageRepository> logger)
+            : this(hostIdProvider, environment, scriptHost, azureTableStorageProvider, logger, LogFlushInterval) { }
 
         internal TableServiceClient TableClient
         {
             get
             {
-                if (!_environment.IsPlaceholderModeEnabled() && _tableClient == null)
+                if (!_environment.IsPlaceholderModeEnabled() && _tableClient == null && !_azureTableStorageProvider.TryCreateHostingTableServiceClient(out _tableClient))
                 {
-                    string storageConnectionString = _configuration.GetWebJobsConnectionString(ConnectionStringNames.Storage);
-
-                    try
-                    {
-                        _tableClient = new TableServiceClient(storageConnectionString);
-
-                        // The TableServiceClient only verifies the format of the connection string.
-                        // To ensure the storage account exists and supports Table storage, validate the connection string by retrieving the properties of the table service.
-                        _ = _tableClient.GetProperties();
-                    }
-                    catch (Exception ex)
-                    {
-                        _logger.LogError(ex, "The Azure Storage connection string is either empty or invalid. Unable to record diagnostic events, so the diagnostic logging service is being stopped.");
-                        _isEnabled = false;
-                        StopTimer();
-                    }
+                    _logger.LogWarning("An error occurred initializing the Table Storage Client. We are unable to record diagnostic events, so the diagnostic logging service is being stopped.");
+                    _isEnabled = false;
+                    StopTimer();
                 }
 
                 return _tableClient;
diff --git a/test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs b/test/WebJobs.Script.Tests.Integration/Diagnostics/DiagnosticEventTableStorageRepositoryTests.cs
@@ -27,13 +27,13 @@ public class DiagnosticEventTableStorageRepositoryTests
         private const string TestHostId = "testhostid";
         private readonly IHostIdProvider _hostIdProvider;
         private readonly TestLoggerProvider _loggerProvider;
-        private IConfiguration _configuration;
+        private readonly IAzureTableStorageProvider _azureTableStorageProvider;
+
         private ILogger<DiagnosticEventTableStorageRepository> _logger;
         private Mock<IScriptHostManager> _scriptHostMock;
 
         public DiagnosticEventTableStorageRepositoryTests()
         {
-            _configuration = new ConfigurationBuilder().AddEnvironmentVariables().Build();
             _hostIdProvider = new FixedHostIdProvider(TestHostId);
 
             var mockPrimaryHostStateProvider = new Mock<IPrimaryHostStateProvider>(MockBehavior.Strict);
@@ -46,6 +46,9 @@ public DiagnosticEventTableStorageRepositoryTests()
             ILoggerFactory loggerFactory = new LoggerFactory();
             loggerFactory.AddProvider(_loggerProvider);
             _logger = loggerFactory.CreateLogger<DiagnosticEventTableStorageRepository>();
+
+            var configuration = new ConfigurationBuilder().AddEnvironmentVariables().Build();
+            _azureTableStorageProvider = TestHelpers.GetAzureTableStorageProvider(configuration);
         }
 
         [Fact]
@@ -54,7 +57,7 @@ public async Task TimerFlush_CalledOnExpectedInterval()
             IEnvironment testEnvironment = new TestEnvironment();
             int flushInterval = 10;
             Mock<DiagnosticEventTableStorageRepository> mockDiagnosticEventTableStorageRepository = new Mock<DiagnosticEventTableStorageRepository>
-                (_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, NullLogger<DiagnosticEventTableStorageRepository>.Instance, flushInterval);
+                (_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, NullLogger<DiagnosticEventTableStorageRepository>.Instance, flushInterval);
 
             DiagnosticEventTableStorageRepository repository = mockDiagnosticEventTableStorageRepository.Object;
 
@@ -76,7 +79,7 @@ public void WriteDiagnostic_LogsError_WhenHostIdNotSet()
             IEnvironment testEnvironment = new TestEnvironment();
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, null, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(null, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             repository.WriteDiagnosticEvent(DateTime.UtcNow, "eh1", LogLevel.Information, "This is the message", "https://fwlink/", new Exception("exception message"));
 
@@ -91,7 +94,7 @@ public async Task WriteDiagnostic_HasTheCorrectHitCount_WhenCalledFromMultipleTh
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             var eventTask1 = Task.Run(() =>
             {
@@ -136,7 +139,7 @@ public void GetDiagnosticEventsTable_ReturnsExpectedValue_WhenSpecialized()
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
             DateTime dateTime = new DateTime(2021, 1, 1);
             var cloudTable = repository.GetDiagnosticEventsTable(dateTime);
             Assert.NotNull(cloudTable);
@@ -151,16 +154,17 @@ public void GetDiagnosticEventsTable_LogsError_StorageConnectionStringIsNotPrese
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             var configuration = new ConfigurationBuilder().Build();
+            var localStorageProvider = TestHelpers.GetAzureTableStorageProvider(configuration);
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, localStorageProvider, _logger);
 
             // The repository should be initially enabled and then disabled after TableServiceClient failure.
             Assert.True(repository.IsEnabled());
             DateTime dateTime = new DateTime(2021, 1, 1);
             var cloudTable = repository.GetDiagnosticEventsTable(dateTime);
             Assert.Null(cloudTable);
             var messages = _loggerProvider.GetAllLogMessages();
-            Assert.Equal(messages[0].FormattedMessage, "The Azure Storage connection string is either empty or invalid. Unable to record diagnostic events, so the diagnostic logging service is being stopped.");
+            Assert.Equal(messages[0].FormattedMessage, "An error occurred initializing the Table Storage Client. We are unable to record diagnostic events, so the diagnostic logging service is being stopped.");
             Assert.False(repository.IsEnabled());
         }
 
@@ -171,7 +175,7 @@ public async Task QueueBackgroundDiagnosticsEventsTablePurge_PurgesTables()
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             // delete any existing non-current diagnostics events tables
             string tablePrefix = DiagnosticEventTableStorageRepository.TableNamePrefix;
@@ -212,7 +216,7 @@ public async Task QueueBackgroundDiagnosticsEventsTablePurge_PurgesOnlyDiagnosti
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             // delete any existing non-current diagnostics events tables
             string tablePrefix = DiagnosticEventTableStorageRepository.TableNamePrefix;
@@ -267,8 +271,9 @@ public async Task FlushLogs_LogsErrorAndClearsEvents_WhenTableCreatingFails()
                 .AddInMemoryCollection(testData)
                 .Build();
 
+            var localStorageProvider = TestHelpers.GetAzureTableStorageProvider(configuration);
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, localStorageProvider, _logger);
 
             // Act
             repository.WriteDiagnosticEvent(DateTime.UtcNow, "eh1", LogLevel.Information, "This is the message", "https://fwlink/", new Exception("exception message"));
@@ -278,7 +283,7 @@ public async Task FlushLogs_LogsErrorAndClearsEvents_WhenTableCreatingFails()
             var logMessage = _loggerProvider.GetAllLogMessages().SingleOrDefault(m => m.FormattedMessage.Contains("Unable to get table reference"));
             Assert.NotNull(logMessage);
 
-            var messagePresent = _loggerProvider.GetAllLogMessages().Any(m => m.FormattedMessage.Contains("The Azure Storage connection string is either empty or invalid. Unable to record diagnostic events, so the diagnostic logging service is being stopped."));
+            var messagePresent = _loggerProvider.GetAllLogMessages().Any(m => m.FormattedMessage.Contains("An error occurred initializing the Table Storage Client. We are unable to record diagnostic events, so the diagnostic logging service is being stopped."));
             Assert.True(messagePresent);
 
             Assert.Equal(0, repository.Events.Values.Count());
@@ -302,7 +307,7 @@ public async Task FlushLogs_OnPrimaryHost_PurgesPreviousEventVersionTables(strin
             scriptHostMock.As<IServiceProvider>().Setup(m => m.GetService(typeof(IPrimaryHostStateProvider))).Returns(mockPrimaryHostStateProvider.Object);
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             // delete existing tables
             string tablePrefix = DiagnosticEventTableStorageRepository.TableNamePrefix;
@@ -352,7 +357,7 @@ public async Task ExecuteBatchAsync_WritesToTableStorage()
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             var table = repository.GetDiagnosticEventsTable();
             await TableStorageHelpers.CreateIfNotExistsAsync(table, repository.TableClient, 2);
@@ -375,7 +380,7 @@ public async Task FlushLogs_WritesToTableStorage()
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             var table = repository.GetDiagnosticEventsTable();
             await TableStorageHelpers.CreateIfNotExistsAsync(table, repository.TableClient, 2);
@@ -397,7 +402,7 @@ public async Task ExecuteBatchAsync_LogsError()
             testEnvironment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "0");
 
             DiagnosticEventTableStorageRepository repository =
-                new DiagnosticEventTableStorageRepository(_configuration, _hostIdProvider, testEnvironment, _scriptHostMock.Object, _logger);
+                new DiagnosticEventTableStorageRepository(_hostIdProvider, testEnvironment, _scriptHostMock.Object, _azureTableStorageProvider, _logger);
 
             var tableClient = repository.TableClient;
             var table = tableClient.GetTableClient("aa");

Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,6 @@`
`4`	`4`	`using System;`
`5`	`5`	`using Microsoft.Extensions.Configuration;`
`6`	`6`	`using Microsoft.Extensions.DependencyInjection;`
`7`		`-using Microsoft.Extensions.Logging;`
`8`	`7`
`9`	`8`	`namespace Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics`
`10`	`9`	`{`
`@@ -21,11 +20,12 @@ public DiagnosticEventRepositoryFactory(IServiceProvider serviceProvider, IConfi`
`21`	`20`
`22`	`21`	`public IDiagnosticEventRepository Create()`
`23`	`22`	`{`
`24`		`- // Using this to break ciruclar dependency for ILoggers. Typically you cannot log errors within the logging pipeline because it creates infinte loop.`
	`23`	`+ // Using this to break circular dependency for ILoggers. Typically you cannot log errors within the logging pipeline because it creates infinite loop.`
`25`	`24`	`// However in this case that loop is broken because of the filtering in the DiagnosticEventLogger`
`26`	`25`
`27`		`- string storageConnectionString = _configuration.GetWebJobsConnectionString(ConnectionStringNames.Storage);`
`28`		`- if (string.IsNullOrEmpty(storageConnectionString))`
	`26`	`+ IConfigurationSection storageConnectionSection = _configuration.GetWebJobsConnectionSection(ConnectionStringNames.Storage);`
	`27`	`+`
	`28`	`+ if (storageConnectionSection is null \|\| !storageConnectionSection.Exists())`
`29`	`29`	`{`
`30`	`30`	`return new DiagnosticEventNullRepository();`
`31`	`31`	`}`