search

docker-library / rabbitmq

Docker Official Image packaging for RabbitMQ
http://www.rabbitmq.com/
MIT License
785 stars 417 forks source link

Cant use as service in gitlab ci since 3.9 #508

Closed kucix closed 3 years ago

kucix commented 3 years ago

In gitlab-ci.yml we use rabbitmq as service that is configured via ENV props - gitlab does not allow volumes in services.

Example is like this

Acceptance tests:
  image: php
  stage: test
  variables:
    RABBITMQ_ERLANG_COOKIE: xxx""
    RABBITMQ_DEFAULT_USER: "guest"
    RABBITMQ_DEFAULT_PASS: "guest"
    RABBITMQ_DEFAULT_VHOST: "/xxx"
  services:
    - name: rabbitmq:3-management
      alias: rabbitmq
  script:
    - composer tests-acceptance

https://docs.gitlab.com/ee/ci/services/#available-settings-for-services

Now we fixed this by changing to rabbitmq:3.8-management, but what can we do for the future? How can we configure default user/pass and default vhost?

Diggsey commented 3 years ago

I'm seeing a similar issue.

wglambert commented 3 years ago

That would be due to https://github.com/docker-library/rabbitmq/pull/467

Reasoning behind the change https://github.com/docker-library/rabbitmq/issues/506#issuecomment-887786516

kucix commented 3 years ago

Yes I know, I have read it, but how can we use version 3.9 and later in gitlab ci as a service and configure it? That is the question.

michaelklishin commented 3 years ago

This is a great question for the GitLab community. You have several options:

Setting RABBITMQ_DEFAULT_USER and RABBITMQ_DEFAULT_PASS to guest is unnecessary since those are seed user defaults.

michaelklishin commented 3 years ago

Alternatively you can use definition import, both at boot time and after the node has started (with an HTTP API call), to set up any users, virtual hosts and permissions.

The Definitions doc guide uses rabbitmqadmin, there is nothing wrong with using POST /api/definitions with a pre-defined definitions (JSON) file body.

kucix commented 3 years ago
  • Mount or download a rabbitmq.conf with the settings you need and the Erlang cookie file

Yes, if I use rabbitmq as image in which scripts run. But this is not that case. In services section of gitlab-ci.yml is mounting not possible.

And if I get it correctly, guest can't connect "remotely" by default, thus can't set vhost via api etc. Or can?

default:

# guest uses well known
# credentials and can only
# log in from localhost
# by default
loopback_users.guest = true
joaomcarlos commented 3 years ago

This change has broken hundreds of pipelines for us. Is there no easy setup with environment variables path anymore?

michaelklishin commented 3 years ago

@kucix I suggested using CLI tools or the HTTP API, neither of which has any restrictions for the default user.

You can use RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS to override rabbit.loopback_users:

docker run -it --rm --name rabbitmq -p 5672:5672 -p 15672:15672 -e RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS='-rabbit loopback_users "none"' rabbitmq:3.9-management

if really necessary.

michaelklishin commented 3 years ago

@joaomcarlos most environment variables that were specific to this image (not used by RabbitMQ itself) were intentionally removed.

Your options are outlined above in https://github.com/docker-library/rabbitmq/issues/508#issuecomment-888599571.

michaelklishin commented 3 years ago

One small feature in RabbitMQ itself that would simplify initial setup for tests would be introducing a load_definitions counterpart that uses a remote URL instead of a local file. That would instantly take care of users/virtual host/permission setup, or maybe even topologies if needed: https://github.com/rabbitmq/rabbitmq-server/issues/3249

Assuming that an HTTP API call against the management plugin-enabled container is possible on GItLab, I don't see why it would not be possible to import a basic definition file with users/virtual hosts/permissions without any additional features. Then simply use that user in your suites, creating new users over remote guest access has been the recommended way for many years.

gerhard commented 3 years ago

Secrets as environment variables is a bad idea, contrary to what 12factor recommends.

RabbitMQ didn't really support these, which is why docker-entrypoint.sh used in this image became the most complex part to the point that most were afraid of changing it. More importantly, this was creating all sorts of weird issues between Docker and Kubernetes. The container startup command should not be generating any just-in-time config from environment variables, because these will clash with what the container scheduler may need to do, especially when it comes to rotating secrets, or declarative topologies (think GitOps).

The defaults work the same, meaning that user guest with pass guest and / vhost CAN login remotely. I just checked with rabbitmq:3-management. This default guest user config makes that possible: https://github.com/docker-library/rabbitmq/blob/56d547f02dc0e9cf192cb8a0eedaeb70c5b01b15/10-default-guest-user.conf#L8

If you need to change the default user or vhost, mount a config file.

If you can't mount a file, then declare config properties via RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS, as @michaelklishin describes. It's still possible, just harder, because it's not recommended. By the way, the Erlang cookie can be specified this way as well (again, it's bad idea, but it works).

Also, @michaelklishin proposal in https://github.com/rabbitmq/rabbitmq-server/issues/3249 sounds like a sensible third solution for changing users, vhosts, permissions, etc. What do others think?

eriksw commented 3 years ago

Also, @michaelklishin proposal in rabbitmq/rabbitmq-server#3249 sounds like a sensible third solution for changing users, vhosts, permissions, etc. What do others think?

This is not useful unless it supports gs://, s3://, ... etc. (Using ambient creds to access those.)

ECS is a case where there's no way to inject a file, but where it is quite reasonable and well-supported to pass in secrets as environment variables.

Please put the brakes on these anti-environment-variable intentions.

Edit to add:

Please treat the removal of any particular environment variables as being just as breaking to end-users as the removal of a config key from the software itself.

michaelklishin commented 3 years ago

@eriksw why wouldn't downloads over HTTPS (https://github.com/rabbitmq/rabbitmq-server/issues/3249) be sufficient for service container cases? What limits most users to S3 or similar?

michaelklishin commented 3 years ago

@eriksw I strongly disagree that the RabbitMQ core team should treat "removal of any environment variables as breaking as removal of. configuration keys from RabbitMQ itself". This image invented most of the environment variables removed 3.9, and the fragile, difficult to maintain mechanism that generated a rabbitmq.conf from them, reinventing many things that rabbitmq.conf has supported for years along the way.

All environment variables RabbitMQ supports are still available.

If in some cases, e.g. GitLab or GitHub service containers, mounting a configuration file is impossible then something new can be introduced. We suggested https://github.com/rabbitmq/rabbitmq-server/issues/3249 as one idea.

michaelklishin commented 3 years ago

@eriksw why wouldn't downloads over HTTPS (rabbitmq/rabbitmq-server#3249) be sufficient for service container cases? What limits most users to S3 or similar?

If I'm reading #506 right, this is because ECS has some kind of limitations (e.g. can only use S3). So two options are:

While we won't add S3 support to RabbitMQ core in https://github.com/rabbitmq/rabbitmq-server/issues/3249, it can be added to a small new plugin that would load definitions from external sources, and each individual source can support environment variables if needed (e.g. AWS access and secret keys). I suspect this plugin would be significantly easier to reason about and maintain compared to the shell script/environment variable "configuration mechanism" in the 3.8 series of this image.

michaelklishin commented 3 years ago

Specifically for service containers (integration testing), adding three env variables to configure

might be enough for many suites. It won't require any complex shell script in this image and will be an option for non-container users. The values are either strings or a comma-separated list (or an empty string), which is difficult to screw up.

One argument against these additions is that RabbitMQ uses environment variables only when rabbitmq.conf is not an option (e.g. for the path to the configuration file itself, the node name, and so on).

@gerhard @dumbbell WDYT?

dumbbell commented 3 years ago

Just to make it clear to me, do you suggest RabbitMQ natively supports those three environment variables to override whatever the configuration files say?

gerhard commented 3 years ago

I think that if we can agree on the 3-4 environment variables that RabbitMQ would need to support natively, then we should implement this directly in RabbitMQ.

Our issue was the complexity of docker-entrypoint.sh, which we have addressed and deprecated since https://github.com/docker-library/rabbitmq/pull/424. We have been publicly working on this since June 2020. I think that we could have announced it better, and maybe given users more time, before shipping https://github.com/docker-library/rabbitmq/pull/467. Now that we know what the problem is, we can address it relatively quickly, if we can agree on which environment variables you care about the most @eriksw @kucix @Diggsey @wglambert @joaomcarlos

Would adding support for the following environment variables directly in RabbitMQ solve this issue?

kucix commented 3 years ago

For our purposes yes. These four variables.

eriksw commented 3 years ago

This image invented most of the environment variables removed 3.9...

As an end user, I do not care whether the environment variables are invented or implemented by RabbitMQ itself or by an entrypoint script.

I care about being able to configure these things (and everything) using environment variables, without the burden of having to maintain building and distributing a custom image.

I believe it is an important, nearly-fundamental purpose of an official library container image to do what it reasonably can to paper over and correct for things that have awful UX in the container ecosystem, such as file-based logging and config files.

I think that we could have announced it better, and maybe given users more time, before shipping #467.

In the future I'd suggest:

Had a process like that been followed, I think it's pretty reasonable to assume there would have been ample feedback here about the need to configure these things using environment variables; that config files are impractical or impossible to inject in many real-world circumstances.

@eriksw why wouldn't downloads over HTTPS (rabbitmq/rabbitmq-server#3249) be sufficient for service container cases? What limits most users to S3 or similar?

When running in a managed environment like ECS, the obvious place to store a file is the cloud provider's object storage service. Unless world-readable, that requires authorization to access—authorization you would grant to the ambient credential identity available to the running container, which means acting as a fully-fledged client of the various storage services and their various credential-discovery mechanisms.

Edit to add: This is relevant when the file contains sensitive values. If the file is one that doesn't need to contain any sensitive values, such that it'd be totally fine to serve the file in world-readable way, then yes, it would be unusual for there to be anything interfering with outbound access to load from an ordinary https url. (Notwithstanding the poor UX of working at the granularity of the entire file, instead of setting particular items.)

I should have been more clear: I don't think it's appropriate for RabbitMQ to support loading config files from all the various cloud storage providers. (It would be an arguably absurd, ever-growing amount of complexity and dependencies, certainly far worse than even a fully generic configure-anything-via-envar system.) I brought it up to try and illustrate how lacking the config/definitions-from-URL concept is when applied to the practicalities of environments where file mounts are a non-starter.

I appreciate the desire to keep the entrypoint maintainable, but if the RabbitMQ team itself isn't interested in being thoroughly configurable using environment variables, then the maintainers of this image are still in the next-best position to maintain a workaround to mitigate upstream unwillingness/disinterest.

gerhard commented 3 years ago

I am trying to empathise with your perspective @eriksw, but the negative undertones make it difficult. I understand where you are coming from, thank you for your input, and encourage you to think about the wider RabbitMQ ecosystem. Amazon MQ for RabbitMQ is an interesting perspective, as is the cluster-operator & messaging-topology-operator. We will do our best to make this work within the wider context, across multiple container images (this is just one of them), and the sprawling container orchestration landscape.

Thanks @kucix for confirming the environment variables which are most important to you.

What do you think @michaelklishin & @dumbbell, is it a good idea to handle these within RabbitMQ itself?

dumbbell commented 3 years ago

Yes, I think we can do that. I don't know much about the usage in containers, but I find environment variables really handy while testing and debugging.

I will prepare a patch and keep you posted.

dumbbell commented 3 years ago

Hi!

The patch is ready for wider testing (see rabbitmq/rabbitmq-server#3299).

CI should produce a Docker image. Could you please give it a try when that image appears? I don't know how to explain how to use the image built from this branch (I don't know it myself) and, as I use Docker once in a blue Moon, you will probably figure out way before me how to fetch and use that image :-)

michaelklishin commented 3 years ago

We should mention that these default credentials and virtual host are seeded only on first node boot but the cookie will be read and used on every node start. This can potentially be important in some environments.

michaelklishin commented 3 years ago

To pull the automatically built image for the above PR prepared by @dumbbell, use

docker pull pivotalrabbitmq/rabbitmq:add-env-vars-to-set-default-user-pass-vhost-and-erlang-cookie-otp-max

I don't know if there may be any public access limitations, though. The Docker Hub account was originally set up for the needs of the RabbitMQ core team.

michaelklishin commented 3 years ago

I tried

docker run -it --rm --name rabbitmq-pr-3299 -p 5672:5672 -p 15672:15672 -e RABBITMQ_DEFAULT_USER="pr-3299" -e RABBITMQ_DEFAULT_PASS="pr-3299" pivotalrabbitmq/rabbitmq:add-env-vars-to-set-default-user-pass-vhost-and-erlang-cookie-otp-max

and the image refuses to start:

error: RABBITMQ_DEFAULT_PASS is set but deprecated
error: RABBITMQ_DEFAULT_USER is set but deprecated
error: deprecated environment variables detected

so after the above PR is merged and a new release ships, we'd have to whitelist those four variables.

kucix commented 3 years ago

Sorry, that image is not working. It ends on deprecated error and stops.

image

michaelklishin commented 3 years ago

This is because the entrypoint is not aware of the fact that RABBITMQ_DEFAULT_USER and such are/can be/will be supported by RabbitMQ itself.

michaelklishin commented 3 years ago

Filed https://github.com/docker-library/rabbitmq/issues/513 which can be addressed when RabbitMQ 3.9.4 ships (should be in a week or two).

tianon commented 3 years ago

You'll lose a tiny amount of functionality (the few bits the entrypoint still does), but you should be able to test with that image via --entrypoint rabbitmq-server (probably want to also include --user rabbitmq or somesuch).

kucix commented 3 years ago

No, I cant. I!m testing scenario from first post - gitlab ci service.

michaelklishin commented 3 years ago

Those testing with local Docker can use --entrypoint and --user, thank you @tianon.

gerhard commented 3 years ago

This is a great first step, and we have definitely addressed a big part of the problem, but we still have a few issues left in the current pivotalrabbitmq/rabbitmq:v3.9.x-otp-max, specifically pivotalrabbitmq/rabbitmq:b94ca39a24bd84ca7e2f1fd252bf3b4e12bc5c06-otp-max

1/3. Default user cannot log in remotely

If the default user gets changed, then the loopback_users.guest = false should change too.

Would you agree @dumbbell ?

2/3. Defining an Erlang cookie triggers a warning

RABBITMQ_ERLANG_COOKIE env variable support is deprecated and will be REMOVED in a future version. Use the $HOME/.erlang.cookie file or the --erlang-cookie switch instead.

This was introduced via https://github.com/rabbitmq/rabbitmq-server/commit/d7ca0e94461faa87ffa4bb5d2731db1d62c6096b#diff-8fcb6fa84456486ea48b2c2c13a2505f053cfa5ab13604208a400b86719fe243, and if we now support it, we should go back and revisit it.

3/3. Default entrypoint does not work

This is a pretty big one. If we genuinely want to support these environment variables, then we should remove the now supported environment variables from https://github.com/docker-library/rabbitmq/blob/cd44d4e7a4335ae137c1723772e2a777f0466d67/docker-entrypoint.sh#L13-L33

Would you agree with this @tianon?

I was not around when https://github.com/docker-library/rabbitmq/pull/467 was merged, but I would agree that it's less a deprecation, and more a removal. Updating the variables to reflect this seems sensible to me. I can see now that us releasing 3.9.0 accelerated those changes, and in hindsight combining them doesn't seem right. We are now having to deal with the side-effects of this which is not great, but I am confident that we will be able to restore the essential functionality shortly, without needing to revert https://github.com/docker-library/rabbitmq/pull/467.

As an aside, pivotalrabbitmq/rabbitmq are dev container images which track this image closely, the only difference is that they enable all plugins and are built for every single commit via https://github.com/rabbitmq/rabbitmq-server/blob/master/.github/workflows/oci.yaml

dumbbell commented 3 years ago

1/3. Default user cannot log in remotely

If the default user gets changed, then the loopback_users.guest = false should change too.

The new environment variable just overrides the value of the default_user configuration parameter. The behavior behind this parameter didn't change with the patch. So currently, if one sets default_user in the configuration or use $RABBITMQ_DEFAULT_USER, it doesn't interfere with loopback_users.

The value of loopback_users is set independently of default_user. In other words, its default value is set to <<"guest">>, regardless of the value of default_user.

That said, if you think this behavior is wrong, we can change it. I agree it would make sense that loopback_users defaults to whatever default_user is.

gerhard commented 3 years ago

The value of loopback_users is set independently of default_user. In other words, its default value is set to <<"guest">>, regardless of the value of default_user.

That said, if you think this behavior is wrong, we can change it. I agree it would make sense that loopback_users defaults to whatever default_user is.

We may need to introduce another environment variable, so that we wouldn't need to do this: https://github.com/docker-library/rabbitmq/blob/master/10-default-guest-user.conf.

From an end-user perspective, the problem that we are trying to solve is to allow the default user & password to be configurable via environment variables. That user should be able to login via any network interface by default. The implementation is up to the person that writes the code 🙂

michaelklishin commented 3 years ago

Loopback user limitation exists to make sure that a node with the default seeded user (with widely known credentials) does not accept remote connections. If you override seed user credentials, sounds like the most basic "scan and try default credentials" kind of attack won't be effective by definition.

So let's keep the scope of the changes as is and not touch loopback user list defaults?

gerhard commented 3 years ago

I'm all for leaving the scope as is, and not expanding it with loopback_users. RABBITMQ_SERVER_ADDITIONAL_ERL_ARGS can be used for that purpose if needed.

2/3 and 3/3 still seem to be in scope to me. Would you agree @michaelklishin?

yosifkit commented 3 years ago

Yes, the entrypoint is easy to change and if RabbitMQ itself begins to support a variable of those names, they can be removed from the list.

michaelklishin commented 3 years ago

@gerhard yes, definitely, we will have to adjust the image. I have earlier filed #513 for that.

Note that RabbitMQ intentionally logs when the cookie value is overridden using an env variable: I assumed that this must be a warning as this new variable unfortunately can be used as an attack vector, so it should be very obvious from the logs what the cookie value source is.

gerhard commented 3 years ago

OK, sounds good, I'll be back in a week 👋🏻