-
Notifications
You must be signed in to change notification settings - Fork 0
[PD1-66] - Allow api keys mapping to fetch data across tenants #14
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
Conversation
PD1-66 Data Backbone Algorithm Repository
|
Summary of ChangesHello @olivierpichon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the SDK's capability to manage API keys by introducing a flexible mechanism to retrieve them from centralized sources like AWS Secrets Manager or local environment configurations. This change significantly improves the SDK's adaptability for multi-tenant deployments and streamlines API key management, while ensuring backward compatibility and explicit control over key provisioning. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
The pull request introduces a new API key service that allows fetching API keys from AWS Secrets Manager or local environment variables, enhancing flexibility for multi-tenant data fetching. The implementation includes a caching mechanism for API keys and a clear priority for loading sources (AWS Secrets Manager over local environment variables). Comprehensive unit tests have been added for the new service and its integration into the CVec client, covering various scenarios including success, failure, and precedence rules. The poetry.lock and pyproject.toml files were updated to reflect the new boto3 dependency.
Overall, the changes are well-designed and thoroughly tested, significantly improving the API key management within the SDK.
| try: | ||
| self._api_key = get_api_key_for_host(self.host) | ||
| if self._api_key: | ||
| logger.info(f"API key loaded from mapping service for host: {self.host}") | ||
| except ValueError as e: | ||
| # Log the error from the service, but we'll raise our own error below | ||
| logger.debug(f"Failed to load API key from service: {e}") | ||
|
|
||
| if not self._api_key: | ||
| raise ValueError( | ||
| "CVEC_API_KEY must be set either as an argument or environment variable" | ||
| "CVEC_API_KEY must be set either as an argument, environment variable, " | ||
| "or available in the API keys mapping (AWS_API_KEYS_SECRET or API_KEYS_MAPPING)" | ||
| ) |
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.
The current error handling logs a specific ValueError from get_api_key_for_host at debug level and then raises a more generic ValueError. While the generic message is comprehensive, propagating the specific service error message can provide more immediate and actionable feedback to the user about why the API key could not be loaded from the service.
Consider including the service_error_message in the final ValueError to give more context.
| try: | |
| self._api_key = get_api_key_for_host(self.host) | |
| if self._api_key: | |
| logger.info(f"API key loaded from mapping service for host: {self.host}") | |
| except ValueError as e: | |
| # Log the error from the service, but we'll raise our own error below | |
| logger.debug(f"Failed to load API key from service: {e}") | |
| if not self._api_key: | |
| raise ValueError( | |
| "CVEC_API_KEY must be set either as an argument or environment variable" | |
| "CVEC_API_KEY must be set either as an argument, environment variable, " | |
| "or available in the API keys mapping (AWS_API_KEYS_SECRET or API_KEYS_MAPPING)" | |
| ) | |
| service_error_message = None | |
| try: | |
| self._api_key = get_api_key_for_host(self.host) | |
| if self._api_key: | |
| logger.info(f"API key loaded from mapping service for host: {self.host}") | |
| except ValueError as e: | |
| service_error_message = str(e) | |
| logger.debug(f"Failed to load API key from service: {service_error_message}") | |
| if not self._api_key: | |
| error_msg = ( | |
| "CVEC_API_KEY must be set either as an argument, environment variable, " | |
| "or available in the API keys mapping (AWS_API_KEYS_SECRET or API_KEYS_MAPPING)" | |
| ) | |
| if service_error_message: | |
| error_msg += f". Service error: {service_error_message}" | |
| raise ValueError(error_msg) |
5312bcd to
f4c6718
Compare
tenants using API_KEYS fetched via AWS Secret manager or env variable `API_KEYS_MAPPING` for local development. Once the mapping is fetched, it is cached. Providing the SDK with an `api_key` as argument to its initializer as we used to do takes precedence over the new added method. Some test cases make sure this is the case.
f4c6718 to
3742963
Compare
joshuanapoli
left a comment
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 we put the api_key_service somewhere else? The cvec-python SDK is also for customer access (and it is published publicly on PyPI). I don't think that they should see this internal key-distribution detail. We could create a private repo that extends the public SDK with private features.
We want to allow the SDK to fetch data from multiple tenants using API KEYS fetched via AWS Secret manager or env variable
API_KEYS_MAPPINGfor local development.Once the mapping is fetched, it is cached. Providing the SDK with an
api_keyas argument to its initializer takes precedence over the new added method. Some test cases make sure this is the case.