Add proxy protocol support for real client IP logging#919
Add proxy protocol support for real client IP logging#919freyes merged 4 commits intojuju:masterfrom
Conversation
fe9c94e to
49cd9a5
Compare
freyes
left a comment
There was a problem hiding this comment.
you are putting this feature behind a config option , does this mean all charms will be updated to include a new option in their config.yaml? ... can't we just make this the default behavior?, is there any cons on making this the default?
You're right. But I had a question during testing. For example, in the keystone charm, there are its own templates for apache2. e.g wsgi-openstack-api.conf and openstack_https_frontend.conf. I tested this, and it turns out that if a charm includes its own template files, it uses those instead of the ones from charmhelpers. Also, fixes would need to be applied across haproxy, apache templates, and modules all at once, since they're all interdependent. If we adopt conf option, we can split commit to sync with charmhelpers and change each charms. But yes. making this the default also makes sense to me as well. |
|
On Mon, 2025-03-24 at 17:55 -0700, Seyeong Kim wrote:
> you are putting this feature behind a config option , does this mean all charms will be updated
> to include a new option in their config.yaml? ... can't we just make this the default behavior?,
> is there any cons on making this the default?
You're right.
I actually thought the same at first, and it seemed fine to just make it the default behavior.
But I had a question during testing. For example, in the keystone charm, there are its own
templates for apache2. e.g wsgi-openstack-api.conf and openstack_https_frontend.conf.
I tested this, and it turns out that if a charm includes its own template files, it uses those
instead of the ones from charmhelpers.
So even if we make this the default behavior in charmhelpers, we'd still need to update each
individual charm that overrides the templates.
Also, fixes would need to be applied across haproxy, apache templates, and modules all at once,
since they're all interdependent.
So charmhelpers(sync with charm) and the keystone charm itself(templates update) should be updated
together, otherwise things will break I think.
If we adopt conf option, we can split commit to sync with charmhelpers and change each charms.
But yes. making this the default also makes sense to me as well.
—
Reply to this email directly, view it on GitHub [1], or unsubscribe [2].
You are receiving this because you commented.Message ID: <juju/charm-
***@***.***>
xtrusiaxtrusia
left a comment (juju/charm-helpers#919) [1]
> you are putting this feature behind a config option , does this mean all charms will be updated
> to include a new option in their config.yaml? ... can't we just make this the default behavior?,
> is there any cons on making this the default?
You're right.
I actually thought the same at first, and it seemed fine to just make it the default behavior.
But I had a question during testing. For example, in the keystone charm, there are its own
templates for apache2. e.g wsgi-openstack-api.conf and openstack_https_frontend.conf.
This is true, although not that common. AFAIK charm-keystone is an outlier here rather the norm.
I tested this, and it turns out that if a charm includes its own template files, it uses those
instead of the ones from charmhelpers.
So even if we make this the default behavior in charmhelpers, we'd still need to update each
individual charm that overrides the templates.
we would need the usual "charm-helper sync" dance, yes.
Also, fixes would need to be applied across haproxy, apache templates, and modules all at once,
since they're all interdependent.
So charmhelpers(sync with charm) and the keystone charm itself(templates update) should be updated
together, otherwise things will break I think.
If we adopt conf option, we can split commit to sync with charmhelpers and change each charms.
I'm not following you on this one, you could have a charmhelpers sync for keystone and a subsequent
patch that modifies the keystone templates, what makes you think things will break?
[1] view it on GitHub #919 (comment)
[2] unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAYLH7VUVJWBGAOXTTFH632WCSRVAVCNFSM6AAAAABZMOX736VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONBZG42TSMRRGE
|
|
@freyes If we can do both at once, no issue. So, would you like me to change it to default instead of adding config option? I'll change the fix tomorrow. Thanks so much |
freyes
left a comment
There was a problem hiding this comment.
something we haven't talked about is what happens when you have only side of the connection with the proxy protocol enabled, specifically:
- haproxy set with send-proxy-v2, but apache doesn't have remoteip enabled
- haproxy is NOT set with send-proxy-v2, but apache has enabled remoteip
I would assume in the case of (2) nothing happens, and the old behavior where the client ip address is not received by apache, right?, but what about in the case of (1)?
This is why I mentioned in the beginning they should be fixed at the same time.(for keystone) but sorry my explanation was not enough.
Test
Thanks so much. |
|
@freyes I'm wondering if you might check the latest comment and have some time to review this issue. Thanks for your review. |
4049608 to
be0ddac
Compare
|
re-added configuration option since we discussed it in sprint. |
It would also be great to explicitly document the following
Lastly, some review notes
|
98b458c to
1d9b517
Compare
It seems that haproxy and apache2 are tightly coupled so other components connect via haproxy only, they don't have to access to apache2 directly by proxyv2. Testing Notes
RemoteIPTrustedProxy 127.0.0.1 checks only x-forwarded-for. if I use below script(injecting fake client ip), it just pass.
As above structure in HA, I think security issue should be done by new ticket. |
4e58115 to
ba8a07a
Compare
|
The switch to HAProxy was using TCP mode to forward encrypted traffic to Apache, where TLS termination is configured. I verified this against a deployment with this PR applied to the Keystone charm and enabling https:
As a result, encrypted traffic is being sent to a frontend expecting plain HTTP which causes HTTPS to fail. |
@Raven-182 Thanks for the review. I originally went with PROXY Protocol v2 for client IP forwarding, but switched direction after noticing that a rolling upgrade in HA setups could cause a brief window where upgraded and non-upgraded units can't work as expected — since both ends(haproxy and apache in different units) need to speak the same protocol. Would love to hear your thoughts on that if we can fix the issue by using proxyv2. Thanks. |
|
@xtrusia , thanks for putting al this together, and @Raven-182 for reviewing and testing this. So this is my take is:
This should address all the concerns and unblock this PR(?) |
|
Hi @xtrusia, I’ve been testing this and verified that it works with the neutron-api charm when using TCP mode with PROXY protocol. I’m currently seeing some failing health checks, so I’m investigating the cause. I’ll propose a follow-up change for this once I have a fix. |
hello. Thanks! could you please share your test bundle if you have? i would like to check them as well. i tested it with keystone(also had its own templates) as the customer pointed out keystone. thanks |
The bundle I've been using for testing: neutron-api-ha.yaml |
|
@xtrusia I tested this with the neutron-api charm with TLS enabled. Without the Switching |
This is a really good point. I haven't checked v1 yet. I think we may need to add some tests based on your findings. I haven't had a chance to run your bundle yet, but I plan to do so soon. If you can provide more detailed steps, I can quickly follow your setup I'm curious if health check with v2 show "unsupported command 20" ? Thank you so much! |
|
Yes, with PROXYv2 enabled the logs did show "unsupported command 20" for healthchecks.
You will notice that healthchecks are not being performed, and you will need to add the |
Currently, log shows proxy IP but need actual client IP. This patch adds required module and configuration to setup options for Apache2 templates and haproxy templates. Use PROXY protocol v1 (send-proxy) instead of v2 (send-proxy-v2) to avoid Apache mod_remoteip incompatibility with PROXYv2 LOCAL command (unsupported command 0x20) during health checks. Also adds missing 'check' keyword to haproxy server lines to ensure health checks are properly performed. Closes-bug: LP#2107999 Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
Thanks, I uploaded patch with proxyv1 instead of proxyv2, |
|
Thanks @xtrusia . With this change, the proposed fix looks good to me. |
|
we need a patch like this |
Add ::1 to the RemoteIPProxyProtocolExceptions directive alongside 127.0.0.1 to properly handle IPv6 loopback connections in both the HTTPS frontend and WSGI templates. Closes-Bug: #2107999 Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
Pin setuptools<82.0 via constraints.txt and reference it in all tox test environments to avoid compatibility issues with newer setuptools versions. Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>






currently, log shows proxy ip but need actual client ip
This PR suggest to add required module and
configuration method to setup options for Apache2 templates
and options for haproxy templates
#918