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

test fixes for openshift_certificates_expiry #3449

Merged
merged 1 commit into from Mar 2, 2017

Conversation

Projects
None yet
5 participants
@detiber
Copy link
Contributor

commented Feb 22, 2017

  • create pytest fixtures for building certs at runtime

  • update tests to use the fixtures

  • update FakeOpenSSL to encode fields as bytes, where they were str
    previously to be more compatible with pyOpenSSL.

  • other test tweaks:

    • exclude conftest.py and tests from coverage report
    • reduce the fail_under to 25%, since the tests being included were
      inflating our coverage
@detiber

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

@rhcarvalho
Copy link
Contributor

left a comment

I like the improvements. and I love to have less lines of code ;)

There is some care to be taken with regards to encoding, since you started touching that area :)

.coveragerc Outdated
test_*.py
*_tests.py
conftest.py
*/conftest.py

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

I suspect conftest.py and */conftest.py, in this particular case, matches the same things. That's why we didn't have the */ prefix before. I might be wrong, but that was what I observed.

This comment has been minimized.

Copy link
@detiber

detiber Feb 22, 2017

Author Contributor

For some reason on my system it wasn't matching and I had to explicitly add */ to get the files in subdirectories to match.

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 28, 2017

Contributor

Indeed the current pattern is not omitting what it should, thanks for noticing it!

I see that coverage is using fnmatch under the hoods, these should cover all cases, tested:

    */test_*.py
    */*_tests.py
    */conftest.py

These patterns get translated to regexps, e.g.:

In [13]: fnmatch.translate('*/*_tests.py')
Out[13]: '.*\\/.*_tests\\.py\\Z(?ms)'

And coverage is testing against the full path for paths that start with a wildcard, or relative to the current dir for others.
https://coverage.readthedocs.io/en/latest/source.html#execution

Both */conftest.py and conftest.py match conftest.py in the repo root.

# It's just the way we do things in Ansible. So disable this warning
#
# pylint: disable=wrong-import-position,import-error
from ansible.module_utils.basic import AnsibleModule # noqa: E402

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

👍 -- it WAS the way to it, not anymore :)

@@ -11,3 +11,4 @@ coverage
mock
pytest
pytest-cov
sh

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

The usage is so light that I don't feel it justifies adding another dependency... this thing will be installed over and over, when we would probably be fine with using:

sh = subprocess.check_output

sh("foo", "arg1", "arg2", ...)

Though I'd use subprocess.check_output directly, not calling it "sh".

The leftpad incident last year should be a good reminder to weight in the pluses and minuses before adding in dependencies ;)

if __name__ == "__main__":
unittest.main()
assert server_san == f_server_san
assert f_server_san is not None

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

Perhaps it would make sense to test the None sooner?

server_san = None
for ...:
    ...

assert server_san is not None


f_server_san = None
for ...:
    ...

assert f_server_san == server_san
import pytest
import sh
import sys

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

I tend to separate groups of imports with a blank line: first stdlib, then 3rd party, and finally modules or packages internal to the project.

https://www.python.org/dev/peps/pep-0008/#imports

In this case, pytest (and sh) would go into a separate group.

This comment has been minimized.

Copy link
@detiber
f.write(
crypto.dump_certificate(crypto.FILETYPE_PEM, client_cert)
)
return str(crt_file)

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

Same notes as for server_cert_file below.


@pytest.fixture(scope='session')
def ca_dir(tmpdir_factory):
return tmpdir_factory.mktemp('ca')

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

👍

I thought there was a builtin fixture to do just that... but I think I was dreaming, the docs have something like this as an example only.

This comment has been minimized.

Copy link
@detiber

detiber Feb 22, 2017

Author Contributor

Not at the session scope, and with the costly openssl operations it seemed better to only create them once per session.

@@ -129,7 +131,7 @@ def _parse_cert(self):
not_after_raw = l.partition(' : ')[-1]
# Last item: ('Not After', ' : ', 'Feb 7 18:19:35 2019 GMT')
not_after_parsed = datetime.datetime.strptime(not_after_raw, '%b %d %H:%M:%S %Y %Z')
self.not_after = not_after_parsed.strftime('%Y%m%d%H%M%SZ')
self.not_after = not_after_parsed.strftime('%Y%m%d%H%M%SZ').encode()

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

What is intended here?

In both Py2/Py3 datetime.strftime returns a str, though those types are not the same in the two language versions.
Calling .encode will "reencode" in Py2 and return a str, while in Py3 it will return a b"byte string".

Since the default encodings are different, I'd suggest we use an explicit encoding here.


I didn't stop to understand the big picture, but it is strange that we're storing a string representation of the time... would be more natural to parse the file and store the datetime.datetime object, and then serialize when writing.

This comment has been minimized.

Copy link
@detiber

detiber Feb 22, 2017

Author Contributor

I definitely think this needs more scrutiny on the encoding/decoding aspects.

The main reason that I ended up down this path is because of using pyOpenSSL for the tests, where parameters and return values are bytes, which caused comparison failures between the fake class and the actual class.

Since we are trying to mimic the behavior of pyOpenSSL we need to copy the interface they have, so I'm not sure handling encode/decode on serialization is the answer, since we'd still have the potential for different behavior in py3 depending on using openssl or the fake class.

@@ -200,7 +206,7 @@ def __init__(self, subject_string):
self.subjects = []
for s in subject_string.split(', '):
name, _, value = s.partition('=')
self.subjects.append((name, value))
self.subjects.append((name.encode(), value.encode()))

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

Same story with implicit x explicit encoding.

A general advice is to decode strings when reading, using unicode strings (Py2) and strings (Py3) everywhere internally, and encode only once writing.

https://docs.python.org/2/howto/unicode.html#tips-for-writing-unicode-aware-programs
https://docs.python.org/3/howto/unicode.html#tips-for-writing-unicode-aware-programs

This comment has been minimized.

Copy link
@detiber

detiber Feb 22, 2017

Author Contributor

I would agree with that in general, but that is not what pyopenssl does. Instead of pyopenssl, we could use cryptography instead, which is already a dependency for pyopenssl. I don't think it explicitly uses bytes internally.

This comment has been minimized.

Copy link
@detiber

detiber Feb 22, 2017

Author Contributor

It looks like cryptography suffers from the same bytes problem and is also not available on atomic host.

@@ -335,7 +341,8 @@ def load_and_handle_cert(cert_string, now, base64decode=False, ans_module=None):
# string with the alt names separated by a comma and a
# space. Split the string by ', ' and then add our new names
# to the list of existing names
cert_subjects.extend(str(san).split(', '))
san_bytes = [s.encode() for s in str(san).split(', ')]

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 22, 2017

Contributor

One more occurrence of encode() without encoding, and str.

@tbielawa

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

I just wanna throw this out there, I'm a little uncomfortable with this PR right now. There are some changes I don't understand. I think with some discussion I might feel better about this. I'll try to elaborate.

The original tests I created were so I could verify that the FaceOpenSSLCertificate* objects implemented the actual methods used in the code. In this way FakeOpenSSLCertificate is a (limited) drop-in replacement on systems without PyOpenSSL. The tests drove development of the classes. The tests and the classes only implement methods actually present in the actual object you get back from the proper OpenSSL.crypto.load_certificate function.

I didn't realize before that OpenSSL.crypto.X509 objects had a get_extension_count method before. So that's nice to see added. Not necessarily needed since it isn't actually part of the process in load_and_handle_cert.

The new unit tests are completely obfuscated to me.

Please add comments to the unit tests. Joins and list comprehensions are nice for making code short, but they aren't easy to read:

def test_get_subject(client_cert, fake_client_cert, server_cert, fake_server_cert):
    c_subjects = client_cert.get_subject().get_components()
    c_subj = ', '.join(['{}:{}'.format(x, y) for x, y in c_subjects])
    f_subjects = fake_client_cert.get_subject().get_components()
    f_subj = ', '.join(['{}:{}'.format(x, y) for x, y in f_subjects])

The former had docstrings with them that described what each test did. Please add those one-liners back. They're useful for describing what a test does and useful for reading test runner output as most test runners use the first line of the docstring to give a pretty report rather than the simple function name.

test_attributes is a nice function. It tests multiple things at once. IMHO each test should test one thing at a time really. I.e., test_get_serial_number, test_get_notAfter. I like that test_get_subject and test_subject_alt_names are their own functions.

As far as all the encoding stuff is concerned:

I have no idea what's going on with that. Did you have to make those changes in the module to make the new tests work? Is using .encode() the same as using the six.u function? I had to use that to write out to the temporary file.

@tbielawa

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

Another comment, I do think it's pretty cool that you were able to switch from using actual cert files to constructing them using the OpenSSL library. That's a nice change that I totally approve of.

As @ashcrow pointed out before, you should look at using the io library for doing those with open(....) as f: lines for better py2/3 compat. You might run into the same problem I ran into originally where we were feeding a string into the file pointer when writing the file out. That's where the six.u usage came into play.

@detiber

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

I just wanna throw this out there, I'm a little uncomfortable with this PR right now. There are some changes I don't understand. I think with some discussion I might feel better about this. I'll try to elaborate.

👍 I'm not comfortable with the current state.

The original tests I created were so I could verify that the FaceOpenSSLCertificate* objects implemented the actual methods used in the code. In this way FakeOpenSSLCertificate is a (limited) drop-in replacement on systems without PyOpenSSL. The tests drove development of the classes. The tests and the classes only implement methods actually present in the actual object you get back from the proper OpenSSL.crypto.load_certificate function.

Indeed, the test changes did highlight an issue that I think will cause us problems if we don't work it out, though.

pyOpenSSL uses bytes internally to represent data. For python2, the comparisons between the fake class and pyOpenSSL worked without issue. For python3, all of the comparisons failed, because the pyOpenSSL values were all bytes and the fake class were all strings. I am concerned that if we do not implement the fake class in a way that handles the encoding/decoding of the values in a compatible way that we'll end up with hard to track down bugs.

I didn't realize before that OpenSSL.crypto.X509 objects had a get_extension_count method before. So that's nice to see added. Not necessarily needed since it isn't actually part of the process in load_and_handle_cert.

Considering the implementation, seems like it would make sense to replace the current way of iterating the extensions.

The new unit tests are completely obfuscated to me.

Please add comments to the unit tests. Joins and list comprehensions are nice for making code short, but they aren't easy to read:

def test_get_subject(client_cert, fake_client_cert, server_cert, fake_server_cert):
c_subjects = client_cert.get_subject().get_components()
c_subj = ', '.join(['{}:{}'.format(x, y) for x, y in c_subjects])
f_subjects = fake_client_cert.get_subject().get_components()
f_subj = ', '.join(['{}:{}'.format(x, y) for x, y in f_subjects])
The former had docstrings with them that described what each test did. Please add those one-liners back. They're useful for describing what a test does and useful for reading test runner output as most test runners use the first line of the docstring to give a pretty report rather than the simple function name.

+1, wanted to get discussion on things before I was able to clean everything up, considering the encoding issue I found.

test_attributes is a nice function. It tests multiple things at once. IMHO each test should test one thing at a time really. I.e., test_get_serial_number, test_get_notAfter. I like that test_get_subject and test_subject_alt_names are their own functions.

I think if it is just a matter of getting/comparing attributes in the same way then it makes sense to use a parameterized test to handle those, I think the test output makes it clear what is going on as well:

14:08:03 [jdetiber:~/git/github/openshift/openshift-ansible] [py35-ansible22-unit] hacky_cert_parsing(17)+* 4s 1 ± pytest --collect-only roles/openshift_certificate_expiry/test/test_fakeopensslclasses.py
==================================================== test session starts =====================================================
platform linux -- Python 3.5.2, pytest-3.0.6, py-1.4.32, pluggy-0.4.0
rootdir: /home/jdetiber/git/github/openshift/openshift-ansible, inifile: setup.cfg
plugins: cov-2.4.0
collected 4 items 
<Module 'roles/openshift_certificate_expiry/test/test_fakeopensslclasses.py'>
  <Function 'test_attributes[get_serial_number]'>
  <Function 'test_attributes[get_notAfter]'>
  <Function 'test_get_subject'>
  <Function 'test_subject_alt_names'>


----------- coverage: platform linux, python 3.5.2-final-0 -----------
Name                                                                  Stmts   Miss Branch BrPart  Cover
-------------------------------------------------------------------------------------------------------
roles/openshift_certificate_expiry/library/openshift_cert_expiry.py     284    245     78      1    11%
Coverage HTML written to dir cover

That said, I do think there are improvements to be had with making the clarity of what is happening better.

As far as all the encoding stuff is concerned:

I have no idea what's going on with that. Did you have to make those changes in the module to make the new tests work? Is using .encode() the same as using the six.u function? I had to use that to write out to the temporary file.

Yes and no. There is some type impedence happening between pyopenssl and the fake class when using python3.

@pytest.fixture(scope='session')
def client_cert_file(client_cert, ca_dir):
crt_file = ca_dir.join('client.crt')
with open(str(crt_file), 'wb') as f:

This comment has been minimized.

Copy link
@ashcrow

ashcrow Feb 22, 2017

Member

nit: io.open if possible.

@detiber detiber force-pushed the detiber:hacky_cert_parsing branch 2 times, most recently from dc0b74b to d8fd0ee Feb 28, 2017

@detiber

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

@detiber detiber force-pushed the detiber:hacky_cert_parsing branch from d8fd0ee to 90e0825 Feb 28, 2017

@rhcarvalho
Copy link
Contributor

left a comment

3 improvement suggestions, though the current form LGTM

.coveragerc Outdated
test_*.py
*_tests.py
conftest.py
*/conftest.py

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 28, 2017

Contributor

Indeed the current pattern is not omitting what it should, thanks for noticing it!

I see that coverage is using fnmatch under the hoods, these should cover all cases, tested:

    */test_*.py
    */*_tests.py
    */conftest.py

These patterns get translated to regexps, e.g.:

In [13]: fnmatch.translate('*/*_tests.py')
Out[13]: '.*\\/.*_tests\\.py\\Z(?ms)'

And coverage is testing against the full path for paths that start with a wildcard, or relative to the current dir for others.
https://coverage.readthedocs.io/en/latest/source.html#execution

Both */conftest.py and conftest.py match conftest.py in the repo root.


[report]
fail_under = 28
fail_under = 26

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 28, 2017

Contributor

👍 this is also what I observe here when the test files are removed from the report


@pytest.fixture(scope='session',
ids=['client', 'server', 'combined'],
params=[

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 28, 2017

Contributor

Maybe move this list to new variable?

I think it could improve readability. When I was looking at def valid_cert... it was not visible that the brackets and parens above were part of the args to the decorator (too many lines, didn't fit the screen).

# pylint: disable=import-error,wrong-import-position
# place class in our python path
module_path = os.path.join('/'.join(os.path.realpath(__file__).split(os.path.sep)[:-1]), 'library')
module_path = os.path.join('/'.join(os.path.realpath(__file__).split('/')[:-2]), 'library')

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 28, 2017

Contributor

I doesn't make sense to use os.path.join and '/'.join together ;-)

Suggestion:

sys.path.insert(1, os.path.realpath(os.path.join(__file__, os.pardir, os.pardir, 'library')))

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

As @ashcrow would point out here, os.path.sep is the preferred way to split/join (as demonstrated later in that line)

@rhcarvalho

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

Would be good to hear from @tbielawa before merging, I think he's more familiar with the specifics of openshift_certificates_expiry.

@tbielawa

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Would be good to hear from @tbielawa before merging, I think he's more familiar with the specifics of openshift_certificates_expiry.

I'l checking this out again in more detail as soon as I make some ☕️

brb

@ashcrow

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

@tbielawa ☕️ 🍪 ☕️ ☕️ 🍰

@@ -1,42 +0,0 @@
-----BEGIN CERTIFICATE-----

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

👍 for removing old certificate files

@@ -1,82 +0,0 @@
Certificate:

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

👍 for removing old certificate files

@@ -1,19 +0,0 @@
-----BEGIN CERTIFICATE-----

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

👍 for removing old certificate files

@@ -1,75 +0,0 @@
Certificate:

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

👍 for removing old certificate files


# File pointers from io.open require unicode inputs when using their
# `write` method
import six
from six.moves import configparser

import yaml
from ansible.module_utils.basic import AnsibleModule

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

This line and above, 👍 for fixing up lints and import orders finally

else:
# Try reading the next extension
i += 1
# is that pyOpenSSL does not give extensions as an iterable

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

👍 for figuring out all of the encoding stuff and localizing it all to the test classes.

'ip': ['10.0.0.2', '192.168.0.2']
}
])
def valid_cert(request, ca):

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

I absolutely love that you were able to translate the actual cert files I was using originally into objects just by using the library. Very slick and very cool. Most impressive, really 😀

'''
Test class for FakeOpenSSL classes
'''
from openshift_cert_expiry import FakeOpenSSLCertificate # noqa: E402

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

All these tests are MUCH MORE readable since the first draft we reviewed. 👍

# pylint: disable=import-error,wrong-import-position
# place class in our python path
module_path = os.path.join('/'.join(os.path.realpath(__file__).split(os.path.sep)[:-1]), 'library')
module_path = os.path.join('/'.join(os.path.realpath(__file__).split('/')[:-2]), 'library')

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

As @ashcrow would point out here, os.path.sep is the preferred way to split/join (as demonstrated later in that line)

@tbielawa

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

I have an additional note I'd like to discuss. Note this section here in the module.

try:
    # You can comment this import out and include a 'pass' in this
    # block if you're manually testing this module on a NON-ATOMIC
    # HOST (or any host that just doesn't have PyOpenSSL
    # available). That will force the `load_and_handle_cert` function
    # to use the Fake OpenSSL classes.
    import OpenSSL.crypto
except ImportError:
    # Some platforms (such as RHEL Atomic) may not have the Python
    # OpenSSL library installed. In this case we will use a manual
    # work-around to parse each certificate.
    #
    # Check for 'OpenSSL.crypto' in `sys.modules` later.
    pass

We are not currently testing for cases where OpenSSL.crypto is NOT available. I feel like to accomplish that we will need to use mock and introduce a side effect that raises an ImportError when OpenSSL.crypto is imported to be able to verify those code branches are tested too. You can more easily see this in the coverage report:

I tried to include a picture of the report in here but github file uploads are failing

I'm not sure how to make that happen in these tests the way it is now. I would defer to @ashcrow (the master of all testing) for advice on that. It may simply require another test_foo.py file with some mock magic inside.

@tbielawa

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

...continued...

Specifically I am referring to these branches of the load_and_handle function:

https://github.com/openshift/openshift-ansible/blob/master/roles/openshift_certificate_expiry/library/openshift_cert_expiry.py#L271-L294

Also I wrote another test I'd like included: https://gist.github.com/tbielawa/9ba0ab93223d6d9cf9a96c9d42ce11ce

@ashcrow

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

I feel like trying to mock import would end up being more trouble than it's worth. There are a couple other ideas that come to mind. Loop the tests and:

  • del sys.modules['OpenSSL.crypto'], reload(openshift_cert_expiry) and run the same tests again without the module.
  • Use a HAS_OPENSSL variable in openshift_cert_expiry.py. Use that instead of looking in sys.modules. Manipulate that variable in your tests to test both with and without.
@tbielawa

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

@ashcrow said:

I feel like trying to mock import would end up being more trouble than it's worth. There are a couple other ideas that come to mind. Loop the tests and:

  • del sys.modules['OpenSSL.crypto'], reload(openshift_cert_expiry) and run the same tests again without the module.
  • Use a HAS_OPENSSL variable in openshift_cert_expiry.py. Use that instead of looking in sys.modules. Manipulate that variable in your tests to test both with and without.

Those ideas are way smarter and simpler than what I was thinking of. Let's do that instead. The HAS_OPENSSL variable idea would be my favored approach.

@detiber detiber changed the title test fixes for openshift_certificates_expiry [wip] test fixes for openshift_certificates_expiry Feb 28, 2017

@tbielawa
Copy link
Member

left a comment

This commit has improved this PR significantly.

@@ -1,6 +1,38 @@
import pytest
from OpenSSL import crypto

# Parameter list for valid_cert fixture
VALID_CERTIFICATE_PARAMS = [

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

+1 GOOD JOB

@@ -0,0 +1,68 @@
'''
Unit tests for the FakeOpenSSL classes

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member
>>> this = pasta.copy()

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

@detiber I'm sure you already noticed, but this is the only thing I spotted in this commit that needs changing.

import os
import subprocess
import sys

import pytest

module_path = os.path.join('/'.join(os.path.realpath(__file__).split('/')[:-2]), 'library')
module_path = os.path.normpath(os.path.join(__file__, os.pardir, os.pardir, 'library'))

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

I LIKE WHAT YOU GOT

@@ -512,7 +505,7 @@ def main():
######################################################################
for os_cert in filter_paths(openshift_cert_check_paths):
# Open up that config file and locate the cert and CA
with open(os_cert, 'r') as fp:
with io.open(os_cert, 'r', encoding='utf-8') as fp:

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

wrt: encoding. You've been focusing the encoding stuff in this PR a lot so I trust your and the automated testing systems judgements here.

This comment has been minimized.

Copy link
@detiber

detiber Feb 28, 2017

Author Contributor

I would like to get @ashcrow and @rhcarvalho to do a review of the encoding/decoding.

This comment has been minimized.

Copy link
@detiber

detiber Feb 28, 2017

Author Contributor

@tbielawa I'd be more comfortable if we had coverage for testing values derived from files using io.open, and the certificates read using subprocess. I think there are still some encoding demons to work out.

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Feb 28, 2017

Contributor

@tbielawa had ☕️ with super powers today :-)

I'll have another look at this PR tomorrow morning, gotta 😴 now...

@@ -274,30 +274,23 @@ def load_and_handle_cert(cert_string, now, base64decode=False, ans_module=None):
# around a missing library on the target host.
#
# pylint: disable=redefined-variable-type
if 'OpenSSL.crypto' in sys.modules:
if HAS_OPENSSL:

This comment has been minimized.

Copy link
@tbielawa

tbielawa Feb 28, 2017

Member

👍 yeah. That will make mocking out tests wayyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy easier for the non-RHEL platforms.

@rhcarvalho
Copy link
Contributor

left a comment

Left some notes and questions. As already noted this does feel like a good step forward improving the role.

I did inquire some points about the encoding/decoding but a more detailed analysis would require extra time to dive into the implementation, so I'll be trusting you @detiber ;)

# is that pyOpenSSL does not give extensions as an iterable
san = None
for i in range(cert_loaded.get_extension_count()):
# Read the extension at index 'i'

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

Not a very helpful comment to keep? I'd get rid of obvious comments if we have a chance.

@@ -613,7 +591,7 @@ def main():
pass

for kube in filter_paths(kubeconfig_paths):
with open(kube, 'r') as fp:
with io.open(kube, 'r', encoding='utf-8') as fp:
# TODO: Maybe consider catching exceptions here?
cfg = yaml.load(fp)

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

I see the pattern

with io.open(path, 'r', encoding='utf-8') as f:
    cfg = yaml.load(f)
    ...

appears here at least 3 times, maybe we could extract this into a function to load YAML configs from disk?

This comment has been minimized.

Copy link
@detiber

detiber Mar 1, 2017

Author Contributor

I completely agree, I'm not sure I want to tackle refactoring any more of the existing implementation at this point, though.

f_san = None
for i in range(fake_valid_cert.get_extension_count()):
ext = fake_valid_cert.get_extension(i)
if ext.get_short_name() == 'subjectAltName':

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

1. Subtle differences

Why do we compare to b'subjectAltName' above and 'subjectAltName' here? I see the comment, but seems to be too subtle a different to be understood.

2. Code duplication

These two loops look so similar that at first sight I thought they were an accidental double-paste.
Perhaps worth a refactor?

def get_san(cert):
    for i in range(cert.get_extension_count()):
        ext = real_cert.get_extension(i)
        if ext.get_short_name() == b'subjectAltName':
            return ext

san = get_san(real_cert)
f_san = get_san(fake_valid_cert)

3. str?

Why are we calling str(ext)?

4. Terminate the loop early

Without the refactor on item (2) above, these loops miss a break statement to stop on the first match. I don't know if it is possible to have two entries with the same short name, but theoretically these loops are returning the last match instead of the first.

self.assertEqual('CN=172.30.0.1', ', '.join(subjects))
# If there are either dns or ip sans defined, verify common_name present
if valid_cert['ip'] or valid_cert['dns']:
assert 'DNS:{}'.format(valid_cert['common_name']) in f_san

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

In this particular case where the format string is just a prefix, wouldn't it be better to write it like below?

-assert 'DNS:{}'.format(valid_cert['common_name']) in f_san
+assert 'DNS:' + valid_cert['common_name'] in f_san
import pytest

module_path = os.path.normpath(os.path.join(__file__, os.pardir, os.pardir, 'library'))
sys.path.insert(0, module_path)

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

import pytest

module_path = os.path.normpath(os.path.join(__file__, os.pardir, os.pardir, 'library'))

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

Why did you choose normpath instead of realpath that we use elsewhere?


Side note, if we run pylint through this file, it will complain about the invalid name module_path, about the import order and import erorr :-) #3511

(subject,
expiry_date,
time_remaining,
serial) = openshift_cert_expiry.load_and_handle_cert(cert_string, now)

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

Wow, sorry but this looks ugly! I don't remember seeing an assignment in Python in this shape (not the multiple assignment, but the formatting). I think in fighting against a long line it became worse to read.

subject, expiry_date, time_remaining, serial = openshift_cert_expiry.load_and_handle_cert(cert_string, now)

The above is 108 cols long, still below our current 120 col limit.

This comment has been minimized.

Copy link
@ashcrow

ashcrow Mar 1, 2017

Member

I'd rather see it as one line or, if a break is needed, as:

(subject, expiry_date, time_remaining, serial) = openshift_cert_expiry.load_and_handle_cert(
    cert_string, now)

This comment has been minimized.

Copy link
@tbielawa

tbielawa Mar 1, 2017

Member

If you get to changing that @detiber you should know that there are more instances of that pattern lower in the module file. Actually, there's a lot of patterns in the module. They need to get modularized, but I think that's out of scope right now.

This comment has been minimized.

Copy link
@detiber

detiber Mar 1, 2017

Author Contributor

I agree with @tbielawa here. I'd like to see it cleaned up as well, but I don't want to rabbit hole on refactorings.

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 2, 2017

Contributor

Agreed!

"""Params:
* `loaded_cert` comes from the `loaded_cert` fixture in this file
* `valid_cert` comes from the 'valid_cert' fixture in conftest.py

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

Bad/missing indentation here and below in test_expiry.

try:
openssl_decoded = subprocess.Popen(cmd.split(),
stdout=subprocess.PIPE)
openssl_proc = subprocess.Popen(cmd.split(),

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 1, 2017

Contributor

Why do we use the general subprocess.Popen, instead of the recommended convenience functions call / check_call / check_output?
https://docs.python.org/2/library/subprocess.html#using-the-subprocess-module

This comment has been minimized.

Copy link
@detiber

detiber Mar 1, 2017

Author Contributor

@rhcarvalho mainly because it was already using Popen and it was easy to use PIPE and communicate, ~~~I'll update to use StringIO instead.~~~

I had issues when trying to use communicate with check_output, I'll see if I can work those out.

This comment has been minimized.

Copy link
@detiber

detiber Mar 1, 2017

Author Contributor

After some digging, call, check_call, and check_output all use communicate internally, so the alternative to Popen and communicate would involve pre-creating a PIPE and writing to that PIPE while check_output is blocking on a result.

I thought that it might be possible to handle it through StringIO, but since it doesn't produce a fileno it cannot be used with subprocess.

Python 3.4 introduced the input argument to check_output, which would solve the issue, but not for Python 2.7 :(

This comment has been minimized.

Copy link
@rhcarvalho

rhcarvalho Mar 2, 2017

Contributor

Alright, I overlooked that we actually do use stdin to send data. I've seen other places where we were using Popen unnecessarily, not here. Thanks for the investigation and education!

@detiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2017

@detiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

aos-ci-test

@openshift-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 2, 2017

@tbielawa
Copy link
Member

left a comment

I am satisfied with this now. I have ideas for simple tests we can add in the future to raise the coverage for the module. But I don't want to delay this any further by trying to squeeze those in now. Let's shippit shippit shippit shippit shippit shippit shippit shippit shippit

@tbielawa tbielawa referenced this pull request Mar 2, 2017

Open

openshift_certificate_expiry test coverage is low #3545

0 of 5 tasks complete
@rhcarvalho

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

Squash commits and 🚢 it :-)

Jason DeTiberus
test fixes for openshift_certificates_expiry
- create pytest fixtures for building certs at runtime
- update tests to use the fixtures
- add tests for load_and_handle_cert
- fix py2/py3 encode/decode issues raised by tests
- add get_extension_count method to fakeOpenSSLCertificate
- avoid using a temp file for passing ssl certificate to openssl
  subprocess
- other test tweaks:
  - exclude conftest.py and tests from coverage report
  - reduce the fail_under to 26%, since the tests being included were
    inflating our coverage

@detiber detiber force-pushed the detiber:hacky_cert_parsing branch from e394c27 to 293f185 Mar 2, 2017

@detiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

aos-ci-test

@openshift-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 2, 2017

@detiber detiber merged commit 486f35b into openshift:master Mar 2, 2017

10 checks passed

aos-ci-jenkins/OS_3.4_NOT_containerized "openshift-ansible install passed"
Details
aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests "e2e tests passed"
Details
aos-ci-jenkins/OS_3.4_containerized "openshift-ansible install passed"
Details
aos-ci-jenkins/OS_3.4_containerized_e2e_tests "e2e tests passed"
Details
aos-ci-jenkins/OS_3.5_NOT_containerized "openshift-ansible install passed"
Details
aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests "e2e tests passed"
Details
aos-ci-jenkins/OS_3.5_containerized "openshift-ansible install passed"
Details
aos-ci-jenkins/OS_3.5_containerized_e2e_tests "e2e tests passed"
Details
aos-ci-jenkins/OS_unit_tests "all unit tests passed"
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@detiber detiber changed the title [wip] test fixes for openshift_certificates_expiry test fixes for openshift_certificates_expiry Mar 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.