redisenterprise: add test-connection command to address a common supportability ask #9490
Conversation
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| redisenterprise test-connection | cmd redisenterprise test-connection added |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
Hi @yugangw-msft, |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
CodeGen Tools Feedback CollectionThank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey |
Release SuggestionsModule: redisenterprise
Notes
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new test-connection command to the Azure CLI redisenterprise extension to help users verify connectivity to Redis Enterprise clusters. This addresses a common supportability request by providing a diagnostic tool that can test connections using either Entra ID authentication or access key authentication.
Key changes:
- Adds a new
test-connectioncommand that performs write/read/delete operations to verify Redis connectivity - Implements support for two authentication methods: Entra ID and access keys
- Adds the redis package as a dependency to enable Redis client functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/redisenterprise/setup.py | Adds redis package dependency for Redis client functionality |
| src/redisenterprise/azext_redisenterprise/custom.py | Implements connection testing logic with helper functions for creating Redis connections and performing test operations |
| src/redisenterprise/azext_redisenterprise/commands.py | Registers the new test-connection command |
| src/redisenterprise/azext_redisenterprise/_params.py | Defines command parameters including authentication method options |
| src/redisenterprise/azext_redisenterprise/_help.py | Adds help documentation and usage examples for the new command |
| ] | ||
|
|
||
| DEPENDENCIES = [] | ||
| DEPENDENCIES = ['redis~=7.1.0'] |
There was a problem hiding this comment.
The dependency specification uses a pessimistic version constraint (~=7.1.0) which will match 7.1.x versions but not 7.2.0 or higher. This may be overly restrictive and could prevent users from benefiting from bug fixes and improvements in newer versions. Consider using a more flexible constraint like 'redis>=7.1.0,<8.0.0' to allow minor version updates while preventing breaking changes from major version updates.
| DEPENDENCIES = ['redis~=7.1.0'] | |
| DEPENDENCIES = ['redis>=7.1.0,<8.0.0'] |
| # The password is the access token itself | ||
| import jwt | ||
| decoded_token = jwt.decode(access_token, options={"verify_signature": False}) | ||
| user_name = decoded_token.get('oid', decoded_token.get('sub', 'default')) |
There was a problem hiding this comment.
The username is extracted from the JWT token's 'oid' or 'sub' claim with a fallback to 'default'. Using 'default' as a fallback username could lead to authentication failures or unexpected behavior. If neither 'oid' nor 'sub' is present in the token, this indicates a token validation issue that should fail explicitly with a clear error message rather than silently using a placeholder value.
| user_name = decoded_token.get('oid', decoded_token.get('sub', 'default')) | |
| user_name = decoded_token.get('oid') or decoded_token.get('sub') | |
| if not user_name: | |
| raise ValidationError("Failed to extract user identifier ('oid' or 'sub') from Entra access token.") |
| logger.warning("Connecting with Entra ID user (oid): %s", user_name) | ||
|
|
||
| redis_client = _get_redis_connection( | ||
| host_name=host_name, | ||
| port=port, | ||
| password=access_token, | ||
| ssl=True, | ||
| username=user_name | ||
| ) | ||
|
|
||
| logger.warning("Successfully connected to Redis at %s:%s", host_name, port) |
There was a problem hiding this comment.
The warning message uses logger.warning for informational connection status messages. Similar to earlier in the code, this should use logger.info for informational messages about successful operations rather than logger.warning which implies a problem or potential issue.
| result['message'] = (f"Required package not installed: {str(ie)}. " | ||
| "Please install 'redis' and 'PyJWT' packages.") |
There was a problem hiding this comment.
The error message mentions installing both 'redis' and 'PyJWT' packages, but only 'redis' is listed in DEPENDENCIES. Since PyJWT is required for Entra authentication, it should be added to the dependencies or the error message should be updated to clarify that it's an optional dependency for Entra auth.
| result['message'] = (f"Required package not installed: {str(ie)}. " | |
| "Please install 'redis' and 'PyJWT' packages.") | |
| result['message'] = ( | |
| f"Required package not installed: {str(ie)}. " | |
| "Please install the 'redis' package. To enable Entra authentication, " | |
| "also install the optional 'PyJWT' package." | |
| ) |
| database_name = first_database.get('name', 'default') | ||
| port = first_database.get('port', 10000) | ||
|
|
||
| logger.warning("Using database: %s", database_name) |
There was a problem hiding this comment.
Using logger.warning for an informational message about which database is being used. This is normal operational information and should use logger.info instead. The warning log level should be reserved for actual warnings about potential issues.
| logger.warning("Using database: %s", database_name) | |
| logger.info("Using database: %s", database_name) |
| username=username, | ||
| password=password, | ||
| ssl=ssl, | ||
| ssl_cert_reqs=None, |
There was a problem hiding this comment.
The ssl_cert_reqs parameter is set to None, which disables SSL certificate verification. This is a security risk as it makes the connection vulnerable to man-in-the-middle attacks. Unless there's a specific reason for this (which should be documented), SSL certificate verification should be enabled by using ssl_cert_reqs='required' or removing this parameter to use the default secure setting.
| ssl_cert_reqs=None, | |
| ssl_cert_reqs="required", |
| test_logger.warning("Step 1: Writing test key '%s' with value '%s'...", test_key, test_value) | ||
| redis_client.set(test_key, test_value, ex=60) # Expire in 60 seconds | ||
| steps.append({'step': 1, 'action': 'write', 'status': 'success', 'key': test_key, 'value': test_value, | ||
| 'message': f"Successfully wrote key '{test_key}'"}) | ||
| test_logger.warning("Step 1: Successfully wrote test key") | ||
|
|
||
| # Step 2: Read the test key back | ||
| test_logger.warning("Step 2: Reading test key '%s'...", test_key) | ||
| retrieved_value = redis_client.get(test_key) | ||
| steps.append({'step': 2, 'action': 'read', 'status': 'success', 'key': test_key, 'value': retrieved_value, | ||
| 'message': f"Successfully read key '{test_key}', value: '{retrieved_value}'"}) | ||
| test_logger.warning("Step 2: Successfully read test key, value: '%s'", retrieved_value) | ||
|
|
||
| # Step 3: Verify the value | ||
| test_logger.warning("Step 3: Verifying value matches...") | ||
| if retrieved_value == test_value: | ||
| steps.append({'step': 3, 'action': 'verify', 'status': 'success', | ||
| 'message': 'Value verification passed'}) | ||
| test_logger.warning("Step 3: Value verification passed") | ||
| else: | ||
| steps.append({'step': 3, 'action': 'verify', 'status': 'failed', | ||
| 'message': f"Value mismatch: expected '{test_value}', got '{retrieved_value}'"}) | ||
| test_logger.warning("Step 3: Value verification failed") | ||
| return False, f"Value mismatch: expected '{test_value}', got '{retrieved_value}'.", steps | ||
|
|
||
| # Step 4: Delete the test key | ||
| test_logger.warning("Step 4: Deleting test key '%s'...", test_key) | ||
| redis_client.delete(test_key) | ||
| steps.append({'step': 4, 'action': 'delete', 'status': 'success', 'key': test_key, | ||
| 'message': f"Successfully deleted key '{test_key}'"}) | ||
| test_logger.warning("Step 4: Successfully deleted test key") |
There was a problem hiding this comment.
Using the logger.warning level for informational messages about connection test steps is misleading. These are informational messages showing normal operation progress, not warnings about potential issues. Consider using logger.info for these step-by-step progress messages to better reflect their informational nature.
| test_logger.warning("Step 1: Writing test key '%s' with value '%s'...", test_key, test_value) | |
| redis_client.set(test_key, test_value, ex=60) # Expire in 60 seconds | |
| steps.append({'step': 1, 'action': 'write', 'status': 'success', 'key': test_key, 'value': test_value, | |
| 'message': f"Successfully wrote key '{test_key}'"}) | |
| test_logger.warning("Step 1: Successfully wrote test key") | |
| # Step 2: Read the test key back | |
| test_logger.warning("Step 2: Reading test key '%s'...", test_key) | |
| retrieved_value = redis_client.get(test_key) | |
| steps.append({'step': 2, 'action': 'read', 'status': 'success', 'key': test_key, 'value': retrieved_value, | |
| 'message': f"Successfully read key '{test_key}', value: '{retrieved_value}'"}) | |
| test_logger.warning("Step 2: Successfully read test key, value: '%s'", retrieved_value) | |
| # Step 3: Verify the value | |
| test_logger.warning("Step 3: Verifying value matches...") | |
| if retrieved_value == test_value: | |
| steps.append({'step': 3, 'action': 'verify', 'status': 'success', | |
| 'message': 'Value verification passed'}) | |
| test_logger.warning("Step 3: Value verification passed") | |
| else: | |
| steps.append({'step': 3, 'action': 'verify', 'status': 'failed', | |
| 'message': f"Value mismatch: expected '{test_value}', got '{retrieved_value}'"}) | |
| test_logger.warning("Step 3: Value verification failed") | |
| return False, f"Value mismatch: expected '{test_value}', got '{retrieved_value}'.", steps | |
| # Step 4: Delete the test key | |
| test_logger.warning("Step 4: Deleting test key '%s'...", test_key) | |
| redis_client.delete(test_key) | |
| steps.append({'step': 4, 'action': 'delete', 'status': 'success', 'key': test_key, | |
| 'message': f"Successfully deleted key '{test_key}'"}) | |
| test_logger.warning("Step 4: Successfully deleted test key") | |
| test_logger.info("Step 1: Writing test key '%s' with value '%s'...", test_key, test_value) | |
| redis_client.set(test_key, test_value, ex=60) # Expire in 60 seconds | |
| steps.append({'step': 1, 'action': 'write', 'status': 'success', 'key': test_key, 'value': test_value, | |
| 'message': f"Successfully wrote key '{test_key}'"}) | |
| test_logger.info("Step 1: Successfully wrote test key") | |
| # Step 2: Read the test key back | |
| test_logger.info("Step 2: Reading test key '%s'...", test_key) | |
| retrieved_value = redis_client.get(test_key) | |
| steps.append({'step': 2, 'action': 'read', 'status': 'success', 'key': test_key, 'value': retrieved_value, | |
| 'message': f"Successfully read key '{test_key}', value: '{retrieved_value}'"}) | |
| test_logger.info("Step 2: Successfully read test key, value: '%s'", retrieved_value) | |
| # Step 3: Verify the value | |
| test_logger.info("Step 3: Verifying value matches...") | |
| if retrieved_value == test_value: | |
| steps.append({'step': 3, 'action': 'verify', 'status': 'success', | |
| 'message': 'Value verification passed'}) | |
| test_logger.info("Step 3: Value verification passed") | |
| else: | |
| steps.append({'step': 3, 'action': 'verify', 'status': 'failed', | |
| 'message': f"Value mismatch: expected '{test_value}', got '{retrieved_value}'"}) | |
| test_logger.info("Step 3: Value verification failed") | |
| return False, f"Value mismatch: expected '{test_value}', got '{retrieved_value}'.", steps | |
| # Step 4: Delete the test key | |
| test_logger.info("Step 4: Deleting test key '%s'...", test_key) | |
| redis_client.delete(test_key) | |
| steps.append({'step': 4, 'action': 'delete', 'status': 'success', 'key': test_key, | |
| 'message': f"Successfully deleted key '{test_key}'"}) | |
| test_logger.info("Step 4: Successfully deleted test key") |
| import jwt | ||
| decoded_token = jwt.decode(access_token, options={"verify_signature": False}) |
There was a problem hiding this comment.
The PyJWT dependency is used to decode JWT tokens but is not listed in the DEPENDENCIES in setup.py. This will cause ImportError at runtime if PyJWT is not installed. Add 'PyJWT' to the DEPENDENCIES list in setup.py to ensure it's installed along with the extension.
| ssl=True | ||
| ) | ||
|
|
||
| logger.warning("Successfully connected to Redis at %s:%s", host_name, port) |
There was a problem hiding this comment.
The warning message uses logger.warning for an informational connection status message. This should use logger.info for informational messages about successful operations.
| logger.warning("Successfully connected to Redis at %s:%s", host_name, port) | |
| logger.info("Successfully connected to Redis at %s:%s", host_name, port) |
| def redisenterprise_test_connection(cmd, | ||
| resource_group_name, | ||
| cluster_name, | ||
| auth): | ||
| """ | ||
| Test connection to a Redis Enterprise cluster using the specified authentication method. | ||
| :param cmd: The command instance. | ||
| :param resource_group_name: The name of the resource group. | ||
| :param cluster_name: The name of the Redis Enterprise cluster. | ||
| :param auth: The authentication method to use ('entra' or 'access-key'). | ||
| :return: Connection test result. | ||
| """ | ||
| # Get cluster information | ||
| cluster = _ClusterShow(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| "cluster_name": cluster_name, | ||
| "resource_group": resource_group_name}) | ||
|
|
||
| if not cluster: | ||
| raise ValidationError(f"Cluster '{cluster_name}' not found in resource group '{resource_group_name}'.") | ||
|
|
||
| # Get the hostname from the cluster | ||
| host_name = cluster.get('hostName') | ||
| if not host_name: | ||
| raise ValidationError(f"Unable to retrieve hostname for cluster '{cluster_name}'.") | ||
|
|
||
| # Get the list of databases in the cluster | ||
| databases = list(_DatabaseList(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| "cluster_name": cluster_name, | ||
| "resource_group": resource_group_name})) | ||
|
|
||
| if not databases: | ||
| raise ValidationError(f"No databases found in cluster '{cluster_name}'. " | ||
| "Please create a database before testing the connection.") | ||
|
|
||
| # Use the first database | ||
| first_database = databases[0] | ||
| database_name = first_database.get('name', 'default') | ||
| port = first_database.get('port', 10000) | ||
|
|
||
| logger.warning("Using database: %s", database_name) | ||
|
|
||
| result = { | ||
| 'clusterName': cluster_name, | ||
| 'resourceGroup': resource_group_name, | ||
| 'hostName': host_name, | ||
| 'port': port, | ||
| 'databaseName': database_name, | ||
| 'authMethod': auth, | ||
| 'connectionStatus': 'NotTested', | ||
| 'message': '' | ||
| } | ||
|
|
||
| if auth == 'entra': | ||
| # Get token from current Azure CLI credentials for Redis | ||
| try: | ||
| from azure.cli.core._profile import Profile | ||
|
|
||
| profile = Profile(cli_ctx=cmd.cli_ctx) | ||
| # Use get_raw_token with the Redis resource | ||
| creds, _, _ = profile.get_raw_token(resource="https://redis.azure.com") | ||
| access_token = creds[1] | ||
|
|
||
| logger.debug("Successfully obtained Entra ID token for Redis.") | ||
|
|
||
| # Create Redis connection with the token | ||
| # For Entra auth, username is the object ID (oid) from the token | ||
| # The password is the access token itself | ||
| import jwt | ||
| decoded_token = jwt.decode(access_token, options={"verify_signature": False}) | ||
| user_name = decoded_token.get('oid', decoded_token.get('sub', 'default')) | ||
|
|
||
| logger.warning("Connecting with Entra ID user (oid): %s", user_name) | ||
|
|
||
| redis_client = _get_redis_connection( | ||
| host_name=host_name, | ||
| port=port, | ||
| password=access_token, | ||
| ssl=True, | ||
| username=user_name | ||
| ) | ||
|
|
||
| logger.warning("Successfully connected to Redis at %s:%s", host_name, port) | ||
|
|
||
| # Test the connection with a write operation | ||
| success, message, _ = _test_redis_connection_with_write(redis_client) | ||
|
|
||
| if success: | ||
| result['connectionStatus'] = 'Success' | ||
| result['message'] = message | ||
| else: | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = message | ||
|
|
||
| except ImportError as ie: | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = (f"Required package not installed: {str(ie)}. " | ||
| "Please install 'redis' and 'PyJWT' packages.") | ||
| except Exception as e: # pylint: disable=broad-except | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = f'Entra authentication failed: {str(e)}' | ||
|
|
||
| elif auth == 'access-key': | ||
| # Get access keys for the database | ||
| try: | ||
| keys = _DatabaseListKey(cli_ctx=cmd.cli_ctx)(command_args={ | ||
| "cluster_name": cluster_name, | ||
| "resource_group": resource_group_name, | ||
| "database_name": database_name}) | ||
|
|
||
| access_key = None | ||
| if keys: | ||
| access_key = keys.get('primaryKey') or keys.get('secondaryKey') | ||
|
|
||
| if not access_key: | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = ('Access keys authentication may be disabled. ' | ||
| 'Enable access keys authentication or use Entra authentication.') | ||
| return result | ||
|
|
||
| # Create Redis connection with the access key | ||
| redis_client = _get_redis_connection( | ||
| host_name=host_name, | ||
| port=port, | ||
| password=access_key, | ||
| ssl=True | ||
| ) | ||
|
|
||
| logger.warning("Successfully connected to Redis at %s:%s", host_name, port) | ||
|
|
||
| # Test the connection with a write operation | ||
| success, message, _ = _test_redis_connection_with_write(redis_client) | ||
|
|
||
| if success: | ||
| result['connectionStatus'] = 'Success' | ||
| result['message'] = message | ||
| else: | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = message | ||
|
|
||
| except ImportError as ie: | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = f"Required package not installed: {str(ie)}. Please install 'redis' package." | ||
| except Exception as e: # pylint: disable=broad-except | ||
| result['connectionStatus'] = 'Failed' | ||
| result['message'] = f'Failed to connect with access key: {str(e)}' | ||
|
|
||
| return result |
There was a problem hiding this comment.
The new test-connection command lacks test coverage. The repository has comprehensive automated testing infrastructure with scenario tests, but no tests are included for the new command. Add test cases covering both authentication methods (Entra and access-key), success scenarios, and failure scenarios (e.g., when databases don't exist, when authentication fails).
3 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.