check-elb-health.rb doesn't look into $HOME/.aws/credentials

[arthurzenika]

sensu@host:~$ cat ~/.aws/credentials
[default]
region = eu-west-1
aws_access_key_id = xxxx
aws_secret_access_key = xxxx
sensu@host:~$ /usr/local/bin/check-elb-latency.rb -r eu-west-1
CheckELBLatency OK: <AWS::ELB::LoadBalancer name:elb-id>; (Average within 60 seconds between 2017-11-02 11:16:35 +0000 to 2017-11-02 11:17:35 +0000)
sensu@host:~$ /usr/local/bin/check-elb-health.rb -n elb-id
Digest::Digest is deprecated; use Digest
Digest::Digest is deprecated; use Digest
ELBHealth CRITICAL: An issue occured while communicating with the AWS EC2 API: AWS access keys are required to operate on ELB

[majormoses]

Thanks for reporting this, it is using right_aws it is not using the aws sdk directly it is using rightscales gem which has not been updated since June 2015 and needs to be re-written to use the aws-sdk (v2) and our common config provider.

[multani]

@majormoses There are 3 "ELB health" checks currently in the repository, wouldn't it make sense to drop check-elb-health.rb and check-elb-health-fog.rb, which are not using the AWS SDK directly and rename check-elb-health-sdk.rb, which uses the AWS SDK to check-elb-health.rb?

Considering the options provided by the 3 plugins, the only breaking changes would be the removal of the -a/--aws-access-key and -k/--aws-secret-access-key options from the "fog" variant, which don't exist in the the other variants but otherwise all the options are the same or are a subset of the "sdk" variant.

[multani]

Also, @arthurlogilab are you able to try the "sdk" variant of this check to see if it works, for some reason all my ELB are reported as "not active" and I can't test any of these checks :'(

In your setup, running something like this should work:

sensu@host:~$ /usr/local/bin/check-elb-health-sdk.rb -n elb-id

[majormoses]

There are 3 "ELB health" checks currently in the repository, wouldn't it make sense to drop check-elb-health.rb and check-elb-health-fog.rb, which are not using the AWS SDK directly and rename check-elb-health-sdk.rb, which uses the AWS SDK to check-elb-health.rb?

I checked and this does seem to be the case so this makes sense to me. @eheydrick any concerns about this that I am not seeing?

Hmm, this check seems horribly broken:

$ bundle exec ./bin/check-elb-health-sdk.rb -r us-west-2 -n dev-pub-elb-sensu
ELBHealth OK: 
$ bundle exec ./bin/check-elb-health-sdk.rb -r us-west-2 -n non-existent-elb
ELBHealth OK:

@eheydrick am I missing something? Looking at your testing artifacts I don't see what I might be doing wrong.

[eheydrick]

I think we should remove check-elb-health and check-elb-health-fog in favor of check-elb-health-sdk. Really no reason to keep old implementations around.

@majormoses that looks like expected output for a healthy single ELB. -v will give you details. Checking a non-existent ELB should probably result in a non-OK.

[majormoses]

I think the first one is definitely a bug and calls into question confidence in the whole check actually working. Minimally there is a bug when the ELB is not present.

$ bundle exec ./bin/check-elb-health-sdk.rb -r us-west-2 -n non-existent-elb -v
ELBHealth OK:
$ bundle exec ./bin/check-elb-health-sdk.rb -r us-west-2 -n dev-pub-elb-sensu -v
ELBHealth OK: dev-pub-elb-sensu => healthy.

Really no reason to keep old implementations around.

Agreed, question is whether it's worth making a breaking change to rename check-elb-health-sdk.rb to check-elb-health.rb? I do think it makes sense as unless it specifically has some other lib it uses other than the aws sdk the current name seems silly. We could do to ease the pain of transition is to rename it and create a binstub that marks the file for deprecation for a major version to be removed at some point. This would avoid a breaking change on that front as it gives users an easy transition and notice that they should update their config to use the new check name. Thoughts?

[arthurzenika]

@multani I have indeed disabled the check-elb-health.rb in favor of using the check-elb-nodes.rb which works fine for me

/usr/local/bin/check-elb-nodes.rb -r eu-west-1 -n valid-id -W 50

@majormoses I have the same bug with the check-elb-health-sdk.rb

What's the different between check-elb-health-sdk and check-elb-nodes ?

[majormoses]

What's the different between check-elb-health-sdk and check-elb-nodes ?

Well taking a super quick peek at them feature parity wise check-elb-health-sdk.rb checks if any instances are unhealthy where check-elb-nodes.rb checks the number of unhealthy instances.