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

Add support for application gateway path-based routing #452

Merged
merged 21 commits into from
Aug 6, 2021

Conversation

l3ender
Copy link
Contributor

@l3ender l3ender commented Mar 11, 2021

SUMMARY

This PR adds support for using path-based routing rules in an application gateway. This allows the request to be routed to different backend pools based on the URL path.

Fixes #448.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/azure_rm_appgateway.py

ADDITIONAL INFORMATION

This adds support for using rule_type: path_based_routing in a request routing rule. The routing rule references the URL path map resource for the gateway.

- name: Create instance of Application Gateway with path based rules
  azure_rm_appgateway:
    ...
    request_routing_rules:
      - rule_type: path_based_routing
        http_listener: sample_http_listener
        name: rule1
        url_path_map: path_mappings
    url_path_maps:
      - name: path_mappings
        default_backend_address_pool: test_backend_address_pool
        default_backend_http_settings: sample_appgateway_http_settings
        path_rules:
          - name: path_rules
            backend_address_pool: test_backend_address_pool
            backend_http_settings: sample_appgateway_http_settings
            paths:
              - "/abc"
              - "/123/*"

@Fred-sun
Copy link
Collaborator

@l3ender Thank you for your contribution. We will complete the review and push forward the merger as soon as possible! Thank you very much!

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Mar 18, 2021
@Fred-sun
Copy link
Collaborator

@l3ender Lines 820,821,830,831,936,837,877, 878 and 879 continuation line over indented for visual indent! Thank you very much!

@Fred-sun
Copy link
Collaborator

@l3ender There is something wrong with the newly added parameter in idempotency, could you please help to check it again? Thank you very much!

@l3ender
Copy link
Contributor Author

l3ender commented Mar 26, 2021

@Fred-sun I've corrected indentation and added a test case for idempotency. Can you please review? If you're seeing other issues with idempotency please share detail!

Thank you!

@Fred-sun
Copy link
Collaborator

@Fred-sun I've corrected indentation and added a test case for idempotency. Can you please review? If you're seeing other issues with idempotency please share detail!

Thank you!

@l3ender I have tested it and there is still something wrong with the idempotency of the parameter. Could you please try again?Also, when this parameter is added, the creation resource takes too long to use. Can you do something to optimize this? Thank you very much!

@l3ender
Copy link
Contributor Author

l3ender commented May 22, 2021

@Fred-sun I have corrected the idempotency issues you identified--thank you!

Regarding the time taken to create a resource with this new feature, I am seeing very similar timings comparing app gateways with routing of either type (basic or path_based_routing). I tested with SKU=standard_v2 and capacity=1, and the only difference in creation being the routing type.

request_routing_rules:
  - rule_type: Basic
    backend_address_pool: "webapp"
    backend_http_settings: "webapp-profile"
    http_listener: "inbound-https"
    name: rule1

vs

request_routing_rules:
  - name: "url-routing"
    rule_type: "path_based_routing"
    http_listener: "inbound-https"
    url_path_map: "url-routes"
url_path_maps:
  - name: "url-routes"
    default_backend_address_pool: "webapp"
    default_backend_http_settings: "webapp-profile"
    path_rules:
      - name: "webapp-traffic"
        backend_address_pool: "webapp"
        backend_http_settings: "webapp-profile"
        paths:
          - "/myapp/*"

I created 3 new resources of each type and added timing around the module's call to azure-mgmt-network. Results:

  • Basic: 383 seconds
  • Basic: 384 seconds
  • Basic: 384 seconds
  • Path-based: 383 seconds
  • Path-based: 383 seconds
  • Path-based: 384 seconds

Please let me know if there is anything else. Thank you for your patience with me while I complete this effort!

@Fred-sun
Copy link
Collaborator

Fred-sun commented Jun 22, 2021

@l3ender I have run the test case, but there are some problems. Can you help execute the test cases(ests/integration/targets/azure_rm_appgateway/tasks/main.yml) and provide the results? . Thank you very much!

@Fred-sun
Copy link
Collaborator

@l3ender Could you please help to authorize me to update this PR? Thank you very much!

@l3ender
Copy link
Contributor Author

l3ender commented Jun 22, 2021

@Fred-sun I've sent an invite so you have access to update PR. I'm still trying to figure out how to run test cases, can you please advise?

@Fred-sun
Copy link
Collaborator

@Fred-sun I've sent an invite so you have access to update PR. I'm still trying to figure out how to run test cases, can you please advise?

@l3ender You can try as following playbook ( include tests/integration/targets/azure_rm_appgateway/tasks/main.yml), Also copy the file under “tests/integration/targets/azure_rm_appgateway/files/cert*.txt” to the current path.

---
- name: Using Azure collection
  hosts: localhost
  connection: local
  collections:
    - azure.azcollection
  vars:
    resource_group: myresourcegroup
  tasks:
    - name: create resource gorup
      azure_rm_resourcegroup:
        name: "{{ resource_group }}"
        location: eastus
    - include: main.yml

@l3ender
Copy link
Contributor Author

l3ender commented Jun 24, 2021

@Fred-sun I have run the test using your playbook. Here is the recap:

PLAY RECAP PLAY RECAP *******************************************************************************************************
localhost                  : ok=26   changed=10   unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

And here is the full execution output log: test-results2.txt.

Thanks!

@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged and removed work in In trying to solve, or in working with contributors labels Jun 25, 2021
@l3ender
Copy link
Contributor Author

l3ender commented Jul 20, 2021

Hi @Fred-sun, any update on this PR?

@Fred-sun
Copy link
Collaborator

Fred-sun commented Aug 2, 2021

Hi @Fred-sun, any update on this PR?

@l3ender I will move forward with the merger as soon as possible. Thank you very much!

@xuzhang3 xuzhang3 merged commit b51429e into ansible-collections:dev Aug 6, 2021
@l3ender l3ender deleted the path-based-url-mapping branch August 6, 2021 15:34
Fred-sun added a commit to Fred-sun/ansible_collections_azure that referenced this pull request Aug 11, 2021
…ctions#452)

* support url path map for application gateway

* update documentation and add test coverage

* correct indentation

* test idempotency for app gw with path based rules

* correct merge

* correct idempotency for path_based_routing

* deep clone old/new props for accurate idempotency check

* simplify path_based_routing examples and tests

* prevent original parameter modification

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to create application gateway with path based routing
3 participants