WritableDirectory#value now creates the directory if needed.

This also makes the failure reports from WritableDirectory's validation
more specific (parent directory doesn't exist, or does exist and is
isn't writable, or the path itself isn't writable, etc).

Fixes #6023
This commit is contained in:
Jordan Sissel 2016-10-06 15:15:14 -07:00
parent c0c40c04d1
commit 9a5b3436e2
2 changed files with 163 additions and 5 deletions

View file

@ -1,4 +1,5 @@
# encoding: utf-8
require "logstash/util/loggable"
module LogStash
class Settings
@ -118,6 +119,8 @@ module LogStash
end
class Setting
include LogStash::Util::Loggable
attr_reader :name, :default
def initialize(name, klass, default=nil, strict=true, &validator_proc)
@ -389,11 +392,44 @@ module LogStash
class WritableDirectory < Setting
def initialize(name, default=nil, strict=false)
super(name, ::String, default, strict) do |path|
if ::File.directory?(path) && ::File.writable?(path)
true
else
raise ::ArgumentError.new("Path \"#{path}\" is not a directory or not writable.")
super(name, ::String, default, strict)
end
def validate(path)
super(path)
if ::File.directory?(path)
if !::File.writable?(path)
raise ::ArgumentError.new("Path \"#{path}\" must be a writable directory. It is not writable.")
end
elsif ::File.symlink?(path)
# TODO(sissel): I'm OK if we relax this restriction. My experience
# is that it's usually easier and safer to just reject symlinks.
raise ::ArgumentError.new("Path \"#{path}\" must be a writable directory. It cannot be a symlink.")
elsif ::File.exist?(path)
raise ::ArgumentError.new("Path \"#{path}\" must be a writable directory. It is not a directory.")
else
parent = ::File.dirname(path)
if !::File.writable?(parent)
raise ::ArgumentError.new("Path \"#{path}\" does not exist and I cannot create it because the parent path \"#{parent}\" is not writable.")
end
end
# If we get here, the directory exists and is writable.
true
end
def value
super.tap do |path|
if !::File.directory?(path)
# Create the directory if it doesn't exist.
begin
logger.info("Creating directory", setting: name, path: path)
::FileUtils.mkdir_p(path)
rescue => e
# TODO(sissel): Catch only specific exceptions?
raise ::ArgumentError.new("Path \"#{path}\" does not exist, and I failed trying to create it: #{e.class.name} - #{e}")
end
end
end
end

View file

@ -0,0 +1,122 @@
# encoding: utf-8
require "spec_helper"
require "logstash/settings"
require "stud/temporary"
require "socket" # for UNIXSocket
describe LogStash::Setting::WritableDirectory do
let(:mode_rx) { 0555 }
let(:parent) { Stud::Temporary.pathname }
let(:path) { File.join(parent, "fancy") }
before { Dir.mkdir(parent) }
after { Dir.exist?(path) && Dir.unlink(path) rescue nil }
after { Dir.unlink(parent) }
shared_examples "failure" do
before { subject.set(path) }
it "should fail" do
expect { subject.validate_value }.to raise_error
end
end
subject do
# Create a new WritableDirectory setting with no default value strict
# disabled.
described_class.new("fancy.path", "", false)
end
describe "#value" do
before { subject.set(path) }
context "when the directory is missing" do
context "and the parent is writable" do
after {
Dir.unlink(path)
}
it "creates the directory" do
subject.value # need to invoke `#value` to make it do the work.
expect(::File.directory?(path)).to be_truthy
end
end
context "and the directory cannot be created" do
before { File.chmod(mode_rx, parent) }
it "should fail" do
expect { subject.value }.to raise_error
end
end
end
end
describe "#set and #validate_value" do
context "when the directory exists" do
before { Dir.mkdir(path) }
after { Dir.unlink(path) }
context "and is writable" do
before { subject.set(path) }
# assume this spec already created a directory that's writable... fair? :)
it "should return true" do
expect(subject.validate_value).to be_truthy
end
end
context "but is not writable" do
before { File.chmod(0, path) }
it_behaves_like "failure"
end
end
context "when the path exists" do
after { File.unlink(path) }
context "but is a file" do
before { File.new(path, "w").close }
it_behaves_like "failure"
end
context "but is a socket" do
let(:socket) { UNIXServer.new(path) }
before { socket } # realize `socket` value
after { socket.close }
it_behaves_like "failure"
end
context "but is a symlink" do
before { File::symlink("whatever", path) }
it_behaves_like "failure"
end
end
context "when the directory is missing" do
# Create a path with at least one subdirectory we can try to fiddle with permissions
context "but can be created" do
before do
# If the path doesn't exist, we want to try creating it, so let's be
# extra careful and make sure the path doesn't exist yet.
expect(File.directory?(path)).to be_falsey
subject.set(path)
end
after do
Dir.unlink(path)
end
it "should return true" do
expect(subject.validate_value).to be_truthy
end
end
context "and cannot be created" do
before do
# Remove write permission on the parent
File.chmod(mode_rx, parent)
end
it_behaves_like "failure"
end
end
end
end