After upgrade to Ocelot 23.2, `ocelot.global.json` file no longer gets merged correctly

[silviuantochicommify]

Expected Behavior

Data inside ocelot.global.json file should be taken into account when creating the IInternalConfiguration

Actual Behavior

Defaults are being set, the data from the ocelot.global.json is completely ignored (we have a RequestIdKey set and a custom ServiceDiscoveryProvider)

Steps to Reproduce the Problem

1. Create multiple json files inside a folder, each containing different routes
2. Create ocelot.global.json file inside the same folder
3. Use the Configuration.AddOcelot(string folder, IWebHostEnvironment env, MergeOcelotJson.ToMemory) overload
4. At runtime, use the IInternalConfigurationRepository to get the Ocelot configuration.
5. The data found in the InternalConfiguration object regarding global configuration contains default values, instead of the ones from the ocelot.global.json

Specifications

• Version: 23.2.2 on .NET 8
• Platform: Windows

After the upgrade to 23.2.2, the ocelot.global.json no longer gets interpreted when doing the merge, regardless if it's done InMemory or the old way, in the ocelot.json file. Since the deployment will be done in a Kubernetes cluster, we really want to use the InMemory option, but currently, it looks like there's an issue that was introduced with 23.2.2.

Just to make it clear, I've tested with 23.1.0 and everything is working as before, without any other changes to the code outside of removing the MergeOcelotJson from the Program.cs file to make it compile

I'm more than happy to give more details in case it's needed

[silviuantochicommify]

Update: I've managed to make it work if I do this:

builder.Configuration.AddOcelot("GatewayConfiguration", builder.Environment, MergeOcelotJson.ToMemory, globalConfigFile: "GatewayConfiguration/ocelot.global.json");

It looks like the FileInfo instance created in the method GetMergedOcelotJson for the global config file (line 121) doesn't take into account the folder where the ocelot.*.json files are → https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs#L121 So the the current implementation will only work for root folder, since the file name is the same, but the full name is only the same if all the ocelot json files are in the root folder, making the if lower at lines 135-136 always returning false and never applying the configuration → https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs#L135-L136

[raman-m]

4. At runtime, use the IInternalConfigurationRepository to get the Ocelot configuration.
5. The data found in the InternalConfiguration object regarding global configuration contains default values, instead of the ones from the ocelot.global.json

I don't recommend to use IInternalConfigurationRepository object to read configuration which is already in DI: https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/OcelotBuilder.cs#L55 So, use IOptions<FileConfiguration> object for constructor's injection. For route config: use httpContext.Items.DownstreamRoute();

[raman-m]

@silviuantochicommify commented on Jun 3:

It looks like the FileInfo instance created in the method GetMergedOcelotJson for the global config file (line 121) doesn't take into account the folder where the ocelot.*.json files are → https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs#L121

You are wrong! Look at the following lines 112-117: https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs#L112-L117 The expression new DirectoryInfo(folder).EnumerateFiles() gets all files of the folder including the global one, ocelot.global.json! Just debug in runtime and you will see that is true. All files must be located at the same folder.

[raman-m]

So the the current implementation will only work for root folder, since the file name is the same, but the full name is only the same if all the ocelot json files are in the root folder, making the if lower at lines 135-136 always returning false and never applying the configuration → https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs#L135-L136

We will double check this! It can be an issue. I wonder if paths of FullName differ and the condition is false.

[silviuantochicommify]

So the the current implementation will only work for root folder, since the file name is the same, but the full name is only the same if all the ocelot json files are in the root folder, making the if lower at lines 135-136 always returning false and never applying the configuration → https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs#L135-L136

We will double check this! It can be an issue. I wonder if paths of FullName differ and the condition is false.

This is what I wanted to point out. Sorry if I didn't explain it properly initially.

I checked with a very simple console application and this is exactly the case. If you don't include the subfolder when creating the FileInfo, the Name property contains the same value, but FullName differs. I'm guessing that's why using the overload with the <subfolder>/ocelot.global.json works.

[raman-m]

@silviuantochicommify ❗ You have one day to submit a pull request with a hotfix to ensure its inclusion in version 23.3 (the current release). Otherwise, I will address the issue with a potential fix targeted for version 24.0 (the next upcoming release).

Initially, we need to create TDD tests that can replicate the issue in the current development version.

[silviuantochicommify]

@silviuantochicommify ❗ You have one day to submit a pull request with a hotfix to ensure its inclusion in version 23.3 (the current release). Otherwise, I will address the issue with a potential fix targeted for version 24.0 (the next upcoming release).

Initially, we need to create TDD tests that can replicate the issue in the current development version.

Hello @raman-m,

I'm taking some days off, so I won't be able to submit any PR. Best I can do is next week, on Monday.

Thanks

[raman-m]

Silviu, will you be working during these days in July? FYI, the issue has been added to the v23.3.x Hotfixes milestone.

[ben-bartholomew]

I was running into this issue too so I jumped on it to get something in since it was simple. The defect here only mentions running into the problem with the global config file, but the environment and primary config default file paths are built the same way (and so would be incorrectly looked for outside the folder), so I applied the change to those also.