-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Support Organization-scoped Galaxy installs #7817
Support Organization-scoped Galaxy installs #7817
Conversation
Build succeeded.
|
awx/main/redact.py
Outdated
exclude_list += [settings.PRIMARY_GALAXY_URL] | ||
if settings.FALLBACK_GALAXY_SERVERS: | ||
exclude_list += [server['url'] for server in settings.FALLBACK_GALAXY_SERVERS] | ||
# TODO: replace this with the server URLs from proj.org.credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to refactor this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, @AlanCoding I don't think we actually need this anymore at all - the point of this code is to filter HTTP basic auth creds out of URLs - starting w/ this PR we're only supporting tokens, so I don't actually think we need this anymore.
5da4bfd
to
e6b29e3
Compare
# List of dicts of fallback (additional) Galaxy servers. If configured, these | ||
# will be higher precedence than public Galaxy, but lower than primary Galaxy. | ||
# Available options: 'id', 'url', 'username', 'password', 'token', 'auth_url' | ||
FALLBACK_GALAXY_SERVERS = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wenottingham @AlanCoding I decided not to put this into the migration; I can't see where we ever actually documented this as a feature (it's not exposed in the API, either), and given the landscape of "Galaxy-like services" before now, I'd argue we can probably assume there's not (m)any people who've done this (unless you two know of any).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on what folks like GTS are doing. But I think the new way of setting this up is more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I decided to just support it and write some tests.
@@ -74,31 +74,6 @@ export default ['i18n', function(i18n) { | |||
AWX_SHOW_PLAYBOOK_LINKS: { | |||
type: 'toggleSwitch', | |||
}, | |||
PRIMARY_GALAXY_URL: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look, @mabashian, I know JavaScript!
env['ANSIBLE_GALAXY_SERVER_LIST'] = ','.join([server.get('id', 'unnamed') for server in galaxy_servers]) | ||
|
||
# build out env vars for Galaxy credentials (in order) | ||
galaxy_server_list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wenottingham @rebeccahhh @chrismeyersfsu @AlanCoding one behavioral change worth pointing out here is that for new installs and new Organizations (ignore old settings that get migrated on upgrade), the default behavior is "don't pull from galaxy.ansible.com by default". I think this is what I heard @wenottingham say he wanted - is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to document this, but I'm OK with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duly noted 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the API behavior we landed on, but I think as a nicety in the UI, when you create a new org, we're talking about pre-filling the field to include Ansible Galaxy. So at the time users create an org, they are immediately given the choice to opt in/out of "community Galaxy" at org creation time. But if you're automating org creation (via e.g., the CLI or modules), you've got to explicit set these credentials up yourself to represent where you want to pull content from by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see: #7817 (comment)
e6b29e3
to
520dfb0
Compare
@@ -12,23 +10,13 @@ class UriCleaner(object): | |||
|
|||
@staticmethod | |||
def remove_sensitive(cleartext): | |||
# exclude_list contains the items that will _not_ be redacted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little unclear why were doing this in the first place, @AlanCoding:
The point of this function is to take a giant blob of stdout and discover embedded URLs (printed by e.g., git
) like:
https://USER:PASS@foo
...and turn them into https://*********@foo
Why would we ever want to whitelist Galaxy/Hub URLs to prevent that behavior? I don't think we need this code at all anymore since we're losing basic auth support, but I think it was flawed to start with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it was important we would have test coverage for it. Maybe I was going crazy, I'm not sure.
Build succeeded.
|
6d89b96
to
9ec9499
Compare
Build succeeded.
|
# by default, prior versions of AWX/Tower automatically pulled content | ||
# from galaxy.ansible.com | ||
public_galaxy_enabled = True | ||
public_galaxy_setting = Setting.objects.filter(key='PUBLIC_GALAXY_ENABLED').first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice, that's how I would have done this inside of migrations.
awx/main/migrations/_galaxy.py
Outdated
organization=org, | ||
credential_type=galaxy_type, | ||
inputs = { | ||
'url': 'https://galaxy.ansible.com/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by this in particular. What happens if you create a new organization after this migration has finished? What credentials does it have associated, and how does it go about doing Galaxy commands before the user has associated any credentials?
Also, why would each organization get a new public galaxy "credential"? Wouldn't all orgs just share the same credential? It's 1 URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, shouldn't the public Galaxy credential be visible to all users? In this implementation, it seems like it would only be visible to superusers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by this in particular. What happens if you create a new organization after this migration has finished? What credentials does it have associated...
None. We're changing the default behavior when you create new Organizations to not pull unofficial community content (cc @wenottingham in case I've misunderstood this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why would each organization get a new public galaxy "credential"? Wouldn't all orgs just share the same credential? It's 1 URL.
That might be a better way to do this. Who's allowed to edit that one global credential, though? How would we even represent that with our current RBAC model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would each org be able to use this credential without weird backend logic on org creation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One potential solution - Galaxy creds must belong to an Org (not a team or user). I think that's pretty reasonable, because there's no context you'd use them other than via their ordered assignment to an Organization (which is something an Org Admin and higher would have exclusively have access to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach I like (which I've pushed to this PR) is to just introduce the concept of Credential.managed_by_tower
, just like we did with CredentialType.managed_by_tower
, and to have one global, non-editable Credential that represents galaxy.ansible.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you create a new organization after this migration has finished? What credentials does it have associated, and how does it go about doing Galaxy commands before the user has associated any credentials?
Could all new organizations start with a default credential in their Galaxy credential list? The default credential could point to galaxy.ansible.com
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively (& I think this might be preferable to what I mentioned above):
We could address this by showing a clear error / warning message if the user tries to run a JT that needs to fetch content without having configured a usable content credential that the job can use. There's a variety of places we could surface such a message (e.g job details page).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could all new organizations start with a default credential in their Galaxy credential list? The default credential could point to galaxy.ansible.com.
What I've discussed with @wenottingham @AlanCoding and @mabashian is just this - we're going to make it so that the Org Creation form pre-fills with "Ansible Galaxy".
credential_type=galaxy_type, | ||
inputs=inputs | ||
) | ||
cred.save() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to have a lot of RBAC questions that stem from this. Before we even get into any feature-specific discussions, does this correctly populate the credential's role fields and their parents/ancestors? I don't trust that it does in migrations.
You made the organization of the credential the same as the organization it gets associated. Do you intend to enforce this for new credentials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good questions - this is an area where our usage of a credential to represent "pull from galaxy.ansible.com" here is a little weird. It doesn't really make sense to create one of these credentials outside of the context of its association with at least one Organization (i.e.., there's no reason for somebody lower than Org Admin to make one of these, because they can't actually assign it to an Org).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this correctly populate the credential's role fields and their parents/ancestors?
It seems like it does?
Out[8]: 'Ansible Galaxy'
In [9]: Credential.objects.first().inputs
Out[9]: {'url': 'https://galaxy.ansible.com/'}
In [10]: Credential.objects.first().admin_role.ancestors.all()
Out[10]: <QuerySet [<Role: Admin-36>, <Role: Admin-2>, <Role: Credential Admin-6>, <Role: System Administrator-1>]>
In [12]: Credential.objects.first().admin_role.parents.all()
Out[12]: <QuerySet [<Role: Credential Admin-6>, <Role: System Administrator-1>]>
`/api/v2/organizations/N/galaxy_credentials/`. Authentication | ||
via an API token is optional (i.e., https://galaxy.ansible.com/), but other | ||
content sources (such as Red Hat Ansible Automation Hub) require proper | ||
configuration of the Auth URL and Token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think onprem hub can do token only w/o auth URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is cloud.redhat.com is token + Auth URL (basically, a keycloak server), and that "on premise Automation Hub" is a URL and a token. In any event, the permutation of how you specify all of this is different for each implementation, which is why everything but the "URL" is optional.
cc @chrismeyersfsu @rebeccahhh do I have this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds right to me, on prem Automation Hub should be an API token and URL.
Also yes, it is true that each implementation is different and we do need those other fields to be optional.
Build succeeded.
|
settings.FALLBACK_GALAXY_SERVERS = [] | ||
assert org.galaxy_credentials.count() == 3 | ||
creds = org.galaxy_credentials.all() | ||
assert creds[0].name == 'Private Galaxy (https://example.org/)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @AlanCoding @wenottingham @rebeccahhh @chrismeyersfsu
Highest precedence is "private Hub" aka PRIMARY_GALAXY_XXXXXX
...followed by anything in FALLBACK_GALAXY_SERVERS
...followed by galaxy.ansible.com iff PUBLIC_GALAXY_ENABLED
is not explicitly set to False.
Yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we're using the terminology of FALLBACK
but other than that yes, that all looks correct to me.
(and by not sure, I literally just don't know, it probably is 🤷♀️)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated not actually migrating these FALLBACK_GALAXY_SERVERS
at all, because I'm not confident anybody actually uses them (they're not exposed in the API or documented anywhere that I can tell).
Build succeeded.
|
doing this in the migration *before* any Organizations actually exist is stirring up RBAC dragons that I don't have time to fight this commit meanst that *new* installs will pre-create the default Galaxy (public) credential in create_preload_data, while *upgraded/migrations* installs will do so via the migration
Add the new credential type to the Credential page object and helper methods to manage the galaxy credentials for the Organization page object.
163480a
to
f4af74d
Compare
Build succeeded.
|
org is None | ||
): | ||
raise serializers.ValidationError(_( | ||
"Galaxy credentials must be owned by an Organization." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the org is deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to muck with the credential organization set null behavior
awx/awx/main/models/credential/__init__.py
Lines 103 to 109 in f4af74d
organization = models.ForeignKey( | |
'Organization', | |
null=True, | |
default=None, | |
blank=True, | |
on_delete=models.CASCADE, | |
related_name='credentials', |
So it would leave it null. I'm not too terribly bothered by that. As long as an org admin can still remove it, I'm okay with it.
(note there are 2 orgs in play here, the one who owns the credential and the one using the credential, I am talking about the ownership org being deleted)
list(owner_fields) != ['organization'] | ||
): | ||
raise serializers.ValidationError({"organization": _( | ||
"Galaxy credentials must be owned by an Organization." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. @AlanCoding wants to do away with team ownership and this aligns.
@@ -42,6 +42,16 @@ def handle(self, *args, **kwargs): | |||
}, | |||
created_by=superuser) | |||
c.admin_role.members.add(superuser) | |||
public_galaxy_credential = Credential( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on my cell phone. I assume this only runs on fresh installs and the migration will create this otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues: Add/ Edit Credential form does not list Organization as a required field when creating or editing an 'Ansible Galaxy/Automation Hub API Token' credential Tests: |
ManagedCredentialType( | ||
namespace='galaxy_api_token', | ||
kind='galaxy', | ||
name=ugettext_noop('Ansible Galaxy/Automation Hub API Token'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These highly descriptive names are kind of messy from the user's perspective using the name to reference the object, from the CLI or the collection like
tower_credential:
credential_type: "Ansible Galaxy/Automation Hub API Token"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but no change here makes a lot of sense. We might want to add an issue to reference credential types by namespace
in the collection modules.
@@ -2674,6 +2694,14 @@ def validate(self, attrs): | |||
if attrs.get('team'): | |||
attrs['organization'] = attrs['team'].organization | |||
|
|||
if ( | |||
attrs['credential_type'].kind == 'galaxy' and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if attrs['credential_type']
is null or the key is not present?
'type': 'string', | ||
'secret': True, | ||
'help_text': ugettext_noop( | ||
'A token to use for authentication against the Galaxy instance.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get legalistic with this - the token could be used for either Galaxy or Automation Hub, but the auth_url would only be used with Automation Hub.
'roles_enabled': settings.AWX_ROLES_ENABLED, | ||
'collections_enabled': settings.AWX_COLLECTIONS_ENABLED, | ||
'roles_enabled': galaxy_creds_are_defined and settings.AWX_ROLES_ENABLED, | ||
'collections_enabled': galaxy_creds_are_defined and settings.AWX_COLLECTIONS_ENABLED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would predict users having trouble debugging this, and I would like to put a docs note for it.
@tvo318 just to throw out some initial statement and wording:
If an organization does not have any Galaxy credentials associated with it, it will not download roles or collections from requirements files. On creation of a new organization:
- the UI will automatically associate the public Galaxy credential (for now, may change in later releases)
- organizations created via the API need to have the public Galaxy credential added in order to make use of roles/collection requirements files.
@john-westcott-iv recently added some tests to help keep the AWX collection up-to-date with the API, but for now related lists are out of scope of those checks (it is hard to determine what the editable relationships are) Given the default API behavior, I do see a lot of value in making the corresponding collection update. We would be adding the galaxy_credentials relationship as a list field. This can be modeled off a field like Additionally, since the public Galaxy credential is statically named, I think we should add that to the examples for Spitball: - name: Create tower organization
tower_organization:
name: "My New Organization"
description: "This has public galaxy enabled and will install roles and collections"
galaxy_credentials:
- Ansible Galaxy |
Build succeeded (gate pipeline).
|
see: #7813
cc @cutwater @bmbouter