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

Run Roles tests against Role/Collection installed from Ansible Galaxy #75

Closed
wants to merge 3 commits into from

Conversation

john-odonnell
Copy link
Contributor

@john-odonnell john-odonnell commented Dec 22, 2021

Desired Outcome

An issue was reported in the Conjur docs regarding the Ansible Collection, described below.
The changes in this PR are intended to explicitly test use of the conjur-host-identity module installed from Ansible Galaxy, both individually and as part of the Conjur Collection.

Issue Report Details

Environment:

Ansible 2.9.27
CentOS 8.4

Procedure:

  1. Install the Conjur Ansible Collection:

    ansible-galaxy collection install cyberark.conju
    
  2. Run the following playbook to set up a Conjur role:

    - hosts: servers
      roles:
        - role: cyberark.conjur.conjur-host-identity
          conjur_appliance_url: 'https://conjur.myorg.com',
          conjur_account: 'myorg',
          conjur_host_factory_token: "{{ lookup('env', 'HFTOKEN') }}",
          conjur_host_name: "{{ inventory_hostname }}"
          conjur_ssl_certificate: "{{ lookup('file', '/path/to/conjur.pem') }}"
          conjur_validate_certs: yes
    

    This results in the following error:

    ERROR! the role 'cyberark.conjur.conjur-host-identity' was not found in /huydd/ansible/ansible-conjur-testing/roles:/root/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles:/huydd/ansible/ansible-conjur-testing
    

    The error was eventually resolved by using the individual Role instructions, meant for Ansible 2.8.

Issue Solution

The problem, in short:
Installing the Conjur Collection provides the role cyberark.conjur.conjur_host_identity, note the underscores (_)
Installing the standalone Role provides the role cyberark.conjur-host-identity, note the dashes (-)

From Ansible documentation on Role names:

Role names are limited to lowercase word characters (i.e., a-z, 0-9) and ‘’. No special characters are allowed, including ‘.’, ‘-‘, and space. During import, any ‘.’ and ‘-‘ characters contained in the repository name or role_name will be replaced with ‘’.

The error here is in the documentation:

...You can configure each Ansible node with a Conjur identity by including a section like the example below in your Ansible playbook:

- hosts: servers
 roles:
   - role: cyberark.conjur.conjur-host-identity
     conjur_appliance_url: 'https://conjur.myorg.com',
     conjur_account: 'myorg',
     conjur_host_factory_token: "{{ lookup('env', 'HFTOKEN') }}",
     conjur_host_name: "{{ inventory_hostname }}"
     conjur_ssl_certificate: "{{ lookup('file', '/path/to/conjur.pem') }}"
     conjur_validate_certs: yes

NOTE: Ansible 2.8 users who leverage the standalone role instead of the collection, should reference the role as: cyberark.conjur-host-identity instead.

The solution here should be to release a new version of the standalone Role, where the naming convention is corrected, using underscores. This would include updating the documentation to reference the Role by each method with underscores.

Implemented Changes

Updates to conjur-host-identity testing:

  • ansible service in docker-compose no longer mounts the repo for use in tests.
    • Instead, the Dockerfile used to build this service installs both the Conjur Collection and the Role module.
  • Updated test cases:
    • configure-conjur-identity-from-collection uses Collection-based Role
    • configure-conjur-identity-from-role uses individual Role

Connected Issue/Story

Resolves #72
Resolves #73
CyberArk internal issue link: ONYX-14387

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: insert issue ID
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@john-odonnell john-odonnell force-pushed the installation-bug-2-9 branch 3 times, most recently from 812116b to d2d5f3b Compare December 23, 2021 17:10
Copy link
Contributor

@doodlesbykumbi doodlesbykumbi left a comment

Choose a reason for hiding this comment

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

Some comments

@@ -71,7 +73,7 @@ function run_test_case {
"
docker exec "${ansible_cid}" bash -ec "
cd tests
py.test --junitxml=./junit/${test_case} --connection docker -v test_cases/${test_case}/tests/test_default.py
py.test --junitxml=./junit/${test_case} --connection docker -v test_cases/common/test_default.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Initially my feedback was going to be. Let's add a comment up top to clarify why we avoid this folder, and a comment here to clarify that the common folder contains the code for the shared common test cases. Now I'm thinking that we should perhaps

  1. Have common folder that has those common test cases in test_default.py
  2. Retain the convention/interface of each test case needing to specify a test_default.py so that line 76 statys as
py.test --junitxml=./junit/${test_case} --connection docker -v test_cases/common/test_default.py
  1. In each test case's test_default.py it can just import (1)

Comment on lines +45 to +46
RUN ansible-galaxy collection install cyberark.conjur && \
ansible-galaxy install cyberark.conjur-host-identity
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure about this, some considerations...

  1. This repo should really be testing its own source code, instead of what is on Ansible Galaxy
  2. The standalone role is separate to the collection, and I think any test for it should reside in the standalone role's repo. All that to maintain a separation of concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

For (1) I think if we want a smoke test to ensure that we can always install the role from galaxy then we should define that separate to the tests that currently exist.

- name: Configuring conjur identity on remote hosts from Role
hosts: testapp
roles:
- role: cyberark.conjur-host-identity
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea how this is still working given the role name constraints ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Troubleshoot Ansible Conjur installation on 2.9 Troubleshoot Ansible Conjur installation on 2.9
3 participants