kemra102 / puppet-cloudwatchlogs

Puppet module for configuring AWS Cloudwatch Logs on Amazon Linux, Ubuntu, Red Hat & CentOS EC2 instances.
BSD 2-Clause "Simplified" License
11 stars 36 forks source link

Adding support for IAM based credentials, customizable stream name. #1

Closed flyinbutrs closed 9 years ago

flyinbutrs commented 9 years ago

As it says.... I made region, secret key, and access key id optional and assume IAM roles will be available if not provided. I also made streamname a variable that defaults to {instance-id} so that you can supply an alternate one at the class level. I suppose this could be modded to be at the log level, but that's for another day.

Also, I removed the "log.sort.map" as I'm not entirely sure why a single hash should be sorted and then mapped... If there's a good reason, reject and I'll put it back.

kemra102 commented 9 years ago

Have the changes been tested? I seem to remember the 'map' being needed but not the 'sort' necessarily, though I can't remember why exactly right now.

If it's not tested let me know and I'll spin up some instances, thanks.

flyinbutrs commented 9 years ago

I have tested them with IAM roles and it worked perfectly. I haven't verified that it still works properly when you specify credentials, but I can't imagine that would have broken. Tested with puppet 3.7.5.

flyinbutrs commented 9 years ago

Oh, and re: the map/sort issue, I'm not sure how it would have worked as written. It seems to me that it could have been done in the reverse order... sort and map the array of logs, then use each on the resulting hashes to populate the entries. But as written, doing each on the array first, the |log| inside of the each loop would be a hash, and there's no map method for hashes. I'm curious if it worked in the first place, and why.

kemra102 commented 9 years ago

You're right it shouldn't work, not sure what was going on before but we definitely had it working.

It was bugging me so I tested it, your code is giving me the following config:

state_file = /var/awslogs/state/agent-state

[path]
datetime_format = %b %d %H:%M:%S
file = /var/log/messages
buffer_duration = 5000
log_stream_name = {instance-id}
initial_position = start_of_file
log_group_name = path
[name]
datetime_format = %b %d %H:%M:%S
file = Messages
buffer_duration = 5000
log_stream_name = {instance-id}
initial_position = start_of_file
log_group_name = name
[path]
datetime_format = %b %d %H:%M:%S
file = /var/log/secure
buffer_duration = 5000
log_stream_name = {instance-id}
initial_position = start_of_file
log_group_name = path
[name]
datetime_format = %b %d %H:%M:%S
file = Secure
buffer_duration = 5000
log_stream_name = {instance-id}
initial_position = start_of_file
log_group_name = name

Which is more or less the same as what the current code is doing.

I played around a little with it and the following code should get the correct format (we're dealing with an array of hashes so it's not as straightforward as just using an each):

state_file = <%= @state_file %>

<% @logs.each do |log| -%>
[<%= log['name'] %>]
datetime_format = %b %d %H:%M:%S
file = <%= log['path'] %>
buffer_duration = 5000
log_stream_name = <%= @streamname %>
initial_position = start_of_file
log_group_name = <%= log['name'] %>
<% end -%>

Do you want to amend your request to include this change? If not I can do it later.

flyinbutrs commented 9 years ago

How did you declare your class to get that? And on what version of puppet? I am currently using my version and I'm not getting those repeat values. For instance:

class {'cloudwatchlogs':
    streamname => $::hostname,
    logs       => [
      {'Syslog' => '/var/log/syslog'},
      {'Secure' => '/var/log/auth.log'},
      {'NginxError' => "/var/log/nginx/${common::pubweb_vhost}.error.log"},
    ]
  }

Generates this file for me:

state_file = /var/awslogs/state/agent-state

[Syslog] datetime_format = %b %d %H:%M:%S file = /var/log/syslog buffer_duration = 5000 log_stream_name = {scrubbed-hostname} initial_position = start_of_file log_group_name = Syslog [Secure] datetime_format = %b %d %H:%M:%S file = /var/log/auth.log buffer_duration = 5000 log_stream_name = {scrubbed-hostname} initial_position = start_of_file log_group_name = Secure [NginxError] datetime_format = %b %d %H:%M:%S file = /var/log/nginx/{scrubbed-hostname}.error.log buffer_duration = 5000 log_stream_name = {scrubbed-hostname} initial_position = start_of_file log_group_name = NginxError

kemra102 commented 9 years ago

Very strange, I have:

class { 'cloudwatchlogs':
  logs       => [
    { name => 'Messages', path => '/var/log/messages', },
    { name => 'Secure', path => '/var/log/secure', },
  ],
  region                => 'eu-west-1',
  aws_access_key_id     => 'AKIAIOSFODNN7EXAMPLE',
  aws_secret_access_key => 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY',
}

This is on Puppet 3.7.5 on a CentOS 6 box with the default Ruby 1.8.7 install.

flyinbutrs commented 9 years ago

Oh, I see... that's not how it's documented in the readme.md. The difference is:

{'Syslog' => '/var/log/syslog'} vs {name => 'Syslog', path => '/var/log/syslog'}

The former is how it's listed in the readme.md, but the latter is probably better anyway because it also would make it trivial to add a per-logfile streamname variable or add other params on a per-logfile basis.

kemra102 commented 9 years ago

Just goes to show what a busy shift will do to the mind. I think what we have for now (i.e. as documented and what you are using) is best and I'll look at changing it later in a major release.

Thanks for your help.

kemra102 commented 9 years ago

Latest release pushed to Puppet Forge.