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

Allow application gateway probe to use host header from HTTP settings #450

Merged
merged 22 commits into from
Aug 10, 2021

Conversation

l3ender
Copy link
Contributor

@l3ender l3ender commented Mar 11, 2021

SUMMARY

This PR adds configuration for an application gateway's probe so that it can use the host header defined in the associated backend HTTP settings.

This is an existing configuration option for probes, it just wasn't exposed/available for usage.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_appgateway.py

ADDITIONAL INFORMATION

Without this setting, the hostname needs to be defined manually for the probe.

Before:

- name: "Create application gateway."
  azure_rm_appgateway:
    ...
    probes:
      - name: "myprobe"
        interval: 30
        timeout: 30
        path: "/abc"
        protocol: "https"
        host: "my-hostname"

After:

- name: "Create application gateway."
  azure_rm_appgateway:
    ...
    probes:
      - name: "myprobe"
        interval: 30
        timeout: 30
        path: "/abc"
        protocol: "https"
        pick_host_name_from_backend_http_settings: true

@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 Have you tested the newly added Parameter? Can you share your test results? Thank you very much!

@l3ender
Copy link
Contributor Author

l3ender commented Mar 26, 2021

@Fred-sun Yes, I have tested and here is a screenshot from the portal of the created application gateway probe:

image

I added a test case using the new probe property which is succeeded. However, there doesn't seem to be a way to assert the probe property on the updated object, as there is no state returned from azure_rm_appgateway module.

If there is other detail I can provide, please let me know. Thank you!

@Fred-sun
Copy link
Collaborator

@l3ender In addition, the parameter you newly added is not idempotent. Could you please help to check it again? Thank you very much!

@l3ender
Copy link
Contributor Author

l3ender commented Mar 31, 2021

In addition, the parameter you newly added is not idempotent.

@Fred-sun Could you share detail on how you are testing for idempotency, and what specifically is failing? This will help me ensure correctness and add test coverage.

@Fred-sun
Copy link
Collaborator

@l3ender In addition, the parameter you newly added is not idempotent. Could you please help to check it again? Thank you very much!

@l3ender
Copy link
Contributor Author

l3ender commented Apr 1, 2021

@Fred-sun Yes, can you please share detail on how you are testing for idempotency, and what specifically is failing?

@Fred-sun
Copy link
Collaborator

Fred-sun commented Apr 1, 2021

@l3ender Execute the PlayBook that created the resource again. Thank you very much!

@Fred-sun
Copy link
Collaborator

@Fred-sun Yes, can you please share detail on how you are testing for idempotency, and what specifically is failing?

Do you have re-execute the playbook? Any update for this?

@l3ender
Copy link
Contributor Author

l3ender commented Apr 27, 2021

Do you have re-execute the playbook? Any update for this?

My apologies for the delay! I will test, update, and let you know!

@l3ender
Copy link
Contributor Author

l3ender commented May 22, 2021

@Fred-sun I have corrected the idempotency issue and added a test case to ensure it! Please take another look and let me know if you have any other findings.

My sincere apologies for the long delay before finalizing the PR: I have had to be singularly dedicated to a critical issue at my organization which has taken all my time until now. Thank you for your patience!

@Fred-sun
Copy link
Collaborator

I have corrected the idempotency issue and added a test case to ensure it! Please take another look and let me know if you have any other findings.

My sincere apologies for the long delay before finalizing the PR: I have had to be singularly dedicated to a critical issue at my organization which has taken all my time until now. Thank you for your patience!

@l3ender Thank you for your update and contribution. I also made this PR for other things. I will complete the review as soon as possible! Thank you very much!

@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 ran the test using your playbook and it succeeded. Here is the recap:

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

Here is the full execution output: test-results.txt.

Let me know if I can provide any other info. Thank you!

@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
Copy link
Collaborator

LGTM

@xuzhang3 xuzhang3 self-requested a review August 10, 2021 02:29
@xuzhang3
Copy link
Collaborator

LGTM

@xuzhang3 xuzhang3 merged commit d4eb5bb into ansible-collections:dev Aug 10, 2021
Fred-sun added a commit to Fred-sun/ansible_collections_azure that referenced this pull request Aug 11, 2021
…ansible-collections#450)

* allow probe to use host header from http settings

* correct documentation verbiage

* add test coverage

* correct probe idempotency, add test coverage

* corrrect usage of using backend hostname in probe

* Add default value for pick_host_name_from_backend_http_settings

* correct assert verbiage

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Co-authored-by: Fred-sun <xiuxi.sun@qq.com>
Co-authored-by: xuzhang3 <57888764+xuzhang3@users.noreply.github.com>
@l3ender l3ender deleted the probe-hostname branch August 11, 2021 14:08
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.

None yet

3 participants