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

New module for azure virtual network gateway #44411

Conversation

Madhura-CSI
Copy link
Contributor

SUMMARY

New module for azure virtual network gateway

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

azure_rm_virtualnetworkgateway

ANSIBLE VERSION
ansible 2.6.2
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/centos-dev/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Aug  4 2017, 00:39:18) [GCC 4.8.5 20150623 (Red Hat 4.8.5-16)]
ADDITIONAL INFORMATION

Currently we don't have any existing module to create/update/delete virtual network gateway(VPN gateway) in azure cloud. Hence, I would like to contribute this module to community.
Sample module call is as follows:

    - name: Create virtual network gateway without bgp settings
      azure_rm_virtualnetworkgateway:
        resource_group: testrg
        name: testvpngw
        ip_configurations:
          - name: testipconfig
            private_ip_allocation_method: Dynamic
            public_ip_address_name: testipaddr
        virtual_network: testvnet
        tags:
          common: "xyz"
      register: vgw_without_bgp

By default module waits for VPN gateway to be created.(using poller)

@Madhura-CSI
Copy link
Contributor Author

WIP

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.7 This issue/PR affects Ansible v2.7 azure cloud module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Aug 20, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 20, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:335:20: undefined-variable Undefined variable 'fh'

The test ansible-test sanity --test validate-modules [explain] failed with 9 errors:

lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E322 "active_active" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E322 "gateway_default_site" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E322 "vpn_client_configuration" is listed in the argument_spec, but not documented in the module
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E325 argument_spec for "active_active" defines type="bool" but documentation does not
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E325 argument_spec for "enable_bgp" defines type="bool" but documentation does not
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "gateway_type" does not match the documentation (['Vpn', 'ExpressRoute'])
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "sku" does not match the documentation (['VpnGw1', 'VpnGw2', 'VpnGw3'])
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "vpn_type" does not match the documentation (['RouteBased', 'PolicyBased'])
lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:167:21: E313 RETURN is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_virtualnetworkgateway.py:167:21: error RETURN: syntax error: expected ',' or '}', but got '<scalar>'

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Aug 20, 2018
@webknjaz webknjaz removed the needs_triage Needs a first human triage before being processed. label Aug 21, 2018
@Fred-sun
Copy link
Contributor

@Madhura-CSI Thanks for the contribution, Could you help confirm the PR ready for review or not? if yes, I will push to review! Thanks!

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 5, 2018
@Fred-sun
Copy link
Contributor

@Madhura-CSI Thanks for the contribution. Could you help finished this PR change? when you finished, I will push to review. Thanks!

@Fred-sun
Copy link
Contributor

Fred-sun commented Nov 1, 2018

@Madhura-CSI Thanks for your contribution, Are you still here?Could you help finish the PR changed? if you finished, I will push to review? Thanks!

@Madhura-CSI
Copy link
Contributor Author

Yes @Fred-sun , I am working on this PR. Will need few more days to finish the stuff. I will update here once this is ready for review.

@Fred-sun
Copy link
Contributor

Fred-sun commented Nov 5, 2018

@Madhura-CSI Thanks for your update, When you finished changed, I will test,. Thanks!

@Fred-sun
Copy link
Contributor

kindly ping

1 similar comment
@Fred-sun
Copy link
Contributor

kindly ping

@zikalino
Copy link
Contributor

@Madhura-CSI I would like to add a few things and also integration tests and merge this pr.
would it be ok for you to add me to your fork?
I would also do the same with local network gateway.

@Madhura-CSI
Copy link
Contributor Author

Sure @zikalino . Please go ahead and make the necessary changes as per your requirement.

@zikalino
Copy link
Contributor

@Madhura-CSI great! :-) could you add me as a collaborator to your fork?

@Madhura-CSI
Copy link
Contributor Author

@zikalino One problem I see here is: I think we need to add write access at repository/org level and not at the pull-request level. Can't really do that as of now. Do you see any probable alternative to this? I would be glad to assist.

@zikalino
Copy link
Contributor

@Madhura-CSI ok then. no problem. I will just fork your fork and create PR on the top of your branch.
a bit more complex, but should work.

@Madhura-CSI
Copy link
Contributor Author

@zikalino sure, thanks!

if not self.ip_configurations:
self.fail('Parameter error: ip_configurations required when creating a vpn gateway')
subnet = self.network_models.SubResource(
'/subscriptions/{0}/resourceGroups/{1}/providers/Microsoft.Network/virtualNetworks/{2}/subnets/GatewaySubnet'.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

@Madhura-CSI i have just started working on this pr, and I have a quick question.
why is GatewaySubned hardcoded here. shouldnt' we refer any subnet?
any special reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zikalino, 'GatewaySubnet' < this name is mandatory for the subnet where we deploy the virtual network gateway virtual machines. AFAIK, we cannot change this name. If we ever do that, network gateway creation will fail at the end. Please confirm this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Madhura-CSI thanks! I will create PR to your fork soon :-)

@Fred-sun
Copy link
Contributor

Fred-sun commented Mar 7, 2019

@Madhura-CSI Are you still here? Could you give a feedback for zikalino's comments? Thanks!

@Madhura-CSI
Copy link
Contributor Author

@Fred-sun replied to the code review comment above. Please check.

@Fred-sun
Copy link
Contributor

Fred-sun commented Mar 7, 2019

@Madhura-CSI Thanks for your update.
@zikalino Once you create PR , please paste link at here! Thanks!

@zikalino
Copy link
Contributor

@Madhura-CSI I have created a pr to your fork some time ago. could you take a look and merge it?

@Madhura-CSI
Copy link
Contributor Author

sure @zikalino . I will update this in a day or two and will let you know.

@zikalino
Copy link
Contributor

zikalino commented Apr 4, 2019

@Madhura-CSI could you try to merge changes today? the freeze is coming :-)
otherwise I may try to merge in a separate pr

@ansibot
Copy link
Contributor

ansibot commented Apr 8, 2019

@Madhura-CSI this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Apr 8, 2019

@Madhura-CSI This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibot ansibot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 8, 2019
@Madhura-CSI Madhura-CSI force-pushed the module-azure_rm_virtualnetworkgateway branch from 3ec43c2 to 980e67f Compare April 8, 2019 08:06
@ansibot ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Apr 8, 2019
@Madhura-CSI
Copy link
Contributor Author

@zikalino , sorry for the delay. I have now merged your branch with this one. Please review and proceed with upstream merge if everything looks good to you.
Thanks!

@Madhura-CSI Madhura-CSI changed the title [WIP] New module for azure virtual network gateway New module for azure virtual network gateway Apr 8, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 8, 2019

@brusMX @caohai @devigned @gustavomcarmo @haroldwongms @iphilpot @julienstroheker @lmazuel @obsoleted @sozercan @techknowlogick @trstringer @tstringer @xscript @yaweiw @yuwzho

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

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Apr 8, 2019
@nitzmahone nitzmahone merged commit fe31b7e into ansible:devel Apr 11, 2019
@mmyyrroonn
Copy link
Contributor

@Madhura-CSI Hello. Thanks for this great PR. Recently, we met a small issue, I just wonder, could you share your design purpose for the subnet option?

subnet:
description:
- ID of the gateway subnet for VPN.
default: GatewaySubnet

Based on the current implementation, it seems that this option is not used in the code now. I checked the sdk, the param of the same name is constructed by the information in the virtual network.
subnet = self.network_models.SubResource(
id='/subscriptions/{0}/resourceGroups/{1}/providers/Microsoft.Network/virtualNetworks/{2}/subnets/GatewaySubnet'.format(
self.virtual_network['subscription_id'],
self.virtual_network['resource_group'],
self.virtual_network['name']))

I think there should be some original reason for you to create this option. we can discuss about it and continue improving this module after our discussion.

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 azure cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants