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.

Sign up for GitHub

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

Jump to bottom

feat(client): allow to set a specific sni hostname per request #1171

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

feat(client): allow to set a specific sni hostname per request #1171

wants to merge 1 commit into from

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented Dec 26, 2024 •
edited
Loading

This allow to set a specific hostname for sni, use the request hostname like before by default, but allow to change it if needed.

@joelwurtz joelwurtz marked this pull request as ready for review December 26, 2024 14:49
@joelwurtz joelwurtz force-pushed the feat/client-sni-hostname branch from f1b82f5 to d45c9ff Compare January 9, 2025 13:53
@joelwurtz joelwurtz force-pushed the feat/client-sni-hostname branch 2 times, most recently from 2c9b9e8 to e20d8af Compare February 5, 2025 16:02
@joelwurtz joelwurtz force-pushed the feat/client-sni-hostname branch 2 times, most recently from bf5cce0 to 8f47b24 Compare February 13, 2025 09:32
@fakeshadow
Copy link
Collaborator

fakeshadow commented Feb 14, 2025 •
edited
Loading

Sorry for late review. I have looked into this last month but I cant decide if we should follow this "rule" in all xitca crates:

xitca-web/http/src/http.rs

Line 124 in cec4ef4

// http::Extensions is often brought up as an alternative for extended states but in general

Ideally we want to use http crate for it's public types but also we prefer strongly typed extension than using runtime dynamic type casting. I also can see if we change the client API to use Request<RequestExt<Body>> it can be cumbersome and tiring for library user.

@joelwurtz
Copy link
Contributor Author

Not sure what's the best here, we could either :

  • Use a specific request like the server that add this specific extension instead of the one from http
  • Add specific field to this struct instead of the extensions
  • Or simply make this field external and link it in the service request struct (like the timeout)

It's mainly a api difference when writing middleware, all works, but having those fields in either a specific request or in the extensions of the request reduce future bc break if we want to add more field (so either 1 or 2).

Personally I would go for a specific request struct, where we add all fields that the builder allow to set, extensions point should be only used for data exchange between middleware if needed.

@joelwurtz
Copy link
Contributor Author

There is 3 PR that add information on the service request :

The first two, i did not use the extensions system (because value implement Copy), and put them in the ServiceRequest struct directly, but if we decide to have a specific request struct there may be added there.

Let me know what you want to do for those extra fields and i will adapt others PR accordingly

@joelwurtz joelwurtz force-pushed the feat/client-sni-hostname branch from 8f47b24 to 69fd6bb Compare February 25, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joelwurtz @fakeshadow