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

Implement Host header bypassing #155

Merged
merged 6 commits into from Jan 25, 2024
Merged

Implement Host header bypassing #155

merged 6 commits into from Jan 25, 2024

Conversation

ffix
Copy link
Contributor

@ffix ffix commented Feb 12, 2023

This pull request adds the ability to bypass the Host header in Reproxy for compatibility with services like Grafana as mentioned in #133.

It includes the following changes:

  • A global option has been added to enable bypassing the Host header for the entire Reproxy instance.
  • A per-container trigger has been added to enable or disable bypassing the Host header on a per-container basis.

It hasn't been tested with a consul catalog as me don't' have access to one.

I'm not familiar with the coding standards of this project, so I would appreciate any comments or fixes. 

@ffix ffix requested a review from umputun as a code owner February 12, 2023 08:57
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, completely missed the PR. Generally, it's looking good, I'll check it in detail on the weekend

@@ -37,6 +37,7 @@ type URLMapper struct {
PingURL string
MatchType MatchType
RedirectType RedirectType
KeepHost *bool
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why this is a reference? I guess the goal is to have three-state bool here, but why? Default false for the normal bool should be fine i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary reason we use a reference here, and in other discovery providers, is to enable the maintenance of a three-state (true/false/unset) value. If the value is not set, we'll default to the value specified in the global settings.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, this is both on the global level as well on the provider level

One more thing - you don't have any tests for proxy.go changes, could you pls add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@Semior001
Copy link

Is there any chance of being added? Still waiting for being able to use grafana :)

# Conflicts:
#	README.md
#	app/discovery/discovery.go
#	app/discovery/provider/consulcatalog/consulcatalog.go
#	app/discovery/provider/consulcatalog/consulcatalog_test.go
#	app/discovery/provider/docker.go
#	app/discovery/provider/file.go
#	app/discovery/provider/file_test.go
#	app/discovery/provider/testdata/config.yml
#	app/main.go
@umputun
Copy link
Owner

umputun commented Nov 27, 2023

@ffix - I have updated your branch with all the current changes from the master. It had a lot of conflicts, but hopefully, I have them adequately resolved. Pls, take a look and let me know if everything is fine

@MrZoidberg
Copy link

@ffix @umputun any progress with this? tried to run Grafana behind reproxy and failed with same problem. Any help needed?

# Conflicts:
#	README.md
#	app/main.go
@umputun umputun merged commit fe24cf9 into umputun:master Jan 25, 2024
5 checks passed
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.

None yet

4 participants