add nginx_conf resource #1889
add nginx_conf resource #1889
Conversation
|
All lines in the nginx_conf mock are relevant, but we aren't yet testing for everything like server. I plan to add it in the 3rd PR in this series that adds a proper interface to the resource. |
There was a problem hiding this comment.
@arlimus this is shaping up nicely and I'm excited to see this get added to core. I need help understanding parts of this, which I believe will mean others will too. So I think we need to rethink one method or at least add comments/examples, and then add some tests for it as well.
lib/resources/nginx_conf.rb
Outdated
| def parse_nginx(path) | ||
| content = read_content(path) | ||
| data = NginxConfig.parse(content) | ||
| resolve_references(data, File.dirname(path)) |
There was a problem hiding this comment.
This may cause problems if a Windows user is scanning a Linux host since File::SEPARATOR may not be what you expect. It's usually fine, but not always...
There was a problem hiding this comment.
Do you have any suggestion on what we could do for now?
There was a problem hiding this comment.
Tested with InSpec 1.26 on 2012 R2, and it appears to be enough cross-platform-aware that this shouldn't be an issue, and I'm just being paranoid. Let's leave it for now.
lib/resources/nginx_conf.rb
Outdated
| end | ||
|
|
||
| def resolve_references(data, rel_path) | ||
| return data.map { |x| resolve_references(x, rel_path) } if data.is_a?(Array) |
There was a problem hiding this comment.
I've stared at this for a while, but I'm seriously still confused as to what you're trying to accomplish in this method.
- When will the data passed in be an Array instead of a Hash?
- In line 54, you return
dataunless it's a Hash, but then the rest of this method does work to make it a hash...
This could really use some comments and examples as to what you're trying to accomplish. I can see this method as a major area of frustration when troubleshooting.
There was a problem hiding this comment.
It has to iterate through all the parsed data, which includes Arrays, Hashes, and other forms of data. You can see a lot of Arrays in the parser due to the way nginx is internally structured (aggregated calls). If you look at the tests, you can see that http as the outermost entry in the hash already features an array to more data. It needs to be cycled through recursively.
There was a problem hiding this comment.
I'll add more comments to make this clear, thank you!
lib/resources/nginx_conf.rb
Outdated
| data = NginxConfig.parse(content) | ||
| resolve_references(data, File.dirname(path)) | ||
| rescue StandardError => e | ||
| raise "Cannot parse NginX config in #{path}: #{e}" |
There was a problem hiding this comment.
I thought we weren't going to raise inside resources until we better handled them? Otherwise, this will unwind the entire InSpec run...
| # verify multiline | ||
| _(resource.params['http'][0]['log_format']).must_equal [['main', 'multi', 'line']] | ||
| end | ||
| end |
There was a problem hiding this comment.
I'd really like to see tests added for #resolve_references - there's enough conditional/branch logic in that private method to warrant it.
There was a problem hiding this comment.
It'd be great to cover a few more use-cases down the line. The current e2e tests verify flat includes (i.e. non-glob) as they are used realistically. It covers every single line in this method.
arlimus
force-pushed
the
dr/nginx_conf
branch
3 times, most recently
from
b5b520b
to
f49ded3
Compare
| @@ -22,7 +22,7 @@ class NginxParser < Parslet::Parser | |||
| } | |||
|
|
|||
| rule(:identifier) { | |||
| (match('[a-zA-Z]') >> match('[a-zA-Z0-9_]').repeat).as(:identifier) >> space >> space.repeat | |||
| (match('[a-zA-Z]') >> match('\S').repeat).as(:identifier) >> space >> space.repeat | |||
There was a problem hiding this comment.
Tiny explanation: having / in these lines messed up the parser. I tracked down what identifiers potentially look like and it's really anything but a space. If we get more details we can specify it closer.
| 'Linux and UNIX platforms.' | ||
| example " | ||
| describe nginx_conf.params ... | ||
| describe nginx_conf('/path/to/my/nginx.conf').params ... |
There was a problem hiding this comment.
Short on examples for now until we reach to point of a better interface.
|
@arlimus You've got a lint error holding up Travis and AppVeyor as well :) |
|
The issue with windows is the hardcoded |
arlimus
force-pushed
the
dr/nginx_conf
branch
2 times, most recently
from
150f9ba
to
62d074f
Compare
The resource itself only offers contents and params right now. It resolved all include calls it can find and creates the aggregated config object. This is limited in functionality. One last (set of) PR(s) is needed to add an interface that makes querying this config file easier. It is due to the file's inherent complexity that I want to explore which methods are needed to be effective. In the meantime, this resource offers accessors to the underlying data that are stable. Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
|
can we get the resources page updated to include this? |
|
I'll open a new issue to ask @arlimus to write up some docs for this. |
The resource itself only offers contents and params right now. It resolved
all include calls it can find and creates the aggregated config object.
This is limited in functionality. One last (set of) PR(s) is needed to
add an interface that makes querying this config file easier. It is due
to the file's inherent complexity that I want to explore which methods
are needed to be effective. In the meantime, this resource offers accessors
to the underlying data that are stable.