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

Use merged ocelot.json directly from memory instead of from file #1216

Closed
ebjornset opened this issue Apr 29, 2020 · 5 comments · Fixed by #1227
Closed

Use merged ocelot.json directly from memory instead of from file #1216

ebjornset opened this issue Apr 29, 2020 · 5 comments · Fixed by #1227
Assignees
Labels
bug Identified as a potential bug Configuration Ocelot feature: Configuration feature A new feature Feb'24 February 2024 release merged Issue has been merged to dev and is waiting for the next release
Milestone

Comments

@ebjornset
Copy link
Contributor

ebjornset commented Apr 29, 2020

New Feature

The ConfigurationBuilderExtensions.AddOcelot method merges the ocelot.*.json files into one ocelot.json file that is written back to disk and then added to the IConfigurationBuilder.

Would it be possible to make another AddOcelot method that adds the merged JSON directly from memory into the IConfigurationBuilder using builder.AddJsonStream instead?

Motivation for New Feature

From time to time we experience a problem at startup where something prevents the server process from writing the merged ocelot.json back to disk. When this happens we get an UnauthorizedAccessException (example below) at startup and the process must be killed.

We have not been able to see a consistent pattern in the behavior, and we don't have access to the server since IT operation is outsourced, so this is very hard to debug.

Exception System.UnauthorizedAccessException

Access to the path C:\Octopus\Applications\Dev\Xxxxxxxx\20200429.1_5\ocelot.json is denied.

   at System.IO.File.WriteAllText(String path, String contents)
   at Ocelot.DependencyInjection.ConfigurationBuilderExtensions.AddOcelot(IConfigurationBuilder builder, String folder, IWebHostEnvironment env)
   at Xxxxxxxx.Program.<>c.<CreateHostBuilder>b__1_2(WebHostBuilderContext hostingContext, IConfigurationBuilder config)
   at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.<>c__DisplayClass8_0.<ConfigureAppConfiguration>b__0(HostBuilderContext context, IConfigurationBuilder builder)
   at Microsoft.Extensions.Hosting.HostBuilder.BuildAppConfiguration()
   at Microsoft.Extensions.Hosting.HostBuilder.Build()`
@pablo0219
Copy link

pablo0219 commented Jun 10, 2020

I really believe that this issue is very important. I'm experimenting this issue when deploying to an App Service in Azure. I have different microservices and ocelot.json is overridden every time. Of course, I'm getting UnauthorizedAccessException. I know, Ocelot was thought first for containers, but that approach has its own challenges when working in development. I think, Ocelot shouldn't change ocelot.json in any way (should be kept in memory) and if some kind of persistence needs to be done, it should be in some kind of DB.

@raman-m raman-m added feature A new feature bug Identified as a potential bug labels Jul 28, 2023
@raman-m
Copy link
Member

raman-m commented Aug 1, 2023

@ebjornset commented on Apr 29, 2020

Eirik,
Thanks for your interest in Ocelot!

I believe this feature will be a great enhancement of Ocelot core.
Yeah, merging configs to a file works in non-virtual environments like IIS, Kestrel, Docker where we have real file system.
You've pointed the attention to virtual environments like all cloud providers like AWS, Azure and GCP.

My opinion, it would be better to introduce a feature switch between non-virtual and virtual environments to control file system and config merging. And, right, merging in a memory will be an option, for sure.

Also, pay attention we have some PRs which are similar to your one, because they solve the same problematic by merging in a memory. See the following:

Let's discuss more...

@raman-m
Copy link
Member

raman-m commented Aug 1, 2023

@pablo0219 commented on Jun 10, 2020

Hi Pablo!
Thanks for your ideas!

I'm not sure about DB usage here for this issue. But you are welcome to create new PR to present real implementation which will use DB decoupling by interface.

And, yes, in any virtual environment (all cloud providers) we have virtual file system, and finally we get the exception you've mentioned.
My suggestion is decoupling of file operations by introducing a new file service which must handle all real and virtual file operations to abstract on file storage. And that must solve the problem with exceptions during operations of file system.
And for sure, we have to implement file storages components for all cloud providers.

Your opinion here?

@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Aug 1, 2023
@raman-m
Copy link
Member

raman-m commented Aug 1, 2023

+ Accepted

...due to ready PR #1227

@raman-m raman-m added Feb'24 February 2024 release Configuration Ocelot feature: Configuration labels Feb 23, 2024
@raman-m raman-m added this to the February'24 milestone Feb 24, 2024
@raman-m
Copy link
Member

raman-m commented Mar 12, 2024

@ebjornset @pablo0219
Are you online?
Are you still with Ocelot?
If Yes, could you review current code of #1227 plz? Any your feedback is welcome!

@raman-m raman-m changed the title Use merged ocelot.json directly from memory instead of from file Use merged ocelot.json directly from memory instead of from file Mar 12, 2024
raman-m added a commit that referenced this issue Mar 15, 2024
… configuration files to memory (#1227)

* Added new ConfigurationBuilderExtensions.AddOcelot
overload to support merging of configuration files to memory

* Fix warnings

* File-scoped namespaces

* Update configuration.rst

Use '^' for subsubsections.
Sections: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections

* Review the code after merge and refactor

* Remove and sort usings

* Fix warnings

* Tried to fix

* A small but funny refactoring

* Update configuration.rst

* More refactoring: add overloaded extensions

* Review helpers of unit tests

* Add `FileUnitTest` and full rewrite of config tests

* Refactor `DiskFileConfigurationRepository` class.
Make unit tests parallel, independent, and thread safe

* some minor changes

* Review and update docs of the Configuration feature

* Review and update docs of the `Dependency Injection` feature

* Check references for `Dependency Injection` feature aka `ref:` and `doc:`

* Update configuration.rst

Replace "kind" with "type"

---------

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
Co-authored-by: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug Configuration Ocelot feature: Configuration feature A new feature Feb'24 February 2024 release merged Issue has been merged to dev and is waiting for the next release
Projects
None yet
3 participants