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.

Sign up for GitHub

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

fix: added controller config validation #2103

Merged
merged 2 commits into from
Aug 10, 2022
Merged

fix: added controller config validation #2103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8314d17
fix: make webhook url optional
daniel-codefresh Jul 24, 2022
29346df
fix: added controller config validation
daniel-codefresh Jul 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion controllers/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,16 @@ const (
func Start(namespaced bool, managedNamespace string) {
logger := logging.NewArgoEventsLogger().Named(eventbus.ControllerName)
config, err := controllers.LoadConfig(func(err error) {
logger.Errorf("Failed to reload global configuration file", zap.Error(err))
logger.Errorw("Failed to reload global configuration file", zap.Error(err))
})
if err != nil {
logger.Fatalw("Failed to load global configuration file", zap.Error(err))
}

if err = controllers.ValidateConfig(config); err != nil {
logger.Fatalw("Global configuration file validation failed", zap.Error(err))
}

imageName, defined := os.LookupEnv(imageEnvVar)
if !defined {
logger.Fatalf("required environment variable '%s' not defined", imageEnvVar)
Expand Down
12 changes: 12 additions & 0 deletions controllers/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,15 @@ func LoadConfig(onErrorReloading func(error)) (*GlobalConfig, error) {
})
return r, nil
}

func ValidateConfig(config *GlobalConfig) error {
if len(config.supportedJetStreamVersions()) == 0 {
return fmt.Errorf("no jetstream versions were provided in the controller config")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this error always throw in case u are using just STAN?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whynowy WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if len(config.supportedSTANVersions()) == 0 {
return fmt.Errorf("no stan versions were provided in the controller config")
}

return nil
}