plugins: Fix default trigger mode in the non-discovery path

When individual plugins (except discovery) were configured, the
default trigger mode was set as periodic. So if the plugin specified
a different mode (eg. manual), the configuration check would incorrectly
fail on account of a mode mismatch. This commit fixes the issue by
updating the trigger mode check to not specify a default mode in
scenarios when only plugins (except discovery) are configured.

Fixes: #3797

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

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"
 )

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)
+				}
+			}
+		})
+	}
 }

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

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)
 	}

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
 }

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\")"),
 		},
 	}
 

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

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
 }

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\")"),
 		},
 	}