nexedi / slapos

Changed title: WIP: erp5: Add entries to access zope directly → WIP: [RFC] erp5: Add entries to access zope directly

@romain @luke @vpelletier @rafael was this considered / discussed ? Am I missing a reason not to have this ?

I remember discuting this with Rafael at some point and you suggested hiding this behind some "random path" and rewrite rules, but I don't really see a need for this. To restrict access, we can configure that the zope family needs client ssl-authentication and it will also be applied for these.

Unmarked this merge request as a Work In Progress

Marked this merge request as a Work In Progress

The intent looks good to me.

Please indent inside the jinja2 tags, as is already done in the rest of this template.

I do not like much that it enforces IPv6: we would want to access nodes just as well from IPv4 if the partition only has IPv4.

I do like that VirtualHostMonster gets patched, and consider it independent (can go in without waiting for the rest IMHO).

What about using a path prefix on a single port rather than individual ports ? We can have many zopes in an instance, so we would burn as many ports. Suggested scheme: %(family)s%(i)2i. Ex: https://.../activity00/, https://.../activity01/, https://.../web00/ etc. These would still be on a port separate from individual families.

About access restrictions, to me they have to be implemented on all ports anyway: I want to be able to only allow (some set of) frontends to access a backend. So to me they are a separate topic indeed.

I do not like much that it enforces IPv6: we would want to access nodes just as well from IPv4 if the partition only has IPv4.

What about using a path prefix on a single port rather than individual ports ?

In that patch I did not change the apache template (for simplicity), so it was not possible, but yes, I also like these suggestions.

How about a scheme that would keep the same names as on logfiles and program names in supervisor ? https://.../activity/zope-0/, https://.../activity/zope-1/, https://.../web/zope-0/ ?

Also, because ssl-authentication is at the level a family, I'm thinking it would be easier to have one port for each family, with paths like /zope-0/, /zope-1/, so that we can enable client ssl on the port for this family as well if if it's enabled for the family.

How about a scheme that would keep the same names as on logfiles and program names in supervisor ?

Fine for me.

Also, because ssl-authentication is at the level a family, I'm thinking it would be easier to have one port for each family, with paths like /zope-0/, /zope-1/, so that we can enable client ssl on the port for this family as well if if it's enabled for the family.

ssl-authentication is AFAIK intended so clients can authenticate with ssl certificates. I would expect this to not be needed for these very sysadmin-ish, non-standard-use to not need this. Unless I'm missing something.

I do not intend to restrict backend's ability to use https to the port using this flag - I would rather have all backends require https everywhere, even without client auth. But this is another task.

Added 22 commits:

• a87e5160...90b10c1b - 20 commits from branch nexedi:master
• 7bc5c1c6 - erp5: Zope2 patch to support ipv6 hostnames in VirtualHostMonster
• 2d3da5f3 - erp5/balancer: add entries for direct access to zopes

I don't think it matters but it's worth noting that this is also visible in the Server header in HTTP response. It becomes Zope/(2.13.27+slapospatched001, python 2.7.14, linux2) ZServer/1.1 (it was Zope/(2.13.27, python 2.7.14, linux2) ZServer/1.1 )

So far, we have always patched Zope2 with monkey-patches in product/ERP5Type/patches. That's easier to maintain and we shouldn't mix style.

I see you wrote a test: do we run Zope2 test suite ?

So far, we have always patched Zope2 with monkey-patches in product/ERP5Type/patches. That's easier to maintain and we shouldn't mix style.

Ah yes, that's true. I did not realise that fact.

This is an old patch from @luke that I found on old zope bug tracker https://bugs.launchpad.net/zope2/+bug/699865 and just converted to a git patch ( https://github.com/jerome-nexedi/Zope/commit/4e4ffe940a8927abb0c0c8a2b704eb0a0218bf26 ).

About converting it to monkey patch, if we take a look a the modified function, rewriting as monkey patch means we have to copy the whole function, so it's not practical.

BTW, we are not so strict to that rule, we can see in component/egg-patch that we patch Products.DCworkflow a bit by patches and a also in product/ERP5Type/patches.

I see you wrote a test: do we run Zope2 test suite ?

The test is also from @luke old patch.

We don't run the test suite automatically. I ran it locally and it passes.

If this patch was submitted to zope and applied to 2.13 branch, we could probably just update to 2.13.29 ( according to https://github.com/zopefoundation/Zope/blob/2.13/doc/CHANGES.rst ).

I didn't know for DCworkflow.

If this patch was submitted to zope and applied to 2.13 branch

I am member of zopefoundation so I can commit. You could submit a PR and warn we'd merge in a 2 weeks if noone comments.

I made PR into Zope: https://github.com/zopefoundation/Zope/pull/314

mentioned in merge request !420

added 151 commits

• 2d3da5f3...ef8968eb - 146 commits from branch nexedi:master
• bcf99a5d - erp5: Zope2 patch to support ipv6 hostnames in VirtualHostMonster
• 758d27dd - component/apache: enable mod_reqtimeout
• d09f4827 - component/apache: increase Timeout for direct access case
• 6569cc07 - component/apache: support zope path with a /
• 30074e28 - erp5/balancer: add entries for direct access to zopes

Compare with previous version

added 5 commits

• 92131e9c - erp5: Zope2 patch to support ipv6 hostnames in VirtualHostMonster
• 654477c0 - component/apache: enable mod_reqtimeout
• 7671b6ff - component/apache: increase Timeout for direct access case
• e75f3152 - component/apache: support zope path with a /
• 7c4ceb0a - erp5/balancer: add entries for direct access to zopes

Compare with previous version

It's still WIP, but I updated a bit ( in two steps, I first made a typo SlaposPatched001 vs SlapOSPatched001)

• The zope patch is still in this branch (it's still the original patch). The plan is to get the zope PR merged and released on the 2.13 series and update our egg, but it's still here for now ( but if this takes too long, we can also merge this in a first step and later drop the patch when we update zope )
• apache entries for direct access now have a longer timeout ( and uses mod_reqtimeout for slowloris ), the main goal was to solve a test failure due to "Proxy Error" have now on testFunctionalConfigurator, see erp5@b955dbe0 (comment 67667) cc @arnau . This is running on test nodes ... and we can see that in fact the test failure is not only a timeout problem.
• the published URLs follow the form https://[2001:1:2:3:4:5]:2156/activities/zope-0 and https://[2001:1:2:3:4:5]:2161/login/zope-0 (each family on a different port ). They are only published at the level of the balancer partition. Maybe we need to publish them up to the root partition. The balancer partition publishes something like this:

2018-10-17 09:13:06 slapos[16980] INFO slappart8: balancer (type balancer)
2018-10-17 09:13:06 slapos[16980] INFO     _ = {
  "activities": "https://10.0.30.235:2155",
  "login-test-runner-url-list": [
    "https://10.0.30.235:2158/unit_test_0/",
    "https://10.0.30.235:2158/unit_test_1/",
    "https://10.0.30.235:2158/unit_test_2/"
  ],
  "login-v6": "https://[2001:1:2:3:4:5]:2160",
  "monitor-base-url": "",
  "activities-v6": "https://[2001:1:2:3:4:5]:2155",
  "activities-direct-zope-url-list": [
    "https://[2001:1:2:3:4:5]:2156/activities/zope-0",
    "https://[2001:1:2:3:4:5]:2156/activities/zope-1",
    "https://[2001:1:2:3:4:5]:2156/activities/zope-2",
    "https://[2001:1:2:3:4:5]:2156/activities/zope-3"
  ],
  "login-direct-zope-url-list": [
    "https://[2001:1:2:3:4:5]:2161/login/zope-0",
    "https://[2001:1:2:3:4:5]:2161/login/zope-1"
  ],
  "login": "https://10.0.30.235:2160",
  "activities-test-runner-url-list": [
    "https://10.0.30.235:2153/unit_test_0/",
    "https://10.0.30.235:2153/unit_test_1/",
    "https://10.0.30.235:2153/unit_test_2/"
  ]
}

I don't understand why there's no progress on the PR upstream. Nothing blocks to apply the last requested change and merge. Then we upgrade Zope rather than wasting time with a slapos patch.

I don't understand why there's no progress on the PR upstream

If I understand correctly the "IPv6-no-port scenario" problem that was raised during review was solved and after this Tres Seaver approved the changes.

for https://github.com/zopefoundation/Zope/pull/314#discussion_r216144642 there was multiple alternative approaches discussed, but no real consensus on what's the best solution. I feel that everybody can accept any of the proposed solutions. When I commented I was just trying to revive the discussion and make thing progress. Personally, I'm really OK with any of the discussed solutions. I guess the impact on performance of each proposed solution should be similar.

I think it would be nice to amend to match testIPv6NoPort naming ( https://github.com/zopefoundation/Zope/pull/314#discussion_r216147948 ), it's minor, but it's "not cool" if we just ignore this comment.

I don't understand why there's no progress on the PR upstream.

I am the responsible one here probably, as I am not doing progress there.

I was taken over by another tasks, but now a window opens for me, I will try to proceed.

Thanks @luke and @jm , the PR was merged on master. So we still need to get it on 2.13 and to have a 2.13 release. I guess we have to make another PR for 2.13 branch.

For the records, I made the PR to 2.13 branch ( https://github.com/zopefoundation/Zope/pull/395 )

We could use a patched egg in meanwhile, with your fix. It is not bad so bad (considering it is only this).

mentioned in commit erp5@19da91e5

For this MR, I'd like to have a test before we merge.

But this idea of using a patched egg in the meantime might be a good solution. @vincentB also have a similar patch ( erp5@19da91e5 (comment 70604) ), applying these patches as egg patch might be a way to progress faster.

[edit grammar]

mentioned in commit nexedi/erp5@b955dbe0

this commit was applied to master independently, see nexedi/erp5@b955dbe0 (comment 72442)

mentioned in commit erp5@b955dbe0

a new Zope was released with the fix needed for this MR ( https://pypi.org/project/Zope2/#id1 )

/cc @kazuhiko

I pushed the change in erp5-component branch as a3f0151e. let's see test results.

The change is now merged into master.