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

HTTP API: allow connections to be listed and closed by username #5319

Conversation

NuwanSameera
Copy link

@NuwanSameera NuwanSameera commented Jul 23, 2022

This feature was added to RabbitMQ management plugin. New HTTP API added to delete connection by username.

Following is the API
/api/connections/username/admin-test

Current implementation provides delete connection by connection name. Connection name doesn't have known parameters. User name is a known parameter. This feature is important to delete connection programmatically in SSL certificate login enable server. In this case user name contains client CN key of the certificate.

If RabbitMQ uses custom authentication backend, this feature is very useful. We can implement logic to find valid CN key and we can remove connection if that logic says username is invalid. With this implementation we can block reconnection.

See:

@lukebakken lukebakken self-assigned this Jul 23, 2022
@lukebakken lukebakken self-requested a review July 23, 2022 15:38
Copy link
Collaborator

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

I fixed the whitespace and one compilation warning. This API is different (and better!) than what was previously described.

Copy link
Collaborator

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

Instead of only supporting DELETE, we should support GET as well to list connections by username.

I'm not 100% convinced that /connections/username is the best REST resource for this. @michaelklishin thoughts? How about this

/connections?username=FOO

I think the version with the query parameter makes the most sense. I'll see if we do something similar elsewhere in the API.

Good discussion here: https://stackoverflow.com/questions/34244627/getting-a-resource-using-another-resources-id-naming-in-rest-api

@NuwanSameera please note the API docs will have to be updated as well - https://github.com/rabbitmq/rabbitmq-server/blob/master/deps/rabbitmq_management/priv/www/api/index.html

@NuwanSameera
Copy link
Author

@lukebakken I updated API docs for DELETE API.

Also I enabled and check for GET request for same code. But it gave 500 error.

@lukebakken lukebakken self-requested a review July 23, 2022 17:49
Copy link
Collaborator

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

Hi @NuwanSameera -

Also I enabled and check for GET request for same code. But it gave 500 error.

I don't see changes in your pull request for enabling GET. Did you push them to your fork at https://github.com/NuwanSameera/rabbitmq-server.git ?

deps/rabbit/src/rabbit_connection_tracking.erl Outdated Show resolved Hide resolved
@NuwanSameera NuwanSameera force-pushed the feature-delete-connection-by-username branch from 6116a46 to aa9bbb6 Compare July 24, 2022 04:09
@NuwanSameera
Copy link
Author

@lukebakken Enabled GET method to get all connections by username.
API documentation also updated.

@michaelklishin
Copy link
Member

/connections/by/username/{username} or /connections/filtered/username/{username} would be fine with me.

An overly generic endpoint that allows filtering by every potential argument will quickly
grow large and complex. On top of that, query string values are rarely validated and supported
paths by definition are.

@michaelklishin
Copy link
Member

@NuwanSameera thank you for considering to contribute. This looks like a promising start.
It lacks a CLI command but given all core API elements in place, we can easily add one on your behalf.

Note that we are about to switch from Mnesia tables to a node-local ETS one for connection tracking in #5301. This is significantly
more efficient and helps with the footprint in high churn scenarios. That's for 3.11 only.

That means that this PR will have to change to adapt and for 3.10, we'd have to submit
a version that looks like what we have today.

@michaelklishin michaelklishin changed the title Feature delete connection by username HTTP API: allow connections to be listed and closed by username Jul 25, 2022
@NuwanSameera
Copy link
Author

Have any additional work need to do from myside.

@mkuratczyk
Copy link
Contributor

Thanks a lot for taking the time to work on a feature you need!

CLI support already exists: #2771. There's some code that could be shared:

  • PR2771 relies on rabbit_connection_tracking:list_of_user/1, so I think list_by_username/1 is redundant
  • PR2771 added rabbit_networking:close_all_user_connections/2, which I think could be used in this PR

@NuwanSameera
Copy link
Author

@mkuratczyk I updated code with suggested methods. List all connections method is working properly.
But delete all connections method didn't work for me.

Following method every time return empty list.
https://github.com/NuwanSameera/rabbitmq-server/blob/ccc4f69471cf073eeaedf268f296387396edc333/deps/rabbit/src/rabbit_networking.erl#L411

So

https://github.com/NuwanSameera/rabbitmq-server/blob/ccc4f69471cf073eeaedf268f296387396edc333/deps/rabbit/src/rabbit_networking.erl#L477

condition false every time. Any mistake in my code ?

@mkuratczyk
Copy link
Contributor

I've just tested the current state of the PR and it seems to work for me:

❯ curl -s http://guest:guest@localhost:15672/api/connections/username/guest | jq
[
  {
    "name": "127.0.0.1:58072 -> 127.0.0.1:5672",
    "vhost": "/",
    "user": "guest",
    "node": "rabbit@mkuratczyk-a02"
  },
  {
    "name": "127.0.0.1:58074 -> 127.0.0.1:5672",
    "vhost": "/",
    "user": "guest",
    "node": "rabbit@mkuratczyk-a02"
  },
  {
    "name": "[::1]:58073 -> [::1]:5672",
    "vhost": "/",
    "user": "guest",
    "node": "rabbit@mkuratczyk-a02"
  },
  {
    "name": "[::1]:58075 -> [::1]:5672",
    "vhost": "/",
    "user": "guest",
    "node": "rabbit@mkuratczyk-a02"
  }
]

❯ curl -s -X DELETE http://guest:guest@localhost:15672/api/connections/username/guest | jq

❯ curl -s http://guest:guest@localhost:15672/api/connections/username/guest | jq
[]

@NuwanSameera
Copy link
Author

NuwanSameera commented Jul 26, 2022

@mkuratczyk It is working for AMQP connections. But not work for MQTT connections.

@michaelklishin
Copy link
Member

MQTT connections likely use a separate set of functions that lists them. Querying tables
directly worked around this.

I'd wait for #5301 to be finished, then use whatever newer version of the connection tracking API we would end up with, then backport
and adapt to the API 3.10.x has/will have.

@NuwanSameera
Copy link
Author

I updated PR with changing delete method. It is tested with MQTT connections. Also removed duplicate list connection method.

Is it able to get official docker image with these APIs ?

@mkuratczyk
Copy link
Contributor

The official image (rabbitmq) will be built once we release a new RabbitMQ version with the changes merged. For PRs, we generally build test images (https://hub.docker.com/r/pivotalrabbitmq/rabbitmq/tags), but since your PR comes from a fork, this process wasn't triggered. You can try make package-generic-unix && make docker-image to build an image locally.

@michaelklishin
Copy link
Member

AFAIK all PRs related to connection tracking are in. @NuwanSameera can you please rebase and we will QA? Thank you.

@NuwanSameera
Copy link
Author

@michaelklishin
git rebase --onto master v3.10.6 feature-delete-connection-by-username
Is this correct command to rebase ?

@michaelklishin
Copy link
Member

michaelklishin commented Jul 29, 2022

@NuwanSameera simply pull master, then switch back to your branch, git rebase master and force-push to your fork using something like git push -u {name of your remote} HEAD -f.

@NuwanSameera NuwanSameera force-pushed the feature-delete-connection-by-username branch from 957378d to e067c05 Compare July 29, 2022 13:23
@NuwanSameera
Copy link
Author

@michaelklishin pushed changes to my fork branch. But it is not compile.

Following error getting when executing make run-broker

Generated escript escript/rabbitmqctl with MIX_ENV=dev
make[2]: Leaving directory '/home/STL-NUWAN/rabbitmq-server/deps/rabbitmq_cli'
make[2]: Entering directory '/home/STL-NUWAN/rabbitmq-server/deps/amqp10_common'
make[2]: Leaving directory '/home/STL-NUWAN/rabbitmq-server/deps/amqp10_common'
make[2]: *** No rule to make target 'include/feature_flags.hrl', needed by 'src/rabbit_core_ff.erl'.  Stop.
make[1]: *** [../../erlang.mk:5197: app] Error 2
make[1]: Leaving directory '/home/STL-NUWAN/rabbitmq-server/deps/rabbit'
make: *** [erlang.mk:4512: deps] Error 2

@michaelklishin
Copy link
Member

There were quite a few changes in master in the last week or so. You have to run

gmake distclean

from repo root.

@NuwanSameera
Copy link
Author

NuwanSameera commented Jul 29, 2022

@michaelklishin It also gave an error.

make[2]: Leaving directory '/home/STL-NUWAN/rabbitmq-server/deps/amqp10_common'
make[2]: *** No rule to make target 'include/feature_flags.hrl', needed by 'src/rabbit_core_ff.erl'.  Stop.
make[1]: *** [../../erlang.mk:5197: app] Error 2
make[1]: Leaving directory '/home/STL-NUWAN/rabbitmq-server/deps/rabbit'
make: *** [erlang.mk:4512: deps] Error 2

Is latest PR work for you?

@michaelklishin
Copy link
Member

git clone https://github.com/NuwanSameera/rabbitmq-server.git
cd rabbitmq-server
git checkout feature-delete-connection-by-username
cd deps/rabbit
gmake

does successfully build for me. So does running a node from source using

gmake run-broker PLUGINS="rabbitmq_management"

I use Bazel exclusively these days but it's clearly an issue you'd run into with Make when you switch branches or rebase as master moves quickly.

You can try

gmake -C deps/rabbit/apps/rabbitmq_prelaunch distclean
gmake -C deps/rabbit_common distclean
gmake -C deps/rabbit distclean

gmake -C deps/rabbit/apps/rabbitmq_prelaunch
gmake -C deps/rabbit_common
gmake -C deps/rabbit

to clean all the core bits that have changed recently and recompile them.

@NuwanSameera
Copy link
Author

NuwanSameera commented Jul 30, 2022

@michaelklishin work after following your suggested steps. I tested list and close connections with latest code. It is working fine.

Thank you.

@lukebakken
Copy link
Collaborator

@NuwanSameera we decided to keep your original API -

localhost:15672/api/connections/username/guest

Format code

Fix whitespace, fix warning

Update API docs

Remove blank lines

Add get all connections by username

Fix method name issue

Enable GET method to get connections by username

Update API documentation

Modify list all connections of username method

Remove list_by_username method and modify get all connections of user API

Code formatting, break up lines for readability

Refactor code to use pattern matching more effectively

Typo
@lukebakken lukebakken force-pushed the feature-delete-connection-by-username branch from 8b12642 to 6eb2630 Compare August 5, 2022 20:35
@michaelklishin
Copy link
Member

I will take a look first thing tomorrow morning

@michaelklishin michaelklishin merged commit 8a86dcb into rabbitmq:master Aug 5, 2022
michaelklishin added a commit that referenced this pull request Aug 5, 2022
HTTP API: allow connections to be listed and closed by username (backport #5319)
michaelklishin added a commit that referenced this pull request Aug 6, 2022
HTTP API: allow connections to be listed and closed by username (backport #5319) (backport #5456)
@NuwanSameera NuwanSameera deleted the feature-delete-connection-by-username branch August 12, 2022 10:08
@NuwanSameera
Copy link
Author

Have any estimated date to release this feature ?

@michaelklishin
Copy link
Member

It will ship in 3.11.0. We do not make any ETA promises.

@NuwanSameera
Copy link
Author

In the release note of RabbitMQ 3.11.0-rc.1 mentioned this feature as

New endpoint for listing connections of a specific user. I think better if it change as New endpoint for listing and closing connections of a specific user

michaelklishin added a commit that referenced this pull request Sep 9, 2022
3.11 release notes: reword as suggested in #5319
mergify bot pushed a commit that referenced this pull request Sep 9, 2022
michaelklishin added a commit that referenced this pull request Sep 9, 2022
3.11 release notes: reword as suggested in #5319 (backport #5732)
michaelklishin added a commit to michaelklishin/rabbit-hole that referenced this pull request Jun 6, 2023
michaelklishin added a commit to michaelklishin/rabbit-hole that referenced this pull request Jun 6, 2023
and rabbitmq/rabbitmq-server#5319.

Currently skipped, as they will fail on all GA releases
available right now. They do pass against RabbitMQ main and 3.12, 3.11
release branches with a backport.
@michaelklishin
Copy link
Member

This shipped with a bug where for un(der)privileged users these new endpoints failed with a 500, addressed in #8483.

It now also has some test coverage in michaelklishin/rabbit-hole#271.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants