Skip to content

Refactor configuration for azure queue service for events to include queue name #6724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
brant-livefront wants to merge 4 commits into
base: main
Choose a base branch
from
+35 −9
Open

Refactor configuration for azure queue service for events to include queue name #6724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bc0ea9
Refactor configuration for azure queue service for events to include …
brant-livefront Dec 11, 2025
c20f8d3
Address PR feedback
brant-livefront Dec 11, 2025
3054b20
Add check for queue name before writing to Azure Queue Service
brant-livefront Dec 11, 2025
aaf8d48
Fix file encoding (lint error)
brant-livefront Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions dev/secrets.json.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
"id": "<your Installation Id>",
"key": "<your Installation Key>"
},
"events": {
"connectionString": "",
Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation needed: The example shows queueName: "event" as the default value, but it's not clear:

  1. Whether this queue name must match exactly in production environments
  2. What happens if this is left empty (service is disabled, which is intentional per PR author's comment)
  3. Any naming conventions or restrictions for queue names

Consider adding a comment above this section:

// Azure Queue Storage configuration for events
// Leave connectionString and queueName empty to disable the service in production
// Default queue name is "event" for local development
"events": {

This would help developers understand the configuration's purpose and behavior.

"queueName": "event"
Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation suggestion: While the example shows "event" as the queue name, it's not clear to developers:

  • Whether this exact value is required in production
  • What happens if left empty (service is disabled)
  • Naming conventions or restrictions for queue names

Consider adding a comment:

// Azure Queue Storage configuration for events
// Leave connectionString and queueName empty to disable the service
// Default queue name "event" is used for local development
"events": {
  "connectionString": "",
  "queueName": "event"
},

},
"licenseDirectory": "<full path to license directory>",
"enableNewDeviceVerification": true,
"enableEmailVerification": true
Expand Down
3 changes: 2 additions & 1 deletion src/Core/AdminConsole/EventIntegrations/EventIntegrationsServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ public static IServiceCollection AddEventWriteServices(this IServiceCollection s
return services;
}

if (CoreHelpers.SettingHasValue(globalSettings.Events.ConnectionString))
if (CoreHelpers.SettingHasValue(globalSettings.Events.ConnectionString) &&
Copy link
Contributor

@claude claude bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Correctly Addresses Previous Review Finding #2

This now checks both ConnectionString and QueueName before registering the Azure Queue service, preventing the hardcoded queue name issue from the previous review.

The check aligns with the documentation in the XML comments (lines 69-70) and the read-side implementation in AzureQueueHostedService.cs (lines 60-63).

CoreHelpers.SettingHasValue(globalSettings.Events.QueueName))
Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage: While the implementation correctly validates both ConnectionString and QueueName, there's no test case covering the scenario where ConnectionString is present but QueueName is missing (or vice versa).

Consider adding test cases like:

  • AddEventWriteServices_ConnectionStringPresentButQueueNameMissing_DoesNotRegisterAzureQueueService
  • AddEventWriteServices_QueueNamePresentButConnectionStringMissing_DoesNotRegisterAzureQueueService

This would ensure the validation logic works correctly and prevent regression if someone changes the && to || by mistake.

Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Missing test coverage for partial configuration: The check validates both ConnectionString AND QueueName, but there are no tests covering scenarios where only one is present.

Recommended test cases:

[Fact]
public void AddEventWriteServices_ConnectionStringPresentButQueueNameMissing_RegistersNoopService()
{
    var globalSettings = CreateGlobalSettings(new Dictionary<string, string?>
    {
        ["GlobalSettings:Events:ConnectionString"] = "DefaultEndpoints...",
        ["GlobalSettings:Events:QueueName"] = null
    });
    
    services.AddEventWriteServices(globalSettings);
    
    Assert.Contains(services, s => s.ServiceType == typeof(IEventWriteService) 
        && s.ImplementationType == typeof(NoopEventWriteService));
}

[Fact]
public void AddEventWriteServices_QueueNamePresentButConnectionStringMissing_RegistersNoopService()
{
    var globalSettings = CreateGlobalSettings(new Dictionary<string, string?>
    {
        ["GlobalSettings:Events:ConnectionString"] = null,
        ["GlobalSettings:Events:QueueName"] = "event"
    });
    
    services.AddEventWriteServices(globalSettings);
    
    Assert.Contains(services, s => s.ServiceType == typeof(IEventWriteService) 
        && s.ImplementationType == typeof(NoopEventWriteService));
}

This ensures the && logic works correctly and prevents regression.

{
services.TryAddSingleton<IEventWriteService, AzureQueueEventWriteService>();
return services;
Expand Down
2 changes: 1 addition & 1 deletion src/Core/AdminConsole/Services/Implementations/AzureQueueEventWriteService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Bit.Core.Services;
public class AzureQueueEventWriteService : AzureQueueService<IEvent>, IEventWriteService
{
public AzureQueueEventWriteService(GlobalSettings globalSettings) : base(
new QueueClient(globalSettings.Events.ConnectionString, "event"),
new QueueClient(globalSettings.Events.ConnectionString, globalSettings.Events.QueueName),
Copy link
Contributor

@claude claude bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Potential Null Reference Exception

If globalSettings.Events.QueueName is null, this will throw an ArgumentNullException when instantiating the QueueClient, even though the service collection registration checks for null values.

While the registration in AddEventWriteServices (line 94-95 of EventIntegrationsServiceCollectionExtensions.cs) now checks both ConnectionString and QueueName, if someone were to manually instantiate this service or if the registration check fails to prevent it, this would throw.

Recommendation: Add a guard clause or null check with a more descriptive error message:

public AzureQueueEventWriteService(GlobalSettings globalSettings) : base(
    new QueueClient(
        globalSettings.Events.ConnectionString ?? throw new ArgumentNullException(nameof(globalSettings.Events.ConnectionString)),
        globalSettings.Events.QueueName ?? throw new ArgumentNullException(nameof(globalSettings.Events.QueueName))
    ),
    JsonHelpers.IgnoreWritingNull)
{ }

This would make debugging easier by clearly identifying which parameter is null.

Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential runtime exception if QueueName is null: While AddEventWriteServices now checks for null QueueName before registering this service, this constructor doesn't validate that globalSettings.Events.QueueName is non-null. If this service is ever instantiated directly (outside of DI), it could throw ArgumentNullException from the QueueClient constructor.

Consider adding a guard clause for defensive programming:

public AzureQueueEventWriteService(GlobalSettings globalSettings) : base(
    new QueueClient(
        globalSettings.Events.ConnectionString 
            ?? throw new ArgumentNullException(nameof(globalSettings), "Events.ConnectionString cannot be null"),
        globalSettings.Events.QueueName 
            ?? throw new ArgumentNullException(nameof(globalSettings), "Events.QueueName cannot be null")),
    JsonHelpers.IgnoreWritingNull)
{ }

This provides better error messages and fail-fast behavior if the service is misconfigured.

Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Defensive programming suggestion: While the service collection registration validates both values, this constructor could still throw if instantiated directly (outside of DI).

Consider adding guard clauses for better error messages:

public AzureQueueEventWriteService(GlobalSettings globalSettings) : base(
    new QueueClient(
        globalSettings.Events.ConnectionString 
            ?? throw new ArgumentNullException(nameof(globalSettings), "Events.ConnectionString cannot be null"),
        globalSettings.Events.QueueName 
            ?? throw new ArgumentNullException(nameof(globalSettings), "Events.QueueName cannot be null")),
    JsonHelpers.IgnoreWritingNull)
{ }

This provides fail-fast behavior with clear error messages if misconfigured.

JsonHelpers.IgnoreWritingNull)
{ }

Expand Down
20 changes: 19 additions & 1 deletion src/Core/Settings/GlobalSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
public virtual EventLoggingSettings EventLogging { get; set; } = new EventLoggingSettings();
public virtual MailSettings Mail { get; set; } = new MailSettings();
public virtual IConnectionStringSettings Storage { get; set; } = new ConnectionStringSettings();
public virtual ConnectionStringSettings Events { get; set; } = new ConnectionStringSettings();
public virtual AzureQueueEventSettings Events { get; set; } = new AzureQueueEventSettings();
Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backward compatibility concern: Changing from ConnectionStringSettings to AzureQueueEventSettings is a breaking change in the configuration schema. Existing appsettings.json files across the codebase only have:

"events": {
  "connectionString": "SECRET"
}

They don't have the new queueName field. While the code handles this gracefully (the service won't register if QueueName is null), this means:

  1. Existing deployments will break silently - The Azure Queue event write service won't start, and events won't be queued, but there will be no error - just a log message in AzureQueueHostedService.
  2. All appsettings files need updating - Every environment configuration file needs the queueName field added.

Recommendations:

  • Document this breaking change in the PR description
  • Provide migration instructions for updating all appsettings files
  • Consider whether a default value (e.g., "event") should be used when queueName is not specified for backward compatibility
  • Update all appsettings*.json files that currently have events.connectionString to also include queueName

public virtual DistributedCacheSettings DistributedCache { get; set; } = new DistributedCacheSettings();
public virtual NotificationsSettings Notifications { get; set; } = new NotificationsSettings();
public virtual IFileStorageSettings Attachment { get; set; }
Expand Down Expand Up @@ -116,7 +116,7 @@
{
return null;
}
return string.Format("http://{0}:5000", name);

Check warning on line 119 in src/Core/Settings/GlobalSettings.cs

View workflow job for this annotation

GitHub Actions / Sonar / Quality scan 

Using http protocol is insecure. Use https instead. (https://rules.sonarsource.com/csharp/RSPEC-5332)
}

public string BuildDirectory(string explicitValue, string appendedPath)
Expand Down Expand Up @@ -395,6 +395,24 @@
}
}

public class AzureQueueEventSettings : IConnectionStringSettings
Copy link
Contributor

@claude claude bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Note: Inconsistent Null Handling Pattern in Codebase

The new AzureQueueEventSettings class uses null-conditional operators (value?.Trim('"')) in both ConnectionString and QueueName setters (lines 406, 412), which is the safe approach.

However, most other settings classes in this file use value.Trim('"') without null checks (e.g., ConnectionStringSettings at line 423), which will throw NullReferenceException if value is null.

For this PR: The null-safe approach here is correct and aligns with the intentional design of allowing null QueueName to disable the service.

For future consideration: The codebase has an inconsistent pattern that could be standardized in a separate refactoring PR.

{
private string _connectionString;
private string _queueName;

public string ConnectionString
{
get => _connectionString;
set => _connectionString = value?.Trim('"');
Copy link
Contributor

@claude claude bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good Null Safety Implementation

The null-conditional operator (value?.Trim('"')) correctly handles null values, allowing the QueueName to be null (which is intentional per the author's note to disable the service in production).

This is consistent with the pattern used in other settings classes like AzureServiceBusSettings.ConnectionString (line 326).

Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good null safety: The null-conditional operator value?.Trim('"') correctly handles null values. However, note that ConnectionStringSettings.ConnectionString (line 423, not changed in this PR) uses value.Trim('"') without the null-conditional operator, creating an inconsistency in the codebase.

While this isn't a blocker for this PR, consider filing a follow-up issue to address the null safety inconsistency in ConnectionStringSettings to maintain consistency across similar settings classes.

}

public string QueueName
{
get => _queueName;
set => _queueName = value?.Trim('"');
Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good null safety: The null-conditional operator value?.Trim('"') correctly allows null values, which is intentional for disabling the service in production (as noted by the PR author).

This is consistent with patterns used elsewhere in the codebase like AzureServiceBusSettings.ConnectionString at line 326.

}
}

public class ConnectionStringSettings : IConnectionStringSettings
{
private string _connectionString;
Expand Down
12 changes: 7 additions & 5 deletions src/EventsProcessor/AzureQueueHostedService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
using Bit.Core;
using Bit.Core.Models.Data;
using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Utilities;

namespace Bit.EventsProcessor;

public class AzureQueueHostedService : IHostedService, IDisposable
{
private readonly ILogger<AzureQueueHostedService> _logger;
private readonly IConfiguration _configuration;
private readonly GlobalSettings _globalSettings;

private Task _executingTask;
private CancellationTokenSource _cts;
Expand All @@ -22,10 +23,10 @@ public class AzureQueueHostedService : IHostedService, IDisposable

public AzureQueueHostedService(
ILogger<AzureQueueHostedService> logger,
IConfiguration configuration)
GlobalSettings globalSettings)
{
_logger = logger;
_configuration = configuration;
_globalSettings = globalSettings;
}

public Task StartAsync(CancellationToken cancellationToken)
Expand Down Expand Up @@ -56,11 +57,12 @@ public void Dispose()

private async Task ExecuteAsync(CancellationToken cancellationToken)
{
var storageConnectionString = _configuration["azureStorageConnectionString"];
var queueName = _configuration["azureQueueServiceQueueName"];
var storageConnectionString = _globalSettings.Events.ConnectionString;
var queueName = _globalSettings.Events.QueueName;
if (string.IsNullOrWhiteSpace(storageConnectionString) ||
string.IsNullOrWhiteSpace(queueName))
{
_logger.LogInformation("Azure Queue Hosted Service is disabled. Missing connection string or queue name.");
Copy link
Contributor

@claude claude bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Excellent Error Handling

The information-level log message clearly indicates why the service is disabled. This is much better than the previous silent failure.

Minor suggestion: Consider using structured logging to make this more queryable:

_logger.LogInformation("Azure Queue Hosted Service is disabled. ConnectionStringPresent: {ConnectionStringPresent}, QueueNamePresent: {QueueNamePresent}", 
    !string.IsNullOrWhiteSpace(storageConnectionString), 
    !string.IsNullOrWhiteSpace(queueName));

This would help operators understand exactly which configuration is missing without needing to look at code.

Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good improvement: This log message is helpful for troubleshooting configuration issues. The service now gracefully handles missing configuration and logs appropriately.

Minor suggestion: Consider using structured logging with named parameters for better log aggregation and filtering:

_logger.LogInformation("Azure Queue Hosted Service is disabled. ConnectionString present: {HasConnectionString}, QueueName present: {HasQueueName}", 
    !string.IsNullOrWhiteSpace(storageConnectionString), 
    !string.IsNullOrWhiteSpace(queueName));

This would make it easier to diagnose which specific configuration value is missing in production environments.

Copy link
Contributor

@claude claude bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Excellent error handling: The information-level log message properly addresses Finding #3 from the previous review. This makes troubleshooting much easier.

Minor enhancement suggestion (optional): Consider using structured logging to make this more queryable in production:

_logger.LogInformation(
    "Azure Queue Hosted Service is disabled. ConnectionString present: {HasConnectionString}, QueueName present: {HasQueueName}", 
    !string.IsNullOrWhiteSpace(storageConnectionString), 
    !string.IsNullOrWhiteSpace(queueName));

This would help operators quickly identify which specific configuration value is missing without inspecting code.

return;
}

Expand Down
3 changes: 2 additions & 1 deletion ...e.Test/AdminConsole/EventIntegrations/EventIntegrationServiceCollectionExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,8 @@ public void AddEventWriteServices_EventsConnectionStringPresent_RegistersAzureQu
var services = new ServiceCollection();
var globalSettings = CreateGlobalSettings(new Dictionary<string, string?>
{
["GlobalSettings:Events:ConnectionString"] = "DefaultEndpointsProtocol=https;AccountName=test;AccountKey=test;EndpointSuffix=core.windows.net"
["GlobalSettings:Events:ConnectionString"] = "DefaultEndpointsProtocol=https;AccountName=test;AccountKey=test;EndpointSuffix=core.windows.net",
["GlobalSettings:Events:QueueName"] = "event"
Copy link
Contributor

@claude claude bot Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Missing Test Coverage

The author mentioned adding "a test to prevent service start if QueueName is null/empty", but I cannot find such a test in this file.

There should be tests for these scenarios:

  1. When ConnectionString is present but QueueName is null/empty → Should register NoopEventWriteService
  2. When QueueName is present but ConnectionString is null/empty → Should register NoopEventWriteService

Recommendation: Add these test cases:

[Fact]
public void AddEventWriteServices_EventsConnectionStringPresentButQueueNameMissing_RegistersNoopService()
{
    var services = new ServiceCollection();
    var globalSettings = CreateGlobalSettings(new Dictionary<string, string?>
    {
        ["GlobalSettings:Events:ConnectionString"] = "DefaultEndpointsProtocol=https;AccountName=test;AccountKey=test;EndpointSuffix=core.windows.net",
        ["GlobalSettings:Events:QueueName"] = null
    });

    services.AddEventWriteServices(globalSettings);

    Assert.Contains(services, s => s.ServiceType == typeof(IEventWriteService) && s.ImplementationType == typeof(NoopEventWriteService));
}

[Fact]
public void AddEventWriteServices_QueueNamePresentButConnectionStringMissing_RegistersNoopService()
{
    var services = new ServiceCollection();
    var globalSettings = CreateGlobalSettings(new Dictionary<string, string?>
    {
        ["GlobalSettings:Events:ConnectionString"] = null,
        ["GlobalSettings:Events:QueueName"] = "event"
    });

    services.AddEventWriteServices(globalSettings);

    Assert.Contains(services, s => s.ServiceType == typeof(IEventWriteService) && s.ImplementationType == typeof(NoopEventWriteService));
}

});

services.AddEventWriteServices(globalSettings);
Expand Down
Loading