nexedi / erp5

mentioned in merge request slapos!571

What about unconditional setRequest(old_request) ? (and remove the unused import)

Right. The way it's implemented, clearRequest() does the same as setRequest(None).

When importing a significant amount of code from another project, we usually preserve indentation to make upstream changes easier to backport. We must also preserve the license.

I'll change the indentation back (I wasn't sure).

The project paste.translogger no longer seems to exist. Its documentation is still available but not its source code. I got the Translogger class from pyramid_translogger, which seems to be a kind of fork. So, I do not have access to the original license of paste.translogger. It seems to be the MIT license, though.

Should I also change the whole file indentation to 4 spaces?

I changed it to 4 spaces, for uniformity. I also added the "MIT license" from pyramid_translogger.

      while chunksize < rest:
        new_wsgi_input.write(original_wsgi_input.read(chunksize))
        rest -= chunksize
      if rest:
        new_wsgi_input.write(original_wsgi_input.read(rest))

I changed it.

I'm curious to know what duplicates we may have.

In any case, if it's a list, you can do:

del z2log.handlers[1:]

Using conf.access() was not a correct way to get the logger. ZConfig.loadConfig already creates one called "access", so we can simply do z2log = logging.getLogger("access").

bad indentation (logger is not a parameter of app_wrapper)

Right, it's fixed.

Can you sort imports as follows:

• first built-in
• then everything else but ERP5
• at last ERP5

And within each group, alphabetically.

Right, it was a bit messy. I sorted them.

Can you add a preliminary commit that only adds a verbatim copy of WSGIPublisher.py so that our changes in this file are visible ?

Why "backports.py" and not "WSGIResponse.py" ?

The idea is to avoid having 1 file for every small piece of code we backport.

"backports.py" ended up containing more than backports and the original place of the patch class is in ZPublisher.WSGIPublisher, so we merged into patches.WSGIPublisher

Ah yes. I liked that idea of putting backports in a dedicated file (because we know that when we update dependencies we can drop backports), but as you said it's not always possible.

Is this safe ? when reseting components isn't it clearing sys.modules or doing something that could be incompatible ?

I looked the code of components it only deletes its own module, so that should be OK.

added 5 commits

• 52971ef5 - patch: backport WSGIPublisher from Zope4
• 74b6b534 - patches/WSGIPublisher: make the backport compatible with Zope2
• 3307d78f - ERP5/bin: create the zopewsgi script
• 6f72b9fc - Localizer: replace the 'Global Request' monkey-patch by an import of zope.globalrequest
• 213bcdc0 - patch: force the removal of hop-by-hop headers before finalizing the response

Compare with previous version

added 5 commits

• 91a48711 - patch: backport WSGIPublisher from Zope4
• adc331f1 - patches/WSGIPublisher: make the backport compatible with Zope2
• 0503604a - ERP5/bin: create the zopewsgi script
• fa2e3626 - Localizer: replace the 'Global Request' monkey-patch by an import of zope.globalrequest
• c7dd0b84 - patch: force the removal of hop-by-hop headers before finalizing the response

Compare with previous version

added 7 commits

• 96870399 - ERP5TypeTestCase: replace ZPublisher.Test.publish_module with ZPublisher.Publish…
• 4ae24d65 - patch: backport WSGIPublisher from Zope4
• a564d976 - patches/WSGIPublisher: make the backport compatible with Zope2
• f8dd39b3 - Localizer: replace the 'Global Request' monkey-patch by an import of zope.globalrequest
• ec7ef2f0 - ERP5/bin: create the zopewsgi script
• fb6f4e6d - CMFActivity: Make ActivityTool compatible with WSGI
• 435b6119 - TimerService: make timerserver compatible with WSGI

Compare with previous version

added 2 commits

• dc664528 - CMFActivity: Make ActivityTool compatible with WSGI
• 635ea405 - TimerService: make timerserver compatible with WSGI

Compare with previous version

added 3 commits

• 1d2c9a6f - ERP5/bin: create the zopewsgi script
• e311b8de - CMFActivity: Make ActivityTool compatible with WSGI
• 56418968 - TimerService: make timerserver compatible with WSGI

Compare with previous version

unmarked as a Work In Progress

It is also required to update TIDStorage: see $377

For me, it's ready to be merged. The most difficult part is in error handling in WSGIPublisher.transaction_pubevents.

For the deadlock debugger, I hope it's enough. I'd like to avoid writing to much code that's specific to the WSGI server.

Isn't it what we are doing in WSGIResponse.finalize patch ? does this comment still apply in our patch ?

Yes, to be removed. And the following lines can be optimized:

            for name, value in headers:
                if name.lower() == 'content-length':
                    break
            else:
                value = None
            self.write_log(environ, method, req_uri, start, status, value)

Or maybe the comment refers to the case of a streaming response.

BTW, about streaming responses, I realized that "RESPONSE.write does not stream data in wsgi", this is now being discussed in https://github.com/zopefoundation/Zope/issues/662

(I assume that for DMS and shacache we'll need a way to stream responses)

I assume that for DMS and shacache we'll need a way to stream responses)

...and wendelin

Here is a snippet showing how we can achieve streaming, using a custom iterator class. It should work with Medusa and WSGI.

That's cool, I saw there was this class but did not tried so far. I just tried that snippet, after modifying a bit to add a sleep so that we can see that it's really streaming and with ZServer/medusa it is correctly streaming. I also tried it on a vanilla wsgi zope4 and it's also streaming.

ERP5 has a few RESPONSE.write, but not so many. Zope also have a few. I don't know where to check wendelin. Maybe we can just rewrite all this to use IStreamIterator ?

That "proof of concept" patch ( https://github.com/perrinjerome/Zope/commit/dd3f109ae ) seems working, but I guess nobody wants to use this deprecated way.

There's one thing we will miss I think is that pattern in our maintenance scripts (in restricted python):

# make changes to ZODB
print "something"
context.RESPONSE.REQUEST.write(str(printed))
if dry_run:
  raise ValueError('something to abort transaction')

We are also using this in business template export, so that exporting a BT does not add anything to ZODB (it was causing database to grow a lot for nothing really useful). But this (the fact that raising exception abort transaction) was not a documented feature of zope as far as I know (or maybe in the "zope secrets" book).

The best way to simulate is to use transaction.doom(). Or in the published callable:

def foo(self, commit=False):
  try:
    do_something()
    return "whatever"
  finally:
    if not commit:
      transaction.abort()

We are also using this in business template export, so that exporting a BT does not add anything to ZODB

This reminds me something but I can't find, and I'm sure it is/was not done the way you described.

I was talking about this.

My mistake. It must be changed to use transaction.doom(), which is allowed in restricted python.

From IStreamIterator interface:

IStreamIterators must not read from the object database.

Trying to modify an object cause

  File "ZODB-5.5.1-py2.7.egg/ZODB/Connection.py", line 840, in _register
    self.transaction_manager.get().join(self)
AttributeError: 'NoneType' object has no attribute 'get'

Loading a new object raise ZODB.POSException.ConnectionStateError: Shouldn't load state for Products.PythonScripts.PythonScript.PythonScript 0x09 when the connection is closed (in this case I was trying to load a python script object)

So it also seems impossible to just rewrite our current usages of RESPONSE.write for streaming as IStreamIterator, isn't it ?

---

some more or less related notes:

• In zope3 (bluebream) there was "no response.write" https://github.com/zopefoundation/zope.publisher/blob/b21e67576837a843c51ccb93e2941bf3b43dbcef/src/zope/publisher/httpresults.txt#L12

• There have been other attempts in the past to bring wsgi support for zope I saw https://github.com/infrae/infrae.wsgi which in the readme says "It pay specially attention to properly implement streaming." ( and uses the same not recommended start_response()(written) approach)

• some people seem to work on porting ZServer to python 3 ( https://github.com/zopefoundation/ZServer/issues/16 ) and use twisted http server ( https://community.plone.org/t/moonshot-for-migrating-zserver-to-python-3-at-plog-2019/8191/19 )

It might be possible to make another class similar to IStreamIterator, but that keep the transaction and ZODB connection running until the iterator is completely consumed or until an error happen, don't you think ? But it seems not easy, especially regarding error management, I don't see how WSGI could send the errors such as connection closed up to that iterator (maybe this can be implemented in __del__ ? )

some people seem to work on porting ZServer to python 3 ( https://github.com/zopefoundation/ZServer/issues/16 ) and use twisted http server ( https://community.plone.org/t/moonshot-for-migrating-zserver-to-python-3-at-plog-2019/8191/19 )

I really don't want that. WSGI is important for security: on one side, we have people caring about the HTTP layer, and on the other side, the application developers. Given how long ZServer was abandoned, without even IPv6 support, I don't trust it.

I had asked Bryton to check streaming response with a known length, but I missed that we usually want to keep the ZODB open. It's crazy to see that Zope devs didn't insist on supporting this use case. That's what we must do.

I was talking about this.

Fixed in cdaef829.

For stream results accessing the ZODB, 4ca4d7f6 is a possible solution. Before getting there, we pushed intermediate change (3c2fbcb6) to a test node: https://nexedi.erp5.net/test_result_module/20190628-4296C49C

Note sure what's better between a finally clause or __del__.

For stream results accessing the ZODB, 4ca4d7f6 is a possible solution.

Yes, I think this is in the right direction. This is also more or less what repoze.tm2 was doing.

What is different is that IStreamIterator is specifically documented as being consumed after connection is closed, which makes it suitable for long requests which does not need to be part of transaction ( like proxying a request from another server like this stack overflow question ), but I don't think we are using this. Nevertheless, I guess this needs to be a different Interface (such as maybe ITransactionAwareStreamIterator) that would be consumed during transaction lifetime to have this integrated in upstream zope.

Before getting there, we pushed intermediate change (3c2fbcb6) to a test node: https://nexedi.erp5.net/test_result_module/20190628-4296C49C

In my understanding we don't have anything testing WSGI yet, do we ?

For stream results accessing the ZODB, 4ca4d7f6 is a possible solution.

Yes, I think this is in the right direction. This is also more or less what repoze.tm2 was doing.

Where does repoze.tm2 manage the opening/closure of ZODB connections ?

On the other side, one can wonder whether the stream iteration should happen before or after the transaction commit. Both ways have their drawbacks:

• before -> if the DB is modified and an abort happens (conflict), we would have returned invalid data
• after -> the processing of the request and the code streaming the result don't work at the same transaction (in the case of ZODB, we could solve with a historical connection)

I suggested the later because it looks easier. But there's a transaction.begin() missing, now that it should be explicit.

What is different is that IStreamIterator is specifically documented as being consumed after connection is closed, which makes it suitable for long requests which does not need to be part of transaction ( like proxying a request from another server like this stack overflow question ), but I don't think we are using this. Nevertheless, I guess this needs to be a different Interface (such as maybe ITransactionAwareStreamIterator) that would be consumed during transaction lifetime to have this integrated in upstream zope.

I missed the doc.

I find that programming with such interfaces is annoying (cf Bryton's snippet) and leads to ugly code if the publisher has to check each possible interface. Ideally:

• the user would just have to pass an iterator to some response factory
• publish_module should handle only 2 types of results: the usual bytes and something with the most generic API that can plug the way it wants (before commit, between commit and connection closure, or after connection closure, if all those 3 cases are useful)

Before getting there, we pushed intermediate change (3c2fbcb6) to a test node: https://nexedi.erp5.net/test_result_module/20190628-4296C49C

In my understanding we don't have anything testing WSGI yet, do we ?

Yes. I didn't include what you did in slapos.git: you can do it. On ERP5 side, WSGI could be tested by functional tests: I think it's not the case yet.

Where does repoze.tm2 manage the opening/closure of ZODB connections ?

mmh I don't know, probably in another middleware, maybe repoze.zodbconn. What looked interesting was the way the response chunks are yielded in a try except block taking care of exception handling (here). There's also a few tests exercising publishing responses with errors that might be interesting to reproduce.

On the other side, one can wonder whether the stream iteration should happen before or after the transaction commit. Both ways have their drawbacks: ( ... )

Exactly. Saying "streaming responses cannot use database so that they don't fail" is an easy way around all this. I think I read that some plone products were handling the streaming by writing to a temporary file during transaction and returning a ZPublisher.Iterators.filestream_iterator( but I don't find reference / code ).

In our use cases, writing to a temporary file sounds impossible, but I guess in most cases we don't modify database in streaming responses, so maybe we can have this requirement that streaming responses should do their best not to fail (a bit like when we send emails in a non-transactional way).

I know a case (in project specific code) where we have a "downloaded state" on some PDF documents and we change the workflow state the first time it's downloaded, but that's very special case, our other streaming cases are just to send a big "file" as response. If we use the before way with a rule "don't write to ZODB when streaming", this could by implemented by using an intermediate download script:

context.markDownloaded()
return context.Base_redirect('download') # redirect to streaming view, streaming will be another transaction

so maybe using the before way is ok ?

I find that programming with such interfaces is annoying (cf Bryton's snippet) and leads to ugly code if the publisher has to check each possible interface.

Well it's zope way... using interfaces could allow to make two interfaces and control when to iterate and finish transaction, one for before one for after but I think this means some big if else with similar code, so it's probably a bad idea at this point.

Yes. I didn't include what you did in slapos.git: you can do it.

Ah yes, thanks, I'll do this.

On ERP5 side, WSGI could be tested by functional tests: I think it's not the case yet.

Ah yes, that can be nice. It still uses startZServer I believe.

Is this the correct remote address ? zope supports "trusted proxies" and only use HTTP_X_FORWARDED_FOR from if origin is a trusted proxy.

We have a patch for that, but in practice we never set trusted proxy (and there are plans to base this on a certificate infrastructure rather than frontend ip).

I forgot we have such patch. Again, more checking needed...

If properly configured, waitress does everything for us, and REMOTE_ADDR is what we want from the beginning of the request.

Unfortunately, we're stuck on slapos side because at the time we instanciate zopes, the proxy ip is unknown. Waitress server with trusted_proxy='*' works and as long as zopes listen to local ipv4, it looks acceptable.

Bryton will update the MR with these changes.

Currently we have equivalent of trusted_proxy = '', probably we can keep this behaviour for now.

Where is this set BTW ? It's off topic, but if there's a way to use something else than the basic auth header, it's interesting in #20170609-22C98E1

like client SSL auth ?

We'll have to do more checks about authentication.

For Z2.log, request's basic authentication header is decoded ( here ) to find the login from encoded login:password and this is what goes in log.

When using authentication tokens ( from erp5_access_token ) or google/facebook logins, there's no basic auth header so there's no login/user id in log. SSL client auth, I don't know but I guess it's same.

I was asking because it seems with WSGI we'll be able to improve this, but it's not something blocking.

We are actually talking about 2 different things.

For me, REMOTE_USER is linked to client SSL auth, which is used in some projects. We checked and waitress is stricter about HTTP headers:

• headers containing a '_' are dropped
• non proxy-specific headers are mangled as follows: - replaced by _, upcased, and 'HTTP_' prepended (we could easily avoid the prefix by modifying the rename_headers global)

Which means that our apache.conf is incompatible with waitress. We have to find another headers, e.g. Remote-User, and change the External Authentication Plugin in ERP5 to use HTTP_REMOTE_USER.

The primary goal is to have WSGI working as well as Medusa so that nobody has an excuse not to switch to WSGI. Therefore, doing better than Medusa is not a priority and fixing the bug you mentioned is off-topic. Unless you have a solution to propose, I prefer to wait that this MR is merged.

waitress is stricter about HTTP headers

gevent has same behaviour

we could easily avoid the prefix by modifying the rename_headers global

so this is a bad idea

Ah yes we were talking about two different things. I agree that that bug is out of scope, I was asking in case it's easy to access the user object to put its login ( or user id ) in Z2.log.

About SSL client authentication, I think that this is what we have in slapos-master apache config apache config, this: RequestHeader set REMOTE_USER %{SSL_CLIENT_S_DN_CN}s that is read by a slapos.core PAS plugin.

But waitress is probably right to strip headers with - in https://github.com/Pylons/waitress/commit/6d4dab6bed88917b973066a6d5222917661802b7 , because the way we do SSL client auth is vulnerable from the problem from django website. Probably slapos guys want to fix this (maybe just by using a header without underscore).

for medusa, we could do this !901 ... know that I understand the magic ( !901 (comment 84379) )which makes username appear in Z2.log when we log in with cookie crumbler, I'm not sure this works for WSGI setup.

edit this = having username in access log.

This works if query string is non ascii ? ( what happens when visiting http://erp5?héhé for example )

( I'm asking because I saw that there are still problems in zope https://github.com/zopefoundation/Zope/issues/575 )

Thanks. We forgot to check whether waitress produces environ dicts with same string types as medusa. We'll have to do that.

Here, if the 3 environ vars are bytes (utf-8), then it's fine.

We confirm they are utf-8 bytes.

zope4 uses a "zope.ini" where a "pipeline" is defined:

[pipeline:main]
pipeline =
    egg:Zope#httpexceptions
    translogger
    zope

we don't do this way and integrate the logic of the pipelines directly in zopewsgi (for logging) and in WSGIPublisher(for exception handling - retry, unauthorized), is it ?

Yes.

At some point, we chose gevent, but this wsgi server does not define a paste entry point so it was not possible such config file.

About WSGIPublisher, Zope4 already merged into it what was before in repoze.tm2 and repoze.retry middlewares. Error handling is something we'll have to rework but currently, the only way to get the behaviour we're used to is to modify WSGIPublisher.

There remains translogger. I don't know what's the best version between those from paster and pyramid. It looks like we'll anyway need our own.

Assembling a pipeline is anyway so easy to do programmatically that I prefer to stay away from paste.

added 1 commit

• 0fec9e6d - fixup! ERP5/bin: create the zopewsgi script

Compare with previous version

marked as a Work In Progress from 0fec9e6d

added 6 commits

• 81c8663f - patches: backport WSGIPublisher from Zope4
• f33ea934 - patches: make the WSGIPublisher backport work with Zope 2.13
• 3383137a - Localizer: replace the 'Global Request' monkey-patch with zope.globalrequest
• c33d2d1d - New ERP5/bin/zopewsgi script
• f46d70f9 - CMFActivity: make ActivityTool compatible with WSGI
• 5eeb52d5 - TimerService: make timerserver compatible with WSGI

Compare with previous version

unmarked as a Work In Progress

merged

mentioned in commit d0310869

self._streaming is only defined in Zope 4's WSGIResponse and does not exist in Zope 2's HTTPResponse, isn't it ?

If I'm reading https://github.com/zopefoundation/Zope/blob/2.13/src/ZPublisher/HTTPResponse.py#L332 correctly, in setHeader we always use the lower-cased header name as key, so we probably want to use the lower cased key here as well:

headers.pop('connection', None)