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

View logs if spring-cloud-data-flow-server is running behind a reverse proxy is not possible #1994

Closed
klopfdreh opened this issue Apr 3, 2024 · 20 comments
Labels
type/bug Is a bug report
Milestone

Comments

@klopfdreh
Copy link
Contributor

klopfdreh commented Apr 3, 2024

Description:
If you use spring-cloud-data-flow-server behind a reverse proxy it is not possible to view the logs within the ui as the "_links" are calculated from the backend with its own domain name.

Release versions:
2.11.2

Custom apps:
N/A

Steps to reproduce:
Use spring-cloud-data-flow-server behind a reverse proxy.

Screenshots:
N/A

Additional context:
PR in SCDF UI will be provided to fix this issue

@github-actions github-actions bot added the status/need-triage Team needs to triage and take a first look label Apr 3, 2024
@cppwfs cppwfs added type/feature Is a feature request and removed status/need-triage Team needs to triage and take a first look labels Apr 8, 2024
@cppwfs cppwfs added this to the 3.4.3 milestone Apr 8, 2024
@cppwfs cppwfs added type/bug Is a bug report and removed type/feature Is a feature request labels Apr 8, 2024
@corneil
Copy link
Contributor

corneil commented Apr 9, 2024

@klopfdreh Is your reverse proxy configured to add headers: X-Forwarded-Proto, X-Forwarded-Port

@klopfdreh
Copy link
Contributor Author

@klopfdreh Is your reverse proxy configured to add headers: X-Forwarded-Proto, X-Forwarded-Port

Yes, but the way the API is implemented the request does not respect any headers but uses only the domain of the backend. See the PR for more details.

@corneil
Copy link
Contributor

corneil commented Apr 10, 2024

@klopfdreh Is your reverse proxy configured to add headers: X-Forwarded-Proto, X-Forwarded-Port

Yes, but the way the API is implemented the request does not respect any headers but uses only the domain of the backend. See the PR for more details.

Have you tried server.forward-headers-strategy=NATIVE and server.forward-headers-strategy=FRAMEWORK?

@corneil
Copy link
Contributor

corneil commented Apr 10, 2024

@klopfdreh Is your reverse proxy configured to add headers: X-Forwarded-Proto, X-Forwarded-Port

Yes, but the way the API is implemented the request does not respect any headers but uses only the domain of the backend. See the PR for more details.

If you have multiple load-balancers en-route the addition of x-forwarded headers should be disabled on the intermediate LBs/ingress servers.

@klopfdreh
Copy link
Contributor Author

I am going to check it out and give feedback. 👍

@klopfdreh
Copy link
Contributor Author

Hey @corneil,

I checked both server settings, now. With server.forward-headers-strategy=NATIVE and with server.forward-headers-strategy=FRAMEWORK the URLs are still shipped with the domain of the backend but not with the reverse proxy if you access the endpoint through the reverse proxy.

image

@corneil
Copy link
Contributor

corneil commented May 6, 2024

Can the ingress on k8s be configured to leave the x-forwarded headers in place instead of replacing with the cluuster dns?

@klopfdreh
Copy link
Contributor Author

klopfdreh commented May 7, 2024

The issue here is that the Angular frontend does not send the header to tell the backend to use the scheme / domain / port from the reverse proxy. (No x-forwarded-for)

List of all request headers:
image

To my understanding the headers must be added, or we can use my code to adjust the url to fit the one of the frontend without any infrastructure changes.

@corneil
Copy link
Contributor

corneil commented May 7, 2024

@klopfdreh
The reverse proxy has to be configured to add the x-forwarded-* headers and the loadbalancer/ingress has to be configured to not override the x-forwarded-* headers

@corneil
Copy link
Contributor

corneil commented May 7, 2024

@klopfdreh
Copy link
Contributor Author

I am going to ask if we can apply those changes and report back. 👍 But maybe it would be nice to have this option as well - in this case (with my changes) no reverse proxy changes are required.

@onobc
Copy link
Contributor

onobc commented May 10, 2024

@klopfdreh we are going to push this to the next release as we are still not sure which direction we want to go and we are starting the release process in the next 1-2 days. Let's see how things are once the changes are applied (if that is possible). Thank you for your patience.

@onobc onobc modified the milestones: 3.4.3, 3.4.x May 10, 2024
@klopfdreh
Copy link
Contributor Author

Hey @onobc - no problem - we are also having a look at this issue and try to adjust some things in our infra. 👍

@klopfdreh
Copy link
Contributor Author

Hey @onobc / @corneil

I know that in normal cases if you configure the backend correctly (tomcat) and apply the header in the reverse proxy, the client should translate the response _links to the reverse-proxy domain

However, I asked our infra team and it seems that they can set the header but be able not fill them. So I kindly ask if there is a way to implement this feature as sort of compatiblitiy mode for reverse proxies which are not able to set those headers? So in the settings a toggle and if you activate it the header all of the _link urls is translated the way I implemented it.

That would be awesome.

@onobc
Copy link
Contributor

onobc commented May 15, 2024

Hi @klopfdreh ,
I would not be opposed to adding the feature in an opt-in fashion. I am not sure when we will be able to get to it. If you could adjust the code proposal to include this, that would help w/ timeline.

Thanks,
Chris

@klopfdreh
Copy link
Contributor Author

Hey @onobc - no problem - almost done. 😄

@klopfdreh
Copy link
Contributor Author

klopfdreh commented May 15, 2024

@onobc - done 👍 - just have a look at the PR.

@onobc
Copy link
Contributor

onobc commented May 17, 2024

Wow @klopfdreh - that was fast. I hope that I did not mislead you but we are currently in the process of releasing 2.11.3 (UI 3.4.3) and will look at this once the release is complete.

@klopfdreh
Copy link
Contributor Author

Hey @onobc - no problem - I am just so glad that this feature is accepted. 👍

@klopfdreh
Copy link
Contributor Author

klopfdreh commented May 27, 2024

@onobc - as the PR was reviewed by @oodamien and the 2.11.4 release is out - I think we can merge it to be in 2.11.5 and close the issue.

@onobc onobc closed this as completed in f0f0820 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Is a bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants