rebus-org / Rebus.RabbitMq

:bus: RabbitMQ transport for Rebus
https://mookid.dk/category/rebus
Other
62 stars 44 forks source link

Vhost is forced to start with `/` #111

Closed micdah closed 7 months ago

micdah commented 9 months ago

Looks like an unintended bug might have been introduced by this commit: https://github.com/rebus-org/Rebus.RabbitMq/commit/96eead68ef808a852feff2ced51c1e15bd7f0f1b

From the looks of it, this change means any vhost must begin with a /, because uri.LocalPath will have the leading / included, so if I have a URI amqp://foo:bar@1.2.3.4/vhost then LocalPath looks like it will be /vhost, and previously when passing the entire Uri, RabbitMQ (correctly) interpreted the vhost to be vhost not /vhost

Sadly this seems to have just broken our production environment, so I am hastenly trying to rollback our updated 🔥

micdah commented 9 months ago

Looks like ConnectionFactory handles this explicitly, by taking the 2nd segment thus skipping the / which is the first segment:

        private void SetUri(Uri uri)
        {
            Endpoint = new AmqpTcpEndpoint();

            if (string.Equals("amqp", uri.Scheme, StringComparison.OrdinalIgnoreCase))
            {
                // nothing special to do
            }
            else if (string.Equals("amqps", uri.Scheme, StringComparison.OrdinalIgnoreCase))
            {
                Ssl.Enabled = true;
                Ssl.Version = AmqpUriSslProtocols;
                Ssl.AcceptablePolicyErrors = SslPolicyErrors.RemoteCertificateNameMismatch;
                Port = AmqpTcpEndpoint.DefaultAmqpSslPort;
            }
            else
            {
                throw new ArgumentException($"Wrong scheme in AMQP URI: {uri.Scheme}");
            }
            string host = uri.Host;
            if (!string.IsNullOrEmpty(host))
            {
                HostName = host;
            }
            Ssl.ServerName = HostName;

            int port = uri.Port;
            if (port != -1)
            {
                Port = port;
            }

            string userInfo = uri.UserInfo;
            if (!string.IsNullOrEmpty(userInfo))
            {
                string[] userPass = userInfo.Split(':');
                if (userPass.Length > 2)
                {
                    throw new ArgumentException($"Bad user info in AMQP URI: {userInfo}");
                }
                UserName = UriDecode(userPass[0]);
                if (userPass.Length == 2)
                {
                    Password = UriDecode(userPass[1]);
                }
            }

            /* C# automatically changes URIs into a canonical form
               that has at least the path segment "/". */
            if (uri.Segments.Length > 2)
            {
                throw new ArgumentException($"Multiple segments in path of AMQP URI: {string.Join(", ", uri.Segments)}");
            }
            if (uri.Segments.Length == 2)
            {
                VirtualHost = UriDecode(uri.Segments[1]);
            }

            _uri = uri;
        }
micdah commented 9 months ago

We rolled back to 8.0.0 just before the before mentioned commit, which fixed the issue.

Phew, that was an intense hour there were nearly all our services went down 😅 Luckily our API's remained online because they are slow enough to get ready that Rebus failed before the health probe was satisfied - whereas our background services where to quick to come online that the health probes were happy before Rebus threw an exception due to not being able to connect to RabbitMQ (becuase the vhost didn't exist).

I would strongly suggest we add some tests to this package to validate that the vhost is being configured properly when not using default / vhost 😄

micdah commented 9 months ago

Of course, sadly our acceptances tests uses a setup where it's the default vhost we are using, whereas production uses different vhosts - there always has to be something 🙈

EraYaN commented 9 months ago

A rollback indeed seems to fix it when a vhost is included in the connection string.

ldeluigi commented 8 months ago

@mookid8000 please release a hotfix, this is a breaking change!

micdah commented 8 months ago

I'm not entirely sure what the rewrite of how the URI is parsed does, otherwise I would have suggested simply reverting the commit which introduced this bug / behaviour.

But yeah, it's pretty troublesome as we effectively can't upgrade past 8.0.0 now as any version past this would break our entire production environment as we use multiple vhosts and none use the default.

I would expect most production environments use vhosts

ldeluigi commented 8 months ago

Yeah, us too since today as we found out this bug

ldeluigi commented 8 months ago

https://github.com/rebus-org/Rebus.RabbitMq/blob/a8e0d9a1593cdfcbe7887e73689b73ddce6b16a9/Rebus.RabbitMq/Internals/ConnectionManager.cs#L147

This line contains the issue: LocalPath property prefixes the result with one ore more slashes that must be stripped

mksergiy commented 7 months ago

@mookid8000 please release a hotfix, this is a breaking change!

it's not breaking change, it's a bug Nobody can use virtual host without leading / in Rebus.RabbitMq > 8.0.0

micdah commented 7 months ago

@mookid8000 please release a hotfix, this is a breaking change!

it's not breaking change, it's a bug Nobody can use virtual host without leading / in Rebus.RabbitMq > 8.0.0

I would argue that a "breaking change" is a type of bug, a particular critical type of bug, as it has breaking changes for those affected. 😄

I know it's getting caught up on terminology - but to be honest I'm also unsure what you are trying to convey by your comment, whether you are arguing that this issue is more critical or less so.

mksergiy commented 7 months ago

@mookid8000 please release a hotfix, this is a breaking change!

it's not breaking change, it's a bug Nobody can use virtual host without leading / in Rebus.RabbitMq > 8.0.0

I would argue that a "breaking change" is a type of bug, a particular critical type of bug, as it has breaking changes for those affected. 😄

I know it's getting caught up on terminology - but to be honest I'm also unsure what you are trying to convey by your comment, whether you are arguing that this issue is more critical or less so.

I agree with you that this bug is very critical. I just concern about the bug not being resolved, even as new versions continue to be released with the same issue.

micdah commented 7 months ago

@mookid8000 please release a hotfix, this is a breaking change!

it's not breaking change, it's a bug Nobody can use virtual host without leading / in Rebus.RabbitMq > 8.0.0

I would argue that a "breaking change" is a type of bug, a particular critical type of bug, as it has breaking changes for those affected. 😄 I know it's getting caught up on terminology - but to be honest I'm also unsure what you are trying to convey by your comment, whether you are arguing that this issue is more critical or less so.

I agree with you that this bug is very critical. I just concern about the bug not being resolved, even as new versions continue to be released with the same issue.

Ah yes I see, I am also curious why this hasn't been fixed yet.

It's especially troublesome, as new deployments using the bugged version, will have the exact same issue existing deployments have, i.e. they will have all their vhosts named /foo, /bar etc., and will need to update their configuration after it's fixed to include those forward slashes in the vhost name.

But that at least is doable, there is currently no way to make Rebus.RabbitMQ use an existing vhost named foo, as all vhosts must be named /<something> due to this bug.

micdah commented 7 months ago

I'd seriously consider unlisting the new version until a fix is implemented, and then have a changelog with huge warnings for any who have used the bugged versions on how they need to handle the upgrade if they have deployments where their vhosts are named /foo etc.

Otherwise userbase is slowly being split into those with vhosts with and without a leading slash, and it's going to be a pain for both at some point

mookid8000 commented 7 months ago

I believe this is fixed in Rebus.RabbitMq 9.0.0, which is on NuGet.org now 🙂 (together with Rebus 8 and all other official Rebus libs updated to work with that)*


(*) although, depending on how fast you are, you might run into a lib or two that hasn't been published yet... it's a matter of hours (maybe minutes, even) before I've gone through them all