ErnestOrt / Trampoline

Admin Spring Boot Locally
http://ernestort.github.io/Trampoline/
Apache License 2.0
356 stars 82 forks source link

Including '/' in front of actuator prefix resolves to incorrect logfile url #74

Closed Damian-R-Smith closed 5 years ago

Damian-R-Smith commented 5 years ago

When setting a new instance, if you enter your prefix with '/' (eg. '/my-actuator') this will result in the Log button for that instance redirecting to :

http://\<host>:\<port> // myactuator/logfile

The placeholder for this input indicates that this format is what the user should enter, but irregardless I think this input field should be sanitized as part of validation to anticipate incorrectly formatted data.

I'll commit a suggested fix soon, but wanted to add the issue here for tracking purposes.

ErnestOrt commented 5 years ago

Thanks @Damian-R-Smith for your feedback and time!

Feel free to create a PR for this issue! Do not forget to jot your twitter account down here, we will mention you on next release (if you want, of course).

Happy coding and Nollaig Shona Dhuit :)

Cheers!

Damian-R-Smith commented 5 years ago

Havn't used twitter in quite a while, but no harm to add it: @Javawocky

Nollaig faoi shéan is faoi shonas duit! 😄

ErnestOrt commented 5 years ago

Some things @Damian-R-Smith

Trampoline was failing when using actuator url (collecting metrics, checking status, killing instances)

"/" character was expected so, since your changes remove it, all those actions were ending up being a 404.

Saying that, things done:

  1. Encapsulate url generation on Instance object:

https://github.com/ErnestOrt/Trampoline/blob/master/trampoline/src/main/java/org/ernest/applications/trampoline/entities/Instance.java

  1. Avoid DRY

https://github.com/ErnestOrt/Trampoline/blob/master/trampoline/src/main/java/org/ernest/applications/trampoline/utils/SanitizeActuatorPrefix.java

  1. Sanitize Actuator Prefix on getters and setters Instance and Microservice objects:

https://github.com/ErnestOrt/Trampoline/blob/master/trampoline/src/main/java/org/ernest/applications/trampoline/entities/Instance.java

https://github.com/ErnestOrt/Trampoline/blob/master/trampoline/src/main/java/org/ernest/applications/trampoline/entities/Microservice.java

  1. Create a Unit test https://github.com/ErnestOrt/Trampoline/blob/master/trampoline/src/test/java/org/ernest/SanitizeActuatorPrefixTest.java

The thing here, is that the same situation would be faced by a spring 1.x user or if you have your endpoint defined on the root... anyway I guess can be leaved like this :)

Damian-R-Smith commented 5 years ago

Appreciate you doing a review, your additions are much cleaner too!

I'm not sure I fully understood your last point though, do you mean this is a potential future defect with the other endpoints for spring 1.x users?

The thing here, is that the same situation would be faced by a spring 1.x user or if you have your endpoint defined on the root...

ErnestOrt commented 5 years ago

Simply, that at instances.html, line 194, if actuatorPrefix =="" then you will still see:

http://<host>:<port> // myactuator/logfile

Anyway, since 2.x spring boot version already have a default actuatorPrefix (/actuator), I think is fine to prioritze current spring boot features rather than deal with old compatibilities, although is good to keep them in mind and mitigate if possible.