Make HTTP client configurable #537

ueokande · 2018-12-04T01:08:47Z

I propose that the user can configure HTTP client to access internet by it.

In our case, the http proxy settings is required when the client accesses to Internet.
The HTTP proxy is currently set via environment variable.

image/pkg/tlsclientconfig/tlsclientconfig.go

Line 101 in 63a1cbd

Proxy: http.ProxyFromEnvironment,

This patch adds filed HTTPClient *http.Client to SystemContext to use on pulling image or fetching informations from the Internet. The docker client use HTTPClient field if set, and ignores cert configs in SystemContext fields, otherwise it use default http client from cert configs in SystemContext.

Add `HTTPClient` field into `types.SystemContext` to configure http client by caller. If the `HTTPClient` filed is set, cert configurations are ignored in `types.SystemContext` on fetching images.

rhatdan · 2018-12-11T13:03:41Z

@vrothberg PTAL

vrothberg · 2018-12-11T14:04:37Z

Thanks for the PR. It's an interesting idea to move that into the API!

We have a highly related discussion over at containers/buildah. I think we can extend the ideas over there to fit your needs in the API as well. However, we need to be careful to not introduce too many ways of configuring proxies.

@ueokande could you have a look at the linked issue and share your thoughts there?

ueokande · 2018-12-12T21:38:13Z

@vrothberg
We don't use buildah, and import containers/image library instead.

In our scenario, updater tools deployed in the data-center fetches newer container image via HTTP proxy.
I think setting HTTP client is practical requirement. Some API client such as GitHub client has attribute of the configurable *http.Client. That is usable not only setting HTTP proxy but also setting throttle, compression, and so on.

We currently use os.Setenv() hacks to set HTTP proxy in the updater tool (see here).

vrothberg

@ueokande, sorry for the late response, I lost this PR from my radar.

LGTM, just a minor nitpick, also you need to sign the commits (git commit -s).

@mtrmac WDYT?

vrothberg · 2019-01-11T17:09:08Z

docker/docker_client.go

+
+// newHTTPClient returns new http client for docker client.  It returns HTTPClient in SystemContext if set,
+// otherwise returns default client.
+func newHTTPClient(sys *types.SystemContext, registry, hostName, reference string) (*http.Client, error) {


Could we document the arguments as for newDockerClient?

mtrmac · 2019-01-14T03:41:45Z

I don’t really like this… in the sense that the vast majority of clients should not need to care at this low level (notably we definitely don’t want Buildah/podman to have its own code to deal with proxies; that should be handled for them transparently in c/image).

If a typical client of c/image needed to set its own http.Client, that would clearly be a sign that c/image is not doing what it should do.

Of course, that does not automatically mean that there can’t be exceptional clients that would benefit from this capability — but once we add this option, we will lose the feedback channel because callers who find c/image insufficient can just use its own http.Client without telling us.

In this case, wouldn’t it be enough to add a HTTPProxy func(*Request) (*url.URL, error) option, to be set by c/image in the created http.Transport? OTOH…

That is usable not only setting HTTP proxy but also setting throttle, compression, and so on.

is true as well.

I’m honestly unsure.

mtrmac

(Just noting smaller issues I have noticed, this does not automatically mean that overall I think this should be merged.)

mtrmac · 2019-01-14T03:42:15Z

docker/daemon/client.go

 	var httpClient *http.Client
 	if url.Scheme != "unix" {
-		if url.Scheme == "http" {
+		if sys.HTTPClient != nil {


sys == nil is always valid, and must be treated the same as &types.SystemContext{}.

mtrmac · 2019-01-14T03:45:11Z

types/types.go

 	DirForceCompress bool
+
+	// HTTPClient is the HTTP client to use.  If the HTTPClient field is not nil,
+	// the docker client is created with HTTPClient, and the below cert


This does not apply only to “docker”, and it should be in the “Global configuration overrides” section.

mtrmac · 2019-01-14T03:45:43Z

types/types.go

+
+	// HTTPClient is the HTTP client to use.  If the HTTPClient field is not nil,
+	// the docker client is created with HTTPClient, and the below cert
+	// config are ignored.


There are no options “below” — and while this makes sense in a way, it can lead to surprising behavior changes.

mtrmac · 2019-01-14T03:54:22Z

After reading the code in more detail: the way HTTPClient overrides so many other SystemContext options — although it does make sense, if the client is overridable at all, it must be overridable completely — this can lead to very surprising results where important security-related options can be silently ignored.

That’s another point in favor of a more localized HTTPProxy option. Or, maybe, instead of having a straight override which just replaces the http.Client, there could be an “edit hook” (vaguely similar to httpWrapper in #254), which would still leave all of the options supported and used by default, but allow callers to make specific localized edits to the http.Client without affecting the other options — but callers still could replace the implementation wholesale if they really wanted to.

(Still unsure, but leaning much more towards a more targeted option. I’d love to head what others think as well.)

vrothberg · 2019-01-14T09:21:30Z

I think @mtrmac makes some good points, especially regarding the subtleties regarding security. A more targetted/dedicated option to support proxies via the API might be easier to handle than a complete http client. @runcom @rhatdan PTAL.

rhatdan · 2019-01-14T14:48:25Z

I agree this should be done in containers/image not in any of the packages that use containers/image.

rhatdan · 2019-03-08T13:53:35Z

What is the state of this PR?

jamescassell · 2019-04-12T20:31:37Z

I'd argue that the primary method of configuring the system to use a HTTP Proxy should be to use the http_proxy, https_proxy, and no_proxy environment variables. A competent sysadmin should set these system-wide so that they are automatically set for all users and services in the case that they are needed. As long as c/image honors those settings, I'm indifferent whether any other method is supported for setting a proxy.

rhatdan · 2019-04-25T08:54:35Z

@ueokande Are you still interested in this PR?

ueokande · 2019-04-25T12:40:55Z

Sorry for replying so late.

I noticed that the design is not so good. Adding HTTPClient parameter brings duplicated TLS configurations in SystemContext. They are confusing for users.

Additionally, some methods create an HTTP client by &http.Client{}. If HTTPClient is added, we fix all of them and it makes huge changes on the behavior.

Thanks for the duscuttion, but I am inclined to close this PR once.

ueokande force-pushed the expose-http-client branch from d88d09e to 4587128 Compare December 4, 2018 01:26

*: Add HTTPClient field

784e349

Add `HTTPClient` field into `types.SystemContext` to configure http client by caller. If the `HTTPClient` filed is set, cert configurations are ignored in `types.SystemContext` on fetching images.

ueokande force-pushed the expose-http-client branch from 4587128 to 784e349 Compare December 4, 2018 01:30

Merge branch 'master' into expose-http-client

4298a4c

vrothberg reviewed Jan 11, 2019

View reviewed changes

mtrmac reviewed Jan 14, 2019

View reviewed changes

rhatdan mentioned this pull request Mar 8, 2019

Build/Push/Pull via HTTP Proxy containers/buildah#765

Closed

ueokande closed this Apr 25, 2019

mtrmac mentioned this pull request Apr 1, 2021

Customize http.Client and/or http.Transport #1194

Closed

anjannath mentioned this pull request Nov 8, 2022

Proxy support to pull the bundle image from quay crc-org/crc#3354

Closed

Make HTTP client configurable #537

Make HTTP client configurable #537

Uh oh!

Conversation

ueokande commented Dec 4, 2018

Uh oh!

rhatdan commented Dec 11, 2018

Uh oh!

vrothberg commented Dec 11, 2018

Uh oh!

ueokande commented Dec 12, 2018

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg Jan 11, 2019

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Jan 14, 2019

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

mtrmac Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

mtrmac Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Jan 14, 2019

Uh oh!

vrothberg commented Jan 14, 2019

Uh oh!

rhatdan commented Jan 14, 2019

Uh oh!

rhatdan commented Mar 8, 2019

Uh oh!

jamescassell commented Apr 12, 2019

Uh oh!

rhatdan commented Apr 25, 2019

Uh oh!

ueokande commented Apr 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants