-
Notifications
You must be signed in to change notification settings - Fork 6.6k
feat: Custom Credential Supplier Documentation #13634
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
base: main
Are you sure you want to change the base?
feat: Custom Credential Supplier Documentation #13634
Conversation
|
Here is the summary of changes. You are about to add 2 region tags.
This comment is generated by snippet-bot.
|
Summary of ChangesHello @vverman, 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 significantly enhances the authentication samples by adding support for Custom Credential Suppliers in Python. It provides practical examples and comprehensive documentation for integrating external identity providers like AWS and Okta with Google Cloud, facilitating secure access in hybrid or multi-cloud environments. The changes aim to make it easier for developers to leverage Workload Identity Federation with non-standard authentication sources. 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
This pull request introduces new samples for using custom credential suppliers with the Google Cloud Python SDK, specifically for AWS and Okta. The code is well-structured and includes good documentation and tests. My review includes suggestions to improve error handling by using more specific exceptions and directing error messages to stderr, which is a standard practice. I've also pointed out a few minor issues like missing newlines at the end of files and an opportunity to make the Okta supplier more flexible by not hardcoding the OAuth scope. These changes will enhance the robustness and reusability of the samples.
|
|
||
| ```bash | ||
| eksctl delete cluster --name your-cluster-name | ||
| ``` No newline at end of file |
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.
| import json | ||
| import os | ||
|
|
||
| import boto3 | ||
| from google.auth import aws | ||
| from google.auth import exceptions | ||
| from google.auth.transport import requests as auth_requests |
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.
To support writing to standard error, the sys module should be imported. It's also a good practice to group standard library imports together, followed by third-party imports, as per PEP 8.
| import json | |
| import os | |
| import boto3 | |
| from google.auth import aws | |
| from google.auth import exceptions | |
| from google.auth.transport import requests as auth_requests | |
| import json | |
| import os | |
| import sys | |
| import boto3 | |
| from google.auth import aws | |
| from google.auth import exceptions | |
| from google.auth.transport import requests as auth_requests |
| if not all([gcp_audience, gcs_bucket_name]): | ||
| print( | ||
| "Required environment variables missing: GCP_WORKLOAD_AUDIENCE, GCS_BUCKET_NAME" | ||
| ) | ||
| return | ||
|
|
||
| try: | ||
| print(f"Retrieving metadata for bucket: {gcs_bucket_name}...") | ||
| metadata = authenticate_with_aws_credentials( | ||
| gcs_bucket_name, gcp_audience, sa_impersonation_url | ||
| ) | ||
| print("--- SUCCESS! ---") | ||
| print(json.dumps(metadata, indent=2)) | ||
| except Exception as e: | ||
| print(f"Authentication or Request failed: {e}") |
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.
There are a couple of improvements we can make here:
- Error messages should be printed to standard error (
sys.stderr) instead of standard output. This is a standard practice for separating normal output from error diagnostics. - Catching a generic
Exceptionis too broad and can hide bugs by catching unexpected errors (likeKeyboardInterrupt). It's better to catch more specific exceptions that are expected from the authentication and request process, such asgoogle.auth.exceptions.GoogleAuthErrorandrequests.exceptions.HTTPError.
| if not all([gcp_audience, gcs_bucket_name]): | |
| print( | |
| "Required environment variables missing: GCP_WORKLOAD_AUDIENCE, GCS_BUCKET_NAME" | |
| ) | |
| return | |
| try: | |
| print(f"Retrieving metadata for bucket: {gcs_bucket_name}...") | |
| metadata = authenticate_with_aws_credentials( | |
| gcs_bucket_name, gcp_audience, sa_impersonation_url | |
| ) | |
| print("--- SUCCESS! ---") | |
| print(json.dumps(metadata, indent=2)) | |
| except Exception as e: | |
| print(f"Authentication or Request failed: {e}") | |
| if not all([gcp_audience, gcs_bucket_name]): | |
| print( | |
| "Required environment variables missing: GCP_WORKLOAD_AUDIENCE, GCS_BUCKET_NAME", | |
| file=sys.stderr, | |
| ) | |
| return | |
| try: | |
| print(f"Retrieving metadata for bucket: {gcs_bucket_name}...") | |
| metadata = authenticate_with_aws_credentials( | |
| gcs_bucket_name, gcp_audience, sa_impersonation_url | |
| ) | |
| print("--- SUCCESS! ---") | |
| print(json.dumps(metadata, indent=2)) | |
| except (exceptions.GoogleAuthError, auth_requests.requests.exceptions.HTTPError) as e: | |
| print(f"Authentication or Request failed: {e}", file=sys.stderr) |
| @@ -0,0 +1,84 @@ | |||
| Here is the adapted `README.md` for the Python version of the Okta Custom Credential Supplier sample. | |||
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.
| python3 snippets.py | ||
| ``` | ||
|
|
||
| The script will then authenticate with Okta to get an OIDC token, exchange that token for a GCP federated token (and optionally a Service Account token), and use it to list metadata for the specified GCS bucket. No newline at end of file |
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.
It's a good practice for text files, including Markdown files, to end with a newline character. This can prevent issues with some tools and file concatenations.
| The script will then authenticate with Okta to get an OIDC token, exchange that token for a GCP federated token (and optionally a Service Account token), and use it to list metadata for the specified GCS bucket. | |
| The script will then authenticate with Okta to get an OIDC token, exchange that token for a GCP federated token (and optionally a Service Account token), and use it to list metadata for the specified GCS bucket. | |
| @@ -0,0 +1,3 @@ | |||
| requests==2.32.3 | |||
| google-auth==2.43.0 | |||
| python-dotenv==1.1.1 No newline at end of file | |||
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.
| import json | ||
| import os | ||
| import time | ||
| import urllib.parse | ||
|
|
||
| from google.auth import identity_pool | ||
| from google.auth.transport import requests as auth_requests | ||
| import requests |
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.
To support writing to standard error and for more specific exception handling, the sys and google.auth.exceptions modules should be imported. It's also a good practice to group standard library imports together, followed by third-party imports, as per PEP 8.
| import json | |
| import os | |
| import time | |
| import urllib.parse | |
| from google.auth import identity_pool | |
| from google.auth.transport import requests as auth_requests | |
| import requests | |
| import json | |
| import os | |
| import sys | |
| import time | |
| import urllib.parse | |
| from google.auth import exceptions, identity_pool | |
| from google.auth.transport import requests as auth_requests | |
| import requests |
| } | ||
| data = { | ||
| "grant_type": "client_credentials", | ||
| "scope": "gcp.test.read", # Set scope as per Okta app config. |
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 Okta scope is hardcoded here. For better reusability, consider passing the scope(s) as an argument to the OktaClientCredentialsSupplier constructor. This would allow users of the class to specify the required scopes for their particular Okta application configuration without modifying the class source code.
| if not all( | ||
| [gcp_audience, gcs_bucket_name, okta_domain, okta_client_id, okta_client_secret] | ||
| ): | ||
| print("Missing required environment variables.") | ||
| return | ||
|
|
||
| try: | ||
| print(f"Retrieving metadata for bucket: {gcs_bucket_name}...") | ||
| metadata = authenticate_with_okta_credentials( | ||
| bucket_name=gcs_bucket_name, | ||
| audience=gcp_audience, | ||
| domain=okta_domain, | ||
| client_id=okta_client_id, | ||
| client_secret=okta_client_secret, | ||
| impersonation_url=sa_impersonation_url, | ||
| ) | ||
| print("--- SUCCESS! ---") | ||
| print(json.dumps(metadata, indent=2)) | ||
| except Exception as e: | ||
| print(f"Authentication or Request failed: {e}") |
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.
There are a couple of improvements we can make here:
- Error messages should be printed to standard error (
sys.stderr) instead of standard output. This is a standard practice for separating normal output from error diagnostics. - Catching a generic
Exceptionis too broad and can hide bugs by catching unexpected errors (likeKeyboardInterrupt). It's better to catch more specific exceptions that are expected from the authentication and request process, such asgoogle.auth.exceptions.GoogleAuthErrorandrequests.exceptions.HTTPError.
| if not all( | |
| [gcp_audience, gcs_bucket_name, okta_domain, okta_client_id, okta_client_secret] | |
| ): | |
| print("Missing required environment variables.") | |
| return | |
| try: | |
| print(f"Retrieving metadata for bucket: {gcs_bucket_name}...") | |
| metadata = authenticate_with_okta_credentials( | |
| bucket_name=gcs_bucket_name, | |
| audience=gcp_audience, | |
| domain=okta_domain, | |
| client_id=okta_client_id, | |
| client_secret=okta_client_secret, | |
| impersonation_url=sa_impersonation_url, | |
| ) | |
| print("--- SUCCESS! ---") | |
| print(json.dumps(metadata, indent=2)) | |
| except Exception as e: | |
| print(f"Authentication or Request failed: {e}") | |
| if not all( | |
| [gcp_audience, gcs_bucket_name, okta_domain, okta_client_id, okta_client_secret] | |
| ): | |
| print("Missing required environment variables.", file=sys.stderr) | |
| return | |
| try: | |
| print(f"Retrieving metadata for bucket: {gcs_bucket_name}...") | |
| metadata = authenticate_with_okta_credentials( | |
| bucket_name=gcs_bucket_name, | |
| audience=gcp_audience, | |
| domain=okta_domain, | |
| client_id=okta_client_id, | |
| client_secret=okta_client_secret, | |
| impersonation_url=sa_impersonation_url, | |
| ) | |
| print("--- SUCCESS! ---") | |
| print(json.dumps(metadata, indent=2)) | |
| except (exceptions.GoogleAuthError, requests.exceptions.HTTPError) as e: | |
| print(f"Authentication or Request failed: {e}", file=sys.stderr) |
Adding documentation for Custom Credential Suppliers.
Custom Credential Suppliers enable developers to securely integrate third-party authentication directly into the Google Cloud SDKs. Custom Credential Suppliers are primarily used to handle authentication in non-standard cloud environments.
The design and scopes for this are documented under this design doc
Checklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)These tests will safely skip if the env variables aren't provided.