8000
Skip to content

Support a proxy in splunk log driver - #36220

Merged
vdemeester merged 2 commits into
moby:masterfrom
dnephin:support-proxy-in-splunk-driver
Feb 8, 2018
Merged

Support a proxy in splunk log driver#36220
vdemeester merged 2 commits into
moby:masterfrom
dnephin:support-proxy-in-splunk-driver

Conversation

@dnephin
@dnephin dnephin commented Feb 6, 2018
Copy link
Copy Markdown
Member

No description provided.

thaJeztah
thaJeztah approved these changes Feb 6, 2018
View reviewed changes
@thaJeztah thaJeztah left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@anusha-ragunathan
anusha-ragunathan commented Feb 6, 2018 •
edited
Loading
Copy link
Copy Markdown
Contributor

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
thaJeztah commented Feb 6, 2018
Copy link
Copy Markdown
Member

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
thaJeztah commented Feb 6, 2018
Copy link
Copy Markdown
Member

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
mavenugo reviewed Feb 6, 2018
View reviewed changes
@mavenugo mavenugo left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dnephin can you pls add a test-case ?

@dnephin
update vendor
7d29652 
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin
dnephin force-pushed the support-proxy-in-splunk-driver branch from becf86a to 88aa839 Compare February 7, 2018 19:46
@dnephin
dnephin commented Feb 7, 2018 •
edited
Loading
Copy link
Copy Markdown
Member Author

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 support-proxy-in-splunk-driver branch from 88aa839 to 0823c9b Compare February 7, 2018 19:50
@dnephin
Support a proxy in splunk log driver
3c4537d 
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin
dnephin force-pushed the support-proxy-in-splunk-driver branch from 0823c9b to 3c4537d Compare February 7, 2018 19:52
thaJeztah
thaJeztah approved these changes Feb 7, 2018
View reviewed changes
@thaJeztah thaJeztah left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a unit test sounds sane to me

LGTM

mavenugo
mavenugo approved these changes Feb 7, 2018
View reviewed changes
@mavenugo mavenugo left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @dnephin . LGTM

@vdemeester
vdemeester merged commit f653485 into moby:master Feb 8, 2018
@dnephin
dnephin deleted the support-proxy-in-splunk-driver branch February 8, 2018 15:54
@thaJeztah thaJeztah mentioned this pull request Feb 10, 2018
Merged
@gbarr01 gbarr01 mentioned this pull request Feb 20, 2018
Merged
@gbarr01 gbarr01 mentioned this pull request Mar 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes
+2 more reviewers
@mavenugo mavenugo mavenugo approved these changes
@anusha-ragunathan anusha-ragunathan anusha-ragunathan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/logging impact/changelog status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dnephin @anusha-ragunathan @thaJeztah @mavenugo @vdemeester @yongtang @GordonTheTurtle
0