Skip to content

Conversation

@JonBendtsen
Copy link
Contributor

This PR introduces 2 new environment variables that will control apaches configuration. The changes happens at the very end of docker-run.sh just before apache is started.

APACHE_REMOTEIP_CONF configures apaches remote ip module for which header it should consider for knowing the true remote ip. Useful if you have a proxy server in front of your dolibarr. If this variable is set, and not empty, then it will also enable the remoteip module - because if you set the configuration, surely you want it enabled.

APACHE_MODULES is supposed to be a single space character separated list of apache modules that you want enabled. It will just for loop through the list and a2enmod them one by one. Order may matter if modules depends on other modules.

If you want remoteip module enabled, but not configured you can use this environment variable to enable it in apache.

@creekorful creekorful self-assigned this Mar 20, 2025
| **DOLI_CRON_KEY** | | Security key launch cron jobs
| **DOLI_CRON_USER** | | Dolibarr user used for cron jobs
| **DOLI_INSTANCE_UNIQUE_ID** | | Secret ID used as a salt / key for some encryption. By default, it is set randomly when the docker container is created.
| **APACHE_REMOTEIP_CONF** | empty or variable not set | If this variable is set and it is not empty, then the full contents of this variable will be echoed to /etc/apache2/mods-available/remoteip.conf and then a2enmod remoteip will be run
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this option.

Instead we should let the user enable the module using APACHE_MODULES env variable and then tweak the load the configuration file like described here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this option.

Instead we should let the user enable the module using APACHE_MODULES env variable and then tweak the load the configuration file like described here.

I disagree. Even if people can do it the other way, this way is neater and easier for most people, and more and more people run stuff in containers, more and more will run it inside Kubernetes where it will be behind a proxy - or even outside Dolibarr they are more likely to run a proxy into multiple different containers - dolibarr being one of them.

Therefore there is a need for this apache container as easily as possible will log the real IP address which is connected to Dolibarr.

With remoteip it is not enough to enable the module, you also actually have to configure which header is used to store the real IP address in, and that should be as easy as possible Having an environment variable for that is much better than mounting a file at the right location. AND it also enables the module, so you get 2 for 1.

| **DOLI_CRON_USER** | | Dolibarr user used for cron jobs
| **DOLI_INSTANCE_UNIQUE_ID** | | Secret ID used as a salt / key for some encryption. By default, it is set randomly when the docker container is created.
| **APACHE_REMOTEIP_CONF** | empty or variable not set | If this variable is set and it is not empty, then the full contents of this variable will be echoed to /etc/apache2/mods-available/remoteip.conf and then a2enmod remoteip will be run
| **APACHE_MODULES** | empty or variable not set | If this variable is set and not empty, then all the apache modules in this variable will be installed using a2enmod. Apache module names should be separated by a single space character.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename this to APACHE_ENABLED_MODULES.

And add APACHE_DISABLED_MODULES for a list of module we want to disable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about it, I expect to have more people use the remoteIP than adding or removing other modules. And if we recommend that people disable certain modules by default, why not just permanently disable those modules (see my other PR's).

@creekorful
Copy link
Member

creekorful commented Mar 20, 2025

Hello @JonBendtsen, thanks for your PR!

I added some comments in order to make the implementation more flexible. However I ask myself:

Is it really needed for the base image to provide such functionality? Wouldn't it bloat too much the image feature wise?

Another thought: If enabling a module requires a configuration file, then the end user will need to bundle it too. In this case wouldn't it be better if the end user simply build a custom Docker image enabling the needed modules and copying the configuration file alongside ?

Do you have any opinion @tuxgasy ?

@creekorful creekorful added the question Further information is requested label Mar 20, 2025
@tuxgasy
Copy link
Contributor

tuxgasy commented Mar 20, 2025

Hello,

I ask myself too. But what about for all vars PHP_INI_* ? I'm torn.

Now this type of customization (for Apache and PHP) can be easily done with a custom init script without build a custom image.

This change will not make the image bigger because there are no additional packages. But each such addition may lengthen the service startup time.

@creekorful
Copy link
Member

Hi @tuxgasy,

Thanks for pointing to the custom init script. With this approach it is definitely easy to custom PHP and Apache behavior without any complications, therefore I see less interest for this PR. I think I would prefer to keep the base image and scripts as simple as possible.

@JonBendtsen
Copy link
Contributor Author

Hello @JonBendtsen, thanks for your PR!

I added some comments in order to make the implementation more flexible. However I ask myself:

Is it really needed for the base image to provide such functionality? Wouldn't it bloat too much the image feature wise?

Another thought: If enabling a module requires a configuration file, then the end user will need to bundle it too. In this case wouldn't it be better if the end user simply build a custom Docker image enabling the needed modules and copying the configuration file alongside ?

I think it is much easier and faster for someone to use an environment variable to configure remoteip that I expect more and more people to actually use.

Not every module will need special configuration - or they already have the configuration they mostly use. This is not the same with remoteip, there is no remoteip.conf file in /etc/apache2/mods-available/ - there is only the .load file, so the configuration is very much used.

@JonBendtsen
Copy link
Contributor Author

This change will not make the image bigger because there are no additional packages. But each such addition may lengthen the service startup time.

I don't think the start up time will be annoyingly longer, and most likely the dolibarr container will be running for months or years - so let's focus on making it as easy as possible to get the configuration safe and useful.

Comment on lines +483 to +486
if [[ "" != "${APACHE_REMOTEIP_CONF}" ]]; then
echo "RemoteIPHeader X-Forwarded-For" > /etc/apache2/mods-available/remoteip.conf
a2enmod remoteip
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay that's maybe why It never worked out of the box for me
having X-Client-IP as an optional value for this line would be cool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamdes what never worked out of the box for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I confused my containers, it is nextcloud that has some remoteip stuff configured via ENV and it does not work

But https://github.com/Dolibarr/dolibarr-docker?tab=readme-ov-file#running-your-dolibarr-behind-a-proxy
Works fine, but the header name should be an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamdes you can put any header name in the file that you mount into this location

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR only if it checks that the file does not exist yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamdes don't worry, I don't think this PR will get in, I felt that the consensus was against it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see..
Maybe check nextcloud?
I did not have the time to investigate why my docker logs have not got the remote ip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants