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

Refactor secrets stores to store all secret metadata in the DB #2193

Merged
merged 20 commits into from
Jan 12, 2024

Conversation

stefannica
Copy link
Contributor

@stefannica stefannica commented Dec 22, 2023

Describe changes

Refactor all secrets store to ONLY store secret values in the external back-end, as opposed to storing all the meta-information (UUID, name, user, workspace, create & update timestamps) in the external back-end.

  • the secret CRUD methods are moved to the SQL/REST zen store
  • the SQL zen store always keeps track of secrets in the ZenML database, but not their values
  • the secrets store is now an implementation detail belonging to the SQL zen store
  • the list_secrets method in particular doesn't need to interact with the external secrets manager anymore, which means that ZenML no longer needs "list all secrets" permissions from the Cloud Secrets Manager APIs.
  • the AWS/GCP/Azure/HashiCorp Secrets Manager are only called to store, update and delete the secret key-values for the ZenML secrets.
  • the SQL secrets store is a mere extension of the SQL Zen store now and will store/update/delete secret key-values in the ZenML database
  • the REST secrets store has been completely removed

This change is fully backwards-compatible. All existing secrets stored in external secrets stores will be added to the DB as part of the DB migration, while their values and their external secret IDs will continue to be stored externally. Existing users will not be aware that this change even happened.

TODO

  • DB migration script to populate the SQL DB for secrets currently stored in external back-ends

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have based my new branch on develop and the open PR is targeting develop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.
  • If my changes require changes to the dashboard, these changes are communicated/requested.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

Summary by CodeRabbit

  • Documentation

    • Updated the IAM user permissions guide for AWS S3 buckets to recommend specific rather than broad policies.
  • New Features

    • Introduced a new secret management interface with enhanced validation and configuration options.
    • Added support for handling secret values updates and deletions more effectively.
  • Refactor

    • Streamlined secret store configurations, focusing on environment-specific setups.
    • Removed deprecated methods and refactored existing ones for better secret handling across various platforms.
  • Bug Fixes

    • Fixed issues related to secret value data types to ensure proper handling of optional and required fields.
  • Deprecations

    • Deprecated certain methods in favor of new implementations for secret management.
  • Chores

    • Cleaned up unnecessary imports and outdated references across multiple files related to secret stores.

Copy link
Contributor

coderabbitai bot commented Dec 22, 2023

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Walkthrough

The overall change indicates a significant refactoring of the secrets management system within an application, likely ZenML. The adjustments suggest a shift towards more granular permission controls, particularly for AWS S3 buckets, and the standardization of secret handling across different secret stores. There's a clear move away from broad permissions to a more secure, permission-specific approach. The codebase has been streamlined by removing outdated methods, improving secret handling for SQL stores, and updating the secrets store interface.

Changes

File Path Change Summary
.../component-guide/artifact-stores/s3.md Updated IAM user permissions for S3 to specific actions from broad full access.
.../config/global_config.py Added check to warn about environment variables if the secrets store is not SQL.
.../config/store_config.py Introduced new imports and a root_validator for secrets store configuration.
src/zenml/enums.py Simplified SecretsStoreType enum values and removed docstring.
.../models/v2/core/secret.py Added methods for updating, setting, and removing secret values.
.../zen_server/deploy/deployer.py Removed RestSecretsStoreConfiguration usage and related parameter.
.../zen_stores/base_zen_store.py Removed SecretsStoreInterface inheritance and refactored secrets store handling.
.../zen_stores/rest_zen_store.py Added constants, new classes, and updated RestZenStoreConfiguration.
.../zen_stores/schemas/secret_schemas.py Changed values field to optional and updated methods for SQL secret handling.
.../zen_stores/secrets_stores/... General overhaul of secrets stores, removing outdated functionality and adding new methods.
.../zen_stores/zen_store_interface.py Added methods related to the lifecycle management of secrets.

*Note: The ... in the file paths represents abbreviated directory structures to condense the table for readability.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@github-actions github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Dec 22, 2023
)

return secret_model
logger.debug(f"Created AWS secret: {aws_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
# longer exists. Here we pretend that the secret does
# not exist.
continue
logger.debug(f"Fetched AWS secret: {aws_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
)

return secret_model
logger.debug(f"Updated AWS secret: {aws_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
@@ -849,3 +397,137 @@
raise RuntimeError(
f"Error deleting secret with ID {secret_id}: {e}"
)

logger.debug(f"Deleted AWS secret: {aws_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
workspace=workspace,
),
)
logger.debug(f"Created Azure secret: {azure_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
@@ -695,3 +382,90 @@
raise KeyError(f"Secret with ID {secret_id} not found")
except Exception as e:
raise RuntimeError(f"Failed to delete secret: {str(e)}") from e

logger.debug(f"Deleted GCP secret: {gcp_secret_name}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text.
except VaultError as e:
raise RuntimeError(f"Error creating secret: {e}")

logger.debug(f"Created HashiCorp Vault secret: {vault_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
@track_decorator(AnalyticsEvent.CREATED_SECRET)
def create_secret(self, secret: SecretRequest) -> SecretResponse:
"""Creates a new secret.
logger.debug(f"Fetched HashiCorp Vault secret: {vault_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.

return secret_model
logger.debug(f"Updated HashiCorp Vault secret: {vault_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
vault_secret,
hydrate=hydrate,
)
logger.debug(f"Deleted HashiCorp Vault secret: {vault_secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (secret)](1) as clear text. This expression logs [sensitive data (secret)](2) as clear text. This expression logs [sensitive data (secret)](3) as clear text. This expression logs [sensitive data (secret)](4) as clear text.
@stefannica
Copy link
Contributor Author

@coderabbitai review

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 10

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between fab3097 and c309bb6.
Files selected for processing (18)
  • docs/book/stacks-and-components/component-guide/artifact-stores/s3.md (1 hunks)
  • src/zenml/config/global_config.py (1 hunks)
  • src/zenml/config/store_config.py (2 hunks)
  • src/zenml/enums.py (1 hunks)
  • src/zenml/models/v2/core/secret.py (2 hunks)
  • src/zenml/zen_server/deploy/deployer.py (2 hunks)
  • src/zenml/zen_stores/base_zen_store.py (7 hunks)
  • src/zenml/zen_stores/rest_zen_store.py (8 hunks)
  • src/zenml/zen_stores/schemas/secret_schemas.py (4 hunks)
  • src/zenml/zen_stores/secrets_stores/aws_secrets_store.py (13 hunks)
  • src/zenml/zen_stores/secrets_stores/azure_secrets_store.py (9 hunks)
  • src/zenml/zen_stores/secrets_stores/base_secrets_store.py (10 hunks)
  • src/zenml/zen_stores/secrets_stores/gcp_secrets_store.py (10 hunks)
  • src/zenml/zen_stores/secrets_stores/hashicorp_secrets_store.py (7 hunks)
  • src/zenml/zen_stores/secrets_stores/secrets_store_interface.py (2 hunks)
  • src/zenml/zen_stores/secrets_stores/sql_secrets_store.py (3 hunks)
  • src/zenml/zen_stores/sql_zen_store.py (13 hunks)
  • src/zenml/zen_stores/zen_store_interface.py (2 hunks)
Files skipped from review due to trivial changes (1)
  • src/zenml/zen_server/deploy/deployer.py
Additional comments: 84
src/zenml/zen_stores/secrets_stores/secrets_store_interface.py (5)
  • 47-57: The method store_secret_values has been renamed and its signature updated. Ensure that all implementations of this interface and calls to this method are updated accordingly.

  • 60-71: The method get_secret_values has been refactored from the previous get_secret. Verify that the implementation and usage of this method reflect the new signature and intended behavior.

  • 75-88: The update_secret method has been split into update_secret_values and delete_secret_values. Confirm that the logic for updating and deleting secret values is correctly implemented in the respective methods.

  • 108-115: A new method list_secrets has been added. Verify that this method is implemented in all concrete classes and that it adheres to the intended functionality of listing secrets without their values.

  • 104-106: The old delete_secret method is deprecated. Ensure that it is no longer used in the codebase and that the new delete_secret_values method is used instead.

src/zenml/config/store_config.py (1)
  • 95-116: The root_validator method validate_secrets_store has been added to validate the secrets store configuration. Verify that this validation logic is correct and that it is being called appropriately during the configuration of a secrets store.
src/zenml/enums.py (1)
  • 122-128: The SecretsStoreType enum has been modified, with the SQL and REST values replaced by string literals. Confirm that these changes are reflected throughout the codebase and that the removal of the explanatory docstring does not reduce clarity.
src/zenml/zen_stores/schemas/secret_schemas.py (5)
  • 52-52: The values field in the SecretSchema class has been changed from a required bytes type to an optional bytes type. Verify that this change is consistent with the new approach to handling secret values and that it does not introduce any regressions.

  • 136-146: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [136-157]

The from_request method in the SecretSchema class has been modified to no longer store secret values implicitly. Confirm that the SQL secret store is now responsible for calling store_secret_values to store the values separately.

  • 160-178: The update method in the SecretSchema class has been modified to no longer update secret values implicitly. Ensure that the SQL secret store is now responsible for calling set_secret_values to update the values separately.

  • 199-258: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [183-210]

The to_model method in the SecretSchema class has been modified to no longer load secret values implicitly. Confirm that the SQL secret store is now responsible for calling get_secret_values to load the values separately.

  • 218-258: New methods get_secret_values and set_secret_values have been added to the SecretSchema class to handle secret values explicitly. Verify that these methods are implemented correctly and that they are being used appropriately by the SQL secret store.
src/zenml/zen_stores/secrets_stores/sql_secrets_store.py (5)
  • 175-203: The store_secret_values method has been added to the SqlSecretsStore class. Verify that this method correctly stores the secret values in the database and that it is being called in the appropriate places.

  • 205-226: The get_secret_values method has been added to the SqlSecretsStore class. Confirm that this method correctly retrieves the secret values from the database and that it is being used where necessary.

  • 228-243: The update_secret_values method has been added to the SqlSecretsStore class. Ensure that this method correctly updates the secret values in the database and that it is being called instead of the previous update method.

  • 245-263: The delete_secret_values method has been added to the SqlSecretsStore class. Verify that this method correctly deletes the secret values from the database and that it is being used in place of the previous delete method.

  • 269-285: The list_secrets method is deprecated in the SqlSecretsStore class. Confirm that this method is no longer being used and that the new method for listing secrets without their values is being used instead.

src/zenml/models/v2/core/secret.py (3)
  • 87-97: The get_secret_values_update method has been added to the SecretUpdate class. Verify that this method correctly returns a dictionary with the secret values to update and that it is being used appropriately.

  • 209-210: The remove_secrets method in the SecretUpdate class has been extended to include the removal of secret values while keeping the keys. Confirm that this change is consistent with the intended functionality and that it does not introduce any issues.

  • 212-218: The set_secrets method has been introduced in the SecretUpdate class to set the secret values of the secret. Ensure that this method is implemented correctly and that it is being used where necessary.

src/zenml/zen_stores/secrets_stores/base_secrets_store.py (10)
  • 14-26: The import statements are standard and necessary for the functionality of the base secrets store.

  • 30-40: The import statements and class imports are appropriate for the secrets store functionality.

  • 53-59: The BaseSecretsStore class is correctly defined as an abstract base class for secrets stores.

  • 88-93: The __init__ method for initializing the secrets store is correctly implemented with proper exception handling.

  • 150-155: The get_store_class method correctly returns the appropriate class for the secrets store based on the configuration.

  • 229-230: The zen_store property is correctly implemented with proper error handling to ensure the store is initialized.

  • 263-270: The _get_secret_metadata and _verify_secret_metadata methods are correctly implemented to handle metadata associated with secrets.

  • 312-319: The _create_secret_from_metadata method is marked as deprecated and is kept for migration purposes. Ensure that the deprecation is communicated and migration is tracked.

  • 355-356: The continuation of the _create_secret_from_metadata method is correctly implemented with proper exception handling.

  • 383-386: The _create_secret_from_metadata method correctly returns a SecretResponse object, encapsulating the secret data.

src/zenml/zen_stores/secrets_stores/azure_secrets_store.py (9)
  • 15-20: The import statements are standard and necessary for the functionality of the Azure secrets store.

  • 32-40: The import statements and class imports are appropriate for the Azure secrets store functionality.

  • 44-50: The class imports for SecretResponse and the logger initialization are correctly implemented.

  • 127-138: The AzureSecretsStore class is correctly defined with a comment explaining the implementation highlights.

  • 208-230: The store_secret_values method is correctly implemented to store secret values in Azure Key Vault.

  • 236-256: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [241-273]

The get_secret_values method is correctly implemented to retrieve secret values from Azure Key Vault.

  • 291-315: The update_secret_values method is correctly implemented to update secret values in Azure Key Vault.

  • 326-342: The delete_secret_values method is correctly implemented to delete secret values from Azure Key Vault.

  • 357-410: The _convert_azure_secret and list_secrets methods are correctly implemented for secret conversion and listing. Ensure that the deprecation is communicated and migration is tracked.

src/zenml/zen_stores/secrets_stores/gcp_secrets_store.py (9)
  • 15-29: The import statements are standard and necessary for the functionality of the GCP secrets store.

  • 32-40: The import statements and class imports are appropriate for the GCP secrets store functionality.

  • 44-50: The class imports for SecretResponse and the logger initialization are correctly implemented.

  • 15-29: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [127-138]

The GCPSecretsStore class is correctly defined with a comment explaining the implementation highlights.

  • 202-222: The store_secret_values method is correctly implemented to store secret values in GCP Secrets Manager.

  • 230-263: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [248-273]

The get_secret_values method is correctly implemented to retrieve secret values from GCP Secrets Manager.

  • 291-340: The update_secret_values method is correctly implemented to update secret values in GCP Secrets Manager.

  • 362-375: The delete_secret_values method is correctly implemented to delete secret values from GCP Secrets Manager.

  • 392-441: The _convert_gcp_secret and list_secrets methods are correctly implemented for secret conversion and listing. Ensure that the deprecation is communicated and migration is tracked.

docs/book/stacks-and-components/component-guide/artifact-stores/s3.md (1)
  • 223-227: The documentation now correctly advises on granting minimal necessary permissions (s3:PutObject, s3:GetObject, s3:ListBucket, s3:DeleteObject) for IAM users, which aligns with the principle of least privilege and is a security best practice.
src/zenml/zen_stores/base_zen_store.py (3)
  • 65-69: The removal of SecretsStoreInterface from BaseZenStore aligns with the PR's objective to refactor secrets management. Ensure that all references and usages of the interface methods are also updated accordingly.

  • 303-313: The secrets_store property now raises a NotImplementedError if no secrets store is configured. This is a clear and appropriate way to handle the case where a secrets store is expected but not available.

  • 65-69: The removal of event handling methods (register_event_handler and _trigger_event) should be checked to ensure that all event-related functionality has been appropriately refactored or is no longer required.

src/zenml/zen_stores/secrets_stores/hashicorp_secrets_store.py (6)
  • 184-213: The store_secret_values method has been updated to store secret values and metadata in HashiCorp Vault. Ensure that the cas parameter is set correctly to prevent overwriting existing secrets and that the metadata is being stored as intended.

  • 219-273: The get_secret_values method now retrieves secret values and metadata from HashiCorp Vault. Verify that the error handling is robust and that the method correctly distinguishes between different types of errors (e.g., secret not found vs. Vault error).

  • 278-312: The update_secret_values method has been refactored to update secret values in HashiCorp Vault. Ensure that the method correctly updates the secret values and metadata, and that it handles errors appropriately.

  • 316-338: The delete_secret_values method has been refactored to delete secret values in HashiCorp Vault. Verify that the method correctly deletes the secret and all its versions, and that it provides clear error messages when the secret does not exist or other errors occur.

  • 343-387: The _convert_vault_secret method is deprecated and retained for migration from version 0.53.0. Ensure that this deprecation is clearly communicated to developers and that the method is not used in new code.

  • 181-404: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [389-456]

The list_secrets method has been updated to list all secrets without including secret values. Verify that the method correctly lists secrets and handles errors appropriately, such as when the Vault API returns an unexpected error.

src/zenml/zen_stores/secrets_stores/aws_secrets_store.py (14)
  • 16-18: The imports for json and datetime are still present, which implies they are used elsewhere in the file. Ensure that their usage aligns with the new architecture.

  • 29-31: The import of ClientError from botocore.exceptions is necessary for exception handling in AWS API calls, which is a good practice.

  • 41-42: The import of get_logger is used for logging, which is essential for debugging and tracking the flow of the application.

  • 59-62: The AWSSecretsStoreConfiguration class has had several attributes removed. Ensure that the removal of list_page_size and secret_list_refresh_timeout aligns with the new architecture and that these configurations are no longer necessary.

  • 66-67: The region property has been removed from the AWSSecretsStoreConfiguration class. Verify that the region is now being handled appropriately elsewhere in the configuration, possibly through the auth_config.

  • 117-118: The Config class within AWSSecretsStoreConfiguration has been modified to allow extra attributes. This change could be to maintain backward compatibility with configurations from previous versions. Ensure that this does not introduce any security or stability issues.

  • 135-138: The comments have been updated to reflect the new metadata storage strategy. Ensure that the actual implementation of metadata storage in the AWS tags is consistent with these comments.

  • 221-235: The store_secret_values method has been added. Ensure that this method correctly implements the logic for storing secret values in AWS Secrets Manager, including proper error handling and logging.

  • 254-268: The get_secret_values method has been added. Verify that it correctly fetches secret values from AWS Secrets Manager and handles errors as expected.

  • 324-340: The update_secret_values method has been added. Verify that it correctly updates secret values in AWS Secrets Manager and handles potential errors.

  • 366-377: The delete_secret_values method has been added. Verify that it correctly deletes secret values from AWS Secrets Manager and handles errors, especially the immediate deletion without recovery.

  • 407-438: The _convert_aws_secret method has been marked as deprecated. Ensure that the deprecation is part of a managed transition to the new architecture and that there is a clear migration path for existing secrets.

  • 441-469: The _get_aws_secret_filters method has been marked as deprecated. Verify that the deprecation is part of a managed transition to the new architecture and that there is a clear migration path for existing secrets.

  • 473-531: The list_secrets method has been marked as deprecated. Ensure that the deprecation is part of a managed transition to the new architecture and that there is a clear migration path for existing secrets.

src/zenml/config/global_config.py (1)
  • 574-588: The conditional check added warns and ignores environment variable configuration for secrets stores if the type is not SQL. This is a good practice to ensure that the configuration is only applied when appropriate. Verify that this logic is consistent with the overall architecture and that it does not introduce any unexpected behavior.
src/zenml/zen_stores/zen_store_interface.py (6)
  • 87-90: The addition of SecretFilter, SecretRequest, SecretResponse, and SecretUpdate classes aligns with the PR objective to refactor secrets management. Ensure that these classes are used consistently across the codebase and that their fields are properly validated.
Verification successful

It appears that the rg command did not recognize the file type python. This could be due to a misconfiguration or an issue with the version of ripgrep being used. To ensure we can search for the usage of the new classes across the Python files in the codebase, we should use a different approach.

Let's try using rg without specifying the file type, as ripgrep will automatically search through text files, which should include Python files by default.


The search results confirm that the new classes SecretFilter, SecretRequest, SecretResponse, and SecretUpdate are being used throughout the codebase, particularly in the zen_stores and models modules, as well as in the integration tests and documentation. This indicates that the additions are consistent with the PR's objective to refactor secrets management and are integrated into the system.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the new classes are used in the methods that manage secrets.
rg --type python 'SecretFilter|SecretRequest|SecretResponse|SecretUpdate'

Length of output: 106



Script:

#!/bin/bash
# Search for the new classes across all files, as ripgrep will include text files by default.
rg 'SecretFilter|SecretRequest|SecretResponse|SecretUpdate'

Length of output: 26515

* 1323-1350: The `create_secret` method includes comprehensive documentation and raises appropriate exceptions. Verify that the scoping rules enforced in the secrets store are correctly implemented and tested.
Verification successful

The verification process has confirmed that the create_secret method is implemented in multiple files within the codebase, including client.py, rest_zen_store.py, sql_zen_store.py, zen_store_interface.py, workspaces_endpoints.py, and secret.py. Additionally, the presence of unit tests for the create_secret method has been established, with several tests found in test_client.py and test_secret.py that cover default scope, user scope, existing name scope, and different scopes.

Based on the output, it is evident that the scoping rules for the create_secret method are likely to be implemented given the method's presence in various parts of the codebase, and the existence of multiple tests suggests that these rules are also being tested.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the scoping rules are implemented and covered by unit tests.
rg --type python 'create_secret'
rg --type python 'test_create_secret'

Length of output: 132



Script:

#!/bin/bash
# Search for the implementation of the create_secret method to verify scoping rules.
rg 'def create_secret'

# Search for unit tests related to the create_secret method.
rg 'def test_create_secret'

Length of output: 1292

* 1352-1368: The `get_secret` method should ensure that the `hydrate` flag is respected in the implementation. Confirm that the method correctly handles the hydration of the secret's metadata.
  • 1370-1391: The list_secrets method's documentation clearly states that secret values are not included in the response. This is a good security practice. Ensure that the implementation adheres to this specification and that pagination is handled correctly.

  • 1393-1425: The update_secret method's logic for handling secret values appears sound, with clear rules for merging and removing values. Ensure that the implementation follows these rules and that the scoping rules are validated.

  • 1427-1436: The delete_secret method is straightforward, but ensure that any cascading effects of deleting a secret are handled (e.g., updating configurations that relied on the deleted secret).

src/zenml/zen_stores/rest_zen_store.py (4)
  • 71-71: The addition of the SECRETS constant is consistent with the PR's objective to refactor secrets management. This constant likely serves as a route for REST API calls related to secrets.

  • 166-169: The addition of SecretFilter, SecretRequest, SecretResponse, and SecretUpdate classes aligns with the PR's objective to refactor the secrets management system. These classes are likely used to handle the new structure of secrets within the ZenML framework.

  • 245-252: The removal of the secrets_store attribute from the RestZenStoreConfiguration class is consistent with the PR's objective to change how secrets are managed. This change likely reflects the shift to storing secret metadata in the ZenML database.

  • 1643-1761: The addition of methods for managing secrets (create_secret, get_secret, list_secrets, update_secret, delete_secret) in the RestZenStore class is a critical part of the PR's objective to refactor secrets management. These methods will facilitate the CRUD operations for secrets within the new system architecture.

src/zenml/zen_stores/sql_zen_store.py (1)
  • 3557-3627: The method _check_sql_secret_scope provides a clear mechanism for checking the existence of a secret within a given scope. It's important to ensure that the error message returned when a secret already exists is informative and actionable. The current message fulfills this requirement.

src/zenml/zen_stores/sql_zen_store.py Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Outdated Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Show resolved Hide resolved
src/zenml/zen_stores/sql_zen_store.py Show resolved Hide resolved
@stefannica stefannica force-pushed the feature/cloud-479-store-secrets-metadata-in-db branch 2 times, most recently from 9d64a4a to 932927e Compare December 22, 2023 22:47
@stefannica stefannica changed the title [WIP] Refactor secrets stores to store all secret metadata in the DB Refactor secrets stores to store all secret metadata in the DB Dec 22, 2023
@stefannica stefannica force-pushed the feature/cloud-479-store-secrets-metadata-in-db branch from caf0702 to aaa3c0a Compare January 8, 2024 10:00
ValueError: If the user account credentials JSON is not a valid
JSON object.
"""
if isinstance(values.get("user_account_json"), dict):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just something left over from the "use service connectors to authenticate secrets stores" PR and revealed during testing this PR.

Returns:
The validated configuration values.
"""
if isinstance(values.get("service_account_json"), dict):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just something left over from the "use service connectors to authenticate secrets stores" PR and revealed during testing this PR.


def upgrade() -> None:
"""Upgrade database schema and/or data, creating a new revision."""
from zenml.config.global_config import GlobalConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

I commented something similar on a cloudapi PR before where we decided importing these classes in migrations is okay as it will just run once.

I think here it is a little more complicated: This migration will also run in zenml 0.75.x in a few months/years from now, which means we need to keep those classes and all their (deprecated) methods around. Do you think this will lead to any trouble in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely correct, we have to keep a lot of deprecated code around, even though it's only used in a DB migration script. Specifically, I'm talking about the deprecated "list_secrets" method in each secrets store implementation. We have two inter-related situations that I need to address here:

  1. getting rid of deprecated code: this is a bit laborious, because I'd have to retest the migration again for all possible store types, but easy. I just have to move all those functions and their dependents inside the migration script.
  2. getting rid of all imports in the migration script: technically impossible. These imports are strictly required to allow the migration script to connect to the external secrets store back-end. This needs to happen today, tomorrow, a few months from now or even a few years from now. As long as we have to support direct upgrades from 0.50.0 or earlier versions. On the bright side, we're only importing major concepts here: the global configuration and the SQL store. As long as we don't call attributes or methods prone to removal or deprecation on these objects (see the previous point), this should be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the point is, we won't be able to remove those classes anyway as long as we want to support upgrading from pre-0.50 right? In that case I guess that's the best case we have, even though ideally this would not even be an automated migration in my opinion but rather something users do manually as there might be multiple reasons for this to fail.

@stefannica stefannica force-pushed the feature/cloud-479-store-secrets-metadata-in-db branch from 5a74645 to b9766fb Compare January 10, 2024 18:56
@stefannica stefannica force-pushed the feature/cloud-479-store-secrets-metadata-in-db branch from 1aaf15d to fff470c Compare January 11, 2024 16:44
@stefannica stefannica merged commit 1edad92 into develop Jan 12, 2024
35 checks passed
@stefannica stefannica deleted the feature/cloud-479-store-secrets-metadata-in-db branch January 12, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal To filter out internal PRs and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants