Skip to content
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

RFC: WIP: Pipeline configuration improvements #380

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .fixtures.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,5 @@ fixtures:
apt: puppetlabs/apt
elastic_stack: elastic/elastic_stack
stdlib: puppetlabs/stdlib
concat: puppetlabs/concat
zypprepo: darin/zypprepo

symlinks:
logstash: #{source_dir}
2 changes: 1 addition & 1 deletion manifests/init.pp
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@
$settings = {},
$startup_options = {},
$jvm_options = [],
Array $pipelines = [],
Variant[Boolean,Array[Hash,1]] $pipelines = false,
Boolean $manage_repo = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to do this without overloading the $pipelines variable? Having a variable that can be either an array or a boolean is a bit odd and it kinda goes against Puppet's continuing march toward type safety.

Also, what would happen if I declared an array of pipelines and also added another one as a defined type elsewhere? Can I do that with this implementation?

I think the ideal API would allow:

  • 0 or more pipelines passed as an array to the class
  • 0 or more pipelines declared as defined types

Ideally, neither of these should require a flag to "turn them on" and they should be able to co-exist. Do you think it's possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

The good news is that you like the idea of being able to declare pipelines outside of the main class. The tricky bit has been making the implementation backwards compatible and compatible with people not using multiple pipelines. Maybe I will need a separate flag parameter. I'll see how I can make this better. Maybe we could deprecate the old Array implementation and support passing a hash of logstash::pipeline resources to the main class instead? That's quite a common pattern for being able to create defined types from a module's base class.

Also, what would happen if I declared an array of pipelines and also added another one as a defined type elsewhere? Can I do that with this implementation?

That does work :)

)
{
Expand Down
41 changes: 41 additions & 0 deletions manifests/pipeline.pp
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
define logstash::pipeline
(
Hash $config = {},
Optional[String] $content = undef,
Optional[String] $source = undef,
Optional[Stdlib::Unixpath] $path = undef,
Optional[String] $id = undef,
)
{
unless $logstash::pipelines { fail('You must set base class `logstash`\'s `pipeline` parameter to `true` to use this defined type.') }
if $content and $source { fail('You can\'t specify both `content` and `source`') }

# overwrite keys in $config with values from $path and $id (if they're set).
$real_config = $config + {
'pipeline.id' => $id,
'path.config' => $path,
}.filter |$key, $val| { $val =~ NotUndef }

unless 'pipeline.id' in $real_config { fail("logstash::pipeline ${title} is missing a pipeline id") }

$yaml = [$real_config].to_yaml
$yaml_without_header = join($yaml.split("\n")[1,-1],"\n")

concat::fragment { "logstash pipeline ${title}":
target => '/etc/logstash/pipelines.yml',
content => "${yaml_without_header}\n",
}

if $content or $source {
if 'path.config' in $real_config {
$real_path = $real_config['path.config']
} else {
fail('To use logstash::pipeline with `content` or `source`, the `config` hash must contain a `path.config` key or you must specify `path`')
}
logstash::configfile { "Config file for pipeline.id ${real_config['pipeline.id']}":
content => $content,
source => $source,
path => $real_path,
}
}
}
27 changes: 21 additions & 6 deletions manifests/service.pp
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,29 @@
# ..and pipelines.yml, if the user provided such. If they didn't, zero out
# the file, which will default Logstash to traditional single-pipeline
# behaviour.
if(empty($pipelines)) {
file {'/etc/logstash/pipelines.yml':
content => '',
if $pipelines {
# Either a non empty array of hashes, or true.
concat { '/etc/logstash/pipelines.yml':
ensure => present,
}
}
else {
concat_fragment { 'pipelines.yml header':
content => "---\n",
target => '/etc/logstash/pipelines.yml',
order => 1,
}

if $pipelines =~ Array {
$pipelines.each |Hash $pipeline| {
$unique_resource_id = digest(String($pipeline))
logstash::pipeline { $unique_resource_id:
config => $pipeline,
}
}
}
} else {
# Traditional single-pipeline behaviour.
file {'/etc/logstash/pipelines.yml':
content => template('logstash/pipelines.yml.erb'),
content => '',
}
}

Expand Down
4 changes: 4 additions & 0 deletions metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
"name": "puppetlabs/stdlib",
"version_requirement": ">=3.2.0 <6.0.0"
},
{
"name": "puppetlabs/concat",
"version_requirement": ">=4.1.0 <6.0.0"
},
{
"name": "elastic/elastic_stack",
"version_requirement": ">=6.0.0 <7.0.0"
Expand Down
2 changes: 1 addition & 1 deletion templates/logstash.yml.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<%# removing the 'path.config' setting. -%>
<%# -%>
<%# REF: https://github.com/elastic/logstash/issues/8420 -%>
<% @settings.delete('path.config') unless @pipelines.empty? -%>
<% @settings.delete('path.config') if @pipelines -%>
<%# -%>
<%# Similiarly, when using centralized pipeline management, path.config -%>
<%# is an invalid setting, and should be removed. -%>
Expand Down