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

make tasks dynamic #194

Closed
wants to merge 3 commits into from
Closed

make tasks dynamic #194

wants to merge 3 commits into from

Conversation

tb3088
Copy link

@tb3088 tb3088 commented Nov 16, 2018

I have a general hardening Playbook which has both RHEL6 and RHEL7 roles in the hierarchy which should be perfectly valid and reasonable. The use of 'import_tasks' forcibly adds the items and causes them to be evaluated individually only to be skipped by virtue of WHEN pre-conditions higher up. Changing to 'include_tasks' dramatically speeds up Ansible run time.

juliedavila
juliedavila previously approved these changes Nov 16, 2018
Copy link
Contributor

@juliedavila juliedavila left a comment

Choose a reason for hiding this comment

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

I'm okay with this.

@jamescassell
Copy link
Collaborator

This breaks --list-tasks and --tags=cat1,2,3. Also, even ignoring that, we'd have to add tags: always to allow continued use of --tags RHEL-07-XXXXXX

@jamescassell
Copy link
Collaborator

When writing a playbook that includes both RHEL6 and RHEL7, I've found it most successful to have the 6 and 7 roles in separate plays, specifically to avoid collusion of handlers, but also it has the side effect of not evaluating each RHEL6 task on RHEL7, and vice versa, likely solving your issue.

@juliedavila
Copy link
Contributor

Ah never realized the tags problem, yeah that's not good. @tb3088 have you attempted to use the roles in separate plays (within the same playbook)?

@tb3088
Copy link
Author

tb3088 commented Nov 19, 2018

my novice status with Ansible is showing... I split my playbook into separate plays.
I dont' know what the solution is WRT tags, but I find it incredibly inefficient that an Ansible run will iterate thru every test in the Cat1-3 collections only to skip them due to the Skip-Tags.
Why optimize for the do-nothing command '--list-tasks'?

@tb3088
Copy link
Author

tb3088 commented Nov 19, 2018

I was hoping to do something like this but the 2 roles (rhel6 and 7) have all their variable names prepended with the role name. If they were all standardized (they're not) and stripped of OS and revision specifiers (eg. rhel6stig_cat1 -> stig_cat1, rhel_07_021021 -> stig_021021) then it would be straight-forward, no?

---
- name: Apply STIG remediation
  hosts: all
  vars:
    do_cat1: yes
    login_banner: some long text
  tasks:
  -import_role:
    name: "RHEL{{ansible_facts['distribution_major_version']}}-STIG"
    vars:
      stig_cat1: "{{ do_cat1 }}"
      ...

@shepdelacreme
Copy link
Contributor

The variable names in each role are named that way so that they stay namespaced to the role. In Ansible if you have two different roles included in a playbook and those roles have a variable named the same one of the values will overwrite the other. See: ansible/ansible#2796

For the other stuff I plan to do some testing to see what you are trying to accomplish and whether there is some improvements that can be made. I too dislike the skipped task output but I'm not sure there is a way around it since we still want tags to work the way they do/etc.

@tb3088
Copy link
Author

tb3088 commented Nov 19, 2018

wow, I didn't realize Vars were not natively name-spaced. I'm coming from Chef and Puppet so of course they would be...

@jamescassell
Copy link
Collaborator

ANSIBLE_STDOUT_CALLBACK=skippy will hide the skipped tasks from output. Switching to a dynamic include would only improve the case of running a subset of the role based on category.

I'd recommend explicitly noting that running multiple lockdown roles in the same play is not supported. Otherwise, you end up calling RHEL6 handlers on RHEL7 hosts and vice versa.

If you really want them in the same play, use the dynamic include_role to achieve your goals wrt skipped tasks, and make that include_role dependent on os_family.

@shepdelacreme
Copy link
Contributor

What @jamescassell said about using include_role instead of the import_role you have in your example @tb3088 works. However you are still going to run into an issue with conflicting handlers. See ansible/ansible#15476 for more information about this issue.

@jamescassell should we consider namespacing handlers or putting OS conditionals on each handler to avoid this issue?

Example playbook below:

---
- name: Apply STIG
  hosts: all
  become: yes

  tasks:
    - name: Apply STIG
      include_role:
        name: "RHEL{{ansible_facts['distribution_major_version']}}-STIG"

@jamescassell
Copy link
Collaborator

I haven't tested, but I suspect your example playbook would do what most users would expect and only pull in handlers for the appropriate OS. I wouldn't jump to namespacing A or conditionalizing handlers unless we really have to...

@tb3088
Copy link
Author

tb3088 commented Nov 20, 2018

I'm getting closer...
https://gist.github.com/tb3088/4ea9324ce6c3d10ea7ef08559797c5c9

Looks like some kind of variable scoping problem?

fatal: [default]: FAILED! => {"msg": "The conditional check 'rhel6stig_cat3' failed. The error was: error while evaluating conditional (rhel6stig_cat3): 'do_cat3' is undefined\n\nThe error appears to have been in '/home/ec2-user/ami-config/ansible/OSCAP/roles/RedHat-6-STIG/tasks/prelim.yml': line 149, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: "PRELIM | Check for device files with unlabeled_t context"\n ^ here\n"}

I checked out the Git repo (a submodule) as 'RedHat-#' to follow Ansible Facts values.

@shepdelacreme
Copy link
Contributor

@jamescassell Nope in my example play the RHEL7 handlers run on the RHEL6 host unfortunately. Using - meta: flush_handlers between roles doesn't help either.

@tb3088 In your example gist you have 3 separate plays in a single file...which is the correct approach to avoid the handler problem for now, except the vars you set in the first play are scoped to that first play only which is why do_cat3 is undefined...really they are all undefined. See https://docs.ansible.com/ansible/2.7/user_guide/playbooks_variables.html#scoping-variables

If you want to set/use the vars like that then you should create a file like group_vars/all.yml and drop them in there.

@tb3088
Copy link
Author

tb3088 commented Nov 22, 2018

After much "learning" with Ansible, I've settled on the following. If you note, I introduced my own topical variables with allow me to get some inkling as to what behavior I'm modifying. It won't scale too far but I don't expect to have to maintain too many divergent items.

https://gist.github.com/tb3088/561d4bc2cee6f32e8da7efb603b04dd6

@shepdelacreme
Copy link
Contributor

@tb3088 I'm going to close this PR since you've worked out the method to accomplish what you need!

@tb3088
Copy link
Author

tb3088 commented Nov 27, 2018

The Example playbook in the docs is a little light. Would there be merit to including a section of mine that would help others figure it out more readily, or was my problem my lack of experience with Ansible and therefore the correct solution being RTFM?

Also, does it make sense to standardize variables in the form header.topic.sub-item instead of header_topic.sub-item? eg. rhel7stig_password_complexity.maxrepeat -> rhel7stig.password.complexity.maxrepeat?

@shepdelacreme
Copy link
Contributor

@tb3088 For the first part...yes we are working on better docs for the role and we plan to put more detailed examples of usage in the https://github.com/ansible/ansible-lockdown repo.

For the variable part...I'm not sure it makes sense to have header.topic.sub-item To me that would make the yaml harder to read?

rhel7stig_password_complexity:
    maxrepeat: 24
    somethingelse: 111

vs

rhel7stig:
    password:
        complexity:
            maxrepeat: 4
            soemthingelse: 111

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

Successfully merging this pull request may close these issues.

None yet

4 participants