DBZ-8101 Initial implementation of signal REST API for DS #118
Conversation
|
@jpechane since this introduces an insecure REST API, should we add some way of disabling/preventing access now or in a separate PR? |
jcechace
force-pushed
the
DBZ-8101
branch
2 times, most recently
from
f713bf7
to
9c8162f
Compare
|
Fixed the test failure, so everything should be passing now when run through maven. In IntelliJ the situation is reversed now -- while integration tests work, unit tests are broken. Fun fact: Adding surefire configuration besides failsafe causes idea to pick up only that -- which in turns breaks integrations tests again |
|
@vjuranek well, let me know if you know how to do that. Otherwise the toggle is going to cause the API to simply return 305 as if signaler were not available |
I guess you mean 503 and IMHO it's fine (read: I don't know how to completely turn it off:-). Basically we just need to make sure Debezium server won't accept any signals unless it's explicitly configured to do so, with assumption the admin knows what he does when turning it on. |
|
@vjuranek done, though we should probably consider disabled as the default value. |
| @ConfigMapping(prefix = "debezium") | ||
| public interface DebeziumServerConfig { | ||
| Api api(); | ||
|
|
||
| interface Api { | ||
| @WithDefault("false") | ||
| boolean enabled(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Eventually this should grow into a proper configuration mapping for the whole DS. It's about time we start slowing clearing up the implementation.
| @@ -0,0 +1,3 @@ | |||
| debezium.api.enabled=false | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Just for context. So, the problem is that by this file needs to be renamed and modified -- which means it can't really be used to set defaults as user may easily omit them.
However we do place this file on class path and thus run into a problem with classloader order -- although I believe that the application.properties inside the jar will be loaded first (and thus the one we add in startup script would be able to override the values inside it) it is not something we should be relying on.
The main issue is that we shouldn't be adding the configuration file on cp in the first place. Quarkus (or rather smallrye config) will also automatically recognise config/application.properties and give it a higher precedence than cp config by default. However, to make things more complicated -- we don't just add the config file on cp, but rather the entire conf directory since while application.properties doesn't have to be on cp, things like conf/cassandra/driver.conf do.
There was a problem hiding this comment.
The discussed idea of not changing this at the moment and just relying on default in code will also not be feasible -- while it would generally work for this specific property, it pretty much means that we would not be able to do partial configuration mapping to a java class (which is cleaner than having bunch of @ConfigProperty randomly in the code) as that needs unknown property validation to be disabled (see the next line smallrye.config.mapping.validate-unknown=false).
Thus as annoying as the change is, we do need to sort this out
There was a problem hiding this comment.
Opened #120 which needs to be merged prior to this
|
@jcechace Please rebase on the latest main and verify that the PR still works and if yes then feel free to merge yourself. |
https://issues.redhat.com/browse/DBZ-8101