Fix SASL/SCRAM + add option for SASL IAM & add option to disable unauthenticated clients #1764
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
|
@eRadical I see you fighting with the formatting/darker stuff If you install "darker" ( https://pypi.org/project/darker/ ) it should be possible to just run the darker command in the |
There was a problem hiding this comment.
Many thanks for this.
Down at line 695 there's a "module_args" parameter, this will also need to be updated, the DOCUMENTATION fragment is only documentation (upstream Ansible is aware that this isn't the greatest, but changing this for modules is non-trivial)
Does it make sense for all of these new options to have default values? Will they conflict with each other? it might make more sense for
msk_cluster:
...
authentication: {}
...
To be the way to disable authentication, rather than an explicitly parameter (especially one with a default value.
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 41s |
You need to explicitly disable "Unauthenticated access". When I created a cluster w/ Ansible it created only w/ "Unauthenticated" checked. So I suspect the boto default is actually "Unauthenticated". It looks like this: As you can see there are 4 independed checkboxes. In AWS console "IAM role-based authentication" is checked by default. |
|
The API ref. is not clearer either: https://docs.aws.amazon.com/MSK/2.0/APIReference/resources.html :( |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 40s |
For us it is the default then. Otherwise it would break backwards compatibility. |
is fixed and committed.
diff --git a/plugins/modules/msk_cluster.py b/plugins/modules/msk_cluster.py
index 31e2fe6a..46681d95 100644
--- a/plugins/modules/msk_cluster.py
+++ b/plugins/modules/msk_cluster.py
@@ -126,12 +126,10 @@ options:
sasl_iam:
description: IAM authentication is enabled or not.
type: bool
- default: False
version_added: 5.5.0
unauthenticated:
description: Option to explicitly turn on or off authentication
type: bool
- default: True
enhanced_monitoring:
description: Specifies the level of monitoring for the MSK cluster.
choices:
sanity is surviving here locally. As mentioned above, we must keep backwards compatibility. So the module must result in the same settings, when those parameters are not set on execution. |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 49s |
|
Can I do default & required? Note in |
|
Nvm, I just did a push, let's see how it goes! |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 44s |
|
recheck |
ansibullbot
added
bug
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 11s |
|
hm, maybe the entire msk test takes to long now. Any ideas @tremble? |
|
Yes, it might be. As they are now: tests are starting and stopping 4 clusters:
For me it takes about 1h to do a cluster change (switch unauthenticated to off or add sasl_iam). |
|
@eRadical this PR contains the following merge commits: Please rebase your branch to remove these commits. |
tremble
force-pushed
the
msk_cluster-authentication
branch
from
57da2b2 to
6d4c805
Compare
|
recheck |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 43s |
|
Build failed. ❌ ansible-galaxy-importer FAILURE in 4m 02s |
Unclear for me if just the integration test has a bug or if this is a side effect by splitting the test. |
|
I'll test tomorrow in the office as I also need to start the prod clusters. |
The actual failure is: The cluster is then being left in the "deleting" state, and when it's retried (with added verbosity) it fails because the deletion hadn't completed. |
tremble
removed
merge_commit
|
@eRadical it looks like the failing test was caused by a combination of 2 things:
|
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 19s |
|
regate |
|
Build succeeded (gate pipeline). ✔️ ansible-galaxy-importer SUCCESS in 3m 45s |
Backport to stable-5: 💚 backport PR created✅ Backport PR branch: Backported as #1780 🤖 @patchback |
…thenticated clients (#1764) Fix SASL/SCRAM + add option for SASL IAM & add option to disable unauthenticated clients SUMMARY fix SASL/SCRAM - Fixes #1761 add IAM authentication add option to disable unauthenticated clients Many thanx to @markuman for throwing me into this issue. ISSUE TYPE Bugfix Pull Request COMPONENT NAME msk_cluster ADDITIONAL INFORMATION I will probably add more tests after working w/ this. Reviewed-by: Mark Chappell Reviewed-by: Gabriel PREDA Reviewed-by: Markus Bergholz <[email protected]> (cherry picked from commit 2fe39ba)
|
Thanks for your work on this. Your work should be available when we release 5.5.0. This is expected within the next couple of weeks (along with 6.0.0) |
…thenticated clients (#1764) (#1780) [PR #1764/2fe39baa backport][stable-5] Fix SASL/SCRAM + add option for SASL IAM & add option to disable unauthenticated clients This is a backport of PR #1764 as merged into main (2fe39ba). SUMMARY fix SASL/SCRAM - Fixes #1761 add IAM authentication add option to disable unauthenticated clients Many thanx to @markuman for throwing me into this issue. ISSUE TYPE Bugfix Pull Request COMPONENT NAME msk_cluster ADDITIONAL INFORMATION I will probably add more tests after working w/ this. Reviewed-by: Mark Chappell
|
@tremble - It was my pleasure! ...and (I think) also the normal path after working & promoting Ansible for a long time. I just hope it's just the beginning. |
…ns#1764) Copy 2.16 ignore file to 2.17 (route53 'get' mode) SUMMARY 2.16 has branched, devel is now 2.17 and needs its own ignore file. ISSUE TYPE Tests Pull Request COMPONENT NAME tests/sanity/ignore-2.17.txt ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis
SUMMARY
Many thanx to @markuman for throwing me into this issue.
ISSUE TYPE
COMPONENT NAME
msk_cluster
ADDITIONAL INFORMATION
I will probably add more tests after working w/ this.