Relax configuration check when Discovery is enabled

Previously if Discovery was enabled, other features like bundle downloading and status reporting could not be configured manually.
The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. It's possible that
the system serving the discovered config is unaware of all options locally available in OPA. Hence, we relax the configuration
check when discovery is enabled so that the bootstrap configuration can contain plugin configurations. In case of conflicts,
the bootstrap configuration for plugins wins. These local configuration overrides from the bootstrap configuration are included
in the Status API messages so that management systems can get visibility into the local overrides.

**In general, the bootstrap configuration overrides the discovered configuration.** Previously this was not the case for all
configuration fields. For example, if the discovered configuration changes the `labels` section, only labels that are
additional compared to the bootstrap configuration are used, all other changes are ignored. This implies labels in the
bootstrap configuration override those in the discovered configuration. But for fields such as `default_decision`, `default_authorization_decision`,
`nd_builtin_cache`, the discovered configuration would override the bootstrap configuration. Now the behavior is more consistent
for the entire configuration and helps to avoid accidental configuration errors.

Fixes: #5722

Signed-off-by: Ashutosh Narkar <[email protected]>

Author: ashutosh-narkar

CHANGELOG.md

@@ -9,6 +9,24 @@ The `go` stanza of OPA's `go.mod` had become out of sync: Projects importing OPA
 This was introduced with v0.63.0, but the main branch's `go.mod` still claimed to be a `go 1.20` compatible.
 Now, it states the true state of affairs: OPA, used as Go dependency, requires at least Go 1.21, and thus works with all officially supported Go versions (1.21.x and 1.22.x).
 
+#### Breaking Change
+
+##### Bootstrap configuration overrides Discovered configuration
+
+Previously if Discovery was enabled, other features like bundle downloading and status reporting could not be configured manually.
+The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. It's possible that
+the system serving the discovered config is unaware of all options locally available in OPA. Hence, we relax the configuration
+check when discovery is enabled so that the bootstrap configuration can contain plugin configurations. In case of conflicts,
+the bootstrap configuration for plugins wins. These local configuration overrides from the bootstrap configuration are included
+in the Status API messages so that management systems can get visibility into the local overrides.
+
+**In general, the bootstrap configuration overrides the discovered configuration.** Previously this was not the case for all
+configuration fields. For example, if the discovered configuration changes the `labels` section, only labels that are
+additional compared to the bootstrap configuration are used, all other changes are ignored. This implies labels in the
+bootstrap configuration override those in the discovered configuration. But for fields such as `default_decision`, `default_authorization_decision`,
+`nd_builtin_cache`, the discovered configuration would override the bootstrap configuration. Now the behavior is more consistent
+for the entire configuration and helps to avoid accidental configuration errors.
+
 ## 0.63.0
 
 This release contains a mix of features, performance improvements, and bugfixes.

docs/content/management-discovery.md

@@ -24,9 +24,9 @@ ways to structure the discovery bundle:
 > option.
 
 If discovery is enabled, other features like bundle downloading and status
-reporting **cannot** be configured manually. Similarly, discovered configuration
-cannot override the original discovery settings in the configuration file that
-OPA was booted with.
+reporting **can** be configured manually. In case of conflicts, the bootstrap configuration
+for plugins would override the discovered configuration. **In general, the bootstrap configuration
+overrides the discovered configuration.**
 
 See the [Configuration Reference](../configuration) for configuration details.
 

plugins/discovery/discovery.go

@@ -32,6 +32,7 @@ import (
 	"github.com/open-policy-agent/opa/plugins/status"
 	"github.com/open-policy-agent/opa/rego"
 	"github.com/open-policy-agent/opa/storage/inmem"
+	"github.com/open-policy-agent/opa/util"
 )
 
 const (
@@ -49,19 +50,21 @@ const (
 // started it will periodically download a configuration bundle and try to
 // reconfigure the OPA.
 type Discovery struct {
-	manager           *plugins.Manager
-	config            *Config
-	factories         map[string]plugins.Factory
-	downloader        bundle.Loader                       // discovery bundle downloader
-	status            *bundle.Status                      // discovery status
-	listenersMtx      sync.Mutex                          // lock for listener map
-	listeners         map[interface{}]func(bundle.Status) // listeners for discovery update events
-	etag              string                              // discovery bundle etag for caching purposes
-	metrics           metrics.Metrics
-	readyOnce         sync.Once
-	logger            logging.Logger
-	bundlePersistPath string
-	hooks             hooks.Hooks
+	manager              *plugins.Manager
+	config               *Config
+	factories            map[string]plugins.Factory
+	downloader           bundle.Loader                       // discovery bundle downloader
+	status               *bundle.Status                      // discovery status
+	listenersMtx         sync.Mutex                          // lock for listener map
+	listeners            map[interface{}]func(bundle.Status) // listeners for discovery update events
+	etag                 string                              // discovery bundle etag for caching purposes
+	metrics              metrics.Metrics
+	readyOnce            sync.Once
+	logger               logging.Logger
+	bundlePersistPath    string
+	hooks                hooks.Hooks
+	bootConfig           map[string]interface{}
+	overriddenConfigKeys []string
 }
 
 // Factories provides a set of factory functions to use for
@@ -85,6 +88,12 @@ func Hooks(hs hooks.Hooks) func(*Discovery) {
 	}
 }
 
+func BootConfig(bootConfig map[string]interface{}) func(*Discovery) {
+	return func(d *Discovery) {
+		d.bootConfig = bootConfig
+	}
+}
+
 // New returns a new discovery plugin.
 func New(manager *plugins.Manager, opts ...func(*Discovery)) (*Discovery, error) {
 	result := &Discovery{
@@ -107,10 +116,6 @@ func New(manager *plugins.Manager, opts ...func(*Discovery)) (*Discovery, error)
 		return result, nil
 	}
 
-	if names := manager.Config.PluginNames(); len(names) > 0 {
-		return nil, fmt.Errorf("discovery prohibits manual configuration of %v", strings.Join(names, " and "))
-	}
-
 	result.config = config
 	restClient := manager.Client(config.service)
 	if strings.ToLower(restClient.Config().Type) == "oci" {
@@ -353,6 +358,14 @@ func (c *Discovery) processUpdate(ctx context.Context, u download.Update) {
 		c.status.SetError(nil)
 		c.status.SetActivateSuccess(u.Bundle.Manifest.Revision)
 
+		// include the local overrides in the status update
+		if len(c.overriddenConfigKeys) != 0 {
+			msg := fmt.Sprintf("Keys in the discovered configuration overridden by boot configuration: %v", strings.Join(c.overriddenConfigKeys, ", "))
+			c.logger.Debug(msg)
+			c.status.Message = msg
+		}
+		c.overriddenConfigKeys = nil
+
 		// On the first activation success mark the plugin as being in OK state
 		c.readyOnce.Do(func() {
 			c.manager.UpdatePluginStatus(Name, &plugins.Status{State: plugins.StateOK})
@@ -394,6 +407,33 @@ func (c *Discovery) reconfigure(ctx context.Context, u download.Update) error {
 	return nil
 }
 
+func (c *Discovery) applyLocalPluginConfigOverride(conf *config.Config) (*config.Config, []string, error) {
+	raw, err := json.Marshal(conf)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	var newConfig map[string]interface{}
+	err = util.Unmarshal(raw, &newConfig)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	_, overriddenKeys := mergeValuesAndListOverrides(newConfig, c.bootConfig, "")
+
+	bs, err := json.Marshal(newConfig)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	parsedConf, err := config.ParseConfig(bs, c.manager.ID)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return parsedConf, overriddenKeys, nil
+}
+
 func (c *Discovery) processBundle(ctx context.Context, b *bundleApi.Bundle) (*pluginSet, error) {
 
 	config, err := evaluateBundle(ctx, c.manager.ID, c.manager.Info, b, c.config.query)
@@ -454,11 +494,23 @@ func (c *Discovery) processBundle(ctx context.Context, b *bundleApi.Bundle) (*pl
 		}
 	}
 
-	if err := c.manager.Reconfigure(config); err != nil {
+	overriddenConfig, overriddenKeys, err := c.applyLocalPluginConfigOverride(config)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := c.manager.Reconfigure(overriddenConfig); err != nil {
 		return nil, err
 	}
 
-	return getPluginSet(c.factories, c.manager, config, c.metrics, c.config.Trigger)
+	ps, err := getPluginSet(c.factories, c.manager, overriddenConfig, c.metrics, c.config.Trigger)
+	if err != nil {
+		return nil, err
+	}
+
+	c.overriddenConfigKeys = overriddenKeys
+
+	return ps, nil
 }
 
 // discoveryBundleDirName returns the name of the directory where the discovery bundle will be persisted.
@@ -678,3 +730,45 @@ func registerBundleStatusUpdates(m *plugins.Manager) {
 		bp.RegisterBulkListener(pluginlistener(status.Name), sp.BulkUpdateBundleStatus)
 	}
 }
+
+// mergeValuesAndListOverrides will merge source and destination map, preferring values from the source map.
+// It will also return a list of keys in the destination map which were overridden by those in the source map
+func mergeValuesAndListOverrides(dest map[string]interface{}, src map[string]interface{}, prefix string) (map[string]interface{}, []string) {
+	overriddenKeys := []string{}
+
+	for k, v := range src {
+		// If the key doesn't exist already, then just set the key to that value
+		if _, exists := dest[k]; !exists {
+			dest[k] = v
+			continue
+		}
+
+		fullKey := k
+		if prefix != "" {
+			fullKey = fmt.Sprintf("%v.%v", prefix, k)
+		}
+
+		nextMap, ok := v.(map[string]interface{})
+		// If it isn't another map, overwrite the value
+		if !ok {
+			if dest[k] != v {
+				overriddenKeys = append(overriddenKeys, fullKey)
+			}
+			dest[k] = v
+			continue
+		}
+		// Edge case: If the key exists in the destination, but isn't a map
+		destMap, isMap := dest[k].(map[string]interface{})
+		// If the source map has a map for this key, prefer it
+		if !isMap {
+			dest[k] = v
+			overriddenKeys = append(overriddenKeys, fullKey)
+			continue
+		}
+		// If we got to this point, it is a map in both, so merge them
+		merged, overridden := mergeValuesAndListOverrides(destMap, nextMap, fullKey)
+		dest[k] = merged
+		overriddenKeys = append(overriddenKeys, overridden...)
+	}
+	return dest, overriddenKeys
+}