Include vhost for RabbitMQ when retrieving queue info with useRegex #2591

Photonios · 2022-02-02T08:24:18Z

The vhost is included when you don't use useRegex: true. It makes
sense to have consistent behaviour between the two modes.

Fixes #2498

Provide a description of what has been changed

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated

Fixes #

crisp2u · 2022-02-04T13:48:58Z

@JorTurFer Any ideea why the test are failing now ? They passed yesterday and the only change I did was changelog

JorTurFer · 2022-02-04T20:21:48Z

@JorTurFer Any ideea why the test are failing now ? They passed yesterday and the only change I did was changelog

Rerunning, it could be a transient error

JorTurFer

Hi,
Thanks for your contribution ❤️
Could you add any test to check this new feature please? I mean, to check that the vHost is correctly appended to the url

CHANGELOG.md

crisp2u · 2022-02-05T09:27:01Z

I was not sure about the breaking change. I mean without vhost is the same but with vhost is not. I can move it to improvements, your call. We'll have a look at the tests next week

JorTurFer · 2022-02-05T18:13:57Z

I was not sure about the breaking change. I mean without vhost is the same but with vhost is not.

That's why I said, if a user is already using rabbitmq with regex, vhost is not needed, so I'd assume that is not preset. In that case when vhost is not defined, there is not any breaking change.
any opinion about this @kedacore/keda-core-contributors ?

zroubalik · 2022-02-17T15:20:52Z

I was not sure about the breaking change. I mean without vhost is the same but with vhost is not.

That's why I said, if a user is already using rabbitmq with regex, vhost is not needed, so I'd assume that is not preset. In that case when vhost is not defined, there is not any breaking change. any opinion about this @kedacore/keda-core-contributors ?

Agree

zroubalik · 2022-02-17T15:22:11Z

Is there a particular reason why is this marked as draft?

zroubalik

Looking good,

CHANGELOG.md

crisp2u · 2022-02-21T08:37:31Z

@zroubalik I guess we marked it as draft because we were not sure if the tests are correct and then we could not change the status.
This can be merged.

JorTurFer · 2022-02-21T08:56:42Z

/run-e2e rabbitmq-queue*
Update: You can check the progres here

pkg/scalers/rabbitmq_scaler.go

JorTurFer · 2022-02-21T10:38:40Z

/run-e2e rabbitmq-queue*
Update: You can check the progres here

zroubalik · 2022-02-28T18:18:35Z

@Photonios any update on this please?

crisp2u · 2022-03-01T07:26:26Z

Sorry guys, we've been very busy and did not have time for this. I think some e2e tests need to be updated. We will try to have a look this week

JorTurFer · 2022-03-01T11:52:49Z

Sorry guys, we've been very busy and did not have time for this. I think some e2e tests need to be updated. We will try to have a look this week

I'm not sure if the e2e test is wrong. I mean, the e2e test is testing regex without any vHost, so it should work in the same way even after your changes. Adding one e2e test to check the regex with vHost sounds good, but the current e2e test is testing the other scenario and we should keep it (or do a breaking change, but I think we should avoid it).
I proposed a change on my review that could solve this problem, supporting both cases. WDYT?

valentindeaconu · 2022-03-02T15:40:05Z

After further investigations, I decided to rework the vhost computation part because the way it was computed in our previous commits completely removed the flow in which no vhost was provided (when the queue name regex was used). Indeed, this was the reason that led to the failing E2E test, but that was another problem that I tried to fix. The E2E test was created to test the case when a vhost and a queue name regex are provided, but the connection string omitted the vhost part, so the test was actually running on “All” vhosts, which led to the failure from our previous commits. I want to underline the fact that in my opinion, a new E2E test should be added, which will properly test the case with vhost = "" (which in fact is the equivalent for the "All" option in the RabbitMQ management console).

I spent some time in the RabbitMQ management console and I observed that if you're using any vhost (but the "All" option), the URL will contain the /<vhost> part, including the root vhost ('/'), which will lead the URL in the following format: /%2F. If the “All” option is selected, the URL does not contain the vhost at all, jumping directly to the query params.

In the end, I want to mention that I tried to set up the environment locally in order to run the E2E tests, but I think I failed since on my local machine all tests were passing, except for the scalers/rabbitmq-queue-trigger-auth.test.ts.

@JorTurFer can you please re-run the E2E tests?

JorTurFer · 2022-03-02T17:30:53Z

tests/scalers/rabbitmq-queue-http-regex.test.ts

@@ -22,7 +22,7 @@ test.before(t => {

  sh.config.silent = true
  // create deployment
-  const httpConnectionString = `http://${username}:${password}@rabbitmq.${rabbitmqNamespace}.svc.cluster.local`
+  const httpConnectionString = `http://${username}:${password}@rabbitmq.${rabbitmqNamespace}.svc.cluster.local/${vhost}`


Why is this change needed? I mean, current test is working without this change. In fact, this change could hide the problem from my changes request.
If you want to test another feature, creating a new test is better.
This test checks that using an empty vHost (not in parameter but neither in connectionString), regex returns results from all vHosts, that's why the queues are under test-vh-regex vHost and KEDA connectionString doesn't have any vHost.
Could you add another test case for this new case (regex inside a specific vHost)?

BTW, I think that the change in the code solves the problem, but I'd like to maintain and increase the e2e test suite instead of changing one for another

I had a different point of view of that test and believed that the behavior you just described was not intended. Sure, I can undo the change and just duplicate the test for this behavior if you think that's ok.

IDK if undo the change or simply adding other case with current behavior, but both behaviors should be tested IMO because in fact, that test discovered a breaking change.
WDYT?

I agree with you, both behaviors should be tested, that's why I created another test. Adding another case to the existing test should require calling the createDeployment method twice and IMO the complexity of the test logic will grow, so I decided to keep things simple and just duplicate the test and set the parameters to validate the behavior with the connection string containing the vhost.

I don't believe the test discovered a breaking change. Before this fix we're trying to bring it in keda, the vhost part was completely ignored by the logic (when the regex was used for querying queue names). We just improved that flow and added support for that logic. The test was failing because initially, by adding this fix we removed the support for "All" vhosts, and the test was testing exactly that behavior. I fixed the problem and I improved the unit tests to ensure all 3 cases are tested:

Using no vhost - vhost = "" (meaning "All" vhosts)

Using root vhost - vhost = "/" or vhost = "%2F"

Using any vhost - vhost = "some-vhost"

JorTurFer · 2022-03-02T19:50:13Z

/run-e2e rabbitmq*
Update: You can check the progres here

JorTurFer · 2022-03-03T07:28:56Z

hi @Photonios @valentindeaconu,
Could you rebase main into your branch?

The vhost is included when you don't use `useRegex: true`. It makes sense to have consistent behaviour between the two modes. Fixes kedacore#2498 Signed-off-by: Swen Kooij <swen@sectorlabs.ro>

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

JorTurFer · 2022-03-03T15:17:34Z

/run-e2e rabbitmq*
Update: You can check the progres here

JorTurFer

LGTM.
Only 2 nits:

Please add the issue instead of the PR to the changelog (we have changed that system some days ago 😄 )
Related with e2e test, Could you create the dummy queues in different vHost and push message to all of them? I mean, instead of adding dummy queues to the same vHost that you are using for scaling, in others. With this change you will test that regex only applies to your vHost

BTW. Thanks a ton for this contribution and for all the effort that you have done ❤️ ❤️ ❤️ ❤️

CHANGELOG.md

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

valentindeaconu · 2022-03-04T13:44:12Z

LGTM. Only 2 nits:

Please add the issue instead of the PR to the changelog (we have changed that system some days ago 😄 )

Related with e2e test, Could you create the dummy queues in different vHost and push message to all of them? I mean, instead of adding dummy queues to the same vHost that you are using for scaling, in others. With this change you will test that regex only applies to your vHost

BTW. Thanks a ton for this contribution and for all the effort that you have done ❤️ ❤️ ❤️ ❤️

Sure, I pushed the changes.

We're glad we could fix this problem so we can both benefit from it. Will it be possible to have a bug-fix release containing this PR? We'll need it soon and doing a local build will require a lot of extra work.

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

JorTurFer · 2022-03-04T15:45:58Z

/run-e2e rabbit*
Update: You can check the progres here

JorTurFer · 2022-03-04T15:52:46Z

We're glad we could fix this problem so we can both benefit from it. Will it be possible to have a bug-fix release containing this PR? We'll need it soon and doing a local build will require a lot of extra work.

Hey, unluckily that's not possible because in fact, this is not a bug-fix, it's a new feature. :(

I'd suggest to, after this PR is merged (and the image generated) pull it from KEDA's register, push it into one registry under your control and use that registry in your scenario till next version. With this approach, you will not need to build it from scratch by your self and you'll have a frozen version till next release in 2 months.

Other option could be use the image's sha instead of the tag for pulling always the same image but IDK if we could delete old images untagged sometime

JorTurFer

LGTM

scalen · 2022-03-04T16:25:40Z

We're glad we could fix this problem so we can both benefit from it. Will it be possible to have a bug-fix release containing this PR? We'll need it soon and doing a local build will require a lot of extra work.

Hey, unluckily that's not possible because in fact, this is not a bug-fix, it's a new feature. :(

Just to interject here, the RabbitMQ scaler has two relevant features:

Use regex in queueName parameter to select queue[s] instead of full name ([applying sum, max or avg] to compute the number of messages [across many queues]).
[Set the] Vhost to use for the connection.

[...] is text I added to clarify/frame the two features better for this discussion, the rest is quoted directly from the documentation.

The documentation does not say anywhere that these two features are incompatible. As such, the fact that these two features are incompatible is a bug: either a documentation bug or an application bug, depending on how you choose to see it. Because of this, people will continue trying to use these two features together, perceiving the results as a bug until they search around enough to find this PR or the issue from which it stems.

The perception of a bug on the part of all users with this particular (not unusual or unexpected) use case would generally be a good argument for treating the feature-gap as a bug: either patching the documentation to highlight the incompatibility, or (as the application fix is already complete) patching the application between minor releases.

JorTurFer · 2022-03-04T16:42:27Z

We're glad we could fix this problem so we can both benefit from it. Will it be possible to have a bug-fix release containing this PR? We'll need it soon and doing a local build will require a lot of extra work.

Hey, unluckily that's not possible because in fact, this is not a bug-fix, it's a new feature. :(

Just to interject here, the RabbitMQ scaler has two relevant features:

Use regex in queueName parameter to select queue[s] instead of full name ([applying sum, max or avg] to compute the number of messages [across many queues]).

[Set the] Vhost to use for the connection.

[...] is text I added to clarify/frame the two features better for this discussion, the rest is quoted directly from the documentation.

The documentation does not say anywhere that these two features are incompatible. As such, the fact that these two features are incompatible is a bug: either a documentation bug or an application bug, depending on how you choose to see it. Because of this, people will continue trying to use these two features together, perceiving the results as a bug until they search around enough to find this PR or the issue from which it stems.

The perception of a bug on the part of all users with this particular (not unusual or unexpected) use case would generally be a good argument for treating the feature-gap as a bug: either patching the documentation to highlight the incompatibility, or (as the application fix is already complete) patching the application between minor releases.

Okey,
Even do we treat this as a bug, we don't plan to do any urgent release with these changes, sorry.
KEDA's release plan is defined in governance repository
Do you have any other pov @kedacore/keda-maintainers ?

scalen · 2022-03-04T17:35:24Z

Okey, Even do we treat this as a bug, we don't plan to do any urgent release with these changes, sorry. KEDA's release plan is defined in governance repository Do you have any other pov @kedacore/keda-maintainers ?

Fair enough. I can't see a discussion of your policy on patch releases in the linked governance repo, but I can see your most recent patch release was a week or two after your latest minor version release.

That patch had a small number of small updates, such as a fix to an e2e test. The main fix seems to be to avoid sometimes-flaky behaviour when there are multiple triggers for a single scaled object. I assume that that gives an idea of the impact threshold past which you will do a patch release.

JorTurFer · 2022-03-04T19:22:11Z

just for clarifying, any extra release could be done if @kedacore/keda-maintainers agree with it. 😄

v2.6.1 was released with the agreement of maintainers for solving a critical bug in the own core which affected whole the service after hard work before release v2.6.0 solving other bugs related with the cache in the core.

In this case, the potential impact of the error seems lower, so using the workaround for having this patch sounds reasonable. Using main tag (under your own risk) is other possibility for having this patch available since now

…edacore#2591) Signed-off-by: SushmithaVReddy <sushmithavangala@gmail.com>

…edacore#2591) Signed-off-by: Ram Cohen <ram.cohen@gmail.com>

Photonios force-pushed the useregex-vhost branch 2 times, most recently from 2284346 to 4603a4f Compare February 2, 2022 08:43

valentindeaconu force-pushed the useregex-vhost branch 2 times, most recently from 6adfe64 to 6b7bcee Compare February 3, 2022 16:16

crisp2u force-pushed the useregex-vhost branch from a474e1d to d6a823f Compare February 4, 2022 12:01

JorTurFer reviewed Feb 5, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

JorTurFer requested a review from a team February 14, 2022 19:45

zroubalik approved these changes Feb 17, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

JorTurFer marked this pull request as ready for review February 21, 2022 08:55

JorTurFer marked this pull request as draft February 21, 2022 09:12

JorTurFer requested changes Feb 21, 2022

View reviewed changes

pkg/scalers/rabbitmq_scaler.go Show resolved Hide resolved

zroubalik marked this pull request as ready for review February 28, 2022 08:51

JorTurFer reviewed Mar 2, 2022

View reviewed changes

Photonios and others added 2 commits March 3, 2022 13:19

Include vhost for RabbitMQ when retrieving queue info with useRegex

4ef5df8

The vhost is included when you don't use `useRegex: true`. It makes sense to have consistent behaviour between the two modes. Fixes kedacore#2498 Signed-off-by: Swen Kooij <swen@sectorlabs.ro>

Fix TestGetQueueInfoWithRegex

e1c9da3

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

valentindeaconu added 5 commits March 3, 2022 13:38

fix changelog conflict

e4011d8

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

Fix vhost computation

95bf8d3

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

Revamp unit tests; Add e2e test for regex w/ vhost

47c081e

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

Fix bug when overriding with empty string; Fix TestGetQueueInfo

5f7e741

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

Store root vhost path as constant

7692edf

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

valentindeaconu force-pushed the useregex-vhost branch from dbfae76 to 7692edf Compare March 3, 2022 11:40

JorTurFer reviewed Mar 3, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

update changelog

14f32c4

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

valentindeaconu force-pushed the useregex-vhost branch from fd8918d to 14f32c4 Compare March 4, 2022 10:30

Improve e2e test

03f8642

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

valentindeaconu force-pushed the useregex-vhost branch from 45ef243 to 03f8642 Compare March 4, 2022 13:20

remove whitespace

9a16563

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

align variables namings

f1fca11

Signed-off-by: Valentin Deaconu <valentin.deaconu@sectorlabs.ro>

JorTurFer approved these changes Mar 4, 2022

View reviewed changes

JorTurFer merged commit 7eeb9ba into kedacore:main Mar 4, 2022

sushmithavangala pushed a commit to sushmithavangala/keda that referenced this pull request Mar 15, 2022

Include vhost for RabbitMQ when retrieving queue info with useRegex (k…

0ea850c

…edacore#2591) Signed-off-by: SushmithaVReddy <sushmithavangala@gmail.com>

RamCohen pushed a commit to RamCohen/keda that referenced this pull request Mar 23, 2022

Include vhost for RabbitMQ when retrieving queue info with useRegex (k…

800b16f

…edacore#2591) Signed-off-by: Ram Cohen <ram.cohen@gmail.com>

JorTurFer mentioned this pull request Aug 26, 2022

fix(rabbitmq): Parse vhost correctly if it's provided in the host url #3604

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include vhost for RabbitMQ when retrieving queue info with useRegex #2591

Include vhost for RabbitMQ when retrieving queue info with useRegex #2591

Photonios commented Feb 2, 2022 •

edited

Loading

crisp2u commented Feb 4, 2022

JorTurFer commented Feb 4, 2022

JorTurFer left a comment •

edited

Loading

crisp2u commented Feb 5, 2022

JorTurFer commented Feb 5, 2022 •

edited

Loading

zroubalik commented Feb 17, 2022

zroubalik commented Feb 17, 2022

zroubalik left a comment

crisp2u commented Feb 21, 2022

JorTurFer commented Feb 21, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Feb 21, 2022 •

edited by github-actions bot

Loading

zroubalik commented Feb 28, 2022

crisp2u commented Mar 1, 2022

JorTurFer commented Mar 1, 2022

valentindeaconu commented Mar 2, 2022 •

edited

Loading

JorTurFer Mar 2, 2022 •

edited

Loading

JorTurFer Mar 2, 2022

valentindeaconu Mar 3, 2022

JorTurFer Mar 3, 2022

valentindeaconu Mar 3, 2022

JorTurFer commented Mar 2, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 3, 2022 •

edited

Loading

JorTurFer commented Mar 3, 2022 •

edited by github-actions bot

Loading

JorTurFer left a comment •

edited

Loading

valentindeaconu commented Mar 4, 2022

JorTurFer commented Mar 4, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 4, 2022

JorTurFer left a comment

scalen commented Mar 4, 2022

JorTurFer commented Mar 4, 2022

scalen commented Mar 4, 2022

JorTurFer commented Mar 4, 2022

Include vhost for RabbitMQ when retrieving queue info with useRegex #2591

Include vhost for RabbitMQ when retrieving queue info with useRegex #2591

Conversation

Photonios commented Feb 2, 2022 • edited Loading

Checklist

crisp2u commented Feb 4, 2022

JorTurFer commented Feb 4, 2022

JorTurFer left a comment • edited Loading

Choose a reason for hiding this comment

crisp2u commented Feb 5, 2022

JorTurFer commented Feb 5, 2022 • edited Loading

zroubalik commented Feb 17, 2022

zroubalik commented Feb 17, 2022

zroubalik left a comment

Choose a reason for hiding this comment

crisp2u commented Feb 21, 2022

JorTurFer commented Feb 21, 2022 • edited by github-actions bot Loading

JorTurFer commented Feb 21, 2022 • edited by github-actions bot Loading

zroubalik commented Feb 28, 2022

crisp2u commented Mar 1, 2022

JorTurFer commented Mar 1, 2022

valentindeaconu commented Mar 2, 2022 • edited Loading

JorTurFer Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

JorTurFer Mar 2, 2022

Choose a reason for hiding this comment

valentindeaconu Mar 3, 2022

Choose a reason for hiding this comment

JorTurFer Mar 3, 2022

Choose a reason for hiding this comment

valentindeaconu Mar 3, 2022

Choose a reason for hiding this comment

JorTurFer commented Mar 2, 2022 • edited by github-actions bot Loading

JorTurFer commented Mar 3, 2022 • edited Loading

JorTurFer commented Mar 3, 2022 • edited by github-actions bot Loading

JorTurFer left a comment • edited Loading

Choose a reason for hiding this comment

valentindeaconu commented Mar 4, 2022

JorTurFer commented Mar 4, 2022 • edited by github-actions bot Loading

JorTurFer commented Mar 4, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

scalen commented Mar 4, 2022

JorTurFer commented Mar 4, 2022

scalen commented Mar 4, 2022

JorTurFer commented Mar 4, 2022

Photonios commented Feb 2, 2022 •

edited

Loading

JorTurFer left a comment •

edited

Loading

JorTurFer commented Feb 5, 2022 •

edited

Loading

JorTurFer commented Feb 21, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Feb 21, 2022 •

edited by github-actions bot

Loading

valentindeaconu commented Mar 2, 2022 •

edited

Loading

JorTurFer Mar 2, 2022 •

edited

Loading

JorTurFer commented Mar 2, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Mar 3, 2022 •

edited

Loading

JorTurFer commented Mar 3, 2022 •

edited by github-actions bot

Loading

JorTurFer left a comment •

edited

Loading

JorTurFer commented Mar 4, 2022 •

edited by github-actions bot

Loading