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

HTTP proxy feature #448

Merged
merged 3 commits into from
Apr 12, 2024
Merged

HTTP proxy feature #448

merged 3 commits into from
Apr 12, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented Apr 5, 2024

What was changed

  • Added HTTP proxy support to the runner (including confirmation
  • Added Go HTTP proxy features
    • Added "subprocess" support since Go gRPC's only way of proxying is via env var
    • Skip Go tests when using loopback since that's not supported in Go
  • Added Java HTTP proxy features
    • Also fixed issue where Java summary not properly reporting

@cretz cretz requested a review from a team April 5, 2024 16:34
@cretz cretz mentioned this pull request Apr 5, 2024

func (h HTTPProxyTest) Execute(ctx context.Context, r *harness.Runner) (client.WorkflowRun, error) {
// Since HTTP proxy in gRPC only works with the environment variable, we have
// to test in a subprocess to not infect other things in this process. The
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we also use a different Dialer? Would that be easier then running a separate subprocess?

https://github.com/grpc/grpc-go/blob/master/Documentation/proxy.md

Copy link
Member Author

@cretz cretz Apr 10, 2024

Choose a reason for hiding this comment

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

Using a different dialer means that you have to do the whole CONNECT HTTP request part yourself which basically means you're implementing your own client proxy logic. I think we want to prove that the env var works. If there were a better way to just set a boolean or single string the way we set an env var, I'd do that, but I can find no way. The only way the logic in https://github.com/grpc/grpc-go/blob/master/internal/transport/proxy.go uses a proxy in mapAddress is via https://pkg.go.dev/net/http#ProxyFromEnvironment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, but why do we need to test in a subprocess since HTTPS_PROXY will only apply if the environment variable matches the URL right? and no other test would connect to the proxies address?

Copy link
Member Author

@cretz cretz Apr 10, 2024

Choose a reason for hiding this comment

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

It doesn't matter the URL you're wanting to connect to, HTTPS_PROXY applies for all gRPC client connections. The only URL matching that occurs is checking whether it's http vs https, but gRPC always assumes https (that's why HTTP_PROXY doesn't apply, just HTTPS_PROXY).

It does have some weird other code that makes it not apply for loopback calls and to not match the proxy itself causing recursion. Ref https://cs.opensource.google/go/x/net/+/refs/tags/v0.24.0:http/httpproxy/proxy.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK my mistake I read

The environment values may be either a complete URL or a "host[:port]", in which case the "http" scheme is assumed. The schemes "http", "https", and "socks5" are supported. An error is returned if the value is a different form.

A nil URL and nil error are returned if no proxy is defined in the environment, or a proxy should not be used for the given request, as defined by NO_PROXY.

and thought it meant if the URL didn't match it was excluded, I guess it needs to be in NO_PROXY which is a lot more annoying.

@cretz cretz merged commit 252b8de into main Apr 12, 2024
18 checks passed
@cretz cretz deleted the http-connect-proxy branch April 12, 2024 12:37
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.

2 participants