[1.3.x] MODCLUSTER-824 Improve handling of overlapping aliases in APP commands #840

jajik · 2024-10-21T09:12:45Z

Resolves MODCLUSTER-824

Opening as a draft since it's a backport.

rhusar

Please update the commit message so that the Jira ID is the first in the commit message.

…P commands

jajik · 2024-10-21T11:43:48Z

ref modcluster/mod_proxy_cluster#290

jajik · 2024-10-24T14:55:19Z

Upstream is merged, marking ready for review

rhusar

Can we please validate the removal behavior? Just to make sure there isn't a side-effect that we are missing.

jajik · 2024-10-30T09:01:19Z

Can we please validate the removal behavior? Just to make sure there isn't a side-effect that we are missing.

Before the patch, two virtual hosts with overlapping aliases are created. When REMOVE-APP is sent to the overlapping alias, only one of the hosts gets removed. (That does no affect the case when the remove command is sent with /* path, then both get removed.)

With this PR, only one virtual hosts is present because in case of an overlap, the two get merged. Then the removal removes the only host as expected.

jajik · 2024-12-12T12:37:25Z

@rhusar any updates on this?

rhusar · 2024-12-16T20:40:59Z

native/mod_manager/mod_manager.c

+        while (host == NULL && i + start < strlen(vhost->host)) {
+            while (vhost->host[start+i] != ',' && vhost->host[start+i] != '\0') {
+                i++;
+            }
+
+            strncpy(hostinfo.host, vhost->host + start, i);
+            hostinfo.host[i] = '\0';
+            host = read_host(hoststatsmem, &hostinfo);
+            start = start + i + 1;
+            i = 0;
+        }


@jajik Sorry for delay, but I am under suspicion this is still a little wrong / problematic. While the 'matching' fix is correct and should really result in 1 vhost if any of the aliases are overlapping - the result bit should not be a merge, but a replace.

E.g. an existing vhost with aliases A,B receives update with B,C, the resulting vhost should be B,C rather than a merge of A,B,C.

Let me know if I misunderstood.

@jfclere What do you think? May that result in some node infighting?

@rhusar hm do we want to merge?... because if the back-end send a ENABLE context/hosts, so you are thinking of:

/test A,B

/other B, C
that means you have in the back-end
VM1 : A, B
VM2: B, C
So a request to /test on A should go to test on host1 and a request to /test on C (host2) should give a 404 (well look to find other context/hosts).

so merging looks wrong that way... B, C also looks wrong no?

host1, host2 are jvmroutes and A, B, C aliases in this case?

and:
/test A,B (VM1)
/test B,C (VM2)
Should be rejected : we don't know how to route /test B ...

But in case of different JVMRoutes the merge does not happen.

@jajik correct.

It would create

Node host1

Contexts

/test

Aliases

A, B

Node host2

Contexts

/other

Aliases

B, C

@rhusar @jajik so code is correct.
VM1 /test A, B
some configuration changes
VM1 /test B, C
We should not route A / test there correct?

the merge would happen only if new ENABLE-APP with /test B, C went from host1, so that it would be like this afterwards

Node host1

Contexts

/test

Aliases

A, B, C

Node host2

Contexts

/other

Aliases

B, C

instead of the current behaviour

Node host1

Context

/test

Aliases

A, B

Context

/test

Aliases

B, C

Node host2

Contexts

/other

Aliases

B, C

jajik · 2024-12-17T10:16:33Z

Alright, so it seems we have come to a conclusion that replace is the correct action (instead of the proposed merge). I'll fix this PR once I fix the upstream.

jajik · 2024-12-17T11:40:57Z

Alright, so it seems we have come to a conclusion that replace is the correct action (instead of the proposed merge). I'll fix this PR once I fix the upstream.

Or maybe not. In case of following configuration of tomcat

      <Host name="localhost" appBase="webapps"
            unpackWARs="true" autoDeploy="true">
            <Alias>default.example.com</Alias>
        </Host>

      
       <Host name="testing"  appBase="webapps-example"
            unpackWARs="true" autoDeploy="true">
            <Alias>default.example.com</Alias>
        </Host>

the merge seems the right way...

jfclere · 2024-12-17T13:18:38Z

in server.xml:

+++
      <Host name="A"    appBase="Aapps" unpackWARs="true" autoDeploy="true">
         <Alias>B</Alias>
      </Host>
      <Host name="B" appBase="Bapps" unpackWARs="true" autoDeploy="true">
         <Alias>C</Alias>
      </Host>
+++

deploy demo-1.0.war in HostA... you have in mod_proxy_cluster Alias a, b.
deploy test.war in HostB... you have in mod_proxy_cluster Alias a, b, c .
The above looks wrong demo-1.0 was never for c and test was never for a.

undeploy test.war in HostB.... you still have Alias a, b, c ... so c still has demo-1.0 which is wrong.

jajik · 2024-12-17T13:35:49Z

in server.xml:
+++
      <Host name="A"    appBase="Aapps" unpackWARs="true" autoDeploy="true">
         <Alias>B</Alias>
      </Host>
      <Host name="B" appBase="Bapps" unpackWARs="true" autoDeploy="true">
         <Alias>C</Alias>
      </Host>
+++
deploy demo-1.0.war in HostA... you have in mod_proxy_cluster Alias a, b. deploy test.war in HostB... you have in mod_proxy_cluster Alias a, b, c . The above looks wrong demo-1.0 was never for c and test was never for a.

undeploy test.war in HostB.... you still have Alias a, b, c ... so c still has demo-1.0 which is wrong.

Right, so we have multiple levels at which both the current state and the proposed fix are not working. 😄 I'll mark this as draft, we should think it through first.

rhusar · 2024-12-17T13:42:17Z

Right, so we have multiple levels at which both the current state and the proposed fix are not working. 😄 I'll mark this as draft, we should think it through first.

A partial fix went into upstream/main, so just make sure we don't forget to fix there too. 👍🏼

jfclere · 2024-12-17T15:34:57Z

Another funny thing:

      <Host name="A"    appBase="Aapps" unpackWARs="true" autoDeploy="true">
         <Alias>B</Alias>
      </Host>
      <Host name="B" appBase="Bapps" unpackWARs="true" autoDeploy="true">
         <Alias>A</Alias>
      </Host>

deploy demo-1.0 in Aapps, then in Bapps, wait and later undeploy demo-1.0 in Aapps, the VirtualHost is gone :-(

I don't see how to handle this "configuration error"...

jfclere · 2024-12-17T15:45:12Z

     <Host name="A"    appBase="Aapps" unpackWARs="true" autoDeploy="true">
         <Alias>B</Alias>
      </Host>
      <Host name="A" appBase="Bapps" unpackWARs="true" autoDeploy="true">
         <Alias>A</Alias>
      </Host>

gives an error in tomcat, as expected.

jajik · 2024-12-17T15:55:17Z

gives an error in tomcat, as expected.

Hm, but it gives an error due to name=A conflict, but having alias/name conflict is not problem. So this should be fixed in the java part as well.

Although thinking about it, maybe this is something to fix in tomcat directly, isn't it?

rhusar requested changes Oct 21, 2024

View reviewed changes

Resolve MODCLUSTER-824: Improve handling of overlapping aliases in AP…

af4d92a

…P commands

jajik force-pushed the MODCLUSTER-824 branch from bcda1b1 to af4d92a Compare October 21, 2024 09:23

jajik requested a review from rhusar October 21, 2024 09:23

rhusar changed the title ~~[1.3.x] Improve handling of overlapping aliases in APP commands~~ [1.3.x] MODCLUSTER-824 Improve handling of overlapping aliases in APP commands Oct 21, 2024

jajik added the 1.3.x Issues and PRs for the 1.3.x branch label Oct 21, 2024

jajik added the bug label Oct 21, 2024

jajik marked this pull request as ready for review October 24, 2024 14:55

jajik assigned rhusar Oct 25, 2024

jfclere approved these changes Oct 29, 2024

View reviewed changes

rhusar requested changes Oct 29, 2024

View reviewed changes

jajik requested a review from rhusar December 6, 2024 14:11

rhusar requested changes Dec 16, 2024

View reviewed changes

jajik marked this pull request as draft December 17, 2024 13:35

jajik mentioned this pull request Dec 17, 2024

Follow up on MODCLUSTER-824: Fix the behaviour once we agree on it modcluster/mod_proxy_cluster#310

Open

jfclere self-requested a review December 17, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.3.x] MODCLUSTER-824 Improve handling of overlapping aliases in APP commands #840

[1.3.x] MODCLUSTER-824 Improve handling of overlapping aliases in APP commands #840

jajik commented Oct 21, 2024

rhusar left a comment

jajik commented Oct 21, 2024

jajik commented Oct 24, 2024

rhusar left a comment

jajik commented Oct 30, 2024

jajik commented Dec 12, 2024

rhusar Dec 16, 2024

jajik Dec 17, 2024

jfclere Dec 17, 2024 •

edited

Loading

jajik Dec 17, 2024

jfclere Dec 17, 2024

jajik Dec 17, 2024

jfclere Dec 17, 2024

jajik Dec 17, 2024

jfclere Dec 17, 2024

jajik Dec 17, 2024 •

edited

Loading

jajik commented Dec 17, 2024 •

edited

Loading

jajik commented Dec 17, 2024

jfclere commented Dec 17, 2024 •

edited

Loading

jajik commented Dec 17, 2024

rhusar commented Dec 17, 2024

jfclere commented Dec 17, 2024 •

edited

Loading

jfclere commented Dec 17, 2024

jajik commented Dec 17, 2024

[1.3.x] MODCLUSTER-824 Improve handling of overlapping aliases in APP commands #840

Are you sure you want to change the base?

[1.3.x] MODCLUSTER-824 Improve handling of overlapping aliases in APP commands #840

Conversation

jajik commented Oct 21, 2024

rhusar left a comment

Choose a reason for hiding this comment

jajik commented Oct 21, 2024

jajik commented Oct 24, 2024

rhusar left a comment

Choose a reason for hiding this comment

jajik commented Oct 30, 2024

jajik commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfclere Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jajik Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

jajik commented Dec 17, 2024 • edited Loading

jajik commented Dec 17, 2024

jfclere commented Dec 17, 2024 • edited Loading

jajik commented Dec 17, 2024

rhusar commented Dec 17, 2024

jfclere commented Dec 17, 2024 • edited Loading

jfclere commented Dec 17, 2024

jajik commented Dec 17, 2024

jfclere Dec 17, 2024 •

edited

Loading

jajik Dec 17, 2024 •

edited

Loading

jajik commented Dec 17, 2024 •

edited

Loading

jfclere commented Dec 17, 2024 •

edited

Loading

jfclere commented Dec 17, 2024 •

edited

Loading