Migrate ansible-conjur-host-identity #38

BradleyBoutcher · 2020-08-17T17:56:13Z

What does this PR do?

Migrates project files (tasks, templates, meta)
Migrates Github issue templates
Adds code climate and Gitleaks configurations
Migrates test files
- Since the Conjur tests and host-identity tests have some subtle differences,
  they were moved to their own test directories. However, this can be improved later to potentially use the same images.
- Test runner updated and Jenkinsfile updated to run both sets of tests in parallel

What ticket does this PR close?

Connected to cyberark/ansible-conjur-host-identity#30

Checklists

Change log

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, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

Follow-up work

Generate images used for integration tests with a build.sh script located in the ci directory. Since both sets of tests depend on similar images, we can probably cut down on build time by centralizing them.

andytinkham

Quality-Architects are on the review for the .codeclimate.yml file. That looks good to me. The rest is not anything I reviewed though.

izgeri

This has made some nice progress, but I've left a bunch of comments still - some of which can be filed as follow-on issues to get this in and continue work in smaller follow-on issues. I also want to note:

I don't think meta/runtime.yml should have been moved - it looks like that belongs in the project root
if we're going to move the lookup plugin tests to a subdirectory of tests/, can the subdirectory be named to better indicate what it contains? e.g. lookup_plugin?
I noted in the PR that the role tests should also live under the project root tests/ directory; file a follow up issue if that's easier, but either way I think we'll need to do this

CHANGELOG.md

izgeri · 2020-09-23T18:04:28Z

Jenkinsfile

-        junit 'tests/junit/*'
+      parallel {
+        stage("Test conjur_lookup Plugin") {
+          agent { label 'executor-v2-large' }


why does this need a large executor?

It doesn't, but I ran into issues with all the test containers running on the same executor and this was the only solution that I had at the time. I'm not sure how to fix it otherwise right now.

was it just midair collisions due to not setting COMPOSE_PROJECT_NAME or something else?

regardless, this merits a follow-up issue too

That might be it, I'll investigate

This has been resolved! The COMPOSE_PROJECT_NAME was being set in the individual test scripts for the plugin and role, which overrode the project name I was assigning. I changed them a bit so that they won't confuse docker.

README.md

ci/parse-changelog.sh

ci/test.sh

izgeri · 2020-09-23T18:48:08Z

roles/conjur_host_identity/meta/main.yml

+    - name: Ubuntu
+      versions:
+        - trusty
+        - xenial
+    - name: EL
+      versions:
+        - 7


this is worth digging into at some point, but looks ok for now

@BradleyBoutcher should be captured in an issue, that we need to review the supported platforms for the role

izgeri · 2020-09-24T16:58:28Z

CHANGELOG.md

+- Added retries to tasks/identity/Request identity from Conjur.
+  This will increase the reliability of host factory requests without introducing any extra delay 


is this something you did in this PR, or was this ported from an unreleased change in the role?

This was ported from the Conjur-host-identity project. I wasn't sure what to do with it, because those features are currently unreleased.

I think we can leave it in there, and tag that project just before we tag this one to indicate the migration

just rereading and realizing this is not clear, I meant that we can drop this entry from the collection changelog and just be clear in the collection changelog that we migrated at the point-in-time of the {soon to be added} last role release.

Ah gotcha, I will do so!

izgeri

left a few more minor comments - this is getting quite close to done, and then we'll have a handful of follow-up issues to work on before we can tag. can you please be sure to add a comment to this PR that aggregates the links to the follow-up issues we'll need after this PR is merged?

izgeri · 2020-09-24T16:59:45Z

Jenkinsfile

-        junit 'tests/junit/*'
+      parallel {
+        stage("Test conjur_lookup Plugin") {
+          agent { label 'executor-v2-large' }


regardless, this merits a follow-up issue too

README.md

izgeri · 2020-09-24T18:13:27Z

README.md

+- `CONJUR_ACCOUNT` : The Conjur account name
+- `CONJUR_APPLIANCE_URL` : URL of the running Conjur service
+- `CONJUR_CERT_FILE` : Path to the Conjur certificate file
+- `CONJUR_AUTHN_LOGIN` : A valid Conjur host username
+- `CONJUR_AUTHN_API_KEY` : The api key that corresponds to the Conjur host username
+- `CONJUR_AUTHN_TOKEN_FILE` : Path to a file containing a valid Conjur auth token


it's striking me as weird that you configure the role with playbook keys (?) and you configure the lookup plugin with env vars. how do you use them together? if I set up the role, will the plugin not be configured?

this might be a follow-up issue (it would go hand-in-hand with your own experiments running the e2e flow)

I think that's just the nature of how they work. The role is configured through the playbook with what are called "role variables", while the plugin is an executable that may depend on variables being present in the system. Both environment variables and role variables can be set in the playbook, but it's an important distinction, and people who are familiar with playbooks will know how to use them. Boilerplate READMEs for Ansible roles include the role variables section, and for the plugin, it's a good idea for us to have them there.

On the topic of "can I set them once and have them used in both places?" - I'm not sure. There's probably a way to chain things together, but I'm not sure if that's necessary or a "safe" way to handle parameters for a role and plugin.

@BradleyBoutcher can we make sure it's captured in a follow-up issue that we need to review the UX of using the role and then the lookup plugin within the collection?

izgeri · 2020-09-24T18:14:12Z

README.md

+```
+
+This example:
+- Registers the host `{insert whatever the hostname is}` with Conjur, adding it into the Conjur 


the note insert whatever the hostname is is for you - what is the hostname? is it the value of inventory_hostname? should this be replaced with {{ inventory_hostname }}, or is there something more clear we could put here instead?

Agreed - it should tie into the example above with {{ inventory_hostname }}

README.md

izgeri · 2020-09-24T18:16:10Z

CHANGELOG.md

+- Added retries to tasks/identity/Request identity from Conjur.
+  This will increase the reliability of host factory requests without introducing any extra delay 


I think we can leave it in there, and tag that project just before we tag this one to indicate the migration

izgeri · 2020-09-24T18:16:56Z

README.md


-This collection contains plugins to be used for CyberArk Conjur & DAP hosted in [ansible galaxy](https://galaxy.ansible.com/cyberark/conjur).
+- [CyberArk Ansible Conjur Collection](#cyberark-ansible-conjur-collection)


can the TOC be updated to remove this top level item? it doesn't add anything, and it just indents the whole TOC over one

izgeri · 2020-09-24T18:17:23Z

README.md

+```yml
+- hosts: servers
+  roles:
+    - role: cyberark.conjur-host-identity


is this definition the same once it's part of the collection?

I'm not sure, but I should be able to answer this once I've finished testing more

might be good to know for sure before we merge, since this will land in the project README on merge

@BradleyBoutcher I think this still needs to be verified ...

Based on the documentations and my short experimenting, the correct syntax is cyberark.conjur.conjur_host_identity when using this role installed through the collection.

izgeri · 2020-09-24T19:00:45Z

README.md

+## Conjur Ansible Role
+This Ansible role provides the ability to grant Conjur machine identity to a host. Based on that
+identity, secrets can then be retrieved securely using the [Conjur Lookup
+Plugin](#conjur_variable-lookup-plugin) or using the [Summon](https://github.com/cyberark/summon)


Suggested change

Plugin](#conjur_variable-lookup-plugin) or using the [Summon](https://github.com/cyberark/summon)

Plugin](#conjur-ansible-lookup-plugin) or using the [Summon](https://github.com/cyberark/summon)

README.md

sgnn7

Few nits

sgnn7 · 2020-09-25T19:33:35Z

.gitignore

+molecule/conjur.pem
+*.tmp
+vendor/
+.pytest_cache


nit: These should be grouped a bit better

sgnn7 · 2020-09-25T19:34:42Z

README.md

+```
+
+## Conjur Ansible Role
+**NOTE**: This role is currently not available in releases installed through Ansible Galaxy, but will be added in the next release. Follow [issue #30](https://github.com/cyberark/ansible-conjur-collection/issues/35) for updates.


Nit: there should be an empty line between headers and text in this file

sgnn7 · 2020-09-25T19:35:27Z

README.md

-None.
-<br>
-<br>
+* `conjur_appliance_url` (_Optional)_: URL of the running Conjur service


Suggested change

* `conjur_appliance_url` (_Optional)_: URL of the running Conjur service

* `conjur_appliance_url` _(Optional)_: URL of the running Conjur service

I'm guessing these other ones need that too

sgnn7 · 2020-09-25T19:36:09Z

README.md

+* `summon.version`: version of Summon to install. Default is `0.8.2`.
+* `summon_conjur.version`: version of Summon-Conjur provider to install. Default is `0.5.3`.
+
+The variables marked with _`(Optional)`_ are required fields. All other variables are required for


Hmm... an optional value is required?

HA good catch

sgnn7 · 2020-09-25T19:37:32Z

README.md

+- Installs Summon with the Summon Conjur provider for secret retrieval from Conjur.
+
+### Summon & Service Managers
+With Summon installed, using Conjur with a Service Manager (like SystemD) becomes a snap.  Here's a


Suggested change

With Summon installed, using Conjur with a Service Manager (like SystemD) becomes a snap. Here's a

With Summon installed, using Conjur with a Service Manager (like `systemd`) becomes a snap. Here's a

sgnn7 · 2020-09-25T19:37:52Z

README.md

+- Installs Summon with the Summon Conjur provider for secret retrieval from Conjur.
+
+### Summon & Service Managers
+With Summon installed, using Conjur with a Service Manager (like SystemD) becomes a snap.  Here's a


Needs empty newline before header

sgnn7 · 2020-09-25T19:38:05Z

README.md

+
+### Summon & Service Managers
+With Summon installed, using Conjur with a Service Manager (like SystemD) becomes a snap.  Here's a
+simple example of a SystemD file connecting to Conjur:


Suggested change

simple example of a SystemD file connecting to Conjur:

simple example of a `systemd` file connecting to Conjur:

Per Ansible Collection documentation, the conjur_host_identity role has been moved to a `role` subdirectory. A `tests` subdirectory has been added for this role, and all relevant tests moved there.

izgeri

There is one last item to review the supported platforms, but everything else LGTM. Thanks for sticking with this one @BradleyBoutcher! Looking forward to addressing the follow up issues and releasing an updated version of this collection.

My review was partial and at this time outdated so it can be dismissed

BradleyBoutcher requested review from a team as code owners August 17, 2020 17:56

BradleyBoutcher force-pushed the migrate-host-identity branch 15 times, most recently from 763f814 to 579000e Compare August 18, 2020 21:30

BradleyBoutcher changed the title ~~WIP: Migrate ansible-conjur-host-identity~~ Migrate ansible-conjur-host-identity Aug 18, 2020

BradleyBoutcher force-pushed the migrate-host-identity branch 6 times, most recently from faf2b0b to 0cc0384 Compare August 19, 2020 15:16

andytinkham reviewed Aug 19, 2020

View reviewed changes

BradleyBoutcher force-pushed the migrate-host-identity branch 5 times, most recently from 6c03545 to 6525585 Compare August 19, 2020 20:17

BradleyBoutcher force-pushed the migrate-host-identity branch 4 times, most recently from aebc97e to 4dd364a Compare September 23, 2020 16:06

BradleyBoutcher requested review from sgnn7 and izgeri September 23, 2020 16:07

izgeri suggested changes Sep 23, 2020

View reviewed changes

BradleyBoutcher force-pushed the migrate-host-identity branch 4 times, most recently from f97c052 to 54e6955 Compare September 24, 2020 15:58

izgeri reviewed Sep 24, 2020

View reviewed changes

BradleyBoutcher force-pushed the migrate-host-identity branch from 54e6955 to dca4c33 Compare September 24, 2020 18:15

izgeri suggested changes Sep 24, 2020

View reviewed changes

BradleyBoutcher force-pushed the migrate-host-identity branch from dca4c33 to f7bed15 Compare September 24, 2020 18:50

izgeri reviewed Sep 24, 2020

View reviewed changes

BradleyBoutcher force-pushed the migrate-host-identity branch 2 times, most recently from b7f0bd5 to e8ccd5d Compare September 25, 2020 14:34

BradleyBoutcher mentioned this pull request Sep 25, 2020

Roles and plugins share a build stage and test directory structure #44

Open

BradleyBoutcher force-pushed the migrate-host-identity branch from e8ccd5d to 9a72886 Compare September 25, 2020 15:29

izgeri reviewed Sep 25, 2020

View reviewed changes

README.md Show resolved Hide resolved

BradleyBoutcher force-pushed the migrate-host-identity branch from 9a72886 to c0a9b74 Compare September 25, 2020 16:56

sgnn7 reviewed Sep 25, 2020

View reviewed changes

BradleyBoutcher force-pushed the migrate-host-identity branch from c0a9b74 to e182a0b Compare September 25, 2020 21:42

Create role subdirectory

00c1514

Per Ansible Collection documentation, the conjur_host_identity role has been moved to a `role` subdirectory. A `tests` subdirectory has been added for this role, and all relevant tests moved there.

BradleyBoutcher force-pushed the migrate-host-identity branch from e182a0b to 00c1514 Compare September 25, 2020 21:56

izgeri approved these changes Sep 28, 2020

View reviewed changes

BradleyBoutcher merged commit 01b0c87 into master Sep 29, 2020

BradleyBoutcher deleted the migrate-host-identity branch September 29, 2020 14:44

		- Added retries to tasks/identity/Request identity from Conjur.
		This will increase the reliability of host factory requests without introducing any extra delay


		This collection contains plugins to be used for CyberArk Conjur & DAP hosted in [ansible galaxy](https://galaxy.ansible.com/cyberark/conjur).
		- [CyberArk Ansible Conjur Collection](#cyberark-ansible-conjur-collection)

	Plugin](#conjur_variable-lookup-plugin) or using the [Summon](https://github.com/cyberark/summon)
	Plugin](#conjur-ansible-lookup-plugin) or using the [Summon](https://github.com/cyberark/summon)

	* `conjur_appliance_url` (_Optional)_: URL of the running Conjur service
	* `conjur_appliance_url` _(Optional)_: URL of the running Conjur service

	With Summon installed, using Conjur with a Service Manager (like SystemD) becomes a snap. Here's a
	With Summon installed, using Conjur with a Service Manager (like `systemd`) becomes a snap. Here's a

	simple example of a SystemD file connecting to Conjur:
	simple example of a `systemd` file connecting to Conjur:

Migrate ansible-conjur-host-identity #38

Migrate ansible-conjur-host-identity #38

Conversation

BradleyBoutcher commented Aug 17, 2020 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

Follow-up work

andytinkham left a comment

Choose a reason for hiding this comment

izgeri 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

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

izgeri 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

BradleyBoutcher Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

BradleyBoutcher Sep 24, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgnn7 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

izgeri left a comment

Choose a reason for hiding this comment

BradleyBoutcher commented Aug 17, 2020 •

edited

Loading

BradleyBoutcher Sep 24, 2020 •

edited

Loading

BradleyBoutcher Sep 24, 2020 •

edited

Loading