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

fix upstream appendheader to upstream query string lost #1077

Merged
merged 1 commit into from Apr 2, 2020

Conversation

champly
Copy link
Member

@champly champly commented Apr 1, 2020

Issues associated with this PR

#1030

config

"listeners":[
    {
        "name":"serverListener",
        "address": "127.0.0.1:2046",
        "bind_port": true,
        "filter_chains": [{
            "filters": [
                {
                    "type": "proxy",
                    "config": {
                        "downstream_protocol": "Http2",
                        "upstream_protocol": "Http1",
                        "router_config_name":"server_router"
                    }
                }
            ]
        }]
    },
    {
        "name":"clientListener",
        "address": "127.0.0.1:2045",
        "bind_port": true,
        "filter_chains": [{
            "filters": [
                {
                    "type": "proxy",
                    "config": {
                        "downstream_protocol": "Http1",
                        "upstream_protocol": "Http2",
                        "router_config_name":"client_router"
                    }
                }
            ]
        }]
    }
]

request

if curl "http://localhost:2045/aabb?aa=123" mosn will be return 400 http code, and print parse query parameters error,parameters = %!(EXTRA string=aa=123)

solution

upstream's AppendHeaders function format URI error, because lost query placeholder.

@champly champly changed the title fix downstream appendheader to upstream query string lost fix upstream appendheader to upstream query string lost Apr 1, 2020
@codecov-io
Copy link

Codecov Report

Merging #1077 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
+ Coverage   56.04%   56.07%   +0.03%     
==========================================
  Files         246      246              
  Lines       23073    23073              
==========================================
+ Hits        12931    12939       +8     
+ Misses       8980     8973       -7     
+ Partials     1162     1161       -1     
Impacted Files Coverage Δ
pkg/network/connection.go 34.97% <0.00%> (-0.38%) ⬇️
pkg/module/http2/transport.go 78.47% <0.00%> (+0.14%) ⬆️
pkg/module/http2/server.go 82.36% <0.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0be6c06...1e32330. Read the comment docs.

@taoyuanyuan taoyuanyuan self-requested a review April 2, 2020 04:58
Copy link
Contributor

@taoyuanyuan taoyuanyuan left a comment

Choose a reason for hiding this comment

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

thx LGTM

@taoyuanyuan taoyuanyuan merged commit c166f90 into mosn:master Apr 2, 2020
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

3 participants