Feature Request: load_config pattern

[cdenneen]

If deemed valuable to others:

Would it be possible for someone with ruby experience to update the globbing of configs to be *.conf (might be just as simple as changing line 288 of agent.rb). This would allow you to also keep backups of old configs or move configs out of the way if you want to piecemeal during testing which configs are live. (luckily the added bonus even with * is that private directories like .git aren't sourced so the entire logstash/conf.d can be under version control)

    287   def load_config(path)
    288     path = File.join(path, "*") if File.directory?(path)
    289
    290     if Dir.glob(path).length == 0
    291       fail(I18n.t("logstash.agent.configuration.file-not-found", :path => path))
    292     end
    293
    294     config = ""
    295     Dir.glob(path).sort.each do |file|
    296       next unless File.file?(file)
    297       if file.match(/~$/)
    298         @logger.debug("NOT reading config file because it is a temp file", :file => file)
    299         next
    300       end
    301       @logger.debug("Reading config file", :file => file)
    302       config << File.read(file) + "\n"
    303     end
    304     return config
    305   end # def load_config

[ph]

If we do this, this one is also related.
#2271

[jordansissel]

Is there still interest in this?

[cdenneen]

For me there is. Also I believe I recall another issue opened about globbling only *.conf files.

[suyograo]

+1 I think this would be a nicer default for users to only glob *.conf. @cdenneen we recently got this #4713, but I like the change you propose

[suyograo]

@jordansissel WDYT?

[jordansissel]

I would be ok with dropping directory support and only supporting globs or
a specific file. Then we can encourage default behaviors like *.conf --
thoughts?

On Tuesday, March 8, 2016, Suyog Rao notifications@github.com wrote:

@jordansissel https://github.com/jordansissel WDYT?

—
Reply to this email directly or view it on GitHub
#1831 (comment).

[cdenneen]

I think doing this would allow people to specify -f logstash/conf.d and it
would grab only the *.conf files and skip over any *.conf.bak or whatever
people might have there during their testing.

I'm not sure if it's even possible but wondering if there is a way to have
conf.d that's entire repo of all your possible logstash configs but only
select the ones you want? So same repo shared between indexers, brokers,
etc but only use the ones pertaining to that type? I'd have to think this
through a little more, sorry for the random thought, just trying to think
of easy way to manage all configs in one place.
On Tue, Mar 8, 2016 at 9:10 PM Jordan Sissel notifications@github.com
wrote:

I would be ok with dropping directory support and only supporting globs or
a specific file. Then we can encourage default behaviors like *.conf --
thoughts?

On Tuesday, March 8, 2016, Suyog Rao notifications@github.com wrote:

@jordansissel https://github.com/jordansissel WDYT?

—
Reply to this email directly or view it on GitHub
<#1831 (comment)
.

—
Reply to this email directly or view it on GitHub
#1831 (comment).

[jordansissel]

@cdenneen I confess that my resistance to "ignore backup files" or similar is because it's been a very long time since I've managed machines in a way that you describe, for example, git clone into a production box to ship its configuration files, or similar, or editing files on live systems frequently enough that it leaves backup files lying around in the same directory.

If I try to think as a user might, having directory support (-f /etc/logstash/conf.d/) to imply *.conf, I feel, would be upsetting to me if I am not ending config files in .conf. I'd almost rather have it fail quickly and noisily with some error "Directories are no longer supported, use [some alternative glob thingy] instead". Personally, I have had bad experiences with systems reading only certain files from a directory when on other similar systems (older versions?) it didn't have that behavior.

So my proposal is:

• Remove directory support and give users a very clear error message indicating the change and some possible remediation steps.
• Optional: Make our packages ship service configurations (init scripts, etc) that default to /etc/logstash/conf.d/*.conf

The above, I hope, would solve your problem of .conf.bak files being read unintentionally and also hopefully wouldn't silently surprise users when they upgrade.

Thoughts?

[magnusbaeck]

Optional: Make our packages ship service configurations (init scripts, etc) that default to /etc/logstash/conf.d/*.conf

I don't consider this optional. Some kind of support for globbing configuration files is necessary.

I suppose that means Logstash's command line interface would be changed to not accept configuration files via the -f option but rather as positional arguments?

[jordansissel]

@magnusbaeck oops, I may have been confusing.

Some kind of support for globbing configuration files is necessary

I completely agree. Summary of my proposal roughly:

• Globbing: Yes -- -f '/etc/logstash/*.conf'
• Single files: yes -- -f '/etc/logstash/foo.conf'
• Directory: no -- -f '/etc/logstash/'

My proposal is to remove directory support only. Glob support would stay, so users would still be able to do -f '/some/path/*.conf' or even -f /some/path/*/*.conf. Does this make sense?

[jordansissel]

My optional suggestion was to ensure that our system packages default to using something like -f '/etc/logstash/conf.d/*.conf' at some point in the future to help prevent further confusion.

Sorry if I was being confusing; let me know if I'm making sense :)

[magnusbaeck]

The problem with -f '/etc/logstash/*.conf' is that it runs contrary to just about every Unix tool out there (which is reflected in users' expectations of how Logstash should work). Glob expansion is the responsibility of the shell, and requiring users to quote the patterns or escape wildcard characters to avoid confusing error messages is not a good idea.

[cdenneen]

I tend to agree explicitly adding wildcards could get messy and the
directory support should be kept just intentionally only read *.conf files.
If you feel this is too much of a problem for users I can close this issue.
On Wed, Mar 9, 2016 at 7:29 AM Magnus Bäck notifications@github.com wrote:

The problem with -f '/etc/logstash/*.conf' is that it runs contrary to
just about every Unix tool out there (which is reflected in users'
expectations of how Logstash should work). Glob expansion is the
responsibility of the shell, and requiring users to quote the patterns or
escape wildcard characters to avoid confusing error messages is not a good
idea.

—
Reply to this email directly or view it on GitHub
#1831 (comment).

[jordansissel]

@magnusbaeck I agree with your philosophy about unix tooling and that the shell can do the globbing in most cases. However, Logstash has for years supported this globbing via -f '/some/*glob*/*here.conf' with success. Further, rsync, git, apache, and many other tools ranging from small CLI tools to full applications have their own globbing support external to the shell. Additionally, Logstash supports Windows, and DOS BATCH files don't really do globbing, for example:

C:\Users\jls>echo *
*

And we support Windows.

So what I"m saying is the -f flag already supports glob syntax and has for a long time -- and I am not proposing a change to that, but if you want to propose that, I'm open to discussing removing glob support in a separate ticket.

[jordansissel]

directory support should be kept just intentionally only read *.conf files

I don't feel this change would be safe for users as folks who do not name their files with .conf and use a directory for configuration, today, would have quite unexpected (and confusing) behavior changes when they upgrade. I am not comfortable with putting that burden on these users.

I'm still open to my proposal as stated, but right now, I am against having a directory imply *.conf for reasons stated above.

[magnusbaeck]

rsync, git, apache, and many other tools ranging from small CLI tools to full applications have their own globbing support external to the shell.

In the rsync and Git cases they need to have their own globbing since the shell can't do it for them which at least for me is much easier to understand. The important part here is making sure that logstash -f *.conf (note absense of quoting) won't result in a really weird error message when the shell expands it to logstash -f foo.conf bar.conf.

DOS BATCH files don't really do globbing

Indeed not, which makes cross-platform programming a PITA. Well, it's one of those things that make Windows programming in general a PITA.

So what I"m saying is the -f flag already supports glob syntax and has for a long time -- and I am not proposing a change to that, but if you want to propose that, I'm open to discussing removing glob support in a separate ticket.

I see no reason to change that.

[jordansissel]

The important part here is making sure that logstash -f *.conf (not absense of quoting) won't result in a really weird error message when the shell expands it to logstash -f foo.conf bar.conf.

Let's open a different issue to discuss this point.

[rhoml]

Any news about this change @jordansissel ?

[jordansissel]

@rhoml I'm not sure what the exact proposal is anymore, hah. Can someone write up a summary of the behavior that is being proposed? I'm open to whatever :)

[suyograo]

@jordansissel I would like to pick this one up for 6.0. A few users have got hit by this and 6.0 is a good time to do this change. I can summarize my proposal here so we can discuss:

1. Add a new setting in logstash.yml that exposes an array setting: config.dir.inclusion_glob. The default is [*.conf].
2. Directory support in -f is allowed, but it only picks up config files based on the glob specified above.

This way we can harden it OOTB, but allow for users who don't use .conf in their directory.

WDYT?

[suyograo]

hmm, your proposal in #6979 actually makes more sense. I'll move forward with that.