Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get client IP when using Cloudflare #2723

Merged
merged 3 commits into from May 28, 2021

Conversation

ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle commented May 18, 2021

When proxying an app through Cloudflare, the user's real IP is set in the CF-Connecting-IP header. This is more convenient than having to manually maintain an allow-list of trusted proxies.

Reference: https://support.cloudflare.com/hc/en-us/articles/200170986-How-does-Cloudflare-handle-HTTP-Request-headers-

@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #2723 (e0e1d5c) into master (0cbb30a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2723   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          41       41           
  Lines        2070     2074    +4     
=======================================
+ Hits         2043     2047    +4     
  Misses         15       15           
  Partials       12       12           
Impacted Files Coverage Δ
gin.go 99.18% <ø> (ø)
context.go 97.65% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cbb30a...e0e1d5c. Read the comment docs.

@appleboy appleboy added this to the v1.8 milestone May 18, 2021
@appleboy appleboy requested a review from thinkerou May 18, 2021
@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented May 19, 2021

Thinking about it, I wonder if rather than using booleans, it might make more sense to create an enum platform (or something like that)?

This way, once the next platform is inevitably added, we don't add yet another boolean.

The downside is around what to do with the existing Google App Engine flag.

@thinkerou
Copy link
Member

thinkerou commented May 24, 2021

Thinking about it, I wonder if rather than using booleans, it might make more sense to create an enum platform (or something like that)?

This way, once the next platform is inevitably added, we don't add yet another boolean.

The downside is around what to do with the existing Google App Engine flag.

Yes, I also like enum

@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented May 24, 2021

I can update to use an enum-like.

What should I do for the existing Google App Engine flag? Keep that for legacy and "translate" that into the enum?

Copy link
Member

@thinkerou thinkerou left a comment

lgtm

@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented May 28, 2021

Wait before you merge.... I want to update it to use an enum. Before we introduce even more legacy. :) just let me know what I should do with the existing Boolean flag for Google App Engine

@thinkerou
Copy link
Member

thinkerou commented May 28, 2021

I can update to use an enum-like.

What should I do for the existing Google App Engine flag? Keep that for legacy and "translate" that into the enum?

@ItalyPaleAle I merged the pull request at first, please commit other pull request using(refactoring) enum, thanks!

@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented May 28, 2021

Ok. Will do it over the weekend or sooner.

What do you want me to do with the existing Boolean for GAE? I can't remove it because it would be backwards-incompatible. I can keep it as a legacy alias that then is translated to the enum?

@thinkerou
Copy link
Member

thinkerou commented May 28, 2021

@ItalyPaleAle please read https://github.com/gin-gonic/gin/blob/master/deprecated.go for reference, we should deprecated warning at first and remove.

@thinkerou thinkerou merged commit 6703dea into gin-gonic:master May 28, 2021
3 checks passed
@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented May 28, 2021

@thinkerou I made the change in #2739 hope this works!

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this issue Nov 22, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this issue Nov 22, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this issue Nov 22, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this issue Nov 22, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
thinkerou added a commit that referenced this issue Nov 23, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this issue Nov 23, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
thinkerou added a commit that referenced this issue Nov 24, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
thinkerou added a commit that referenced this issue Nov 24, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants