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

Fixed remote addr (on Caddy reverse proxy) problem #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonirrings
Copy link

please check

close #15

@jonirrings
Copy link
Author

I did not find better way than just adding an else if

to lower case all header names, may bring more operations than an else if and a hashmap.get

do you get any better way?

@SudoDios
Copy link
Collaborator

SudoDios commented Dec 3, 2024

Thank you @jonirrings
There is a better idea.
This problem needs to be solved from the root.
Because problems like this can occur in other cases
This way we will have to write if & else for the rest of our lives 😂.
I created new string type

#[derive(Eq)]
struct UniCaseString(String);

impl PartialEq for UniCaseString {
    fn eq(&self, other: &Self) -> bool {
        self.0.to_lowercase() == other.0.to_lowercase()
    }
}

impl Hash for UniCaseString {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.0.to_lowercase().hash(state);
    }
}

This is better.
What do you think?
Implement this in the code.
If you have any questions, ask.

@jonirrings
Copy link
Author

got it
I'm trying

@jonirrings jonirrings changed the title Fixed remote addr (on Caddy reverse proxy) problem WIP: Fixed remote addr (on Caddy reverse proxy) problem Dec 3, 2024
@jonirrings jonirrings force-pushed the patch-1 branch 3 times, most recently from f34c0c4 to 784834d Compare December 4, 2024 02:22
@jonirrings
Copy link
Author

code updated.
I don't like the .into() at request.rs:L204
should we avoid that?

@@ -201,7 +201,7 @@ where
} else {
let mut header_parts = header_line.splitn(2, ':');
if let (Some(header_key),Some(header_val)) = (header_parts.next(),header_parts.next()) {
headers_out.insert(header_key.trim().to_string(),header_val.trim().to_string());
headers_out.insert(header_key.trim().to_string().into(),header_val.trim().to_string());
Copy link
Author

@jonirrings jonirrings Dec 4, 2024

Choose a reason for hiding this comment

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

any way to avoid the into?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because it's too long?
Remove to_string

Copy link
Author

Choose a reason for hiding this comment

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

😂, because I‘d like to modify code as less as possible.
I'm wondering any implicit coercion, which may be bad practice.

This code seems feasible for me now,
Do you have any other suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

😅
very great 👍.
thank you.
I will be back in just 3 days.
I am on my way to a place where there is no internet.
If you find something yourself, do it and we will apply it

Copy link
Author

Choose a reason for hiding this comment

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

nice~~
enjoy yourself.

@jonirrings jonirrings changed the title WIP: Fixed remote addr (on Caddy reverse proxy) problem Fixed remote addr (on Caddy reverse proxy) problem Dec 4, 2024
@SudoDios
Copy link
Collaborator

SudoDios commented Dec 7, 2024

Hi @jonirrings
Are you sure it's working correctly?
I just tested the code and no headers were being read from the hashmap.
Borrow<str> probably doesn't work properly.

@jonirrings
Copy link
Author

jonirrings commented Dec 9, 2024

sorry for late reply, I was moving house last weekend, 😂no internet either.
my fault, forgot to test after a manual refactor.
I will test and fix this later in the next few days.

@SudoDios
Copy link
Collaborator

SudoDios commented Dec 9, 2024

Okay 👍 😂

@SudoDios
Copy link
Collaborator

@jonirrings How are you?

@jonirrings
Copy link
Author

@SudoDios just fine, busy decorating house these days,😂
I tried somehow, Borrow<str> did not make it.
maybe use some case-insensitve map?

@SudoDios
Copy link
Collaborator

We should use this for a custom hashmap

@jonirrings
Copy link
Author

I prefer https://github.com/philipdaniels/case-insensitive-hashmap , maybe a wrapper won't add up too much size?

@jonirrings
Copy link
Author

I tried the case insensitive hashmap above, it works out as expected, can you test it later?

librespeed-rs behind caddy, http
behind_caddy
librespeed-rs behind caddy, https
behind_caddy_https
librespeed-rs direct connect
direct

It seems Caddy adds a lot of overhead~🤡

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.

How to setup nginx/caddy before librespeed?
2 participants