Adds cors support

[schmetzyannick]

Related to #476

[CLAassistant]

All committers have signed the CLA.

[nitram509]

Hi @schmetzyannick ,
thank you for your contribution.
Would you please re-format your code to Google's Java code style?
This is because we would like to ensure consistent formatting in this project,
as outlined in https://github.com/camunda-community-hub/zeebe-simple-monitor/blob/main/CONTRIBUTING.md

[nitram509]

@schmetzyannick
looking at the proposed changes, I can't verify, whether this would help solve your problem.
Zeebe Simple Monitor is not providing CORS headers by default, so I wonder where your settings come from (#476).
Also, I believe just registering cors headers for websockets is just half the fix. It should be configured for the Spring web responses as well, to be consistent.

If you would extend your PR to also register Spring web (https://spring.io/guides/gs/rest-service-cors/) and Stomp
together, then it would be a "complete" feature.
Next to my other comments, it would be also helpful, to mention/document this new feature in the README.md

[schmetzyannick]

@nitram509 thanks for your quick feedback and sorry for my code. It is now 7 years ago that i wrote my last java code and I never worked with spring :S So please double check it ;)

Yes I think that the unset CORS properties for the REST connection is causing the second problem described in #476

[schmetzyannick]

@nitram509 deployments ar now working fine in k8s when I have only one gateway (see camunda helm chart), would you like an example.yaml file to setup simple monitor on k8s? And if so I would suggest a seperate PR?

[nitram509]

Hi @schmetzyannick
thanks for updating the PR.
I made some smaller comments and would be glad you incorporate them in the PR as well.
With these proposals applied, I would be good to go for this PR.

BUT, I wonder about the overall PR does not seem to work.
See my next comment (with details).

[nitram509]

@schmetzyannick

I did run your code locally and set this URL: server.allowedOriginsUrls=http://xxx:123/
I was able to confirm via debug breakpoints, your code changes did pick up and e.g. .allowedOrigins(allowedOriginsUrlArr); method was called.

I do expect, that I would now see HTTP response headers like this
Access-Control-Allow-Origin: http://xxx:123/
for all resources and websocket notification responses.
But actually, I can't confirm that. See attached screenshots.

How does it work for you?

[schmetzyannick]

Hi @nitram509,

I can confirm your screenshots for the REST connection. For my understanding, the vary: Access-Control-Request-Headers, set by the spring framework, is some sort of cache for the Access-Control-Allow-Origin header you expect to see.

spring-projects/spring-framework#20959
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary

In the WS connection the header is set as expected:

[nitram509]

@schmetzyannick thanks for shedding a light, but something I seem to miss...

I did set the ${server.allowedOriginsUrls} variable via the environment, and being able to confirm via debugger, that my settings are picked up, see

There you also see I've fetched your branch and run it locally. Both breakpoints are confirmed and show the URL you're using.

Unfortunately, the headers are not set...

Your screenshot does show the settings.

Is there anything left in your environment configuration, which might alter the behavior?

PS: I hesitate to merge until I know how to use/activate the feature.

[schmetzyannick]

Hi @nitram509,

I think you should setup an environement where you really perform a cors request. So the origin should differ from the hostname of the simple-monitore service. If you have a k8s cluster, I could provide my yaml file. Another possiblity would be a proxy on your machine which forwards requests to a VM, which then runs the simple-monitor service.

[schmetzyannick]

@nitram509 can i provide you something to get this pr merged?

[nitram509]

@schmetzyannick I's sorry for the delay.
I was able to test your changes, they do work perfectly, thanks for your last hint.

One minor point: the comment in the README is could be somewhat more obvious - it took me a while to spot the "s" difference.

For future contributions, please provide a test case as well.
I did my local testing by writing an automated test and will commit this on top.

Thanks for your contribution 👍
Best regards
Martin

[nitram509]

Minor issues, will solve them on my own.