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

Rename KMS modules #1284

Merged

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Jun 29, 2022

SUMMARY

In line with the naming guidelines, rename aws_kms and aws_kms_info

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/aws_kms.py
plugins/modules/aws_kms_info.py
plugins/modules/kms_key.py
plugins/modules/kms_key_info.py

ADDITIONAL INFORMATION

@github-actions
Copy link

github-actions bot commented Jun 29, 2022

Docs Build 📝

Thank you for contribution!✨

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

@ansibullbot
Copy link

@ansibullbot ansibullbot added community_review docs feature This issue/PR relates to a feature request integration tests/integration module module needs_triage new_module New module new_plugin New plugin plugins plugin (any type) tests tests labels Jun 29, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 5m 13s (non-voting)
✔️ build-ansible-collection SUCCESS in 4m 53s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 57s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 50s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 39s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 39s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 7m 43s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 24s
✔️ ansible-test-splitter SUCCESS in 2m 30s
✔️ integration-community.aws-1 SUCCESS in 11m 53s
⚠️ integration-community.aws-2 SKIPPED
⚠️ 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

Copy link
Contributor

@jatorcasso jatorcasso left a comment

Choose a reason for hiding this comment

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

Does it make sense to refactor the integration tests with the new module name? Otherwise we'll have to revisit when they all start breaking

@tremble
Copy link
Contributor Author

tremble commented Jun 29, 2022

I was going to follow up with the integration tests separately. Leaving them with the old name initially demonstrates that all of the aliasing pieces work.

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

Build failed (gate pipeline). For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

✔️ ansible-galaxy-importer SUCCESS in 4m 36s (non-voting)
✔️ build-ansible-collection SUCCESS in 4m 47s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 28s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 11m 37s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 35s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 10m 47s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 06s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 5m 44s
✔️ ansible-test-splitter SUCCESS in 2m 25s
integration-community.aws-1 FAILURE in 17m 58s
⚠️ integration-community.aws-2 SKIPPED
⚠️ 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
Copy link
Contributor Author

tremble commented Jun 29, 2022

regate - KMS tests are known to be a little flakey

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 3m 54s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 24s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 04s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 42s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 04s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 9m 38s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 43s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 20s
✔️ ansible-test-splitter SUCCESS in 2m 31s
✔️ integration-community.aws-1 SUCCESS in 11m 39s
⚠️ integration-community.aws-2 SKIPPED
⚠️ 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 644225c into ansible-collections:main Jun 29, 2022
@tremble tremble deleted the renames/kms branch July 7, 2022 19:23
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
Rename KMS modules

SUMMARY
In line with the naming guidelines, rename aws_kms and aws_kms_info
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/modules/aws_kms.py
plugins/modules/aws_kms_info.py
plugins/modules/kms_key.py
plugins/modules/kms_key_info.py
ADDITIONAL INFORMATION

Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Alina Buzachis <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@644225c
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 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  ansible-collections#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 ansible-collections#1284
However, this PR does not target ansible-collections#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 ansible-collections#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review docs feature This issue/PR relates to a feature request integration tests/integration mergeit Merge the PR (SoftwareFactory) module module needs_triage new_module New module new_plugin New plugin plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants