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

Use a regex to match environment variables #27565

Merged
merged 1 commit into from Mar 31, 2017

Conversation

@rothrock
Contributor

rothrock commented Oct 20, 2016

- What I did
Addressing #27561, I added an option to the splunk log driver to include as attributes the environment variables that match a regex.
- How I did it
I added the --splunk-env-regex option to the splunk logging driver (splunk.go).
I added code to scan and match container environment variables against the regex supplied in the above option. I add these matching variables to the 'attrs' map in the log driver.
- How to verify it
In splunk_test.go I added TestEnvByRegex() and updated the TestValidateLogOpt() to check for this new option.
- Description for the changelog
Allow the splunk log driver to pick up environment variables that match a supplied regular expression.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 24, 2016

Member

I'd be interested to see what the performance impact is for this, especially in a situation with high volume logging.

/cc @unclejack

Member

thaJeztah commented Oct 24, 2016

I'd be interested to see what the performance impact is for this, especially in a situation with high volume logging.

/cc @unclejack

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Oct 24, 2016

Contributor

Thanks for the comment. It's exercised when the logger is instantiated. The regex is not exercised for each log entry generated, so I don't see any performance impact as a function of log events.

This proposed code puts the same performance demand as the flag:

--log-driver=splunk --log-opt env=VAR_1,VAR_2,SOME_OTHER_VAR,...

It just adds a more expressive way to choose environment variables.

Contributor

rothrock commented Oct 24, 2016

Thanks for the comment. It's exercised when the logger is instantiated. The regex is not exercised for each log entry generated, so I don't see any performance impact as a function of log events.

This proposed code puts the same performance demand as the flag:

--log-driver=splunk --log-opt env=VAR_1,VAR_2,SOME_OTHER_VAR,...

It just adds a more expressive way to choose environment variables.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 25, 2016

Member

Ah, thanks for explaining, I didn't look in-depth, so makes sense

Member

thaJeztah commented Oct 25, 2016

Ah, thanks for explaining, I didn't look in-depth, so makes sense

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 7, 2016

Contributor

Seems like we should support this more generically since it's not really log driver specific. WDYT?

Contributor

cpuguy83 commented Nov 7, 2016

Seems like we should support this more generically since it's not really log driver specific. WDYT?

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Nov 7, 2016

Contributor

@cpuguy83 I think that is a good idea. I'm happy to move the relevant code into context.go. Should the cmd line arg be "--env-regex=" ?

Contributor

rothrock commented Nov 7, 2016

@cpuguy83 I think that is a good idea. I'm happy to move the relevant code into context.go. Should the cmd line arg be "--env-regex=" ?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 7, 2016

Contributor

@rothrock I'd just keep it as a log-opt still.

Contributor

cpuguy83 commented Nov 7, 2016

@rothrock I'd just keep it as a log-opt still.

rothrock added a commit to rothrock/docker that referenced this pull request Nov 9, 2016

Use a regex to match environment variables moby#27565
Signed-off-by: Joseph Rothrock <rothrock@rothrock.org>
@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Nov 13, 2016

Contributor

Hi, @cpuguy83

I generalized the functionality as you suggested.

Contributor

rothrock commented Nov 13, 2016

Hi, @cpuguy83

I generalized the functionality as you suggested.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 14, 2016

Contributor

Thanks, looks good.
Now I'm wondering if we should just support the regex in env instead of a separate opt?
@thaJeztah, @vdemeester?

An integration test would also be really helpful.

Contributor

cpuguy83 commented Nov 14, 2016

Thanks, looks good.
Now I'm wondering if we should just support the regex in env instead of a separate opt?
@thaJeztah, @vdemeester?

An integration test would also be really helpful.

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Nov 14, 2016

Contributor

Hi @cpuguy83 There are tests for this regex functionality in jsonfilelog_test.go and in splunk_test.go, so that seems like a kind of integration test. If there's something else you'd like to see then I'm happy to code it.

Contributor

rothrock commented Nov 14, 2016

Hi @cpuguy83 There are tests for this regex functionality in jsonfilelog_test.go and in splunk_test.go, so that seems like a kind of integration test. If there's something else you'd like to see then I'm happy to code it.

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Nov 30, 2016

Contributor

Hi @thaJeztah, @cpuguy83, and @vdemeester,

Do y'all have and lingering reservations or concerns about merging this PR? Any other questions that I can answer?

Contributor

rothrock commented Nov 30, 2016

Hi @thaJeztah, @cpuguy83, and @vdemeester,

Do y'all have and lingering reservations or concerns about merging this PR? Any other questions that I can answer?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 3, 2016

Contributor

@rothrock Hey, sorry for the delay, busy preparing the 1.13 release. We'll get this in soon.

Contributor

cpuguy83 commented Dec 3, 2016

@rothrock Hey, sorry for the delay, busy preparing the 1.13 release. We'll get this in soon.

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Dec 4, 2016

Contributor

@cpuguy83 Great! Thanks very much to all of you for your help.

Contributor

rothrock commented Dec 4, 2016

@cpuguy83 Great! Thanks very much to all of you for your help.

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Jan 9, 2017

Contributor

Hi @thaJeztah, @cpuguy83, and @vdemeester, did y'all change your mind about integrating this change?

Contributor

rothrock commented Jan 9, 2017

Hi @thaJeztah, @cpuguy83, and @vdemeester, did y'all change your mind about integrating this change?

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Jan 18, 2017

Contributor

Clearly y'all have accepted a change (#28852) that now makes this pr impossible to merge. What happens now?

Contributor

rothrock commented Jan 18, 2017

Clearly y'all have accepted a change (#28852) that now makes this pr impossible to merge. What happens now?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 18, 2017

Contributor

I think we're fine to move to code-review unless someone has an objection.

@rothrock Would you be fine to rebase with the needed changes or would you prefer someone to carry it?

Contributor

cpuguy83 commented Jan 18, 2017

I think we're fine to move to code-review unless someone has an objection.

@rothrock Would you be fine to rebase with the needed changes or would you prefer someone to carry it?

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Jan 18, 2017

Contributor

@cpuguy83 I'm fine with rebasing and reworking. Thanks.

Contributor

rothrock commented Jan 18, 2017

@cpuguy83 I'm fine with rebasing and reworking. Thanks.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 18, 2017

Contributor

@rothrock Let's make sure there's no objections from other maintainers before having you go through the motions here.

Thanks!

Contributor

cpuguy83 commented Jan 18, 2017

@rothrock Let's make sure there's no objections from other maintainers before having you go through the motions here.

Thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 18, 2017

Member

Now I'm wondering if we should just support the regex in env instead of a separate opt?

If we can properly distinguish between it being a regex, or a "plain" value, I'm ok with reusing the --env flag (maybe we need a syntax for that?)

One thing to take into account is templating (i.e., if we're expanding this to services)

@vdemeester any thoughts on that?

Member

thaJeztah commented Jan 18, 2017

Now I'm wondering if we should just support the regex in env instead of a separate opt?

If we can properly distinguish between it being a regex, or a "plain" value, I'm ok with reusing the --env flag (maybe we need a syntax for that?)

One thing to take into account is templating (i.e., if we're expanding this to services)

@vdemeester any thoughts on that?

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Jan 19, 2017

Member

If we can properly distinguish between it being a regex, or a "plain" value, I'm ok with reusing the --env flag (maybe we need a syntax for that?)

I don't think we can distinguish.
There are some variables that look like "not plain". e.g. ProgramFiles(x86)

Member

AkihiroSuda commented Jan 19, 2017

If we can properly distinguish between it being a regex, or a "plain" value, I'm ok with reusing the --env flag (maybe we need a syntax for that?)

I don't think we can distinguish.
There are some variables that look like "not plain". e.g. ProgramFiles(x86)

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 19, 2017

Contributor

Yeah, the separate flag is fine.

Contributor

cpuguy83 commented Jan 19, 2017

Yeah, the separate flag is fine.

rothrock added a commit to rothrock/docker that referenced this pull request Jan 19, 2017

Use a regex to match environment variables moby#27565
Signed-off-by: Joseph Rothrock <rothrock@rothrock.org>
Use a regex to match environment variables #27565
Signed-off-by: Joseph Rothrock <rothrock@rothrock.org>
@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Jan 20, 2017

Contributor

Hi @cpuguy83 this is rebased and hopeful of your approval.

Contributor

rothrock commented Jan 20, 2017

Hi @cpuguy83 this is rebased and hopeful of your approval.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 27, 2017

Contributor

I'm going to go ahead and move to code review.

Contributor

cpuguy83 commented Jan 27, 2017

I'm going to go ahead and move to code review.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Mar 13, 2017

Member

code LGTM
PTAL @cpuguy83

Member

AkihiroSuda commented Mar 13, 2017

code LGTM
PTAL @cpuguy83

@AkihiroSuda AkihiroSuda changed the title from Use a regex to match environment variables to [splunk] Use a regex to match environment variables Mar 16, 2017

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 17, 2017

Contributor

LGTM but needs an integration test.
@rothrock Would you like to do this or if you want I can push a test up.

Contributor

cpuguy83 commented Mar 17, 2017

LGTM but needs an integration test.
@rothrock Would you like to do this or if you want I can push a test up.

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Mar 17, 2017

Contributor

Hi, I'm not sure what's needed for an integration test, so I'd be much obliged if you'd create one.

FWIW, I did expand the tests in splunk_test.go and jsonlogfile_test.go to exercise this new functionality.

Contributor

rothrock commented Mar 17, 2017

Hi, I'm not sure what's needed for an integration test, so I'd be much obliged if you'd create one.

FWIW, I did expand the tests in splunk_test.go and jsonlogfile_test.go to exercise this new functionality.

@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit 1325f66 into moby:master Mar 31, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29819 has succeeded
Details
janky Jenkins build Docker-PRs 38419 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9451 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 31, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 31, 2017

Member

@rothrock can you open a pull request to update the documentation for the Splunk driver? the pull request should be against the vnext-engine branch here; https://github.com/docker/docker.github.io/blob/vnext-engine/engine/admin/logging/splunk.md

Member

thaJeztah commented Mar 31, 2017

@rothrock can you open a pull request to update the documentation for the Splunk driver? the pull request should be against the vnext-engine branch here; https://github.com/docker/docker.github.io/blob/vnext-engine/engine/admin/logging/splunk.md

@rothrock

This comment has been minimized.

Show comment
Hide comment
@rothrock

rothrock Mar 31, 2017

Contributor

@thaJeztah yes, will do.

Contributor

rothrock commented Mar 31, 2017

@thaJeztah yes, will do.

@thaJeztah thaJeztah removed this from Revisit in maintainers-session Apr 6, 2017

rothrock added a commit to rothrock/docker.github.io that referenced this pull request Apr 7, 2017

@rothrock rothrock referenced this pull request Apr 7, 2017

Merged

Env regex #2688

@albers albers changed the title from [splunk] Use a regex to match environment variables to Use a regex to match environment variables May 5, 2017

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