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.

Sign up for GitHub

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

Jump to bottom

elbv2 - Fix load balancer listener comparison #2377

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

elbv2 - Fix load balancer listener comparison #2377

wants to merge 4 commits into from

Conversation

ichekaldin
Copy link
Contributor

SUMMARY

Fixes #2376.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

elbv2
elb_application_lb

ADDITIONAL INFORMATION

@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/63dcde26c8c94e308286bd31722c4d79

✔️ ansible-galaxy-importer SUCCESS in 5m 25s
✔️ build-ansible-collection SUCCESS in 10m 10s
✔️ ansible-test-splitter SUCCESS in 3m 55s
✔️ integration-amazon.aws-1 SUCCESS in 12m 55s
✔️ integration-community.aws-1 SUCCESS in 18m 49s
Skipped 42 jobs

mandar242
mandar242 reviewed Nov 12, 2024
View reviewed changes
tests/integration/targets/elb_application_lb/tasks/main.yml
MessageBody: Not available
StatusCode: "404"
register: alb
check_mode: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
check_mode: true
check_mode: true

mandar242
mandar242 reviewed Nov 12, 2024
View reviewed changes
tests/integration/targets/elb_application_lb/tasks/main.yml Outdated
StatusCode: "404"
register: alb
check_mode: true
- ansible.builtin.assert:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ansible.builtin.assert:
- name: Assert check_mode result
ansible.builtin.assert:

mandar242
mandar242 reviewed Nov 12, 2024
View reviewed changes
tests/integration/targets/elb_application_lb/tasks/main.yml Outdated
MessageBody: Not available
StatusCode: "404"
register: alb
- ansible.builtin.assert:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ansible.builtin.assert:
- name: Assert update ALB result
ansible.builtin.assert:

mandar242
mandar242 reviewed Nov 12, 2024
View reviewed changes
tests/integration/targets/elb_application_lb/tasks/main.yml Outdated
StatusCode: "404"
register: alb
check_mode: true
- ansible.builtin.assert:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ansible.builtin.assert:
- name: Assert check_mode idempotence result
ansible.builtin.assert:

mandar242
mandar242 reviewed Nov 12, 2024
View reviewed changes
tests/integration/targets/elb_application_lb/tasks/main.yml Outdated
MessageBody: Not available
StatusCode: "404"
register: alb
- ansible.builtin.assert:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ansible.builtin.assert:
- name: Assert idempotence result
ansible.builtin.assert:

mandar242
mandar242 approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@mandar242 mandar242 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall lgtm! Thanks for contribution @ichekaldin

@ichekaldin
Copy link
Contributor Author

Overall lgtm! Thanks for contribution @ichekaldin

Thank you! I added the suggested changes.

@ichekaldin ichekaldin force-pushed the elb_application_lb-fix-listener-comparison branch from 8351e26 to 9718600 Compare November 13, 2024 00:34
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/be67d7910874406d8f35d21bd4c26b30

✔️ ansible-galaxy-importer SUCCESS in 4m 08s
✔️ build-ansible-collection SUCCESS in 10m 01s
✔️ ansible-test-splitter SUCCESS in 4m 11s
✔️ integration-amazon.aws-1 SUCCESS in 9m 59s
✔️ integration-community.aws-1 SUCCESS in 18m 44s
Skipped 42 jobs

@fabricat-mdb
Copy link

Bug #2376 is blocking me.
How can we facilitate this PR?

@fabricat-mdb
Copy link

fabricat-mdb commented Nov 22, 2024 •
edited
Loading

The modified code is not working for me.
The partial stack trace of the module failure is:

  File "..../ansible_amazon.aws.elb_application_lb_payload.zip/ansible_collections/amazon/aws/plugins/modules/elb_application_lb.py", line 860, in create_or_update_alb
  File "..../ansible_amazon.aws.elb_application_lb_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/elbv2.py", line 933, in compare_listeners
  File "..../ansible_amazon.aws.elb_application_lb_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/elbv2.py", line 865, in _group_listeners
  File "..../ansible_amazon.aws.elb_application_lb_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/elbv2.py", line 807, in _compare_listener
  File "..../ansible_amazon.aws.elb_application_lb_payload.zip/ansible_collections/amazon/aws/plugins/module_utils/elbv2.py", line 163, in _sort_listener_actions
TypeError: '<' not supported between instances of 'NoneType' and 'dict'

I really need this bug to be fixed, so (even if I'm not a Python coder) I struggled a bit to debug this.
Maybe one of the problems is that AuthenticateOidcConfig, FixedResponseConfig, and RedirectConfig are dictionaries, while _sort_listener_actions signature expects strings (actions: List[Dict[str, str]]))?

As an alternative, is it possible to use _sort_actions instead of _sort_listener_actions?
It simply uses Order as the sorting key. I noticed that Order is always present in current_listener (and is required in module parameters) when the listener has more than one default action.
When there is only one default action, sorting is not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandar242 mandar242 mandar242 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

elb_application_lb - Module fails if listener default action is anything other than Forward
3 participants
@ichekaldin @fabricat-mdb @mandar242