rhtconsulting / rhc-ose

OpenShift Automation and Utilities by Red Hat Consulting
42 stars 34 forks source link

Add OpenStack security group detection and creation #135

Closed vvaldez closed 8 years ago

vvaldez commented 8 years ago

What does this PR do?

Adds check for any defined security groups and SSH rule. If these do not exist these tasks will attempt to create them.

How should this be manually tested?

Delete any security groups before running ose-provision.yml. The security groups and SSH rule will be created before attempting to provision instances.

Is there a relevant Issue open for this?

No current issues

Who would you like to review this?

/cc @oybed @etsauer @sabre1041

oybed commented 8 years ago

Including @sabre1041 for review as well.

sabre1041 commented 8 years ago

Since with_items is being used to loop over the security groups, i added the following variable in the roles/openstack_create/defaults/main.yml file

security_groups: 
  - group1
  - group2

It ran through the new functionality, but failed at the nova_compute task with the following error:

 TASK: [openstack-create | Provision OpenStack master] ************************* 
failed: [localhost] => (item=1) => {"failed": true, "item": "1", "parsed": false}
Traceback (most recent call last):
  File "/root/.ansible/tmp/ansible-tmp-1459381797.16-177874326749596/nova_compute", line 2306, in <module>
    main()
  File "/root/.ansible/tmp/ansible-tmp-1459381797.16-177874326749596/nova_compute", line 583, in main
    _create_server(module, nova)
  File "/root/.ansible/tmp/ansible-tmp-1459381797.16-177874326749596/nova_compute", line 425, in _create_server
    'security_groups': module.params['security_groups'].split(','),
AttributeError: 'list' object has no attribute 'split'

FATAL: all hosts have already failed -- aborting

nova_compute expects security groups to be in a comma separated list.

I would recommend modifying your with_items logic to split a string such as:

with_items: security_groups.split(',')

By splitting the string, I believe it may help resolve #139

I am also seeing the following warning when running the playbook:

 [WARNING]: It is unnecessary to use '{{' in conditionals, leave variables in
loop expressions bare.

Finally, there is logic to always ensure security groups have port 22 added. For the sakes of composition, not every security group may want to include port 22 in their definition. Maybe checking that at least one security group has that port included would suffice?

vvaldez commented 8 years ago

@sabre1041 I've implemented your suggestions and tested with multiple security groups with success. The expected list of security_groups should be comma separated. This also work with only one group specified. I also changed the logic of how I'm checking for existing groups so I no longer need to ignore errors and check the results output, instead I'm capturing the results and using a search filter to find the group. I use this same technique to check for an existing SSH rule and if none if found in all the specified security groups, create a rule in the first element of the list only. I also removed all the {{}} in conditionals.

Issue #139 is no longer an issue as there is no need to do a validation. A failure on the command will use Ansible's standard error detection and will exit.

sabre1041 commented 8 years ago

@vvaldez lgtm. tested various scenarios and all pass. great work

@oybed did you want to take a look or should I merge?

oybed commented 8 years ago

@sabre1041 I'll take a look as well. I'm thinking it's all good, but a second set of eyes never hurts. :-) It's on my todo list, so I'll try to get to it later tonight or over the weekend.

oybed commented 8 years ago

Looked good - merged.