-
Notifications
You must be signed in to change notification settings - Fork 307
Fix: consider additional config for default user creds #1974
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
base: main
Are you sure you want to change the base?
Fix: consider additional config for default user creds #1974
Conversation
|
Hold merge until we have produced 2.17.0 |
mkuratczyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks. My only worry is the compatibility of the ini format as used by RabbitMQ vs github.com/go-ini/ini/. If I'm not mistaken, with this change, we now require additionalConfig to pass go-ini validation, which was not the case before. I'm sure it it will work in most cases, but I don't know about the compatibility of ini formats with more complex content
|
That's an interesting point I hadn't consider 🤔 It's always possible to fallback to advanced config. Undesirable, but go-ini validation won't block any user completely. Happy to hold if you want to try to break this PR? |
7f6fd46 to
f450af0
Compare
|
Bump for reviews! @MirahImage @mkuratczyk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for reading default RabbitMQ user credentials from the spec.rabbitmq.additionalConfig field instead of always generating them randomly. When default_user and/or default_pass are specified in additionalConfig, those values are used in the default-user Secret. If not specified, credentials are randomly generated as before.
- Adds logic to parse
additionalConfigas INI format and extractdefault_useranddefault_pass - Implements fallback behavior to generate credentials when not found in config
- Adds unit tests and integration tests for the new functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/resource/default_user_secret.go | Implements credential extraction from additionalConfig with fallback to random generation, includes import formatting fixes |
| internal/resource/default_user_secret_test.go | Adds unit test for the new credential extraction behavior, includes import formatting fix |
| controllers/rabbitmqcluster_controller_test.go | Adds integration test verifying credentials from additionalConfig appear in the Secret |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The RabbitMQ operator now respects `default_user` and `default_pass` values provided in the `additionalConfig` field of the `RabbitmqCluster` manifest when creating the default user secret. Previously, the operator would generate random credentials, leading to mismatches and confusion. This change modifies the `Build` function in `internal/resource/default_user_secret.go` to parse the `additionalConfig` string. If `default_user` and `default_pass` are present, their values are used. Otherwise, random credentials are generated, maintaining backward compatibility. Unit tests in `internal/resource/default_user_secret_test.go` and integration tests in `controllers/rabbitmqcluster_controller_test.go` have been added to verify this behavior.
Because the behaviour has changed and this comment facilitates understanding better than scanning the code. Co-authored-by: Copilot <[email protected]>
dc85848 to
aaee7d6
Compare
This closes #1018
Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Summary Of Changes
The RabbitMQ operator now respects
default_useranddefault_passvalues provided in the
additionalConfigfield of theRabbitmqClustermanifest when creating the default user secret. Previously, the operator
would generate random credentials, leading to mismatches and confusion.
This change modifies the
Buildfunction ininternal/resource/default_user_secret.goto parse theadditionalConfigstring. If
default_useranddefault_passare present, their valuesare used. Otherwise, random credentials are generated, maintaining backward
compatibility.
Unit tests in
internal/resource/default_user_secret_test.goandintegration tests in
controllers/rabbitmqcluster_controller_test.gohavebeen added to verify this behavior.
Local Testing
Unit and integration tests passing.