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

preserve host header + set forward-headers-strategy FRAMEWORK #109

Merged

Conversation

edevosc2c
Copy link
Member

Related to #104

  1. This has been discussed to set forward-headers-strategy to FRAMEWORK in order to pass through the x-forwarded headers sent by the reverse proxy.
    A deep explanation is available here: https://stackoverflow.com/a/68318932
  2. And also not touching the host header sent by the reverse proxy using the parameter PreserveHostHeader

@edevosc2c
Copy link
Member Author

edevosc2c commented Mar 13, 2024

Just launched the tests but still the same error as in the other PR:

| [INFO] Running org.georchestra.gateway.autoconfigure.accounts.RabbitmqEventsAutoConfigurationTest
| 14:47:10.639 [main] WARN  o.s.a.r.c.CachingConnectionFactory - ***
| Automatic Recovery was Enabled in the provided connection factory;
| while Spring AMQP is generally compatible with this feature, there
| are some corner cases where problems arise. Spring AMQP
| prefers to use its own recovery mechanisms; when this option is true, you may receive
| 'AutoRecoverConnectionNotCurrentlyOpenException's until the connection is recovered.
| It has therefore been disabled; if you really wish to enable it, use
| 'getRabbitConnectionFactory().setAutomaticRecoveryEnabled(true)',
| but this is discouraged.
| 14:47:10.705 [main] INFO  o.s.a.r.c.CachingConnectionFactory - Attempting to connect to: test.rabbit:3333
| 14:47:11.037 [main] INFO  o.s.a.r.l.SimpleMessageListenerContainer - Broker not available; cannot force queue declarations during start: java.net.UnknownHostException: test.rabbit: Name or service not known
| 14:47:11.048 [org.springframework.amqp.rabbit.config.ListenerContainerFactoryBean#0-1] INFO  o.s.a.r.c.CachingConnectionFactory - Attempting to connect to: test.rabbit:3333
| 14:47:11.051 [org.springframework.amqp.rabbit.config.ListenerContainerFactoryBean#0-2] INFO  o.s.a.r.l.SimpleMessageListenerContainer - Waiting for workers to finish.
| 14:47:11.051 [org.springframework.amqp.rabbit.config.ListenerContainerFactoryBean#0-2] INFO  o.s.a.r.l.SimpleMessageListenerContainer - Successfully waited for workers to finish.
| [INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.701 s - in org.georchestra.gateway.autoconfigure.accounts.RabbitmqEventsAutoConfigurationTest
| [INFO] Running org.georchestra.gateway.accounts.admin.ldap.GeorchestraLdapAccessConfigurationTest
| [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.054 s - in org.georchestra.gateway.accounts.admin.ldap.GeorchestraLdapAccessConfigurationTest
| [INFO] 
| [INFO] Results:
| [INFO] 
| [ERROR] Failures: 
| [ERROR]   GeorchestraGatewayApplicationTests.errorCustomizerReturnsServiceUnavailableInsteadOfServerError:117 Status expected:<503> but was:<500>
| [INFO] 
| [ERROR] Tests run: 99, Failures: 1, Errors: 0, Skipped: 0
| [INFO] 
| [INFO] ------------------------------------------------------------------------
| [INFO] BUILD FAILURE
| [INFO] ------------------------------------------------------------------------
| [INFO] Total time:  15.848 s
| [INFO] Finished at: 2024-03-13T14:47:11Z
| [INFO] ------------------------------------------------------------------------
| [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.22.2:test (default-test) on project georchestra-gateway: There are test failures.
| [ERROR] 
| [ERROR] Please refer to /home/edevos/Documents/synced/clients/georchestra/georchestra-gateway/gateway/target/surefire-reports for the individual test results.
| [ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
| [ERROR] -> [Help 1]
| [ERROR] 
| [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
| [ERROR] Re-run Maven using the -X switch to enable full debug logging.
| [ERROR] 
| [ERROR] For more information about the errors and possible solutions, please read the following articles:
| [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
| make: *** [Makefile:7: test] Error 1
[Build and publish docker image/build]   ❌  Failure - Main Build gateway

It's the PreserveHostHeader that is causing the issue.

@f-necas
Copy link
Collaborator

f-necas commented Apr 9, 2024

I think we should get commits from #105 to get CI working.

@pmauduit
Copy link
Member

pmauduit commented Apr 9, 2024

I think we should get commits from #105 to get CI working.

but @groldan said it was a patch on top of a patch, and I tend to agree with him, we should not have the need for them.

Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

as discussed in the meeting, let's go with this approach and add the host header in all tests

@pmauduit
Copy link
Member

as discussed in the meeting, let's go with this approach and add the host header in all tests

meaning "merging" this PR with #105 / getting commits from #105 in this one

@f-necas f-necas force-pushed the preserve-host-header-forwarded-headers-strategy-framework branch from 96f4492 to 2fab77b Compare May 17, 2024 13:33
edevosc2c and others added 3 commits May 28, 2024 15:40
Following #104, the `Host` http header became mandatory, else throwing a
NPE in Netty's code when reaching the NettyRoutingFilter code:

```
java.lang.NullPointerException: value
	at io.netty.util.internal.ObjectUtil.checkNotNull(ObjectUtil.java:39)
	Suppressed: The stacktrace has been enhanced by Reactor, refer to additional information below:
Error has been observed at the following site(s):
	*__checkpoint ⇢ org.springframework.cloud.gateway.filter.WeightCalculatorWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.authorization.AuthorizationWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.authorization.ExceptionTranslationWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.authentication.logout.LogoutWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.savedrequest.ServerRequestCacheWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.context.SecurityContextServerWebExchangeWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.authentication.AuthenticationWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.context.ReactorContextWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.header.HttpHeaderWriterWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.config.web.server.ServerHttpSecurity$ServerWebExchangeReactorContextWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.web.server.WebFilterChainProxy [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.boot.actuate.metrics.web.reactive.server.MetricsWebFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ org.springframework.security.test.web.reactive.server.SecurityMockServerConfigurers$MutatorFilter [DefaultWebFilterChain]
	*__checkpoint ⇢ HTTP GET "/path/..." [ExceptionHandlingWebHandler]
Original Stack Trace:
		at io.netty.util.internal.ObjectUtil.checkNotNull(ObjectUtil.java:39)
		at io.netty.handler.codec.DefaultHeaders.fromObject(DefaultHeaders.java:1117)
		at io.netty.handler.codec.DefaultHeaders.addObject(DefaultHeaders.java:364)
		at io.netty.handler.codec.http.DefaultHttpHeaders.add(DefaultHttpHeaders.java:115)
		at org.springframework.cloud.gateway.filter.NettyRoutingFilter.lambda$filter$0(NettyRoutingFilter.java:139)
		at reactor.netty.http.client.HttpClient.headers(HttpClient.java:1038)
		at org.springframework.cloud.gateway.filter.NettyRoutingFilter.filter(NettyRoutingFilter.java:133)
		at org.springframework.cloud.gateway.handler.FilteringWebHandler$GatewayFilterAdapter.filter(FilteringWebHandler.java:137)
		at org.springframework.cloud.gateway.filter.OrderedGatewayFilter.filter(OrderedGatewayFilter.java:44)
		at org.springframework.cloud.gateway.handler.FilteringWebHandler$DefaultGatewayFilterChain.lambda$filter$0(FilteringWebHandler.java:117)
```

Tests: `mvn clean verify` on gateway subdir went fine 3 times in a row.

Notes:

* It might deserve a trace in the documentation that the `Host`
  header is now mandatory, somehow ?
* I did not have to patch every WebTestClient calls strangely enough,
  only the one expecting a `200 - OK` return code.
@f-necas f-necas force-pushed the preserve-host-header-forwarded-headers-strategy-framework branch from 8dd855c to 6d35e43 Compare May 28, 2024 13:40
@f-necas
Copy link
Collaborator

f-necas commented May 28, 2024

Force-push : rebased on master

@f-necas f-necas merged commit e331633 into main May 28, 2024
3 checks passed
@f-necas f-necas deleted the preserve-host-header-forwarded-headers-strategy-framework branch May 28, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants