-
Couldn't load subscription status.
- Fork 1.2k
feat(api)!: Allow provider connections to be dynamically managed #3892
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
base: main
Are you sure you want to change the base?
Conversation
|
@raghotham FYI here's the docs for access control: https://llamastack.github.io/docs/distributions/configuration#access-control |
fcab15e to
13b6f3d
Compare
| ... | ||
|
|
||
| @webmethod(route="/admin/providers/{api}/{provider_id}/test", method="POST", level=LLAMA_STACK_API_V1) | ||
| async def test_provider_connection(self, api: str, provider_id: str) -> TestProviderConnectionResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be named health?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you want this to be GET instead of POST then - which would then mean that the health of each connection has to be periodically checked and saved somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raghotham yes GET would be a bit more natural right...
| """ | ||
| ... | ||
|
|
||
| @webmethod(route="/admin/providers/{api}/{provider_id}", method="PUT", level=LLAMA_STACK_API_V1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we level these as alpha first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine with making it alpha
| :param api: API namespace this provider implements (e.g., 'inference', 'vector_io'). | ||
| :param provider_id: Unique identifier for this provider instance. | ||
| :param provider_type: Provider type (e.g., 'remote::openai'). | ||
| :param config: Provider configuration (API keys, endpoints, etc.). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the implementation carefully yet, but is there a security aspect to consider here? we have code which loads external modules -- can someone reference external python paths, etc. via this API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what our security posture is when there are external providers (can we have inline external providers?). These providers can take arbitrary payloads in their APIs and execute them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raghotham I think an attacker being able to inject code from remote is very different than a server admin deciding to include some external code right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true - i was thinking about what guarantees llama stack can provide. Maybe we can just say that the config passed in to the external providers would be typed checked against a config schema defined by the external provider author. Is there a stronger claim we can make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this gets any further, can we make sure all comments/concerns are addressed in the original issue? #3809 folks have left a few comments which I think need to be resolved first. Thanks!
What does this PR do?
Closes #3809
Backward incompatible change: Changes the ambiguous /v1/providers/{provider-id} to mean /v1/providers/{api} and returns a list of provider infos.
Test Plan
Added unit and integration tests