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.

Sign up for GitHub

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

Update aap credential injectors #55

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from
Open

Update aap credential injectors #55

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6156e93
update aap credential injectors
sean-m-sullivan Nov 10, 2024
760e38a
update creds remove path prefix
sean-m-sullivan Dec 6, 2024
3231722
Update plugins.py
sean-m-sullivan Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/awx_plugins/credentials/plugins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,10 +688,30 @@
'type': 'boolean',
'secret': False,
},
{
'id': 'ah_path_prefix',
'label': gettext_noop('Verify SSL'),

Check warning on line 693 in src/awx_plugins/credentials/plugins.py

View check run for this annotation

Codecov / codecov/patch

src/awx_plugins/credentials/plugins.py#L693 

Added line #L693 was not covered by tests
'type': 'string',
'secret': False,
'default': 'galaxy',
'help_text': gettext_noop(

Check warning on line 697 in src/awx_plugins/credentials/plugins.py

View check run for this annotation

Codecov / codecov/patch

src/awx_plugins/credentials/plugins.py#L697 

Added line #L697 was not covered by tests
'API path used to access the Hub api. Either galaxy, '
'or custom. By default it should be galaxy',
Copy link
Member

Choose a reason for hiding this comment

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

Either galaxy or custom

I've never seen this. I can't find it anywhere in AWX. I would ask Hub engineers what the intent is... but this is the "controller" credential type. It doesn't add up to me.

Copy link
Contributor Author

@sean-m-sullivan sean-m-sullivan Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's https://10.242.42.153/api/galaxy/

I just checked, in 2.4 it was customizable in the installer through an obscure variable, with 2.5 it looks like it was hardcoded.

I've removed this entry.

),
},
],
'required': ['host'],
},
injectors={
'extra_vars': {
'aap_hostname': '{{host}}',
'aap_username': '{{password}}',
'aap_password': '{{password}}',
'aap_token': '{{oauth_token}}',
'aap_request_timeout': '{{request_timeout}}',
'aap_validate_certs': '{{verify_ssl}}',
'ah_path_prefix': '{{ah_path_prefix}}',
Copy link
Member

Choose a reason for hiding this comment

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

extra_vars never did anything. That's why they were never put in the injectors before. These appears to correspond to the module arguments, but that implies a syntax like aap_hostname: "{{ aah_hostname }}" used in your playbook. This is far from obvious, and I'm not convinced it's useful. It would be very undocumented. Very hard to document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were writing a blog post on it, but if we just document they are avail, or make them avail, people can use them in places to connect, or use for the lokup plugin or other uses. We shouldn't limite HOW they are used just make them available for the base credential type. I and probably everyone using the validated for a while have also been using custom, just trying to take the standard custom and bring it into the fold.

},
'env': {
'TOWER_HOST': '{{host}}',
'TOWER_USERNAME': '{{username}}',
Expand All @@ -703,6 +723,14 @@
'CONTROLLER_PASSWORD': '{{password}}',
'CONTROLLER_VERIFY_SSL': '{{verify_ssl}}',
'CONTROLLER_OAUTH_TOKEN': '{{oauth_token}}',
'CONTROLLER_REQUEST_TIMEOUT': '{{request_timeout}}',
'AAP_HOSTNAME': '{{host}}',
'AAP_USERNAME': '{{username}}',
'AAP_PASSWORD': '{{password}}',
'AAP_VALIDATE_CERTS': '{{verify_ssl}}',
'AAP_TOKEN': '{{oauth_token}}',
'AAP_REQUEST_TIMEOUT': '{{request_timeout}}',
Copy link
Member

Choose a reason for hiding this comment

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

These seem fine up to this point.

Copy link

@willtome willtome Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

will TOWER_HOST, CONTROLLER_HOST, and AAP_HOSTNAME ever need to be different?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is the point. Using CONTROLLER_HOST by itself is using direct API access to the controller host, meaning, you are not going through the proxy. That way of accessing controller is deprecated (exactly the meaning of that word, I'm not the one to ask), so we want to be moving away from that.

However, this change is allowing to use the address of the proxy host. I think @chrismeyersfsu might have expressed a sentiment that this should be a separate type, and there is some validity to that. Ultimately, if we want existing credentials to "just work", we may need some code-level solution to this so that it inspects the response headers or something to determine which it was given, because this is expected to affect the API path that has to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willtome You can point controller_host at the gateway and things work, I tested this all against a gateway 2.5 address like a customer should be doing, The variable itself doesn't care about proxy, endpoint, etc etc. From what I've tested awx.awx against a 2.5 AAP will not work as it does not do the api translation correctly, however ansible.controller collection does. Technically with upstream it could just be Device FQDN, but we have to call it something, and the user relies on the collection to know the difference and know better.

'GALAXY_API_PATH_PREFIX': '{{ah_path_prefix}}',
},
},
)
Expand Down