Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
9069aad
support app config server side schema
zhiyuanliang-ms Nov 22, 2023
1547ee5
avoid naming with server-side
zhiyuanliang-ms Nov 23, 2023
8a8ca97
fix bug
zhiyuanliang-ms Nov 23, 2023
e4f89f7
testcase added
zhiyuanliang-ms Nov 24, 2023
b5b859d
reorg the code
zhiyuanliang-ms Nov 24, 2023
7933989
update the naming
zhiyuanliang-ms Nov 28, 2023
cd3b3f5
fix typo
zhiyuanliang-ms Nov 28, 2023
1270e37
adjust indentation
zhiyuanliang-ms Nov 28, 2023
da89f81
resolve comments
zhiyuanliang-ms Nov 29, 2023
bae3270
revert unexpected change
zhiyuanliang-ms Nov 30, 2023
8c13231
adjust the logic of get feature flag definition section
zhiyuanliang-ms Nov 30, 2023
f54b66a
revert changes
zhiyuanliang-ms Nov 30, 2023
c4cfcf3
fix bug
zhiyuanliang-ms Nov 30, 2023
8baa4c7
resolve comments
zhiyuanliang-ms Dec 1, 2023
7d73b06
resolved comments
zhiyuanliang-ms Dec 2, 2023
dca69d3
update
zhiyuanliang-ms Dec 2, 2023
9d21987
update
zhiyuanliang-ms Dec 4, 2023
729af66
make schema selection thread-safe
zhiyuanliang-ms Dec 4, 2023
1e41ac1
update testcases
zhiyuanliang-ms Dec 4, 2023
66e3841
update naming
zhiyuanliang-ms Dec 4, 2023
646be65
update
zhiyuanliang-ms Dec 4, 2023
ed5ba39
resolve comments
zhiyuanliang-ms Dec 5, 2023
fb75dfa
thread-safe
zhiyuanliang-ms Dec 6, 2023
5048f80
Merge branch 'main' into zhiyuanliang/support-server-side-schema
zhiyuanliang-ms Dec 7, 2023
a4805a3
improvement
zhiyuanliang-ms Dec 8, 2023
421eb69
resolve comments
zhiyuanliang-ms Dec 9, 2023
0f4a889
use lock
zhiyuanliang-ms Dec 12, 2023
1a565a1
resolve comments
zhiyuanliang-ms Dec 13, 2023
29a6aed
Merge branch 'main' into zhiyuanliang/support-server-side-schema
zhiyuanliang-ms Dec 13, 2023
c0e9db6
Merge branch 'main' into zhiyuanliang/support-server-side-schema
zhiyuanliang-ms Dec 19, 2023
b54142c
Merge branch 'main' into zhiyuanliang/support-server-side-schema
zhiyuanliang-ms Jan 23, 2024
c12feef
rename to Microsoft Feature Flag schema
zhiyuanliang-ms Jan 23, 2024
ebf9bd2
Merge branch 'zhiyuanliang/support-server-side-schema' of https://git…
zhiyuanliang-ms Jan 23, 2024
05dd60a
Merge branch 'main' into zhiyuanliang/support-server-side-schema
zhiyuanliang-ms Jan 24, 2024
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//

namespace Microsoft.FeatureManagement
{
//
// Azure App Configuration feature flag schema: https://github.com/Azure/AppConfiguration/blob/main/docs/FeatureManagement/FeatureFlag.v1.1.0.schema.json
internal static class AzureAppConfigurationFeatureFlagConfigurationFields
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
{
public const string FeatureFlagsSectionName = "FeatureFlags";

public const string Id = "id";
public const string Enabled = "enabled";
public const string Conditions = "conditions";
public const string ClientFilters = "client_filters";
public const string RequirementType = "requirement_type";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@ public sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionP
// IFeatureDefinitionProviderCacheable interface is only used to mark this provider as cacheable. This allows our test suite's
// provider to be marked for caching as well.

private const string FeatureFiltersSectionName = "EnabledFor";
private const string RequirementTypeKeyword = "RequirementType";
private const string FeatureManagementSectionName = "FeatureManagement";
private readonly IConfiguration _configuration;
private readonly ConcurrentDictionary<string, FeatureDefinition> _definitions;
private IDisposable _changeSubscription;
private int _stale = 0;
private bool _azureAppConfigurationFeatureFlagSchemaEnabled;

/// <summary>
/// Creates a configuration feature definition provider.
Expand Down Expand Up @@ -113,16 +111,23 @@ public async IAsyncEnumerable<FeatureDefinition> GetAllFeatureDefinitionsAsync()
// Iterate over all features registered in the system at initial invocation time
foreach (IConfigurationSection featureSection in GetFeatureDefinitionSections())
{
string featureName = GetFeatureFlagSectionName(featureSection);

if (string.IsNullOrEmpty(featureName))
{
continue;
}

//
// Underlying IConfigurationSection data is dynamic so latest feature definitions are returned
yield return _definitions.GetOrAdd(featureSection.Key, (_) => ReadFeatureDefinition(featureSection));
yield return _definitions.GetOrAdd(featureName, (_) => ReadFeatureDefinition(featureSection));
}
}

private FeatureDefinition ReadFeatureDefinition(string featureName)
{
IConfigurationSection configuration = GetFeatureDefinitionSections()
.FirstOrDefault(section => section.Key.Equals(featureName, StringComparison.OrdinalIgnoreCase));
.FirstOrDefault(section => string.Equals(GetFeatureFlagSectionName(section), featureName, StringComparison.OrdinalIgnoreCase));

if (configuration == null)
{
Expand All @@ -133,6 +138,16 @@ private FeatureDefinition ReadFeatureDefinition(string featureName)
}

private FeatureDefinition ReadFeatureDefinition(IConfigurationSection configurationSection)
{
if (_azureAppConfigurationFeatureFlagSchemaEnabled)
{
return ParseAppConfigurationFeatureDefinition(configurationSection);
}

return ParseFeatureDefinition(configurationSection);
}

private FeatureDefinition ParseFeatureDefinition(IConfigurationSection configurationSection)
{
/*

Expand Down Expand Up @@ -167,15 +182,17 @@ We support

*/

RequirementType requirementType = RequirementType.Any;
string featureName = GetFeatureFlagSectionName(configurationSection);

var enabledFor = new List<FeatureFilterConfiguration>();

RequirementType requirementType = RequirementType.Any;

string val = configurationSection.Value; // configuration[$"{featureName}"];

if (string.IsNullOrEmpty(val))
{
val = configurationSection[FeatureFiltersSectionName];
val = configurationSection[ConfigurationFields.FeatureFiltersSectionName];
}

if (!string.IsNullOrEmpty(val) && bool.TryParse(val, out bool result) && result)
Expand All @@ -193,64 +210,182 @@ We support
}
else
{
string rawRequirementType = configurationSection[RequirementTypeKeyword];
string rawRequirementType = configurationSection[ConfigurationFields.RequirementType];

//
// If requirement type is specified, parse it and set the requirementType variable
if (!string.IsNullOrEmpty(rawRequirementType) && !Enum.TryParse(rawRequirementType, ignoreCase: true, out requirementType))
{
throw new FeatureManagementException(
FeatureManagementError.InvalidConfigurationSetting,
$"Invalid requirement type '{rawRequirementType}' for feature '{configurationSection.Key}'.");
FeatureManagementError.InvalidConfigurationSetting,
$"Invalid value '{rawRequirementType}' for '{ConfigurationFields.RequirementType}' field of feature '{featureName}'.");
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
}

IEnumerable<IConfigurationSection> filterSections = configurationSection.GetSection(FeatureFiltersSectionName).GetChildren();
IEnumerable<IConfigurationSection> filterSections = configurationSection.GetSection(ConfigurationFields.FeatureFiltersSectionName).GetChildren();

foreach (IConfigurationSection section in filterSections)
{
//
// Arrays in json such as "myKey": [ "some", "values" ]
// Are accessed through the configuration system by using the array index as the property name, e.g. "myKey": { "0": "some", "1": "values" }
if (int.TryParse(section.Key, out int i) && !string.IsNullOrEmpty(section[nameof(FeatureFilterConfiguration.Name)]))
if (int.TryParse(section.Key, out int _) && !string.IsNullOrEmpty(section[ConfigurationFields.NameKeyword]))
{
enabledFor.Add(new FeatureFilterConfiguration()
{
Name = section[nameof(FeatureFilterConfiguration.Name)],
Parameters = new ConfigurationWrapper(section.GetSection(nameof(FeatureFilterConfiguration.Parameters)))
Name = section[ConfigurationFields.NameKeyword],
Parameters = new ConfigurationWrapper(section.GetSection(ConfigurationFields.FeatureFilterConfigurationParameters))
});
}
}
}

return new FeatureDefinition()
{
Name = configurationSection.Key,
Name = featureName,
EnabledFor = enabledFor,
RequirementType = requirementType
};
}

private IEnumerable<IConfigurationSection> GetFeatureDefinitionSections()
private FeatureDefinition ParseAppConfigurationFeatureDefinition(IConfigurationSection configurationSection)
{
/*

If Azure App Configuration feature flag schema is enabled, we support

FeatureFlags: [
{
id: "myFeature",
enabled: true,
conditions: {
client_filters: ["myFeatureFilter1", "myFeatureFilter2"],
requirement_type: "all",
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
}
},
{
id: "myAlwaysEnabledFeature",
enabled: true,
conditions: {
client_filters: [],
}
},
{
id: "myAlwaysDisabledFeature",
enabled: false,
}
]

*/

string featureName = GetFeatureFlagSectionName(configurationSection);

var enabledFor = new List<FeatureFilterConfiguration>();

RequirementType requirementType = RequirementType.Any;

IConfigurationSection conditions = configurationSection.GetSection(AzureAppConfigurationFeatureFlagConfigurationFields.Conditions);

string rawRequirementType = conditions[AzureAppConfigurationFeatureFlagConfigurationFields.RequirementType];

//
// Look for feature definitions under the "FeatureManagement" section
IConfigurationSection featureManagementConfigurationSection = _configuration.GetSection(FeatureManagementSectionName);
// If requirement type is specified, parse it and set the requirementType variable
if (!string.IsNullOrEmpty(rawRequirementType) && !Enum.TryParse(rawRequirementType, ignoreCase: true, out requirementType))
{
throw new FeatureManagementException(
FeatureManagementError.InvalidConfigurationSetting,
$"Invalid value '{rawRequirementType}' for '{AzureAppConfigurationFeatureFlagConfigurationFields.RequirementType}' field of feature '{featureName}'.");
}

if (featureManagementConfigurationSection.Exists())
string rawEnabled = configurationSection[AzureAppConfigurationFeatureFlagConfigurationFields.Enabled];

bool enabled = false;
Comment thread
jimmyca15 marked this conversation as resolved.

if (!string.IsNullOrEmpty(rawEnabled) && !bool.TryParse(rawEnabled, out enabled))
{
return featureManagementConfigurationSection.GetChildren();
throw new FeatureManagementException(
FeatureManagementError.InvalidConfigurationSetting,
$"Invalid value '{rawEnabled}' for '{AzureAppConfigurationFeatureFlagConfigurationFields.Enabled}' field of feature '{featureName}'.");
}

if (enabled)
{
IEnumerable<IConfigurationSection> filterSections = conditions.GetSection(AzureAppConfigurationFeatureFlagConfigurationFields.ClientFilters).GetChildren();

if (filterSections.Any())
{
foreach (IConfigurationSection section in filterSections)
{
//
// Arrays in json such as "myKey": [ "some", "values" ]
// Are accessed through the configuration system by using the array index as the property name, e.g. "myKey": { "0": "some", "1": "values" }
if (int.TryParse(section.Key, out int _) && !string.IsNullOrEmpty(section[ConfigurationFields.NameKeyword]))
{
enabledFor.Add(new FeatureFilterConfiguration()
{
Name = section[ConfigurationFields.NameKeyword],
Parameters = new ConfigurationWrapper(section.GetSection(ConfigurationFields.FeatureFilterConfigurationParameters))
Comment thread
zhiyuanliang-ms marked this conversation as resolved.
Outdated
});
}
}
}
else
{
enabledFor.Add(new FeatureFilterConfiguration
{
Name = "AlwaysOn"
});
}
}

return new FeatureDefinition()
{
Name = featureName,
EnabledFor = enabledFor,
RequirementType = requirementType
};
}

private IEnumerable<IConfigurationSection> GetFeatureDefinitionSections()
{
IConfigurationSection featureManagementConfigurationSection = _configuration.GetSection(ConfigurationFields.FeatureManagementSectionName);
Comment thread
jimmyca15 marked this conversation as resolved.
Outdated

if (!featureManagementConfigurationSection.Exists())
{
if (!RootConfigurationFallbackEnabled)
{
Logger?.LogDebug($"No configuration section named '{ConfigurationFields.FeatureManagementSectionName}' was found.");

return Enumerable.Empty<IConfigurationSection>();
}

featureManagementConfigurationSection = _configuration as IConfigurationSection;
}

IConfigurationSection featureFlagsConfigurationSection = featureManagementConfigurationSection.GetSection(AzureAppConfigurationFeatureFlagConfigurationFields.FeatureFlagsSectionName);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is assigned to with an as implying this can be null. Looks like a possible null-ref.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, I didn't understand. _configuration cannot be null, otherwise an exception will be thrown in the constructor.
IConfiguration can always be casted to IConfigurationSection. I think there could not be a null-ref.


//
// There is no "FeatureManagement" section in the configuration
if (RootConfigurationFallbackEnabled)
// "FeatureFlag" section should be an array if Azure App Configuration feature flag schema is enabled
_azureAppConfigurationFeatureFlagSchemaEnabled = featureFlagsConfigurationSection.Exists() &&

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I left another comment about this. This is not designed well to be assigned to multiple times. This class is designed as thread safe. Meaning this value can change while another thread is somewhere having previously evaluated it.

Either a bigger redesign is needed, or we need to assign to this only once.

@jimmyca15 jimmyca15 Dec 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The latest update does not solve the problem.

Once a public method is invoked and takes an action based off the schema, it should stick with that schema the entire time. Otherwise unexpected things will happen.

Originally I was thinking to just limit the schema to not be able to change during the runtime of the application. Is this acceptable?

If not, then we can have two separate internal feature definition providers that we invoke in the ConfigurationFeatureDefinitionProvider based off of the detected schema.

  • DefaultConfigurationFeatureDefinitionProvider
  • AacConfigurationFeatureDefinitionProvider

With each child provider understanding only it's own schema.

@jimmyca15 jimmyca15 Dec 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if (_aacProvider.HasFlags)
{
    _aacProvider.Xyz();
}
else
{
    _defaultProvider.Xyz();
}

_configuration.OnChange(() =>
{
    _aacProvider.MarkStale();

    _defaultProvider.MarkStale();
});

@zhiyuanliang-ms zhiyuanliang-ms Dec 5, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Originally I was thinking to just limit the schema to not be able to change during the runtime of the application. Is this acceptable?
It depends on whether we consider the scenario that configuration is dynamically changed to a different schemas during the runtime, is a valid user case.

Richard mentioned this potential user case:
#319 (comment)

I am thinking whether we should allow both schemas to be recognized at the same time.

If we allow two schemas to co-exist, we no longer need to have a volatile private property to indicate which schema is used. All configuration sections will be parsed as they are.

string.IsNullOrEmpty(featureFlagsConfigurationSection.Value) &&
featureFlagsConfigurationSection.GetChildren()
.All(section => int.TryParse(section.Key, out int _));

if (_azureAppConfigurationFeatureFlagSchemaEnabled)
{
return _configuration.GetChildren();
return featureFlagsConfigurationSection.GetChildren();
}

Logger?.LogDebug($"No configuration section named '{FeatureManagementSectionName}' was found.");
return featureManagementConfigurationSection.GetChildren();
}

return Enumerable.Empty<IConfigurationSection>();
private string GetFeatureFlagSectionName(IConfigurationSection section)
{
if (_azureAppConfigurationFeatureFlagSchemaEnabled)
{
return section[AzureAppConfigurationFeatureFlagConfigurationFields.Id];
}

return section.Key;
}
}
}
20 changes: 20 additions & 0 deletions src/Microsoft.FeatureManagement/ConfigurationFields.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//

namespace Microsoft.FeatureManagement
{
internal static class ConfigurationFields
{
// Enum keywords
public const string RequirementType = "RequirementType";

// Feature filters keywords
public const string FeatureFiltersSectionName = "EnabledFor";
public const string FeatureFilterConfigurationParameters = "Parameters";

// Other keywords
public const string NameKeyword = "Name";
public const string FeatureManagementSectionName = "FeatureManagement";
}
}
Loading