Fix zenml deploy secret stores #2454
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent changes involve refactoring the logic for handling authentication in both AWS and GCP secrets stores. The updates focus on improving how Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
safoinme
added
run-slow-ci
and removed
internal
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if values.get("project_id"): | ||
| if not os.environ.get("GOOGLE_APPLICATION_CREDENTIALS"): | ||
| logger.warning( | ||
| "The `GOOGLE_APPLICATION_CREDENTIALS` environment variable " | ||
| "is not set. using an implicit GCP authentication to access " | ||
| "the GCP Secrets Manager API." | ||
| ) | ||
| values["auth_method"] = GCPAuthenticationMethods.IMPLICIT | ||
| values["auth_config"] = dict( | ||
| project_id=values.get("project_id"), | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "The `project_id` GCP secrets store attribute and the " | ||
| "`GOOGLE_APPLICATION_CREDENTIALS` environment variable are " | ||
| "deprecated and will be removed in a future version of ZenML. " | ||
| "Please use the `auth_method` and `auth_config` attributes " | ||
| "instead." | ||
| ) | ||
| values[ | ||
| "auth_method" | ||
| ] = GCPAuthenticationMethods.SERVICE_ACCOUNT | ||
| values["auth_config"] = dict( | ||
| project_id=values.get("project_id"), | ||
| ) | ||
| # Load the service account credentials from the file | ||
| with open(os.environ["GOOGLE_APPLICATION_CREDENTIALS"]) as f: | ||
| values["auth_config"]["service_account_json"] = f.read() |
There was a problem hiding this comment.
The implementation of the conditional logic for handling project_id and GOOGLE_APPLICATION_CREDENTIALS in the populate_config method introduces a clear and understandable approach to defaulting to implicit GCP authentication when necessary. However, there are a few areas that could be refined:
-
Deprecation Warning Clarity: The deprecation warning message could be more specific about what is deprecated and provide a clear action for users to migrate to the new method. It's good practice to include a version number when mentioning deprecation to give users a timeline.
-
Error Handling for File Reading: When reading the
GOOGLE_APPLICATION_CREDENTIALSfile, there's no explicit error handling for potential issues like file not found or permission errors. Wrapping this in a try-except block could improve robustness. -
Security Consideration: Loading and storing the entire service account JSON file in
auth_configcould potentially expose sensitive information. Ensure that this information is handled securely throughout its lifecycle and consider if there are ways to minimize the exposure of sensitive data. -
Performance and Efficiency: The current implementation is straightforward and should not introduce significant performance concerns. However, always consider the implications of file I/O operations and environmental variable access in terms of performance, especially in high-throughput environments.
- "The `project_id` GCP secrets store attribute and the "
- "`GOOGLE_APPLICATION_CREDENTIALS` environment variable are "
- "deprecated and will be removed in a future version of ZenML. "
+ "Using `project_id` directly with `GOOGLE_APPLICATION_CREDENTIALS` is deprecated and will be removed in ZenML version X.Y.Z. "
+ "Migrate to using `auth_method` and `auth_config` for future compatibility."Consider adding error handling for reading the GOOGLE_APPLICATION_CREDENTIALS file to gracefully handle potential issues.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if values.get("project_id"): | |
| if not os.environ.get("GOOGLE_APPLICATION_CREDENTIALS"): | |
| logger.warning( | |
| "The `GOOGLE_APPLICATION_CREDENTIALS` environment variable " | |
| "is not set. using an implicit GCP authentication to access " | |
| "the GCP Secrets Manager API." | |
| ) | |
| values["auth_method"] = GCPAuthenticationMethods.IMPLICIT | |
| values["auth_config"] = dict( | |
| project_id=values.get("project_id"), | |
| ) | |
| else: | |
| logger.warning( | |
| "The `project_id` GCP secrets store attribute and the " | |
| "`GOOGLE_APPLICATION_CREDENTIALS` environment variable are " | |
| "deprecated and will be removed in a future version of ZenML. " | |
| "Please use the `auth_method` and `auth_config` attributes " | |
| "instead." | |
| ) | |
| values[ | |
| "auth_method" | |
| ] = GCPAuthenticationMethods.SERVICE_ACCOUNT | |
| values["auth_config"] = dict( | |
| project_id=values.get("project_id"), | |
| ) | |
| # Load the service account credentials from the file | |
| with open(os.environ["GOOGLE_APPLICATION_CREDENTIALS"]) as f: | |
| values["auth_config"]["service_account_json"] = f.read() | |
| if values.get("project_id"): | |
| if not os.environ.get("GOOGLE_APPLICATION_CREDENTIALS"): | |
| logger.warning( | |
| "The `GOOGLE_APPLICATION_CREDENTIALS` environment variable " | |
| "is not set. using an implicit GCP authentication to access " | |
| "the GCP Secrets Manager API." | |
| ) | |
| values["auth_method"] = GCPAuthenticationMethods.IMPLICIT | |
| values["auth_config"] = dict( | |
| project_id=values.get("project_id"), | |
| ) | |
| else: | |
| logger.warning( | |
| "Using `project_id` directly with `GOOGLE_APPLICATION_CREDENTIALS` is deprecated and will be removed in ZenML version X.Y.Z. " | |
| "Migrate to using `auth_method` and `auth_config` for future compatibility." | |
| ) | |
| values[ | |
| "auth_method" | |
| ] = GCPAuthenticationMethods.SERVICE_ACCOUNT | |
| values["auth_config"] = dict( | |
| project_id=values.get("project_id"), | |
| ) | |
| # Load the service account credentials from the file | |
| with open(os.environ["GOOGLE_APPLICATION_CREDENTIALS"]) as f: | |
| values["auth_config"]["service_account_json"] = f.read() |
| if values.get("region_name"): | ||
| if not values.get("aws_access_key_id") or not values.get( | ||
| "aws_secret_access_key" | ||
| ): | ||
| logger.warning( | ||
| "The `aws_access_key_id` and `aws_secret_access_key` is not " | ||
| "set. Using an implicit authentication method for AWS Secrets " | ||
| ) | ||
| values["auth_method"] = AWSAuthenticationMethods.IMPLICIT | ||
| values["auth_config"] = dict( | ||
| region=values.get("region_name"), | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "The `aws_access_key_id`, `aws_secret_access_key` and " | ||
| "`region_name` AWS secrets store attributes are deprecated and " | ||
| "will be removed in a future version of ZenML. Please use the " | ||
| "`auth_method` and `auth_config` attributes instead." | ||
| ) | ||
| values["auth_method"] = AWSAuthenticationMethods.SECRET_KEY | ||
| values["auth_config"] = dict( | ||
| aws_access_key_id=values.get("aws_access_key_id"), | ||
| aws_secret_access_key=values.get("aws_secret_access_key"), | ||
| region=values.get("region_name"), | ||
| ) |
There was a problem hiding this comment.
The implementation of the conditional logic for handling AWS secrets store attributes in the populate_config method introduces a clear approach to defaulting to implicit AWS authentication when necessary. Similar to the GCP secrets store, there are areas that could be refined:
-
Deprecation Warning Clarity: As with the GCP implementation, the deprecation warning could benefit from being more specific about the deprecated features and providing a clear migration path for users, including a version number for the deprecation timeline.
-
Security Consideration: The handling of AWS credentials (
aws_access_key_idandaws_secret_access_key) should always be done with security in mind. Ensure that these credentials are not logged or exposed inappropriately within the application. -
Best Practices for Configuration Handling: The method of dynamically adjusting the
auth_configbased on the presence of certain keys is practical. However, ensure that this dynamic configuration does not lead to confusion or misconfiguration by clearly documenting how these settings are intended to be used.
- "The `aws_access_key_id`, `aws_secret_access_key` and "
- "`region_name` AWS secrets store attributes are deprecated and "
- "will be removed in a future version of ZenML. "
+ "Using `aws_access_key_id`, `aws_secret_access_key`, and `region_name` directly is deprecated and will be removed in ZenML version X.Y.Z. "
+ "Migrate to using `auth_method` and `auth_config` for future compatibility."Ensure that AWS credentials are handled securely throughout their lifecycle, avoiding logging or exposing them inappropriately.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if values.get("region_name"): | |
| if not values.get("aws_access_key_id") or not values.get( | |
| "aws_secret_access_key" | |
| ): | |
| logger.warning( | |
| "The `aws_access_key_id` and `aws_secret_access_key` is not " | |
| "set. Using an implicit authentication method for AWS Secrets " | |
| ) | |
| values["auth_method"] = AWSAuthenticationMethods.IMPLICIT | |
| values["auth_config"] = dict( | |
| region=values.get("region_name"), | |
| ) | |
| else: | |
| logger.warning( | |
| "The `aws_access_key_id`, `aws_secret_access_key` and " | |
| "`region_name` AWS secrets store attributes are deprecated and " | |
| "will be removed in a future version of ZenML. Please use the " | |
| "`auth_method` and `auth_config` attributes instead." | |
| ) | |
| values["auth_method"] = AWSAuthenticationMethods.SECRET_KEY | |
| values["auth_config"] = dict( | |
| aws_access_key_id=values.get("aws_access_key_id"), | |
| aws_secret_access_key=values.get("aws_secret_access_key"), | |
| region=values.get("region_name"), | |
| ) | |
| if values.get("region_name"): | |
| if not values.get("aws_access_key_id") or not values.get( | |
| "aws_secret_access_key" | |
| ): | |
| logger.warning( | |
| "The `aws_access_key_id` and `aws_secret_access_key` is not " | |
| "set. Using an implicit authentication method for AWS Secrets " | |
| ) | |
| values["auth_method"] = AWSAuthenticationMethods.IMPLICIT | |
| values["auth_config"] = dict( | |
| region=values.get("region_name"), | |
| ) | |
| else: | |
| logger.warning( | |
| "Using `aws_access_key_id`, `aws_secret_access_key`, and `region_name` directly is deprecated and will be removed in ZenML version X.Y.Z. " | |
| "Migrate to using `auth_method` and `auth_config` for future compatibility." | |
| ) | |
| values["auth_method"] = AWSAuthenticationMethods.SECRET_KEY | |
| values["auth_config"] = dict( | |
| aws_access_key_id=values.get("aws_access_key_id"), | |
| aws_secret_access_key=values.get("aws_secret_access_key"), | |
| region=values.get("region_name"), | |
| ) |
Co-authored-by: Stefan Nica <stefan@zenml.io>
* Refactor AWS and GCP secrets store configuration * Apply suggestions from code review Co-authored-by: Stefan Nica <stefan@zenml.io> --------- Co-authored-by: Stefan Nica <stefan@zenml.io>
Describe changes
I implemented/fixed _ to achieve _.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit