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

support proxy CONNECT headers #1147

Closed
wants to merge 1 commit into from
Closed

Conversation

SalmaanP
Copy link

@SalmaanP SalmaanP commented Sep 8, 2019

This PR adds support for custom proxy CONNECT headers. The headers can be added using the proxyHeaders option.

Example:
Setting proxy headers

[k6] cat script.js                                                                                                                                                                                                    proxy-headers  ✱
import http from "k6/http";

export let options = {
    proxyHeaders: {"foo": ["bar"], "bar": ["foo"]}
};

export default function() {
  http.get("https://google.com");
};

Setting proxy

[k6] export HTTPS_PROXY=http://localhost:12345                                                                                                                                                                        proxy-headers  ✱
[k6] ./k6 run script.js

Proxy receives proxy headers

[~] ncat  -kv  -l 12345
Ncat: Version 7.70 ( https://nmap.org/ncat )
Ncat: Listening on :::12345
Ncat: Listening on 0.0.0.0:12345
Ncat: Connection from 127.0.0.1.
Ncat: Connection from 127.0.0.1:52001.
POST http://k6reports.loadimpact.com/ HTTP/1.1
Host: k6reports.loadimpact.com
User-Agent: Go-http-client/1.1
Content-Length: 116
Content-Type: application/json
Accept-Encoding: gzip

{"duration":0,"goarch":"amd64","goos":"darwin","iterations":1,"k6_version":"0.26.0-dev","st_duration":0,"vus_max":1}Ncat: Connection from 127.0.0.1.
Ncat: Connection from 127.0.0.1:52002.
CONNECT google.com:443 HTTP/1.1
Host: google.com:443
User-Agent: Go-http-client/1.1
bar: foo
foo: bar

@CLAassistant
Copy link

CLAassistant commented Sep 8, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-io
Copy link

codecov-io commented Sep 8, 2019

Codecov Report

Merging #1147 into master will decrease coverage by 0.07%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
- Coverage   73.61%   73.53%   -0.08%     
==========================================
  Files         144      144              
  Lines       10527    10534       +7     
==========================================
- Hits         7749     7746       -3     
- Misses       2321     2328       +7     
- Partials      457      460       +3
Impacted Files Coverage Δ
lib/options.go 91.87% <0%> (-0.95%) ⬇️
js/runner.go 84.8% <100%> (+0.06%) ⬆️
js/modules/k6/ws/ws.go 78.94% <0%> (-2.36%) ⬇️
lib/options_tls_go1_13.go
lib/options_tls_go1_12.go 50% <0%> (ø)

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 a2077dd...8013260. Read the comment docs.

@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Sep 9, 2019
@na--
Copy link
Member

na-- commented Sep 9, 2019

Thank you for making this pull request and for advocating for better proxy support! Unfortunately, I'm not sure we would be able to merge the PR right now, or even in the next few months 😞

As I explained in the issue, we currently favor the approach of creating custom transports (#1045 (comment)) for configuring and dealing with HTTP proxies, TLS configurations, HTTP versions and other transport-level details. Adding another global option now, when we know it's very likely going to be deprecated (i.e. moved to a sub-option of something like a new global defaultHttpTransport option) in a few months doesn't make sense to me. It's already going to be tricky enough to move the current global TLS and other HTTP transport-related options to that new global HTTP transport struct option given the complexity and issues of the current configuration (#883), I don't want to make it even more complicated by adding more global options...

We still haven't properly evaluated the approach I mentioned above, that's why it doesn't have its own issue and is just described in a comment of an issue about HTTP proxies. However, it seems likely to me that we might choose to do the implementation if 2 stages - first figure out the specifics and fix the global options (i.e. introduce that new defaultHttpTransport struct there) and then introduce runtime user-configurable transports in the JS API. If that's the case, proxyHeaders is very likely to be a part of that new global defaultHttpTransport option somewhat sooner than if we did everything at once, but it's still going to take some time to get there...

@codecov-commenter
Copy link

Codecov Report

Merging #1147 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
- Coverage   73.54%   73.53%   -0.02%     
==========================================
  Files         144      144              
  Lines       10531    10534       +3     
==========================================
+ Hits         7745     7746       +1     
- Misses       2327     2328       +1     
- Partials      459      460       +1     
Impacted Files Coverage Δ
lib/options.go 91.87% <0.00%> (-0.95%) ⬇️
js/runner.go 84.80% <100.00%> (+0.06%) ⬆️

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 7e438a1...8013260. Read the comment docs.

@na-- na-- added the new-http issues that would require (or benefit from) a new HTTP API label Oct 18, 2021
@na--
Copy link
Member

na-- commented Mar 31, 2022

I'll close this, since for the reasons I explained in #1147 (comment) and in #1045 (comment) (and other issues as well), we want to support such changes with a new HTTP API properly, instead of tacking them onto the current one, on top of all of its existing issues. #2461 was created to track the first exploratory steps in that direction.

@na-- na-- closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 new-http issues that would require (or benefit from) a new HTTP API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants