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

Configure default user/password through conf.d #346

Merged
merged 4 commits into from
Sep 22, 2020
Merged

Conversation

coro
Copy link
Contributor

@coro coro commented Sep 16, 2020

Summary Of Changes

This commit stops the operator configuring the default username and
password for the cluster through environment variables. Instead, these
credentials are configured through /etc/rabbitmq/conf.d/. This is for
two reasons:

  • RabbitMQ is moving away from configuration via env vars
  • The operator should not be coupled to functionality in the entrypoint script in the docker-library/rabbitmq Docker image, as this prevents other RabbitMQ images being used

This effectively removes support for RabbitMQ images pre 3.8.4, where
the ability to configure RabbitMQ via sysctl-style *.conf files in
the conf.d directory.

See #340

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Local Testing

Unit, Integration & System tests passed. Also tested on kind with rabbitmqctl authenticate_user.

Please ensure you run the unit, integration and system tests before approving the PR.

To run the unit and integration tests:

$ make unit-tests integration-tests

You will need to target a k8s cluster and have the operator deployed for running the system tests.

For example, for a Kubernetes context named dev-bunny:

$ kubectx dev-bunny
$ make destroy deploy-dev
# wait for operator to be deployed
$ make system-tests

This commit stops the operator configuring the default username and
password for the cluster through environment variables. Instead, these
credentials are configured through `/etc/rabbitmq/conf.d/`. This is for
two reasons:

* RabbitMQ is moving away from configuration via env vars
* The operator should not be coupled to functionality in the entrypoint script in the docker-library/rabbitmq Docker image, as this prevents other RabbitMQ images being used

This effectively removes support for RabbitMQ images pre 3.8.4, where
the ability to configure RabbitMQ via sysctl-style `*.conf` files in
the conf.d directory.
@coro coro requested a review from Zerpet September 16, 2020 16:27
rabbitmq-admin is the temporary volume for the initContainer, and
contains the actual secret data.

rabbitmq-confd is the empty volume that is populated by the
initContainer from rabbitmq-admin.
@michaelklishin
Copy link
Contributor

To clarify: env variables in RabbitMQ won't be removed but we certainly do not recommend them when other options are viable. The docs have been slowly updated to downplay env variables as a way to configure things that can be configured using rabbitmq.conf. The same strategy has been agreed to by the Docker image maintainers but, of course, it will take significantly longer for the image because of how heavy many images rely on env variables.

Copy link
Member

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

I would like to see the improvement around projecting the Secret key, instead of copying using the init-container. The refactoring in the tests look good and the changes in the secret resource builder look good.

This removes the logic for copying over from the initContainer, which
makes this author a very happy bunny.
@coro
Copy link
Contributor Author

coro commented Sep 18, 2020

FWIW, I checked the other elements in the initContainer, to see if we could mount them with the correct owner/permissions using projectedVolumes like we did here. Unfortunately it's not suitable for anything that needs to be more than read-only, such as any of our config files. Once kubernetes/kubernetes#81089 is addressed, then it should be possible to mount these correctly without needing an initContainer.

Copy link
Member

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

The latest changes are looking good. I've noticed that we are testing the new behaviour of controllers/rabbitmqcluster_controller_test.go in the "Override" section. Functionally this will work, however it doesn't seem to be the right place, as this new behaviour is not specific to the override feature. I think this would fit better in, perhaps, the "it works" section, although that might require some refactor to that test case.

@Zerpet Zerpet self-assigned this Sep 22, 2020
Copy link
Member

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

I (re)reviewed the code and now I see that we have to add an extra case for the rabbitmq-confd volume in the override case because we exact-match the elements in the Volume array 🙂

The feature is tested at unit level in the resource builder, and the integration of the resource builders with the reconcile loop is already tested at integration level. I think this is sufficiently tested and ready to merge ✅

@Zerpet Zerpet merged commit 6a8a87e into main Sep 22, 2020
@Zerpet Zerpet deleted the 340-default-user-pass branch September 22, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants