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

elbv2 - Fix load balancer listener comparison #2377

Conversation

ichekaldin
Copy link
Contributor

SUMMARY

Fixes #2376.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

elbv2
elb_application_lb

ADDITIONAL INFORMATION

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

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

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:

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:

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:

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:

Copy link
Contributor

@mandar242 mandar242 left a comment

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
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

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.

@abikouo abikouo added the mergeit Merge the PR (SoftwareFactory) label Nov 29, 2024
Copy link
Contributor

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/e52323dc38e74dbf841fc648f99f259f

✔️ ansible-galaxy-importer SUCCESS in 4m 19s
✔️ build-ansible-collection SUCCESS in 10m 22s
✔️ ansible-test-splitter SUCCESS in 4m 23s
✔️ integration-amazon.aws-1 SUCCESS in 12m 23s
✔️ integration-community.aws-1 SUCCESS in 16m 56s
Skipped 42 jobs

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit bb3914a into ansible-collections:main Nov 29, 2024
45 checks passed
Copy link

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@ichekaldin ichekaldin deleted the elb_application_lb-fix-listener-comparison branch December 1, 2024 04:48
Copy link

patchback bot commented Dec 2, 2024

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/bb3914afb8e125c896ac6e8e3ff8d025604b9c78/pr-2377

Backported as #2403

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Dec 2, 2024
SUMMARY
Fixes #2376.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
elbv2
elb_application_lb
ADDITIONAL INFORMATION

Reviewed-by: Mandar Kulkarni <[email protected]>
Reviewed-by: Bikouo Aubin
(cherry picked from commit bb3914a)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Dec 3, 2024
This is a backport of PR #2377 as merged into main (bb3914a).
SUMMARY
Fixes #2376.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
elbv2
elb_application_lb
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 mergeit Merge the PR (SoftwareFactory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

elb_application_lb - Module fails if listener default action is anything other than Forward
5 participants