Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

HTTP/2 to upstream#771

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
hongzhidao wants to merge4 commits intonginx:master
base:master
Choose a base branch
Loading
fromhongzhidao:http2-upstream

Conversation

hongzhidao
Copy link

@hongzhidaohongzhidao commentedJul 7, 2025•
edited
Loading

TODO:
buffering
cache

chronolaw reacted with thumbs up emoji
@hongzhidaohongzhidao marked this pull request as draftJuly 7, 2025 03:10
@hongzhidaohongzhidao changed the titleHttp2 upstreamHTTP/2 to upstreamJul 7, 2025
@Maryna-f5Maryna-f5 added this to thenginx-1.29.2 milestoneJul 7, 2025
@hongzhidaohongzhidaoforce-pushed thehttp2-upstream branch 3 times, most recently from5dedbca tocc47b74CompareJuly 8, 2025 02:53
@hongzhidao
Copy link
Author

Hi@arut,
I left some comments on the first commit, please check them.

@hongzhidaohongzhidaoforce-pushed thehttp2-upstream branch 2 times, most recently from66338f9 to63a769cCompareJuly 8, 2025 07:14
@arut
Copy link
Contributor

arut commentedJul 8, 2025

We must avoid sending theConnection header for HTTP/2, as well as for HTTP/3. This is a strictly HTTP/1 header and it makes the request malformed. That's why I added a variable$proxy_internal_connection for this header which evaluates depending on a new context fieldconnection_type and it can be 0. This field is set by the protocol handler. This should probably be a part of the first patch.

hongzhidao reacted with eyes emoji

@hongzhidaohongzhidaoforce-pushed thehttp2-upstream branch 2 times, most recently from8e79530 toa1c2cd9CompareJuly 8, 2025 22:30
Copy link
Member

@ac000ac000 left a comment

Choose a reason for hiding this comment

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

Hi@hongzhidao just some minor things I noticed...

    HTTP/2: ngx_http_v2_proxy_module.    The module allows to use HTTP/2 protocol for proxying.    HTTP/2 proxying is enabled by specifying "proxy_http_version 2.0".    ...

Blank line between the subject and body...

...diff --git a/src/http/v2/ngx_http_v2_proxy_module.c b/src/http/v2/ngx_http_v2_proxy_module.cnew file mode 100644index 0000000000..a69ffcc406--- /dev/null+++ b/src/http/v2/ngx_http_v2_proxy_module.c...+ngx_int_t+ngx_http_v2_proxy_handler(ngx_http_request_t *r)+{...+#if (NGX_HTTP_SSL)+        u->ssl = plcf->ssl;++        if (u->ssl) {+            ngx_str_set(&u->schema, "https://");++        } else {+            ngx_str_set(&u->schema, "https://");+        }+#else+        ngx_str_set(&u->schema, "https://");+#endifMaybe I'm not understanding what the checks for 'ssl' here are for but inthe non-ssl cases shouldn't the schema be 'http://' ? (Otherwise all thechecks seem redundant as we set 'https://' regardless...)...+static ngx_int_t+ngx_http_v2_proxy_body_output_filter(void *data, ngx_chain_t *in)+{...+        /*+         * We have already got the response and were sending some additionals/were/we're/+         * control frames.  Even if there is still something unsent, stop+         * here anyway.+         */...ngx_http_v2_proxy_parse_frame(ngx_http_request_t *r,+    ngx_http_v2_proxy_ctx_t *ctx, ngx_buf_t *b)+{...+        /* suppress warning */+        case ngx_http_v2_proxy_st_payload:+        case ngx_http_v2_proxy_st_padding:+            break;You could just use 'default:' here. Of course you won't get warned if you addmore enum values and don't handle them, but maybe you aren't planning onadding any more or it's not a concern...+        }...+static ngx_int_t+ngx_http_v2_proxy_parse_header(ngx_http_request_t *r,+    ngx_http_v2_proxy_ctx_t *ctx, ngx_buf_t *b)+{...+    if (state < sw_fragment) {++        if (b->last - b->pos < (ssize_t) ctx->rest) {Or even (ptrdiff_t) to be more explicit about _why_ we're casting...+            last = b->last;...+    if (state == sw_padding) {++        if (b->last - b->pos < (ssize_t) ctx->rest) {... and here ... and likely a few other places...+            ctx->rest -= b->last - b->pos;...

@hongzhidao
Copy link
Author

Hi@ac000,
Thanks for the catching.
I borrowed most of the code fromngx_http_grpc_module to make it work.

  1. About the scheme setting.
+#if (NGX_HTTP_SSL)+        u->ssl = plcf->ssl;++        if (u->ssl) {+            ngx_str_set(&u->schema, "https://");++        } else {+            ngx_str_set(&u->schema, "https://");+        }+#else+        ngx_str_set(&u->schema, "https://");+#endif

I feel it should be touched again, I'm looking into@arut implementation about the proxy http3.

  1. I'd like to discuss the others based onngx_http_grpc_module. And we can keep them consistent.
+static ngx_int_t+ngx_http_v2_proxy_body_output_filter(void *data, ngx_chain_t *in)+{...+        /*+         * We have already got the response and were sending some additionals/were/we're/+         * control frames.  Even if there is still something unsent, stop+         * here anyway.+         */

@arut
Copy link
Contributor

arut commentedJul 9, 2025

I feel it should be touched again, I'm looking into@arut implementation about the proxy http3.

ngx_http_proxy_module already has code for this, namely thengx_http_proxy_eval() function. You just need to call it, see the v3 proxy. Both http and https schemas should be supported in HTTP/2. HTTP/3 only supports https.

@hongzhidaohongzhidaoforce-pushed thehttp2-upstream branch 2 times, most recently fromc32eeeb to731645cCompareJuly 15, 2025 11:18
@hongzhidao
Copy link
Author

ngx_http_proxy_module already has code for this, namely thengx_http_proxy_eval() function. You just need to call it, see the v3 proxy. Both http and https schemas should be supported in HTTP/2. HTTP/3 only supports https.

Hi@arut, take a look plz, did you mean something like this?

diff --git a/src/http/v2/ngx_http_v2_proxy_module.c b/src/http/v2/ngx_http_v2_proxy_module.cindex 6a4a54a..cc3b4c2 100644--- a/src/http/v2/ngx_http_v2_proxy_module.c+++ b/src/http/v2/ngx_http_v2_proxy_module.c@@ -232,18 +232,9 @@ ngx_http_v2_proxy_handler(ngx_http_request_t *r)     if (plcf->proxy_lengths == NULL) {         ctx->host = plcf->host;-+        u->schema = plcf->vars.schema; #if (NGX_HTTP_SSL)         u->ssl = plcf->ssl;--        if (u->ssl) {-            ngx_str_set(&u->schema, "https://");--        } else {-            ngx_str_set(&u->schema, "https://");-        }-#else-        ngx_str_set(&u->schema, "https://"); #endif     } else {@@ -252,6 +243,10 @@ ngx_http_v2_proxy_handler(ngx_http_request_t *r)         }     }+    if (u->ssl) {+        ngx_str_set(&u->ssl_alpn_protocol, NGX_HTTP_V2_ALPN_PROTO);+    }+     u->output.tag = (ngx_buf_tag_t) &ngx_http_v2_proxy_module;     u->conf = &plcf->upstream;@@ -266,8 +261,6 @@ ngx_http_v2_proxy_handler(ngx_http_request_t *r)     u->input_filter = ngx_http_v2_proxy_filter;     u->input_filter_ctx = ctx;-    ngx_str_set(&u->ssl_alpn_protocol, NGX_HTTP_V2_ALPN_PROTO);-     r->request_body_no_buffering = 1;     rc = ngx_http_read_client_request_body(r, ngx_http_upstream_init);

@hongzhidao
Copy link
Author

We must avoid sending theConnection header for HTTP/2, as well as for HTTP/3. This is a strictly HTTP/1 header and it makes the request malformed. That's why I added a variable$proxy_internal_connection for this header which evaluates depending on a new context fieldconnection_type and it can be 0. This field is set by the protocol handler. This should probably be a part of the first patch.

Good to learn, I added it into the proxy refactoring patch.

Comment on lines 4410 to 4430
#if (NGX_HTTP_V2)

if (plcf->http_version == NGX_HTTP_VERSION_20) {

if (u.family != AF_UNIX) {

if (u.no_port) {
plcf->host = u.host;

} else {
plcf->host.len = u.host.len + 1 + u.port_text.len;
plcf->host.data = u.host.data;
}

} else {
ngx_str_set(&plcf->host, "localhost");
}
}

#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is almost identical to what we have inngx_http_proxy_set_vars(). We can use host fromctx->host_header. In fact, it's used by default if using HTTP/1 headersngx_http_proxy_headers /ngx_http_proxy_cache_headers instead of grpc-specificngx_http_grpc_headers, which you are already using.

These header sets are not fully compatible though. For example, the following header is empty in HTTP/1:

    { ngx_string("TE"), ngx_string("$grpc_internal_trailers") },

We may think about adding something similar tong_http_proxy_module, but do it natively, NOT under HTTP/2 ifdef. I think this deserves a separate patch. And I'm not sure if we want client trailers to be sent by default to upstream. Anyway, this looks like a separate feature we can think about later.

@hongzhidaohongzhidaoforce-pushed thehttp2-upstream branch 3 times, most recently from4ca718e to87fb0efCompareJuly 15, 2025 15:59
@arut
Copy link
Contributor

This commit is prepared for HTTP/2 and HTTP/3 support.        This resolves SSL context sharing issues between sibling locations with    different HTTP protocol versions. Previously, SSL_CTX was shared between    locations but ALPN protocol was set at SSL context level, causing conflicts    when different locations used different HTTP versions.        The ALPN protocol is now set per-connection in    ngx_http_upstream_ssl_init_connection(), allowing proper protocol negotiation    for each individual upstream connection regardless of SSL context sharing.

I suggest skipping the paragraph about sibling location, those details don't matter here.

@arut
Copy link
Contributor

The refactoring patch has noconnection_type assignment now for some reason. This assignment should be there for HTTP/1, but not for HTTP/2.

@arut
Copy link
Contributor

Also,listen 8443 ssl http2; is a deprecated way of enabling HTTP/2. The preferred way is to use thehttp2 on directive.

hongzhidaoand others added3 commitsJuly 16, 2025 07:46
This commit is prepared for HTTP/2 and HTTP/3 support.The ALPN protocol is now set per-connection inngx_http_upstream_ssl_init_connection(), allowing proper protocol negotiationfor each individual upstream connection regardless of SSL context sharing.
The module allows to use HTTP/2 protocol for proxying.HTTP/2 proxying is enabled by specifying "proxy_http_version 2.0".Example:    server {        listen 8000;        location / {            proxy_http_version 2.0;            proxy_passhttps://127.0.0.1:8443;        }    }    server {        listen 8443 ssl;        http2 on;        ssl_certificate certs/example.com.crt;        ssl_certificate_key certs/example.com.key;        location / {            return 200 foo;        }    }
@@ -1050,7 +937,7 @@ ngx_http_proxy_handler(ngx_http_request_t *r)
}


staticngx_int_t
ngx_int_t
ngx_http_proxy_eval(ngx_http_request_t *r, ngx_http_proxy_ctx_t *ctx,
Copy link
Author

Choose a reason for hiding this comment

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

Hi Roman,
What about changing it like this?

ngx_int_t-ngx_http_proxy_eval(ngx_http_request_t *r, ngx_http_proxy_ctx_t *ctx,+ngx_http_proxy_eval(ngx_http_request_t *r, ngx_http_proxy_vars_t *vars,     ngx_http_proxy_loc_conf_t *plcf) {     u_char               *p;@@ -1015,9 +1015,9 @@ ngx_http_proxy_eval(ngx_http_request_t *r, ngx_http_proxy_ctx_t *ctx,         }     }-    ctx->vars.key_start = u->schema;+    vars->key_start = u->schema;-    ngx_http_proxy_set_vars(&url, &ctx->vars);+    ngx_http_proxy_set_vars(&url, vars);     u->resolved = ngx_pcalloc(r->pool, sizeof(ngx_http_upstream_resolved_t));     if (u->resolved == NULL) {

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

@ac000ac000ac000 left review comments

@arutarutarut left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
nginx-1.29.2
Development

Successfully merging this pull request may close these issues.

4 participants
@hongzhidao@arut@ac000@Maryna-f5
[8]ページ先頭
©2009-2025 Movatter.jp