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

Enable X-Forwarded Headers. #748

Merged

Conversation

galaxy-sea
Copy link
Contributor

spring.cloud.loadbalancer.x-forwarded.enabled=true to Enable X-Forwarded Headers.

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #748 (787e386) into main (666a59d) will increase coverage by 0.62%.
The diff coverage is 88.23%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #748      +/-   ##
============================================
+ Coverage     78.48%   79.11%   +0.62%     
+ Complexity      549      543       -6     
============================================
  Files            66       66              
  Lines          2045     2030      -15     
  Branches        281      276       -5     
============================================
+ Hits           1605     1606       +1     
+ Misses          282      264      -18     
- Partials        158      160       +2     
Impacted Files Coverage Δ
...ign/loadbalancer/XForwardedHeadersTransformer.java 87.50% <87.50%> (ø)
...adbalancer/FeignLoadBalancerAutoConfiguration.java 100.00% <100.00%> (ø)
...gframework/cloud/openfeign/support/FeignUtils.java 80.00% <0.00%> (-6.67%) ⬇️
...amework/cloud/openfeign/FeignClientsRegistrar.java 76.92% <0.00%> (-1.93%) ⬇️
...enfeign/annotation/QueryMapParameterProcessor.java 87.50% <0.00%> (-1.39%) ⬇️
...amework/cloud/openfeign/support/SpringEncoder.java 84.14% <0.00%> (-0.74%) ⬇️
...mework/cloud/openfeign/FeignAutoConfiguration.java 90.14% <0.00%> (-0.14%) ⬇️
...gframework/cloud/openfeign/FeignClientBuilder.java 84.37% <0.00%> (ø)
...ork/cloud/openfeign/DefaultFeignLoggerFactory.java 100.00% <0.00%> (ø)
...eign/annotation/CookieValueParameterProcessor.java 94.11% <0.00%> (ø)
... and 5 more

@spencergibb
Copy link
Member

Can you please explain why this is needed? Feign isn't a proxy

@galaxy-sea
Copy link
Contributor Author

hello @spencergibb , RestTemplate migrates to OpenFeign. RestTemplate provides org.springframework.cloud.loadbalancer.blocking.XForwardedHeadersTransforme, but OpenFeign does not provide XForwardedHeadersTransformer

@galaxy-sea
Copy link
Contributor Author

Refer to RestTemplate and WebClient .

Check Set 'X-Forwarded-*' header when using loadbalanced RestTemplate issus, Don't know why OpenFeign doesn't provide support

@spencergibb

@galaxy-sea
Copy link
Contributor Author

hello @spencergibb , Please consider merging this PR

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Hello @galaxy-sea , thanks for submitting the PR. Looks good. There's a cosmetic issue - have added comment - please address. Also, please document this feature in spring-cloud-openfeign.adoc.

@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request and removed waiting for feedback labels Oct 17, 2022
@OlgaMaciaszek OlgaMaciaszek added this to In progress in 2022.0.x via automation Oct 17, 2022
@galaxy-sea
Copy link
Contributor Author

hello @OlgaMaciaszek , I have updated the document,

@galaxy-sea
Copy link
Contributor Author

galaxy-sea commented Oct 18, 2022

I think these documents should be placed in Spring Cloud Commons

@OlgaMaciaszek
Copy link
Collaborator

@galaxy-sea it is already documented there. But it's good to reiterate here, since we add the code separately (there's no full parity between the OF LB client and the Commons LB clients).

@galaxy-sea
Copy link
Contributor Author

@galaxy-sea it is already documented there. But it's good to reiterate here, since we add the code separately (there's no full parity between the OF LB client and the Commons LB clients).

@OlgaMaciaszek, Thank you for unlocking my doubts

@OlgaMaciaszek OlgaMaciaszek removed this from In progress in 2022.0.x Oct 18, 2022
@OlgaMaciaszek OlgaMaciaszek added this to the 4.0.0-RC1 milestone Oct 18, 2022
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @galaxy-sea . LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit e2c2e6c into spring-cloud:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants