Stop relying on SERVER_SOFTWARE being set #1704
Conversation
Also, deprecate broken functions that rely on GAE_VM
|
|
||
|
|
||
| def is_appengine_sandbox(): | ||
| return is_appengine() and not is_prod_appengine_mvms() | ||
| """Deprecated. Use is_appengine instead.""" |
There was a problem hiding this comment.
This changes the foundational behavior of this method. It should detect if the app is running under the legacy 2.7 runtime in App Engine standard. Making it detect any app engine runtime is likely to cause user code to fail.
There was a problem hiding this comment.
I guess I don't really know what the "foundational behavior" of this method is supposed to be. I couldn't find any documentation for this function, so I only had the old code to go on which I interpreted as "all appengine runtimes except managed VMs are sandboxed."
Given that managed VMs are decommissioned, it seemed that now the situation is that "all appengine runtimes are sandboxed." Am I missing something?
There was a problem hiding this comment.
So "sandboxed" here is a bit weird. You'll have to kinda go back to when this was written - which was before gVisor.
When urllib3 first added rudimentary support for App Engine, there was App Engine (with no qualifier) in a heavily (ptrace) sandboxed environment and there was App Engine managed VMs which had a much less severe KVM sandbox. Urllib3 can run as usual in the KVM sandbox, but can't run as usual in the ptrace sandbox. So this function is used to determine "can I really use sockets and ssl the in a normal way?" which breaks down to this table these days:
Standard legacy 2.7: no
Standard 3.x: yes
Flexible 2.x or 3.x: yes
So this function should return True only in the legacy 2.7 runtime or when using the legacy 2.7 runtime with devappserver. User code uses this function to determine whether or not to use the AppEngineManager (and therefore urlfetch) instead of the normal PoolManager (and therefore sockets).
There was a problem hiding this comment.
That makes sense. I've changed the function to return True for the python27 runtime and added some documentation to explain the behavior.
Codecov Report
@@ Coverage Diff @@
## master #1704 +/- ##
=========================================
- Coverage 99.75% 99.65% -0.1%
=========================================
Files 22 22
Lines 2006 2006
=========================================
- Hits 2001 1999 -2
- Misses 5 7 +2
Continue to review full report at Codecov.
|
|
@sethmlarson this should be ready to merge at your will. :) |
|
@sethmlarson Friendly ping on this. Is there anything left to do before this can be merged? |
|
Thanks for this patch @zevdg! 🎉 |
Whether testbed tests "are appengine" is debatable, but historically this function has returned False in testbed tests. This behavior was inadvertently (and unnecessarily) changed in PR urllib3#1704. This commit undoes that regression for testbed tests.
Whether testbed tests "are appengine" is debatable, but historically this function has returned False in testbed tests. This behavior was inadvertently (and unnecessarily) changed in PR urllib3#1704. This commit undoes that regression for testbed tests.
Whether testbed tests "are appengine" is debatable, but historically this function has returned False in testbed tests. This behavior was inadvertently (and unnecessarily) changed in PR #1704. This commit undoes that regression for testbed tests.
Newer AppEngine environments don't define the SERVER_SOFTWARE env variable. Without this change, importing click dies with a KeyError. See related pull request for urllib3: urllib3/urllib3#1704
Newer AppEngine environments don't define the SERVER_SOFTWARE env variable. Without this change, importing click dies with a KeyError. See related pull request for urllib3: urllib3/urllib3#1704
Whether testbed tests "are appengine" is debatable, but historically this function has returned False in testbed tests. This behavior was inadvertently (and unnecessarily) changed in PR urllib3#1704. This commit undoes that regression for testbed tests.
fixes #1470
Also, mark functions that rely on GAE_VM as broken.