HTTP1 client for Argo server should allow more flexible http client config and use context when creating new requests

[williamburgson]

Summary

Currently the ArgoServerOpts expose a few config options but it does not allow setting http proxies. We have an API gateway that connects to multiple services across different network zones, so

• we need the http proxy for the connection to argo server
• we can't set the HTTP_PROXY env var because the API gateway is responsible for multiple services across network zones and we can't just set a proxy globally for all the services

What change needs making?

1. Add a httpClient option to argo server options for apiclient

type ArgoServerOpts struct {
	// argo server host port, must be `host:port`, e.g. localhost:2746
	URL string
	// any base path needed (e.g. due to being behind an ingress)
	Path               string
	Secure             bool
	InsecureSkipVerify bool
	// whether or not to use HTTP1
	HTTP1   bool
	Headers []string
        // use this http client when supplied
        HTTPClient *http.Client
}

2. Use context when making a new request in http1 facade

func (h Facade) do(ctx context.Context, in interface{}, out interface{}, method string, path string) error {
        ...
	req, err := http.NewRequestWithContext(ctx, method, u.String(), bytes.NewReader(data))
        ...
}

3. update the facade interface to take context and all the http1 service clients to pass the context to facade, e.g.

func (h WorkflowServiceClient) CreateWorkflow(ctx context.Context, in *workflowpkg.WorkflowCreateRequest, _ ...grpc.CallOption) (*wfv1.Workflow, error) {
	out := &wfv1.Workflow{}
	return out, h.Post(ctx, in, out, "/api/v1/workflows/{namespace}")
}

Use Cases

The primary use case is that we would like to set a custom roundtripper for the argo server http1 client.

We have an api gateway (authenticated) that connects to multiple instances of argo servers across multiple network zones and when a request for argo server comes in for the api gateway, it needs to do the following:

• pass the bearer token from incoming request to the outgoing request to the target argo server
• set http proxy if the target argo server is in a different network zone
  both can be done with a custom roundtripper, e.g.

func RoundTrip(r *http.Request) (*http.Response, error) {
        // set auth header for the outgoing request to argo server
        // based on the request received by the api gateway
	token := r.Context().Value("token").(string)
	r.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
        // set proxy for the outgoing request based on network zones
	secureTransport := &http.Transport{Proxy: http.ProxyURL(s.proxy)}
	return secureTransport.RoundTrip(r)
}

When would you use this?

Not urgent, but I think this is nice to have :)

---

Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritize the proposals with the most 👍.

[shimako55]

Can this PR resolve the issue mentioned here? If there are any missing pieces or additional requirements, could you please point them out?

[williamburgson]

Can this #12527 resolve the issue mentioned here? If there are any missing pieces or additional requirements, could you please point them out?

hi @shimako55 thanks! this PR does solve one of the requirements (http proxy)

I'm wondering if we can

• make the HTTP1 facade client more flexible by taking in a fully configured http client struct instead of passing individual config options such as bearer token/http proxy
• use NewRequestWithContext and actually use the context for requests in the http1 service client interfaces

I'm also happy submit PRs for these changes

[shimako55]

Hi @williamburgson

thanks for the response and I could understand the importance of them

I'm also happy submit PRs for these changes

I will be very happy when this issue is resolved

[monaka]

It looks like #12867 is a super set of #12527 . So I suppose we can close #12527 w/o merging. right?