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

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 13:37
@ItalyPaleAle
Copy link
Contributor Author

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

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

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

Choose a reason for hiding this comment

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

lgtm

@ItalyPaleAle
Copy link
Contributor Author

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

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

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

@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
@ItalyPaleAle
Copy link
Contributor Author

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

Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request 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 pull request 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 pull request 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 pull request Nov 22, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
thinkerou added a commit that referenced this pull request Nov 23, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
(cherry picked from commit 6703dea)
thinkerou added a commit that referenced this pull request Nov 24, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
thinkerou added a commit that referenced this pull request Nov 24, 2021
Co-authored-by: thinkerou <thinkerou@gmail.com>
@ItalyPaleAle ItalyPaleAle deleted the codespace-0046 branch January 7, 2024 22:35
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.

3 participants