Tests: Use custom docker/podman network, rework provisioning of containers, and more tweaks #357
Conversation
There was a problem hiding this comment.
The reasons for keeping these comments in here, while I understand the motivation for cleanup, is that when a new version of Tomcat is released, it's easier to compare the two server.xml files and adapt to what has changed.
Set HostNameLookups On to get hostname in %{REMOTE_HOST}, no IP
Drop hardcoded IP addresses, no localhost (except client <-> proxy)
Tomcat container now does not exit when tomcat process is stopped
Minor files & format clean ups
Containers are now using custom docker network with DNS Containers are now using their container names instead of localhost/IPs Parameters/Options now reflect the actual usage
Shift tomcat ports where conflicts might lead to random test failures Code and format clean ups
Also test a little bit more than before UseAlias is not enabled by default in the rest of the testsuite now
Unfortunately, custom podman networks do not support rDNS so tomcat requests are handled by their IP. This causes our `Require host` to not work which leads to test failures. Upon httpd container start, we try to detect podman and if needed, we change `Require host` to `Require ip`.
jfclere
left a comment
jfclere
left a comment
There was a problem hiding this comment.
The problem is more the mix of variable set + call of function on the same line and variable set in several line and then the call of the function. We should use a single one and prefer a single line set + calls (more easy to maintain.
test/MODCLUSTER-734/testit.sh
Outdated
|
|
||
| MPC_CONF=${MPC_CONF:-MODCLUSTER-734/mod_proxy_cluster.conf} | ||
| MPC_NAME=MODCLUSTER-734 httpd_start | ||
| MPC_NAME=MODCLUSTER-734 |
There was a problem hiding this comment.
That looks wrong to me...
MPC_NAME=MODCLUSTER-734 httpd_start
the httpd_start will have MPC_NAME MODCLUSTER-734
MPC_NAME=MODCLUSTER-734
httpd_start
MPC_NAME will be the default value mod_proxy_cluster
I don't think we want that!
There are many places with that modification that looks wrong to me.
There was a problem hiding this comment.
Well that "works" because we source the script containing httpd_start, but that will fail for any script we call without sourcing it...
There was a problem hiding this comment.
The reason for setting it on a separate line is that then it's set for the whole file.
foo() {
echo "VALUE is ${VALUE:-default}"
}
foo
VALUE=test
foo
VALUE=demo foo
foowill print
VALUE is default
VALUE is test
VALUE is demo
VALUE is testThe tomcat_start function uses MPC_NAME so that tomcat knows which host corresponds to the proxy. Alternatively we could set the variable for each tomcat_start & other commands that use it, but this looked more elegant to me.
EDIT: Added default to the example.
There was a problem hiding this comment.
Moved the declaration on the same line in 16daf05 until we set everything clearly at the beginning in the future.
Before all configuration variables will be moved at the beginning.
3 participants
This is a bigger PR but because of the nature of changes, there is not much what to do about it (I've already dropped some changes for future PRs).
The main thing is that host network is no longer used in containerized tests. This allows us to use DNS, isolate tests to a different network (minimizing conflicts with other running services).
In points:
close #348