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

fix[ws]: handle unsupported paths and message types #592

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Alex-Izquierdo
Copy link
Contributor

We are not handling WS requests for unsupported paths having a 500 unhandled exception

eda-ws_1                 | 2024-01-08 13:50:00,821 daphne.server ERROR    Exception inside application: No route found for path 'api/eda/ws/ansible-rulebookk'.
eda-ws_1                 | Traceback (most recent call last):
eda-ws_1                 |   File "/app/venv/lib64/python3.9/site-packages/django/contrib/staticfiles/handlers.py", line 101, in __call__
eda-ws_1                 |     return await self.application(scope, receive, send)
eda-ws_1                 |   File "/app/venv/lib64/python3.9/site-packages/channels/routing.py", line 62, in __call__
eda-ws_1                 |     return await application(scope, receive, send)
eda-ws_1                 |   File "/app/venv/lib64/python3.9/site-packages/channels/routing.py", line 134, in __call__
eda-ws_1                 |     raise ValueError("No route found for path %r." % path)
eda-ws_1                 | ValueError: No route found for path 'api/eda/ws/ansible-rulebookk'.
eda-ws_1                 | 2024-01-08 13:50:00,822 django.channels.server INFO     WebSocket DISCONNECT /api/eda/ws/ansible-rulebookk [10.89.0.20:52444]

Also I have found a bug where we are not handling unsupported message types.
Jira: https://issues.redhat.com/browse/AAP-19294

@Alex-Izquierdo Alex-Izquierdo requested a review from a team as a code owner January 8, 2024 15:53
jshimkus-rh
jshimkus-rh previously approved these changes Jan 8, 2024
src/aap_eda/wsapi/consumers.py Outdated Show resolved Hide resolved
jshimkus-rh
jshimkus-rh previously approved these changes Jan 8, 2024
src/aap_eda/wsapi/consumers.py Show resolved Hide resolved
jshimkus-rh
jshimkus-rh previously approved these changes Jan 8, 2024

async def connect(self):
await self.accept()
await self.send('{"error": "invalid path"}')
Copy link
Contributor

Choose a reason for hiding this comment

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

@Alex-Izquierdo Should we reveal this, it might be better to just close the connection

wsapi_router = URLRouter(
[path("ansible-rulebook", consumers.AnsibleRulebookConsumer.as_asgi())]
[
path("ansible-rulebook", consumers.AnsibleRulebookConsumer.as_asgi()),
Copy link
Contributor

Choose a reason for hiding this comment

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

@Alex-Izquierdo @bzwei has a PR which will put authentication on this path #540

@bzwei
Copy link
Contributor

bzwei commented Jan 11, 2024

@Alex-Izquierdo
Could HandleRouteNotFoundMiddleware suggested in django/daphne#165 be a more elegant solution?

@Alex-Izquierdo
Copy link
Contributor Author

@Alex-Izquierdo Could HandleRouteNotFoundMiddleware suggested in django/daphne#165 be a more elegant solution?

Hi @bzwei, I agree. Middleware seems like a good approach too, right now I don't fully understand how that middleware works, and even if we copy it, it probably should have its own tests. Since you are more familiar with this, if you really trust on that code, I have no problem using it, otherwise I would think the current approach is also simpler and effortless.

@bzwei
Copy link
Contributor

bzwei commented Jan 12, 2024

@Alex-Izquierdo Could HandleRouteNotFoundMiddleware suggested in django/daphne#165 be a more elegant solution?

Hi @bzwei, I agree. Middleware seems like a good approach too, right now I don't fully understand how that middleware works, and even if we copy it, it probably should have its own tests. Since you are more familiar with this, if you really trust on that code, I have no problem using it, otherwise I would think the current approach is also simpler and effortless.

If this is not urgently needed, I can try out the middleware solution.

@Alex-Izquierdo
Copy link
Contributor Author

@bzwei Yeah, I don't think it is critical since ansible-rulebook is the only real client of the websockets. Just I noticed that and I thought I could address it easily.

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.

None yet

4 participants