-
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.
| @@ -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.
| } | ||
| 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) |
this updates to a more modern version of python and leverages the benefits of Docker's Layer caching. requirements.txt will rarely change so this way the depedency layer is cached and only rebuilt if you explictly change requirements.txt.
need to refactor testing based on this separation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
there are some refinements that could be done in the okta, but this looks fine. I've addressed some improvements to speed up feedback to Iyer based on feedback from Siracusa. I also updated the README to be more in alignment with the standards set by DevSite for how we refer to things. for folks who may be looking at this in the future, for context, we decided that there isn't a path for third-party samples currently so there is limited testing and that must be done manually. The owner of the samples has verified that the samples work as expected. This is also why the documentation is the README is much more extensive than for some samples. |
iennae
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.
apologies @vverman
I just noticed when fixing linting in okta, I realized that the GCS bucket code is not using the SDK there. Is it possible to use the SDK and send the credentials object from the supplier and then use the SDK and that would allow simplification with the bucket object metadata?
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
Checklist
nox -s py-3.9(see Test Environment Setup)nox -s lint(see Test Environment Setup)