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

chore(1-3244): only expose streaming endpoint if in streaming mode #663

Conversation

thomasheartman
Copy link
Contributor

Make it so that the /streaming endpoint only accepts connections if the server is in streaming mode. This effectively renders the /streaming endpoint useless if the server is not connected to an upstream stream.

This first solution uses the 403 http status code and tells the user that the endpoint is only enabled in streaming mode.

Discussion

Is there a way to not mount this if we're not streaming? Is introducing a new edge error the right thing to do here, or is there an easier way to return arbitrary status codes?

Copy link

github-actions bot commented Jan 14, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

This seems okay to me. You're gonna have to fix the test though

@sighphyre
Copy link
Member

Is there a way to not mount this if we're not streaming?

There are a few endpoints that we conditionally mount, you can inspect the setup to see how that's done. That being said, I think what you've done is better. Its a nicer user experience to get an error message telling you what went wrong, rather than a NotFound

Is introducing a new edge error the right thing to do here, or is there an easier way to return arbitrary status codes?

That seems fine to me, it signals the intent well enough and you need to wrap it in an error of some kind

@thomasheartman thomasheartman merged commit f977b61 into main Jan 15, 2025
13 of 17 checks passed
@thomasheartman thomasheartman deleted the chore(1-3244)/only-expose-streaming-endpoint-if-mode-is-streaming branch January 15, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants