microsoft/service-fabric-aspnetcore

Feature Request: Service Fabric configuration source

Closed this issue ยท 18 comments

Just wondering if the configuration source implementation for Service Fabric could be included in this repo and published as a separate package. If so, I could prepare a PR from the one I did in aspnet/Configuration#642

@DixonDs Definitely, this would be great to have. It's been a fairly popular way of consuming SF config in ASP.NET Core services. Looking at your other PR, there are a few additional things we'd want to have to fully support SF config packages:

  1. Ability to consume configuration settings from multiple configuration packages. Services can have an arbitrary number of config packages, each with their own settings.xml. The key in the Data dictionary would then be something like this: ConfigPackageName/SectionName/ParameterName
  2. Parameters in settings.xml may be encrypted. This should support reading both encrypted and plaintext values.
  3. The config provider should be able to consume events when the config package is updated.
  4. Unit tests

@MikeWasson wrote up a great blog post on this with some sample code that does 2 and 3 - I would recommend taking a look at that for sure.

@DixonDs Just checking if you have any plans for submitting a PR for this?

@amanbha no, at least not anytime soon, I guess.

any update for this feature request?

@chuanboz At this point this is low in priority because of other high priority work items for ServiceFabric team. You can use the code in the PR mentioned in the first post of the issue, feel free to send us.a PR with the changes for the merge back.

sure, I will try to provide a PR that address the 4 points @vturecek mentioned. I will start to work on this now and try to get it by the end of May.

@vturecek๏ผŒ regarding "Parameters in settings.xml may be encrypted. This should support reading both encrypted and plaintext values."

I see the sample from @MikeWasson handle the encrypted string as follow

Data[$"{section.Name}:{param.Name}"] = param.IsEncrypted ? param.DecryptValue().ToUnsecureString() : param.Value;

that seems violate the security guidelines. A typical safety guideline is to keep them encrypted in memory, and then decrypt (briefly) at time of use.

so I would suggest to treat encrypted and plain text value the same in ServiceFabricConfigurationProvider, and give guideline to the config user to decrypt them at the time of their usage briefly to mitigate the security risk.

One more thing while I am looking at implement this, I see the Settings.xml format has some limitation that it only support section/parameter 2 levels, if use Json file as the configuration file then it adds much flexibility.

As Service Fabric also supports Data Packages which could be any format, one possibility I see, would be to also add the Json file format to this configuration source using Data Package, with the file name supplied by the caller. Any feedback for that?

@chuanboz As far as I remember, normally implementations flatten the configuration sources with multiple levels using ":" as the delimiter.See https://github.com/aspnet/Configuration/blob/dc6fbde39f532bf154ee1eb02d8be08a0a1605e4/src/Config.Abstractions/ConfigurationPath.cs and its usages in implementations. So basically it allows emulating multiple levels in configuration sources like environment variables, for instance.

@chuanboz Having it in Config package in settings.xml allows it to be overwritten at application creation time by application parameters.

I've created a new issue #52 to describe the Json configuration scenario requirement.
@DixonDs, thanks for reference ":" support, I see that could make the hierarchy settings work in parameter but that seems like a hack and kind of ugly than the nature Json file experience.

@amanbha, the built-in override experience is not that ideally for us, see detail that I describe in issue #52 .

for now I will use this issue here for the Settings.xml support and will send PR soon.

@chuanboz Is the PR up? I have written code which does this I can send the PR if you haven't started working on it and I can incorporate feedback you have based on your learnings.

the PR is here: #58
there are open issue raised by Aman I haven't had time to resolve yet.

I've updated the PR #58 to address the issue raised by Aman. @RamjotSingh , can you also take a look of the PR and see if that will also work for your scenario?

@amanbha , I see version 3.2.187 of Microsoft.ServiceFabric.AspNetCore.* is released 15 days ago, but that does not include this new Microsoft.ServiceFabric.AspNetCore.Configuration package, is there any further work needed to get Microsoft.ServiceFabric.AspNetCore.Configuration package released?

also, which github repo shall I use to fire a document issue to get the usage of this new configuration package publically documented in https://docs.microsoft.com?

@chuanboz 3.2.*187 was just a cumulative release and only contained bug fixes. Our current release (3.3 sdk) is already in lockdown, so it will be part of 3.4 sdk release. I will see if I can make a one off release for this package, once 3,4 is out. For documentation, please send email to @amanbha and @vturecek