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

Allow apache_conf to include symlinked configuration files #1406

Merged
merged 4 commits into from
Apr 3, 2017
Merged

Allow apache_conf to include symlinked configuration files #1406

merged 4 commits into from
Apr 3, 2017

Conversation

carldjohnston
Copy link
Contributor

Fix for #1401

Signed-off-by: Carl Johnston carldjohnston@gmail.com

Signed-off-by: Carl Johnston <carldjohnston@gmail.com>
@chris-rock
Copy link
Contributor

@carldjohnston Great work. I would like to see an integration test for this PR to verify everything works as expected.

Carl Johnston and others added 2 commits February 24, 2017 16:07
Same test added to both Ubuntu and Centos for consistency.

Signed-off-by: Carl Johnston <carldjohnston@gmail.com>
@carldjohnston
Copy link
Contributor Author

Hi @chris-rock - Hoping these tests are what you were after.

I've basically mocked the "find ..-type l" commands that the original commit generates and pointed it at "security.conf"; then we're looking for "ServerSignature Off" within that file.

I had to poke through and learn how it's all put together - thanks for teaching me something new :)

@adamleff
Copy link
Contributor

These are great additional unit tests, @carldjohnston - thank you! However, @chris-rock was asking for some integration tests where we actually run InSpec on a real filesystem/configs to check to see that the resource functions correctly.

Here's a suggestion on how you might do that:

Hopefully that makes sense - if not, let me know!

@carldjohnston
Copy link
Contributor Author

Thanks @adamleff, that makes perfect sense.

I'm working through this and have a working config, just the inspec side isn't working as expected.

By default (without parameters) using the apache_conf resource defaults to looking at /etc/httpd/conf/httpd.conf, however the os_prepare cookbook makes use of the httpd cookbook, which is putting configuration in /etc/httpd-default/conf/httpd.conf, and that's not being tested correctly as it's not including files correctly.. See Issue #1394.

I can rework this to use the apache2 cookbook, which replaces configuration in the default location and would make it easier to test for linked files, or we could work through the above issue - what do you think?

@adamleff
Copy link
Contributor

adamleff commented Mar 2, 2017

@carldjohnston so sorry for the delay in my response!

I think there might be a middle-ground we can pursue until a fix for #1394 is committed. In the os_prepare cookbook, in the apache recipe, we can just use a combination of Chef directory and either template or file resource to write out a file in the default location /etc/httpd/conf/httpd.conf for purposes of this test. We don't have to use the httpd helper cookbook to do so. I think that would be enough to lay down the test fixtures we need to properly test this.

Does that make sense? What do you think of that approach?

@carldjohnston
Copy link
Contributor Author

Hi @adamleff, I'm happy to go with that approach, probably makes understanding the os_prepare cookbook a little easier too; I'll put it together in the next couple of days.

Remove httpd helper cookbook and write out simple configuration by hand.

Signed-off-by: Carl Johnston <carldjohnston@gmail.com>
@carldjohnston
Copy link
Contributor Author

Hi @adamleff, I've added those modifications, let me know if you need anything else.

Thanks!

@adamleff
Copy link
Contributor

Haven't forgotten about you @carldjohnston! This is on my list for tomorrow if not later today!

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Superb work, @carldjohnston. Thanks for being agreeable to enhance the tests, as well!

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

@carldjohnston Thank you for this improvement

@adamleff
Copy link
Contributor

adamleff commented Apr 3, 2017

Merging! Thanks again, @carldjohnston!

@adamleff adamleff merged commit 68a930f into inspec:master Apr 3, 2017
@adamleff adamleff added the Type: Enhancement Improves an existing feature label Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants