Enable integration tests for the crypto/ namespace #26684

Spredzy · 2017-07-12T09:00:04Z

SUMMARY

Crypto namespace contains the openssl modules. It has no integration
testing as of now.

This commits aims to add integration tests for the crypto namespace.
This will make it easier to spot breaking changes in the future.

This tests currently apply to:

openssl_privatekey
openssl_publickey
openssl_csr

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

crypto/
test/integration

ANSIBLE VERSION

ansible 2.4.0 (openssl_integration_test caf6ffb395) last updated 2017/07/12 10:58:24 (GMT +200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/spredzy/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/spredzy/Projects/github.com/Spredzy/ansible/lib/ansible
  executable location = /home/spredzy/Projects/github.com/Spredzy/ansible/bin/ansible
  python version = 2.7.13 (default, May 10 2017, 20:04:28) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]

ADDITIONAL INFORMATION

N/A

gundalow

Create a new file called
test/integration/targets/openssl/aliases

Which contains

posix/ci/group1
destructive

Then you can test by doing
ansible-test integration --docker=fedora25 -v openssl_privatekey openssl_publickey

gundalow · 2017-07-12T09:14:05Z

test/integration/openssl.yml

@@ -0,0 +1,6 @@
+---


Don't need this file, ansible-test will create what's needed on the fly

Hmm, I think I still need this file for other scenarios. (Tests not run via ansible-test).

Like:

vyos: https://github.com/ansible/ansible/blob/devel/test/integration/vyos.yaml

ovs: https://github.com/ansible/ansible/blob/devel/test/integration/ovs.yaml

@gundalow is correct. Since these tests are run by ansible-test, no separate playbook is required.

@mattclay I'd like to have this file there because I'd like to be able to test those modules outside the ansible-test scope.

The idea is to test those modules with different OpenSSL implementation:

OpenSSL

LibreSSL

BoringSSL

gundalow · 2017-07-12T09:14:47Z

test/integration/roles/test_openssl/tasks/main.yml

@@ -0,0 +1,19 @@
+- name: Generate a private key


Format is test/integration/targets/$module_name/ Move to targetsi.e. test/integration/targets/openssl/tasks/main.yml

gundalow · 2017-07-12T09:14:59Z

test/integration/roles/test_openssl/tests/validate.yml

@@ -0,0 +1,35 @@
+- name: Validate private key (test)


Move to targets directory

gundalow · 2017-07-12T09:16:38Z

test/integration/roles/test_openssl/tasks/main.yml

@@ -0,0 +1,19 @@
+- name: Generate a private key


Install whatever dependencies are needed to the OS listed in
https://github.com/ansible/ansible/blob/devel/test/runner/completion/docker.txt
+

freebsd/10.3-STABLE
freebsd/11.0-STABLE
osx/10.11
rhel/7.3

mattclay

The tests would be much easier to read if they were all in main.yml instead of split across multiple files. Splitting them up only makes sense when the tests are very large.

mattclay · 2017-07-12T16:31:11Z

test/integration/targets/openssl_csr/tasks/CentOS.yml

+  become: yes
+  package:
+    name: python-pip
+    state: latest


You shouldn't need to install pip. That is handled by ansible-test as part of the test infrastructure.

The same is true for the other tests.

mattclay · 2017-07-12T16:32:32Z

test/integration/targets/openssl_csr/aliases

@@ -0,0 +1,2 @@
+posix/ci/group1
+destructive


You can remove destructive since there doesn't appear to be anything destructive about these tests.

The same is true for the other tests.

I'd said destructive as this test uses pip to upgrade some a package.

mattclay · 2017-07-12T16:36:02Z

test/integration/targets/openssl_csr/tasks/CentOS.yml

+  become: yes
+  package:
+    name: epel-release
+    state: latest


This shouldn't be needed.

The same is true for the other tests.

mattclay · 2017-07-12T16:36:35Z

test/integration/targets/openssl_csr/tasks/CentOS.yml

+  package:
+    name: PyOpenSSL
+    state: latest
+    use: pip


Install using the pip module instead of the package module.

The same is true for the other tests.

mattclay · 2017-07-12T21:46:19Z

CI failure in integration tests. Here's one of the failures:

Traceback (most recent call last):
  File "/tmp/ansible_i__h80ey/ansible_module_openssl_csr.py", line 327, in <module>
    main()
  File "/tmp/ansible_i__h80ey/ansible_module_openssl_csr.py", line 295, in main
    csr = CertificateSigningRequest(module)
  File "/tmp/ansible_i__h80ey/ansible_module_openssl_csr.py", line 204, in __init__
    for (key, value) in self.subject.items():
RuntimeError: dictionary changed size during iteration

pilou- · 2017-07-12T22:05:23Z

test/integration/targets/openssl_csr/tests/validate.yml

+  shell: 'openssl rsa -noout -modulus -in {{ output_dir }}/privatekey.pem | openssl md5'
+  register: privatekey_modulus
+
+- name: Validate CSR (test - Common Name)


-nameopt switch should be used in order to be compatible with OpenSSL >= 1.1.0: the default format was changed between OpenSSL 1.0.2 and OpenSSL 1.1.0.

For example openssl req -noout -subject -in csr -nameopt oneline,-space_eq with csr_cn.stdout.split('=')[-1] could be used.

pilou- · 2017-07-12T22:05:49Z

test/integration/targets/openssl_csr/tasks/main.yml

@@ -0,0 +1,23 @@
+- name: Install libffi-devel


These two first tasks are duplicated in three files (openssl_csr/tasks/main.yml, openssl_privatekey/tasks/main.yml, openssl_publickey/tasks/main.yml): these tasks could be put in a dedicated role, something like setup_openssl.

pilou- · 2017-07-12T22:22:03Z

It seems these tests found a Python 3 related bug:

# on linux/ubuntu1604py3/1:
TASK [openssl_csr : Generate CSR] ***
The full traceback is:
Traceback (most recent call last):
   File "/tmp/ansible_xg20t0t7/ansible_module_openssl_csr.py", line 327, in <module>
     main()
   File "/tmp/ansible_xg20t0t7/ansible_module_openssl_csr.py", line 295, in main
     csr = CertificateSigningRequest(module)
   File "/tmp/ansible_xg20t0t7/ansible_module_openssl_csr.py", line 204, in __init__
     for (key, value) in self.subject.items():
 RuntimeError: dictionary changed size during iteration

OpenSuse builds failed on pip install PyOpenSSL with:

TASK [openssl_csr : Install libffi-devel] **************************************
task path: /root/ansible/test/integration/targets/openssl_csr/tasks/main.yml:1
skipping: [testhost] => {
    "changed": false, 
    "skip_reason": "Conditional result was False", 
    "skipped": true
}

TASK [openssl_csr : pip install PyOpenSSL] 
c/_cffi_backend.c:15:17: fatal error: ffi.h: No such file or directory

ansible_os_family should be used instead of ansible_distribution.

Spredzy · 2017-07-17T17:40:41Z

@mattclay @gundalow @pilou- all review have been addressed, could you please review this PR again ?
Thanks in advance.

pilou- · 2017-07-17T23:37:31Z

test/integration/targets/openssl_csr/tasks/main.yml

+    privatekey_path: '{{ output_dir }}/privatekey.pem'
+    commonName: 'www.ansible.com'
+
+- import_tasks: ../tests/validate.yml


Two things (applicable to openssl_privakey and openssl_publickey too):

why import_tasks: ../tests/validate.yml instead of import_tasks: validate.yml?

why are validation tasks split in another file (validate.yml) and not kept in main.yml?

Apart from these minor points, it looks good to me.

@pilou-

why import_tasks: ../tests/validate.yml instead of import_tasks: validate.yml?

Because for some reason it can't find the proper path to the file if not specified that way.
This is the command I run:

#> source hacking/env-setup #> ansible-test integration --docker=centos7 -v openssl_csr

why are validation tasks split in another file (validate.yml) and not kept in main.yml?

To logically separate what is the scenario I am testing vs. the test suite to validate the scenario.

If this is a blocker I could revert, but if possible, I'd like to keep it that way

mattclay · 2017-07-18T15:28:54Z

test/integration/targets/openssl_csr/tasks/main.yml

@@ -0,0 +1,14 @@
+- import_role:
+    name: setup_openssl


Please use meta dependencies instead of import_role here and in the other tests.

Example: https://github.com/ansible/ansible/blob/devel/test/integration/targets/mysql_db/meta/main.yml

This will permit ansible-test to properly handle the role dependencies when analyzing changes from PRs.

Spredzy · 2017-07-21T17:19:47Z

@mattclay commit updated. Can I ask for a new review?

mattclay · 2017-07-25T07:16:26Z

CI is failing with the following error:

Traceback (most recent call last):
  File "/tmp/ansible_IF6XA1/ansible_module_openssl_privatekey.py", line 294, in <module>
    main()
  File "/tmp/ansible_IF6XA1/ansible_module_openssl_privatekey.py", line 273, in main
    private_key.generate(module)
  File "/tmp/ansible_IF6XA1/ansible_module_openssl_privatekey.py", line 205, in generate
    self.fingerprint = get_fingerprint(self.path, self.passphrase)
  File "/tmp/ansible_IF6XA1/ansible_modlib.zip/ansible/module_utils/crypto.py", line 45, in get_fingerprint
TypeError: Last argument must be string or callable

mattclay · 2017-07-25T07:17:55Z

Once the issue exposed by the tests is fixed this should be good to merge.

Crypto namespace contains the openssl modules. It has no integration testing as of now. This commits aims to add integration tests for the crypto namespace. This will make it easier to spot breaking changes in the future. This tests currently apply to: * openssl_privatekey * openssl_publickey * openssl_csr

Spredzy · 2017-07-25T11:51:42Z

Thanks @mattclay @pilou- @gundalow !

ansibot added affects_2.4 This issue/PR affects Ansible v2.4 feature_pull_request needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 12, 2017

gundalow removed the needs_triage Needs a first human triage before being processed. label Jul 12, 2017

gundalow reviewed Jul 12, 2017

View reviewed changes

Spredzy force-pushed the openssl_integration_test branch 5 times, most recently from 2dc0b62 to 43d0a17 Compare July 12, 2017 13:54

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 12, 2017

mattclay requested changes Jul 12, 2017

View reviewed changes

Spredzy force-pushed the openssl_integration_test branch 4 times, most recently from c778ec7 to 780e480 Compare July 12, 2017 19:40

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jul 12, 2017

pilou- reviewed Jul 12, 2017

View reviewed changes

Spredzy force-pushed the openssl_integration_test branch from 780e480 to c94c132 Compare July 13, 2017 09:13

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 13, 2017

Spredzy force-pushed the openssl_integration_test branch 3 times, most recently from 3b37e61 to c8ff742 Compare July 13, 2017 11:30

ansibot added module This issue/PR relates to a module. owner_pr This PR is made by the module's maintainer. support:community This issue/PR relates to code supported by the Ansible community. labels Jul 13, 2017

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 16, 2017

Spredzy force-pushed the openssl_integration_test branch 3 times, most recently from 0e298fb to 9833e0f Compare July 16, 2017 22:18

ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Jul 16, 2017

pilou- reviewed Jul 17, 2017

View reviewed changes

Spredzy mentioned this pull request Jul 18, 2017

Public Core Meeting Agenda - June, July 2017 ansible/community#162

Closed

mattclay requested changes Jul 18, 2017

View reviewed changes

Spredzy force-pushed the openssl_integration_test branch from 9833e0f to 91e95da Compare July 19, 2017 06:27

ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 19, 2017

Spredzy force-pushed the openssl_integration_test branch from 91e95da to 6ea440d Compare July 19, 2017 21:47

ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 19, 2017

mattclay approved these changes Jul 25, 2017

View reviewed changes

Spredzy force-pushed the openssl_integration_test branch from 6ea440d to 63561b9 Compare July 25, 2017 10:42

gundalow approved these changes Jul 25, 2017

View reviewed changes

gundalow merged commit 8b22c45 into ansible:devel Jul 25, 2017

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 5, 2018

dagwieers added the crypto Crypto community (ACME, openssl, letsencrypt) label Feb 7, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable integration tests for the crypto/ namespace #26684

Enable integration tests for the crypto/ namespace #26684

Spredzy commented Jul 12, 2017

gundalow left a comment

gundalow Jul 12, 2017

Spredzy Jul 12, 2017

mattclay Jul 12, 2017

Spredzy Jul 12, 2017

gundalow Jul 12, 2017

gundalow Jul 12, 2017

gundalow Jul 12, 2017

mattclay left a comment

mattclay Jul 12, 2017

Spredzy Jul 12, 2017

mattclay Jul 12, 2017

Spredzy Jul 12, 2017

gundalow Jul 14, 2017

mattclay Jul 12, 2017

Spredzy Jul 12, 2017

mattclay Jul 12, 2017

Spredzy Jul 12, 2017

mattclay commented Jul 12, 2017

pilou- Jul 12, 2017

Spredzy Jul 13, 2017

pilou- Jul 12, 2017

pilou- commented Jul 12, 2017

Spredzy commented Jul 17, 2017

pilou- Jul 17, 2017 •

edited

Spredzy Jul 18, 2017 •

edited

mattclay Jul 18, 2017

Spredzy Jul 19, 2017

Spredzy commented Jul 21, 2017

mattclay commented Jul 25, 2017

mattclay commented Jul 25, 2017

Spredzy commented Jul 25, 2017

Enable integration tests for the crypto/ namespace #26684

Enable integration tests for the crypto/ namespace #26684

Conversation

Spredzy commented Jul 12, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

gundalow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattclay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattclay commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilou- commented Jul 12, 2017

Spredzy commented Jul 17, 2017

pilou- Jul 17, 2017 • edited

Choose a reason for hiding this comment

Spredzy Jul 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spredzy commented Jul 21, 2017

mattclay commented Jul 25, 2017

mattclay commented Jul 25, 2017

Spredzy commented Jul 25, 2017

pilou- Jul 17, 2017 •

edited

Spredzy Jul 18, 2017 •

edited