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

Added new module avi_user.py #57116

Open
wants to merge 4 commits into
base: devel
from

Conversation

@gitshrikant
Copy link

commented May 29, 2019

SUMMARY
  • Added new module
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • network/avi/avi_user.py
ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@gitshrikant, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in #ansible-network on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@chaitanyaavi @ericsysmin @grastogi23 @khaltore @vivobg

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@chaitanyaavi
Copy link
Contributor

left a comment

+shipit

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

test/units/modules/network/avi/test_avi_user.py:6:0: useless-import-alias Import alias does not rename original package
test/units/modules/network/avi/test_avi_user.py:71:53: bad-whitespace No space allowed around keyword argument assignment         avi_user.avi_ansible_api = Mock(return_value = data['mock_put_res'])                                                      ^
test/units/modules/network/avi/test_avi_user.py:103:52: bad-whitespace No space allowed after keyword argument assignment         avi_user.avi_ansible_api = Mock(return_value= data['mock_del_res'])                                                     ^
test/units/modules/network/avi/test_avi_user.py:107:0: missing-final-newline Final newline missing

The test ansible-test sanity --test pep8 [explain] failed with 13 errors:

test/units/modules/network/avi/test_avi_user.py:9:25: E225 missing whitespace around operator
test/units/modules/network/avi/test_avi_user.py:12:1: E302 expected 2 blank lines, found 1
test/units/modules/network/avi/test_avi_user.py:16:17: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:28:25: E231 missing whitespace after ':'
test/units/modules/network/avi/test_avi_user.py:39:13: E121 continuation line under-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:48:21: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:61:18: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:71:53: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:71:55: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:78:5: E303 too many blank lines (2)
test/units/modules/network/avi/test_avi_user.py:91:21: E231 missing whitespace after ':'
test/units/modules/network/avi/test_avi_user.py:103:54: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:107:35: W292 no newline at end of file

click here for bot help

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

test/units/modules/network/avi/test_avi_user.py:6:0: useless-import-alias Import alias does not rename original package
test/units/modules/network/avi/test_avi_user.py:70:53: bad-whitespace No space allowed around keyword argument assignment         avi_user.avi_ansible_api = Mock(return_value = data['mock_put_res'])                                                      ^
test/units/modules/network/avi/test_avi_user.py:102:52: bad-whitespace No space allowed after keyword argument assignment         avi_user.avi_ansible_api = Mock(return_value= data['mock_del_res'])                                                     ^
test/units/modules/network/avi/test_avi_user.py:106:0: missing-final-newline Final newline missing

The test ansible-test sanity --test pep8 [explain] failed with 13 errors:

test/units/modules/network/avi/test_avi_user.py:9:25: E225 missing whitespace around operator
test/units/modules/network/avi/test_avi_user.py:12:1: E302 expected 2 blank lines, found 1
test/units/modules/network/avi/test_avi_user.py:16:17: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:27:25: E231 missing whitespace after ':'
test/units/modules/network/avi/test_avi_user.py:38:13: E121 continuation line under-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:47:21: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:60:18: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:70:53: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:70:55: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:77:5: E303 too many blank lines (2)
test/units/modules/network/avi/test_avi_user.py:90:21: E231 missing whitespace after ':'
test/units/modules/network/avi/test_avi_user.py:102:54: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:106:35: W292 no newline at end of file

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

@Rohan-sss1 Rohan-sss1 force-pushed the avinetworks:avi-user-con-module branch from 972c06c to 9d5bd35 Jun 8, 2019

@ansibot ansibot removed the ci_verified label Jun 8, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

test/units/modules/network/avi/test_avi_user.py:6:0: useless-import-alias Import alias does not rename original package
test/units/modules/network/avi/test_avi_user.py:70:53: bad-whitespace No space allowed around keyword argument assignment         avi_user.avi_ansible_api = Mock(return_value = data['mock_put_res'])                                                      ^
test/units/modules/network/avi/test_avi_user.py:102:52: bad-whitespace No space allowed after keyword argument assignment         avi_user.avi_ansible_api = Mock(return_value= data['mock_del_res'])                                                     ^
test/units/modules/network/avi/test_avi_user.py:106:0: missing-final-newline Final newline missing

The test ansible-test sanity --test pep8 [explain] failed with 13 errors:

test/units/modules/network/avi/test_avi_user.py:9:25: E225 missing whitespace around operator
test/units/modules/network/avi/test_avi_user.py:12:1: E302 expected 2 blank lines, found 1
test/units/modules/network/avi/test_avi_user.py:16:17: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:27:25: E231 missing whitespace after ':'
test/units/modules/network/avi/test_avi_user.py:38:13: E121 continuation line under-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:47:21: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:60:18: E126 continuation line over-indented for hanging indent
test/units/modules/network/avi/test_avi_user.py:70:53: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:70:55: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:77:5: E303 too many blank lines (2)
test/units/modules/network/avi/test_avi_user.py:90:21: E231 missing whitespace after ':'
test/units/modules/network/avi/test_avi_user.py:102:54: E251 unexpected spaces around keyword / parameter equals
test/units/modules/network/avi/test_avi_user.py:106:35: W292 no newline at end of file

click here for bot help

@ansibot ansibot added the ci_verified label Jun 8, 2019

chaitanyaavi added some commits Jun 12, 2019

@Qalthos
Copy link
Contributor

left a comment

This module is not likely to be accepted in its current format. Specifically, from https://docs.ansible.com/ansible/latest/dev_guide/developing_modules.html:

  • Modules should not require that a user know all the underlying options of an API/tool to be used. For instance, if the legal values for a required module parameter cannot be documented, that’s a sign that the module would be rejected.
  • Modules should typically encompass much of the logic for interacting with a resource. A lightweight wrapper around an API that does not contain much logic would likely cause users to offload too much logic into a playbook, and for this reason the module would be rejected. Instead try creating multiple modules for interacting with smaller individual pieces of the API.

A lot of these modules bend those rules pretty far, but directly using API paths as parameters is a pretty clear violation of the first, and the organization of the arguments as a whole makes me suspect the second. Ansible modules should not be a 1-to-1 mapping of an API, but a coherent set of tools to accomplish specific tasks that may happen to use an API.

- The state that should be applied on the entity.
default: present
choices: ["absent", "present"]
name:

This comment has been minimized.

Copy link
@Qalthos

Qalthos Jun 12, 2019

Contributor

Should have reqiured: true, along with obj_username and obj_password according to the module argspec

@grastogi23

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

This module is not likely to be accepted in its current format. Specifically, from https://docs.ansible.com/ansible/latest/dev_guide/developing_modules.html:

  • Modules should not require that a user know all the underlying options of an API/tool to be used. For instance, if the legal values for a required module parameter cannot be documented, that’s a sign that the module would be rejected.
  • Modules should typically encompass much of the logic for interacting with a resource. A lightweight wrapper around an API that does not contain much logic would likely cause users to offload too much logic into a playbook, and for this reason the module would be rejected. Instead try creating multiple modules for interacting with smaller individual pieces of the API.

A lot of these modules bend those rules pretty far, but directly using API paths as parameters is a pretty clear violation of the first, and the organization of the arguments as a whole makes me suspect the second. Ansible modules should not be a 1-to-1 mapping of an API, but a coherent set of tools to accomplish specific tasks that may happen to use an API.

This module is not likely to be accepted in its current format. Specifically, from https://docs.ansible.com/ansible/latest/dev_guide/developing_modules.html:

  • Modules should not require that a user know all the underlying options of an API/tool to be used. For instance, if the legal values for a required module parameter cannot be documented, that’s a sign that the module would be rejected.

As such this module does not violate that. The legal values are indeed documented here. In addition, ansible module exists in a context of a solution or a product. So, user cannot just use an ansible module without knowing the context of what it is.

  • Modules should typically encompass much of the logic for interacting with a resource. A lightweight wrapper around an API that does not contain much logic would likely cause users to offload too much logic into a playbook, and for this reason the module would be rejected. Instead try creating multiple modules for interacting with smaller individual pieces of the API.

Whenever there has been a requirement of stitching together workflows the modules do that. In this case it is a first class REST resource and so it does warrant a proper ansible module. This allows customers to have proper modularity. Having a single module do multiple things is not so nice in the same way as single API doing multiple REST resources.

A lot of these modules bend those rules pretty far, but directly using API paths as parameters is a pretty clear violation of the first, and the organization of the arguments as a whole makes me suspect the second. Ansible modules should not be a 1-to-1 mapping of an API, but a coherent set of tools to accomplish specific tasks that may happen to use an API.

Actually, I would say anytime an ansible module reduces the scope of the operations that is possible via API then it is sub-optimal. Also, not having consistent parameter names are confusing for the user. Not to mention documentation nightmare. Eg. this is a module for Avi User and caller of this module needs to know what are these settings and how it maps to the settings on the product.

Many of our customers have actually thanked us and highlighted that they can do every thing with our modules that they would be able to do via API or UI. That is very powerful feature that should be encouraged for module development.

@Qalthos

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

From today's IRC meeting:

<grastogi> is core of the debate about using syntax like "/api/tenant?name=admin"?
<Qalthos> grastogi: That is a lot of it
<grastogi> Yes, having that syntax has allowed us to properly check against the reference. In a way your argument is that name should be enough for matching the reference. However, having it this way allowed us to pass hints to the module about type of the reference. This we used to then create a generic implementation to match against what is returned from the Avi controller and not having to pass UUIDs in the playbooks.
<grastogi> yes, there is a path but that is mostly a reference and always in the format of /api/<obj_type>?name=<obj_name>
<Qalthos> Then surely the module could accept a name as a parameter and send the formatted path to the generic implementation
<Qalthos> or a dictionary of name and type if name alone is insufficient
<Qalthos> The user should not have to know or care how the paths are formatted, that is the point I am trying to make
<grastogi> yes, I completely agree with you. It was not our best option. We could not achieve the solution with just name. The other one is not as clean so didn't implement that. User does need to know the type of object as you can't put a name and not know whether it is a virtualservice or pool or tenant
<Qalthos> In that case, I would suggest adding suboptions to each of those parameters, which should allow you to be more explicit about what sorts of values are expected or appropriate

In short, a dictionary representing the name and type of each resource is more acceptable than an API path.

@grastogi23

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

From today's IRC meeting:

is core of the debate about using syntax like "/api/tenant?name=admin"?
grastogi: That is a lot of it
Yes, having that syntax has allowed us to properly check against the reference. In a way your argument is that name should be enough for matching the reference. However, having it this way allowed us to pass hints to the module about type of the reference. This we used to then create a generic implementation to match against what is returned from the Avi controller and not having to pass UUIDs in the playbooks.
yes, there is a path but that is mostly a reference and always in the format of /api/<obj_type>?name=<obj_name>
Then surely the module could accept a name as a parameter and send the formatted path to the generic implementation
or a dictionary of name and type if name alone is insufficient
The user should not have to know or care how the paths are formatted, that is the point I am trying to make
yes, I completely agree with you. It was not our best option. We could not achieve the solution with just name. The other one is not as clean so didn't implement that. User does need to know the type of object as you can't put a name and not know whether it is a virtualservice or pool or tenant
In that case, I would suggest adding suboptions to each of those parameters, which should allow you to be more explicit about what sorts of values are expected or appropriate

In short, a dictionary representing the name and type of each resource is more acceptable than an API path.

Hi @Qalthos,

I wasn't able to get the issue across properly. The Avi APIs need a reference to the object. The reference is a URL REST reference to the object eg. http://avi_endpoint/tenant/tenant-uuid.

The references are similar to the Amazon's ARN or google firewall's network parameter. Eg. ssl_certificate_id: "arn:aws:iam::123456789012:server-certificate/company/servercerts/ProdServerCert" from the Aws module.

However, we made it simpler for the user to give partial URL path rather than the whole reference URL. If you were to compare against aws or google we could have instead used tenant/admin instead of "/api/tenant?name=admin". We went with keeping semantics of API and Ansible same so users don't get confused.

As far as on AVI module side we cannot send a tenant name simply when expectation of API is to receive a full reference without having to re-write all of API code.

We would like you to reconsider in light of what this parameter actually means. It is not a name but a reference. Also, existing modules already work with if someone passes full reference url rather than the filter semantics.

- local_action:
    module: ec2_elb_lb
    name: "test-please-delete"
    state: present
    zones:
      - us-east-1a
      - us-east-1d
    listeners:
      - protocol: http # options are http, https, ssl, tcp
        load_balancer_port: 80
        instance_port: 80
        proxy_protocol: True
      - protocol: https
        load_balancer_port: 443
        instance_protocol: http # optional, defaults to value of protocol setting
        instance_port: 80
        # ssl certificate required for https or ssl
        ssl_certificate_id: "arn:aws:iam::123456789012:server-certificate/company/servercerts/ProdServerCert"

Here are more examples

ssl_certificate_id: "arn:aws:iam::123456789012:server-certificate/company/servercerts/ProdServerCert"

thanks,
Gaurav

@grastogi23

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

@gundalow requesting your comments as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.