-
Notifications
You must be signed in to change notification settings - Fork 115
Read mixer env helm charts for /v2/resolve authentication overrides #5924
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: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @clincoln8, 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 enhances security for the 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 a mechanism to enforce API key authentication for the /v2/resolve endpoint based on Helm chart configurations, a valuable security improvement, and enhances the development workflow by parameterizing image tags and adding documentation. However, a critical security concern has been identified in scripts/deploy_cloud_esp.sh due to unquoted variables, which could lead to argument injection or script failure. Additionally, suggestions are provided to improve the robustness of the shell script logic, increase the safety of the manual image push process by addressing a risky default tag in cloudbuild.push_image.yaml, and fix a minor typo in tools/script_runner/README.md.
keyurva
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.
Thanks for the detailed PR desc and doing it so carefully!
| yq eval -i '.endpoints[0].name = env(SERVICE_NAME)' endpoints.yaml | ||
|
|
||
| # Check for V2Resolve override | ||
| V2_RESOLVE_ALLOW_UNREGISTERED=$(yq eval '.esp.v2_resolve_allow_unregistered' $HELM_VALUES_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.
nit: Perhaps add TODO to remove this (and the corresponding mixer helm files) once we switch to always requiring keys everywhere.
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.
added here and in datacommonsorg/mixer#1731 using the same TODO(/v2/resolve cleanup) prefix
This is paired with datacommonsorg/website#5924. See that PR description for details and testing.
This is paired with datacommonsorg/mixer#1731 to always require an api key for /v2/resolve in autopush and dev environments.
Staging and prod api key protection is configured via apigee and not affected by this PR.
Main Change:
scripts/deploy_cloud_esp.sh, checks to see if the mixer helm environment file sets .esp.v2_resolve_allow_unregistered and sets the value accordingly in endpoints.yaml.Side Changes:
build/ci/cloudbuild.push.yamlandbuild/ci/cloudbuild.push_image.yamltools/script_runner/cloudbuild.push_image.yamland update the corresponding README.Testing:
I first tested that deploying website+mixer PR together successfully blocked unauthenticated calls to /v2/resolve:
tools/script_runner/cloudbuild.push_image.yamlto push a new script-runner image with tagdev-calincthat contained both the mixer and website changes locallygcr.io/datcom-ci/datacommons-script-runner:dev-calinchttps://dev.api.datacommons.org/v2/resolve?nodes=population&resolver=indicatorthrew a 401 Unauthenticated Error.Then I tested that mixer environments that don't override this value will remain unaffected by:
gcr.io/datcom-ci/datacommons-script-runner:dev-calinc2