Create os_keystone_endpoint.py #2994

mlabarre · 2016-09-21T10:28:48Z

ISSUE TYPE

New Module Pull Request
COMPONENT NAME
os_keystone_endpoint
ANSIBLE VERSION

ansible 2.2.0

SUMMARY

Create / modify and delete endpoints

mlabarre

Doc correction

gregdek · 2016-09-21T16:25:12Z

Thanks @mlabarre. To the current maintainers, @emonty, @Shrews, @juliakreger, @j2sol, @rcarrillocruz please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

omgjlk

Not blocking this, as I haven't read through the rest, but I'd rather get questions answered before spending more time reading the code.

omgjlk · 2016-09-21T19:10:40Z

cloud/openstack/os_keystone_endpoint.py

@@ -0,0 +1,334 @@
+#!/usr/bin/python
+
+# Copyright (c) 2016 Helicom


Can we get a contact information on this, in case of future relicensing requests?

omgjlk · 2016-09-21T19:18:58Z

cloud/openstack/os_keystone_endpoint.py

+        - for V2 version, only public_url, internal_url and admin_url
+          can be specified (not interface, neither url).
+        - for V3 version, only interface AND url can be specified
+          (no public_url, internal_url and admin_url).


Is it not possible to just take in URL and interface as module arguments, and then internally or through shade decide how to represent the data to the API? The input to the module shouldn't change. The admin still has to provide a URL and some indication of what type of URL it is. I don't think the arguments to the ansible module should change depending on API version.

Correct j2sol, this should be using ansibles mutually exclusive options. The options can be either public_url, admin_url, internal_url OR interface and url. Both of these types work with either v2.0 or v3. There is no version dependency on making them work.

mlabarre · 2016-09-21T19:57:35Z

Hello
Yesterday I've post a request for the same module having the following signature
`# Add endpoints for keystone/identity (with V3 API)

os_api_endpoint:
cloud: cloud_v3
auth:
token: "{{ admin_token }}"
endpoint: "{{ admin_url_v3 }}"
service_name: keystone
service_type: identity
endpoints:
- { interface: "public", url: "http://server:5000/v2.0", enabled: true, region: "myregion"}
- { interface: "internal", url: "http://server:5000/v2.0", enabled: true, region: "myregion" }
- { interface: "admin", url: "http://server:35357/v2.0", enabled: true, region: "myregion"}
  state: present
  `
  Module refused because I test the API version in code and this kind would be executed by Shade. For admin it was transparent.
  I modified the module to be more agnostic.
  Now, you ask if it's possible to transform publicurl, etc into interface and url. I'm a little lost...
  The module presented yesterday allowed an interface list that allowed in the case of a v2 treat the endpoint as a whole.
  I can redo a third module that accepts only one lot of attributes : interface / url / region [/ enabled] but we must allow me to work with versions in the module to treat all CRUD.
  Thanks

mlabarre · 2016-09-21T19:59:49Z

Format is better below:

Yesterday I've post a request for the same module having the following signature

# Add endpoints for keystone/identity (with V3 API)
- os_api_endpoint:
    cloud: cloud_v3
    auth:
      token: "{{ admin_token }}"
      endpoint: "{{ admin_url_v3 }}"
    service_name: keystone
    service_type: identity
    endpoints:
      - { interface: "public", url: "http://server:5000/v2.0", enabled: true, region: "myregion"}
      - { interface: "internal", url: "http://server:5000/v2.0", enabled: true, region: "myregion" }
      - { interface: "admin", url: "http://server:35357/v2.0", enabled: true, region: "myregion"}
    state: present

Module refused because I test the API version in code and this kind would be executed by Shade. For admin it was transparent.
I modified the module to be more agnostic.
Now, you ask if it's possible to transform publicurl, etc into interface and url. I'm a little lost...
The module presented yesterday allowed an interface list that allowed in the case of a v2 treat the endpoint as a whole.
I can redo a third module that accepts only one lot of attributes : interface / url / region [/ enabled] but we must allow me to work with versions in the module to treat all CRUD.
Thanks

omgjlk · 2016-09-21T20:08:51Z

I believe @SamYaple objected to the code inside the module that was doing v2/v3 detection and logic. He was not objecting to the module input signature, but I'll let him confirm this.

ghost · 2016-09-21T23:53:08Z

cloud/openstack/os_keystone_endpoint.py

+        - for V2 version, only public_url, internal_url and admin_url
+          can be specified (not interface, neither url).
+        - for V3 version, only interface AND url can be specified
+          (no public_url, internal_url and admin_url).


Correct j2sol, this should be using ansibles mutually exclusive options. The options can be either public_url, admin_url, internal_url OR interface and url. Both of these types work with either v2.0 or v3. There is no version dependency on making them work.

ghost · 2016-09-21T23:53:37Z

cloud/openstack/os_keystone_endpoint.py

+     required: true
+   interface:
+     description:
+        - Endpoint type (ie 'public', 'internal', 'admin') (v3 format).


all references to "v3 format" or "v2 format" should be removed

ghost · 2016-09-21T23:54:21Z

cloud/openstack/os_keystone_endpoint.py

+'''
+
+RETURN = '''
+#


commenting so its not lost, im sure you are just waiting, but this should be filled out before merge

ghost · 2016-09-21T23:55:53Z

cloud/openstack/os_keystone_endpoint.py

+
+def main():
+
+    global cloud, module


please do not use globals

ghost · 2016-09-21T23:57:26Z

cloud/openstack/os_keystone_endpoint.py

+    module = AnsibleModule(argument_spec,
+                           supports_check_mode=True,
+                           **module_kwargs)
+    # role grant/revoke API introduced in 1.11.0


this comment is not accurate, specifically it should be that update was added in 1.11.0 and v3 support was fully implemented. But i dont think the comment is needed at all

ghost · 2016-09-21T23:59:21Z

cloud/openstack/os_keystone_endpoint.py

+        interface = module.params.get('interface', None)
+        url = module.params.get('url', None)
+        region = module.params.get('region', None)
+        enabled = "True" == module.params.get('enabled', True)


the bool type if set correctly will prevent the need to do this

ghost · 2016-09-22T00:01:28Z

cloud/openstack/os_keystone_endpoint.py

+        current_endpoint = get_endpoint(service_id, interface)
+
+        if current_endpoint is None and state == 'absent':
+            module.exit_json(changed=False)


See the functions named '_system_state_change' and '_needs_update' in the other ansible shade modules and how those are structured. Please implement that here (rather than in the main() function)

ghost · 2016-09-22T00:08:49Z

cloud/openstack/os_keystone_endpoint.py

+                    else:
+                        # No update possible in V2.0. So we must delete
+                        # and recreate.
+                        cloud.delete_endpoint(current_endpoint.id)


This prevents the module from being idempotent. We cannot do this. Since this is a module that could only be used by cloud operators it is expected they should know if they are using v2.0 and v3, and the appropriate capabilities. Therefore, if shade returns and error letting them know the cloud doesnt support this feature, it should be passed to the user.

ghost · 2016-09-22T00:11:58Z

cloud/openstack/os_keystone_endpoint.py

+        if changed:
+            if state == "present":
+                if current_endpoint is None:
+                    cloud.create_endpoint(service_id,


due to the way shade works, it is safe to pass interface, url, public_url, admin_url, and internal_url for each request, as long as they are set to None. That just means the logic for mutually_exclusive bits must be handled appropriately in the ansible module and then we can safely pass them all through.

ghost · 2016-09-22T00:17:42Z

cloud/openstack/os_keystone_endpoint.py

+        if current_endpoint is None:
+            changed = True
+        else:
+            if url and (enabled is not None


this logic is not correct since you can pass interface+url to a v2.0 endpoint through shade (shade handles that on the backed). Therefore it is not safe to assume just because the url is set this will be a v2.0 endpoint like you appear to be

mlabarre · 2016-09-22T18:05:07Z

ok
Now a simple question.
Shade accept all arguments for an endpoint creation : interface, url, publicurl and so.
If you supply publicurl, internalurl, adminrul in V2 with state=present, you create it
If you supply publicurl, internalurl, adminrul in V3 with state=present, you create it
If you supply publicurl, internalurl, adminrul in V2 with state=absent, you delete it
If you supply publicurl, internalurl, adminrul in V3 with state=absent, you do nothing because search filters does not returns endpoints.
In this logic/semantics, the module will have to interpret nothing and on the other he has to.
Permit all arguments on creation is very confuse...
Another thing. What that really represents "absent" relating, ie, internalurl ? the presence of internalurl in endpoint or its value ? Great discussion which my customer !

ghost · 2016-09-22T18:10:58Z

Deletes are done with the endpoint id for a reason. You must supply an id to shade to delete an endpoint.

v2 create with the *_url variables returns a list with a single endpoint
v3 create with the *_url variables returns a list with three endpoints

v2 delete is the same as v3, the difference in the ansible module would be it would delete 3 endpoints on v3 and only a single endpoint on v2.

As with all the other modules, if an endpoint search reveals multiple matches, we must abort and tell the user to fix that.

mlabarre · 2016-09-22T18:46:55Z

"v2 delete is the same as v3, the difference in the ansible module would be it would delete 3 endpoints on v3 and only a single endpoint on v2."

Absolutely no. in V3 if when you supply an interface with state = "absent" you would delete only this interface, not the endpoint. And you have not necessary 3 endpoints in V3.
And to do that :
. you indicate interface (search returns empty with V2)
. you indicate ie publicurl (search returns empty with V3)
So in the module, when i have a state "absent" all is relative to endpoint(s) found by the search request which use input arguments.
So, for me, search must be done on service id only. So we have 0 to 3 entries in list.
1 entry does not say 'V2' since I can have ie simply public url on my V3 endpoint.
So I must analyze the shade return and scan input arguments (interface/url which does not say necessary 'V3' and public/admin/internal url which does not say necessary 'V2') to try a correspondance.
Ouah !
I use identity v2 and v3 and I want only one version of endpoint module !

ghost · 2016-09-22T20:47:13Z

The return value from shade being normalized for v2.0 and v3 endpoints will be happening, though that doesn't directly influence the flow of this module.

When you specify v2.0 interface+url and set state to absent, then absolutely it will work the same for v2 and v3. With v2.0, a single endpoint has public, admin, and internal_url. But it doesn't have a hard requirement on having all of those set (infact, the only requirement is having public_url set in v2.0). Therefore there will not be a valid match and no change will occur on v2.0 endpoint.

All of this logic is currently baked into shade. It would simply be a matter of properly searching in the ansible module. Remember, the only way to delete is to pass an endpoint id. Searching will be the responsibility of the module.

I have been unfortunately busy in the past few weeks so I have no submitted this PR myself, but if you would like me to submit a PR for this module to show you the logic I can.

mlabarre · 2016-09-23T00:59:41Z

As I said before the creation of the endpoint is not a problem (shade
determines itself what it needs), the most important action is the
search for the endpoint on which we must act to modify or delete it .
When the interface / url arguments are provided, we found (or not) the
endpoint with ServiceId + interface (v3)

Lorsque les arguments publicurl, etc. sont fournis (totalement ou
partiellement) soit ils concernent un endpoint v2 dans tel cas on ne
peut le retrouver qu'avec le serviceid (search avec publicurl ne renvoit
rien). S'ils concernent un v3 (à mon sens c'est une mauvaise
codification) on peut le retrouver en ajoutant au filtre l'interface
avec 'public' comme valeur (par exemple) si l'argument est public_url.

When publicurl arguments, etc. are provided (all or part thereof) :
. if they relate to a v2 endpoint (in this case we can not find them
with the ServiceId (search publicurl returns with nothing).
. if they concern an v3 (I think it is a bad coding) can be found by
adding the filter interface with 'public' as a value (for example) if
the argument is public_url.

Now suppose a E1 endpoint created in v3:
-service: S1 (in the test it is the equivalent of the serviceId =
Service name + type)
-public = P1
-internal = I1

and E2 endpoint created in v2:
-service: S2 (in the test it is the equivalent of the serviceId =
Service name + type)
-public = P2
-internal = I2

execute with v2 API:

test 1.1

os_keystone_api
Service: S1
public_url: X
internal_url: I1
state: present

search with filter = serviceId. It returns one endpoint with
publicurl, InternalURL -> X = P1 -> changed -> update: Shade causes an
exception

test 1.2

os_keystone_api
Service: S1
interface: admin
url: A1
state: present

search with filter = serviceId. It returns one endpoint with
publicurl, InternalURL -> changed -> update: Shade causes an exception

test 1.3

os_keystone_api
Service: S1
public_url: X
internal_url: I1
state: absent

search with filter = serviceId. It returns one endpoint with
publicurl, InternalURL -> Delete while X = P1?

test 1.4

os_keystone_api
Service: S1
interface: internal
url: I1
state: absent

search with filter = serviceId. It returns one endpoint with
publicurl, InternalURL -> Delete?

test 1.5

os_keystone_api
Service: S2
public_url: X
internal_url: I2
state: present

search with filter = serviceId. It returns one endpoint with
publicurl, InternalURL -> X = P1 -> changed -> update: Shade causes an
exception

test 1.6

os_keystone_api
Service: S2
public_url: X
internal_url: I2
state: absent

search with filter = serviceId. It returns only one endpoint with
publicurl, InternalURL but the id of the endpoint is the public_url ->
Delete while X = P2?
If we remove, we remove all interfaces.

It runs with API v3:

2.1 test

os_keystone_api
Service: S1
public_url: X
internal_url: I1
state: present

search with filter = serviceId. It returns 2 endpoints with
interface / url -> X = P1 -> changed -> update: public update ok

2.2 test

os_keystone_api
Service: S1
interface: admin
url: A1
state: present

search with filter = serviceId + interface. It returns 1 endpoints
with interface / url -> changed -> update: add admin ok

2.3 test

os_keystone_api
Service: S1
public_url = X
internal_url = I1
state: absent

search with filter = serviceId. It returns 2 endpoints with
interface / url -> Delete public_url while X = P1? Delete internal_url ok.

test 2.4

os_keystone_api
Service: S1
interface: internal
url: I1
state: absent

search with filter = serviceId + interface. It returns with one
endpoint interface / url -> delete internal ok.

test 2.5

os_keystone_api
Service: S2
public_url = X
internal_url I2 =
state: present

search with filter = serviceId. It returns 2 endpoints with
interface / url -> X = P1 -> changed -> update: public url ok

test 2.6

os_keystone_api
Service: S2
public_url = X
internal_url I2 =
state: absent

search with filter = serviceId. It returns 2 endpoints with
internal / url -> X = P2 -> delete public? delete internal ok.

With the good responses, module would be ok.

Le 22/09/2016 à 22:47, Sam Yaple a écrit :

The return value from shade being normalized for v2.0 and v3 endpoints
will be happening, though that doesn't directly influence the flow of
this module.

When you specify v2.0 interface+url and set state to absent, then
absolutely it will work the same for v2 and v3. With v2.0, a single
endpoint has public, admin, and internal_url. But it doesn't have a
hard requirement on having all of those set (infact, the only
requirement is having public_url set in v2.0). Therefore there will
not be a valid match and no change will occur on v2.0 endpoint.

All of this logic is currently baked into shade. It would simply be a
matter of properly searching in the ansible module. Remember, the only
way to delete is to pass an endpoint.

I have been unfortunately busy in the past few weeks so I have no
submitted this PR myself, but if you would like me to submit a PR for
this module to show you the logic I can.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2994 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIBVXaYwLkr2YF8t9myEsXV6ZOu00D0Dks5qsulVgaJpZM4KCpCG.

ghost · 2016-09-23T15:26:21Z

Keystone v2.0 endpoints cannot be updated. So if a playbook is run that tries to update v2.0 endpoints, it should fail. I don't agree with deleting and recreating the endpoints for v2.0 (nor do we do that in other modules with a similar problem).

In your v2.0 examples, you have several where because it returns a single endpoint, you use it outright, but you should not be. If the endpoint doesn't match what you have, then you should not be using it. In this case, you should be ensuring that the public + admin + internal url all match what the ansible module was given. In the case of passing interface + url to v2.0 api, there is only one time that that will ever match with ansible, and that is when passing the public interface option since the public interface is required for v2.0. It should not be matching internal or admin url individually.

emonty

I agree with @SamYaple in his reviews.

emonty · 2016-10-04T16:38:09Z

cloud/openstack/os_keystone_endpoint.py

+
+DOCUMENTATION = '''
+---
+module: os_api_endpoint


os_keystone_endpoint

ansibot · 2016-12-07T14:14:08Z

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

ansibot · 2017-04-11T23:07:20Z

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

mlabarre added 2 commits September 21, 2016 12:26

Create os_keystone_endpoint.py

86af524

Update os_keystone_endpoint.py

704eb18

mlabarre commented Sep 21, 2016

View reviewed changes

gregdek added cloud new_plugin community_review labels Sep 21, 2016

omgjlk reviewed Sep 21, 2016

View reviewed changes

ghost suggested changes Sep 22, 2016

View reviewed changes

emonty suggested changes Oct 4, 2016

View reviewed changes

cloud/openstack/os_keystone_endpoint.py

DOCUMENTATION = '''

---

module: os_api_endpoint

Copy link

Contributor

emonty Oct 4, 2016

Choose a reason for hiding this comment

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

os_keystone_endpoint

ansibot closed this Apr 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create os_keystone_endpoint.py #2994

Create os_keystone_endpoint.py #2994

mlabarre commented Sep 21, 2016

mlabarre left a comment

gregdek commented Sep 21, 2016

omgjlk left a comment

omgjlk Sep 21, 2016

omgjlk Sep 21, 2016

ghost Sep 21, 2016

mlabarre commented Sep 21, 2016

mlabarre commented Sep 21, 2016

omgjlk commented Sep 21, 2016

ghost Sep 21, 2016

ghost Sep 21, 2016

ghost Sep 21, 2016

ghost Sep 21, 2016

ghost Sep 21, 2016

ghost Sep 21, 2016

ghost Sep 22, 2016 •

edited by ghost

ghost Sep 22, 2016

ghost Sep 22, 2016

ghost Sep 22, 2016

mlabarre commented Sep 22, 2016

ghost commented Sep 22, 2016

mlabarre commented Sep 22, 2016

ghost commented Sep 22, 2016 •

edited by ghost

mlabarre commented Sep 23, 2016

ghost commented Sep 23, 2016

emonty left a comment

emonty Oct 4, 2016

ansibot commented Dec 7, 2016

ansibot commented Apr 11, 2017

		@@ -0,0 +1,334 @@
		#!/usr/bin/python

		# Copyright (c) 2016 Helicom

Create os_keystone_endpoint.py #2994

Create os_keystone_endpoint.py #2994

Conversation

mlabarre commented Sep 21, 2016

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

mlabarre left a comment

Choose a reason for hiding this comment

gregdek commented Sep 21, 2016

omgjlk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlabarre commented Sep 21, 2016

mlabarre commented Sep 21, 2016

omgjlk commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 22, 2016 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlabarre commented Sep 22, 2016

ghost commented Sep 22, 2016

mlabarre commented Sep 22, 2016

ghost commented Sep 22, 2016 • edited by ghost

mlabarre commented Sep 23, 2016

ghost commented Sep 23, 2016

emonty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Dec 7, 2016

ansibot commented Apr 11, 2017

ghost Sep 22, 2016 •

edited by ghost

ghost commented Sep 22, 2016 •

edited by ghost