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

Stop relying on SERVER_SOFTWARE being set #1704

Merged
merged 5 commits into from Oct 18, 2019

Conversation

zevdg
Copy link
Contributor

@zevdg zevdg commented Oct 3, 2019

fixes #1470
Also, mark functions that rely on GAE_VM as broken.

zevdg added 3 commits Oct 3, 2019
@pquentin pquentin requested a review from theacodes Oct 5, 2019
Copy link
Member

@theacodes theacodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some work. We have to be really careful about changing the behavior of these functions as it has lots of ramifications for downstream code.

src/urllib3/contrib/_appengine_environ.py Show resolved Hide resolved


def is_appengine_sandbox():
return is_appengine() and not is_prod_appengine_mvms()
"""Deprecated. Use is_appengine instead."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I've changed the function to return True for the python27 runtime and added some documentation to explain the behavior.

@codecov-io
Copy link

codecov-io commented Oct 7, 2019

Codecov Report

Merging #1704 into master will decrease coverage by 0.09%.
The diff coverage is 60%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/urllib3/contrib/_appengine_environ.py 80% <60%> (-20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08cf7fd...14353c9. Read the comment docs.

@theacodes
Copy link
Member

@sethmlarson this should be ready to merge at your will. :)

@zevdg
Copy link
Contributor Author

zevdg commented Oct 14, 2019

@sethmlarson Friendly ping on this. Is there anything left to do before this can be merged?

@sethmlarson sethmlarson merged commit 79e81f9 into urllib3:master Oct 18, 2019
@sethmlarson
Copy link
Member

Thanks for this patch @zevdg! 🎉

@pquentin pquentin mentioned this pull request Nov 11, 2019
zevdg added a commit to zevdg/urllib3 that referenced this pull request Nov 25, 2019
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.
zevdg added a commit to zevdg/urllib3 that referenced this pull request Nov 25, 2019
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.
sethmlarson pushed a commit that referenced this pull request Nov 27, 2019
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.
aaronsarna added a commit to aaronsarna/click that referenced this pull request Jan 30, 2020
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
jab pushed a commit to jab/click that referenced this pull request Jan 31, 2020
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
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contrib/_appengine_environ.py KeyError SERVER_SOFTWARE
5 participants