Skip to content
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

Configuration for use by extensions, custom pipelines, or other extensions #738

Closed
philproctor opened this issue Jan 8, 2019 · 15 comments · Fixed by #1843
Closed

Configuration for use by extensions, custom pipelines, or other extensions #738

philproctor opened this issue Jan 8, 2019 · 15 comments · Fixed by #1843
Labels
Configuration Ocelot feature: Configuration feature A new feature merged Issue has been merged to dev and is waiting for the next release Spring'24 Spring 2024 release
Milestone

Comments

@philproctor
Copy link
Contributor

New Feature

Add some container for storing configuration values into the configuration container into both the GlobalConfiguration and into the individual ReRoute configurations. Additionally, the creator methods for reroutes and possibly other configurations (such as service discovery) should have hooks that provide a mechanism for reloading these configurations on configuration change and adding properties to the configured object (such as the ReRoute object)

Motivation for New Feature

Currently the Ocelot configuration has predefined values based on what Ocelot internally uses. This works for official extensions, however it makes it difficult for third parties to extend with configurations that are not already in the standard FileConfiguration. When an option needs to be applied to an individual reroute, using the standard IConfiguration options from ASP.NET require knowing the array index and other information that may not be easily available.

@philproctor philproctor added the feature A new feature label Jan 8, 2019
@philproctor philproctor self-assigned this Jan 8, 2019
@philproctor philproctor added proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance and removed feature A new feature labels Jan 8, 2019
@margaale
Copy link
Contributor

I really like this feature

@philproctor
Copy link
Contributor Author

@margaale Any ideas as to what that configuration container should look like? That's where I'm a bit unsure as we need some way of providing a JSON Scheme onto FileConfiguration.

My initial thought is it would be a Dictionary<string, Dictionary<string, object>> and the "extension" would receive the Dictionary<string, object> from the key that matches the extension name. So it would be something like this:

"Extensions": {
    "ExtensionName": {
        "Key1": "Value1",
        "Key2": true
    }
}

Then the extension named "ExtensionName" would receive the Dictionary<string, object> that contains "Key1" with the string "Value1" and "Key2" with the boolean "true"

@margaale
Copy link
Contributor

That would be my first choice too, but probably we could Json.Deserialize the configuration into an user defined class

@philproctor
Copy link
Contributor Author

The ASP.NET configuration used doesn't allow that out of the box without defining the structure up front, that I'm aware of. I think in order to do that we'd have to implement our own way of loading the configuration from disk instead of relying on ASP.NET's

@philproctor
Copy link
Contributor Author

In OcelotMiddlewareExtensions.cs we have this:

            // make configuration from file system?
            // earlier user needed to add ocelot files in startup configuration stuff, asp.net will map it to this
            var fileConfig = builder.ApplicationServices.GetService<IOptionsMonitor<FileConfiguration>>();

            // now create the config
            var internalConfigCreator = builder.ApplicationServices.GetService<IInternalConfigurationCreator>();
            var internalConfig = await internalConfigCreator.Create(fileConfig.CurrentValue);

As you can see, it's already been noted that config could be made from the filesystem. If we did that instead, we could deserialize into a JObject easily and allow extensions to define their own configuration structured.

However, this comes with at least two costs... Cons to this approach are:

  • We would have to implement our own file change monitor. The IOptionsMonitor currently takes care of that for us.
  • Current code would break as the previous pattern of loading the config into ASP.NET would no longer work and we would require a config file location in UseOcelot()

@margaale
Copy link
Contributor

margaale commented Jan 10, 2019 via email

@philproctor
Copy link
Contributor Author

I'm leaning towards providing the Dictionary with options, at least with the initial implementation. Perhaps we could allow some further customization in an update, but doing it with the Dictionary requires the least amount of disruption with the current configuration system.

The transform layer would be difficult for any configuration that is not flat, we would need to reload the raw configuration. That may be feasible but would require a lot of mangling of the config, the Dictionary implementation is the simplest short-term to at least prove out the concept.

@margaale
Copy link
Contributor

Sure, let's start with the simple solution, we will have time to further refine it. Tell me if i can help you. I'm gonna give it a shota at the caching issue in the meanwhile

@philproctor
Copy link
Contributor Author

Sounds good, I'll start work on this one soon. I'll probably also pick up #737 as I have found that one would resolve a number of other issues/requests in the backlog.

@philproctor philproctor added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs feedback Issue is waiting on feedback before acceptance labels Jan 11, 2019
@TomPallister
Copy link
Member

This is interesting, I am not really clear what problem it is solving in my head or what it would look like though!

@gchadid
Copy link

gchadid commented Oct 30, 2019

hi, this feature would be great, any update?

@raman-m raman-m removed the accepted Bug or feature would be accepted as a PR or is being worked on label Jun 8, 2023
@raman-m raman-m added feature A new feature accepted Bug or feature would be accepted as a PR or is being worked on labels Dec 11, 2023
@raman-m
Copy link
Member

raman-m commented Dec 11, 2023

@philproctor What's up, Phil?

Your feature has been accepted. So, the feature is in development now officially. Congrats! 🎉
Are you still with Ocelot?

@raman-m raman-m added the 2023 Annual 2023 release label Feb 17, 2024
@raman-m raman-m added this to the Annual 2023 milestone Feb 17, 2024
@raman-m raman-m changed the title Proposal: Configuration for use by extensions, custom pipelines, or other extensions Configuration for use by extensions, custom pipelines, or other extensions Feb 17, 2024
@raman-m raman-m added the Configuration Ocelot feature: Configuration label Feb 17, 2024
@raman-m
Copy link
Member

raman-m commented Mar 5, 2024

@vantm Welcome! You are assigned!

ggnaegi added a commit that referenced this issue May 21, 2024
* feat(configuration): adding route metadata

* feat(configuration): update docs

* feat(configuration): replace Dictionary<> by IDictionary<>, code cleaning

* feat(configuration): replace Dictionary<> by IDictionary<>

* feat(configuration): replace Dictionary<> by IDictionary<>

* feat(configuration): update the data type of FileDynamicRoute Metadata

* formatting

* feat(configuration): fix integration tests

* feat !1843 add extension methods for DownstreamRoute to get metadata

* feat !1843 add extension methods for DownstreamRoute

* feat !1843 update docs

* feat !1843 update docs

* feat !1843 cleanup split string logic

* SA1505: An opening brace should not be followed by a blank line

* IDE1006: Naming rule violation: These words must begin with upper case characters: should_xxx

* Fix compile errors after rebasing

* Fix unit tests + AAA pattern

* First Version, providing a generic extension method GetMetadata<T> with global configuration

* Adding ConvertToNumericType method to be able to use the NumberStyles enum

* adding first acceptance tests

* The tests are now passing again...

* adding latest test cases. That should be enough (includes global configuration changes too)

* Update metadata.rst

* adding the xml docs for IMetadataCreator and MetadataCreator

* renaming MetadataCreator to DefaultMetadataCreator

* number tests for .net 6 too

* Moving Metadata specific downstream route extensions to the Metadata folder

* cleanup

* applying some of the requested changes

* Final code review by @raman-m

* Add traits

* Fix docs build error

---------

Co-authored-by: Raman Maksimchuk <[email protected]>
Co-authored-by: Guillaume Gnaegi <[email protected]>
@raman-m
Copy link
Member

raman-m commented May 21, 2024

Closed by #1843

@raman-m raman-m closed this as completed May 21, 2024
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release Spring'24 Spring 2024 release and removed proposal Proposal for a new functionality in Ocelot accepted Bug or feature would be accepted as a PR or is being worked on 2023 Annual 2023 release labels May 21, 2024
@raman-m raman-m modified the milestones: Annual 2023, Spring'24 May 22, 2024
raman-m added a commit that referenced this issue May 23, 2024
* feat(configuration): adding route metadata

* feat(configuration): update docs

* feat(configuration): replace Dictionary<> by IDictionary<>, code cleaning

* feat(configuration): replace Dictionary<> by IDictionary<>

* feat(configuration): replace Dictionary<> by IDictionary<>

* feat(configuration): update the data type of FileDynamicRoute Metadata

* formatting

* feat(configuration): fix integration tests

* feat !1843 add extension methods for DownstreamRoute to get metadata

* feat !1843 add extension methods for DownstreamRoute

* feat !1843 update docs

* feat !1843 update docs

* feat !1843 cleanup split string logic

* SA1505: An opening brace should not be followed by a blank line

* IDE1006: Naming rule violation: These words must begin with upper case characters: should_xxx

* Fix compile errors after rebasing

* Fix unit tests + AAA pattern

* First Version, providing a generic extension method GetMetadata<T> with global configuration

* Adding ConvertToNumericType method to be able to use the NumberStyles enum

* adding first acceptance tests

* The tests are now passing again...

* adding latest test cases. That should be enough (includes global configuration changes too)

* Update metadata.rst

* adding the xml docs for IMetadataCreator and MetadataCreator

* renaming MetadataCreator to DefaultMetadataCreator

* number tests for .net 6 too

* Moving Metadata specific downstream route extensions to the Metadata folder

* cleanup

* applying some of the requested changes

* Final code review by @raman-m

* Add traits

* Fix docs build error

---------

Co-authored-by: Raman Maksimchuk <[email protected]>
Co-authored-by: Guillaume Gnaegi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Ocelot feature: Configuration feature A new feature merged Issue has been merged to dev and is waiting for the next release Spring'24 Spring 2024 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants