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

More work on ELBv2 module_utils #940

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Jul 21, 2022

SUMMARY
  • Refactors LB creation and makes sure that ip_address_type is set on creation (bug found when working on fixing NLB tests)
  • Fixes bug with DefaultAction comparisons
  • Extends tests for _prune_ForwardConfig
  • Extends tests for _prune_secrets
  • Fixes KeyError bug uncovered when extending tests for _prune_secrets
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/module_utils/elbv2.py

ADDITIONAL INFORMATION

Fixes: ansible-collections/community.aws#604
See also: ansible-collections/community.aws#1365

@tremble tremble marked this pull request as ready for review July 21, 2022 16:10
@softwarefactory-project-zuul

This comment was marked as outdated.

@tremble
Copy link
Contributor Author

tremble commented Jul 21, 2022

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

@ansibullbot
Copy link

@ansibullbot ansibullbot added the integration tests/integration label Jul 21, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 14s
✔️ build-ansible-collection SUCCESS in 4m 54s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 11m 19s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 8m 37s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 49s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 7m 34s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 7m 07s
✔️ ansible-test-splitter SUCCESS in 3m 33s
✔️ integration-amazon.aws-1 SUCCESS in 18m 19s
✔️ integration-amazon.aws-2 SUCCESS in 14m 31s
✔️ integration-amazon.aws-3 SUCCESS in 27m 45s
✔️ integration-amazon.aws-4 SUCCESS in 41m 09s
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
✔️ integration-community.aws-1 SUCCESS in 8m 33s
✔️ integration-community.aws-2 SUCCESS in 33m 06s
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@tremble tremble added the mergeit Merge the PR (SoftwareFactory) label Jul 22, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 4m 23s
✔️ build-ansible-collection SUCCESS in 4m 53s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 12m 42s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 53s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 55s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 7m 41s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 49s
✔️ ansible-test-splitter SUCCESS in 2m 42s
✔️ integration-amazon.aws-1 SUCCESS in 17m 04s
✔️ integration-amazon.aws-2 SUCCESS in 14m 56s
✔️ integration-amazon.aws-3 SUCCESS in 19m 45s
✔️ integration-amazon.aws-4 SUCCESS in 38m 23s
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
✔️ integration-community.aws-1 SUCCESS in 7m 49s
✔️ integration-community.aws-2 SUCCESS in 31m 03s
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 9e182cf into ansible-collections:main Jul 22, 2022
softwarefactory-project-zuul bot pushed a commit to ansible-collections/community.aws that referenced this pull request Jul 22, 2022
Re-enable integration tests for elb_network_lb

Depends-On: ansible-collections/amazon.aws#940
SUMMARY

Re-enables integration tests for elb_network_lb
Moves from hard-coded SSL certs to generating them on the fly
Fixes bug where ip_address_type in return value wasn't updated

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
tests/integration/targets/elb_network_lb
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
@tremble tremble deleted the issues/community/604 branch September 9, 2022 08:52
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 23, 2023
elbv2: respect UseExistingClientSecret

SUMMARY
Since amazon.aws 5.0.0, elb_application_lb runs into an exception, when using Type: authenticate-oidc in a rule, even when UseExistingClientSecret: True parameter is given. That works as expected with amazon.aws 4.x.x.
The logic gets broken in  #940
Basically AWS won't return both, UseExistingClientSecret and  ClientSecret.
But when requesting against boto3,  both parameters are mutually exclusive!
When the user set UseExistingClientSecret: True, the ClientSecret must be removed for the request.
When the user does not set UseExistingClientSecret or set it to False,  the UseExistingClientSecret must be included in the request.
While diving deeper, I've noticed a basic change detection problem for default values, that are not requested, but AWS will return them. I've summerized it in #1284
However, this PR does not target #1284, it just fixes the exception and restores the functionality and hotfix the change-detection only for Type: authenticate-oidc.
origin PR description

The error was: botocore.errorfactory.InvalidLoadBalancerActionException: An error occurred (InvalidLoadBalancerAction) when calling the ModifyRule operation: You must either specify a client secret or set UseExistingClientSecret to true

UseExistingClientSecret is not respected anymore since a.a 5
Introduced in #940
Furthermore, AWS returns also Scope and  SessionTimeout parameters that are filled with default values if not requested.
'Scope': 'openid',
'SessionTimeout': 604800,

That make the module always returns a change, if they are not requested.
This fix does not break backwards compatibility, because the values are already set by aws, when not requested yet.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
plugins/module_utils/elbv2.yml
ADDITIONAL INFORMATION


          - Conditions:
              - Field: host-header
                Values:
                  - some.tld
              - Field: path-pattern
                Values:
                  - "/admin/*"
            Actions:
              - Type: authenticate-oidc
                Order: 1
                AuthenticateOidcConfig:
                  Issuer: https://login.microsoftonline.com/32rw-ewad53te-ef/v2.0
                  AuthorizationEndpoint: https://login.microsoftonline.com/324re-dafs6-6tw/oauth2/v2.0/authorize
                  TokenEndpoint: https://login.microsoftonline.com/432535ez-rfes-32543ter/oauth2/v2.0/token
                  UserInfoEndpoint: https://graph.microsoft.com/oidc/userinfo
                  ClientId: fasgd-235463-fsgd-243
                  ClientSecret: "{{ lookup('onepassword', 'some cool secret', vault='some important vault') }}"
                  SessionCookieName: AWSELBAuthSessionCookie
                  OnUnauthenticatedRequest: authenticate
                  UseExistingClientSecret: True
              - TargetGroupName: "{{ some_tg }}"
                Type: forward
                Order: 2

Reviewed-by: Alina Buzachis
Reviewed-by: Mark Chappell
patchback bot pushed a commit that referenced this pull request Feb 23, 2023
elbv2: respect UseExistingClientSecret

SUMMARY
Since amazon.aws 5.0.0, elb_application_lb runs into an exception, when using Type: authenticate-oidc in a rule, even when UseExistingClientSecret: True parameter is given. That works as expected with amazon.aws 4.x.x.
The logic gets broken in  #940
Basically AWS won't return both, UseExistingClientSecret and  ClientSecret.
But when requesting against boto3,  both parameters are mutually exclusive!
When the user set UseExistingClientSecret: True, the ClientSecret must be removed for the request.
When the user does not set UseExistingClientSecret or set it to False,  the UseExistingClientSecret must be included in the request.
While diving deeper, I've noticed a basic change detection problem for default values, that are not requested, but AWS will return them. I've summerized it in #1284
However, this PR does not target #1284, it just fixes the exception and restores the functionality and hotfix the change-detection only for Type: authenticate-oidc.
origin PR description

The error was: botocore.errorfactory.InvalidLoadBalancerActionException: An error occurred (InvalidLoadBalancerAction) when calling the ModifyRule operation: You must either specify a client secret or set UseExistingClientSecret to true

UseExistingClientSecret is not respected anymore since a.a 5
Introduced in #940
Furthermore, AWS returns also Scope and  SessionTimeout parameters that are filled with default values if not requested.
'Scope': 'openid',
'SessionTimeout': 604800,

That make the module always returns a change, if they are not requested.
This fix does not break backwards compatibility, because the values are already set by aws, when not requested yet.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
plugins/module_utils/elbv2.yml
ADDITIONAL INFORMATION

          - Conditions:
              - Field: host-header
                Values:
                  - some.tld
              - Field: path-pattern
                Values:
                  - "/admin/*"
            Actions:
              - Type: authenticate-oidc
                Order: 1
                AuthenticateOidcConfig:
                  Issuer: https://login.microsoftonline.com/32rw-ewad53te-ef/v2.0
                  AuthorizationEndpoint: https://login.microsoftonline.com/324re-dafs6-6tw/oauth2/v2.0/authorize
                  TokenEndpoint: https://login.microsoftonline.com/432535ez-rfes-32543ter/oauth2/v2.0/token
                  UserInfoEndpoint: https://graph.microsoft.com/oidc/userinfo
                  ClientId: fasgd-235463-fsgd-243
                  ClientSecret: "{{ lookup('onepassword', 'some cool secret', vault='some important vault') }}"
                  SessionCookieName: AWSELBAuthSessionCookie
                  OnUnauthenticatedRequest: authenticate
                  UseExistingClientSecret: True
              - TargetGroupName: "{{ some_tg }}"
                Type: forward
                Order: 2

Reviewed-by: Alina Buzachis
Reviewed-by: Mark Chappell
(cherry picked from commit c6906a3)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 23, 2023
[PR #1270/c6906a3f backport][stable-5] elbv2: respect UseExistingClientSecret

This is a backport of PR #1270 as merged into main (c6906a3).
SUMMARY
Since amazon.aws 5.0.0, elb_application_lb runs into an exception, when using Type: authenticate-oidc in a rule, even when UseExistingClientSecret: True parameter is given. That works as expected with amazon.aws 4.x.x.
The logic gets broken in  #940
Basically AWS won't return both, UseExistingClientSecret and  ClientSecret.
But when requesting against boto3,  both parameters are mutually exclusive!
When the user set UseExistingClientSecret: True, the ClientSecret must be removed for the request.
When the user does not set UseExistingClientSecret or set it to False,  the UseExistingClientSecret must be included in the request.
While diving deeper, I've noticed a basic change detection problem for default values, that are not requested, but AWS will return them. I've summerized it in #1284
However, this PR does not target #1284, it just fixes the exception and restores the functionality and hotfix the change-detection only for Type: authenticate-oidc.
origin PR description

The error was: botocore.errorfactory.InvalidLoadBalancerActionException: An error occurred (InvalidLoadBalancerAction) when calling the ModifyRule operation: You must either specify a client secret or set UseExistingClientSecret to true

UseExistingClientSecret is not respected anymore since a.a 5
Introduced in #940
Furthermore, AWS returns also Scope and  SessionTimeout parameters that are filled with default values if not requested.
'Scope': 'openid',
'SessionTimeout': 604800,

That make the module always returns a change, if they are not requested.
This fix does not break backwards compatibility, because the values are already set by aws, when not requested yet.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME
plugins/module_utils/elbv2.yml
ADDITIONAL INFORMATION


          - Conditions:
              - Field: host-header
                Values:
                  - some.tld
              - Field: path-pattern
                Values:
                  - "/admin/*"
            Actions:
              - Type: authenticate-oidc
                Order: 1
                AuthenticateOidcConfig:
                  Issuer: https://login.microsoftonline.com/32rw-ewad53te-ef/v2.0
                  AuthorizationEndpoint: https://login.microsoftonline.com/324re-dafs6-6tw/oauth2/v2.0/authorize
                  TokenEndpoint: https://login.microsoftonline.com/432535ez-rfes-32543ter/oauth2/v2.0/token
                  UserInfoEndpoint: https://graph.microsoft.com/oidc/userinfo
                  ClientId: fasgd-235463-fsgd-243
                  ClientSecret: "{{ lookup('onepassword', 'some cool secret', vault='some important vault') }}"
                  SessionCookieName: AWSELBAuthSessionCookie
                  OnUnauthenticatedRequest: authenticate
                  UseExistingClientSecret: True
              - TargetGroupName: "{{ some_tg }}"
                Type: forward
                Order: 2

Reviewed-by: Mark Chappell
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…tions#940)

Refactor s3_bucket_notifications to support SNS / SQS

SUMMARY
Refactor s3_bucket_notifications to extend module to support the extra targets of SNS and SQS along with the currently supported Lambda functions.
Summary of changes:

Refactor module to support SNS/SQS targets along with current Lambda function support.
Fix check mode coverage
Update integration tests to more comprehensive cover functionality.
Update documentation in sns_topic and sqs_queue modules to add policy setting example.

Fixes: ansible-collections#140
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
s3_bucket_notifications
ADDITIONAL INFORMATION
https://boto3.amazonaws.com/v1/documentation/api/1.16.0/reference/services/s3.html#S3.Client.put_bucket_notification_configuration

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Mark Woolley <[email protected]>
Reviewed-by: Markus Bergholz <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
)

Re-enable integration tests for elb_network_lb

Depends-On: ansible-collections#940
SUMMARY

Re-enables integration tests for elb_network_lb
Moves from hard-coded SSL certs to generating them on the fly
Fixes bug where ip_address_type in return value wasn't updated

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
tests/integration/targets/elb_network_lb
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…tions#940)

Refactor s3_bucket_notifications to support SNS / SQS

SUMMARY
Refactor s3_bucket_notifications to extend module to support the extra targets of SNS and SQS along with the currently supported Lambda functions.
Summary of changes:

Refactor module to support SNS/SQS targets along with current Lambda function support.
Fix check mode coverage
Update integration tests to more comprehensive cover functionality.
Update documentation in sns_topic and sqs_queue modules to add policy setting example.

Fixes: ansible-collections#140
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
s3_bucket_notifications
ADDITIONAL INFORMATION
https://boto3.amazonaws.com/v1/documentation/api/1.16.0/reference/services/s3.html#S3.Client.put_bucket_notification_configuration

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Mark Woolley <[email protected]>
Reviewed-by: Markus Bergholz <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
)

Re-enable integration tests for elb_network_lb

Depends-On: ansible-collections#940
SUMMARY

Re-enables integration tests for elb_network_lb
Moves from hard-coded SSL certs to generating them on the fly
Fixes bug where ip_address_type in return value wasn't updated

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
tests/integration/targets/elb_network_lb
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…tions#940)

Refactor s3_bucket_notifications to support SNS / SQS

SUMMARY
Refactor s3_bucket_notifications to extend module to support the extra targets of SNS and SQS along with the currently supported Lambda functions.
Summary of changes:

Refactor module to support SNS/SQS targets along with current Lambda function support.
Fix check mode coverage
Update integration tests to more comprehensive cover functionality.
Update documentation in sns_topic and sqs_queue modules to add policy setting example.

Fixes: ansible-collections#140
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
s3_bucket_notifications
ADDITIONAL INFORMATION
https://boto3.amazonaws.com/v1/documentation/api/1.16.0/reference/services/s3.html#S3.Client.put_bucket_notification_configuration

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Mark Woolley <[email protected]>
Reviewed-by: Markus Bergholz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review has_issue integration tests/integration mergeit Merge the PR (SoftwareFactory) module_utils module_utils needs_triage plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

elb_network_lb always shown as changed
3 participants