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

fix: do not use the GAE APIs on gen2+ runtimes #807

Merged
merged 3 commits into from Jul 23, 2021

Conversation

zevdg
Copy link
Contributor

@zevdg zevdg commented Jul 20, 2021

Currently, this library uses the App Engine credentials in all environments if
the APIs can be imported successfully. This assumption made sense when
the API was only available on gen1, but this is no longer the case.
See https://github.com/GoogleCloudPlatform/appengine-python-standard

In order to comply with AIP-4115, we must treat GAE gen2+ as a "compute
engine equivalent environment" even if the GAE APIs are importable.
In other words, google.auth.default() should not return an
app_engine.Credental on GAE gen2+.

@zevdg zevdg requested a review from a team as a code owner July 20, 2021 16:01
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2021
@zevdg zevdg force-pushed the gae-gen2-apis branch 2 times, most recently from d0490ce to f4f3f0f Compare July 20, 2021 16:08
@zevdg
Copy link
Contributor Author

zevdg commented Jul 20, 2021

It's worth noting that this change could be disruptive if a user migrates their app from GAE gen1 to GAE gen2 with a version of google.auth that doesn't include this change. Then, instead of the ADC changing when they upgrade runtimes as the spec dictates, their ADC would change when they upgrade this library to include this change. As such, any migration guides should encourage users to upgrade this library to the latest py2-compatible version BEFORE migrating to gen2.

Conversely, if we do not make this change, then gen2 users could run into the opposite problem. Their ADC could suddenly start returning an app_engine.Credential instead of a compute_engine.Credential if they (or one of their dependencies) adds appengine-python-standard to their requirements.txt.

The sooner we land this change, the less people will be affected by either of those scenarios.

google/auth/_default.py Outdated Show resolved Hide resolved
@busunkim96
Copy link
Contributor

CC @wescpy and @engelke since they're listed as DevRel contacts for this project.

Asking folks to update to a minimum version of google-auth is captured in a comment in a documentation bug (186268245).

@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 20, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 20, 2021
@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 20, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 20, 2021
@busunkim96 busunkim96 changed the title do not use the GAE APIs on gen2+ runtimes fix: do not use the GAE APIs on gen2+ runtimes Jul 21, 2021
@busunkim96 busunkim96 added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 21, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2021
@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 21, 2021
@arithmetic1728
Copy link
Contributor

@zevdg Kokoro build failed because coverage test failed. It seems we need some additional unit tests to cover google/auth/_default.py line 243-245 and 251-255.

@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 22, 2021
Currently, this library uses the App Engine API in all environments if
it can be imported successfully. This assumption made sense when the API
was only available on gen1, but this is no longer the case.
See https://github.com/GoogleCloudPlatform/appengine-python-standard

In order to comply with AIP-4115, we must treat GAE gen2+ as a "compute
engine equivalent environment" even if the GAE APIs are importable.
In other words, google.auth.default() must never return an
app_engine.Credental on GAE gen2+.Currently, this library uses the App Engine API in all environments if
it can be imported successfully. This assumption made sense when the API
was only available on gen1, but this is no longer the case.
See https://github.com/GoogleCloudPlatform/appengine-python-standard

In order to comply with AIP-4115, we must treat GAE gen2+ as a "compute
engine equivalent environment" even if the GAE APIs are importable.
In other words, google.auth.default() should not return an
app_engine.Credental on GAE gen2+.
@busunkim96 busunkim96 added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 23, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2021
@arithmetic1728 arithmetic1728 merged commit 7f7d92d into googleapis:master Jul 23, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants