Add `IFeatureService` & Related Utilities #37

justindbaur · 2024-10-18T18:27:10Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-12639

📔 Objective

Adds IFeatureService and related utilities to the Bitwarden.Extensions.Hosting library. This is not an exact copy & paste and would be a breaking change from the current implementation.

One of the biggest improvements this implementation has over the previous one is that it allows for runtime configuration changes that can be immediately consumed. It does this through using IOptionsMonitor<>.

The main consumption point of this API IFeatureService.IsEnabled(string, bool) remains unchanged. The other consumption point [RequireFeature(string)] doesn't change from a use standpoint but behaviorally works differently behind the scenes. It uses middleware instead of an action filter to filter out endpoints that shouldn't be invoked. To use this, you have to add the new middleware UseFeatureChecks() between UseRouting() and UseEndpoints(), this change was made so that we can support minimal API's which do not support action filters. This make it possible to tag and endpoint (or group) with .RequireFeature(string) to do the same thing as the attribute.

The other major breaking change is how known feature flags are added. Currently they are added at globalSettings:launchDarkly:FlagDataFilePath which is supposed to be a path to a file containing flags and their values or at globalSettings:launchDarkly:FlagValues. Instead they now have to be added at Features:FlagValues or through IServiceCollection.AddFeatureFlagValues(IEnumerable<KeyValuePair<string, string>>) this method is meant to replace the this in server. Adding flag values from a file is not supported anymore, the reasoning is to limit decision fatigue. The benefit the file had was that flags could be added there and auto update the feature service, this is now done automatically, all configuration providers that support change detection can be used instead. If you want a file that ONLY contains feature flags you can add another configuration provider.

IFeatureService.GetAll() gets a minor facelift as it now returns a IReadOnlyDictionary<string, JsonValue> instead of Dictionary<string, object>, this is because you should not edit the returned object and JsonValue allows you to more easily know what the type of the value is if you want to consume it.

IFeatureService.IsOnline() is also removed, it's not currently consumed anywhere in server and I don't really understand the need. The IFeatureService takes care of internally making sure that it runs fine while online or offline, it's not a thing consumers should have to check.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

codecov · 2024-10-18T18:29:49Z

Codecov Report

Attention: Patch coverage is 78.89908% with 69 lines in your changes missing coverage. Please review.

Project coverage is 37.88%. Comparing base (a13749d) to head (6c4ac01).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...Hosting/src/Features/LaunchDarklyFeatureService.cs	71.42%	29 Missing and 9 partials ⚠️
...en.Extensions.Hosting/src/Utilities/VersionInfo.cs	71.42%	9 Missing and 1 partial ⚠️
...Hosting/src/Utilities/HostEnvironmentExtensions.cs	43.75%	7 Missing and 2 partials ⚠️
...en.Extensions.Hosting/src/HostBuilderExtensions.cs	70.37%	7 Missing and 1 partial ⚠️
...ons.Hosting/src/Features/FeatureCheckMiddleware.cs	92.15%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #37       +/-   ##
===========================================
+ Coverage   14.71%   37.88%   +23.17%     
===========================================
  Files          30       38        +8     
  Lines         836     1127      +291     
  Branches       71       99       +28     
===========================================
+ Hits          123      427      +304     
+ Misses        695      666       -29     
- Partials       18       34       +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

justindbaur · 2024-10-18T18:30:42Z

extensions/Bitwarden.Extensions.Hosting/src/HostBuilderExtensions.cs

-    /// <summary>
-    /// Gets a logger that is suitable for use during the bootstrapping (startup) process.
-    /// </summary>
-    /// <returns></returns>
-    public static ILogger GetBootstrapLogger()
-    {
-        return new LoggerConfiguration()
-            .MinimumLevel.Override("Microsoft", LogEventLevel.Information)
-            .Enrich.FromLogContext()
-            .WriteTo.Console()
-            .CreateBootstrapLogger();
-    }
-


Not used anymore.

github-actions · 2024-10-18T18:30:45Z

Checkmarx One – Scan Summary & Details – 1c992839-0173-4761-80ab-fb03b110b0b5

No New Or Fixed Issues Found

justindbaur · 2024-10-18T18:31:13Z

extensions/Bitwarden.Extensions.Hosting/src/HostBuilderExtensions.cs

+            {
+                builder.WriteTo.Console();
+            }


The compact formatter is pretty hard to use locally, we should keep it just for production scenarios.

withinfocus

Few comments and questions but these are all great changes as I see it, and this helps clean up some of the original unknowns that we did or didn't put into practice e.g. IsOnline.

My only lingering thought is about how we establish the "known feature flags" and at the scale some projects will need. Yes, it's bad to have too many of these around but there are a lot in server today and it seems a little awkward for this to be in the service configuration / startup area of code.

withinfocus · 2024-10-22T18:08:53Z

extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureApplicationBuilderExtensions.cs

+    /// </summary>
+    /// <param name="app">The <see cref="IApplicationBuilder"/> to add the middleware to.</param>
+    /// <returns>A reference to <paramref name="app"/> after the operation has completed.</returns>
+    public static IApplicationBuilder UseFeatureChecks(this IApplicationBuilder app)


💭 on including "flag" in the language here.

I like it, I changed it. Do you think we should also name the namespace/folder FeatureFlags instead of Features?

I am flip-flopping a bit in my head over this. We have "feature checks" and we "require features", but we use feature flags to do that. This may be overcomplicating it and we just call everything one or another. Would like to hear others' opinions.

withinfocus · 2024-10-22T18:10:53Z

extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureCheckMiddleware.cs

+    {
+        if (_logger.IsEnabled(LogLevel.Debug))
+        {
+            _logger.LogFailedFeatureCheck(failedFeature.ToString() ?? "Unknown Check");


❓ How could failedFeature return a null ToString()?

Technically ToString is marked as a nullable return: https://github.com/dotnet/runtime/blob/9ecef8c2d709b88b959d9d6a00ad62a8f72a094f/src/libraries/System.Private.CoreLib/src/System/Object.cs#L39

I kinda doubt we will run into that scenario though, I'm happy to ! away the compiler warning instead.

Yeah, seems cleaner to me. I like having less hardcoded strings in all this.

withinfocus · 2024-10-22T18:12:08Z

extensions/Bitwarden.Extensions.Hosting/src/Features/FeatureCheckMiddleware.cs

+        problemDetails.Title = "Resource not found.";
+        problemDetails.Status = StatusCodes.Status404NotFound;
+
+        // Message added for legacy reasons. We should start preferring title/detail


❓ Legacy? So Message isn't common anymore?

Yeah, a lot of things in ASP.NET Core will return a ProblemDetails object unless we specifically go and and change it. So my preference would be to also make that be our return for problems. That way we can start on a unified error format.

It also sends back application/problem+json as the content type so you can introspect on that more on the client side to trust that if you got a bad status code and the content type indicates problem details, you can read the respond body like it's a problem details object and then return a more high quality error.

Ah, I like that. Do we just get rid of the message then? I'd like to not carry in debt and just expect callers to adjust when they adopt this library.

withinfocus · 2024-10-22T18:21:24Z

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs

+        var httpContext = _httpContextAccessor.HttpContext;
+        if (httpContext == null)
+        {
+            // Likely a mistake, should we log a warning?


ℹ️ I would say yes -- the context should never be null, we just will have some calls that don't have a client type e.g. a user.

withinfocus · 2024-10-22T18:22:31Z

extensions/Bitwarden.Extensions.Hosting/examples/MinimalApi/Program.cs

+
+app.MapGet("/", (IConfiguration config) => ((IConfigurationRoot)config).GetDebugView());
+
+app.MapGet("/requires-feature", (IFeatureService featureService) =>


🌱 Should this example code be expected to declare known flags?

It's declaring its known flags in appsettings.Development.json.

withinfocus · 2024-10-22T18:23:44Z

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs

+        var organizations = httpContext.User.FindAll("organization");
+
+        var contextBuilder = Context.Builder(subject)
+            .Kind(ContextKind.Default) // TODO: This is not right


❓ What's not right?

So this ties into your comment below about organization and service accounts missing. This is wrong because it hard codes user and I don't want it to. I totally missed putting this in my PR description because I meant to go back to this.

I want to design this in such a way that this services doesn't have to be intimately aware of all the token types and claims structures our application has or could have down the road. Instead I want all the format to have to conform to a pattern that this understands (or just be alright with it not having as much context). It feels a lot more stable in the long run but it requires changes before you could start using this. What I did for now, sub and organization (where multiple are allowed), but I would also need a claim to indicate the context kind, sub_type maybe? If we dont' think claims are the best place for this then maybe a request feature? https://learn.microsoft.com/en-us/aspnet/core/fundamentals/request-features?view=aspnetcore-8.0

Request features feel pretty low-level and not about business logic to me, and I think this needs to be a claim. I thought we had that already but I guess not.

withinfocus · 2024-10-22T18:25:32Z

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs

+
+        return contextBuilder.Build();
+    }
+}


🎨 Organization and service account client types are missing.

withinfocus · 2024-10-22T18:26:28Z

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs

+                .ApplicationId(_hostEnvironment.ApplicationName)
+                .ApplicationName(_hostEnvironment.ApplicationName)
+                .ApplicationVersion(AssemblyHelpers.GetGitHash() ?? $"v{AssemblyHelpers.GetVersion()}")
+            // .ApplicationVersionName(AssemblyHelpers.GetVersion()) THIS DOESN'T WORK


❓ What doesn't work?

Actually looks like both version and version name don't work. I get this error:

2024-10-23 05:14:01.341 -04:00 [ApplicationInfoBuilder] WARN: Issue setting ApplicationVersion to value 'v1.0.0+8956cdbdcdef6de8baa0384fa2b6897ddf6abc5f'. Contains invalid characters. 2024-10-23 05:14:01.342 -04:00 [ApplicationInfoBuilder] WARN: Issue setting ApplicationVersionName to value '1.0.0+8956cdbdcdef6de8baa0384fa2b6897ddf6abc5f'. Contains invalid characters.

It doesn't block the use of the service though.

Hmm, I went and looked at versions e.g. https://app.launchdarkly.com/settings/applications/Api/versions (you probably can't load this) and it seems fine:

Per https://docs.launchdarkly.com/home/observability/config-deployment#set-the-application-version I am not sure if we're actually doing something incorrectly.

Ahh, it's because we are doing it in a different format, I was kinda confused as to how we were getting any git info at all in this project because we don't have all the stuff making it work in server here. But we have it because it was added as built-in to .NET 8 SDK.

I think we should lean on the built in format and just change the parsing of it in here.

I agree, let's enhance our version access here in the library.

Updated it, we do now get the full hash vs before we were trimming it to only 8 characters, would you want to trim to 8 with the new way too?

I suppose we could offer a new property for the trimmed version too, but their docs indicate the full hash is best so I think we leave this implementation as-is.

withinfocus · 2024-10-22T18:27:19Z

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs

+    private TestData BuildDataSource(Dictionary<string, string> data)
+    {
+        _loggerFactory.CreateLogger("Test").LogWarning("KnownValues: {Count}", data.Count);
+        // TODO: We could support updating just the test data source with


ℹ️ I am not sure LD's SDK allows that to change once it's constructed.

I'm going off a comment in the first example here: https://launchdarkly.github.io/dotnet-server-sdk/pkgs/sdk/server/api/LaunchDarkly.Sdk.Server.Integrations.TestData.html

But also that example wouldn't compile, so mileage may vary.

extensions/Bitwarden.Extensions.Hosting/src/Features/LoggerExtensions.cs

…ensions.cs Co-authored-by: Matt Bishop <[email protected]>

justindbaur · 2024-10-23T09:35:01Z

My only lingering thought is about how we establish the "known feature flags" and at the scale some projects will need. Yes, it's bad to have too many of these around but there are a lot in server today and it seems a little awkward for this to be in the service configuration / startup area of code.

@withinfocus I think we will need to first decide how a /config endpoint will work with multiple services. Will we only have a single service with a single config endpoint or will every service be responsible for their own. The known flags are only needed to make the GetAll() method work and that method is, at least currently, only used to make the config endpoint work. Also, I toyed around with a different name for this: ClientFlags because that is technically what it is. We don't have any need to send down certain flags to the client, because they only need to be consumed in the server. With this change, that becomes possible (it's possible today, just against convention).

The thing I just definitely wanted to avoid was having this library become the repository of all feature flags. So I designed it that it could be config based but we could also just call services.AddKnownFeatureFlags(FeatureFlagKeys.GetAllKeys()) and the current status quo of adding feature flags would continue to work. They would technically be added in startup but they way people interact with it would still be at compile time.

withinfocus · 2024-10-23T16:05:54Z

Left a few more comments.

I'm thinking that each service owns how something might ask it for flags and their values. We did this so that our clients can use our API instead of an (expensive) LD client SDK primarily.

withinfocus

Couple more things we might want to adjust, or merge now and expect related changes elsewhere and then follow up.

extensions/Bitwarden.Extensions.Hosting/src/Utilities/VersionInfo.cs

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs

withinfocus

There are several to-dos here, but we can iterate. What's most important to me at the moment is the new claim we need for the identity client type.

Add IFeatureService & Utilities

335c51a

justindbaur commented Oct 18, 2024

View reviewed changes

justindbaur requested a review from withinfocus October 18, 2024 18:31

justindbaur marked this pull request as ready for review October 18, 2024 18:31

justindbaur requested review from a team as code owners October 18, 2024 18:31

justindbaur requested a review from addisonbeck October 18, 2024 18:31

justindbaur changed the title ~~Add IFeatureService & Utilities~~ Add IFeatureService & Related Utilities Oct 18, 2024

withinfocus reviewed Oct 22, 2024

View reviewed changes

justindbaur and others added 3 commits October 23, 2024 04:25

Log Warning On Missing HttpContext

ae3cf97

UseFeatureChecks -> UseFeatureFlagChecks

49cf70c

Update extensions/Bitwarden.Extensions.Hosting/src/Features/LoggerExt…

8956cdb

…ensions.cs Co-authored-by: Matt Bishop <[email protected]>

justindbaur requested a review from withinfocus October 23, 2024 09:35

Remove Test Logger

96d17ad

withinfocus reviewed Oct 23, 2024

View reviewed changes

justindbaur added 5 commits October 23, 2024 16:56

Remove "Backup" String

af18b29

Handle New Versioning Format

86a3888

Format Test Project

4e4208d

Format Source Project

aedfad4

Format Examples

9b2c6c4

withinfocus reviewed Oct 25, 2024

View reviewed changes

extensions/Bitwarden.Extensions.Hosting/src/Utilities/VersionInfo.cs Show resolved Hide resolved

extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Outdated Show resolved Hide resolved

Make GitHash Optional

6c4ac01

withinfocus approved these changes Oct 25, 2024

View reviewed changes

justindbaur merged commit cc49c76 into main Oct 25, 2024
14 checks passed

justindbaur deleted the add-feature-flags branch October 25, 2024 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `IFeatureService` & Related Utilities #37

Add `IFeatureService` & Related Utilities #37

justindbaur commented Oct 18, 2024 •

edited by withinfocus

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

justindbaur Oct 18, 2024

github-actions bot commented Oct 18, 2024 •

edited

Loading

justindbaur Oct 18, 2024

withinfocus left a comment

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

withinfocus Oct 23, 2024

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

withinfocus Oct 23, 2024

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

withinfocus Oct 23, 2024

withinfocus Oct 22, 2024

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

withinfocus Oct 23, 2024

withinfocus Oct 22, 2024

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

withinfocus Oct 23, 2024

justindbaur Oct 24, 2024

withinfocus Oct 24, 2024

justindbaur Oct 25, 2024

withinfocus Oct 25, 2024

withinfocus Oct 22, 2024

justindbaur Oct 23, 2024

justindbaur commented Oct 23, 2024

withinfocus commented Oct 23, 2024

withinfocus left a comment

withinfocus left a comment


		app.MapGet("/", (IConfiguration config) => ((IConfigurationRoot)config).GetDebugView());

		app.MapGet("/requires-feature", (IFeatureService featureService) =>

Add IFeatureService & Related Utilities #37

Add IFeatureService & Related Utilities #37

Conversation

justindbaur commented Oct 18, 2024 • edited by withinfocus Loading

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2024 • edited Loading

No New Or Fixed Issues Found

Choose a reason for hiding this comment

withinfocus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justindbaur commented Oct 23, 2024

withinfocus commented Oct 23, 2024

withinfocus left a comment

Choose a reason for hiding this comment

withinfocus left a comment

Choose a reason for hiding this comment

Add `IFeatureService` & Related Utilities #37

Add `IFeatureService` & Related Utilities #37

justindbaur commented Oct 18, 2024 •

edited by withinfocus

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading