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

Set defaults from params after loading files, allowing params to override #44142

Merged
merged 2 commits into from Sep 10, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 20 additions & 29 deletions lib/ansible/module_utils/k8s/common.py
Expand Up @@ -139,46 +139,37 @@ def get_api_client(self, **auth_params):
auth_params = auth_params or getattr(self, 'params', {})
auth = copy.deepcopy(auth_params)

configuration = kubernetes.client.Configuration()
# Pull authorization settings from environment variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment here should be more extensive and state precisely what order we're processing configuration.

for key, value in iteritems(auth_params):
if key in auth_args and value is not None:
if key == 'api_key':
setattr(configuration, key, {'authorization': "Bearer {0}".format(value)})
else:
setattr(configuration, key, value)
elif key in auth_args and value is None:
if key in auth_args and value is None:
env_value = os.getenv('K8S_AUTH_{0}'.format(key.upper()), None)
if env_value is not None:
if key == 'api_key':
setattr(configuration, key, {'authorization': "Bearer {0}".format(env_value)})
else:
setattr(configuration, key, env_value)
auth[key] = env_value

kubernetes.client.Configuration.set_default(configuration)
auth[key] = env_value

if auth.get('username') and auth.get('password') and auth.get('host'):
auth_method = 'params'
elif auth.get('api_key') and auth.get('host'):
auth_method = 'params'
if ((auth.get('username') and auth.get('password') and auth.get('host')) or
(auth.get('api_key') and auth.get('host'))):
pass
elif auth.get('kubeconfig') or auth.get('context'):
auth_method = 'file'
kubernetes.config.load_kube_config(auth.get('kubeconfig'), auth.get('context'))
else:
auth_method = 'default'

# First try to do incluster config, then kubeconfig
if auth_method == 'default':
# First try to do incluster config, then kubeconfig
try:
kubernetes.config.load_incluster_config()
return DynamicClient(kubernetes.client.ApiClient())
except kubernetes.config.ConfigException:
return DynamicClient(self.client_from_kubeconfig(auth.get('kubeconfig'), auth.get('context')))
kubernetes.config.load_kube_config(auth.get('kubeconfig'), auth.get('context'))

if auth_method == 'file':
return DynamicClient(self.client_from_kubeconfig(auth.get('kubeconfig'), auth.get('context')))

if auth_method == 'params':
return DynamicClient(kubernetes.client.ApiClient(configuration))
configuration = kubernetes.client.Configuration()
for key, value in iteritems(auth):
Copy link
Contributor

Choose a reason for hiding this comment

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

should be auth.items() for python 2 and 3 compatibility.

Edit: I see iteritems comes from six, I'm just not sure why it's needed when auth.items() will work fine

if key in auth_args and value is not None:
if key == 'api_key':
setattr(configuration, key, {'authorization': "Bearer {0}".format(value)})
else:
setattr(configuration, key, value)

kubernetes.client.Configuration.set_default(configuration)
return DynamicClient(kubernetes.client.ApiClient(configuration))


def client_from_kubeconfig(self, config_file, context):
Copy link
Contributor

Choose a reason for hiding this comment

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

After this refactor this method is no longer referenced, probably safe to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely

try:
Expand Down