Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd Splunk logging driver #16207 #16488
Conversation
GordonTheTurtle
added
the
status/0-triage
label
Sep 22, 2015
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Sep 22, 2015
This PR is for #16207 |
This comment has been minimized.
This comment has been minimized.
Updated OP's description to include the issue it fixes (so that it automatically closes it if this PR is merged) |
albers
reviewed
Sep 23, 2015
@@ -270,6 +270,7 @@ __docker_log_drivers() { | |||
json-file | |||
none | |||
syslog | |||
splunk |
This comment has been minimized.
This comment has been minimized.
albers
Sep 23, 2015
Member
Please insert this in alphabetical order. This also applies to the other additions.
This comment has been minimized.
This comment has been minimized.
albers
reviewed
Sep 23, 2015
@@ -280,10 +281,11 @@ __docker_log_driver_options() { | |||
local json_file_options="max-file max-size" | |||
local syslog_options="syslog-address syslog-facility tag" | |||
local awslogs_options="awslogs-region awslogs-group awslogs-stream" | |||
local splunk_options="splunk-url splunk-token splunk-source splunk-sourcetype splunk-index splunk-capath splunk-caname splunk-insecureskipverify" |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
albers
reviewed
Sep 23, 2015
@@ -347,6 +352,16 @@ __docker_complete_log_driver_options() { | |||
" -- "${cur#=}" ) ) | |||
return | |||
;; | |||
*splunk-url=*) | |||
COMPREPLY=( $( compgen -W "http https" -S "://" -- "${cur#=}" ) ) |
This comment has been minimized.
This comment has been minimized.
albers
Sep 23, 2015
Member
This does not behave as expected because :
is special (contained in COMP_WORDBREAKS
).
docker run --log-opt splunk-url=http:
will not complete anything.
Please change to
COMPREPLY=( $( compgen -W "http:// https://" -- "${cur#=}" ) )
compopt -o nospace
__ltrim_colon_completions "${cur}"
return
;;
This comment has been minimized.
This comment has been minimized.
outcoldman
Sep 23, 2015
Author
Contributor
@albers weird, I took gelf-address
and syslog-address
as examples, see https://github.com/docker/docker/pull/16488/files#diff-754420541b12dbcbbea00825942a4635R326
This comment has been minimized.
This comment has been minimized.
Thanks very much for also implementing bash completion. That's really cool stuff! |
This comment has been minimized.
This comment has been minimized.
@albers thank you for review! Just updated PR |
bfirsh
added
status/1-design-review
and removed
status/0-triage
labels
Sep 23, 2015
This comment has been minimized.
This comment has been minimized.
Bash completion LGTM. Thanks for updating. |
This comment has been minimized.
This comment has been minimized.
Didn't read the code, but I'm +1 for a splunk driver since we already have many similar logging drivers. |
This comment has been minimized.
This comment has been minimized.
ping @sdurrheimer zsh completion: new log-driver |
GordonTheTurtle
added
the
dco/no
label
Sep 24, 2015
outcoldman
force-pushed the
splunk:splunk_log_driver
branch
from
0c6955f
to
5c8a50b
Sep 24, 2015
GordonTheTurtle
removed
the
dco/no
label
Sep 24, 2015
This comment has been minimized.
This comment has been minimized.
@outcoldman Is there any easy way to test driver? |
This comment has been minimized.
This comment has been minimized.
@LK4D4 it depends on the definition of "easy". I did all my tests manually. To test it we need to
Do you think that we can perform first 2 steps? Do you have an example? |
This comment has been minimized.
This comment has been minimized.
@outcoldman I mean test manually, not unit-tests. Yes, I thought maybe you have docker image with splunk. |
This comment has been minimized.
This comment has been minimized.
Ah, I see, so you are asking for instructions how to test it manually. This make sense.
|
outcoldman
reviewed
Sep 25, 2015
@@ -0,0 +1,256 @@ | |||
// +build linux |
This comment has been minimized.
This comment has been minimized.
outcoldman
Sep 25, 2015
Author
Contributor
@LK4D4 one question about this line. I saw it in other logging drivers. What is the purpose of this line? As we don't use anything special - our driver should work everywhere where you can run the daemon.
Is it some kind of transition to Windows? Our driver should work on Windows out of box. Should I remove it?
This comment has been minimized.
This comment has been minimized.
LK4D4
Sep 25, 2015
Contributor
Yup, you should remove it then.
Actually let's ask @jhowardmsft too :)
This comment has been minimized.
This comment has been minimized.
jhowardmsft
Sep 25, 2015
Contributor
Yeah, if it works on Windows and you've verified it does, then remove the line. But you'll need a line added to daemon\logdrivers_windows.go too to add splunk.
This comment has been minimized.
This comment has been minimized.
glennblock
Sep 25, 2015
Supporting Windows is not critical right now as the WIndows support for Docker is till being developed. Once it settles, we'll revisit. We certainly would like long term for it to work on Windows.
This comment has been minimized.
This comment has been minimized.
outcoldman
Sep 25, 2015
Author
Contributor
Ok, so per @glennblock we do not support Windows right now. I mean it should work, but we have not tested it.
Should I keep this line for now?
Btw, do I need to have special file "splunk_unsupported.go"?
This comment has been minimized.
This comment has been minimized.
glennblock
Sep 25, 2015
@jhowardmsft the driver technically should work, but the test effort for us to verify on Windows is pretty significant today, based on our recent eval. Is this something that you guys could help us with? We'd be happy to get you a Splunk instance you could test pushing to.
This comment has been minimized.
This comment has been minimized.
glennblock
Sep 25, 2015
@LK4D4 the Splunk instance can be running on any platform. The driver is just making simple HTTP requests, so I don't see why it would not work on Windows.
This comment has been minimized.
This comment has been minimized.
jhowardmsft
Sep 25, 2015
Contributor
@glennblock - Would be happy to verify, just not immediately, busy getting the last few pieces in for technical preview 4 of Windows Server 2016. Drop me a line.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
LK4D4
added
status/2-code-review
and removed
status/1-design-review
labels
Sep 25, 2015
This comment has been minimized.
This comment has been minimized.
@outcoldman thanks, I'll try. Moving to code-review in the meantime. |
This comment has been minimized.
This comment has been minimized.
btw need rebase, sorry :) |
outcoldman
force-pushed the
splunk:splunk_log_driver
branch
from
b6f7df6
to
9f9c075
Sep 30, 2015
This comment has been minimized.
This comment has been minimized.
@LK4D4 np, rebased! |
halr9000
reviewed
Sep 30, 2015
@@ -304,7 +304,7 @@ Json Parameters: | |||
systems, such as SELinux. | |||
- **LogConfig** - Log configuration for the container, specified as a JSON object in the form | |||
`{ "Type": "<driver_name>", "Config": {"key1": "val1"}}`. | |||
Available types: `json-file`, `syslog`, `journald`, `gelf`, `awslogs`, `none`. | |||
Available types: `json-file`, `syslog`, `journald`, `gelf`, `awslogs`, `splunk`, `none`. |
This comment has been minimized.
This comment has been minimized.
halr9000
Sep 30, 2015
This is getting long. Maybe it should be sorted alpha, with "none" remaining at the end?
This comment has been minimized.
This comment has been minimized.
thaJeztah
Oct 21, 2015
Member
hm, yes, we can do that; possible none
as the first. I'm fine with leaving that for another PR though.
@outcoldman since this won't make it into docker 1.9, these changes should be moved to docker_remote_api_v1.22.md
now
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I would recommend accepting both username & password, as well as http event collector tokens. |
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Sep 30, 2015
Hi @phemmer Thanks for the input. Event Collector is available in Splunk Cloud, but it needs to be enabled via a support request. There's no UI yet for self-service. But it is definitely available. Please contact Splunk Cloud support and they will get you up and running. As to the username / password. Going forward we will encourage people not to use the REST API to send events as it requires a username and password that can potentially leak access to Splunk. Also EC has been designed in a more scalable / available fashion than the receiver endpoints. Event Collector tokens are not attached to user credentials, they are specifically designed just for logging events. It is a special endpoint that only allows sending data with a valid token. This makes it more secure from a Splunk perspective as using the token will not allow accessing any part of Splunk, you can't even authenticate to the management port (8089) with the token. Does that make sense? You are correct that it is only in Splunk 6.3, but you can stand up dedicated Event Collector instances within a 6.2 environment and they will work fine. EC can run anywhere, an indexer, a forwarder, etc. |
This comment has been minimized.
This comment has been minimized.
@moxiegirl gave me a "LGTM" in person |
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Oct 23, 2015
Sweet!
|
This comment has been minimized.
This comment has been minimized.
I'd recommend clarifying that one paragraph. Otherwise, LGTM |
This comment has been minimized.
This comment has been minimized.
Oh, boy.. looks like this needs a rebase :'( sorry for that @outcoldman edit: sorry for the wrong ping @mountkin, autocomplete hit me :( |
outcoldman
force-pushed the
splunk:splunk_log_driver
branch
from
ca2d315
to
1f1dbf3
Oct 23, 2015
This comment has been minimized.
This comment has been minimized.
@thaJeztah np, rebased! |
This comment has been minimized.
This comment has been minimized.
Al green! merging \o/ |
added a commit
that referenced
this pull request
Oct 24, 2015
thaJeztah
merged commit 8d4888d
into
moby:master
Oct 24, 2015
5 checks passed
This comment has been minimized.
This comment has been minimized.
@outcoldman @glennblock congrats! |
This comment has been minimized.
This comment has been minimized.
Thank you all for helping! And my personal huge thank to the @LK4D4. |
This comment has been minimized.
This comment has been minimized.
Thanks for contributing @outcoldman open source FTW! |
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Oct 24, 2015
Yay!!!! This rocks!
|
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Oct 24, 2015
Thank you all! We are seeing a lot of interest in Docker in our Splunk
|
This comment has been minimized.
This comment has been minimized.
Keewl! =) |
moxiegirl
added this to the 1.9.2 milestone
Dec 10, 2015
This comment has been minimized.
This comment has been minimized.
@thaJeztah Looks like this was missed and not cherry picked in. Any idea when 1.9.3 is going out? |
This comment has been minimized.
This comment has been minimized.
@moxiegirl I dunno why you think it should be in a milestone. It's a new feature == shouldn't be in the minor release. |
thaJeztah
modified the milestones:
1.10,
1.9.2
Dec 11, 2015
This comment has been minimized.
This comment has been minimized.
There was some confusion about the timing, but this PR was merged after the 1.9.0 code freeze (1.9.0-rc1 was released on October 14th). This driver will be part of the 1.10 release. For people wanting to give it a spin before that, it is available in the "Experimental" or "Master" builds (not intended for production) |
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Dec 11, 2015
Our understanding was it was part of 1.10. Thanks for confirming. On Thu, Dec 10, 2015 at 4:48 PM Sebastiaan van Stijn <
|
thaJeztah
added
the
impact/changelog
label
Jan 15, 2016
This comment has been minimized.
This comment has been minimized.
juniorjbn
commented
Aug 17, 2016
Maybe the splunk driver could receive more than one host in "--log-opt splunk-url" for the cases splunk running in cluster ? |
This comment has been minimized.
This comment has been minimized.
glennblock
commented
Aug 17, 2016
Thanks for the suggestion. With HEC the recommendation is always to have a load balancer in front of the HEC pool. Based on that I don't think we need clustering support. Also that will complicate the implementation. |
outcoldman commentedSep 22, 2015
Allow to send Splunk logs using Http Event Collector
Signed-off-by: Denis Gladkikh denis@gladkikh.email
Fixes #16207