diff --git a/go.mod b/go.mod index 04a654811..bf213d377 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( github.com/operator-framework/operator-registry v1.61.0 github.com/prometheus/client_golang v1.23.2 github.com/prometheus/common v0.67.2 + github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 github.com/spf13/cobra v1.10.1 github.com/spf13/pflag v1.0.10 github.com/stretchr/testify v1.11.1 @@ -192,7 +193,6 @@ require ( github.com/rivo/uniseg v0.4.7 // indirect github.com/rubenv/sql-migrate v1.8.0 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect - github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect github.com/secure-systems-lab/go-securesystemslib v0.9.1 // indirect github.com/shopspring/decimal v1.4.0 // indirect github.com/sigstore/fulcio v1.7.1 // indirect diff --git a/internal/operator-controller/applier/provider.go b/internal/operator-controller/applier/provider.go index ffb5eb559..639376a5c 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -69,19 +69,19 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens } if r.IsSingleOwnNamespaceEnabled { - bundleConfigBytes := extensionConfigBytes(ext) - // treat no config as empty to properly validate the configuration - // e.g. ensure that validation catches missing required fields - if bundleConfigBytes == nil { - bundleConfigBytes = []byte(`{}`) + schema, err := rv1.Get() + if err != nil { + return nil, fmt.Errorf("error getting configuration schema: %w", err) } - bundleConfig, err := bundle.UnmarshalConfig(bundleConfigBytes, rv1, ext.Spec.Namespace) + + bundleConfigBytes := extensionConfigBytes(ext) + bundleConfig, err := bundle.UnmarshalConfig(bundleConfigBytes, schema, ext.Spec.Namespace) if err != nil { - return nil, fmt.Errorf("invalid bundle configuration: %w", err) + return nil, fmt.Errorf("invalid ClusterExtension configuration: %w", err) } - if bundleConfig != nil && bundleConfig.WatchNamespace != nil { - opts = append(opts, render.WithTargetNamespaces(*bundleConfig.WatchNamespace)) + if watchNS := bundleConfig.GetWatchNamespace(); watchNS != nil { + opts = append(opts, render.WithTargetNamespaces(*watchNS)) } } diff --git a/internal/operator-controller/applier/provider_test.go b/internal/operator-controller/applier/provider_test.go index 4ec20bead..4138284a0 100644 --- a/internal/operator-controller/applier/provider_test.go +++ b/internal/operator-controller/applier/provider_test.go @@ -97,7 +97,7 @@ func Test_RegistryV1ManifestProvider_Integration(t *testing.T) { _, err := provider.Get(bundleFS, ext) require.Error(t, err) - require.Contains(t, err.Error(), "invalid bundle configuration") + require.Contains(t, err.Error(), "invalid ClusterExtension configuration") }) t.Run("returns rendered manifests", func(t *testing.T) { @@ -326,7 +326,7 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) { }, }) require.Error(t, err) - require.Contains(t, err.Error(), "required field \"watchNamespace\" is missing") + require.Contains(t, err.Error(), `required field "watchNamespace" is missing`) }) t.Run("accepts bundles with {OwnNamespace} install modes when the appropriate configuration is given", func(t *testing.T) { @@ -371,7 +371,7 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) { }, }) require.Error(t, err) - require.Contains(t, err.Error(), "required field \"watchNamespace\" is missing") + require.Contains(t, err.Error(), `required field "watchNamespace" is missing`) }) t.Run("rejects bundles with {OwnNamespace} install modes when watchNamespace is not install namespace", func(t *testing.T) { @@ -392,7 +392,9 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) { }, }) require.Error(t, err) - require.Contains(t, err.Error(), "invalid 'watchNamespace' \"not-install-namespace\": must be install namespace (install-namespace)") + require.Contains(t, err.Error(), "invalid ClusterExtension configuration:") + require.Contains(t, err.Error(), "watchNamespace must be") + require.Contains(t, err.Error(), "install-namespace") }) t.Run("rejects bundles without AllNamespaces, SingleNamespace, or OwnNamespace install mode support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) { diff --git a/internal/operator-controller/rukpak/bundle/config.go b/internal/operator-controller/rukpak/bundle/config.go index 7ec4bfdf0..bd913d5d6 100644 --- a/internal/operator-controller/rukpak/bundle/config.go +++ b/internal/operator-controller/rukpak/bundle/config.go @@ -1,3 +1,27 @@ +// Package bundle validates configuration for different bundle types. +// +// How it works: +// +// Each bundle type (like registry+v1 or Helm) knows what configuration it accepts. +// When a user provides configuration, we validate it before creating a Config object. +// Once created, a Config is guaranteed to be valid - you never need to check it again. +// +// The validation uses JSON Schema: +// 1. Bundle provides its schema (what config is valid) +// 2. We validate the user's config against that schema +// 3. If valid, we create a Config object +// 4. If invalid, we return a helpful error message +// +// Design choices: +// +// - Validation happens once, when creating the Config. There's no Validate() method +// because once you have a Config, it's already been validated. +// +// - Config doesn't hold onto the schema. We only need the schema during validation, +// not after the Config is created. +// +// - You can't create a Config directly. You must go through UnmarshalConfig so that +// validation always happens. package bundle import ( @@ -6,119 +30,349 @@ import ( "fmt" "strings" - "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apimachinery/pkg/util/validation" + "github.com/santhosh-tekuri/jsonschema/v6" "sigs.k8s.io/yaml" +) + +const ( + // configSchemaID is a name we use to identify the config schema when compiling it. + // Think of it like a file name - it just needs to be consistent. + configSchemaID = "config-schema.json" - "github.com/operator-framework/api/pkg/operators/v1alpha1" + // OwnNamespace mode: watchNamespace must equal install namespace + formatOwnNamespaceInstallMode = "ownNamespaceInstallMode" + // SingleNamespace mode: watchNamespace must differ from install namespace + formatSingleNamespaceInstallMode = "singleNamespaceInstallMode" ) +// ConfigSchemaProvider lets each bundle type describe what configuration it accepts. +// +// Different bundle types provide schemas in different ways: +// - registry+v1: builds schema from the operator's install modes +// - Helm: reads schema from values.schema.json in the chart +// - registry+v2: (future) will have its own way +// +// Note: We don't store this in the Config struct. We only need it when validating +// the user's input. After that, the Config has the validated data and doesn't need +// the schema anymore. +type ConfigSchemaProvider interface { + // Get returns a JSON Schema describing what configuration is valid. + // Returns nil if this bundle type doesn't need configuration validation. + Get() (map[string]any, error) +} + +// Config holds validated configuration data from a bundle. +// +// Different bundle types have different configuration options, so we store +// the data in a flexible format and provide accessor methods to get values out. +// +// Why there's no Validate() method: +// We validate configuration when creating a Config. If you have a Config object, +// it's already been validated - you don't need to check it again. The unexported +// 'data' field means you can't create a Config yourself; you have to use +// UnmarshalConfig, which does the validation. type Config struct { - WatchNamespace *string `json:"watchNamespace"` + data map[string]any } -// UnmarshalConfig returns a deserialized *bundle.Config based on bytes and validated -// against rv1 and the desired install namespaces. It will error if: -// - rv is nil -// - bytes is not a valid YAML/JSON object -// - bytes is a valid YAML/JSON object but does not follow the registry+v1 schema -// - if bytes is nil, a nil *bundle.Config is returned with no error -func UnmarshalConfig(bytes []byte, rv1 RegistryV1, installNamespace string) (*Config, error) { - if bytes == nil { - return nil, nil +// newConfig creates a Config from already-validated data. +// This is unexported so all Configs must be created through UnmarshalConfig, +// which ensures validation always happens. +func newConfig(data map[string]any) *Config { + return &Config{data: data} +} + +// GetWatchNamespace returns the watchNamespace value if present in the configuration. +// Returns nil if watchNamespace is not set or is explicitly set to null. +func (c *Config) GetWatchNamespace() *string { + if c == nil || c.data == nil { + return nil + } + val, exists := c.data["watchNamespace"] + if !exists { + return nil + } + // User set watchNamespace: null - treat as "not configured" + if val == nil { + return nil + } + // User set watchNamespace: "some-value" - return it + if str, ok := val.(string); ok { + return &str } + // Shouldn't happen with schema validation, but handle non-string values + str := fmt.Sprintf("%v", val) + return &str +} - bundleConfig := &Config{} - if err := yaml.UnmarshalStrict(bytes, bundleConfig); err != nil { - return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshalError(err)) +// UnmarshalConfig takes user configuration, validates it, and creates a Config object. +// This is the only way to create a Config. +// +// What it does: +// 1. Checks the user's configuration against the schema (if provided) +// 2. If valid, creates a Config object +// 3. If invalid, returns an error explaining what's wrong +// +// Parameters: +// - bytes: the user's configuration in YAML or JSON. If nil, we treat it as empty ({}) +// - schema: describes what configuration is valid. If nil, we skip validation +// - installNamespace: the namespace where the operator will be installed. We use this +// to validate namespace constraints (e.g., OwnNamespace mode requires watchNamespace +// to equal installNamespace) +// +// If the user doesn't provide any configuration but the bundle requires some fields +// (like watchNamespace), validation will fail with a helpful error. +func UnmarshalConfig(bytes []byte, schema map[string]any, installNamespace string) (*Config, error) { + // nil config becomes {} so we can validate required fields + if bytes == nil { + bytes = []byte("{}") } - // collect bundle install modes - bundleInstallModeSet := sets.New(rv1.CSV.Spec.InstallModes...) + // Step 1: Validate against the schema if provided + if schema != nil { + if err := validateConfigWithSchema(bytes, schema, installNamespace); err != nil { + return nil, fmt.Errorf("invalid configuration: %w", err) + } + } - if err := validateConfig(bundleConfig, installNamespace, bundleInstallModeSet); err != nil { - return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", err) + // Step 2: Parse into Config struct + // We use yaml.Unmarshal to parse the validated config into an opaque map. + // Schema validation has already ensured correctness. + var configData map[string]any + if err := yaml.Unmarshal(bytes, &configData); err != nil { + return nil, fmt.Errorf("error unmarshalling configuration: %w", formatUnmarshalError(err)) } - return bundleConfig, nil + return newConfig(configData), nil } -// validateConfig validates a *bundle.Config against the bundle's supported install modes and the user-give installNamespace. -func validateConfig(config *Config, installNamespace string, bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) error { - // no config, no problem - if config == nil { - return nil +// validateConfigWithSchema checks if the user's config matches the schema. +// +// We create a fresh validator each time because the namespace constraints depend on +// which namespace this specific ClusterExtension is being installed into. Each +// ClusterExtension might have a different installNamespace, so we can't reuse validators. +func validateConfigWithSchema(configBytes []byte, schema map[string]any, installNamespace string) error { + var configData interface{} + if err := yaml.Unmarshal(configBytes, &configData); err != nil { + return formatUnmarshalError(err) + } + + compiler := jsonschema.NewCompiler() + + compiler.RegisterFormat(&jsonschema.Format{ + Name: formatOwnNamespaceInstallMode, + Validate: func(value interface{}) error { + // Check it equals install namespace (if installNamespace is set) + // If installNamespace is empty, we can't validate the constraint properly, + // so we skip validation and accept any value. This is a fallback for edge + // cases where the install namespace isn't known yet. + if installNamespace == "" { + return nil + } + str, ok := value.(string) + if !ok { + return fmt.Errorf("value must be a string") + } + if str != installNamespace { + return fmt.Errorf("invalid value %q: watchNamespace must be %q (the namespace where the operator is installed) because this operator only supports OwnNamespace install mode", str, installNamespace) + } + return nil + }, + }) + compiler.RegisterFormat(&jsonschema.Format{ + Name: formatSingleNamespaceInstallMode, + Validate: func(value interface{}) error { + // Check it does NOT equal install namespace (if installNamespace is set) + // If installNamespace is empty, we can't validate the constraint properly, + // so we skip validation and accept any value. This is a fallback for edge + // cases where the install namespace isn't known yet. + if installNamespace == "" { + return nil + } + str, ok := value.(string) + if !ok { + return fmt.Errorf("value must be a string") + } + if str == installNamespace { + return fmt.Errorf("invalid value %q: watchNamespace must be different from %q (the install namespace) because this operator uses SingleNamespace install mode to watch a different namespace", str, installNamespace) + } + return nil + }, + }) + + if err := compiler.AddResource(configSchemaID, schema); err != nil { + return fmt.Errorf("failed to load schema: %w", err) + } + + compiledSchema, err := compiler.Compile(configSchemaID) + if err != nil { + return fmt.Errorf("failed to compile schema: %w", err) + } + + if err := compiledSchema.Validate(configData); err != nil { + return formatSchemaError(err) } - // if the bundle does not support the watchNamespace configuration and it is set, treat it like any unknown field - if config.WatchNamespace != nil && !isWatchNamespaceConfigSupported(bundleInstallModeSet) { - return errors.New(`unknown field "watchNamespace"`) + return nil +} + +// formatSchemaError converts JSON schema validation errors into user-friendly messages. +// If multiple validation errors exist, it combines them into a single error message. +func formatSchemaError(err error) error { + ve := &jsonschema.ValidationError{} + ok := errors.As(err, &ve) + if !ok { + // Not a ValidationError, return as-is + // Caller (UnmarshalConfig) will add "invalid configuration:" prefix + return err } - // if watchNamespace is required then ensure that it is set - if config.WatchNamespace == nil && isWatchNamespaceConfigRequired(bundleInstallModeSet) { - return errors.New(`required field "watchNamespace" is missing`) + // Use BasicOutput() to get structured error information + // This is more robust than parsing error strings + output := ve.BasicOutput() + if output == nil || len(output.Errors) == 0 { + // No structured errors available, fallback to error message + // Note: Using %s not %w since ve.Error() is already a formatted string + return fmt.Errorf("%s", ve.Error()) } - // if watchNamespace is set then ensure it is a valid namespace - if config.WatchNamespace != nil { - if errs := validation.IsDNS1123Subdomain(*config.WatchNamespace); len(errs) > 0 { - return fmt.Errorf("invalid 'watchNamespace' %q: namespace must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", *config.WatchNamespace) + // Collect all error messages + var errorMessages []string + for _, errUnit := range output.Errors { + msg := formatSingleError(errUnit) + if msg != "" { + errorMessages = append(errorMessages, msg) } } - // only accept install namespace if OwnNamespace install mode is supported - if config.WatchNamespace != nil && *config.WatchNamespace == installNamespace && - !bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}) { - return fmt.Errorf("invalid 'watchNamespace' %q: must not be install namespace (%s)", *config.WatchNamespace, installNamespace) + if len(errorMessages) == 0 { + return fmt.Errorf("invalid configuration: %s", ve.Error()) } - // only accept non-install namespace is SingleNamespace is supported - if config.WatchNamespace != nil && *config.WatchNamespace != installNamespace && - !bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) { - return fmt.Errorf("invalid 'watchNamespace' %q: must be install namespace (%s)", *config.WatchNamespace, installNamespace) + // Single error - return it directly + if len(errorMessages) == 1 { + return errors.New(errorMessages[0]) } - return nil + // Multiple errors - combine them + return fmt.Errorf("multiple errors found:\n - %s", strings.Join(errorMessages, "\n - ")) } -// isWatchNamespaceConfigSupported returns true when the bundle exposes a watchNamespace configuration. This happens when: -// - SingleNamespace and/or OwnNamespace install modes are supported -func isWatchNamespaceConfigSupported(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool { - return bundleInstallModeSet.HasAny( - v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}, - v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}, - ) +// formatSingleError formats a single validation error from the schema library. +func formatSingleError(errUnit jsonschema.OutputUnit) string { + // Check the keyword location to identify the error type + switch { + case strings.Contains(errUnit.KeywordLocation, "/required"): + // Missing required field + fieldName := extractFieldNameFromMessage(errUnit.Error) + if fieldName != "" { + return fmt.Sprintf("required field %q is missing", fieldName) + } + return "required field is missing" + + case strings.Contains(errUnit.KeywordLocation, "/additionalProperties"): + // Unknown/additional field + fieldName := extractFieldNameFromMessage(errUnit.Error) + if fieldName != "" { + return fmt.Sprintf("unknown field %q", fieldName) + } + return "unknown field" + + case strings.Contains(errUnit.KeywordLocation, "/type"): + // Type mismatch (e.g., got null, want string) + fieldPath := buildFieldPath(errUnit.InstanceLocation) + if fieldPath != "" { + // Check if this is a "null instead of required value" case + if errUnit.Error != nil && strings.Contains(errUnit.Error.String(), "got null") { + return fmt.Sprintf("required field %q is missing", fieldPath) + } + return fmt.Sprintf("invalid type for field %q: %s", fieldPath, errUnit.Error.String()) + } + return fmt.Sprintf("invalid type: %s", errUnit.Error.String()) + + case strings.Contains(errUnit.KeywordLocation, "/format"): + // Custom format validation (e.g., OwnNamespace, SingleNamespace constraints) + // These already have good error messages from our custom validators + if errUnit.Error != nil { + return errUnit.Error.String() + } + fieldPath := buildFieldPath(errUnit.InstanceLocation) + return fmt.Sprintf("invalid format for field %q", fieldPath) + + case strings.Contains(errUnit.KeywordLocation, "/anyOf"): + // anyOf validation failed - could be null or wrong type + // This happens when a field accepts [null, string] but got something else + fieldPath := buildFieldPath(errUnit.InstanceLocation) + if fieldPath != "" { + return fmt.Sprintf("invalid value for field %q", fieldPath) + } + return "invalid value" + + default: + // Unknown error type - return the library's error message + // This serves as a fallback for future schema features we haven't customized yet + if errUnit.Error != nil { + return errUnit.Error.String() + } + return "" + } +} + +// extractFieldNameFromMessage extracts the field name from error messages. +// Example: "missing property 'watchNamespace'" -> "watchNamespace" +// Example: "additional properties 'unknownField' not allowed" -> "unknownField" +func extractFieldNameFromMessage(errOutput *jsonschema.OutputError) string { + if errOutput == nil { + return "" + } + msg := errOutput.String() + + // Look for field names in single quotes (library's format) + if idx := strings.Index(msg, "'"); idx != -1 { + remaining := msg[idx+1:] + if endIdx := strings.Index(remaining, "'"); endIdx != -1 { + return remaining[:endIdx] + } + } + + return "" } -// isWatchNamespaceConfigRequired returns true if the watchNamespace configuration is required. This happens when -// AllNamespaces install mode is not supported and SingleNamespace and/or OwnNamespace is supported -func isWatchNamespaceConfigRequired(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool { - return isWatchNamespaceConfigSupported(bundleInstallModeSet) && - !bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true}) +// buildFieldPath constructs a field path from instance location array. +// Example: ["watchNamespace"] -> "watchNamespace" +// Example: ["spec", "namespace"] -> "spec.namespace" +func buildFieldPath(location string) string { + // Instance location comes as a JSON pointer like "/watchNamespace" + if location == "" || location == "/" { + return "" + } + // Remove leading slash + path := strings.TrimPrefix(location, "/") + // Replace JSON pointer slashes with dots for readability + path = strings.ReplaceAll(path, "/", ".") + return path } -// formatUnmarshalError format JSON unmarshal errors to be more readable +// formatUnmarshalError makes YAML/JSON parsing errors easier to understand. func formatUnmarshalError(err error) error { - var unmarshalErr *json.UnmarshalTypeError - if errors.As(err, &unmarshalErr) { - if unmarshalErr.Field == "" { + var typeErr *json.UnmarshalTypeError + if errors.As(err, &typeErr) { + if typeErr.Field == "" { return errors.New("input is not a valid JSON object") - } else { - return fmt.Errorf("invalid value type for field %q: expected %q but got %q", unmarshalErr.Field, unmarshalErr.Type.String(), unmarshalErr.Value) } + return fmt.Errorf("invalid value type for field %q: expected %q but got %q", + typeErr.Field, typeErr.Type.String(), typeErr.Value) } - // unwrap error until the core and process it + // Unwrap to core error and strip "json:" or "yaml:" prefix + current := err for { - unwrapped := errors.Unwrap(err) + unwrapped := errors.Unwrap(current) if unwrapped == nil { - // usually the errors present in the form json: or yaml: - // we want to extract if we can - errMessageComponents := strings.Split(err.Error(), ":") - coreErrMessage := strings.TrimSpace(errMessageComponents[len(errMessageComponents)-1]) - return errors.New(coreErrMessage) + parts := strings.Split(current.Error(), ":") + coreMessage := strings.TrimSpace(parts[len(parts)-1]) + return errors.New(coreMessage) } - err = unwrapped + current = unwrapped } } diff --git a/internal/operator-controller/rukpak/bundle/config_test.go b/internal/operator-controller/rukpak/bundle/config_test.go index a6c4b394a..d3cb1c585 100644 --- a/internal/operator-controller/rukpak/bundle/config_test.go +++ b/internal/operator-controller/rukpak/bundle/config_test.go @@ -1,6 +1,7 @@ package bundle_test import ( + "encoding/json" "testing" "github.com/stretchr/testify/require" @@ -14,276 +15,264 @@ import ( func Test_UnmarshalConfig(t *testing.T) { for _, tc := range []struct { - name string - rawConfig []byte - supportedInstallModes []v1alpha1.InstallModeType - installNamespace string - expectedErrMessage string - expectedConfig *bundle.Config + name string + rawConfig []byte + supportedInstallModes []v1alpha1.InstallModeType + installNamespace string + expectedErrMessage string + expectedWatchNamespace *string // Expected value from GetWatchNamespace() }{ { - name: "returns nil for nil config", - rawConfig: nil, - expectedConfig: nil, + name: "accepts nil config when AllNamespaces is supported", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces}, + rawConfig: nil, + expectedWatchNamespace: nil, }, { - name: "accepts json config", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "rejects nil config when OwnNamespace-only requires watchNamespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, + rawConfig: nil, + expectedErrMessage: `required field "watchNamespace" is missing`, }, { - name: "accepts yaml config", + name: "rejects nil config when SingleNamespace-only requires watchNamespace", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`watchNamespace: some-namespace`), - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + rawConfig: nil, + expectedErrMessage: `required field "watchNamespace" is missing`, + }, + { + name: "accepts json config", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "install-ns", // SingleNamespace requires watchNamespace != installNamespace + expectedWatchNamespace: ptr.To("some-namespace"), + }, + { + name: "accepts yaml config", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`watchNamespace: some-namespace`), + installNamespace: "install-ns", // SingleNamespace requires watchNamespace != installNamespace + expectedWatchNamespace: ptr.To("some-namespace"), }, { name: "rejects invalid json", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{"hello`), - expectedErrMessage: `error unmarshalling registry+v1 configuration: found unexpected end of stream`, + expectedErrMessage: `invalid configuration: found unexpected end of stream`, }, { name: "rejects valid json that isn't of object type", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`true`), - expectedErrMessage: `error unmarshalling registry+v1 configuration: input is not a valid JSON object`, + expectedErrMessage: `got boolean, want object`, }, { name: "rejects additional fields", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces}, rawConfig: []byte(`somekey: somevalue`), - expectedErrMessage: `error unmarshalling registry+v1 configuration: unknown field "somekey"`, + expectedErrMessage: `unknown field "somekey"`, }, { name: "rejects valid json but invalid registry+v1", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{"watchNamespace": {"hello": "there"}}`), - expectedErrMessage: `error unmarshalling registry+v1 configuration: invalid value type for field "watchNamespace": expected "string" but got "object"`, - }, - { - name: "rejects bad namespace format", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "bad-Namespace-"}`), - expectedErrMessage: "invalid 'watchNamespace' \"bad-Namespace-\": namespace must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", + expectedErrMessage: `got object, want string`, }, { name: "rejects with unknown field when install modes {AllNamespaces}", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedErrMessage: "unknown field \"watchNamespace\"", + expectedErrMessage: `unknown field "watchNamespace"`, }, { name: "rejects with unknown field when install modes {MultiNamespace}", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedErrMessage: "unknown field \"watchNamespace\"", + expectedErrMessage: `unknown field "watchNamespace"`, }, { name: "reject with unknown field when install modes {AllNamespaces, MultiNamespace}", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeMultiNamespace}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedErrMessage: "unknown field \"watchNamespace\"", + expectedErrMessage: `unknown field "watchNamespace"`, }, { name: "reject with required field when install modes {OwnNamespace} and watchNamespace is null", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{"watchNamespace": null}`), - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "reject with required field when install modes {OwnNamespace} and watchNamespace is missing", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{}`), - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "reject with required field when install modes {MultiNamespace, OwnNamespace} and watchNamespace is null", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace, v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{"watchNamespace": null}`), - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "reject with required field when install modes {MultiNamespace, OwnNamespace} and watchNamespace is missing", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace, v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{}`), - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { - name: "accepts when install modes {SingleNamespace} and watchNamespace != install namespace", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "accepts when install modes {SingleNamespace} and watchNamespace != install namespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "install-ns", + expectedWatchNamespace: ptr.To("some-namespace"), }, { - name: "accepts when install modes {AllNamespaces, SingleNamespace} and watchNamespace != install namespace", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "accepts when install modes {AllNamespaces, SingleNamespace} and watchNamespace != install namespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "install-ns", + expectedWatchNamespace: ptr.To("some-namespace"), }, { - name: "accepts when install modes {MultiNamespace, SingleNamespace} and watchNamespace != install namespace", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace, v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "accepts when install modes {MultiNamespace, SingleNamespace} and watchNamespace != install namespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace, v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "install-ns", + expectedWatchNamespace: ptr.To("some-namespace"), }, { - name: "accepts when install modes {OwnNamespace, SingleNamespace} and watchNamespace != install namespace", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - installNamespace: "not-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "accepts when install modes {OwnNamespace, SingleNamespace} and watchNamespace != install namespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "not-namespace", + expectedWatchNamespace: ptr.To("some-namespace"), }, { name: "rejects when install modes {SingleNamespace} and watchNamespace == install namespace", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), installNamespace: "some-namespace", - expectedErrMessage: "invalid 'watchNamespace' \"some-namespace\": must not be install namespace (some-namespace)", + expectedErrMessage: "invalid configuration:", }, { name: "rejects when install modes {AllNamespaces, SingleNamespace} and watchNamespace == install namespace", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), installNamespace: "some-namespace", - expectedErrMessage: "invalid 'watchNamespace' \"some-namespace\": must not be install namespace (some-namespace)", + expectedErrMessage: "invalid configuration:", }, { name: "rejects when install modes {MultiNamespace, SingleNamespace} and watchNamespace == install namespace", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace, v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), installNamespace: "some-namespace", - expectedErrMessage: "invalid 'watchNamespace' \"some-namespace\": must not be install namespace (some-namespace)", + expectedErrMessage: "invalid configuration:", }, { - name: "accepts when install modes {AllNamespaces, OwnNamespace} and watchNamespace == install namespace", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - installNamespace: "some-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "accepts when install modes {AllNamespaces, OwnNamespace} and watchNamespace == install namespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "some-namespace", + expectedWatchNamespace: ptr.To("some-namespace"), }, { - name: "accepts when install modes {OwnNamespace, SingleNamespace} and watchNamespace == install namespace", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace}, - rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), - installNamespace: "some-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: ptr.To("some-namespace"), - }, + name: "accepts when install modes {OwnNamespace, SingleNamespace} and watchNamespace == install namespace", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace}, + rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), + installNamespace: "some-namespace", + expectedWatchNamespace: ptr.To("some-namespace"), }, { name: "rejects when install modes {AllNamespaces, OwnNamespace} and watchNamespace != install namespace", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{"watchNamespace": "some-namespace"}`), installNamespace: "not-some-namespace", - expectedErrMessage: "invalid 'watchNamespace' \"some-namespace\": must be install namespace (not-some-namespace)", + expectedErrMessage: "invalid configuration:", }, { name: "rejects with required field error when install modes {SingleNamespace} and watchNamespace is nil", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{"watchNamespace": null}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "rejects with required field error when install modes {SingleNamespace, OwnNamespace} and watchNamespace is nil", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{"watchNamespace": null}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "rejects with required field error when install modes {SingleNamespace, MultiNamespace} and watchNamespace is nil", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeMultiNamespace}, rawConfig: []byte(`{"watchNamespace": null}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "rejects with required field error when install modes {SingleNamespace} and watchNamespace is missing", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, rawConfig: []byte(`{}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "rejects with required field error when install modes {SingleNamespace, OwnNamespace} and watchNamespace is missing", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace}, rawConfig: []byte(`{}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "rejects with required field error when install modes {SingleNamespace, MultiNamespace} and watchNamespace is missing", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeMultiNamespace}, rawConfig: []byte(`{}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { name: "rejects with required field error when install modes {SingleNamespace, OwnNamespace, MultiNamespace} and watchNamespace is nil", supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace}, rawConfig: []byte(`{"watchNamespace": null}`), installNamespace: "not-some-namespace", - expectedErrMessage: "required field \"watchNamespace\" is missing", + expectedErrMessage: `required field "watchNamespace" is missing`, }, { - name: "accepts null watchNamespace when install modes {AllNamespaces, OwnNamespace} and watchNamespace is nil", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, - rawConfig: []byte(`{"watchNamespace": null}`), - installNamespace: "not-some-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: nil, - }, + name: "accepts null watchNamespace when install modes {AllNamespaces, OwnNamespace} and watchNamespace is nil", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, + rawConfig: []byte(`{"watchNamespace": null}`), + installNamespace: "not-some-namespace", + expectedWatchNamespace: nil, }, { - name: "accepts null watchNamespace when install modes {AllNamespaces, OwnNamespace, MultiNamespace} and watchNamespace is nil", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace}, - rawConfig: []byte(`{"watchNamespace": null}`), - installNamespace: "not-some-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: nil, - }, + name: "accepts null watchNamespace when install modes {AllNamespaces, OwnNamespace, MultiNamespace} and watchNamespace is nil", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace}, + rawConfig: []byte(`{"watchNamespace": null}`), + installNamespace: "not-some-namespace", + expectedWatchNamespace: nil, }, { - name: "accepts no watchNamespace when install modes {AllNamespaces, OwnNamespace} and watchNamespace is nil", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, - rawConfig: []byte(`{}`), - installNamespace: "not-some-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: nil, - }, + name: "accepts no watchNamespace when install modes {AllNamespaces, OwnNamespace} and watchNamespace is nil", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace}, + rawConfig: []byte(`{}`), + installNamespace: "not-some-namespace", + expectedWatchNamespace: nil, }, { - name: "accepts no watchNamespace when install modes {AllNamespaces, OwnNamespace, MultiNamespace} and watchNamespace is nil", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace}, - rawConfig: []byte(`{}`), - installNamespace: "not-some-namespace", - expectedConfig: &bundle.Config{ - WatchNamespace: nil, - }, + name: "accepts no watchNamespace when install modes {AllNamespaces, OwnNamespace, MultiNamespace} and watchNamespace is nil", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace}, + rawConfig: []byte(`{}`), + installNamespace: "not-some-namespace", + expectedWatchNamespace: nil, }, { - name: "rejects with format error when install modes are {SingleNamespace, OwnNamespace} and watchNamespace is ''", - supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace}, - rawConfig: []byte(`{"watchNamespace": ""}`), - installNamespace: "not-some-namespace", - expectedErrMessage: "invalid 'watchNamespace' \"\": namespace must consist of lower case alphanumeric characters", + name: "skips validation when installNamespace is empty for OwnNamespace only", + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, + rawConfig: []byte(`{"watchNamespace": "valid-ns"}`), + installNamespace: "", + expectedWatchNamespace: ptr.To("valid-ns"), }, } { t.Run(tc.name, func(t *testing.T) { @@ -297,14 +286,288 @@ func Test_UnmarshalConfig(t *testing.T) { } } - config, err := bundle.UnmarshalConfig(tc.rawConfig, rv1, tc.installNamespace) - require.Equal(t, tc.expectedConfig, config) + schema, err := rv1.Get() + require.NoError(t, err) + + config, err := bundle.UnmarshalConfig(tc.rawConfig, schema, tc.installNamespace) + if tc.expectedErrMessage != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedErrMessage) + } else { + require.NoError(t, err) + require.NotNil(t, config) + if tc.expectedWatchNamespace == nil { + require.Nil(t, config.GetWatchNamespace()) + } else { + require.Equal(t, *tc.expectedWatchNamespace, *config.GetWatchNamespace()) + } + } + }) + } +} + +// Test_UnmarshalConfig_EmptySchema tests when a bundle doesn't provide a configuration schema. +func Test_UnmarshalConfig_EmptySchema(t *testing.T) { + for _, tc := range []struct { + name string + rawConfig []byte + expectedErrMessage string + expectedWatchNamespace *string + }{ + { + name: "no config provided", + rawConfig: nil, + expectedWatchNamespace: nil, + }, + { + name: "empty config provided", + rawConfig: []byte(`{}`), + expectedWatchNamespace: nil, + }, + { + name: "config with watchNamespace provided", + rawConfig: []byte(`{"watchNamespace": "some-ns"}`), + expectedWatchNamespace: ptr.To("some-ns"), + }, + { + name: "config with unknown fields provided", + rawConfig: []byte(`{"someField": "someValue"}`), + expectedWatchNamespace: nil, + }, + } { + t.Run(tc.name, func(t *testing.T) { + emptySchemaBundle := &mockEmptySchemaBundle{} + schema, err := emptySchemaBundle.Get() + require.NoError(t, err) + + config, err := bundle.UnmarshalConfig(tc.rawConfig, schema, "my-namespace") + + if tc.expectedErrMessage != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedErrMessage) + } else { + require.NoError(t, err) + require.NotNil(t, config) + if tc.expectedWatchNamespace == nil { + require.Nil(t, config.GetWatchNamespace()) + } else { + require.Equal(t, *tc.expectedWatchNamespace, *config.GetWatchNamespace()) + } + } + }) + } +} + +// Test_UnmarshalConfig_HelmLike proves validation works the same for ANY bundle type. +// +// - registry+v1 -> generates schema from install modes +// - Helm -> reads values.schema.json from chart +// - registry+v2 -> (future) provides schema via its own mechanism +// +// Same validation process regardless of bundle type. +func Test_UnmarshalConfig_HelmLike(t *testing.T) { + for _, tc := range []struct { + name string + rawConfig []byte + helmSchema string // what values.schema.json would contain + expectedErrMessage string + expectedWatchNamespace *string + }{ + { + name: "Helm chart with typical config values (no watchNamespace)", + rawConfig: []byte(`{ + "replicaCount": 3, + "image": {"tag": "v1.2.3"}, + "service": {"port": 8080} + }`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "replicaCount": {"type": "integer", "minimum": 1}, + "image": { + "type": "object", + "properties": { + "tag": {"type": "string"} + } + }, + "service": { + "type": "object", + "properties": { + "port": {"type": "integer"} + } + } + } + }`, + expectedWatchNamespace: nil, + }, + { + name: "Helm chart that ALSO uses watchNamespace (mixed config)", + rawConfig: []byte(`{ + "watchNamespace": "my-app-namespace", + "replicaCount": 2, + "debug": true + }`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "watchNamespace": {"type": "string"}, + "replicaCount": {"type": "integer"}, + "debug": {"type": "boolean"} + } + }`, + // watchNamespace gets extracted, other fields validated by schema + expectedWatchNamespace: ptr.To("my-app-namespace"), + }, + { + name: "Schema validation catches constraint violations (replicaCount below minimum)", + rawConfig: []byte(`{"replicaCount": 0}`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "replicaCount": {"type": "integer", "minimum": 1} + } + }`, + expectedErrMessage: "invalid configuration:", + }, + { + name: "Schema validation catches type mismatches", + rawConfig: []byte(`{"replicaCount": "three"}`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "replicaCount": {"type": "integer"} + } + }`, + expectedErrMessage: "invalid configuration:", + }, + { + name: "Empty config is valid when no required fields", + rawConfig: nil, + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "replicaCount": {"type": "integer", "default": 1} + } + }`, + expectedWatchNamespace: nil, + }, + { + name: "Required fields are enforced by schema", + rawConfig: []byte(`{"optional": "value"}`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "required": ["requiredField"], + "properties": { + "requiredField": {"type": "string"}, + "optional": {"type": "string"} + } + }`, + expectedErrMessage: `required field "requiredField" is missing`, + }, + { + name: "Helm with watchNamespace accepts any string value (K8s validates at runtime)", + rawConfig: []byte(`{ + "watchNamespace": "any-value-here", + "replicaCount": 2 + }`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "watchNamespace": {"type": "string"}, + "replicaCount": {"type": "integer"} + } + }`, + expectedWatchNamespace: ptr.To("any-value-here"), + }, + { + name: "Helm with watchNamespace using ownNamespaceInstallMode format (OwnNamespace-like)", + rawConfig: []byte(`{ + "watchNamespace": "wrong-namespace" + }`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "required": ["watchNamespace"], + "properties": { + "watchNamespace": {"type": "string", "format": "ownNamespaceInstallMode"} + } + }`, + expectedErrMessage: "invalid configuration:", + }, + { + name: "Helm with watchNamespace using singleNamespaceInstallMode format (SingleNamespace-like)", + rawConfig: []byte(`{ + "watchNamespace": "my-namespace" + }`), + helmSchema: `{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "required": ["watchNamespace"], + "properties": { + "watchNamespace": {"type": "string", "format": "singleNamespaceInstallMode"} + } + }`, + expectedErrMessage: "invalid configuration:", + }, + } { + t.Run(tc.name, func(t *testing.T) { + // Create a mock Helm bundle (real Helm would read values.schema.json) + helmBundle := &mockHelmBundle{schema: tc.helmSchema} + schema, err := helmBundle.Get() + require.NoError(t, err) + + // Same validation function works for Helm, registry+v1, registry+v2, etc. + config, err := bundle.UnmarshalConfig(tc.rawConfig, schema, "my-namespace") + if tc.expectedErrMessage != "" { require.Error(t, err) require.Contains(t, err.Error(), tc.expectedErrMessage) } else { require.NoError(t, err) + require.NotNil(t, config) + if tc.expectedWatchNamespace == nil { + require.Nil(t, config.GetWatchNamespace()) + } else { + require.Equal(t, *tc.expectedWatchNamespace, *config.GetWatchNamespace()) + } } }) } } + +// mockHelmBundle shows how Helm would plug into the validation system. +// +// Real implementation would: +// 1. Read values.schema.json from the Helm chart package +// 2. Parse it into a map[string]any +// 3. Return it (just like registry+v1 returns its generated schema) +// 4. Let the shared validation logic handle the rest +type mockHelmBundle struct { + schema string +} + +// Get returns the schema (in real Helm, read from values.schema.json). +func (h *mockHelmBundle) Get() (map[string]any, error) { + if h.schema == "" { + return nil, nil + } + var schemaMap map[string]any + if err := json.Unmarshal([]byte(h.schema), &schemaMap); err != nil { + return nil, err + } + return schemaMap, nil +} + +// mockEmptySchemaBundle represents a bundle that doesn't provide a configuration schema. +type mockEmptySchemaBundle struct{} + +func (e *mockEmptySchemaBundle) Get() (map[string]any, error) { + return nil, nil +} diff --git a/internal/operator-controller/rukpak/bundle/error_formatting_test.go b/internal/operator-controller/rukpak/bundle/error_formatting_test.go new file mode 100644 index 000000000..0fdb5621a --- /dev/null +++ b/internal/operator-controller/rukpak/bundle/error_formatting_test.go @@ -0,0 +1,173 @@ +package bundle_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" + + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing/clusterserviceversion" +) + +// Test_ErrorFormatting_SchemaLibraryVersion verifies error messages from the JSON schema +// library and our custom format validators. +// +// These tests serve two purposes: +// 1. Guard against breaking changes if we upgrade github.com/santhosh-tekuri/jsonschema/v6 +// (tests for formatSchemaError parsing may need updates) +// 2. Document the actual error messages end users see (especially for namespace constraints) +func Test_ErrorFormatting_SchemaLibraryVersion(t *testing.T) { + for _, tc := range []struct { + name string + rawConfig []byte + supportedInstallModes []v1alpha1.InstallModeType + installNamespace string + // We verify the error message contains these key phrases + expectedErrSubstrings []string + }{ + { + name: "Unknown field error formatting", + rawConfig: []byte(`{"unknownField": "value"}`), + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces}, + expectedErrSubstrings: []string{ + "unknown field", + "unknownField", + }, + }, + { + name: "Required field missing error formatting", + rawConfig: []byte(`{}`), + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, + expectedErrSubstrings: []string{ + "required field", + "watchNamespace", + "is missing", + }, + }, + { + name: "Required field null error formatting", + rawConfig: []byte(`{"watchNamespace": null}`), + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + expectedErrSubstrings: []string{ + "required field", + "watchNamespace", + "is missing", + }, + }, + { + name: "Type mismatch error formatting", + rawConfig: []byte(`{"watchNamespace": 123}`), + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + expectedErrSubstrings: []string{ + "invalid type", + "watchNamespace", + }, + }, + { + name: "OwnNamespace constraint error formatting", + rawConfig: []byte(`{"watchNamespace": "wrong-namespace"}`), + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, + installNamespace: "correct-namespace", + expectedErrSubstrings: []string{ + "invalid value", + "wrong-namespace", + "watchNamespace must be", + "correct-namespace", + "the namespace where the operator is installed", + "OwnNamespace install mode", + }, + }, + { + name: "SingleNamespace constraint error formatting", + rawConfig: []byte(`{"watchNamespace": "install-ns"}`), + supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace}, + installNamespace: "install-ns", + expectedErrSubstrings: []string{ + "invalid value", + "install-ns", + "watchNamespace must be different from", + "the install namespace", + "SingleNamespace install mode", + "watch a different namespace", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + rv1 := bundle.RegistryV1{ + CSV: clusterserviceversion.Builder(). + WithName("test-operator"). + WithInstallModeSupportFor(tc.supportedInstallModes...). + Build(), + } + + schema, err := rv1.Get() + require.NoError(t, err) + + _, err = bundle.UnmarshalConfig(tc.rawConfig, schema, tc.installNamespace) + require.Error(t, err, "Expected validation error") + + errMsg := err.Error() + for _, substring := range tc.expectedErrSubstrings { + require.Contains(t, errMsg, substring, + "Error message should contain %q. Full error: %s", substring, errMsg) + } + }) + } +} + +// Test_ErrorFormatting_YAMLParseErrors verifies YAML/JSON parsing errors are formatted correctly. +func Test_ErrorFormatting_YAMLParseErrors(t *testing.T) { + for _, tc := range []struct { + name string + rawConfig []byte + expectedErrSubstrings []string + }{ + { + name: "Malformed JSON", + rawConfig: []byte(`{"incomplete`), + expectedErrSubstrings: []string{ + "unexpected end of stream", + }, + }, + { + name: "Non-object JSON", + rawConfig: []byte(`true`), + expectedErrSubstrings: []string{ + "invalid type", + "got boolean, want object", + }, + }, + { + name: "Wrong type for field", + rawConfig: []byte(`{"watchNamespace": {"nested": "object"}}`), + expectedErrSubstrings: []string{ + "invalid type", + "got object, want string", + "watchNamespace", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + rv1 := bundle.RegistryV1{ + CSV: clusterserviceversion.Builder(). + WithName("test-operator"). + WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace). + Build(), + } + + schema, err := rv1.Get() + require.NoError(t, err) + + _, err = bundle.UnmarshalConfig(tc.rawConfig, schema, "test-namespace") + require.Error(t, err, "Expected parse error") + + errMsg := err.Error() + for _, substring := range tc.expectedErrSubstrings { + require.Contains(t, errMsg, substring, + "Error message should contain %q. Full error: %s", substring, errMsg) + } + }) + } +} diff --git a/internal/operator-controller/rukpak/bundle/registryv1.go b/internal/operator-controller/rukpak/bundle/registryv1.go index cffc374e9..82e53caf5 100644 --- a/internal/operator-controller/rukpak/bundle/registryv1.go +++ b/internal/operator-controller/rukpak/bundle/registryv1.go @@ -3,6 +3,7 @@ package bundle import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" "github.com/operator-framework/api/pkg/operators/v1alpha1" ) @@ -17,3 +18,134 @@ type RegistryV1 struct { CRDs []apiextensionsv1.CustomResourceDefinition Others []unstructured.Unstructured } + +// Get builds a validation schema based on what install modes the operator supports. +// +// For registry+v1 bundles, we look at the CSV's install modes and generate a schema +// that matches. For example, if the operator only supports OwnNamespace mode, we'll +// require the user to provide a watchNamespace that equals the install namespace. +func (rv1 RegistryV1) Get() (map[string]any, error) { + installModes := sets.New(rv1.CSV.Spec.InstallModes...) + return buildBundleConfigSchema(installModes) +} + +// buildBundleConfigSchema creates validation rules based on what the operator supports. +// +// Examples of how install modes affect validation: +// - AllNamespaces only: user can't set watchNamespace (operator watches everything) +// - OwnNamespace only: user must set watchNamespace to the install namespace +// - SingleNamespace only: user must set watchNamespace to a different namespace +// - AllNamespaces + OwnNamespace: user can optionally set watchNamespace +func buildBundleConfigSchema(installModes sets.Set[v1alpha1.InstallMode]) (map[string]any, error) { + schema := map[string]any{ + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "additionalProperties": false, // Reject unknown fields (catches typos and misconfigurations) + } + + properties := map[string]any{} + var required []any + + // Add watchNamespace property if the bundle supports it + if isWatchNamespaceConfigurable(installModes) { + watchNSProperty, isRequired := buildWatchNamespaceProperty(installModes) + properties["watchNamespace"] = watchNSProperty + if isRequired { + required = append(required, "watchNamespace") + } + } + + schema["properties"] = properties + if len(required) > 0 { + schema["required"] = required + } + + return schema, nil +} + +// buildWatchNamespaceProperty creates the validation rules for the watchNamespace field. +// +// The rules depend on what install modes are supported: +// - AllNamespaces supported: watchNamespace is optional (can be null) +// - Only Single/Own supported: watchNamespace is required +// - Only OwnNamespace: must equal install namespace +// - Only SingleNamespace: must be different from install namespace +// +// Returns the validation rules and whether the field is required. +func buildWatchNamespaceProperty(installModes sets.Set[v1alpha1.InstallMode]) (map[string]any, bool) { + watchNSProperty := map[string]any{ + "description": "The namespace that the operator should watch for custom resources", + } + + hasOwnNamespace := installModes.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}) + hasSingleNamespace := installModes.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) + + format := selectNamespaceFormat(hasOwnNamespace, hasSingleNamespace) + + if isWatchNamespaceConfigRequired(installModes) { + watchNSProperty["type"] = "string" + if format != "" { + watchNSProperty["format"] = format + } + return watchNSProperty, true + } + + // allow null or valid namespace string + stringSchema := map[string]any{ + "type": "string", + } + if format != "" { + stringSchema["format"] = format + } + // Convert to []any for JSON schema compatibility + watchNSProperty["anyOf"] = []any{ + map[string]any{"type": "null"}, + stringSchema, + } + + return watchNSProperty, false +} + +// selectNamespaceFormat picks which namespace constraint to apply. +// +// - OwnNamespace only: watchNamespace must equal install namespace +// - SingleNamespace only: watchNamespace must be different from install namespace +// - Both or neither: no constraint, any namespace name is valid +func selectNamespaceFormat(hasOwnNamespace, hasSingleNamespace bool) string { + if hasOwnNamespace && !hasSingleNamespace { + return formatOwnNamespaceInstallMode + } + if hasSingleNamespace && !hasOwnNamespace { + return formatSingleNamespaceInstallMode + } + return "" // No format constraint needed for generic case +} + +// isWatchNamespaceConfigurable checks if the user can set a watchNamespace. +// +// Returns true if: +// - SingleNamespace is supported (user picks a namespace to watch) +// - OwnNamespace is supported (user sets watchNamespace to the install namespace) +// +// Returns false if: +// - Only AllNamespaces is supported (operator always watches everything) +func isWatchNamespaceConfigurable(installModes sets.Set[v1alpha1.InstallMode]) bool { + return installModes.HasAny( + v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}, + v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}, + ) +} + +// isWatchNamespaceConfigRequired checks if the user must provide a watchNamespace. +// +// Returns true (required) when: +// - Only OwnNamespace is supported +// - Only SingleNamespace is supported +// - Both OwnNamespace and SingleNamespace are supported +// +// Returns false (optional) when: +// - AllNamespaces is supported (user can leave it unset to watch all namespaces) +func isWatchNamespaceConfigRequired(installModes sets.Set[v1alpha1.InstallMode]) bool { + return isWatchNamespaceConfigurable(installModes) && + !installModes.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true}) +} diff --git a/test/e2e/single_namespace_support_test.go b/test/e2e/single_namespace_support_test.go index 7e8fe7bd5..2c3b825a1 100644 --- a/test/e2e/single_namespace_support_test.go +++ b/test/e2e/single_namespace_support_test.go @@ -148,7 +148,7 @@ func TestClusterExtensionSingleNamespaceSupport(t *testing.T) { require.NotNil(ct, cond) require.Equal(ct, metav1.ConditionTrue, cond.Status) require.Equal(ct, ocv1.ReasonRetrying, cond.Reason) - require.Contains(ct, cond.Message, "required field \"watchNamespace\" is missing") + require.Contains(ct, cond.Message, `required field "watchNamespace" is missing`) }, pollDuration, pollInterval) t.Log("By updating the ClusterExtension configuration with a watchNamespace") @@ -296,7 +296,7 @@ func TestClusterExtensionOwnNamespaceSupport(t *testing.T) { require.NotNil(ct, cond) require.Equal(ct, metav1.ConditionTrue, cond.Status) require.Equal(ct, ocv1.ReasonRetrying, cond.Reason) - require.Contains(ct, cond.Message, "required field \"watchNamespace\" is missing") + require.Contains(ct, cond.Message, `required field "watchNamespace" is missing`) }, pollDuration, pollInterval) t.Log("By updating the ClusterExtension configuration with a watchNamespace other than the install namespace") @@ -318,7 +318,9 @@ func TestClusterExtensionOwnNamespaceSupport(t *testing.T) { require.NotNil(ct, cond) require.Equal(ct, metav1.ConditionTrue, cond.Status) require.Equal(ct, ocv1.ReasonRetrying, cond.Reason) - require.Contains(ct, cond.Message, fmt.Sprintf("invalid 'watchNamespace' \"some-namespace\": must be install namespace (%s)", clusterExtension.Spec.Namespace)) + require.Contains(ct, cond.Message, "invalid ClusterExtension configuration") + require.Contains(ct, cond.Message, fmt.Sprintf("watchNamespace must be \"%s\"", clusterExtension.Spec.Namespace)) + require.Contains(ct, cond.Message, "OwnNamespace install mode") }, pollDuration, pollInterval) t.Log("By updating the ClusterExtension configuration with a watchNamespace = install namespace")