Adding nginx_ssl_verify as a config option for nginx

[JohnLZeller]

Adding an 'nginx_ssl_verify' option to the nginx yaml config. This allows for setting the verify option in requests.get() when calling the url set by 'nginx_status_url'. The nginx_ssl_verify option defaults to True, staying consistent with requests.get(), which also defaults verify to True.

Working on a simple integration test now.

[JohnLZeller]

I cleaned up the nginx tests with some comments, and separated 2 different tests for clarity. Added 'nginx_ssl_verify' to the self.config for the test. This doesn't actually test functionality with the verify option for requests.get(). To do that will require an https endpoint on localhost to run a request against, however this is testing the requests submodule, and not our code; therefore, I think an ssl_verify check here is unnecessary. The important part is that our config gets picked up, and defaults correctly, which this test, and the yaml config checks already do.

[remh]

@elafarge can you review please ?

[miketheman]

None of these tests actually are made against an https:// url, nor do they flip the flag from True to False.
Is that expected?

[JohnLZeller]

Yeah, actually I referenced that in my comment above in this issue.

I basically confirm that this doesn't actually test functionality with the verify option for requests.get(), and that to do that would require an https endpoint on localhost to run a request against. However this is testing the requests submodule, and not our code, so I think an ssl_verify check here is unnecessary. The important part is that our config gets picked up.

But, I'm all for total coverage, so if you think adding this explicitly to the test is wise, then that's no problem. Is there an existing https endpoint on localhost that the test suite can access?

[elafarge]

Not that I know of.

The nginx config we use in our CI test suite is located here : https://github.com/DataDog/dd-agent/blob/master/ci/resources/nginx/nginx.conf .

If you do want to test that (it doesn't seem very useful indeed... but almost no test does until it breaks :) ) you'll have to create an HTTPS VirtualHost in Nginx in the conf file above and enable status on it.

Oh and I don't think there's a need to enable nginx_ssl_verify for all the tests on http endpoints.

[JohnLZeller]

Yeah, I agree, and it's doubly useless in this test, since True is the default :)
My thought was that if it was at least added in there, it wouldn't be forgotten :P

[miketheman]

Cool stuff.
Should the toggle name nginx_ssl_verify be changed? It's already implicit that it's nginx due to being in nginx.yaml file, and I think the terminology should use validation vs verify.
Something like ssl_validation: True/False makes more sense to me.

[JohnLZeller]

Yeah, those changes are fine. I was going for consistency of naming conventions, and noticed that the status url was called nginx_status_url. But, I agree, nginx is a bit redundant :)

I'll go with chaning to ssl_validation

[JohnLZeller]

Okay, what do we think about the updated tests now?
@miketheman @elafarge

[elafarge]

Wouldn't it be possible to use assertRaises to make it a little cleaner (to avoid having a pass and using e that isn't really supposed to be defined in this block ?

[JohnLZeller]

Bah! Yeah, you're totally right, that'd be a lot cleaner.

[elafarge]

Awesome job ! Congrats! Apart from the small notice I wrote concerning assertRaises everything looks great.

[remh]

@JohnLZeller tests are failing on

======================================================================
ERROR: test_nginx_ssl_validation_enabled (tests.checks.integration.test_nginx.TestNginx)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/DataDog/dd-agent/tests/checks/integration/test_nginx.py", line 82, in test_nginx_ssl_validation_enabled
    with self.assertRaises(requests.exceptions.SSLError):
TypeError: failUnlessRaises() takes at least 3 arguments (2 given)

[JohnLZeller]

Ah, yeah. Turns out that the unittest submodule added the ability to use assertRaises() as a context manager only in 2.7, not 2.6. So I reverted the use back to a state that should work for both.

[elafarge]

My bad, I didn't realize that when I suggested to use assertRaises :(

[JohnLZeller]

It's okay :) Still using it, just not as a context manager.

[JohnLZeller]

@remh should be g2g now

[remh]

Nice, can you squash your commits into one please ?

[JohnLZeller]

Squashed 👍

[remh]

👍

[miketheman]

💃 👯 💃 🍰 🎉 👯