Skip to content
This repository has been archived by the owner on Apr 27, 2021. It is now read-only.

Revert to using proxy_protocol_addr for real ip #1

Merged
merged 1 commit into from
Aug 10, 2017
Merged

Conversation

karanthukral
Copy link

What?

  • This is attempt at fixing the issue of not having access to the accurate client_id

  • This reverts to the changes made to the nginx config regarding the real_ip logic from Improve variable configuration for source IP address kubernetes/ingress-nginx#890

  • This does indeed fix the issue of real_ip - demo in staging

  • I am not sure if we could get this merged upstream since this change was made to rectify another issue but this could provide us with a fix while I try to ping the maintainer and try and work towards a fix?

cc/ @n1koo @ibawt thoughts?

@karanthukral
Copy link
Author

I commented on the issue again and tagged the author of the PR to at least provide more insight into the change he made. He hasn't really commented even after Niko's ping. 🤞

@n1koo
Copy link

n1koo commented Aug 10, 2017

I think upstream will fix this at some point. Its pretty obvious oversight, eg. if your SNI things go through your 443->442 hack that uses proxy protocol thats what you gotta use underneath.

PR looks good for us

@karanthukral
Copy link
Author

Do we want to setup a CI pipeline for this or hand build the container locally and push? I can set it up on pipa otherwise

@ibawt
Copy link

ibawt commented Aug 10, 2017

pipa if you can, for bin auth

@n1koo
Copy link

n1koo commented Aug 10, 2017

We have builds for https://github.com/Shopify/shopify-docker-images/tree/master/ingress so you could just sync this for that

@karanthukral
Copy link
Author

We have builds for https://github.com/Shopify/shopify-docker-images/tree/master/ingress so you could just sync this for that

I didn't notice that. I'll just sync that to upstream and make the change

@ibawt
Copy link

ibawt commented Aug 10, 2017

might even be worth just cloning the repo as part of the build process. then we don't need to sync, and concievably we can just change the target to the upstream one for binauth'd nginx's doo-dads.

@karanthukral karanthukral merged commit 80cc81e into master Aug 10, 2017
@karanthukral karanthukral deleted the proxy_addr branch August 10, 2017 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants