Skip to content

Simplify Reverse Proxy Settings#19161

Closed
MikeAlhayek wants to merge 2 commits intomainfrom
ma/fix-forwarded
Closed

Simplify Reverse Proxy Settings#19161
MikeAlhayek wants to merge 2 commits intomainfrom
ma/fix-forwarded

Conversation

@MikeAlhayek
Copy link
Copy Markdown
Member

No description provided.

@MikeAlhayek MikeAlhayek requested a review from gvkries April 16, 2026 22:41
@Skrypt
Copy link
Copy Markdown
Contributor

Skrypt commented Apr 17, 2026

Is it not something that you actually want to have as configurable with appsettings.json file? Now, if the DB service is down then the proxy configurations are too? That's biting it's own tail. It adds a dependency on the proxy too which I think is debatable.

If you look at yarp.net it is all config file based.

@gvkries
Copy link
Copy Markdown
Member

gvkries commented Apr 17, 2026

The reverse proxy can be configured either via the admin dashboard or through appsettings. Configuration via appsettings is opt-in, enabled through an extension method, and takes precedence over the admin UI settings. For details, see the documentation here:
https://docs.orchardcore.net/en/latest/reference/modules/ReverseProxy/ — it’s actually very well documented 🙂

Whether configuration is done through the admin UI or appsettings is governed by the ReverseProxySettings options. Removing them here would therefore break this functionality entirely.

@hishamco
Copy link
Copy Markdown
Member

@gvkries I think we talked about this last time, then we agreed to do what's currently there - no ISiteService, right?

I think you started a PR to introduce an infrastructure configuration that provides different sources

@gvkries
Copy link
Copy Markdown
Member

gvkries commented Apr 17, 2026

Yes, we recently fixed several issues in this area and removed an unnecessary ReverseProxyService.

The ISiteService is used within an IConfigureOptions callback to populate the admin UI settings. A subsequent PostConfigure callback is then registered, which merges those admin UI settings with the configuration coming from appsettings.

Finally, IOptions must be resolved to obtain the effective, combined configuration.

The PR you’re referring to is this one:
#18796

I initially started working on it with the help of AI, but wasn’t sure whether it was worth continuing at the time, so I closed it for now. Maybe this is something we can pick up again for v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants