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

Allow auth to accept API keys #326

Merged
merged 16 commits into from
Oct 20, 2023

Conversation

saiatmakuri
Copy link
Contributor

first step in API key migration - chain functions tried

@saiatmakuri saiatmakuri added the enhancement New feature or request label Oct 16, 2023
@saiatmakuri saiatmakuri self-assigned this Oct 16, 2023
Copy link
Member

@seanshi-scale seanshi-scale left a comment

Choose a reason for hiding this comment

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

I'm not super confident we should be exposing notions e.g. user id + api key for the public repo, e.g. this functionality may belong better somewhere else (e.g. model-engine-internal)

@yixu34
Copy link
Member

yixu34 commented Oct 16, 2023

I'm not super confident we should be exposing notions e.g. user id + api key for the public repo, e.g. this functionality may belong better somewhere else (e.g. model-engine-internal)

Hmm I suppose that may be true. If we're hosting internally, we don't need to mention API keys here because the rest of our stack handles that; if we're deploying in customer VPCs, we rely on them to manage API keys and auth.

The main difference is that the EGP APIs do need to do this, because they're more of a fully fledged platform product that needs to manage its own API keys. cc @eric-scale

@saiatmakuri
Copy link
Contributor Author

saiatmakuri commented Oct 16, 2023

I'm not super confident we should be exposing notions e.g. user id + api key for the public repo, e.g. this functionality may belong better somewhere else (e.g. model-engine-internal)

I guess that I'm just acknowledging the underlying auth_repo functions here more loudly. That is, we currently use auth_repo.get_auth_from_user_id_async and now I'm referencing auth_repo.get_auth_from_api_key_async in a separate routine. The final steady state will be to only use the latter method with api keys. I believe we can introduce a new notion of UserRepo that can perform authentication via API keys. However, I think we defer this abstraction to a later time.

Copy link
Member

@yixu34 yixu34 left a comment

Choose a reason for hiding this comment

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

【we love dependency injection】

from model_engine_server.domain.gateways.monitoring_metrics_gateway import MonitoringMetricsGateway


class DatadogMonitoringMetricsGateway(MonitoringMetricsGateway):
Copy link
Member

Choose a reason for hiding this comment

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

did this file get moved to internal?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose that dependencies on public vendors (e.g. Datadog) can be put in our public repo. It's the Scale-specific bits that need to be internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, now a plugin

@saiatmakuri saiatmakuri enabled auto-merge (squash) October 20, 2023 22:57
@saiatmakuri saiatmakuri merged commit 280b867 into main Oct 20, 2023
5 checks passed
@saiatmakuri saiatmakuri deleted the saiatmakuri/llm-engine-auth-migration-pt-1 branch October 20, 2023 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants