-
Notifications
You must be signed in to change notification settings - Fork 681
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
elasticsearch resource #1956
elasticsearch resource #1956
Conversation
5fbbbc3
to
12003d7
Compare
5b16a2a
to
5ed9d31
Compare
Signed-off-by: Rony Xavier <[email protected]> Signed-off-by: Aaron Lippold <[email protected]>
5ed9d31
to
1530818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @rx294 - welcome to the InSpec community and thanks for your first PR!
I've left you some feedback - please don't hesitate to ask if you have any questions. Thanks!
docs/resources/elasticsearch.md.erb
Outdated
* `{ should eq 'value' }` is the value that is expected | ||
|
||
## Supported Properties | ||
* `'cluster_name'`, `'node_id'`, `'node_name'`,`'host'`, `'ip'`, `'version'`, `'build_hash'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just make each item a single bullet item, alphabetized? Makes it easier to read through and easier for a user to find what they're looking for.
docs/resources/elasticsearch.md.erb
Outdated
### cluster_name ([String]) | ||
|
||
describe elasticsearch.where { node_name == 'package-centos-72' } do | ||
its('cluster_name.first') { should match 'elasticsearch' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the cmp
matcher will change a single-element array to a non-array, I'd change this to:
its('cluster_name') { should cmp 'elasticsearch' }
docs/resources/elasticsearch.md.erb
Outdated
# Verify the version of the node that matches the name node2 | ||
|
||
describe elasticsearch.where { node_name == 'package-centos-72' } do | ||
its('version.first') { should match '5.4.1' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I'd use the cmp
matcher here rather than teaching users to use first
lib/inspec/resource.rb
Outdated
@@ -90,6 +90,7 @@ def self.validate_resource_dsl_version!(version) | |||
require 'resources/docker_image' | |||
require 'resources/docker_container' | |||
require 'resources/etc_group' | |||
require 'resources/elasticsearch.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop the .rb
from this
lib/resources/elasticsearch.rb
Outdated
module Inspec::Resources | ||
class Elasticsearch < Inspec.resource(1) | ||
name 'elasticsearch' | ||
desc 'See Description below' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The desc
contents are displayed in the inspec shell
output... so there would be no description printed. Please uncomment similar to other resources.
lib/resources/elasticsearch.rb
Outdated
|
||
def parse_node_process(node) | ||
{ | ||
'process' => node, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would cause the node hash to have a full copy of itself. Did you forget to set a key here?
lib/resources/elasticsearch.rb
Outdated
end | ||
|
||
def to_s | ||
'Elasticsearch Environment' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Environment
is a loaded term... how about Elasticsearch ClusterInfo
?
www/Gemfile.lock
Outdated
@@ -1,275 +0,0 @@ | |||
PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please revert this change from your commit? The Gemfile.lock for the www
should remain, and if there is a desire to drop it, we should do that as a separate PR. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adamleff, I have reorganized and made the recommended changes to this PR. I belive I have reverted the Gemfile.lock, please let me know if it has not been reverted.
Please review it when you get a chance. Thank you
_(resource.version).must_equal ["5.4.1"] | ||
end | ||
|
||
it 'Verify elasticsearch node mlockall state' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation lines 16-34 is off.. should be in line with the block above in lines 11-14.
require 'inspec/resource' | ||
|
||
describe 'Inspec::Resources::Elasticsearch' do | ||
describe 'elasticsearch' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a duplicative describe
block... can we remove this?
lib/resources/elasticsearch.rb
Outdated
# copyright: 2017 | ||
# author: Rony Xavier, [email protected] | ||
# author: Aaron Lippold, [email protected] | ||
# license: All rights reserved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line. InSpec is released under Apache 2.0. We have a pending PR to remove this line from all our files (acknowledging that you probably copy/pasted this from another existing file 🙂 ).
# encoding: utf-8 | ||
# copyright: 2017 | ||
# author: Rony Xavier, [email protected] | ||
# license: All rights reserved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line. InSpec is released under Apache 2.0. We have a pending PR to remove this line from all our files (acknowledging that you probably copy/pasted this from another existing file 🙂 ).
@rx294 just a heads up on this PR, it now has merge conflicts. Be sure to rebase your branch on the latest master and address those issues. One is on |
…icsearch_resource Signed-off-by: Aaron Lippold <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rx294 I may not have cycles to give this a thorough review until I return from my vacation, but perhaps the other maintainers might.
However, right out of the gate, I notice that your change still deletes the Gemfile.lock file and adds it to the .gitignore
file. Neither of these changes can be accepted in this PR.
7e7e0fb
to
0a4e5b9
Compare
Signed-off-by: Rony Xavier <[email protected]>
0a4e5b9
to
848d595
Compare
01c73b7
to
efe96e6
Compare
Signed-off-by: Rony Xavier <[email protected]>
…icsearch_resource Signed-off-by: Aaron Lippold <[email protected]>
b63aed7
to
7112051
Compare
Signed-off-by: Rony Xavier <[email protected]>
7112051
to
8d206b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, let's just tidy this up!
lib/resources/elasticsearch.rb
Outdated
class Elasticsearch < Inspec.resource(1) | ||
name 'elasticsearch' | ||
desc "Use the Elasticsearch InSpec audit resource to test the Elasticsearch | ||
configuration settings. The data is extract from the live elastic search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 51-53, please indent 2 spaces
lib/resources/elasticsearch.rb
Outdated
its('name') { should_not include 'node_name' } | ||
end | ||
elasticsearch.nodes.os.each do |nodeos| | ||
describe nodeos do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
66-68, needs one more space
lib/resources/elasticsearch.rb
Outdated
end | ||
end | ||
elasticsearch.nodes.version.each do |node_version| | ||
describe node_version do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
71-73, fix indentation, and please git rid of that pesky tab on line 72 :)
lib/resources/elasticsearch.rb
Outdated
its('path.conf') {should cmp '/etc/elasticsearch'} | ||
end | ||
end | ||
elasticsearch.nodes.where{name == 'package-centos-72' }.settings.each do |node_settings| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 80 - more tabs! You're killing me... slowly... 🙂
lib/resources/elasticsearch.rb
Outdated
" | ||
|
||
def initialize(url = 'http://localhost:9200/_nodes/') | ||
return skip_resource 'Package `curl` not avaiable on the host' unless inspec.package('curl').installed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to:
unless inspec.command('curl').exist?
lib/resources/elasticsearch.rb
Outdated
if command.stderr =~ /Failed to connect/ | ||
return skip_resource 'Connection refused please check ip and port provided' | ||
end | ||
@content = JSON.parse(command.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protect against a failed JSON parse to avoid a Ruby exception unwinding the whole InSpec run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this, and the few lines that follow, should be broken out into a helper method.
def initialize
# blah blah
@content = read_es_data(url)
end
def read_es_data(url)
# call curl
# handle error, skip_resource if curl fails
@content = JSON.parse(curl_output)
rescue JSON::ParserError => e
skip_resource "Couldn't parse ES data: #{e.message}"
end
lib/resources/elasticsearch.rb
Outdated
node.node_id = node_id | ||
node.plugin_list = plugin_list(node) | ||
node.module_list = module_list(node) | ||
node.cluster = node.settings.cluster.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to think about, should this be cluster_name
?
And for sake of clarity, I would set this from node_data vs. setting it from itself...
node.cluster_name = node_data['settings']['cluster']['name']
lib/resources/elasticsearch.rb
Outdated
node.cluster = node.settings.cluster.name | ||
@nodes.push(node) | ||
end | ||
rescue JSON::ParserError => _e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rescue JSON::ParserError
lib/resources/elasticsearch.rb
Outdated
@nodes.push(node) | ||
end | ||
rescue JSON::ParserError => _e | ||
return Hashie::Mash.new({}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hashie::Mash.new
lib/resources/elasticsearch.rb
Outdated
def plugin_list(node) | ||
plugins_list = [] | ||
node.plugins.each do |plugins| | ||
plugins_list.push(plugins.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugins_list << plugins.name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your education only :)
8d206b2
to
b63aed7
Compare
…ippold/inspec into al/elasticsearch_resource
Signed-off-by: Rony Xavier <[email protected]>
@adamleff hey Adam this PR is ready...although looks like one of the Travis tests are failing for an unrelated issue. Thank you |
lib/resources/elasticsearch.rb
Outdated
def read_es_data(url) | ||
cmd = inspec.command("curl #{url}") | ||
if !cmd.exit_status.zero? | ||
return skip_resource "Error using the curl #{url}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps as a follow-up to this PR, I'd suggest adding the stderr from the command to this message in hopes that it will help the user troubleshoot:
return skip_resource "Error fetching Elastcsearch data from #{url}: #{cmd.stderr}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @adamleff that makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rx294 this looks good, just one thing that we need to clean up in the read_es_data method re: error handling. I don't think we'll ever get to one of your error handling steps because of the way the method is written.
lib/resources/elasticsearch.rb
Outdated
if !cmd.exit_status.zero? | ||
return skip_resource "Error using the curl #{url}" | ||
end | ||
if cmd.stderr =~ /Failed to connect/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If curl
fails, it will catch at line 97 above and then we'll return. I don't think you'll ever get to this part of your method if it failed to connect.
If you want to provide a specific "connection refused" skip message, you will likely need to move lines 100-102 above line 97, and let the if
statement at line 97 be a "catch-all error except connection refused events" handler. Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the catch. That makes sense.
Signed-off-by: Rony Xavier <[email protected]>
@adamleff I am getting a travisci error as below: example inheritance profile#test_0004_ensure json/check command do not fetch remote profiles if vendored [/home/travis/build/chef/inspec/test/functional/inspec_vendor_test.rb:61]: It looks unrelated to the elasticsearch resource, please let me know if it is not. Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super-close, @rx294 -- just some doc fixes and a discussion about the default URL. Thanks!
lib/resources/elasticsearch.rb
Outdated
end | ||
elasticsearch.nodes.os.each do |nodeos| | ||
describe nodeos do | ||
its('name'){ should_not cmp 'MacOS' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need space between )
and {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cluster using the curl command `curl localhost:9200/_nodes/`. The resource | ||
makes uses of FilterTable and its functionality can be used " | ||
|
||
example " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a newline between each describe block so it's clearer and consistent with other documentation? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/resources/elasticsearch.rb
Outdated
end | ||
elasticsearch.nodes.version.each do |node_version| | ||
describe node_version do | ||
it{ should be > '1.2.0' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space needed between it
and {
lib/resources/elasticsearch.rb
Outdated
it{ should be > '1.2.0' } | ||
end | ||
end | ||
elasticsearch.nodes.where{os.name == 'Linux' }.settings.each do |node_settings| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space needed between where
and {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/resources/elasticsearch.rb
Outdated
end | ||
elasticsearch.nodes.where{os.name == 'Linux' }.settings.each do |node_settings| | ||
describe node_settings do | ||
its('path.conf') {should cmp '/etc/elasticsearch'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add wrapper spaces within the braces...
{ should cmp '/etc/elasticsearch' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/resources/elasticsearch.rb
Outdated
end | ||
elasticsearch.nodes.where{name == 'package-centos-72' }.settings.each do |node_settings| | ||
describe node_settings do | ||
its('path.conf') {should cmp '/etc/elasticsearch'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add wrapper spaces within the braces...
{ should cmp '/etc/elasticsearch' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you @adamleff , I should have known better by now haha
lib/resources/elasticsearch.rb
Outdated
end | ||
" | ||
|
||
def initialize(url = 'http://localhost:9200/_nodes/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to include the _nodes
part here? I think perhaps this should be the base URL for wherever Elasticsearch lives, and then we can call /_nodes
within the resource. That way if we want to hit other endpoints later, such as /_cluster/health
, we don't have to rework this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _nodes part is important because it exposes all the settings we are exposing using the resource and our parsers are expecting the json structure the '_nodes' curl request returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rx294 that's exactly my point. Your resource depends on hitting /_nodes
to do its job, but that is not the top-level URL to an Elasticsearch node/cluster. You are shifting an implementation detail responsibility onto the consumer of your resource.
You should ask expect the user to give you the top-level URL to their cluster, and then you as the resource author should tack on the /_nodes
endpoint to the URL before you start making curl calls. Otherwise, you are forcing users to know that detail... and in the future, if we need to retrieve data from different endpoints (such as /_cluster
or /_cat
, we don't have to do janky regsub or break backward compatibility.
_(node_roles).must_include 'master' | ||
end | ||
end | ||
resource_with_url = load_resource('elasticsearch','http://localhost:9200/_nodes/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test with a non-default URL. This really isn't testing anything other than our ability to pass in a second argument. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: Rony Xavier <[email protected]>
@@ -32,7 +32,7 @@ | |||
_(node_roles).must_include 'master' | |||
end | |||
end | |||
resource_with_url = load_resource('elasticsearch','http://localhost:9200/_nodes/') | |||
resource_with_url = load_resource('elasticsearch','http://localhost:9200/_nodes/ -u es_admin:password') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I meant... that's not part of the URL, that's now a command flag you pass to curl, but in the future, this resource may use a different method of fetching an HTTP endpoint.
It's possible to have elasticsearch clusters on different hostnames and different URIs. For example:
http://mydatacenter.mycompany.biz/myelasticsearch
... in which case, your resource would fetch from http://mydatacenter.mycompany.biz/myelasticsearch/_nodes
…icsearch_resource
Signed-off-by: Adam Leff <[email protected]>
Status as I see it today: my concern about the user needing to supply |
Signed-off-by: Rony Xavier <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
…ippold/inspec into al/elasticsearch_resource
…ippold/inspec into al/elasticsearch_resource Signed-off-by: Rony Xavier <[email protected]>
…ippold/inspec into al/elasticsearch_resource Signed-off-by: Rony Xavier <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
Hey @adamleff . I have made the updates. Please review it when you get a chance. Thank you! |
Signed-off-by: Rony Xavier <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rx294 I only looked at it briefly, and this is closer, but I believe you made a decision that will hinder some users' ability to use it.
Specifically, you eliminated passing a URL and instead opted for parameters es_host and es_port. This means users to have Elasticsearch installed on a machine with a non-root URI (i.e. http://myhost:9201/elasticsearch) will not be able to use this resource. We should really go back to using URIs instead of parameterizing the host and port
Also, we should use better parameter names - there is no need to use the es_
prefix since the parameters are specific to this resource. Let's try to use clear and easy-to-understand parameter names.
docs/resources/elasticsearch.md.erb
Outdated
|
||
* `es_ip` elasticsearch ip address, default : 0.0.0.0 | ||
* `es_port` elasticsearch port address, default : 9200 | ||
* `es_user` elasticsearch user name, default : nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to prefix with es_ - use username
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/resources/elasticsearch.md.erb
Outdated
* `es_ip` elasticsearch ip address, default : 0.0.0.0 | ||
* `es_port` elasticsearch port address, default : 9200 | ||
* `es_user` elasticsearch user name, default : nil | ||
* `es_pass` elasticsearch user password, default : nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to prefix with es_ - use password
docs/resources/elasticsearch.md.erb
Outdated
* `es_port` elasticsearch port address, default : 9200 | ||
* `es_user` elasticsearch user name, default : nil | ||
* `es_pass` elasticsearch user password, default : nil | ||
* `https(bool)` elasticsearch connection protocol, default : true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have this as a parameter - just accept a full URI to the elasticsearch top-level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/resources/elasticsearch.md.erb
Outdated
* `es_user` elasticsearch user name, default : nil | ||
* `es_pass` elasticsearch user password, default : nil | ||
* `https(bool)` elasticsearch connection protocol, default : true | ||
* `self_signed_cert(bool)` elasticsearch uses self-signed certificate , default : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more reasons you need to disable SSL certificate validation other than a self-signed cert is in use. For consistency with other resources, please name this ssl_verify
and you can default to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/resources/elasticsearch.md.erb
Outdated
|
||
where | ||
|
||
* `es_ip` elasticsearch ip address, default : 0.0.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should accept a URI and not an IP/port - see my top-level review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Done.
lib/resources/elasticsearch.rb
Outdated
return skip_resource "Couldn't parse ES data: #{e.message}" | ||
end | ||
|
||
return skip_resource "Security Exception: #{@content['error']}" unless @content['error'].nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this? What's a "security exception"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have modified it to show status of the error from elasticsearch
lib/resources/elasticsearch.rb
Outdated
|
||
cmd = inspec.command("curl #{url}") | ||
return skip_resource 'Connection refused please check ip and port provided' if cmd.stderr =~ /Failed to connect/ | ||
return skip_resource "Connection refused Peer's Certificate issuer is not recognized" if cmd.stderr =~ /Peer's Certificate issuer is not recognized/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? The stderr will be displayed in the skip_resource message in line 112.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would help capture the reason of failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but the reason for the failure you're matching on ("Peer's Certificate issue is not recognized") is in the stderr output already, and it will be output in line 112. The skip message you're returning in this line 111 is not adding any additional value that wouldn't be handled properly in line 112.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh i remmember, the Peer certificate stderr is this big ugly block
"% Total % Received % Xferd Average Speed Time Time Time Current\n Dload Upload Total Spent Left Speed\n\r 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0\r 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0\ncurl: (60) Peer's Certificate issuer is not recognized.\nMore details here: http://curl.haxx.se/docs/sslcerts.html\n\ncurl performs SSL certificate verification by default, using a "bundle"\n of Certificate Authority (CA) public keys (CA certs). If the default\n bundle file isn't adequate, you can specify an alternate file\n using the --cacert option.\nIf this HTTPS server uses a certificate signed by a CA represented in\n the bundle, the certificate verification probably failed due to a\n problem with the certificate (it might be expired, or the name might\n not match the domain name in the URL).\nIf you'd like to turn off curl's verification of the certificate, use\n the -k (or --insecure) option.\n"
I figured this would be a cleaner skip message of a known possible error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great reason. Let's add a comment above this line that explains why we're specifically catching this one error type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Signed-off-by: Rony Xavier <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
Signed-off-by: Rony Xavier <[email protected]>
@rx294 I still think there are some issues with this that I've unsuccessfully tried to communicate (i.e. the URL should be an argument rather than an item in the option hash), and there are still issues (i.e. the default URL shouldn't use 0.0.0.0 or https by default), so I'm going to adopt your PR with attribution and try and push it across the finish line. I'm traveling for the next couple of days, so I should be able to work on this sometime next week. Unfortunately, it's just taking too much time to try and address the remaining issues through the PR back-and-forth process. I'll close this PR with an new updated one once I've done so. Thanks. |
Got it. thank you adam |
This is a new resource for testing an Elasticsearch cluster. It operates by fetching the `_nodes` endpoint from a given Elasticsearch node and collects data about each node in a cluster, even if there's only a single node. This work is based on inspiration from an initial PR #1956 submitted by @rx294. Signed-off-by: Rony Xavier <[email protected]> Signed-off-by: Aaron Lippold <[email protected]> Signed-off-by: Adam Leff <[email protected]>
Superseded by PR #2261 - thanks for your work on this. |
* new resource: elasticsearch resource, test cluster/node state This is a new resource for testing an Elasticsearch cluster. It operates by fetching the `_nodes` endpoint from a given Elasticsearch node and collects data about each node in a cluster, even if there's only a single node. This work is based on inspiration from an initial PR #1956 submitted by @rx294. Signed-off-by: Rony Xavier <[email protected]> Signed-off-by: Aaron Lippold <[email protected]> Signed-off-by: Adam Leff <[email protected]> * Reduce mock data on non-default tests Signed-off-by: Adam Leff <[email protected]>
Documentation and Unit Tests
Signed-off-by: Rony Xavier [email protected]
Signed-off-by: Aaron Lippold [email protected]