Skip to content
This repository was archived by the owner on Nov 1, 2023. It is now read-only.

Commit c44bd03

Browse files
PorgesPorges
authored
Make ServiceConfiguration eagerly evalulated (#3136)
See #3316. This helps diagnosing problems when running the service. Read all configuration variables in the service upon startup to detect problems as early as possible. This also allows us to remove a bunch of nullability annotations and makes the config easier to use, as well as enforcing stronger types like `Url` and `ResourceIdentifier`.
1 parent 938176b commit c44bd03

File tree

19 files changed

+121
-203
lines changed

19 files changed

+121
-203
lines changed

‎src/ApiService/ApiService/Functions/Events.cs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,13 @@
11
using Microsoft.Azure.Functions.Worker;
22
using Microsoft.Azure.Functions.Worker.Http;
3-
using Microsoft.Extensions.Logging;
43
using Microsoft.OneFuzz.Service.Auth;
54
namespace Microsoft.OneFuzz.Service.Functions;
65

76
public class EventsFunction {
8-
private readonly ILogger _log;
97
private readonly IOnefuzzContext _context;
108

11-
public EventsFunction(ILogger<EventsFunction> log, IOnefuzzContext context) {
9+
public EventsFunction(IOnefuzzContext context) {
1210
_context = context;
13-
_log = log;
1411
}
1512

1613
[Function("Events")]

‎src/ApiService/ApiService/Program.cs‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -70,17 +70,19 @@ public static async Async.Task Main() {
7070
.ConfigureAppConfiguration(builder => {
7171
// Using a connection string in dev allows us to run the functions locally.
7272
if (!string.IsNullOrEmpty(configuration.AppConfigurationConnectionString)) {
73-
var _ = builder.AddAzureAppConfiguration(options => {
74-
var _ = options
73+
builder.AddAzureAppConfiguration(options => {
74+
options
7575
.Connect(configuration.AppConfigurationConnectionString)
7676
.UseFeatureFlags(ffOptions => ffOptions.CacheExpirationInterval = TimeSpan.FromSeconds(30));
7777
});
78-
} else {
79-
var _ = builder.AddAzureAppConfiguration(options => {
80-
var _ = options
81-
.Connect(new Uri(configuration.AppConfigurationEndpoint!), new DefaultAzureCredential())
78+
} else if (!string.IsNullOrEmpty(configuration.AppConfigurationEndpoint)) {
79+
builder.AddAzureAppConfiguration(options => {
80+
options
81+
.Connect(new Uri(configuration.AppConfigurationEndpoint), new DefaultAzureCredential())
8282
.UseFeatureFlags(ffOptions => ffOptions.CacheExpirationInterval = TimeSpan.FromMinutes(1));
8383
});
84+
} else {
85+
throw new InvalidOperationException($"One of APPCONFIGURATION_CONNECTION_STRING or APPCONFIGURATION_ENDPOINT must be set");
8486
}
8587
})
8688
.ConfigureServices((context, services) => {
@@ -201,13 +203,10 @@ public static async Async.Task SetupStorage(IStorage storage, IServiceConfig ser
201203
}
202204
}
203205

204-
var storageAccount = serviceConfig.OneFuzzFuncStorage;
205-
if (storageAccount is not null) {
206-
var tableClient = await storage.GetTableServiceClientForAccount(storageAccount);
207-
await Async.Task.WhenAll(toCreate.Select(async t => {
208-
// don't care if it was created or not
209-
_ = await tableClient.CreateTableIfNotExistsAsync(serviceConfig.OneFuzzStoragePrefix + t.Name);
210-
}));
211-
}
206+
var tableClient = await storage.GetTableServiceClientForAccount(serviceConfig.OneFuzzFuncStorage);
207+
await Async.Task.WhenAll(toCreate.Select(async t => {
208+
// don't care if it was created or not
209+
_ = await tableClient.CreateTableIfNotExistsAsync(serviceConfig.OneFuzzStoragePrefix + t.Name);
210+
}));
212211
}
213212
}

‎src/ApiService/ApiService/ServiceConfiguration.cs‎

Lines changed: 55 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -10,57 +10,51 @@ public enum LogDestination {
1010

1111

1212
public interface IServiceConfig {
13-
public LogDestination[] LogDestinations { get; set; }
14-
13+
#region Parameters for logging & application insights
14+
public LogDestination[] LogDestinations { get; }
1515
public ApplicationInsights.DataContracts.SeverityLevel LogSeverityLevel { get; }
16-
1716
public string? ApplicationInsightsAppId { get; }
1817
public string? ApplicationInsightsInstrumentationKey { get; }
18+
#endregion
19+
20+
#region Parameters for feature flags
1921
public string? AppConfigurationEndpoint { get; }
2022
public string? AppConfigurationConnectionString { get; }
21-
public string? AzureSignalRConnectionString { get; }
22-
public string? AzureSignalRServiceTransportType { get; }
23-
24-
public string? AzureWebJobDisableHomePage { get; }
25-
public string? AzureWebJobStorage { get; }
23+
#endregion
2624

27-
public string? DiagnosticsAzureBlobContainerSasUrl { get; }
28-
public string? DiagnosticsAzureBlobRetentionDays { get; }
25+
#region Auth parameters for CLI app
2926
public string? CliAppId { get; }
3027
public string? Authority { get; }
3128
public string? TenantDomain { get; }
3229
public string? MultiTenantDomain { get; }
33-
public ResourceIdentifier? OneFuzzDataStorage { get; }
34-
public ResourceIdentifier? OneFuzzFuncStorage { get; }
35-
public string? OneFuzzInstance { get; }
36-
public string? OneFuzzInstanceName { get; }
37-
public string? OneFuzzEndpoint { get; }
38-
public string? OneFuzzKeyvault { get; }
39-
30+
#endregion
31+
32+
public ResourceIdentifier OneFuzzResourceGroup { get; }
33+
public ResourceIdentifier OneFuzzDataStorage { get; }
34+
public ResourceIdentifier OneFuzzFuncStorage { get; }
35+
public Uri OneFuzzInstance { get; }
36+
public string OneFuzzInstanceName { get; }
37+
public Uri? OneFuzzEndpoint { get; }
38+
public string OneFuzzKeyvault { get; }
4039
public string? OneFuzzMonitor { get; }
4140
public string? OneFuzzOwner { get; }
42-
43-
public string? OneFuzzResourceGroup { get; }
4441
public string? OneFuzzTelemetry { get; }
45-
4642
public string OneFuzzVersion { get; }
47-
4843
public string? OneFuzzAllowOutdatedAgent { get; }
4944

5045
// Prefix to add to the name of any tables & containers created. This allows
5146
// multiple instances to run against the same storage account, which
5247
// is useful for things like integration testing.
5348
public string OneFuzzStoragePrefix { get; }
54-
55-
public Uri OneFuzzBaseAddress { get; }
5649
}
5750

5851
public class ServiceConfiguration : IServiceConfig {
5952

6053
// Version is baked into the assembly by the build process:
61-
private static readonly string? _oneFuzzVersion =
54+
private static readonly string _oneFuzzVersion =
6255
Assembly.GetExecutingAssembly()
63-
.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
56+
.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion
57+
?? throw new InvalidOperationException("Unable to read OneFuzz version from assembly");
6458

6559
public ServiceConfiguration() {
6660
#if DEBUG
@@ -72,78 +66,57 @@ public ServiceConfiguration() {
7266

7367
private static string? GetEnv(string name) {
7468
var v = Environment.GetEnvironmentVariable(name);
75-
if (String.IsNullOrEmpty(v))
76-
return null;
77-
78-
return v;
69+
return string.IsNullOrEmpty(v) ? null : v;
7970
}
8071

72+
private static string MustGetEnv(string name)
73+
=> GetEnv(name) ?? throw new InvalidOperationException($"Environment variable {name} is required to be set");
74+
8175
//TODO: Add environment variable to control where to write logs to
82-
public LogDestination[] LogDestinations { get; set; }
76+
public LogDestination[] LogDestinations { get; private set; }
8377

8478
//TODO: Get this from Environment variable
8579
public ApplicationInsights.DataContracts.SeverityLevel LogSeverityLevel => ApplicationInsights.DataContracts.SeverityLevel.Verbose;
8680

87-
public string? ApplicationInsightsAppId => GetEnv("APPINSIGHTS_APPID");
88-
public string? ApplicationInsightsInstrumentationKey => GetEnv("APPINSIGHTS_INSTRUMENTATIONKEY");
81+
public string? ApplicationInsightsAppId { get; } = GetEnv("APPINSIGHTS_APPID");
8982

90-
public string? AppConfigurationEndpoint => GetEnv("APPCONFIGURATION_ENDPOINT");
83+
public string? ApplicationInsightsInstrumentationKey { get; } = GetEnv("APPINSIGHTS_INSTRUMENTATIONKEY");
9184

92-
public string? AppConfigurationConnectionString => GetEnv("APPCONFIGURATION_CONNECTION_STRING");
85+
public string? AppConfigurationEndpoint { get; } = GetEnv("APPCONFIGURATION_ENDPOINT");
9386

94-
public string? AzureSignalRConnectionString => GetEnv("AzureSignalRConnectionString");
95-
public string? AzureSignalRServiceTransportType => GetEnv("AzureSignalRServiceTransportType");
87+
public string? AppConfigurationConnectionString { get; } = GetEnv("APPCONFIGURATION_CONNECTION_STRING");
9688

97-
public string? AzureWebJobDisableHomePage { get => GetEnv("AzureWebJobsDisableHomepage"); }
98-
public string? AzureWebJobStorage { get => GetEnv("AzureWebJobsStorage"); }
89+
public string? CliAppId { get; } = GetEnv("CLI_APP_ID");
9990

100-
public string? DiagnosticsAzureBlobContainerSasUrl { get => GetEnv("DIAGNOSTICS_AZUREBLOBCONTAINERSASURL"); }
101-
public string? DiagnosticsAzureBlobRetentionDays { get => GetEnv("DIAGNOSTICS_AZUREBLOBRETENTIONINDAYS"); }
102-
public string? CliAppId { get => GetEnv("CLI_APP_ID"); }
103-
public string? Authority { get => GetEnv("AUTHORITY"); }
104-
public string? TenantDomain { get => GetEnv("TENANT_DOMAIN"); }
105-
public string? MultiTenantDomain { get => GetEnv("MULTI_TENANT_DOMAIN"); }
91+
public string? Authority { get; } = GetEnv("AUTHORITY");
10692

107-
public ResourceIdentifier? OneFuzzDataStorage {
108-
get {
109-
var env = GetEnv("ONEFUZZ_DATA_STORAGE");
110-
return env is null ? null : new ResourceIdentifier(env);
111-
}
112-
}
93+
public string? TenantDomain { get; } = GetEnv("TENANT_DOMAIN");
11394

114-
public ResourceIdentifier? OneFuzzFuncStorage {
115-
get {
116-
var env = GetEnv("ONEFUZZ_FUNC_STORAGE");
117-
return env is null ? null : new ResourceIdentifier(env);
118-
}
119-
}
95+
public string? MultiTenantDomain { get; } = GetEnv("MULTI_TENANT_DOMAIN");
12096

121-
public string? OneFuzzInstance { get => GetEnv("ONEFUZZ_INSTANCE"); }
122-
public string? OneFuzzInstanceName { get => GetEnv("ONEFUZZ_INSTANCE_NAME"); }
123-
public string? OneFuzzEndpoint { get => GetEnv("ONEFUZZ_ENDPOINT"); }
124-
public string? OneFuzzKeyvault { get => GetEnv("ONEFUZZ_KEYVAULT"); }
125-
public string? OneFuzzMonitor { get => GetEnv("ONEFUZZ_MONITOR"); }
126-
public string? OneFuzzOwner { get => GetEnv("ONEFUZZ_OWNER"); }
127-
public string? OneFuzzResourceGroup { get => GetEnv("ONEFUZZ_RESOURCE_GROUP"); }
128-
public string? OneFuzzTelemetry { get => GetEnv("ONEFUZZ_TELEMETRY"); }
129-
130-
public string OneFuzzVersion {
131-
get {
132-
// version can be overridden by config:
133-
return GetEnv("ONEFUZZ_VERSION")
134-
?? _oneFuzzVersion
135-
?? throw new InvalidOperationException("Unable to read OneFuzz version from assembly");
136-
}
137-
}
97+
public ResourceIdentifier OneFuzzDataStorage { get; } = new(MustGetEnv("ONEFUZZ_DATA_STORAGE"));
13898

139-
public string? OneFuzzAllowOutdatedAgent => GetEnv("ONEFUZZ_ALLOW_OUTDATED_AGENT");
140-
public string OneFuzzStoragePrefix => ""; // in production we never prefix the tables
99+
public ResourceIdentifier OneFuzzFuncStorage { get; } = new(MustGetEnv("ONEFUZZ_FUNC_STORAGE"));
141100

142-
public Uri OneFuzzBaseAddress {
143-
get {
144-
var hostName = Environment.GetEnvironmentVariable("WEBSITE_HOSTNAME");
145-
var scheme = Environment.GetEnvironmentVariable("HTTPS") != null ? "https" : "http";
146-
return new Uri($"{scheme}://{hostName}");
147-
}
148-
}
101+
public Uri OneFuzzInstance { get; } = new Uri(MustGetEnv("ONEFUZZ_INSTANCE"));
102+
103+
public string OneFuzzInstanceName { get; } = MustGetEnv("ONEFUZZ_INSTANCE_NAME");
104+
105+
public Uri? OneFuzzEndpoint { get; } = GetEnv("ONEFUZZ_ENDPOINT") is string value ? new Uri(value) : null;
106+
107+
public string OneFuzzKeyvault { get; } = MustGetEnv("ONEFUZZ_KEYVAULT");
108+
109+
public string? OneFuzzMonitor { get; } = GetEnv("ONEFUZZ_MONITOR");
110+
111+
public string? OneFuzzOwner { get; } = GetEnv("ONEFUZZ_OWNER");
112+
113+
public ResourceIdentifier OneFuzzResourceGroup { get; } = new(MustGetEnv("ONEFUZZ_RESOURCE_GROUP"));
114+
115+
public string? OneFuzzTelemetry { get; } = GetEnv("ONEFUZZ_TELEMETRY");
116+
117+
public string OneFuzzVersion { get; } = GetEnv("ONEFUZZ_VERSION") ?? _oneFuzzVersion;
118+
119+
public string? OneFuzzAllowOutdatedAgent { get; } = GetEnv("ONEFUZZ_ALLOW_OUTDATED_AGENT");
120+
121+
public string OneFuzzStoragePrefix => ""; // in production we never prefix the tables
149122
}

‎src/ApiService/ApiService/onefuzzlib/ConfigOperations.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,12 @@ public ConfigOperations(ILogger<ConfigOperations> log, IOnefuzzContext context,
2626
public Task<InstanceConfig> Fetch()
2727
=> _cache.GetOrCreateAsync(_instanceConfigCacheKey, async entry => {
2828
entry = entry.SetAbsoluteExpiration(TimeSpan.FromMinutes(1)); // cached for 1 minute
29-
var key = _context.ServiceConfiguration.OneFuzzInstanceName ?? throw new Exception("Environment variable ONEFUZZ_INSTANCE_NAME is not set");
29+
var key = _context.ServiceConfiguration.OneFuzzInstanceName;
3030
return await GetEntityAsync(key, key);
3131
})!; // NULLABLE: only this class inserts _instanceConfigCacheKey so it cannot be null
3232

3333
public async Async.Task Save(InstanceConfig config, bool isNew = false, bool requireEtag = false) {
34-
var newConfig = config with { InstanceName = _context.ServiceConfiguration.OneFuzzInstanceName ?? throw new Exception("Environment variable ONEFUZZ_INSTANCE_NAME is not set") };
34+
var newConfig = config with { InstanceName = _context.ServiceConfiguration.OneFuzzInstanceName };
3535
ResultVoid<(HttpStatusCode Status, string Reason)> r;
3636
if (isNew) {
3737
r = await Insert(newConfig);

‎src/ApiService/ApiService/onefuzzlib/Containers.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ public string AuthDownloadUrl(Container container, string filename) {
266266
queryString.Add("container", container.String);
267267
queryString.Add("filename", filename);
268268

269-
return $"{instance}/api/download?{queryString}";
269+
return $"{instance}api/download?{queryString}";
270270
}
271271

272272
public async Async.Task<OneFuzzResultVoid> DownloadAsZip(Container container, StorageType storageType, Stream stream, string? prefix = null) {

‎src/ApiService/ApiService/onefuzzlib/Creds.cs‎

Lines changed: 18 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -50,50 +50,29 @@ public Creds(IServiceConfig config, IHttpClientFactory httpClientFactory, IMemor
5050
_httpClientFactory = httpClientFactory;
5151
_cache = cache;
5252
_azureCredential = new DefaultAzureCredential();
53-
_armClient = new ArmClient(this.GetIdentity(), this.GetSubscription());
53+
_armClient = new ArmClient(GetIdentity(), GetSubscription());
5454

5555
}
5656

57-
public DefaultAzureCredential GetIdentity() {
58-
return _azureCredential;
59-
}
57+
public DefaultAzureCredential GetIdentity() => _azureCredential;
6058

61-
public string GetSubscription() {
62-
var storageResourceId = _config.OneFuzzDataStorage
63-
?? throw new System.Exception("Data storage env var is not present");
64-
return storageResourceId.SubscriptionId
65-
?? throw new Exception("OneFuzzDataStorage did not have subscription ID");
66-
}
59+
public string GetSubscription() => _config.OneFuzzDataStorage.SubscriptionId
60+
?? throw new InvalidOperationException("OneFuzzDataStorage did not have subscription ID");
6761

68-
public string GetBaseResourceGroup() {
69-
var storageResourceId = _config.OneFuzzDataStorage
70-
?? throw new System.Exception("Data storage env var is not present");
71-
return storageResourceId.ResourceGroupName
72-
?? throw new Exception("OneFuzzDataStorage did not have resource group name");
73-
}
62+
public string GetBaseResourceGroup() => _config.OneFuzzDataStorage.ResourceGroupName
63+
?? throw new InvalidOperationException("OneFuzzDataStorage did not have resource group name");
7464

75-
public ResourceIdentifier GetResourceGroupResourceIdentifier() {
76-
var resourceId = _config.OneFuzzResourceGroup
77-
?? throw new System.Exception("Resource group env var is not present");
78-
return new ResourceIdentifier(resourceId);
79-
}
65+
public ResourceIdentifier GetResourceGroupResourceIdentifier() => _config.OneFuzzResourceGroup;
8066

81-
public string GetInstanceName() {
82-
var instanceName = _config.OneFuzzInstanceName
83-
?? throw new System.Exception("Instance Name env var is not present");
67+
public string GetInstanceName() => _config.OneFuzzInstanceName;
8468

85-
return instanceName;
86-
}
69+
public ResourceGroupResource GetResourceGroupResource()
70+
=> ArmClient.GetResourceGroupResource(GetResourceGroupResourceIdentifier());
8771

88-
public ResourceGroupResource GetResourceGroupResource() {
89-
var resourceId = GetResourceGroupResourceIdentifier();
90-
return ArmClient.GetResourceGroupResource(resourceId);
91-
}
72+
public SubscriptionResource GetSubscriptionResource()
73+
=> ArmClient.GetSubscriptionResource(SubscriptionResource.CreateResourceIdentifier(GetSubscription()));
9274

93-
public SubscriptionResource GetSubscriptionResource() {
94-
var id = SubscriptionResource.CreateResourceIdentifier(GetSubscription());
95-
return ArmClient.GetSubscriptionResource(id);
96-
}
75+
public Uri GetInstanceUrl() => _config.OneFuzzEndpoint ?? new($"https://{GetInstanceName()}.azurewebsites.net");
9776

9877
private static readonly object _baseRegionKey = new(); // we only need equality/hashcode
9978
public Async.Task<Region> GetBaseRegion() {
@@ -106,11 +85,6 @@ public Async.Task<Region> GetBaseRegion() {
10685
})!; // NULLABLE: only this method inserts _baseRegionKey so it cannot be null
10786
}
10887

109-
public Uri GetInstanceUrl() {
110-
var onefuzzEndpoint = _config.OneFuzzEndpoint;
111-
return onefuzzEndpoint != null ? new Uri(onefuzzEndpoint) : new($"https://{GetInstanceName()}.azurewebsites.net");
112-
}
113-
11488
public record ScaleSetIdentity(string principalId);
11589

11690
public Async.Task<Guid> GetScalesetPrincipalId() {
@@ -128,16 +102,14 @@ public ResourceIdentifier GetScalesetIdentityResourcePath() {
128102
var scalesetIdName = $"{GetInstanceName()}-scalesetid";
129103
var resourceGroupPath = $"/subscriptions/{GetSubscription()}/resourceGroups/{GetBaseResourceGroup()}/providers";
130104

131-
return new ResourceIdentifier($"{resourceGroupPath}/Microsoft.ManagedIdentity/userAssignedIdentities/{scalesetIdName}");
105+
return new($"{resourceGroupPath}/Microsoft.ManagedIdentity/userAssignedIdentities/{scalesetIdName}");
132106
}
133107

134-
public GenericResource ParseResourceId(ResourceIdentifier resourceId) {
135-
return ArmClient.GetGenericResource(resourceId);
136-
}
108+
public GenericResource ParseResourceId(ResourceIdentifier resourceId)
109+
=> ArmClient.GetGenericResource(resourceId);
137110

138-
public GenericResource ParseResourceId(string resourceId) {
139-
return ArmClient.GetGenericResource(new ResourceIdentifier(resourceId));
140-
}
111+
public GenericResource ParseResourceId(string resourceId)
112+
=> ArmClient.GetGenericResource(new ResourceIdentifier(resourceId));
141113

142114
public async Async.Task<GenericResource> GetData(GenericResource resource) {
143115
if (!resource.HasData) {

‎src/ApiService/ApiService/onefuzzlib/Secrets.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,7 @@ public async Task<Uri> StoreSecret(ISecret secret) {
7575

7676
public Uri GetKeyvaultAddress() {
7777
// https://docs.microsoft.com/en-us/azure/key-vault/general/about-keys-secrets-certificates#vault-name-and-object-name
78-
var keyvaultName = _config!.OneFuzzKeyvault;
79-
return new Uri($"https://{keyvaultName}.vault.azure.net");
78+
return new Uri($"https://{_config.OneFuzzKeyvault}.vault.azure.net");
8079
}
8180

8281

0 commit comments

Comments
 (0)