mod_remoteip doesn't add RemoteIpHeader in vhost

[mwinkens]

Describe the Bug

When using apache::mod::remote_ip the vhost doesn't get edited and the keywords aren't added. Therefore the remoteip isn't working, because the X-Forwarded-For header isn't set if apache get's used by a loadbalancer

Expected Behavior

I expect the RemoteIPHeader X-Forwarded-For to be set in the vhost if the module is used

Steps to Reproduce

Steps to reproduce the behavior:

1. create an apache managed with puppet and served by a loadbalancer which also uses the X-Forwarded-For Header
2. Use the apache::mod::remote_ip
3. See, that it's not working because a flag is missing in the vhost

Environment

• Version [e.g. 1.27.0]
• Platform [e.g. Ubuntu 18.04]

Additional Context

Add any other context about the problem here.

[ekohl]

I wouldn't expect a module to modify a vhost. Instead it deploys a mod config, which sets a default. Isn't that enough? If not, I'd think that apache::vhost needs a parameter to enable it.

[mwinkens]

I know, that the mod config is written, but in my testing it wasn't applied. Also it set RemoteIPInternalProxy to 127.0.0.1 which is undocumented behavior, as it states in the reference that it's undef by default

After changing it to the real loadbalancer IP it still didn't work, only after turning off the puppet agent and manually adding the vhost option it was running again and the server was receiving real IP adresses

$ cat /etc/apache2/mods-available/remoteip.conf 
# Declare the header field which should be parsed for useragent IP addresses
RemoteIPHeader X-Forwarded-For

# Declare client intranet IP addresses trusted to present
# the RemoteIPHeader value
RemoteIPInternalProxy 127.0.0.1

[mwinkens]

I tested this on a smaller test system and it seems to work there BUT I didn't use puppet and set it up manually without the vhost variable RemoteIPHeader and with the remoteip.conf. I didn't set the RemoteIPInternalProxy, so my guess is, that the issue comes from the undocumented behavior of setting this while being undef and I didn't test it right before (maybe the puppet agent was running and setting it back)

So the real issue is: Puppet is setting RemoteIPInternalProxy to 127.0.0.1 while the documentation says the default value is undef.

[ekohl]

That would be here.
https://github.com/puppetlabs/puppetlabs-apache/blob/03c507fa3d0ac3afb33cd997ffd9dd57ac035b93/manifests/mod/remoteip.pp#L73C4-L73C4

I agree it's a confusing way. Prior to 4e4bad0 it was clearer. I don't understand why it was implemented this way.

[Ramesh7]

That would be here. https://github.com/puppetlabs/puppetlabs-apache/blob/03c507fa3d0ac3afb33cd997ffd9dd57ac035b93/manifests/mod/remoteip.pp#L73C4-L73C4

I agree it's a confusing way. Prior to 4e4bad0 it was clearer. I don't understand why it was implemented this way.

Looks like before the change introduced, the $proxy_ips was defaulting to ['127.0.0.1'] and I think thats what the new change kept that with introducing more customizing.
Ref : https://github.com/puppetlabs/puppetlabs-apache/pull/1882/files#diff-028c1d52a4575aafbdadecbbf6fdf6de097f6e2fd4a3ad0312f292c45085f1dfL3
So based on my understanding this is unchanged behaviour but I think the documentation needs to get updated aligned with what code does.
Please correct if I am wrong here. Thanks

[ekohl]

That would be here. https://github.com/puppetlabs/puppetlabs-apache/blob/03c507fa3d0ac3afb33cd997ffd9dd57ac035b93/manifests/mod/remoteip.pp#L73C4-L73C4
I agree it's a confusing way. Prior to 4e4bad0 it was clearer. I don't understand why it was implemented this way.

Looks like before the change introduced, the $proxy_ips was defaulting to ['127.0.0.1'] and I think thats what the new change kept that with introducing more customizing. Ref : https://github.com/puppetlabs/puppetlabs-apache/pull/1882/files#diff-028c1d52a4575aafbdadecbbf6fdf6de097f6e2fd4a3ad0312f292c45085f1dfL3 So based on my understanding this is unchanged behaviour but I think the documentation needs to get updated aligned with what code does. Please correct if I am wrong here. Thanks

Previously the reference documentation did specify it, because it was a default. Now it's using a very obscure way to set a default. And I sort of get it: it's undef when proxy_ips is passed. So short term better documentation is needed. Long term the deprecated parameters should be removed and parameter defaults should be used again.

[ekohl]

I opened #2512 to remove the deprecated parameters and properly make it clear that ['127.0.0.1'] is used as a default value.