[1.3.x] MODCLUSTER-801 mod_proxy_cluster returns 404 now instead of 503 when there's no matching context

[jfclere]

Fix for https://issues.redhat.com/browse/MODCLUSTER-801

mod_proxy_cluster returns 404 now instead of 503 when there's no matching context

modcluster/mod_proxy_cluster#173

[jajik]

Few details, but ok overall.

Do we want to support this in 2.0 too? If so, I would consider the proposed naming.

[jfclere]

Return503WhenNoContext.... Return503NoContext?

[jajik]

NoContext503, NoContextCode503, NoContextGive503, NoContextResp503...

In theory, this could be parametrized, require one argument – the value, then it could be something like NoContextResponse, NoContextStatus, NoContextCode... + an argument.

[rhusar]

NoContext503, NoContextCode503, NoContextGive503, NoContextResp503...

In theory, this could be parametrized, require one argument – the value, then it could be something like NoContextResponse, NoContextStatus, NoContextCode... + an argument.

Then we can call it something like - 'ResponseStatusCodeOnNoContext' / 'ResponseStatusCodeWhenNoContext'. The question though is which of the codes are appropriate in this case anyway other than 503? Looking at e.g. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#5xx_server_errors, perhaps 502 or 504 - if there were a timeout waiting for contexts to register - which there should be anyway, no?

[rhusar]

@jfclere Shouldn't we be attempting to wait until context is available? IIUC we don't but we should - we are expecting to servers boot again and register again.

[jfclere]

@jfclere Shouldn't we be attempting to wait until context is available? IIUC we don't but we should - we are expecting to servers boot again and register again.

No we might wait "for ever" and get deny of service too easily.

[sgala]

NoContext503, NoContextCode503, NoContextGive503, NoContextResp503...
In theory, this could be parametrized, require one argument – the value, then it could be something like NoContextResponse, NoContextStatus, NoContextCode... + an argument.

Then we can call it something like - 'ResponseStatusCodeOnNoContext' / 'ResponseStatusCodeWhenNoContext'. The

I like the idea, it is clear on what the configuration does

question though is which of the codes are appropriate in this case anyway other than 503? Looking at e.g. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#5xx_server_errors, perhaps 502 or 504 - if there were a timeout waiting for contexts to register - which there should be anyway, no?

502 seems assumes that an invalid response was received from the server proxied, so I guess it should be 503 or 504.

[rhusar]

I think the changes are in the right direction of flexibility. However, this makes me wonder, if we shouldn't limit the response codes to a reasonable set? e.g. if someone configured code to be 600 and we don't sanitize it, that would violate the standard.

[jajik]

LGTM overall, but wouldn't it be better to have

static int responsecode_when_no_context = HTTPD_NOT_FOUND;

and then the block could be replaced with single

ap_log_error(APLOG_MARK, response_when_no_context == HTTPD_NOT_FOUND ? APLOG_DEBUG : 
             "proxy: CLUSTER: (%s). No context for the URL returns %d",
             (*balancer)->s->name, response_when_no_context);

?

[jfclere]

@rhusar invalid codes are replaced by 500 and an error message:
+++
Fri Dec 08 14:08:29.107638 2023] [core:error] [pid 80785:tid 140242959771328] [client 127.0.0.1:41278] AH00524: Handler for proxy-server returned invalid result code 666
+++

[jajik]

LGTM overall, but wouldn't it be better to have

static int responsecode_when_no_context = HTTPD_NOT_FOUND;

and then the block could be replaced with single

ap_log_error(APLOG_MARK, response_when_no_context == HTTPD_NOT_FOUND ? APLOG_DEBUG : 
             "proxy: CLUSTER: (%s). No context for the URL returns %d",
             (*balancer)->s->name, response_when_no_context);

?

The reason I ask is also that this would change the logging level only in case something different than the default is returned. The current PR would change the level even in case of

ResponseStatusCodeOnNoContext 404

but I would expect this should behave as the default one.

[jajik]

LGTM. Could you please just remove the unnecessary double blank space and squash those four commits into one before merging?

[rhusar]

@rhusar invalid codes are replaced by 500 and an error message: +++ Fri Dec 08 14:08:29.107638 2023] [core:error] [pid 80785:tid 140242959771328] [client 127.0.0.1:41278] AH00524: Handler for proxy-server returned invalid result code 666 +++

OK great, so that is sanitized.

[rhusar]

As @jajik says, please squash into a single commit.

[rhusar]

@jfclere Can you please prepare PR for https://github.com/modcluster/mod_proxy_cluster ?

[rhusar]

@aogburn Could you please give this a quick look (given you are the original reporter)? Thanks!