Merge ~michal-maloszewski99/ubuntu/+source/apache2:apache2-jammy-fix-proxy-hcheck into ubuntu/+source/apache2:ubuntu/jammy-devel

[Michał Małoszewski]

PPA:
https://launchpad.net/~michal-maloszewski99/+archive/ubuntu/lp1998311-apache2-mod-proxy-ppa-jammy-v2

[Michał Małoszewski (michal-maloszewski99)]

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy/?format=plain)
  apache2 @ amd64:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy/jammy/amd64/a/apache2/20230302_093708_af353@/log.gz
    02.03.23 09:37:08 ✅ Triggers: apache2/2.4.52-1ubuntu4.4
  apache2 @ arm64:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy/jammy/arm64/a/apache2/20230302_094854_45cbf@/log.gz
    02.03.23 09:48:54 ✅ Triggers: apache2/2.4.52-1ubuntu4.4
  apache2 @ armhf:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy/jammy/armhf/a/apache2/20230302_094322_9a95f@/log.gz
    02.03.23 09:43:22 ✅ Triggers: apache2/2.4.52-1ubuntu4.4
  apache2 @ ppc64el:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy/jammy/ppc64el/a/apache2/20230302_091503_af353@/log.gz
    02.03.23 09:15:03 ✅ Triggers: apache2/2.4.52-1ubuntu4.4
  apache2 @ s390x:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy/jammy/s390x/a/apache2/20230302_091322_af353@/log.gz
    02.03.23 09:13:22 ✅ Triggers: apache2/2.4.52-1ubuntu4.4

[Michał Małoszewski (michal-maloszewski99)]

Working on SRU template right now

[Michał Małoszewski (michal-maloszewski99)]

Link to the SRU template
https://docs.google.com/document/d/1YSULtNENTPS0DGe7uxL7PrCRzqGnS0Ba52pY2jjzaGc/edit?usp=sharing

[Bryce Harrington (bryce)]

Thanks for including the draft of the SRU text. I've got a few suggestions but they won't affect the review of the packaging so will give them separate.

[Impact]

What the SRU team looks for here is more of an explanation of why the bug deserves to be accepted as SRU. So, explaining how it impacts end users - i.e. how it disrupts their usage of the software - is important. Sometimes that requires explaining acronyms or other jargon that may not be generally known by Ubuntu developers; Wikipedia often comes to the rescue here *grin*.

However, at the same time I'd avoid using judgement words like "impossible", "unusable", "randomly", etc. that bug reporters frequently tend to use. They might mean one thing to one person, and something different to another. Usually these can be restated in a more precise way without too much difficulty.

One non-obvious thing about the impact section, is that the SRU team doesn't really care about how the bug occurs or how it is fixed, that's almost like an implementational detail. They suggest moving that to a [Discussion] section, particularly if the explanation is wordy, but if possible I usually try to include a brief statement about how the issue is fixed.

So, with all that, I'd suggest editing the impact section a bit:

[Impact]
The Apache JServe Protocol (AJP) proxies inbound requests to an application server, such as health checks via CPING/CPONG. mod_proxy_hcheck added support for AJP/CPING in 22.04, however the following error is encountered when enabling it, which prevents the new feature from operating:

   BalancerMember Health check method CPING not (yet) implemented

This is caused by a incorrect check for AJP support in hc_post_config() that occurs too late, after the configuration syntax has already been marked invalid by the time the "hcmethod=CPING" token is found. The fix is to move the check from hc_post_config() to hc_pre_config().

[Where problems could occur]

What you've written is all true, but much of that is generically true of any SRU. So, I don't think it hurts to include it, but I think what the SRU team is looking for is more about what is distinctive of *this* SRU.

Often I like to think of this section as answering the question, if I were an Apache2 user and updated to this version of the package, and something weird started happening, what types of bugs would I reasonably suspect are this update's fault, vs. ones that are likely to be unrelated? Usually to answer that you consider what the patch actually does, and if there was an undiscovered error in the code what kinds of failures would we expect to see?

So in this case, the patch itself modifies the code of mod_proxy_hcheck, so any new bugs involving that module would be suspect. The patch changes configuration code, so issues cropping up that seem related to module configuration could be suspect. Finally, since the patch modifies C code, issues typical of C code (segfaults, memory leaks, etc.) would be plausible, however since this moves a chunk of code unmodified this seems extremely unlikely.

[Test Plan]
The test case looks great. I'll run through it while doing the packaging review, but on read throug...

[Bryce Harrington (bryce)]

One thing I'd recommend when filing MP's, is to include a link to the PPA in your MP description, just as a convenience.

You only need one PPA, not two. A PPA can hold multiple packages each for a different Ubuntu release. So you can upload both the kinetic and jammy versions of your SRU package to the same PPA. Doing it with two PPAs doesn't cause any problems, so it's ok, but by convention we typically have a single PPA per SRU.

I would also recommend putting the PPA installation directions into the [Test Case]. I.e.:

    $ sudo add-apt-repository -yus ppa:michal-maloszewski99/apache2-ppa-jammy-lp2003189
    $ sudo apt-get -y upgrade

I would put that in the "Example of successful output" section.

[Bryce Harrington (bryce)]

When I ran the [Test Case], on both kinetic and jammy, there seems to be an error in the config snippet, even with your PPA installed I got an error:

root@kinetic-test:~# apachectl -t
(2)No such file or directory: AH02291: Cannot access directory '/etc/apache2/logs/' for error log of vhost defined at /etc/apache2/sites-enabled/httpd-hcheck-ajp.conf:1
AH00014: Configuration check failed
Action '-t' failed.
The Apache error log may have more information.

To fix this, I changed the snippet to prefix APACHE_LOG_DIR, like this:

<VirtualHost *:80>
    ServerAdmin <email address hidden>
    DocumentRoot "/var/www/html"
    ServerName myapp.example.com
    ErrorLog "${APACHE_LOG_DIR}/myapp.example.com-error_log"
    CustomLog "${APACHE_LOG_DIR}/myapp.example.com-access_log" common

    <Proxy balancer://myapp>
        BalancerMember ajp://127.0.0.1:8009/myapp route=app-route timeout=300 ping=3 connectiontimeout=3 hcmethod=CPING
    </Proxy>

    ProxyPass /myapp balancer://myapp stickysession=JSESSIONID

</VirtualHost>

Did the test case work ok for you with just 'ErrorLog “logs/myapp.example.com-error_log”'? From what I've googled, it looks like that should work but it didn't for me.

[Bryce Harrington (bryce)]

Turning to the package, the packaging is solid, good work.

I've sponsored the upload:

stirling: ~/pkg/Apache2/review-lp1998311/apache2-gu$ ls ../*source.changes
../apache2_2.4.52-1ubuntu4.4_source.changes ../apache2_2.4.54-2ubuntu1.2_source.changes
stirling: ~/pkg/Apache2/review-lp1998311/apache2-gu$ grep ^Vcs* ../*source.changes
../apache2_2.4.52-1ubuntu4.4_source.changes:Vcs-Git: https://git.launchpad.net/~bryce/ubuntu/+source/apache2
../apache2_2.4.52-1ubuntu4.4_source.changes:Vcs-Git-Commit: 94e5eb899badd54bb4004c1ab66c54218c5f1c94
../apache2_2.4.52-1ubuntu4.4_source.changes:Vcs-Git-Ref: refs/heads/apache2-jammy-fix-proxy-hcheck
../apache2_2.4.54-2ubuntu1.2_source.changes:Vcs-Git: https://git.launchpad.net/~bryce/ubuntu/+source/apache2
../apache2_2.4.54-2ubuntu1.2_source.changes:Vcs-Git-Commit: 15929b2d6a8c8a9e239f081d5254555c5e4e57f4
../apache2_2.4.54-2ubuntu1.2_source.changes:Vcs-Git-Ref: refs/heads/apache2-kinetic-fix-proxy-hcheck
stirling: ~/pkg/Apache2/review-lp1998311/apache2-gu$ dput ubuntu ../apache2_2.4.52-1ubuntu4.4_source.changes
D: Setting host argument.
Checking signature on .changes
gpg: ../apache2_2.4.52-1ubuntu4.4_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: ../apache2_2.4.52-1ubuntu4.4.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apache2_2.4.52-1ubuntu4.4.dsc: done.
  Uploading apache2_2.4.52-1ubuntu4.4.debian.tar.xz: done.
  Uploading apache2_2.4.52-1ubuntu4.4_source.buildinfo: done.
  Uploading apache2_2.4.52-1ubuntu4.4_source.changes: done.
Successfully uploaded packages.
stirling: ~/pkg/Apache2/review-lp1998311/apache2-gu$ dput ubuntu ../apache2_2.4.54-2ubuntu1.2_source.changes
D: Setting host argument.
Checking signature on .changes
gpg: ../apache2_2.4.54-2ubuntu1.2_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: ../apache2_2.4.54-2ubuntu1.2.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apache2_2.4.54-2ubuntu1.2.dsc: done.
  Uploading apache2_2.4.54-2ubuntu1.2.debian.tar.xz: done.
  Uploading apache2_2.4.54-2ubuntu1.2_source.buildinfo: done.
  Uploading apache2_2.4.54-2ubuntu1.2_source.changes: done.
Successfully uploaded packages.

You can update the SRU text as you wish, and when ready go ahead and subscribe the SRU team to the bug in order to proceed with the next steps.

review: Approve

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: bryce, michal-maloszewski99
Uploaders: bryce
MP auto-approved

review: Approve

[Michał Małoszewski (michal-maloszewski99)]

Version changed from 2.4.52-1ubuntu4.4 to 2.4.52-1ubuntu4.5.
Autopkgtests work fine.

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy-v2/?format=plain)
  apache2 @ amd64:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy-v2/jammy/amd64/a/apache2/20230314_202208_c9926@/log.gz
    14.03.23 20:22:08 ✅ Triggers: apache2/2.4.52-1ubuntu4.5~ppa3
  apache2 @ arm64:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy-v2/jammy/arm64/a/apache2/20230314_203556_ead2e@/log.gz
    14.03.23 20:35:56 ✅ Triggers: apache2/2.4.52-1ubuntu4.5~ppa3
  apache2 @ armhf:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy-v2/jammy/armhf/a/apache2/20230314_201241_fcd9d@/log.gz
    14.03.23 20:12:41 ✅ Triggers: apache2/2.4.52-1ubuntu4.5~ppa3
  apache2 @ ppc64el:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy-v2/jammy/ppc64el/a/apache2/20230314_195411_ead2e@/log.gz
    14.03.23 19:54:11 ✅ Triggers: apache2/2.4.52-1ubuntu4.5~ppa3
  apache2 @ s390x:
    http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-michal-maloszewski99-lp1998311-apache2-mod-proxy-ppa-jammy-v2/jammy/s390x/a/apache2/20230314_200459_5c93e@/log.gz
    14.03.23 20:04:59 ✅ Triggers: apache2/2.4.52-1ubuntu4.5~ppa3

[Michał Małoszewski (michal-maloszewski99)]

Needs to be sponsored again.

[Bryce Harrington (bryce)]

You need to set the status of this MP to Merged, Michal.

[Michał Małoszewski (michal-maloszewski99)]

Done, thx Bryce

diff --git a/debian/changelog b/debian/changelog
index 6f0da90..56903d8 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+apache2 (2.4.52-1ubuntu4.5) jammy; urgency=medium
+
+  * d/p/mod_proxy_hcheck_jammy_fix_to_detect_support.patch: Fix issue
+    where enabling mod_proxy_hcheck results in error (LP: #1998311)
+
+ -- Michal Maloszewski <[e-mail]>  Wed, 01 Mar 2023 23:43:55 +0100
+
 apache2 (2.4.52-1ubuntu4.4) jammy-security; urgency=medium
 
   * SECURITY UPDATE: HTTP request splitting with mod_rewrite and mod_proxy
diff --git a/debian/patches/mod_proxy_hcheck_jammy_fix_to_detect_support.patch b/debian/patches/mod_proxy_hcheck_jammy_fix_to_detect_support.patch
 new file mode 100644
index 0000000..e3b79f5
--- /dev/null
+++ b/debian/patches/mod_proxy_hcheck_jammy_fix_to_detect_support.patch
@@ -0,0 +1,56 @@
+From eaafacd0efc6bae4e2a9de616bf487b8a55437c1 Mon Sep 17 00:00:00 2001
+From: Eric Covener <[e-mail]>
+Date: Tue, 29 Nov 2022 13:24:16 +0000
+Subject: [PATCH] Merge r1904516 from trunk:
+Description: Fix the mod_proxy_hcheck module
+Origin: backport, https://github.com/apache/httpd/commit/eaafacd0efc6bae4e2a9de616bf487b8a55437c1
+Bug: https://bz.apache.org/bugzilla/show_bug.cgi?id=66300
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/apache2/+bug/1998311
+Last-Update: 2022-03-01
+---
+ modules/proxy/mod_proxy_hcheck.c | 23 ++++++++++++-----------
+ 1 file changed, 12 insertions(+), 11 deletions(-)
+
+--- a/modules/proxy/mod_proxy_hcheck.c
++++ b/modules/proxy/mod_proxy_hcheck.c
+@@ -1070,6 +1070,18 @@
+     hctp = NULL;
+     tpsize = HC_THREADPOOL_SIZE;
+ #endif
++
++    ajp_handle_cping_cpong = APR_RETRIEVE_OPTIONAL_FN(ajp_handle_cping_cpong);
++    if (ajp_handle_cping_cpong) {
++       proxy_hcmethods_t *method = proxy_hcmethods;
++       for (; method->name; method++) {
++           if (method->method == CPING) {
++               method->implemented = 1;
++               break;
++           }
++       }
++    }
++
+     return OK;
+ }
+ static int hc_post_config(apr_pool_t *p, apr_pool_t *plog,
+@@ -1126,17 +1138,6 @@
+         s = s->next;
+     }
+ 
+-    ajp_handle_cping_cpong = APR_RETRIEVE_OPTIONAL_FN(ajp_handle_cping_cpong);
+-    if (ajp_handle_cping_cpong) {
+-       proxy_hcmethods_t *method = proxy_hcmethods;
+-       for (; method->name; method++) {
+-           if (method->method == CPING) {
+-               method->implemented = 1;
+-               break;
+-           }
+-       }
+-    }
+-
+     return OK;
+ }
+ 
+--- /dev/null
++++ b/mod_proxy_hcheck_jammy_fix_to_detect_support.patch
+@@ -0,0 +1 @@
++
diff --git a/debian/patches/series b/debian/patches/series
index ff7092d..305f31a 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -31,3 +31,4 @@ CVE-2022-37436.patch
 CVE-2023-25690-1.patch
 CVE-2023-25690-2.patch
 CVE-2023-27522.patch
+mod_proxy_hcheck_jammy_fix_to_detect_support.patch