-
Notifications
You must be signed in to change notification settings - Fork 280
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.
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
Adding support ConfigurationManager based config #366
Conversation
src/CoreWCF.ConfigurationManager/src/ConfigurationManagerExtensions.cs
Outdated
Show resolved
Hide resolved
src/CoreWCF.ConfigurationManager/src/WrappedConfigurationFile.cs
Outdated
Show resolved
Hide resolved
src/CoreWCF.ConfigurationManager/src/WrappedConfigurationFile.cs
Outdated
Show resolved
Hide resolved
namespace CoreWCF.Configuration | ||
{ | ||
public interface IConfigurationHolder | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've got the model slightly wrong with regards the IConfigureOptions feature of DI. This is similar to what ServiceModelOptions should look like. But instead of being methods liks AddBinding, it should have properties which are added to such as IList for the bindings. I'm also looking at XmlConfigEndpoint/IXmlConfigEndpoint and I think these might be able to be unified in a cleaner way. Once I've read more of the code I'll come up with a suggestion on how this could look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been going back and forth on how to approach this. This configuration mechanism using IConfigureOptions kind of mirrors the UseServiceModel apis which takes a delegate which is passed a IServiceBuilder instance where you define the service. The difference is the convention with IConfigureOptions is to have the options type be a concrete class implementation. So I took a look at how the KestrelServerOptions is used. In NetTcpFramingOptionsSetup.Configure
we configure an instance of KestrelServerOptions. We've previously populated an endpoint list on our own internal configuratopm class NetTcpFramingOptions via calls to UseNetTcp. Basically if you call UseNetTcp with two different ports, it will have to endpoints listed in NetTcpFramingOptions.Endpoints. We iterate over the endpoints and call KestrelServerOptions.Listen for each one in turn. The code looks like this:
foreach (IPEndPoint endpoint in _options.EndPoints)
{
options.Listen(endpoint, builder =>
{
builder.UseConnectionHandler<NetMessageFramingConnectionHandler>();
NetMessageFramingConnectionHandler handler = builder.ApplicationServices.GetRequiredService<NetMessageFramingConnectionHandler>();
// Save the ListenOptions to be able to get final port number for adding BaseAddresses later
ListenOptions.Add(builder);
});
}
We should be going for a similar model. The method ConfigurationManagerServiceModelOptions.Configure should have some way to parse the configuration into some kind of object model. The ConfigurationHolder class looks like a good candidate. This class should be internal though. It could either be done in Configure with a method call, or via something that is injected. Then the this object model is iterated over and ServiceModelOptions populated with a more typical set of objects which reflect closely to what IServiceBuilder does. At service startup, CoreWCF should get the ServiceModelOptions and iterate over it's data to set things up. So ConfigurationManagerServiceModelOptions.Configure would look something like this:
public void Configure(ServiceModelOptions options)
{
var configHolder = ParseConfig();
foreach (var endpointName in configHolder.Endpoints.Keys) // expose ConfigurationHolder._endpoints as property
{
var configEndpoint = configHolder.GetXmlConfigEndpoint(endpointName);
options.ConfigureService(configEndpoint.Service, serviceConfig =>
{
serviceConfig.ConfigureServiceEndpoint(endpoint.Service, endpoint.Contract, endpoint.Binding, endpoint.Address, null);
});
}
}
Having ServiceModelOptions and another class ServiceConfigurationBuilder be public types which the ServiceBuilder can pull from DI and use to build the service allows a future capability to add other config options such as JSON. The classes would look like this and live in primitives.
public class ServiceModelOptions
{
private Dictionary<Type, ServiceConfigurationBuilder> _config = new Dictionary<Type, ServiceConfigurationBuilder>();
public void ConfigureService(Type serviceType, Action<ServiceConfigurationBuilder> configure)
{
ServiceConfigurationBuilder configBuilder;
if (!_config.TryGetValue(serviceType, out configBuilder))
{
configBuilder = new ServiceConfigurationBuilder(serviceType);
_config[serviceType] = configBuilder;
}
configBuilder.AddConfigureDelegate(configure);
}
internal void ConfigureServiceBuilder(IServiceBuilder serviceBuilder)
{
foreach (var serviceConfigBuilder in _config.Values)
{
serviceConfigBuilder.ConfigureServiceBuilder(serviceBuilder);
}
}
}
public class ServiceConfigurationBuilder
{
private List<Action<ServiceConfigurationBuilder>> _configDelegates = new List<Action<ServiceConfigurationBuilder>>();
private Dictionary<Type, List<(Type implementedContract, Binding binding, Uri address, Uri listenUri)>> _endpoints;
internal Type ServiceType { get; }
public ServiceConfigurationBuilder(Type serviceType)
{
ServiceType = serviceType;
}
public void AddConfigureDelegate(Action<ServiceConfigurationBuilder> configDelegate)
{
_configDelegates.Add(configDelegate);
}
public void ConfigureServiceEndpoint(Type service, Type implementedContract, Binding binding, Uri address, Uri listenUri)
{
List<(Type implementedContract, Binding binding, Uri address, Uri listenUri)> serviceEndpoints;
if (!_endpoints.TryGetValue(service, out serviceEndpoints))
{
serviceEndpoints = new List<(Type implementedContract, Binding binding, Uri address, Uri listenUri)>();
_endpoints[service] = serviceEndpoints;
}
serviceEndpoints.Add((implementedContract, binding, address, listenUri));
}
internal void ConfigureServiceBuilder(IServiceBuilder serviceBuilder)
{
_endpoints = new Dictionary<Type, List<(Type implementedContract, Binding binding, Uri address, Uri listenUri)>>();
foreach (var configDelegate in _configDelegates)
{
configDelegate(this);
}
foreach (var service in _endpoints)
{
serviceBuilder.AddService(service.Key);
serviceBuilder.AddServiceEndpoint(service.Key, service.Value.implementedContract, service.Value.binding, service.Value.address, service.Value.listenUri);
}
_endpoints = null;
}
}
Then in ServiceBuilder where is creates the service, right at the top it grabs from DI ServiceModelOptions and calls ServiceModelOptions.ConfigureServiceBuilder(this)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I wrote this code in the GitHub comment box so there might be typos or some errors in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mconnew @Ximik87 should we expose another method inside ServiceConfigurationBuilder as ConfigureBehavior(), which would be called from ConfigurationManagerServiceModelOptions.Configure (if behaviors present) and set up the ServiceBehaviors and EndPointBehaviors.. this would enable the TransportWithMessageCred etc... let me know what you both think...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I have little experience with IConfigureOptions and I did not fully understand how it works correctly.
@mconnew , I did as you described, and it works except ServiceModelOptions.ConfigureServiceBuilder (this)
because - AddService in Configure (app, env) calls ServiceModelOptions.ConfigureServiceBuilder (this),
this calls ServiceConfigurationBuilder.onfigureServiceBuilder () and this calls AddService again and so recursively ...
I made an extension method for IServiceBuilder that explicitly calls ServiceModelOptions.ConfigureServiceBuilder and it seems to work, but check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@birojnayak, Yes, as I understand it Matt - all ServiceBehaviors and EndPointBehaviors settings will be read in ConfigurationManagerServiceModelOptions.Configure
but we will use them in the ServiceConfigurationBuilder, make a separate method for this - I don't understand yet, I have to start trying coding
src/CoreWCF.ConfigurationManager/src/CoreWCF/Configuration/ICreateBinding.cs
Outdated
Show resolved
Hide resolved
src/CoreWCF.ConfigurationManager/src/CoreWCF/Configuration/NotFoundBindingException.cs
Outdated
Show resolved
Hide resolved
src/CoreWCF.ConfigurationManager/src/CoreWCF/Configuration/NotFoundEndpointException.cs
Outdated
Show resolved
Hide resolved
src/CoreWCF.ConfigurationManager/src/CoreWCF/Configuration/ServiceDefaults.cs
Show resolved
Hide resolved
...CF.ConfigurationManager/src/CoreWCF/Configuration/ConfigurationManagerServiceModelOptions.cs
Show resolved
Hide resolved
src/CoreWCF.ConfigurationManager/src/CoreWCF/Configuration/WrappedConfigurationFile.cs
Outdated
Show resolved
Hide resolved
...CF.ConfigurationManager/src/CoreWCF/Configuration/ConfigurationManagerServiceModelOptions.cs
Show resolved
Hide resolved
namespace CoreWCF.Configuration | ||
{ | ||
public interface IConfigurationHolder | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mconnew @Ximik87 should we expose another method inside ServiceConfigurationBuilder as ConfigureBehavior(), which would be called from ConfigurationManagerServiceModelOptions.Configure (if behaviors present) and set up the ServiceBehaviors and EndPointBehaviors.. this would enable the TransportWithMessageCred etc... let me know what you both think...
<system.serviceModel>
<services>
<service name="Service" behaviorConfiguration="ServiceBehavior">
<!-- Service Endpoints -->
<endpoint address="" binding="basicHttpBinding" contract="IService">
<!--
Upon deployment, the following identity element should be removed or replaced to reflect the
identity under which the deployed service runs. If removed, WCF will infer an appropriate identity
automatically.
-->
<!-- <identity>
<dns value="tlocalhost"/>
</identity>-->
</endpoint>
<endpoint address="mex" binding="mexHttpBinding" contract="MetadataExchange"/>
</service>
</services>
<behaviors>
<serviceBehaviors>
<behavior name="ServiceBehavior">
<!-- To avoid disclosing metadata information, set the value below to false before deployment -->
<serviceMetadata httpGetEnabled="true"/>
<!-- To receive exception details in faults for debugging purposes, set the value below to true. Set to false before deployment to avoid disclosing exception information -->
<serviceDebug includeExceptionDetailInFaults="false"/>
</behavior>
</serviceBehaviors>
</behaviors>
</system.serviceModel> I was doing some testing with the changes by creating a default WCF service. The endpoint object (corewcf.configurations) doesn't have "identity" (please create an Issue, to separately handle) and importantly since there are 2 end points (with empty names), the ConfigurationManager is dying saying the entry " has already been added... see if that can be fixed. |
@Ximik87 are you working on this PR ? Or if you want I can take it forward from here to bring closure and incorporate review comments... let me know.. |
@birojnayak yes, i'm working, don't close this PR, Matt gave a lot of comments about the code, and it will take some time |
I'm a bit confused about the why and what for of this PR. Is this to consume a web.config/app.config of the running process or just consume any file with a Why just not |
Ideally, something like this would be enough: var configXml = @"<configuration>
<system.serviceModel>
<services>
<service name=""Service"" behaviorConfiguration=""ServiceBehavior"">
<!-- Service Endpoints -->
<endpoint address="""" binding=""basicHttpBinding"" contract=""IService"">
<!--
Upon deployment, the following identity element should be removed or replaced to reflect the
identity under which the deployed service runs. If removed, WCF will infer an appropriate identity
automatically.
-->
<!-- <identity>
<dns value=""tlocalhost""/>
</identity>-->
</endpoint>
<endpoint address=""mex"" binding=""mexHttpBinding"" contract=""MetadataExchange""/>
</service>
</services>
<behaviors>
<serviceBehaviors>
<behavior name=""ServiceBehavior"">
<!-- To avoid disclosing metadata information, set the value below to false before deployment -->
<serviceMetadata httpGetEnabled=""true""/>
<!-- To receive exception details in faults for debugging purposes, set the value below to true. Set to false before deployment to avoid disclosing exception information -->
<serviceDebug includeExceptionDetailInFaults=""false""/>
</behavior>
</serviceBehaviors>
</behaviors>
</system.serviceModel>
</configuration>";
var config = new ConfigurationBuilder()
.AddXmlStream(new MemoryStream(Encoding.UTF8.GetBytes(configXml)))
.Build(); But the first problem here is the |
This is a naïf and not performant way of sourcing the configuration from `/configuration/system.serviceModel´: var configItems = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
var xml = new XmlDocument();
xml.LoadXml(configXml);
var serviceModel = xml.SelectSingleNode("//system.serviceModel");
foreach (XmlNode service in serviceModel.SelectNodes("services/service"))
{
var serviceConfigurationRoot = $"serviceModel{ConfigurationPath.KeyDelimiter}services{ConfigurationPath.KeyDelimiter}{service.Attributes["name"]!.Value}{ConfigurationPath.KeyDelimiter}";
foreach (XmlAttribute attribute in service.Attributes)
{
if (attribute.Name != "name")
{
configItems.Add($"{serviceConfigurationRoot}{attribute.Name}", attribute.Value);
}
}
foreach (XmlNode endpoint in service.SelectNodes("endpoint"))
{
var endpointConfigurationRoot = $"{serviceConfigurationRoot}endpoints{ConfigurationPath.KeyDelimiter}{endpoint.Attributes["address"]!.Value}{ConfigurationPath.KeyDelimiter}";
foreach (XmlAttribute attribute in endpoint.Attributes)
{
if (attribute.Name != "address")
{
configItems.Add($"{endpointConfigurationRoot}{attribute.Name}", attribute.Value);
}
}
}
}
var config = new ConfigurationBuilder()
.AddInMemoryCollection(configItems)
.Build(); The result will be:
Loading this JSON configuration would yield the same result: {
"ServiceModel" : {
"Services" : {
"Service": {
"Endpoints": {
"mex": {
"Contract": "MetadataExchange",
"Binding": "mexHttpBinding",
}
"": {
"Contract": "IService",
"Binding": "ServiceBehavior",
}
}
}
}
}
} Now, what's missing is an options model. |
@paulomorgado, firstly, this is not a refinement of the ConfigurationBuilder as you described, secondly, it is not as simple as you gave in the example implementation, because the serviceModel |
It's not a refinement of I did say that the implementation was "naïf and not performant". |
Here is the logging set up from configuration: https://source.dot.net/#Microsoft.Extensions.Logging.Configuration/ILoggerProviderConfigurationFactory.cs And where it is used in the host builder: https://source.dot.net/#Microsoft.Extensions.Hosting/HostingHostBuilderExtensions.cs,238 |
@birojnayak yes, this PR lacks support for behavior, because I don't understand much how to do it in the code. If you start doing it well, I think you can handle it better |
@Ximik87 I will create an issue once this is merged for behavior support |
src/CoreWCF.ConfigurationManager/tests/CoreWCF.ConfigurationManager.Tests.csproj
Outdated
Show resolved
Hide resolved
I have a few more tweaks to clean up the code and api design a little and then this will be good. I'll try and push that in the next hour or so. |
@Ximik87, I have another commit for you to add to your branch. You can find it here. Basically I made it so you add using the config file in the ConfigureServices method and simply use app.UseServiceModel() in your Configure method. I removed any concept of Xml based configuration from Primitives. I made concrete implementations of interfaces internal. I also modified a few method names to make their intent a little clearer. Feel free to comment on any of my changes if you think it should be a different way. |
@Ximik87, this needs to be rebased on top of main as it looks like there's some conflict now. |
@mconnew Great, i applied these changes manually. I completely agree that the logic for working with XmlConfig should be concentrated in ConfigurationManager project and Primitives should not contain unnecessary code. |
@Ximik87, I just noticed one small problem, a misspelling in one of the file names. Once that's fixed I'll rebase and merge. It would be great to have a blog post about this as part of the release giving a few examples of using the new configuration manager api. Would you be interested in writing that? It doesn't need to be super long. If you don't have the time, that's okay and I'll write one. |
@Ximik87, I just noticed you haven't fixed your branch to be mergable because of conflicts with the main branch. Can you do that? |
@Ximik87, I'd just like to say thank you so much for this contribution. I believe it's going to have some significant impact for many people. |
@mconnew Of course, here's an example blog post Added support for configuration loaded from the classic app.config. Of course, not all options are supported, but this should help when migrating WCF applications.
Next, we create an ASP .Net Core application, add the CoreWCF.Primitives, CoreWCF.NetTcp and CoreWCF.ConfigurationManager nuget packages. Configure services and applications:
Note that if NetTcpBinding is used, then the ports of endpoints using this binding must be additionally specified when configuring WebHostBuilder:
Ready! If there are any configuration problems in the wcf.config file, this will be visible when the application starts. List of supported attributes: |
* Adding support ConfigurationManager config * Add project for CoreWCF.sln and fix file structure includeproject
This feature adding support ConfigurationManager config files from .Net Framework. (Issues #129 )
Now is supported (basic scenario):
BindingsSection - BasicHttpBinding, NetHttpBinding, NetTcpBinding, WSHttpBinding
ServicesSection - Service and endpoint