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 a proxy in splunk log driver #36220

Merged
merged 2 commits into from Feb 8, 2018

Conversation

Projects
None yet
7 participants
@dnephin
Copy link
Member

dnephin commented Feb 6, 2018

No description provided.

@thaJeztah
Copy link
Member

thaJeztah left a comment

LGTM, thanks!

@anusha-ragunathan

This comment has been minimized.

Copy link
Contributor

anusha-ragunathan commented Feb 6, 2018

Can we check to see if ProxyFromEnvironment doesnt return err?
Never mind. Got mixed up with the function https://golang.org/src/net/http/transport.go#L266

LGTM for sure

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Feb 6, 2018

I think this is the way it's supposed to be used (also see https://golang.org/pkg/net/http/#ProxyFromEnvironment, and the example on that page)

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.

As a special case, if req.URL.Host is "localhost" (with or without a port number), then a nil URL and nil error will be returned.

So an error is only returned in situations where no proxy should be used (in which case nil is returned)

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Feb 6, 2018

PowerPC node has some issues; https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/8278/console

17:36:15 Step 6/31 : RUN curl -fsSL "https://golang.org/dl/go${GO_VERSION}.linux-ppc64le.tar.gz" 	| tar -xzC /usr/local
17:36:15  ---> Running in 4abd3531a716
17:36:16 curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104
17:36:16 
17:36:16 gzip: stdin: unexpected end of file
17:36:16 tar: Unexpected EOF in archive
17:36:16 tar: Unexpected EOF in archive
17:36:16 tar: Error is not recoverable: exiting now
@mavenugo
Copy link
Contributor

mavenugo left a comment

@dnephin can you pls add a test-case ?

update vendor
Signed-off-by: Daniel Nephin <dnephin@docker.com>

@dnephin dnephin force-pushed the dnephin:support-proxy-in-splunk-driver branch from becf86a to 88aa839 Feb 7, 2018

@dnephin

This comment has been minimized.

Copy link
Member Author

dnephin commented Feb 7, 2018

Added a unit test.

I tried adding a more complete test using the NewHTTPEventCollectorMock mock in this package but that won't work because golang refuses to use a proxy for any localhost/127.x.x.x url (https://github.com/golang/go/blob/release-branch.go1.9/src/net/http/transport.go#L1218-L1225)

Since the rest of this code is part of the stdlib, all we really need to test is that the transport has the correct proxy factory, which is covered by this test.

@dnephin dnephin force-pushed the dnephin:support-proxy-in-splunk-driver branch from 88aa839 to 0823c9b Feb 7, 2018

Support a proxy in splunk log driver
Signed-off-by: Daniel Nephin <dnephin@docker.com>

@dnephin dnephin force-pushed the dnephin:support-proxy-in-splunk-driver branch from 0823c9b to 3c4537d Feb 7, 2018

@thaJeztah
Copy link
Member

thaJeztah left a comment

a unit test sounds sane to me

LGTM

@mavenugo
Copy link
Contributor

mavenugo left a comment

Thanks @dnephin . LGTM

@vdemeester vdemeester merged commit f653485 into moby:master Feb 8, 2018

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 39144 has succeeded
Details
janky Jenkins build Docker-PRs 47897 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 8310 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4127 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 19428 has succeeded
Details
z Jenkins build Docker-PRs-s390x 8288 has succeeded
Details

@dnephin dnephin deleted the dnephin:support-proxy-in-splunk-driver branch Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment