Optionally get service account file path from env var #54407

AlanCoding · 2019-03-26T15:45:17Z

SUMMARY

Addresses #54406

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

gcp_compute inventory plugin

ADDITIONAL INFORMATION

Tested with

GCE_CREDENTIALS_FILE_PATH="/Users/alancoding/Documents/repos/ansible-inventory-file-examples/private/gce/creds.json" ansible-inventory -i min.gcp_compute.yml --list --export

Where min.gcp_compute.yml has

auth_kind: serviceaccount
filters: null
strict: true
plugin: gcp_compute
projects:
- alans_project
zones:
  - us-east1-d

This will allow users to check their inventory config into source control.

ansibot · 2019-03-26T16:05:48Z

cc @erjohnso @rambleraptor
click here for bot help

bcoca · 2019-03-26T16:24:08Z

lib/ansible/plugins/inventory/gcp_compute.py

@@ -335,6 +338,12 @@ def parse(self, inventory, loader, path, cache=True):
        config_data = {}
        config_data = self._read_config_data(path)

+        # this may optionally be read from environment variable
+        if 'service_account_file' not in config_data:
+            path = self.get_option('service_account_file')


not sure how you expect get_option to work since it pulls from config_data?

Nothing in _read_config_data indicated to me that it reads from environment variables

https://github.com/ansible/ansible/blob/04ce72e80708bcc8139a9d7546aa088aefb0f492/lib/ansible/plugins/inventory/__init__.py#L206-L235

I tested this by putting breakpoints in, and confirmed, yes, that get_option works where the entry is not present in config_data. Not being able to explain it doesn't change the fact that I can confirm that it works this way.

Even the syntax in the BaseInventoryPlugin class seems to confirm that this syntax pattern is common:

if 'cache' in self._options and self.get_option('cache'):

I don't know if this is for the exact same reason, but maybe.

Now, one thing I didn't check, but maybe I should, is to compare self._options versus config_data in the locals here.

config_data is not supposed to read env vars, get_option does, you should not access _options directly

bcoca

so there is a basic problem with this plugin, it is doing it's own validation with incorrect assumptions

I would remove the existing validations like if 'zone' in config_data and just add required: true to the config definition.

bcoca · 2019-03-26T16:59:29Z

lib/ansible/plugins/inventory/gcp_compute.py

@@ -335,6 +338,12 @@ def parse(self, inventory, loader, path, cache=True):
        config_data = {}
        config_data = self._read_config_data(path)

+        # this may optionally be read from environment variable
+        if 'service_account_file' not in config_data:


this conditional is not needed, just use get_option since if read_config_data already consumes the options in the config file

AlanCoding · 2019-03-26T17:55:29Z

It actually wasn't obvious to me from reading the source code that zones should be required. So I just had to try it without giving zones and see, it gave:

[WARNING]: * Failed to parse /Users/alancoding/Documents/repos/ansible-inventory-file-examples/private/gce/min.gcp_compute.yml with auto plugin: [{u'domain': u'global',
u'message': u"Invalid value for field 'zone': 'a'. Unknown zone.", u'reason': u'invalid'}]

So zones should probably be required.

tested the changes for zones, works if I give zones, if I don't give zones:

[WARNING]: * Failed to parse /Users/alancoding/Documents/repos/ansible-inventory-file-examples/private/gce/min.gcp_compute.yml with auto plugin: No setting was provided for
required configuration plugin_type: inventory plugin: gcp_compute setting: zones

It is obvious from the prior code that projects should be required, so I made that required in the docs.

I already dealt with filters in a prior issue, but this allows it to be handled in the standard way.

bcoca · 2019-03-26T18:27:43Z

lib/ansible/plugins/inventory/gcp_compute.py

-                raise AnsibleParserError("Zones must be a list in GCP inventory YAML files")
+        zones = self.get_option('zones')
+        config_data['zones'] = zones
+        if not isinstance(zones, list):


also not needed, add type: list to configuration definition, nothing should be added back to config_data

also not needed, add type: list to configuration definition

I can do that

nothing should be added back to config_data

I can't do that. The entire solution here pivots on putting "service_account_file" in config data, because I don't know how it is used for the request auth. I would be guessing at what you want this to look like. I need more detail.

nothing should use config_data, its only meant for 'extras not covered by get_option', but that is a preexisting problem here

AlanCoding · 2019-03-26T18:47:06Z

I tried this:

diff --git a/lib/ansible/plugins/inventory/gcp_compute.py b/lib/ansible/plugins/inventory/gcp_compute.py
index 361f56233e..e6b1208b2f 100644
--- a/lib/ansible/plugins/inventory/gcp_compute.py
+++ b/lib/ansible/plugins/inventory/gcp_compute.py
@@ -26,9 +26,11 @@ DOCUMENTATION = '''
           description: A list of regions in which to describe GCE instances.
           default: all zones available to a given project
           required: True
+          type: list
         projects:
           description: A list of projects in which to describe GCE instances.
           required: True
+          type: list
         filters:
           description: >
             A list of filter value pairs. Available filters are listed here
@@ -344,20 +346,13 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable):
 
         # get user specifications
         zones = self.get_option('zones')
-        config_data['zones'] = zones
-        if not isinstance(zones, list):
-            raise AnsibleParserError("Zones must be a list in GCP inventory YAML files")
-
-        config_data['filters'] = self.get_option('filters')
-
-        service_account_path = self.get_option('service_account_file')
-        if service_account_path:
-            config_data['service_account_file'] = service_account_path
-
         projects = self.get_option('projects')
+        print('projects')
+        print(projects)
+        config_data['zones'] = zones
         config_data['projects'] = projects
-        if not isinstance(projects, list):
-            raise AnsibleParserError("Projects must be a list in GCP inventory YAML files")
+        config_data['filters'] = self.get_option('filters')
+        config_data['service_account_file'] = self.get_option('service_account_file')

This isn't working, I can put in dictionary, and get_option returns a dictionary.

AlanCoding · 2019-03-27T18:04:31Z

rebased PR to use new standardized system

AlanCoding · 2019-03-28T15:51:10Z

Related #54532

AlanCoding · 2019-04-04T15:28:22Z

What's the status of this?

This consumes the change made in Ansible core ansible/ansible#54407 which is in Ansible 2.8, allowing the plugin injection logic to share the script logic and to be simplified

bcoca reviewed Mar 26, 2019

View reviewed changes

ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 26, 2019

bcoca requested changes Mar 26, 2019

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 26, 2019

bcoca requested changes Mar 26, 2019

View reviewed changes

optionally get service account file path from env var

1aa700d

AlanCoding force-pushed the gcp_env_var branch from 4453e32 to 1aa700d Compare March 27, 2019 16:58

ansibot added the small_patch label Mar 27, 2019

bcoca approved these changes Apr 4, 2019

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 4, 2019

bcoca merged commit aa3f010 into ansible:devel Apr 4, 2019

AlanCoding mentioned this pull request Apr 23, 2019

Simplify gce inventory plugin injector ansible/awx#3754

Merged

ansible locked and limited conversation to collaborators Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally get service account file path from env var #54407

Optionally get service account file path from env var #54407

AlanCoding commented Mar 26, 2019

ansibot commented Mar 26, 2019

bcoca Mar 26, 2019

AlanCoding Mar 26, 2019

bcoca Mar 26, 2019 •

edited

bcoca Mar 26, 2019

bcoca left a comment •

edited

bcoca Mar 26, 2019

AlanCoding commented Mar 26, 2019

bcoca Mar 26, 2019

AlanCoding Mar 26, 2019

bcoca Mar 26, 2019

AlanCoding commented Mar 26, 2019

AlanCoding commented Mar 27, 2019

AlanCoding commented Mar 28, 2019

AlanCoding commented Apr 4, 2019

Optionally get service account file path from env var #54407

Optionally get service account file path from env var #54407

Conversation

AlanCoding commented Mar 26, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Mar 26, 2019

bcoca Mar 26, 2019

Choose a reason for hiding this comment

AlanCoding Mar 26, 2019

Choose a reason for hiding this comment

bcoca Mar 26, 2019 • edited

Choose a reason for hiding this comment

bcoca Mar 26, 2019

Choose a reason for hiding this comment

bcoca left a comment • edited

Choose a reason for hiding this comment

bcoca Mar 26, 2019

Choose a reason for hiding this comment

AlanCoding commented Mar 26, 2019

bcoca Mar 26, 2019

Choose a reason for hiding this comment

AlanCoding Mar 26, 2019

Choose a reason for hiding this comment

bcoca Mar 26, 2019

Choose a reason for hiding this comment

AlanCoding commented Mar 26, 2019

AlanCoding commented Mar 27, 2019

AlanCoding commented Mar 28, 2019

AlanCoding commented Apr 4, 2019

bcoca Mar 26, 2019 •

edited

bcoca left a comment •

edited