From 8e2f1807ac4764f2e0ea3ed36e98e04d6cf09e51 Mon Sep 17 00:00:00 2001 From: Sebastian Spaink Date: Wed, 15 Jul 2026 11:16:03 -0500 Subject: [PATCH] config: validate configuration with Rego and warn on unknown options (#8891) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of #2745 Like most of his ideas, @anderseknert's suggestion to use Rego to replace the `validateAndInjectDefaults` functions throughout the codebase is another winner. This PR starts the migration by replacing the top-level `validateAndInjectDefaults` in `v1/config/config.go` with an embedded policy, `validate.rego`. The policy injects the top-level defaults (`default_decision`, `default_authorization_decision`, `labels`) and reports unrecognized configuration options, so a typo such as `decision_log` instead of `decision_logs` is logged as a warning at startup rather than silently ignored. It's evaluated in `ParseConfig` using the low-level `ast`/`topdown` packages rather than the top-level `rego` package. This keeps `config` off the heavy `rego → bundle → …` dependency web (which would otherwise create import cycles as more packages' tests reach `config`), and we don't need any of the `rego` package's conveniences here — it's one module compiled once and a single query. The Rego unit tests run in CI via `build/run-rego-tests.sh` (and locally with `make rego-test`). This sets the foundation for the other plugin `validateAndInjectDefaults` functions to migrate to Rego as well; where the logic isn't too complicated it should be a fairly easy replacement. At the moment all known keys live in `validate.rego` under `_specs` to support the "warn on unrecognized options" check, but the plugin-specific entries can move closer to each plugin as it migrates. It would also be nice for `_specs` to be auto-generated somehow in the future. Supporting extension of config validation with custom policies is something I'd like to follow up with, so keeping #2745 open for now. I also think these policies could be reusable with [java-opa-sdk](https://github.com/open-policy-agent/java-opa-sdk) 👀 Signed-off-by: Sebastian Spaink --- .github/workflows/pull-request.yaml | 3 + .regal/config.yaml | 1 + Makefile | 4 + build/run-rego-tests.sh | 30 +++++ docs/docs/configuration.md | 24 ++++ v1/config/config.go | 166 +++++++++++++++++------- v1/config/validate.go | 146 +++++++++++++++++++++ v1/config/validate.rego | 165 ++++++++++++++++++++++++ v1/config/validate_test.go | 169 +++++++++++++++++++++++++ v1/config/validate_test.rego | 102 +++++++++++++++ v1/plugins/discovery/discovery.go | 6 + v1/plugins/discovery/discovery_test.go | 44 +++++++ v1/runtime/runtime.go | 5 + v1/util/test/populate.go | 8 ++ 14 files changed, 827 insertions(+), 46 deletions(-) create mode 100755 build/run-rego-tests.sh create mode 100644 v1/config/validate.go create mode 100644 v1/config/validate.rego create mode 100644 v1/config/validate_test.go create mode 100644 v1/config/validate_test.rego diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index 6dc18ff655..a289f53d5b 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -521,6 +521,9 @@ jobs: - name: Test policies run: opa test --schema build/policy/schema --bundle build/policy + - name: Test embedded Rego policies + run: ./build/run-rego-tests.sh + - name: Run file policy checks on changed files run: | if [ -n "${{ github.event.merge_group.base_sha }}" ]; then diff --git a/.regal/config.yaml b/.regal/config.yaml index 45da0a4084..ab76d4581d 100644 --- a/.regal/config.yaml +++ b/.regal/config.yaml @@ -13,6 +13,7 @@ rules: files: - "docs" - "internal/wasm/sdk/examples/*" + - "v1/config/*.rego" style: line-length: ignore: diff --git a/Makefile b/Makefile index ad11cddb20..807516366e 100644 --- a/Makefile +++ b/Makefile @@ -138,6 +138,10 @@ go-test: generate go-test-short: generate $(GO) test $(GO_TAGS) -short ./... +.PHONY: rego-test +rego-test: go-build + OPA=$(CURDIR)/$(BIN) ./build/run-rego-tests.sh + .PHONY: race-detector race-detector: generate CGO_ENABLED=1 GOFLAGS="$(GOFLAGS)" go test $(GO_TAGS),slow -race -vet=off ./... diff --git a/build/run-rego-tests.sh b/build/run-rego-tests.sh new file mode 100755 index 0000000000..334af17a8c --- /dev/null +++ b/build/run-rego-tests.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# Runs `opa test` over every directory that contains *_test.rego files, so Rego +# unit tests for embedded/internal policies are exercised in CI. build/policy is +# excluded because it is tested separately with its JSON schemas; test fixtures +# are excluded because they are not meant to be run as tests. +set -euo pipefail + +OPA="${OPA:-opa}" + +dirs=$(find . -name '*_test.rego' \ + -not -path './build/policy/*' \ + -not -path '*/testdata/*' \ + -not -path '*/testfiles/*' \ + -not -path '*/node_modules/*' \ + -not -path './.git/*' \ + -not -path './.claude/*' \ + -exec dirname {} \; | sort -u) + +if [ -z "$dirs" ]; then + echo "No Rego test directories found." + exit 0 +fi + +status=0 +for d in $dirs; do + echo "==> ${OPA} test ${d}" + "$OPA" test "$d" || status=1 +done + +exit "$status" diff --git a/docs/docs/configuration.md b/docs/docs/configuration.md index 2f579f14c7..9a1c0c8411 100644 --- a/docs/docs/configuration.md +++ b/docs/docs/configuration.md @@ -1039,6 +1039,30 @@ The `server` configuration sets: | `plugins` | `object` | No (default: `{}`) | Location for custom plugin configuration. | | `nd_builtin_cache` | `boolean` | No (default: `false`) | Enable the non-deterministic builtins caching system during policy evaluation, and include the contents of the cache in decision logs. Note that decision logs that are larger than `upload_size_limit_bytes` will drop the `nd_builtin_cache` key from the log entry before uploading. | +## Configuration Validation + +When OPA starts, it validates its configuration using a built-in Rego policy. +Two things come out of this: + +- **Defaults** (such as `default_decision` and `labels`) are injected. +- **Unrecognized options** are reported. If the configuration contains a key OPA + doesn't recognize — usually a typo, or a CLI flag mistaken for a config option — + OPA logs a warning at startup rather than silently ignoring it: + + ```bash + # note the typo: "decision_log" instead of "decision_logs" + $ opa run --server --set decision_log.console=true + {"level":"warning","msg":"unknown configuration option \"decision_log\" encountered", ...} + ``` + + Warnings are non-fatal. Sections that accept arbitrary keys (for example + `labels`, `plugins`, and service `credentials`) are not checked, so extending + them never produces false positives. + +Configuration loaded later via [discovery](#discovery) is validated the same way: +defaults are injected and warnings are logged when the discovered configuration is +applied. + ## Using Environment Variables in Configuration > Only supported with the OPA runtime (`opa run`). diff --git a/v1/config/config.go b/v1/config/config.go index 5d40898bee..b42f7dd12e 100644 --- a/v1/config/config.go +++ b/v1/config/config.go @@ -6,6 +6,8 @@ package config import ( + "bytes" + "context" "encoding/json" "errors" "fmt" @@ -13,13 +15,12 @@ import ( "os" "path/filepath" "reflect" - "sort" + "slices" "strings" "github.com/open-policy-agent/opa/internal/ref" "github.com/open-policy-agent/opa/v1/ast" "github.com/open-policy-agent/opa/v1/util" - "github.com/open-policy-agent/opa/v1/version" ) // ServerConfig represents the different server configuration options. @@ -102,19 +103,110 @@ type Config struct { Server *ServerConfig `json:"server,omitempty"` Storage *StorageConfig `json:"storage,omitempty"` Extra map[string]json.RawMessage `json:"-"` + + // Warnings holds non-fatal messages from config validation (e.g. + // unrecognized options), for callers to surface to the user. + Warnings []string `json:"-"` } // ParseConfig returns a valid Config object with defaults injected. The id // and version parameters will be set in the labels map. +// +// The raw configuration is run through the embedded validation policy (see +// validate.rego): defaults are injected, fatal errors are returned, and warnings +// are attached to the returned Config. func ParseConfig(raw []byte, id string) (*Config, error) { + var rawConfig any + if err := util.Unmarshal(raw, &rawConfig); err != nil { + return nil, err + } + // An absent (or null) configuration is treated as an empty object. + if rawConfig == nil { + rawConfig = map[string]any{} + } + + processed, warnings, err := evaluateConfigPolicy(context.TODO(), rawConfig, id) + if err != nil { + return nil, err + } + + // Build from the original bytes so unchanged sections keep their exact + // encoding, then overlay only the top-level keys the policy actually changed + // (core defaults, or defaults injected by registered plugin policies). + result, err := unmarshalConfig(raw) + if err != nil { + return nil, err + } + if err := result.overlayChanged(raw, processed); err != nil { + return nil, err + } + result.Warnings = warnings + + // Residual checks the policy can't express: decision paths must parse. + if _, err := ref.ParseDataPath(*result.DefaultDecision); err != nil { + return nil, err + } + if _, err := ref.ParseDataPath(*result.DefaultAuthorizationDecision); err != nil { + return nil, err + } + + return result, nil +} + +// overlayChanged applies the policy output onto the Config, but only for the +// top-level keys whose value the policy changed (or added) relative to the +// original config. Unchanged keys are left as parsed from the original bytes so +// their json.RawMessage encoding is preserved exactly. +func (c *Config) overlayChanged(raw []byte, processed map[string]any) error { + var orig map[string]json.RawMessage + if len(raw) > 0 { + if err := util.Unmarshal(raw, &orig); err != nil { + return err + } + } + + knownFields := knownConfigFields(reflect.ValueOf(c).Elem()) + for key, pv := range processed { + if origBytes, ok := orig[key]; ok && jsonEqual(origBytes, pv) { + continue // unchanged + } + chunk, err := json.Marshal(pv) + if err != nil { + return err + } + if field, found := knownFields[key]; found { + field.Set(reflect.Zero(field.Type())) // replace, don't merge + if err := util.Unmarshal(chunk, field.Addr().Interface()); err != nil { + return err + } + } else { + if c.Extra == nil { + c.Extra = map[string]json.RawMessage{} + } + c.Extra[key] = chunk + } + } + return nil +} + +// jsonEqual reports whether raw JSON bytes and a decoded value are semantically +// equal, comparing their canonical encodings. +func jsonEqual(rawValue json.RawMessage, value any) bool { + var decoded any + if err := util.Unmarshal(rawValue, &decoded); err != nil { + return false + } + a, err1 := json.Marshal(decoded) + b, err2 := json.Marshal(value) + return err1 == nil && err2 == nil && bytes.Equal(a, b) +} + +// unmarshalConfig decodes JSON config bytes into a Config, routing known +// top-level keys into struct fields and keeping the rest in Extra. +func unmarshalConfig(raw []byte) (*Config, error) { // NOTE(sr): based on https://stackoverflow.com/a/33499066/993018 var result Config - objValue := reflect.ValueOf(&result).Elem() - knownFields := map[string]reflect.Value{} - for i := 0; i != objValue.NumField(); i++ { - jsonName := strings.Split(objValue.Type().Field(i).Tag.Get("json"), ",")[0] - knownFields[jsonName] = objValue.Field(i) - } + knownFields := knownConfigFields(reflect.ValueOf(&result).Elem()) if err := util.Unmarshal(raw, &result.Extra); err != nil { return nil, err @@ -131,7 +223,20 @@ func ParseConfig(raw []byte, id string) (*Config, error) { if len(result.Extra) == 0 { result.Extra = nil } - return &result, result.validateAndInjectDefaults(id) + return &result, nil +} + +// knownConfigFields maps each JSON key to its struct field, skipping json:"-". +func knownConfigFields(objValue reflect.Value) map[string]reflect.Value { + knownFields := map[string]reflect.Value{} + for i := 0; i != objValue.NumField(); i++ { + jsonName := strings.Split(objValue.Type().Field(i).Tag.Get("json"), ",")[0] + if jsonName == "" || jsonName == "-" { + continue + } + knownFields[jsonName] = objValue.Field(i) + } + return knownFields } // PluginNames returns a sorted list of names of enabled plugins. @@ -148,7 +253,7 @@ func (c Config) PluginNames() (result []string) { for name := range c.Plugins { result = append(result, name) } - sort.Strings(result) + slices.Sort(result) return result } @@ -307,40 +412,14 @@ func (c *Config) Clone() *Config { } } + if c.Warnings != nil { + clone.Warnings = make([]string, len(c.Warnings)) + copy(clone.Warnings, c.Warnings) + } + return clone } -func (c *Config) validateAndInjectDefaults(id string) error { - if c.DefaultDecision == nil { - s := defaultDecisionPath - c.DefaultDecision = &s - } - - _, err := ref.ParseDataPath(*c.DefaultDecision) - if err != nil { - return err - } - - if c.DefaultAuthorizationDecision == nil { - s := defaultAuthorizationDecisionPath - c.DefaultAuthorizationDecision = &s - } - - _, err = ref.ParseDataPath(*c.DefaultAuthorizationDecision) - if err != nil { - return err - } - - if c.Labels == nil { - c.Labels = map[string]string{} - } - - c.Labels["id"] = id - c.Labels["version"] = version.Version - - return nil -} - func removeServiceCredentials(x any) error { switch x := x.(type) { case nil: @@ -397,8 +476,3 @@ func removeKey(x any, keys ...string) error { return nil } - -const ( - defaultDecisionPath = "/system/main" - defaultAuthorizationDecisionPath = "/system/authz/allow" -) diff --git a/v1/config/validate.go b/v1/config/validate.go new file mode 100644 index 0000000000..3374446ee9 --- /dev/null +++ b/v1/config/validate.go @@ -0,0 +1,146 @@ +// Copyright 2025 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package config + +import ( + "context" + _ "embed" + "errors" + "fmt" + "slices" + "strings" + "sync" + + "github.com/open-policy-agent/opa/v1/ast" + "github.com/open-policy-agent/opa/v1/storage/inmem" + "github.com/open-policy-agent/opa/v1/topdown" + "github.com/open-policy-agent/opa/v1/version" +) + +// coreValidationModule is the embedded policy that injects OPA's config +// defaults and reports unrecognized options. +// +//go:embed validate.rego +var coreValidationModule string + +const coreValidationPolicyName = "opa/config/validate.rego" + +// validationQuery yields a document with `processed`, `warnings` and `errors`. +// The result is bound to x. (The topdown package is used directly rather than +// the rego package to avoid a config -> rego -> bundle import cycle.) +const validationQuery = "data.opa.config = x" + +// validationQueryBody is parsed once; the query never changes. +var validationQueryBody = ast.MustParseBody(validationQuery) + +// The policy is static, so it is compiled once and reused across all calls. +var ( + compileOnce sync.Once + compiledPolicy *ast.Compiler + compileErr error +) + +// evaluateConfigPolicy runs the validation policy against raw. It returns the +// config with defaults injected and any warnings; a non-empty set of policy +// `errors` is returned as a single error. +func evaluateConfigPolicy(ctx context.Context, raw any, id string) (map[string]any, []string, error) { + compiler, err := compileValidationPolicy() + if err != nil { + return nil, nil, err + } + + inputValue, err := ast.InterfaceToValue(map[string]any{ + "config": raw, + "runtime": map[string]any{ + "id": id, + "version": version.Version, + }, + }) + if err != nil { + return nil, nil, fmt.Errorf("config validation: %w", err) + } + + store := inmem.New() + txn, err := store.NewTransaction(ctx) + if err != nil { + return nil, nil, fmt.Errorf("config validation: %w", err) + } + defer store.Abort(ctx, txn) + + qrs, err := topdown.NewQuery(validationQueryBody). + WithCompiler(compiler). + WithStore(store). + WithTransaction(txn). + WithInput(ast.NewTerm(inputValue)). + Run(ctx) + if err != nil { + return nil, nil, fmt.Errorf("config validation: %w", err) + } + if len(qrs) != 1 { + return nil, nil, errors.New("config validation: policy produced no result") + } + + result, err := ast.JSON(qrs[0][ast.Var("x")].Value) + if err != nil { + return nil, nil, fmt.Errorf("config validation: %w", err) + } + doc, ok := result.(map[string]any) + if !ok { + return nil, nil, fmt.Errorf("config validation: unexpected result type %T", result) + } + + if errs := stringSet(doc["errors"]); len(errs) > 0 { + slices.Sort(errs) + return nil, nil, errors.New(strings.Join(errs, "; ")) + } + + processed, ok := doc["processed"].(map[string]any) + if !ok { + return nil, nil, errors.New("config validation: policy did not produce a processed configuration") + } + + warnings := stringSet(doc["warnings"]) + slices.Sort(warnings) + + return processed, warnings, nil +} + +// compileValidationPolicy parses and compiles the embedded core policy once. +func compileValidationPolicy() (*ast.Compiler, error) { + compileOnce.Do(func() { + popts := ast.ParserOptions{RegoVersion: ast.RegoV1} + core, err := ast.ParseModuleWithOpts(coreValidationPolicyName, coreValidationModule, popts) + if err != nil { + compileErr = fmt.Errorf("config validation: %w", err) + return + } + compiler := ast.NewCompiler() + if compiler.Compile(map[string]*ast.Module{coreValidationPolicyName: core}); compiler.Failed() { + compileErr = fmt.Errorf("config validation: %w", compiler.Errors) + return + } + compiledPolicy = compiler + }) + return compiledPolicy, compileErr +} + +// stringSet turns a Rego set/array result into a []string, ignoring non-string +// members and returning nil when empty. +func stringSet(v any) []string { + items, ok := v.([]any) + if !ok || len(items) == 0 { + return nil + } + out := make([]string, 0, len(items)) + for _, item := range items { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + if len(out) == 0 { + return nil + } + return out +} diff --git a/v1/config/validate.rego b/v1/config/validate.rego new file mode 100644 index 0000000000..e0c7686510 --- /dev/null +++ b/v1/config/validate.rego @@ -0,0 +1,165 @@ +# METADATA +# description: | +# Structural validation of OPA's own configuration, evaluated by +# config.ParseConfig. It injects the top-level defaults (default_decision, +# default_authorization_decision, labels) and warns on unrecognized option keys +# (checked against a schema of known keys, both top-level and within known +# sections). It does not validate option values or semantics, and does not +# replace each plugin's own config validation. +# +# Input: {"config": , "runtime": {"id", "version"}} +# Entrypoints: processed (config + defaults), errors (fatal), warnings. +package opa.config + +_default_decision := "/system/main" + +_default_authorization_decision := "/system/authz/allow" + +# METADATA +# entrypoint: true +# description: | +# The user config with every _patches fragment merged over it. A fragment +# wins where it overlaps, so enforced values (labels id/version) always apply, +# while defaults are contributed only when the option is absent. +processed := object.union_n(array.concat([input.config], [patch | some patch in _patches])) + +_patches contains {"default_decision": _default_decision} if _absent_or_null("default_decision") + +_patches contains {"default_authorization_decision": _default_authorization_decision} if { + _absent_or_null("default_authorization_decision") +} + +_patches contains {"labels": {"id": input.runtime.id, "version": input.runtime.version}} + +# _absent_or_null is true when a config field is missing or explicitly null. In +# both cases the default is injected, matching the pre-Rego behavior where a nil +# pointer was treated as unset. +_absent_or_null(field) if not input.config[field] + +_absent_or_null(field) if input.config[field] == null + +errors contains msg if { + some field in {"default_decision", "default_authorization_decision"} + value := input.config[field] + value != null + not is_string(value) + msg := sprintf("%s must be a string", [field]) +} + +# warnings reports unrecognized options at any depth. _specs enumerates the known +# keys of each "closed" object; objects without a spec are "open" (any key +# allowed), avoiding false positives for user-extensible sections. +warnings contains msg if { + walk(input.config, [path, _]) + count(path) > 0 + + key := path[count(path) - 1] + is_string(key) # only object keys are validated, not array indices + + parent := array.slice(path, 0, count(path) - 1) + + some spec in _specs + _matches(parent, spec.pattern) + not key in spec.keys + + msg := sprintf("unknown configuration option %q encountered", [_dotted(path)]) +} + +# _matches tests a config path against a spec pattern; "*" matches any segment. +_matches(path, pattern) if { + count(path) == count(pattern) + not _mismatch(path, pattern) +} + +_mismatch(path, pattern) if { + some i, segment in pattern + segment != "*" + segment != path[i] +} + +_dotted(path) := concat(".", [sprintf("%v", [segment]) | some segment in path]) + +# _specs enumerates the known keys of each "closed" object, derived from the +# authoritative Go structs. +_specs := [ + {"pattern": [], "keys": { + "services", "labels", "discovery", "bundle", "bundles", + "decision_logs", "status", "plugins", "keys", "default_decision", + "default_authorization_decision", "caching", "nd_builtin_cache", + "persistence_directory", "distributed_tracing", "metrics_export", + "server", "storage", + }}, + {"pattern": ["decision_logs"], "keys": { + "plugin", "service", "partition_name", "reporting", "request_context", + "mask_decision", "drop_decision", "console", "resource", "nd_builtin_cache", + }}, + {"pattern": ["decision_logs", "reporting"], "keys": { + "buffer_type", "buffer_size_limit_bytes", "buffer_size_limit_events", + "upload_size_limit_bytes", "min_delay_seconds", "max_delay_seconds", + "max_decisions_per_second", "trigger", + }}, + {"pattern": ["decision_logs", "request_context"], "keys": {"http"}}, + {"pattern": ["decision_logs", "request_context", "http"], "keys": {"headers"}}, + {"pattern": ["status"], "keys": { + "plugin", "service", "partition_name", "console", "prometheus", + "prometheus_config", "trigger", + }}, + {"pattern": ["status", "prometheus_config"], "keys": {"collectors"}}, + {"pattern": ["status", "prometheus_config", "collectors"], "keys": {"bundle_loading_duration_ns"}}, + { + "pattern": ["status", "prometheus_config", "collectors", "bundle_loading_duration_ns"], + "keys": {"buckets"}, + }, + {"pattern": ["discovery"], "keys": { + "name", "prefix", "decision", "service", "resource", "signing", + "persist", "trigger", "polling", + }}, + {"pattern": ["discovery", "polling"], "keys": _polling_keys}, + {"pattern": ["bundle"], "keys": { + "name", "prefix", "service", "resource", "signing", "persist", + "size_limit_bytes", "trigger", "polling", + }}, + {"pattern": ["bundle", "polling"], "keys": _polling_keys}, + {"pattern": ["bundles", "*"], "keys": { + "service", "resource", "signing", "persist", "size_limit_bytes", + "trigger", "polling", + }}, + {"pattern": ["bundles", "*", "polling"], "keys": _polling_keys}, + {"pattern": ["server"], "keys": {"metrics", "encoding", "decoding", "logger_plugin"}}, + {"pattern": ["storage"], "keys": {"disk"}}, + {"pattern": ["storage", "disk"], "keys": {"directory", "auto_create", "partitions", "badger"}}, + {"pattern": ["caching"], "keys": {"inter_query_builtin_cache", "inter_query_builtin_value_cache"}}, + {"pattern": ["caching", "inter_query_builtin_cache"], "keys": { + "max_size_bytes", "forced_eviction_threshold_percentage", + "stale_entry_eviction_period_seconds", + }}, + {"pattern": ["caching", "inter_query_builtin_value_cache"], "keys": {"max_num_entries", "named"}}, + { + "pattern": ["caching", "inter_query_builtin_value_cache", "named", "*"], + "keys": {"max_num_entries", "disabled"}, + }, + {"pattern": ["distributed_tracing"], "keys": { + "type", "address", "service_name", "sample_percentage", "encryption", + "allow_insecure_tls", "tls_cert_file", "tls_private_key_file", + "tls_ca_cert_file", "resource", "batch_span_processor_options", + }}, + {"pattern": ["distributed_tracing", "resource"], "keys": { + "service_version", "service_instance_id", "service_namespace", + "deployment_environment", + }}, + {"pattern": ["distributed_tracing", "batch_span_processor_options"], "keys": { + "blocking", "batch_timeout_ms", "export_timeout_ms", + "max_export_batch_size", "max_queue_size", + }}, + {"pattern": ["metrics_export"], "keys": { + "type", "address", "export_interval_ms", "service_name", "encryption", + "allow_insecure_tls", "tls_cert_file", "tls_private_key_file", "tls_ca_cert_file", + }}, + {"pattern": ["services", "*"], "keys": { + "name", "url", "headers", "allow_insecure_tls", + "response_header_timeout_seconds", "tls", "credentials", "type", + }}, + {"pattern": ["keys", "*"], "keys": {"key", "private_key", "algorithm", "scope"}}, +] + +_polling_keys := {"min_delay_seconds", "max_delay_seconds", "long_polling_timeout_seconds"} diff --git a/v1/config/validate_test.go b/v1/config/validate_test.go new file mode 100644 index 0000000000..d980afb240 --- /dev/null +++ b/v1/config/validate_test.go @@ -0,0 +1,169 @@ +// Copyright 2025 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package config + +import ( + "context" + "reflect" + "slices" + "strings" + "testing" + + "github.com/open-policy-agent/opa/v1/ast" + "github.com/open-policy-agent/opa/v1/storage/inmem" + "github.com/open-policy-agent/opa/v1/topdown" +) + +func TestParseConfigWarnsOnUnknownOption(t *testing.T) { + // The motivating example from issue #2745. + conf, err := ParseConfig([]byte(`{"decision_log": {"console": true}}`), "id") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + want := `unknown configuration option "decision_log" encountered` + if !slices.Contains(conf.Warnings, want) { + t.Fatalf("expected warning %q, got %v", want, conf.Warnings) + } +} + +func TestParseConfigNoWarningsForValidConfig(t *testing.T) { + conf, err := ParseConfig([]byte(`{"decision_logs": {"console": true}}`), "id") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(conf.Warnings) != 0 { + t.Fatalf("expected no warnings, got %v", conf.Warnings) + } +} + +func TestParseConfigEmptyInjectsDefaults(t *testing.T) { + // The SDK and other callers parse an absent configuration (nil/empty bytes); + // defaults must still be injected and no error returned. + for name, raw := range map[string][]byte{ + "nil": nil, + "empty": []byte(``), + "empty-obj": []byte(`{}`), + "null": []byte(`null`), + "whitespace": []byte(` `), + } { + t.Run(name, func(t *testing.T) { + conf, err := ParseConfig(raw, "id") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.DefaultDecision == nil || *conf.DefaultDecision != "/system/main" { + t.Fatalf("expected default decision to be injected, got %v", conf.DefaultDecision) + } + if conf.Labels["id"] != "id" { + t.Fatalf("expected id label to be injected, got %v", conf.Labels) + } + }) + } +} + +func TestParseConfigNullDecisionDefaults(t *testing.T) { + // A field explicitly set to null must fall back to the default (not error), + // matching the pre-Rego behavior where a nil pointer was treated as unset. + conf, err := ParseConfig([]byte(`{"default_decision": null}`), "id") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.DefaultDecision == nil || *conf.DefaultDecision != "/system/main" { + t.Fatalf("expected default decision to be injected, got %v", conf.DefaultDecision) + } +} + +func TestParseConfigNonStringDecisionErrors(t *testing.T) { + // A present, non-null, non-string value is still a fatal error. + if _, err := ParseConfig([]byte(`{"default_decision": 42}`), "id"); err == nil { + t.Fatal("expected error for non-string default_decision, got nil") + } +} + +// TestCoreValidationRootSpecMatchesConfigStruct is a drift guard: the set of +// top-level keys known to the core validation policy must exactly match the +// JSON-tagged fields of the Config struct. If a field is added to Config without +// updating validate.rego (or vice versa), this test fails. +func TestCoreValidationRootSpecMatchesConfigStruct(t *testing.T) { + structKeys := map[string]struct{}{} + objType := reflect.TypeOf(Config{}) + for i := range objType.NumField() { + name := strings.Split(objType.Field(i).Tag.Get("json"), ",")[0] + if name == "" || name == "-" { + continue + } + structKeys[name] = struct{}{} + } + + policyKeys := rootSpecKeys(t) + + for k := range structKeys { + if _, ok := policyKeys[k]; !ok { + t.Errorf("config key %q is present in Config struct but missing from validate.rego root spec", k) + } + } + for k := range policyKeys { + if _, ok := structKeys[k]; !ok { + t.Errorf("config key %q is present in validate.rego root spec but not in Config struct", k) + } + } +} + +// rootSpecKeys evaluates the core policy and returns the key set of the spec +// whose pattern is empty (i.e. the top-level configuration object). +func rootSpecKeys(t *testing.T) map[string]struct{} { + t.Helper() + + ctx := context.Background() + compiler, err := compileValidationPolicy() + if err != nil { + t.Fatalf("compile: %v", err) + } + + store := inmem.New() + txn, err := store.NewTransaction(ctx) + if err != nil { + t.Fatalf("txn: %v", err) + } + defer store.Abort(ctx, txn) + + qrs, err := topdown.NewQuery(ast.MustParseBody("data.opa.config._specs = x")). + WithCompiler(compiler). + WithStore(store). + WithTransaction(txn). + Run(ctx) + if err != nil { + t.Fatalf("eval _specs: %v", err) + } + if len(qrs) != 1 { + t.Fatalf("unexpected result set: %v", qrs) + } + + value, err := ast.JSON(qrs[0][ast.Var("x")].Value) + if err != nil { + t.Fatalf("convert _specs: %v", err) + } + specs, ok := value.([]any) + if !ok { + t.Fatalf("unexpected _specs type %T", value) + } + + for _, s := range specs { + spec := s.(map[string]any) + pattern := spec["pattern"].([]any) + if len(pattern) != 0 { + continue + } + keys := map[string]struct{}{} + for _, k := range spec["keys"].([]any) { + keys[k.(string)] = struct{}{} + } + return keys + } + + t.Fatal("no root spec (empty pattern) found in validate.rego") + return nil +} diff --git a/v1/config/validate_test.rego b/v1/config/validate_test.rego new file mode 100644 index 0000000000..594eafdacf --- /dev/null +++ b/v1/config/validate_test.rego @@ -0,0 +1,102 @@ +package opa.config_test + +import data.opa.config + +# _runtime is the runtime document supplied by the Go layer. +_runtime := {"id": "test-id", "version": "test-version"} + +# _input builds a policy input document from a raw configuration object. +_input(raw) := {"config": raw, "runtime": _runtime} + +test_processed_injects_default_decisions if { + result := config.processed with input as _input({}) + result.default_decision == "/system/main" + result.default_authorization_decision == "/system/authz/allow" +} + +test_processed_preserves_configured_decisions if { + raw := {"default_decision": "/foo/bar", "default_authorization_decision": "/baz/qux"} + result := config.processed with input as _input(raw) + result.default_decision == "/foo/bar" + result.default_authorization_decision == "/baz/qux" +} + +test_processed_defaults_when_decision_is_null if { + result := config.processed with input as _input({"default_decision": null}) + result.default_decision == "/system/main" +} + +test_no_error_when_decision_is_null if { + config.errors == set() with input as _input({"default_decision": null}) +} + +test_processed_injects_labels if { + result := config.processed with input as _input({"labels": {"region": "eu"}}) + result.labels == {"region": "eu", "id": "test-id", "version": "test-version"} +} + +# Each case is a configuration whose typo'd option should be reported by exactly +# one warning naming its dotted path. +test_warns_on_unknown_option[tc.note] if { + some tc in [ + { + # The motivating example from issue #2745: "decision_log" vs "decision_logs". + "note": "top-level typo", + "config": {"decision_log": {"console": true}}, + "want": "decision_log", + }, + { + "note": "nested option typo", + "config": {"decision_logs": {"consoel": true}}, + "want": "decision_logs.consoel", + }, + { + "note": "typo in a named map entry", + "config": {"bundles": {"authz": {"servcie": "s1"}}}, + "want": "bundles.authz.servcie", + }, + { + "note": "typo in a service (array) entry", + "config": {"services": [{"name": "s1", "urll": "https://example.com"}]}, + "want": "services.0.urll", + }, + ] + + msgs := config.warnings with input as _input(tc.config) + msgs == {sprintf("unknown configuration option %q encountered", [tc.want])} +} + +# Each case is a valid configuration that must produce no warnings. +test_no_warnings[tc.note] if { + some tc in [ + { + "note": "valid config across sections", + "config": { + "decision_logs": {"console": true}, + "bundles": {"authz": {"service": "s1", "resource": "bundle.tar.gz"}}, + "services": [{"name": "s1", "url": "https://example.com"}], + "labels": {"anything": "goes"}, + "plugins": {"custom_plugin": {"whatever": true}}, + }, + }, + { + "note": "open sections with arbitrary keys", + "config": { + "labels": {"team": "x", "custom": "y"}, + "plugins": {"my_plugin": {"arbitrary": {"nested": true}}}, + "keys": {"my_key": {"key": "abc", "algorithm": "HS256"}}, + }, + }, + ] + + config.warnings == set() with input as _input(tc.config) +} + +test_errors_on_non_string_decision if { + msgs := config.errors with input as _input({"default_decision": 42}) + msgs == {"default_decision must be a string"} +} + +test_no_errors_for_valid_config if { + config.errors == set() with input as _input({"decision_logs": {"console": true}}) +} diff --git a/v1/plugins/discovery/discovery.go b/v1/plugins/discovery/discovery.go index e4e76c3101..276aa7780d 100644 --- a/v1/plugins/discovery/discovery.go +++ b/v1/plugins/discovery/discovery.go @@ -482,6 +482,12 @@ func (c *Discovery) processBundle(ctx context.Context, b *bundleApi.Bundle) (*pl return nil, err } + // Surface configuration warnings (e.g. unrecognized options) for the + // discovered configuration, mirroring what the runtime does at boot. + for _, w := range config.Warnings { + c.logger.Warn(w) + } + // Note: We don't currently support changes to the discovery // configuration. These changes are risky because errors would be // unrecoverable (without keeping track of changes and rolling back...) diff --git a/v1/plugins/discovery/discovery_test.go b/v1/plugins/discovery/discovery_test.go index 80e4220344..8adfcf1b65 100644 --- a/v1/plugins/discovery/discovery_test.go +++ b/v1/plugins/discovery/discovery_test.go @@ -248,6 +248,50 @@ func TestEnvVarSubstitution(t *testing.T) { }`, version.Version)) } +func TestProcessBundleLogsConfigWarnings(t *testing.T) { + ctx := t.Context() + + testLogger := test.New() + manager, err := plugins.New([]byte(`{ + "services": {"default": {"url": "http://localhost:8181"}}, + "discovery": {"name": "config"} + }`), "test-id", inmem.New(), plugins.Logger(testLogger)) + if err != nil { + t.Fatal(err) + } + + disco, err := New(manager) + if err != nil { + t.Fatal(err) + } + + // The discovered config carries an unrecognized top-level option. + bundle := makeDataBundle(1, ` + { + "config": { + "bundle": {"name": "test1"}, + "decision_logz": {} + } + } + `) + + if _, err := disco.processBundle(ctx, bundle); err != nil { + t.Fatal(err) + } + + want := `unknown configuration option "decision_logz" encountered` + found := false + for _, e := range testLogger.Entries() { + if e.Message == want { + found = true + break + } + } + if !found { + t.Fatalf("expected warning %q to be logged, got entries: %+v", want, testLogger.Entries()) + } +} + func TestProcessBundleV1Compatible(t *testing.T) { ctx := t.Context() popts := ast.ParserOptions{RegoVersion: ast.RegoV1} diff --git a/v1/runtime/runtime.go b/v1/runtime/runtime.go index a8ae263244..1de5d473f0 100644 --- a/v1/runtime/runtime.go +++ b/v1/runtime/runtime.go @@ -533,6 +533,11 @@ func NewRuntime(ctx context.Context, params Params) (*Runtime, error) { return nil, fmt.Errorf("config error: %w", err) } + // Surface non-fatal config warnings (e.g. unrecognized options). + for _, w := range manager.Config.Warnings { + logger.Warn(w) + } + if err := manager.Init(ctx); err != nil { return nil, fmt.Errorf("initialization error: %w", err) } diff --git a/v1/util/test/populate.go b/v1/util/test/populate.go index 0d7428ccad..011d19a8a2 100644 --- a/v1/util/test/populate.go +++ b/v1/util/test/populate.go @@ -57,6 +57,14 @@ func populateDefaultTypes(t *testing.T, fieldType reflect.Type, fieldValue refle return true } + if fieldType.Elem().Kind() == reflect.String { + fieldValue.Set(reflect.ValueOf([]string{ + fmt.Sprintf("test-%d-a", index), + fmt.Sprintf("test-%d-b", index), + })) + return true + } + case reflect.Pointer: switch fieldType.Elem().Kind() { case reflect.String: