Skip to content
This repository was archived by the owner on Jan 12, 2022. It is now read-only.

Monkey-patch os.env to be threadlocal and set up environment to mimic GAE#5

Merged
andrewsg merged 1 commit intoGoogleCloudPlatform:masterfrom
andrewsg:environment
Feb 27, 2015
Merged

Monkey-patch os.env to be threadlocal and set up environment to mimic GAE#5
andrewsg merged 1 commit intoGoogleCloudPlatform:masterfrom
andrewsg:environment

Conversation

@andrewsg
Copy link
Copy Markdown
Member

This is a rearchitecture of the environment variable code to reduce code size and complexity.

R: @apolito @mingzhaodotname @gbin CC: @isdal

Comment thread multicore_runtime/middleware.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what about wsgi_env.get('HTTP_X_APPENGINE_' + key, default)
?

And now I see you already had this conversation....

@andrewsg
Copy link
Copy Markdown
Member Author

@apolito Okay, I've refactored so that the ordering of all steps is perfectly clear and in one place, in reset_environment_middleware, and worked out the ordering to match the old VME code. PTAL

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is new behavior right?

I think this is a mistake. People use app.yaml settings for things like keys and user accounts to use. You don't want a request header to be able to overwrite those.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is my best understanding of the current functionality. See meta_app.py starting here: https://github.com/GoogleCloudPlatform/appengine-python-vm-runtime/blob/ad16b62f9911c5ddd81fd24d5ca04dea56dc8755/python_vm_runtime/google/appengine/ext/vmruntime/meta_app.py#L549

This happens late in the process, but it's if key not in os.environ:. I can only interpret that as "this is the very lowest-priority thing", because any other keys including request header keys will overwrite that. Of course, request header keys aren't just dropped in with their original header name -- they're prefixed with "HTTP_" by the time they arrive in wsgi_env. That magic naming convention strikes me as fragile, but I guess that's the precedent set by CGI and carried over into the earliest versions of GAE (and WSGI).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So to verify:

If I have an app.yaml user env FOO set to 'bar'
and a user makes a request with the header FOO: baz

my environment will be

FOO => bar
HTTP_FPP => baz?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that's right. wsgi_env is already full of specially named, mission-critical stuff and namespacing into HTTP_ is how it handles that conflict. (This is an antiquated practice, but I guess that's WSGI.)

@apolito
Copy link
Copy Markdown

apolito commented Feb 27, 2015

LGTM

@andrewsg
Copy link
Copy Markdown
Member Author

Thanks!

@andrewsg
Copy link
Copy Markdown
Member Author

Squashed to df9368

andrewsg added a commit that referenced this pull request Feb 27, 2015
Monkey-patch os.env to be threadlocal and set up environment to mimic GAE
@andrewsg andrewsg merged commit e4f3805 into GoogleCloudPlatform:master Feb 27, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants