cc_ca_certs.py: fix blank line problem when removing CAs and adding n… #483
Conversation
…ew one. Problem: When cc_ca_certs configuration has both "remove-defaults: true" and also specifies one, or more, new trusted CAs to add then the resultant /etc/ca-certificates.conf file's 1st line is blank. As noted in comments in the existing cc_ca_certs.py code blank lines in this file cause problems. Fix: Before adding the cloud-init CA filename to this file first check the size of the file - if is is empty (as all existing CAs have been deleted) then write only the cloud-init CA filename to the file rather than appending it to the file.
There was a problem hiding this comment.
Hi @dermotbradley, thanks for the submission, this is a good bug to fix! I have one inline question, but at a higher-level I'd also like to see a test (or tests) written for this change in behaviour, so we can (a) confirm this fix is behaving as expected now, and (b) detect any regressions to this behaviour in future.
There are already tests for this function in https://github.com/canonical/cloud-init/blob/master/tests/unittests/test_handler/test_handler_ca_certs.py#L139, so hopefully extending that should be relatively simple. If you have any question about how to proceed, please don't hesitate to ask!
…ng new one Existing testcases do not handle the situation where all the existing CA certs are deleted (so resulting in an empty /etc/ca-certificates.conf file and when one, or more, new CAs are added). This testcase verifies that the resultant ca-certificates.conf file does not contain any blank lines which are known to cause problems for the update-ca-certificates utility.
|
Added a new testcase but its failing - looks like I forgot about needing to mock the os.stat.st_size call. I'm not that familiar with mocking in Python so I'll try and figure it out. |
| mock_write = mocks.enter_context( | ||
| mock.patch.object(util, 'write_file')) |
There was a problem hiding this comment.
Per our unit testing guidelines (search for "An important exception"), if we're going to use the assert_* methods on a mock then it needs to be autospecced:
| mock_write = mocks.enter_context( | |
| mock.patch.object(util, 'write_file')) | |
| mock_write = mocks.enter_context( | |
| mock.patch.object(util, 'write_file', autospec=True)) |
There was a problem hiding this comment.
Thanks @dermotbradley, I look forward to your future contributions!
…ew one.
Problem: When cc_ca_certs configuration has both "remove-defaults: true"
and also specifies one, or more, new trusted CAs to add then the resultant
/etc/ca-certificates.conf file's 1st line is blank. As noted in comments
in the existing cc_ca_certs.py code blank lines in this file cause problems.
Fix: Before adding the cloud-init CA filename to this file first check the
size of the file - if is is empty (as all existing CAs have been deleted)
then write only the cloud-init CA filename to the file rather than appending
it to the file.