Skip to content
This repository has been archived by the owner on Jul 11, 2020. It is now read-only.

Initial OpenShift functionality #34

Merged
merged 5 commits into from May 16, 2020

Conversation

tylerauerbeck
Copy link
Contributor

@tylerauerbeck tylerauerbeck commented Mar 16, 2020

Resolves #15

This PR makes some small changes that allow for an easy deployment on OpenShift:

In tower role defaults:

  • Add tower_postgres_data_path and set it to /var/lib/postgresql/data/pgdata
  • Add tower_ingress_type variable to allow specifying either ingress or route. Defaults to ingress to maintain current functionality
  • Add tower_multitenant variable to allow enabling/disabling bubblewrap functionality (i.e. enabling/disabling AWX_PROOT_ENABLED in settings.py found in the configmap). Older 3.X documents seem to point to disabling this via this variable (https://docs.ansible.com/ansible-tower/3.1.3/html/administration/proot_func_variables.html), but it seems to have dissapeared from the 3.6.3 documentation. May just be an oversight as I still see it in use here. Settings this to false disables bubblewrap and removes the privileged: true securityContext from the task pod. Setting it to true sets this variable to TRUE and then enables this securityContext. Would be good to mention in the README that enabling this would then require adding an SCC in order to run on OpenShift.

In tower_postgres.yaml.j2:

  • Add PGDATA env variable and default it to tower_postgres_data_path.
    • Due to some of the chowning/chmoding that happens, this was causing issues with postgres spinning up. By settings this to a path that is under the mount, it is no longer owned by root and we're able to allow changing of permissions
  • Modify volumeMount to utilize different parts of tower_postgres_data_path. Now the mountPath is set to the parent directory of this variable (i.e. /var/lib/postgresql/data) which had been the default path before and the subPath is now set to name of the directory

In tower_web.yaml.j2:

  • If the tower_ingress_type is set to ingress, it will create an ingress object
  • If the tower_ingress_type is set to route, it will create a route object

@tylerauerbeck
Copy link
Contributor Author

@geerlingguy This looks to be passing initial tests and I'm able to verify that everything is up and running on my OpenShift cluster. Let me know if there's anything you would like to see before we call this one complete.

@tylerauerbeck tylerauerbeck changed the title WIP: Initial OpenShift functionality Initial OpenShift functionality Mar 23, 2020
deploy/role.yaml Show resolved Hide resolved
deploy/tower-operator.yaml Show resolved Hide resolved
@@ -1,4 +1,7 @@
---
tower_multitenant: false
Copy link
Owner

Choose a reason for hiding this comment

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

I would rather set the default to true, since many use cases (maybe the majority?) would have multi-tenancy as a default feature and have more than one user in the system.

Regardless, we need to document what this does (if true, it makes the tower/awx pod run with privileged: true and AWX_PROOT_ENABLED is true, and false is vice-versa), and link those docs to the Security docs page.

I do wonder why the docs for this setting were removed in 2.6.x...

Copy link
Owner

Choose a reason for hiding this comment

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

I like what you mentioned earlier in #15:

Or maybe provide a way to do both and just write up the docs that describe some of the benefits/risks of both?

Basically, default to the more secure deployment, and we can also add the Add privileged SCC to service account task maybe... but allow the option for those who choose to turn off that extra security layer by changing the default from true to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll get this documented. I went with false as that actually looks what it is set to by default right now in the configmap template. So that will be a change from how the operator runs by default today.

roles/tower/templates/tower_web.yaml.j2 Show resolved Hide resolved
@geerlingguy
Copy link
Owner

@tylerauerbeck - Just checking in — I'm eager to get this merged (even though it may be a slightly breaking change for existing users), and hopefully make life in OCP clusters a little easier. Would you have time in the next couple weeks to work on these final tweaks?

@tylerauerbeck
Copy link
Contributor Author

@geerlingguy Yepp, sorry for the delay. Just freeing up from a few other things this week. Should hopefully be able to get to this today/this weekend. I'll get those changes pushed and drop you a message here when it's ready.

@tylerauerbeck
Copy link
Contributor Author

@geerlingguy So after taking a look back at this, I think I may have gotten a bit ahead of myself. We don't actually have to mess around with the AWX_PROOT_ENABLED flag at all. The issue deals strictly with the privileged: true security context for the task deployment. So for now, I'm going to revert things back to what they're set to now for AWX_PROOT_ENABLED and just have a setting for tower_task_privileged, which would then allow you to set the task to run as privileged if necessary (as it runs fine with my sample tests without it). Then I'll add to the docs for how you can go about enabling that and the concerns that go along with it.

On a side note, currently AWX_PROOT_ENABLED is set to false by default (and with the official installer it is set to true). I can see situations where you would probably want to either have that enabled or disabled, but I think this is probably suited better for a followup PR. Just wanted to leave something here so we don't forget about it.

@tylerauerbeck
Copy link
Contributor Author

@geerlingguy I think this is in pretty good shape now. Let me know if there's anything else you'd like to see before merging.

@geerlingguy
Copy link
Owner

Sounds good!

@kedark3
Copy link

kedark3 commented May 1, 2020

I would be interested to try this out.

@tylerauerbeck
Copy link
Contributor Author

@geerlingguy Is there anything else needed here before merging?

@geerlingguy geerlingguy merged commit 840fdc2 into geerlingguy:master May 16, 2020
@geerlingguy
Copy link
Owner

@tylerauerbeck - Just a nudge from you! I want to bump the Tower version as well before building a new image version, but I hope to do that early next week.

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

Successfully merging this pull request may close these issues.

Make sure Tower Operator can be deployed easily on OpenShift
3 participants