diff --git a/plugins/bundle/config.go b/plugins/bundle/config.go index 25f26246d6..d5a88fd0f6 100644 --- a/plugins/bundle/config.go +++ b/plugins/bundle/config.go @@ -57,7 +57,8 @@ func ParseConfig(config []byte, services []string) (*Config, error) { // the defined `bundles`. This expects a map of bundle names to resource // configurations. func ParseBundlesConfig(config []byte, services []string) (*Config, error) { - return NewConfigBuilder().WithBytes(config).WithServices(services).WithTriggerMode(nil).Parse() + t := plugins.DefaultTriggerMode + return NewConfigBuilder().WithBytes(config).WithServices(services).WithTriggerMode(&t).Parse() } // NewConfigBuilder returns a new ConfigBuilder to build and parse the bundle config @@ -85,10 +86,6 @@ func (b *ConfigBuilder) WithKeyConfigs(keys map[string]*keys.Config) *ConfigBuil // WithTriggerMode sets the plugin trigger mode func (b *ConfigBuilder) WithTriggerMode(trigger *plugins.TriggerMode) *ConfigBuilder { - if trigger == nil { - t := plugins.DefaultTriggerMode - trigger = &t - } b.trigger = trigger return b } @@ -202,28 +199,11 @@ func (c *Config) validateAndInjectDefaults(services []string, keys map[string]*k return fmt.Errorf("invalid configuration for bundle %q: %s", name, err.Error()) } - if trigger == nil { - t := plugins.DefaultTriggerMode - trigger = &t - } else { - err = validateTriggerMode(*trigger) - if err != nil { - return err - } - } - - if source.Trigger == nil { - source.Trigger = trigger - } else { - err := validateTriggerMode(*source.Trigger) - if err != nil { - return err - } - - if *source.Trigger != *trigger { - return fmt.Errorf("invalid configuration for bundle %q: discovery has trigger mode %s, bundle has %s", name, *trigger, *source.Trigger) - } + t, err := plugins.ValidateAndInjectDefaultsForTriggerMode(trigger, source.Trigger) + if err != nil { + return fmt.Errorf("invalid configuration for bundle %q: %w", name, err) } + source.Trigger = t if err == nil || ignoreServiceConfigErr { err = source.Config.ValidateAndInjectDefaults() @@ -282,15 +262,6 @@ func (c *Config) generateLegacyResourcePath() string { return strings.TrimPrefix(joined, "/") } -func validateTriggerMode(mode plugins.TriggerMode) error { - switch mode { - case plugins.TriggerPeriodic, plugins.TriggerManual: - return nil - default: - return fmt.Errorf("invalid trigger mode %q (want %q or %q)", mode, plugins.TriggerPeriodic, plugins.TriggerManual) - } -} - const ( defaultBundlePathPrefix = "bundles" ) diff --git a/plugins/bundle/config_test.go b/plugins/bundle/config_test.go index d7798ddf3e..161c933d2f 100644 --- a/plugins/bundle/config_test.go +++ b/plugins/bundle/config_test.go @@ -8,6 +8,8 @@ import ( "fmt" "testing" + "github.com/open-policy-agent/opa/plugins" + "github.com/open-policy-agent/opa/keys" "github.com/open-policy-agent/opa/bundle" @@ -403,5 +405,91 @@ func TestConfigIsMultiBundle(t *testing.T) { } }) } - +} + +func TestParseConfigTriggerMode(t *testing.T) { + + tm := plugins.TriggerManual + + tests := []struct { + conf string + services []string + triggerMode *plugins.TriggerMode + expected plugins.TriggerMode + wantError bool + err error + }{ + { + conf: `{"b1":{"service": "s1"}}`, + services: []string{"s1"}, + wantError: false, + triggerMode: nil, + expected: plugins.TriggerPeriodic, + }, + { + conf: `{"b1":{"service": "s1", "trigger": "manual"}}`, + services: []string{"s1"}, + wantError: false, + triggerMode: nil, + expected: plugins.TriggerManual, + }, + { + conf: `{"b1":{"service": "s1"}}`, + services: []string{"s1"}, + wantError: false, + triggerMode: &tm, + expected: plugins.TriggerManual, + }, + { + conf: `{"b1":{"service": "s1", "trigger": "manual"}}`, + services: []string{"s1"}, + wantError: false, + triggerMode: &tm, + expected: plugins.TriggerManual, + }, + { + conf: `{"b1":{"service": "s1", "trigger": "periodic"}}`, + services: []string{"s1"}, + wantError: true, + err: fmt.Errorf("invalid configuration for bundle \"b1\": trigger mode mismatch: manual and periodic (hint: check discovery configuration)"), + triggerMode: &tm, + }, + { + conf: `{"b1":{"service": "s1", "trigger": "foo"}}`, + services: []string{"s1"}, + wantError: true, + err: fmt.Errorf("invalid configuration for bundle \"b1\": invalid trigger mode \"foo\" (want \"periodic\" or \"manual\")"), + triggerMode: nil, + }, + } + + for i := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + config, err := NewConfigBuilder().WithBytes([]byte(tests[i].conf)).WithServices(tests[i].services).WithTriggerMode(tests[i].triggerMode).Parse() + if err != nil && !tests[i].wantError { + t.Fatalf("Unexpected error: %s", err) + } + if err == nil && tests[i].wantError { + t.Fatalf("Expected an error but didn't get one") + } + + if tests[i].wantError { + if err == nil { + t.Fatal("Expected error but got nil") + } + + if tests[i].err != nil && tests[i].err.Error() != err.Error() { + t.Fatalf("Expected error message %v but got %v", tests[i].err.Error(), err.Error()) + } + } else { + if err != nil { + t.Fatalf("Unexpected error %v", err) + } + + if *config.Bundles["b1"].Trigger != tests[i].expected { + t.Fatalf("Expected trigger mode %v but got %v", tests[i].expected, *config.Bundles["b1"].Trigger) + } + } + }) + } } diff --git a/plugins/discovery/discovery.go b/plugins/discovery/discovery.go index d8580c2a43..d9db976d3c 100644 --- a/plugins/discovery/discovery.go +++ b/plugins/discovery/discovery.go @@ -78,8 +78,7 @@ func New(manager *plugins.Manager, opts ...func(*Discovery)) (*Discovery, error) if err != nil { return nil, err } else if config == nil { - t := plugins.DefaultTriggerMode - if _, err := getPluginSet(result.factories, manager, manager.Config, result.metrics, &t); err != nil { + if _, err := getPluginSet(result.factories, manager, manager.Config, result.metrics, nil); err != nil { return nil, err } return result, nil diff --git a/plugins/discovery/discovery_test.go b/plugins/discovery/discovery_test.go index 84ec39bd66..0729b17a59 100644 --- a/plugins/discovery/discovery_test.go +++ b/plugins/discovery/discovery_test.go @@ -1246,7 +1246,7 @@ bundles: confGood, false, nil, }, "trigger_mode_mismatch": { - confBad, true, fmt.Errorf("invalid configuration for bundle \"bundle-new\": discovery has trigger mode manual, bundle has periodic"), + confBad, true, fmt.Errorf("invalid configuration for bundle \"bundle-new\": trigger mode mismatch: manual and periodic (hint: check discovery configuration)"), }, } @@ -1313,7 +1313,7 @@ decision_logs: confGood, false, nil, }, "trigger_mode_mismatch": { - confBad, true, fmt.Errorf("invalid decision_log config, discovery has trigger mode manual, decision_log has periodic"), + confBad, true, fmt.Errorf("invalid decision_log config: trigger mode mismatch: manual and periodic (hint: check discovery configuration)"), }, } @@ -1386,7 +1386,7 @@ status: confGood, false, nil, }, "trigger_mode_mismatch": { - confBad, true, fmt.Errorf("invalid status config, discovery has trigger mode manual, status has periodic"), + confBad, true, fmt.Errorf("invalid status config: trigger mode mismatch: manual and periodic (hint: check discovery configuration)"), }, } @@ -1659,7 +1659,7 @@ func TestPluginManualTriggerLifecycle(t *testing.T) { disco, _ := fixture.server.statusEvent[2].(map[string]interface{}) errMsg := disco["discovery"].(map[string]interface{})["message"] - expErrMsg := "invalid configuration for bundle \"authz\": discovery has trigger mode manual, bundle has periodic" + expErrMsg := "invalid configuration for bundle \"authz\": trigger mode mismatch: manual and periodic (hint: check discovery configuration)" if errMsg != expErrMsg { t.Fatalf("Expected error %v but got %v", expErrMsg, errMsg) } diff --git a/plugins/logs/plugin.go b/plugins/logs/plugin.go index e844ec8ee7..dd82d98b83 100644 --- a/plugins/logs/plugin.go +++ b/plugins/logs/plugin.go @@ -278,28 +278,11 @@ func (c *Config) validateAndInjectDefaults(services []string, pluginsList []stri return fmt.Errorf("invalid decision_log config, must have a `service`, `plugin`, or `console` logging enabled") } - if trigger == nil { - t := plugins.DefaultTriggerMode - trigger = &t - } else { - err := validateTriggerMode(*trigger) - if err != nil { - return err - } - } - - if c.Reporting.Trigger == nil { - c.Reporting.Trigger = trigger - } else { - err := validateTriggerMode(*c.Reporting.Trigger) - if err != nil { - return err - } - - if *c.Reporting.Trigger != *trigger { - return fmt.Errorf("invalid decision_log config, discovery has trigger mode %s, decision_log has %s", *trigger, *c.Reporting.Trigger) - } + t, err := plugins.ValidateAndInjectDefaultsForTriggerMode(trigger, c.Reporting.Trigger) + if err != nil { + return errors.Wrap(err, "invalid decision_log config") } + c.Reporting.Trigger = t min := defaultMinDelaySeconds max := defaultMaxDelaySeconds @@ -349,7 +332,6 @@ func (c *Config) validateAndInjectDefaults(services []string, pluginsList []stri c.MaskDecision = &maskDecision } - var err error c.maskDecisionRef, err = ref.ParseDataPath(*c.MaskDecision) if err != nil { return errors.Wrap(err, "invalid mask_decision in decision_logs") @@ -370,15 +352,6 @@ func (c *Config) validateAndInjectDefaults(services []string, pluginsList []stri return nil } -func validateTriggerMode(mode plugins.TriggerMode) error { - switch mode { - case plugins.TriggerPeriodic, plugins.TriggerManual: - return nil - default: - return fmt.Errorf("invalid trigger mode %q (want %q or %q)", mode, plugins.TriggerPeriodic, plugins.TriggerManual) - } -} - // Plugin implements decision log buffering and uploading. type Plugin struct { manager *plugins.Manager @@ -401,8 +374,9 @@ type reconfigure struct { } // ParseConfig validates the config and injects default values. -func ParseConfig(config []byte, services []string, plugins []string) (*Config, error) { - return NewConfigBuilder().WithBytes(config).WithServices(services).WithPlugins(plugins).WithTriggerMode(nil).Parse() +func ParseConfig(config []byte, services []string, pluginList []string) (*Config, error) { + t := plugins.DefaultTriggerMode + return NewConfigBuilder().WithBytes(config).WithServices(services).WithPlugins(pluginList).WithTriggerMode(&t).Parse() } // ConfigBuilder assists in the construction of the plugin configuration. @@ -438,10 +412,6 @@ func (b *ConfigBuilder) WithPlugins(plugins []string) *ConfigBuilder { // WithTriggerMode sets the plugin trigger mode. func (b *ConfigBuilder) WithTriggerMode(trigger *plugins.TriggerMode) *ConfigBuilder { - if trigger == nil { - t := plugins.DefaultTriggerMode - trigger = &t - } b.trigger = trigger return b } diff --git a/plugins/logs/plugin_test.go b/plugins/logs/plugin_test.go index 57b9c6bd63..3500d396f9 100644 --- a/plugins/logs/plugin_test.go +++ b/plugins/logs/plugin_test.go @@ -1690,14 +1690,14 @@ func TestParseConfigTriggerMode(t *testing.T) { config: []byte(`{"reporting": {"trigger": "manual"}}`), expected: plugins.TriggerPeriodic, wantErr: true, - err: fmt.Errorf("invalid decision_log config, discovery has trigger mode periodic, decision_log has manual"), + err: fmt.Errorf("invalid decision_log config: trigger mode mismatch: periodic and manual (hint: check discovery configuration)"), }, { note: "bad trigger mode", config: []byte(`{"reporting": {"trigger": "foo"}}`), expected: "foo", wantErr: true, - err: fmt.Errorf("invalid trigger mode \"foo\" (want \"periodic\" or \"manual\")"), + err: fmt.Errorf("invalid decision_log config: invalid trigger mode \"foo\" (want \"periodic\" or \"manual\")"), }, } diff --git a/plugins/plugins.go b/plugins/plugins.go index e45f227c1f..5b82322b17 100644 --- a/plugins/plugins.go +++ b/plugins/plugins.go @@ -224,6 +224,46 @@ func getWasmResolversOnContext(context *storage.Context) []*wasm.Resolver { return resolvers } +func validateTriggerMode(mode TriggerMode) error { + switch mode { + case TriggerPeriodic, TriggerManual: + return nil + default: + return fmt.Errorf("invalid trigger mode %q (want %q or %q)", mode, TriggerPeriodic, TriggerManual) + } +} + +// ValidateAndInjectDefaultsForTriggerMode validates the trigger mode and injects default values +func ValidateAndInjectDefaultsForTriggerMode(a, b *TriggerMode) (*TriggerMode, error) { + + if a == nil && b != nil { + err := validateTriggerMode(*b) + if err != nil { + return nil, err + } + return b, nil + } else if a != nil && b == nil { + err := validateTriggerMode(*a) + if err != nil { + return nil, err + } + return a, nil + } else if a != nil && b != nil { + if *a != *b { + return nil, fmt.Errorf("trigger mode mismatch: %s and %s (hint: check discovery configuration)", *a, *b) + } + err := validateTriggerMode(*a) + if err != nil { + return nil, err + } + return a, nil + + } else { + t := DefaultTriggerMode + return &t, nil + } +} + type namedplugin struct { name string plugin Plugin diff --git a/plugins/status/plugin.go b/plugins/status/plugin.go index 54d672a86d..b728433b8d 100644 --- a/plugins/status/plugin.go +++ b/plugins/status/plugin.go @@ -116,44 +116,19 @@ func (c *Config) validateAndInjectDefaults(services []string, pluginsList []stri return fmt.Errorf("invalid status config, must have a `service`, `plugin`, or `console` logging specified") } - if trigger == nil { - t := plugins.DefaultTriggerMode - trigger = &t - } else { - err := validateTriggerMode(*trigger) - if err != nil { - return err - } - } - - if c.Trigger == nil { - c.Trigger = trigger - } else { - err := validateTriggerMode(*c.Trigger) - if err != nil { - return err - } - - if *c.Trigger != *trigger { - return fmt.Errorf("invalid status config, discovery has trigger mode %s, status has %s", *trigger, *c.Trigger) - } + t, err := plugins.ValidateAndInjectDefaultsForTriggerMode(trigger, c.Trigger) + if err != nil { + return errors.Wrap(err, "invalid status config") } + c.Trigger = t return nil } -func validateTriggerMode(mode plugins.TriggerMode) error { - switch mode { - case plugins.TriggerPeriodic, plugins.TriggerManual: - return nil - default: - return fmt.Errorf("invalid trigger mode %q (want %q or %q)", mode, plugins.TriggerPeriodic, plugins.TriggerManual) - } -} - // ParseConfig validates the config and injects default values. -func ParseConfig(config []byte, services []string, plugins []string) (*Config, error) { - return NewConfigBuilder().WithBytes(config).WithServices(services).WithPlugins(plugins).WithTriggerMode(nil).Parse() +func ParseConfig(config []byte, services []string, pluginsList []string) (*Config, error) { + t := plugins.DefaultTriggerMode + return NewConfigBuilder().WithBytes(config).WithServices(services).WithPlugins(pluginsList).WithTriggerMode(&t).Parse() } // ConfigBuilder assists in the construction of the plugin configuration. @@ -189,10 +164,6 @@ func (b *ConfigBuilder) WithPlugins(plugins []string) *ConfigBuilder { // WithTriggerMode sets the plugin trigger mode. func (b *ConfigBuilder) WithTriggerMode(trigger *plugins.TriggerMode) *ConfigBuilder { - if trigger == nil { - t := plugins.DefaultTriggerMode - trigger = &t - } b.trigger = trigger return b } diff --git a/plugins/status/plugin_test.go b/plugins/status/plugin_test.go index 308f83d37e..edc1651901 100644 --- a/plugins/status/plugin_test.go +++ b/plugins/status/plugin_test.go @@ -593,14 +593,14 @@ func TestParseConfigTriggerMode(t *testing.T) { config: []byte(`{"trigger": "manual"}`), expected: plugins.TriggerPeriodic, wantErr: true, - err: fmt.Errorf("invalid status config, discovery has trigger mode periodic, status has manual"), + err: fmt.Errorf("invalid status config: trigger mode mismatch: periodic and manual (hint: check discovery configuration)"), }, { note: "bad trigger mode", config: []byte(`{"trigger": "foo"}`), expected: "foo", wantErr: true, - err: fmt.Errorf("invalid trigger mode \"foo\" (want \"periodic\" or \"manual\")"), + err: fmt.Errorf("invalid status config: invalid trigger mode \"foo\" (want \"periodic\" or \"manual\")"), }, }