fix: added controller config validation #2103
Conversation
Signed-off-by: Daniel Soifer <[email protected]>
Signed-off-by: Daniel Soifer <[email protected]>
|
Fixes: #2086 |
|
|
||
| func ValidateConfig(config *GlobalConfig) error { | ||
| if len(config.supportedJetStreamVersions()) == 0 { | ||
| return fmt.Errorf("no jetstream versions were provided in the controller config") |
There was a problem hiding this comment.
won't this error always throw in case u are using just STAN?
There was a problem hiding this comment.
The controller crashes because there's no EventBusConfig.NATS or EventBusConfig.JetStream is nil, I feel like we only need to add some check along with error returning - the error should be displayed in the EventBus status field. WDYT?
There was a problem hiding this comment.
@whynowy As I see it, there are 2 approaches we can go with: either fail the controller on startup or reflect the error somehow during reconcile. I'm ok with both approaches but I think it would probably make more sense to fail on startup when the controller is provided with an invalid config (as any service would fail when provided with an invalid config) because it would simply prevent the controller from functioning properly. Also, today we face an invalid eventbus config which prevents the controller from reconciling eventbuses but in the future it might be some other config that would make the controller pod crash or prevent it from reconciling other components as well. For that reason I think it might be better to have a general place where we validate the global configuration instead of spreading those checks across different places, and to run the validation as part of the "readiness" requirements.
There was a problem hiding this comment.
@whynowy btw, checking if either EventBusConfig.NATS or EventBusConfig.JetStream are nil would also do the trick and prevent the nil deference crash that we have atm but versions is still a mandatory field for both nats streaming and jestream, and without versions the controller would still fail on every reconcile attempt with a failed to get jetstream/nats streaming version, err: jetstream/nats streaming version configuration not found error. From a user's perspective I think this error might be a bit confusing, especially when it occurs during the reconcile process, as the user might think there was something wrong with the version he provided in the EventBus spec and not in the controller config (which I'm not sure that most users aware about). WDYT?
There was a problem hiding this comment.
@tooptoop4 This change would only validate the global configuration that is provided to the controller. You would still be able to only use stan eventbus. We could support providing only one of jestream or stan versions in the global configuration, and fail during reconcile if a user tries to install an eventbus with no versions provided in the config.
For simplicity, I chose making both mandatory for now but we can change this as well @whynowy
There was a problem hiding this comment.
Sorry for the late response.
I think it's okay to enforce eventbus configuration in the configmap for now, since in our installation files both of them are there. We can always revisit this if anything needs to be changed.
|
|
||
| func ValidateConfig(config *GlobalConfig) error { | ||
| if len(config.supportedJetStreamVersions()) == 0 { | ||
| return fmt.Errorf("no jetstream versions were provided in the controller config") |
There was a problem hiding this comment.
Sorry for the late response.
I think it's okay to enforce eventbus configuration in the configmap for now, since in our installation files both of them are there. We can always revisit this if anything needs to be changed.
* fix: make webhook url optional Signed-off-by: Daniel Soifer <[email protected]> Signed-off-by: Daniel Soifer <[email protected]>
* fix: make webhook url optional Signed-off-by: Daniel Soifer <[email protected]>
No description provided.