Skip to content

Tidy: Complete TrustedProxies feature#2887

Merged
appleboy merged 4 commits into
gin-gonic:masterfrom
Bisstocuz:master
Oct 6, 2021
Merged

Tidy: Complete TrustedProxies feature#2887
appleboy merged 4 commits into
gin-gonic:masterfrom
Bisstocuz:master

Conversation

@Bisstocuz

@Bisstocuz Bisstocuz commented Sep 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Background

Related issues: #2814 #2697 #2819
Related pull requests: #2832 #2692

About #2692 :

This fix has 2 problems:

  1. it's not backward-compatible
  2. is doesn't update related documents

#2692 (comment) by @leafduo

What I do

So I did these things below in this PR:

  1. Update related documents.
  2. Make Engine.TrustedProxies private, force people to use Engine.SetTrustedProxies() to set proxies.
  3. Added proxies safety check and notices in Run, RunTLS, etc.
  4. Completed related unit tests.

This PR should be published with #2692 .

@Bisstocuz Bisstocuz changed the title Refactor: Complete TrustedProxies feature Tidy: Complete TrustedProxies feature Sep 29, 2021
@Bisstocuz

Bisstocuz commented Sep 29, 2021

Copy link
Copy Markdown
Contributor Author

Also hope you guys to help #2844 with code review.

@codecov

codecov Bot commented Sep 29, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2887 (5b8833e) into master (1a2bc0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2887   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files          41       41           
  Lines        3063     3065    +2     
=======================================
+ Hits         3025     3027    +2     
  Misses         26       26           
  Partials       12       12
Flag Coverage Δ
go-1.13 98.76% <100.00%> (+<0.01%) ⬆️
go-1.14 98.59% <100.00%> (+<0.01%) ⬆️
go-1.15 98.59% <100.00%> (+<0.01%) ⬆️
go-1.16 98.59% <100.00%> (+<0.01%) ⬆️
go-1.17 98.49% <100.00%> (+<0.01%) ⬆️
macos-latest 98.76% <100.00%> (+<0.01%) ⬆️
nomsgpack 98.74% <100.00%> (+<0.01%) ⬆️
ubuntu-latest 98.76% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
context.go 97.98% <ø> (ø)
gin.go 99.10% <100.00%> (+<0.01%) ⬆️

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 1a2bc0e...5b8833e. Read the comment docs.

@Bisstocuz

Bisstocuz commented Sep 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hello, help with code review please.
@zihengCat @leafduo @AlbinoGeek @menduo

@thinkerou thinkerou added this to the v1.8 milestone Sep 30, 2021
@appleboy appleboy modified the milestones: v1.8, v1.7.5 Oct 1, 2021
appleboy
appleboy previously approved these changes Oct 1, 2021
View reviewed changes
@appleboy

appleboy commented Oct 1, 2021

Copy link
Copy Markdown
Member

This PR should be merge to v1.7.5

cc @thinkerou

@Bisstocuz

Bisstocuz commented Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

@appleboy
Excuse me, I fixed some typo so you need to approve again plz.

thinkerou
thinkerou approved these changes Oct 5, 2021
View reviewed changes

@thinkerou thinkerou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@Bisstocuz

Bisstocuz commented Oct 5, 2021

Copy link
Copy Markdown
Contributor Author

@appleboy Hi, this PR can be merged.

@Bisstocuz Bisstocuz mentioned this pull request Oct 5, 2021
Closed
@appleboy appleboy merged commit 3918132 into gin-gonic:master Oct 6, 2021
@Bisstocuz Bisstocuz mentioned this pull request Oct 18, 2021
Open
@Bisstocuz Bisstocuz mentioned this pull request Nov 19, 2021
Closed
thinkerou pushed a commit that referenced this pull request Nov 20, 2021
@Bisstocuz @thinkerou
Tidy: Complete TrustedProxies feature (#2887)
d854503
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
@Bisstocuz
Tidy: Complete TrustedProxies feature (gin-gonic#2887)
fbf2024 
(cherry picked from commit 3918132)
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
@Bisstocuz
Tidy: Complete TrustedProxies feature (gin-gonic#2887)
a30e131 
(cherry picked from commit 3918132)
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
@Bisstocuz
Tidy: Complete TrustedProxies feature (gin-gonic#2887)
000e097 
(cherry picked from commit 3918132)
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
@Bisstocuz
Tidy: Complete TrustedProxies feature (gin-gonic#2887)
2461f92 
(cherry picked from commit 3918132)
thinkerou pushed a commit that referenced this pull request Nov 23, 2021
@Bisstocuz @thinkerou
Tidy: Complete TrustedProxies feature (#2887)
7de1f17
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
@Bisstocuz
Tidy: Complete TrustedProxies feature (gin-gonic#2887)
c3c1a01 
(cherry picked from commit 3918132)
thinkerou pushed a commit that referenced this pull request Nov 24, 2021
@Bisstocuz @thinkerou
Tidy: Complete TrustedProxies feature (#2887)
64aa23a
thinkerou pushed a commit that referenced this pull request Nov 24, 2021
@Bisstocuz @thinkerou
Tidy: Complete TrustedProxies feature (#2887)
fc5d6dd
@Bisstocuz Bisstocuz mentioned this pull request Nov 29, 2021
Merged
@ItalyPaleAle

ItalyPaleAle commented Dec 28, 2021

Copy link
Copy Markdown
Contributor

@thinkerou @appleboy
Please note that this:

Make Engine.TrustedProxies private, force people to use Engine.SetTrustedProxies() to set proxies.

Was a breaking change, and should not have been merged in a patch version. It did break people's code.

See https://go.dev/blog/v2-go-modules :

For projects which are declared stable — at major version v1 or higher — breaking changes must be done in a new major version.

daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
@Bisstocuz @daheige
Tidy: Complete TrustedProxies feature (gin-gonic#2887)
c8670a2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@appleboy appleboy appleboy approved these changes

@thinkerou thinkerou thinkerou approved these changes

@javierprovecho javierprovecho Awaiting requested review from javierprovecho

@manucorporat manucorporat Awaiting requested review from manucorporat

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

v1.7.5

Development

Successfully merging this pull request may close these issues.

4 participants

@Bisstocuz @appleboy @ItalyPaleAle @thinkerou