From 36e0721b385204978e4eeb9e2321613090a82346 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 23 Aug 2024 01:13:11 +0800 Subject: [PATCH] Broker class based defaults (#7631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * First draft of the default broker class code Signed-off-by: Leo Li * Change the default broker class config Signed-off-by: Leo Li * Run codegen Signed-off-by: Leo Li * Refactor the code Signed-off-by: Leo Li * Codegen Signed-off-by: Leo Li * Fix the build error and run code gen Signed-off-by: Leo Li * Fix the build error in the tests Signed-off-by: Leo Li * Fix the build error in the tests Signed-off-by: Leo Li * Modify the current existing test, and add more test cases Signed-off-by: Leo Li * Modify the current existing test, and add more test cases Signed-off-by: Leo Li * Pass in the actual broker name instead of leaving it empty Signed-off-by: Leo Li * Modify the condition to check whether Different Namespace is allowed according to the default configs Signed-off-by: Leo Li * Revert the brokerSpec.Name change. Signed-off-by: Leo Li * Fix the failing unit tests by adding the default config for the defaults Signed-off-by: Leo Li * Fix the failing unit tests Signed-off-by: Leo Li * Fix the review comments Signed-off-by: Leo Li * Update pkg/apis/config/defaults.go Co-authored-by: Christoph Stäbler * Change the tests to be table form Signed-off-by: Leo Li * Check if the brokerClassName is the default broker class for the whole cluster Signed-off-by: Leo Li * Fixed the build error Signed-off-by: Leo Li * fix: instead of always passing the empty className, pass in the variable instead Signed-off-by: Leo Li * fix: brokerClass is a string pointer. If the value is null but pointer has value, it will pass. So this commit fix that problem. Signed-off-by: Leo Li * feat: adding some temporary test cases for testing purposes Signed-off-by: Leo Li * feat: adding some temporary test cases for testing purposes Signed-off-by: Leo Li * fix: fix the failing tests Signed-off-by: Leo Li * fix: fix FIXME, the issue that can potentially cause SegFault Signed-off-by: Leo Li * fix: fix the failing rabbitmq issue Signed-off-by: Leo Li * fix: remove the trailing whitespace Signed-off-by: Leo Li * fix: update the comments Signed-off-by: Leo Li * feat: add the high level summary for the code Signed-off-by: Leo Li * fix: fix the review comments Signed-off-by: Leo Li * fix: fix the review comments Signed-off-by: Leo Li * fix: fix Calum's review comments Signed-off-by: Leo Li * fix: fix failed unit test caused by the latest review comment fix Signed-off-by: Leo Li * fix: fix failed unit test caused by the latest review comment fix Signed-off-by: Leo Li * feat: add the test for the deprecating default broker config feature Signed-off-by: Leo Li --------- Signed-off-by: Leo Li Co-authored-by: Christoph Stäbler --- pkg/apis/config/defaults.go | 199 +++- pkg/apis/config/defaults_test.go | 874 +++++++++++++++--- .../config/testdata/config-br-defaults.yaml | 91 +- pkg/apis/config/zz_generated.deepcopy.go | 35 +- pkg/apis/eventing/v1/broker_defaults.go | 15 +- pkg/apis/eventing/v1/broker_defaults_test.go | 193 +++- pkg/apis/eventing/v1/broker_validation.go | 40 +- .../eventing/v1/broker_validation_test.go | 6 +- .../sugar/namespace/namespace_test.go | 13 +- pkg/reconciler/sugar/trigger/trigger_test.go | 14 +- 10 files changed, 1279 insertions(+), 201 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index f12b83ab534..fcbdb50a521 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -68,28 +68,88 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) return NewDefaultsConfigFromMap(config.Data) } +/* +The priority precedence for determining the broker class and configuration is as follows: + +1. If a specific broker class is provided: + a. Check namespace-specific configuration for the given broker class + b. If not found, check cluster-wide configuration for the given broker class + c. If still not found, use the cluster-wide default configuration + +2. If no specific broker class is provided: + a. Check namespace-specific default broker class + b. If found, use its configuration (following step 1) + c. If not found, use cluster-wide default broker class and configuration + +3. If no default cluster configuration is set, return an error + +4. If no default namespace configuration is set, will use the cluster-wide default configuration. + +This can be represented as a flow chart: + + Start + | + v + Is broker class provided? + / \ + Yes No + / \ + Check namespace config Check namespace + for provided class default class + | | + v v + Found? Found? + / \ / \ + Yes No Yes No + | | | | + | | | | + | v | v + | Check cluster | Use cluster + | config for class | default class + | | | and config + | v | + | Found? | + | / \ | + | Yes No | + | | | | + | | v | + | | Use cluster | + | | default config | + v v v + Use found configuration + +The system prioritizes namespace-specific configurations over cluster-wide defaults, +and explicitly provided broker classes over default classes. +*/ + // Defaults includes the default values to be populated by the webhook. type Defaults struct { // NamespaceDefaultsConfig are the default Broker Configs for each namespace. - // Namespace is the key, the value is the KReference to the config. - NamespaceDefaultsConfig map[string]*ClassAndBrokerConfig `json:"namespaceDefaults,omitempty"` + // Namespace is the key, the value is the NamespaceDefaultConfig + NamespaceDefaultsConfig map[string]*DefaultConfig `json:"namespaceDefaults,omitempty"` - // ClusterDefaultBrokerConfig is the default broker config for all the namespaces that - // are not in NamespaceDefaultBrokerConfigs. - ClusterDefault *ClassAndBrokerConfig `json:"clusterDefault,omitempty"` + // ClusterDefaultConfig is the default broker config for all the namespaces that + // are not in NamespaceDefaultBrokerConfigs. Different broker class could have + // different default config. + ClusterDefaultConfig *DefaultConfig `json:"clusterDefault,omitempty"` } -// ClassAndBrokerConfig contains configuration for a given namespace for broker. Allows -// configuring the Class of the Broker, the reference to the -// config it should use and it's delivery. -type ClassAndBrokerConfig struct { - BrokerClass string `json:"brokerClass,omitempty"` +// DefaultConfig struct contains the default configuration for the cluster and namespace. +type DefaultConfig struct { + // DefaultBrokerClass and DefaultBrokerClassConfig are the default broker class for the whole cluster/namespace. + // Users have to specify both of them + DefaultBrokerClass string `json:"brokerClass,omitempty"` + + //Deprecated: Use the config in BrokerClasses instead *BrokerConfig `json:",inline"` + // Optional: BrokerClasses are the default broker classes' config. The key is the broker class name, and the value is the config for that broker class. + BrokerClasses map[string]*BrokerConfig `json:"brokerClasses,omitempty"` + DisallowDifferentNamespaceConfig *bool `json:"disallowDifferentNamespaceConfig,omitempty"` } -// BrokerConfig contains configuration for a given namespace for broker. Allows +// BrokerConfig contains configuration for a given broker. Allows // configuring the reference to the // config it should use and it's delivery. type BrokerConfig struct { @@ -97,36 +157,115 @@ type BrokerConfig struct { Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"` } -// GetBrokerConfig returns a namespace specific Broker Configuration, and if -// that doesn't exist, return a Cluster Default and if that doesn't exist -// return an error. -func (d *Defaults) GetBrokerConfig(ns string) (*BrokerConfig, error) { +// GetBrokerConfig returns a namespace specific Broker Config, and if +// that doesn't exist, return a Cluster Default and if that doesn't exist return an error. +func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerConfig, error) { if d == nil { - return nil, errors.New("Defaults are nil") + return nil, errors.New("Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.") + } + + // Early return if brokerClassName is provided and valid + if brokerClassName != nil && *brokerClassName != "" { + return d.getBrokerConfigByClassName(ns, *brokerClassName) + } + + // Handling empty brokerClassName + return d.getBrokerConfigForEmptyClassName(ns) +} + +// getBrokerConfigByClassName returns the BrokerConfig for the given brokerClassName. +// It first checks the namespace specific configuration, then the cluster default configuration. +func (d *Defaults) getBrokerConfigByClassName(ns string, brokerClassName string) (*BrokerConfig, error) { + // Check namespace specific configuration + if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil { + // check if the brokerClassName is the default broker class for this namespace + if nsConfig.DefaultBrokerClass == brokerClassName { + if nsConfig.BrokerConfig == nil { + // as no default broker class config is set for this namespace, check whether nsconfig's brokerClasses map has the config for this broker class + if config, ok := nsConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil + } + // if not found, return the cluster default config + return d.getClusterDefaultBrokerConfig(brokerClassName) + } else { + // If the brokerClassName exists in the BrokerClasses, return the config in the BrokerClasses + if config, ok := nsConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil + } else { + // If the brokerClassName is the default broker class for the namespace, return the BrokerConfig + return nsConfig.BrokerConfig, nil + } + } + } else { + // if the brokerClassName is not the default broker class for the namespace, check whether nsconfig's brokerClasses map has the config for this broker class + if config, ok := nsConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil + } + } } - value, present := d.NamespaceDefaultsConfig[ns] - if present && value.BrokerConfig != nil { - return value.BrokerConfig, nil + + // Check cluster default configuration + return d.getClusterDefaultBrokerConfig(brokerClassName) +} + +// getBrokerConfigForEmptyClassName returns the BrokerConfig for the given namespace when brokerClassName is empty. +// It first checks the namespace specific configuration, then the cluster default configuration. +func (d *Defaults) getBrokerConfigForEmptyClassName(ns string) (*BrokerConfig, error) { + // Check if namespace has a default broker class + if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil { + if nsConfig.DefaultBrokerClass != "" { + return d.getBrokerConfigByClassName(ns, nsConfig.DefaultBrokerClass) + } } - if d.ClusterDefault != nil && d.ClusterDefault.BrokerConfig != nil { - return d.ClusterDefault.BrokerConfig, nil + + // Fallback to cluster default configuration + return d.getClusterDefaultBrokerConfig("") +} + +// getClusterDefaultBrokerConfig returns the BrokerConfig for the given brokerClassName. +func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*BrokerConfig, error) { + if d.ClusterDefaultConfig == nil || d.ClusterDefaultConfig.BrokerConfig == nil { + return nil, errors.New("Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.") } - return nil, errors.New("Defaults for Broker Configurations have not been set up.") + + // Check if the brokerClassName is the default broker class for the whole cluster + if brokerClassName == "" || d.ClusterDefaultConfig.DefaultBrokerClass == brokerClassName { + // If the brokerClassName exists in the BrokerClasses, return the config in the BrokerClasses + if config, ok := d.ClusterDefaultConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil + } else { + // If the brokerClassName is the default broker class for the cluster, return the BrokerConfig + return d.ClusterDefaultConfig.BrokerConfig, nil + } + } + + if config, ok := d.ClusterDefaultConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil + } + + return d.ClusterDefaultConfig.BrokerConfig, nil } // GetBrokerClass returns a namespace specific Broker Class, and if -// that doesn't exist, return a Cluster Default and if that doesn't exist +// that doesn't exist, return a Cluster Default and if the defaults doesn't exist // return an error. func (d *Defaults) GetBrokerClass(ns string) (string, error) { if d == nil { - return "", errors.New("Defaults are nil") + return "", errors.New("Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.") } - value, present := d.NamespaceDefaultsConfig[ns] - if present && value.BrokerClass != "" { - return value.BrokerClass, nil + + // Check if the namespace has a specific configuration + if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil { + if nsConfig.DefaultBrokerClass != "" { + return nsConfig.DefaultBrokerClass, nil + } } - if d.ClusterDefault != nil && d.ClusterDefault.BrokerClass != "" { - return d.ClusterDefault.BrokerClass, nil + + // Fallback to cluster default configuration if namespace specific configuration is not set + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { + return d.ClusterDefaultConfig.DefaultBrokerClass, nil } - return "", errors.New("Defaults for Broker Configurations have not been set up.") + + // If neither namespace specific nor cluster default broker class is found + return "", fmt.Errorf("Neither namespace specific nor cluster default broker class is found for namespace %q, please set them via ConfigMap config-br-defaults", ns) } diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 1cebf795be4..c85110faf74 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -40,61 +40,150 @@ func TestGetBrokerConfig(t *testing.T) { _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) defaults, err := NewDefaultsConfigFromConfigMap(example) if err != nil { - t.Error("NewDefaultsConfigFromConfigMap(example) =", err) - } - c, err := defaults.GetBrokerConfig("rando") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "somename" { - t.Error("GetBrokerConfig Failed, wanted somename, got:", c.Name) - } - c, err = defaults.GetBrokerConfig("some-namespace") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "someothername" { - t.Error("GetBrokerConfig Failed, wanted someothername, got:", c.Name) + t.Fatal("NewDefaultsConfigFromConfigMap(example) failed:", err) } - // Nil and empty tests - var nilDefaults *Defaults - _, err = nilDefaults.GetBrokerConfig("rando") - if err == nil { - t.Errorf("GetBrokerConfig did not fail with nil") - } - if err.Error() != "Defaults are nil" { - t.Error("GetBrokerConfig did not fail with nil msg, got", err) - } - emptyDefaults := Defaults{} - _, err = emptyDefaults.GetBrokerConfig("rando") - if err == nil { - t.Errorf("GetBrokerConfig did not fail with empty") + tests := []struct { + name string + namespace string + brokerClassName *string + wantConfigName string + wantErrMessage string + }{ + { + name: "Cluster default without broker class name", + namespace: "rando", + wantConfigName: "config-default-cluster-class", + }, + { + name: "Cluster default with given broker class name", + namespace: "rando", + brokerClassName: stringPtr("cluster-class-2"), + wantConfigName: "config-cluster-class-2", + }, + { + name: "Namespace default without broker class name", + namespace: "namespace-1", + wantConfigName: "config-namespace-1-class", + }, + { + name: "Namespace default with given broker class name: namespace-1-class-2", + namespace: "namespace-1", + brokerClassName: stringPtr("namespace-1-class-2"), + wantConfigName: "config-namespace-1-class-2", + }, + { + name: "Namespace default with given broker class name that doesn't have config but cluster does have", + namespace: "namespace-1", + brokerClassName: stringPtr("cluster-class-2"), + wantConfigName: "config-cluster-class-2", + }, + { + name: "Namespace default, specify a broker class name that is cluster's default", + namespace: "namespace-1", + brokerClassName: stringPtr("default-cluster-class"), + wantConfigName: "config-default-cluster-class", + }, + { + name: "Shared config in namespace overrides cluster", + namespace: "namespace-1", + brokerClassName: stringPtr("shared-class"), + wantConfigName: "config-shared-class-in-namespace-1", + }, + { + name: "Namespace default with non-existent broker class name", + namespace: "namespace-1", + brokerClassName: stringPtr("cluster-class-3"), + wantConfigName: "config-default-cluster-class", + }, + { + name: "Namespace without specifying broker class name, default config not set", + namespace: "namespace-2", + wantConfigName: "config-default-cluster-class", + }, + { + name: "Namespace without specifying broker class name, nothing set for namespace", + namespace: "namespace-3", + wantConfigName: "config-default-cluster-class", + }, + { + name: "Nil Defaults", + wantErrMessage: "Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.", + }, + { + name: "Empty Defaults", + namespace: "rando", + wantErrMessage: "Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.", + }, } - if err.Error() != "Defaults for Broker Configurations have not been set up." { - t.Error("GetBrokerConfig did not fail with non-setup msg, got", err) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var c *BrokerConfig + var err error + + // Special handling for the nil and empty defaults test cases + if tt.wantErrMessage != "" { + if tt.wantErrMessage == "Defaults are nil" { + var nilDefaults *Defaults + c, err = nilDefaults.GetBrokerConfig(tt.namespace, tt.brokerClassName) + } else { + emptyDefaults := Defaults{} + c, err = emptyDefaults.GetBrokerConfig(tt.namespace, tt.brokerClassName) + } + } else { + c, err = defaults.GetBrokerConfig(tt.namespace, tt.brokerClassName) + } + + // Check for expected errors + if tt.wantErrMessage != "" { + if err == nil || err.Error() != tt.wantErrMessage { + t.Errorf("Expected error %q, got %q", tt.wantErrMessage, err) + } + return + } + + if err != nil { + t.Errorf("Unexpected error: %v", err) + return + } + + if c.Name != tt.wantConfigName { + t.Errorf("Got config name %q, want %q", c.Name, tt.wantConfigName) + } + }) } } +func stringPtr(s string) *string { + return &s +} + func TestGetBrokerClass(t *testing.T) { _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) defaults, err := NewDefaultsConfigFromConfigMap(example) if err != nil { t.Error("NewDefaultsConfigFromConfigMap(example) =", err) } + + // Test cluster default, the given namespace doesn't have default broker class + // Will use the cluster default broker class + // The class should be clusterbrokerclass c, err := defaults.GetBrokerClass("rando") if err != nil { t.Error("GetBrokerClass Failed =", err) } - if c != "MTChannelBasedBroker" { - t.Error("GetBrokerClass Failed, wanted MTChannelBasedBroker, got:", c) + if c != "default-cluster-class" { + t.Error("GetBrokerClass Failed, wanted default-cluster-class, got:", c) } - c, err = defaults.GetBrokerClass("some-namespace") + + // Test namespace default, the given namespace has default broker class + c, err = defaults.GetBrokerClass("namespace-1") if err != nil { t.Error("GetBrokerClass Failed =", err) } - if c != "someotherbrokerclass" { - t.Error("GetBrokerClass Failed, wanted someothername, got:", c) + if c != "namespace-1-class" { + t.Error("GetBrokerClass Failed, wanted namespace-1-class, got:", c) } // Nil and empty tests @@ -103,15 +192,17 @@ func TestGetBrokerClass(t *testing.T) { if err == nil { t.Errorf("GetBrokerClass did not fail with nil") } - if err.Error() != "Defaults are nil" { - t.Error("GetBrokerClass did not fail with nil msg, got", err) + if err.Error() != "Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults." { + t.Error("GetBrokerClass did not fail with nil msg, got", err.Error()) } + emptyDefaults := Defaults{} _, err = emptyDefaults.GetBrokerClass("rando") if err == nil { t.Errorf("GetBrokerClass did not fail with empty") } - if err.Error() != "Defaults for Broker Configurations have not been set up." { + // if error contains Neither namespace specific nor cluster default broker class is found + if err.Error() != "Neither namespace specific nor cluster default broker class is found for namespace \"rando\", please set them via ConfigMap config-br-defaults" { t.Error("GetBrokerClass did not fail with non-setup msg, got", err) } } @@ -142,7 +233,7 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - broken YAML + broken YAML `, }, }, @@ -150,9 +241,9 @@ func TestDefaultsConfiguration(t *testing.T) { name: "all specified values", wantErr: false, wantDefaults: &Defaults{ - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*DefaultConfig{ "some-namespace": { - BrokerClass: "somenamespaceclass", + DefaultBrokerClass: "somenamespaceclass", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -162,9 +253,19 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somensbrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernsname", + Namespace: "somenamespace", + }, + }, + }, }, "some-namespace-too": { - BrokerClass: "somenamespaceclasstoo", + DefaultBrokerClass: "somenamespaceclasstoo", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -176,8 +277,8 @@ func TestDefaultsConfiguration(t *testing.T) { }, }, }, - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "clusterbrokerclass", + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ Kind: "ConfigMap", @@ -187,8 +288,20 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "someothernamespace", + Name: "someothername", + }, + Delivery: nil, + }, + }, }, }, + config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), @@ -196,35 +309,108 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - clusterDefault: - brokerClass: clusterbrokerclass +clusterDefault: + brokerClass: clusterbrokerclass + apiVersion: v1 + kind: ConfigMap + name: somename + namespace: knative-eventing + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace +namespaceDefaults: + some-namespace: + brokerClass: somenamespaceclass + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + brokerClasses: + somensbrokerclass: apiVersion: v1 kind: ConfigMap - name: somename - namespace: knative-eventing - namespaceDefaults: - some-namespace: - brokerClass: somenamespaceclass - apiVersion: v1 - kind: ConfigMap - name: someothername - namespace: someothernamespace - some-namespace-too: - brokerClass: somenamespaceclasstoo - apiVersion: v1 - kind: ConfigMap - name: someothernametoo - namespace: someothernamespacetoo + name: someothernsname + namespace: somenamespace + some-namespace-too: + brokerClass: somenamespaceclasstoo + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo `, }, }, }, { - name: "only clusterdefault specified values", + name: "all specified values with multiple brokerClasses", wantErr: false, wantDefaults: &Defaults{ - // NamespaceDefaultsConfig: map[string]*duckv1.KReference{}, - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "clusterbrokerclass", + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + DefaultBrokerClass: "somenamespaceclass", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + Delivery: nil, + }, + BrokerClasses: map[string]*BrokerConfig{ + "somensbrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernsname", + Namespace: "somenamespace", + }, + }, + "somebrokerclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, + }, + "some-namespace-too": { + DefaultBrokerClass: "somenamespaceclasstoo", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernametoo", + Namespace: "someothernamespacetoo", + }, + Delivery: nil, + }, + BrokerClasses: map[string]*BrokerConfig{ + "somensbrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernsname", + Namespace: "somenamespace", + }, + }, + "somebrokerclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, + }, + }, + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ Kind: "ConfigMap", @@ -234,8 +420,28 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "someothernamespace", + Name: "someothername", + }, + Delivery: nil, + }, + "somebrokerclass2": { + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "someothernamespace", + Name: "someothername", + }, + }, + }, }, }, + config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), @@ -243,12 +449,196 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - clusterDefault: - brokerClass: clusterbrokerclass +clusterDefault: + brokerClass: clusterbrokerclass + apiVersion: v1 + kind: ConfigMap + name: somename + namespace: knative-eventing + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + somebrokerclass2: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace +namespaceDefaults: + some-namespace: + brokerClass: somenamespaceclass + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + brokerClasses: + somensbrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothernsname + namespace: somenamespace + somebrokerclass2: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + brokerClass: somenamespaceclasstoo + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo + brokerClasses: + somensbrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothernsname + namespace: somenamespace + somebrokerclass2: apiVersion: v1 kind: ConfigMap - name: somename - namespace: knative-eventing + name: someothername + namespace: someothernamespace +`, + }, + }, + }, { + name: "only clusterdefault specified values without brokerClasses", + wantErr: false, + wantDefaults: &Defaults{ + // NamespaceDefaultsConfig: map[string]*duckv1.KReference{}, + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "knative-eventing", + Name: "somename", + }, + Delivery: nil, + }, + }, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` +clusterDefault: + brokerClass: clusterbrokerclass + apiVersion: v1 + kind: ConfigMap + name: somename + namespace: knative-eventing +`, + }, + }, + }, { + name: "only clusterdefault specified values", + wantErr: false, + wantDefaults: &Defaults{ + // NamespaceDefaultsConfig: map[string]*duckv1.KReference{}, + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "knative-eventing", + Name: "somename", + }, + Delivery: nil, + }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "someothernamespace", + Name: "someothername", + }, + }, + }, + }, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` +clusterDefault: + brokerClass: clusterbrokerclass + apiVersion: v1 + kind: ConfigMap + name: somename + namespace: knative-eventing + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace +`, + }, + }, + }, { + name: "only namespace defaults without brokerClasses", + wantErr: false, + wantDefaults: &Defaults{ + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + DefaultBrokerClass: "brokerclassnamespace", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + Delivery: nil, + }, + }, + "some-namespace-too": { + DefaultBrokerClass: "brokerclassnamespacetoo", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernametoo", + Namespace: "someothernamespacetoo", + }, + Delivery: nil, + }, + }, + }, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` + namespaceDefaults: + some-namespace: + brokerClass: brokerclassnamespace + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + brokerClass: brokerclassnamespacetoo + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo `, }, }, @@ -256,9 +646,9 @@ func TestDefaultsConfiguration(t *testing.T) { name: "only namespace defaults", wantErr: false, wantDefaults: &Defaults{ - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*DefaultConfig{ "some-namespace": { - BrokerClass: "brokerclassnamespace", + DefaultBrokerClass: "brokerclassnamespace", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -268,9 +658,19 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, }, "some-namespace-too": { - BrokerClass: "brokerclassnamespacetoo", + DefaultBrokerClass: "brokerclassnamespacetoo", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -290,19 +690,25 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - namespaceDefaults: - some-namespace: - brokerClass: brokerclassnamespace - apiVersion: v1 - kind: ConfigMap - name: someothername - namespace: someothernamespace - some-namespace-too: - brokerClass: brokerclassnamespacetoo - apiVersion: v1 - kind: ConfigMap - name: someothernametoo - namespace: someothernamespacetoo +namespaceDefaults: + some-namespace: + brokerClass: brokerclassnamespace + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + brokerClass: brokerclassnamespacetoo + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo `, }, }, @@ -310,10 +716,97 @@ func TestDefaultsConfiguration(t *testing.T) { name: "only namespace config default, cluster brokerclass", wantErr: false, wantDefaults: &Defaults{ - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "clusterbrokerclass", + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", + }, + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + Delivery: nil, + }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, + }, + "some-namespace-too": { + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernametoo", + Namespace: "someothernamespacetoo", + }, + Delivery: nil, + }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, + }, + }, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` +clusterDefault: + brokerClass: clusterbrokerclass +namespaceDefaults: + some-namespace: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace +`, + }, + }, + }, { + name: "only namespace config default, cluster brokerclass, with multiple brokerClasses", + wantErr: false, + wantDefaults: &Defaults{ + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", }, - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*DefaultConfig{ "some-namespace": { BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ @@ -324,6 +817,24 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + "somebrokerclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, }, "some-namespace-too": { BrokerConfig: &BrokerConfig{ @@ -335,6 +846,24 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + "somebrokerclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, }, }, }, @@ -345,32 +874,54 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - clusterDefault: - brokerClass: clusterbrokerclass - namespaceDefaults: - some-namespace: - apiVersion: v1 - kind: ConfigMap - name: someothername - namespace: someothernamespace - some-namespace-too: - apiVersion: v1 - kind: ConfigMap - name: someothernametoo - namespace: someothernamespacetoo +clusterDefault: + brokerClass: clusterbrokerclass +namespaceDefaults: + some-namespace: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + somebrokerclass2: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + somebrokerclass2: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace `, }, }, }, { - name: "one namespace config default, namespace config default with class, cluster brokerclass", + name: "one namespace config default, namespace config default with class, cluster brokerclass, without brokerClasses", wantErr: false, wantDefaults: &Defaults{ - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "clusterbrokerclass", + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", }, - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*DefaultConfig{ "some-namespace": { - BrokerClass: "namespacebrokerclass", + DefaultBrokerClass: "namespacebrokerclass", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -401,24 +952,105 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - clusterDefault: - brokerClass: clusterbrokerclass - namespaceDefaults: - some-namespace: - brokerClass: namespacebrokerclass - apiVersion: v1 - kind: ConfigMap - name: someothername - namespace: someothernamespace - some-namespace-too: - apiVersion: v1 - kind: ConfigMap - name: someothernametoo - namespace: someothernamespacetoo + clusterDefault: + brokerClass: clusterbrokerclass + namespaceDefaults: + some-namespace: + brokerClass: namespacebrokerclass + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo `, }, }, - }} + }, { + name: "complex broken YAML structure", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` + clusterDefault: + brokerClass: clusterbrokerclass + apiVersion: v1 + kind: ConfigMap + name: somename + namespace: knative-eventing + - invalid yaml + namespaceDefaults: + some-namespace: + brokerClass: somenamespaceclass`, + }, + }, + }, { + // FIXME: Wondering that how should we handle the empty string in critical fields + name: "empty strings in critical fields", + wantErr: false, + wantDefaults: &Defaults{ + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "", + }, + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + DefaultBrokerClass: "", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "", + Namespace: "", + }, + Delivery: nil, + }, + }, + "some-namespace-too": { + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "", + Namespace: "", + }, + Delivery: nil, + }, + }, + }, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` + clusterDefault: + brokerClass: + namespaceDefaults: + some-namespace: + brokerClass: + apiVersion: v1 + kind: ConfigMap + name: + namespace: + some-namespace-too: + apiVersion: v1 + kind: ConfigMap + name: + namespace: +`, + }, + }, + }, + } for _, tt := range configTests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/apis/config/testdata/config-br-defaults.yaml b/pkg/apis/config/testdata/config-br-defaults.yaml index 1e62c0fe5c3..8a452f79b0c 100644 --- a/pkg/apis/config/testdata/config-br-defaults.yaml +++ b/pkg/apis/config/testdata/config-br-defaults.yaml @@ -28,10 +28,10 @@ data: ################################ default-br-config: | clusterDefault: - brokerClass: MTChannelBasedBroker + brokerClass: default-cluster-class apiVersion: v1 kind: ConfigMap - name: somename + name: config-default-cluster-class namespace: knative-eventing delivery: retry: 3 @@ -39,18 +39,53 @@ data: ref: apiVersion: serving.knative.dev/v1 kind: Service - name: handle-error + name: mt-handle-error namespace: knative-eventing backoffPolicy: exponential backoffDelay: 3s + brokerClasses: + cluster-class-2: + delivery: + retry: 3 + deadLetterSink: + ref: + apiVersion: serving.knative.dev/v1 + kind: Service + name: mt-handle-error + namespace: knative-eventing + backoffPolicy: exponential + backoffDelay: 3s + + apiVersion: v1 + kind: ConfigMap + name: config-cluster-class-2 + namespace: knative-eventing + + shared-class: + delivery: + retry: 3 + deadLetterSink: + ref: + apiVersion: serving.knative.dev/v1 + kind: Service + name: kafka-handle-error + namespace: knative-eventing + backoffPolicy: exponential + backoffDelay: 3s + + apiVersion: v1 + kind: ConfigMap + name: config-shared-class + namespace: knative-eventing + namespaceDefaults: - some-namespace: - brokerClass: someotherbrokerclass + namespace-1: + brokerClass: namespace-1-class apiVersion: v1 kind: ConfigMap - name: someothername - namespace: someothernamespace + name: config-namespace-1-class + namespace: namespace-1 delivery: retry: 5 deadLetterSink: @@ -58,6 +93,46 @@ data: apiVersion: serving.knative.dev/v1 kind: Service name: someother-handle-error - namespace: someothernamespace + namespace: knative-eventing backoffPolicy: linear backoffDelay: 5s + brokerClasses: + namespace-1-class-2: + delivery: + retry: 3 + deadLetterSink: + ref: + apiVersion: serving.knative.dev/v1 + kind: Service + name: mt-handle-error + namespace: knative-eventing + backoffPolicy: exponential + backoffDelay: 3s + + apiVersion: v1 + kind: ConfigMap + name: config-namespace-1-class-2 + namespace: knative-eventing + + shared-class: + delivery: + retry: 3 + deadLetterSink: + ref: + apiVersion: serving.knative.dev/v1 + kind: Service + name: kafka-handle-error + namespace: knative-eventing + backoffPolicy: exponential + backoffDelay: 3s + + apiVersion: v1 + kind: ConfigMap + name: config-shared-class-in-namespace-1 + namespace: knative-eventing + + namespace-2: + brokerClass: default-namespace-2-class + + + namespace-3: diff --git a/pkg/apis/config/zz_generated.deepcopy.go b/pkg/apis/config/zz_generated.deepcopy.go index e02665cc874..753fea32bea 100644 --- a/pkg/apis/config/zz_generated.deepcopy.go +++ b/pkg/apis/config/zz_generated.deepcopy.go @@ -53,13 +53,28 @@ func (in *BrokerConfig) DeepCopy() *BrokerConfig { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ClassAndBrokerConfig) DeepCopyInto(out *ClassAndBrokerConfig) { +func (in *DefaultConfig) DeepCopyInto(out *DefaultConfig) { *out = *in if in.BrokerConfig != nil { in, out := &in.BrokerConfig, &out.BrokerConfig *out = new(BrokerConfig) (*in).DeepCopyInto(*out) } + if in.BrokerClasses != nil { + in, out := &in.BrokerClasses, &out.BrokerClasses + *out = make(map[string]*BrokerConfig, len(*in)) + for key, val := range *in { + var outVal *BrokerConfig + if val == nil { + (*out)[key] = nil + } else { + in, out := &val, &outVal + *out = new(BrokerConfig) + (*in).DeepCopyInto(*out) + } + (*out)[key] = outVal + } + } if in.DisallowDifferentNamespaceConfig != nil { in, out := &in.DisallowDifferentNamespaceConfig, &out.DisallowDifferentNamespaceConfig *out = new(bool) @@ -68,12 +83,12 @@ func (in *ClassAndBrokerConfig) DeepCopyInto(out *ClassAndBrokerConfig) { return } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClassAndBrokerConfig. -func (in *ClassAndBrokerConfig) DeepCopy() *ClassAndBrokerConfig { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DefaultConfig. +func (in *DefaultConfig) DeepCopy() *DefaultConfig { if in == nil { return nil } - out := new(ClassAndBrokerConfig) + out := new(DefaultConfig) in.DeepCopyInto(out) return out } @@ -83,22 +98,22 @@ func (in *Defaults) DeepCopyInto(out *Defaults) { *out = *in if in.NamespaceDefaultsConfig != nil { in, out := &in.NamespaceDefaultsConfig, &out.NamespaceDefaultsConfig - *out = make(map[string]*ClassAndBrokerConfig, len(*in)) + *out = make(map[string]*DefaultConfig, len(*in)) for key, val := range *in { - var outVal *ClassAndBrokerConfig + var outVal *DefaultConfig if val == nil { (*out)[key] = nil } else { in, out := &val, &outVal - *out = new(ClassAndBrokerConfig) + *out = new(DefaultConfig) (*in).DeepCopyInto(*out) } (*out)[key] = outVal } } - if in.ClusterDefault != nil { - in, out := &in.ClusterDefault, &out.ClusterDefault - *out = new(ClassAndBrokerConfig) + if in.ClusterDefaultConfig != nil { + in, out := &in.ClusterDefaultConfig, &out.ClusterDefaultConfig + *out = new(DefaultConfig) (*in).DeepCopyInto(*out) } return diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index f4fc1550d5f..8087313778a 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -30,13 +30,18 @@ import ( func (b *Broker) SetDefaults(ctx context.Context) { // Default Spec fields. withNS := apis.WithinParent(ctx, b.ObjectMeta) - b.Spec.SetDefaults(withNS) + b.Spec.SetDefaults(withNS, b.Annotations["eventing.knative.dev/broker.class"]) eventing.DefaultBrokerClassIfUnset(withNS, &b.ObjectMeta) } -func (bs *BrokerSpec) SetDefaults(ctx context.Context) { +func (bs *BrokerSpec) SetDefaults(ctx context.Context, brokerClass string) { cfg := config.FromContextOrDefaults(ctx) - c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace) + c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, &brokerClass) + + if bs.Config != nil { + c, err = cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, &bs.Config.Kind) + } + if err == nil { if bs.Config == nil { bs.Config = c.KReference @@ -49,10 +54,14 @@ func (bs *BrokerSpec) SetDefaults(ctx context.Context) { BackoffDelay: c.Delivery.BackoffDelay, } } + } + // Default the namespace if not given if bs.Config != nil { bs.Config.SetDefaults(ctx) } + bs.Delivery.SetDefaults(ctx) + } diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index bf56dc6fe21..b87854abe78 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -38,8 +38,9 @@ var ( Defaults: &config.Defaults{ // NamespaceDefaultsConfig are the default Broker Configs for each namespace. // Namespace is the key, the value is the KReference to the config. - NamespaceDefaultsConfig: map[string]*config.ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*config.DefaultConfig{ "mynamespace": { + DefaultBrokerClass: "MTChannelBasedBroker", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -61,9 +62,27 @@ var ( BackoffDelay: pointer.String("5s"), }, }, + BrokerClasses: map[string]*config.BrokerConfig{ + "mynamespaceclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "kafka-channel", + }, + }, + "mynamespaceclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "kafka-channel", + }, + }, + }, }, "mynamespace2": { - BrokerClass: "mynamespace2class", + DefaultBrokerClass: "mynamespace2class", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -85,9 +104,19 @@ var ( BackoffDelay: pointer.String("3s"), }, }, + BrokerClasses: map[string]*config.BrokerConfig{ + "mynamespaceclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "natss-channel", + }, + }, + }, }, "mynamespace3": { - BrokerClass: "mynamespace3class", + DefaultBrokerClass: "mynamespace3class", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -109,9 +138,67 @@ var ( }, }, }, + "customns": { + DefaultBrokerClass: "MTChannelBasedBroker", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "test-ns", + Name: "test-config", + }, + }, + BrokerClasses: map[string]*config.BrokerConfig{ + "mynamespaceclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "kafka-channel", + }, + }, + "mynamespaceclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "kafka-channel", + }, + }, + }, + }, + "custom": { + DefaultBrokerClass: "test-broker", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "test-ns", + Name: "test-config", + }, + }, + BrokerClasses: map[string]*config.BrokerConfig{ + "mynamespaceclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "kafka-channel", + }, + }, + "mynamespaceclass2": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "kafka-channel", + }, + }, + }, + }, }, - ClusterDefault: &config.ClassAndBrokerConfig{ - BrokerClass: eventing.MTChannelBrokerClassValue, + ClusterDefaultConfig: &config.DefaultConfig{ + DefaultBrokerClass: eventing.MTChannelBrokerClassValue, BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -133,6 +220,24 @@ var ( BackoffDelay: pointer.String("5s"), }, }, + BrokerClasses: map[string]*config.BrokerConfig{ + "clusterBrokerClass1": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-test", + }, + }, + "MTChannelBasedBroker": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-in-broker-classes", + }, + }, + }, }, }, } @@ -408,7 +513,7 @@ func TestBrokerSetDefaults(t *testing.T) { }, "missing deadLetterSink.ref.namespace, defaulted": { initial: Broker{ - ObjectMeta: metav1.ObjectMeta{Name: "broker", Namespace: "custom"}, + ObjectMeta: metav1.ObjectMeta{Name: "broker", Namespace: "customns"}, Spec: BrokerSpec{ Config: &duckv1.KReference{ Kind: "ConfigMap", @@ -433,7 +538,7 @@ func TestBrokerSetDefaults(t *testing.T) { expected: Broker{ ObjectMeta: metav1.ObjectMeta{ Name: "broker", - Namespace: "custom", + Namespace: "customns", Annotations: map[string]string{ eventing.BrokerClassKey: "MTChannelBasedBroker", }, @@ -449,7 +554,7 @@ func TestBrokerSetDefaults(t *testing.T) { DeadLetterSink: &duckv1.Destination{ Ref: &duckv1.KReference{ Kind: "Service", - Namespace: "custom", + Namespace: "customns", Name: "handle-error", APIVersion: "serving.knative.dev/v1", }, @@ -461,6 +566,78 @@ func TestBrokerSetDefaults(t *testing.T) { }, }, }, + "try to use the config from the brokerclasses": { + initial: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + eventing.BrokerClassKey: "clusterBrokerClass1", + }, + }, + }, + expected: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "", + Annotations: map[string]string{ + eventing.BrokerClassKey: "clusterBrokerClass1", + }, + }, + Spec: BrokerSpec{ + Config: &duckv1.KReference{ + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-test", + APIVersion: "v1", + }, + }, + }, + }, + "if targeted broker class has config exist in both default config and broker classes ": { + initial: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + eventing.BrokerClassKey: "MTChannelBasedBroker", + }, + }, + }, + expected: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "", + Annotations: map[string]string{ + eventing.BrokerClassKey: "MTChannelBasedBroker", + }, + }, + Spec: BrokerSpec{ + Config: &duckv1.KReference{ + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-in-broker-classes", + APIVersion: "v1", + }, + }, + }, + }, + "test and validate namespace broker config": { + initial: Broker{ + ObjectMeta: metav1.ObjectMeta{Name: "broker", Namespace: "custom"}, + }, + expected: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Name: "broker", + Namespace: "custom", + Annotations: map[string]string{ + eventing.BrokerClassKey: "test-broker", + }, + }, + Spec: BrokerSpec{ + Config: &duckv1.KReference{ + Kind: "ConfigMap", + Namespace: "test-ns", + Name: "test-config", + APIVersion: "v1", + }, + }, + }, + }, } for n, tc := range testCases { t.Run(n, func(t *testing.T) { diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index 51e30e10c1a..345d2714d03 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -33,36 +33,48 @@ const ( func (b *Broker) Validate(ctx context.Context) *apis.FieldError { ctx = apis.WithinParent(ctx, b.ObjectMeta) - cfg := config.FromContextOrDefaults(ctx) - var brConfig *config.ClassAndBrokerConfig - if cfg.Defaults != nil { - if c, ok := cfg.Defaults.NamespaceDefaultsConfig[b.GetNamespace()]; ok { - brConfig = c - } else { - brConfig = cfg.Defaults.ClusterDefault - } - } - withNS := ctx - if brConfig == nil || brConfig.DisallowDifferentNamespaceConfig == nil || !*brConfig.DisallowDifferentNamespaceConfig { - withNS = apis.AllowDifferentNamespace(ctx) - } + var errs *apis.FieldError + withNS := determineNamespaceAllowance(ctx, cfg.Defaults) // Make sure a BrokerClassAnnotation exists - var errs *apis.FieldError if bc, ok := b.GetAnnotations()[BrokerClassAnnotationKey]; !ok || bc == "" { errs = errs.Also(apis.ErrMissingField(BrokerClassAnnotationKey)) } + // Further validation logic errs = errs.Also(b.Spec.Validate(withNS).ViaField("spec")) if apis.IsInUpdate(ctx) { original := apis.GetBaseline(ctx).(*Broker) errs = errs.Also(b.CheckImmutableFields(ctx, original)) } + return errs } +// Determine if the namespace allowance based on the given configuration +func determineNamespaceAllowance(ctx context.Context, brConfig *config.Defaults) context.Context { + + // If there is no configurations set, allow different namespace by default + if brConfig == nil || (brConfig.NamespaceDefaultsConfig == nil && brConfig.ClusterDefaultConfig == nil) { + return apis.AllowDifferentNamespace(ctx) + } + namespace := apis.ParentMeta(ctx).Namespace + nsConfig := brConfig.NamespaceDefaultsConfig[namespace] + + // Check if the namespace disallows different namespace first + if nsConfig == nil || nsConfig.DisallowDifferentNamespaceConfig == nil || !*nsConfig.DisallowDifferentNamespaceConfig { + // If there is no namespace specific configuration or DisallowDifferentNamespaceConfig is false, check the cluster level configuration + if brConfig.ClusterDefaultConfig == nil || brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig == nil || !*brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig { + // If there is no cluster level configuration or DisallowDifferentNamespaceConfig in cluster level is false, allow different namespace + return apis.AllowDifferentNamespace(ctx) + } + } + // If we reach here, it means DisallowDifferentNamespaceConfig is true at either level, no need to explicitly disallow, just return the original context + return ctx +} + func (bs *BrokerSpec) Validate(ctx context.Context) *apis.FieldError { var errs *apis.FieldError diff --git a/pkg/apis/eventing/v1/broker_validation_test.go b/pkg/apis/eventing/v1/broker_validation_test.go index 420396a65c9..50210692cc9 100644 --- a/pkg/apis/eventing/v1/broker_validation_test.go +++ b/pkg/apis/eventing/v1/broker_validation_test.go @@ -197,7 +197,7 @@ func TestValidate(t *testing.T) { cfg: &config.Config{ Defaults: &config.Defaults{ NamespaceDefaultsConfig: nil, - ClusterDefault: &config.ClassAndBrokerConfig{ + ClusterDefaultConfig: &config.DefaultConfig{ DisallowDifferentNamespaceConfig: pointer.Bool(true), }, }, @@ -225,7 +225,7 @@ func TestValidate(t *testing.T) { name: "invalid config, cross namespace disallowed, namespace wide", cfg: &config.Config{ Defaults: &config.Defaults{ - NamespaceDefaultsConfig: map[string]*config.ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*config.DefaultConfig{ "ns1": {DisallowDifferentNamespaceConfig: pointer.Bool(true)}, }, }, @@ -253,7 +253,7 @@ func TestValidate(t *testing.T) { name: "valid config, cross namespace allowed, namespace wide", cfg: &config.Config{ Defaults: &config.Defaults{ - NamespaceDefaultsConfig: map[string]*config.ClassAndBrokerConfig{ + NamespaceDefaultsConfig: map[string]*config.DefaultConfig{ "ns1": {DisallowDifferentNamespaceConfig: pointer.Bool(true)}, }, }, diff --git a/pkg/reconciler/sugar/namespace/namespace_test.go b/pkg/reconciler/sugar/namespace/namespace_test.go index 322751795aa..3c23443ac09 100644 --- a/pkg/reconciler/sugar/namespace/namespace_test.go +++ b/pkg/reconciler/sugar/namespace/namespace_test.go @@ -29,6 +29,7 @@ import ( sugarconfig "knative.dev/eventing/pkg/apis/sugar" fakeeventingclient "knative.dev/eventing/pkg/client/injection/client/fake" "knative.dev/eventing/pkg/reconciler/sugar/resources" + duckv1 "knative.dev/pkg/apis/duck/v1" fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake" namespacereconciler "knative.dev/pkg/client/injection/kube/reconciler/core/v1/namespace" "knative.dev/pkg/configmap" @@ -69,8 +70,16 @@ func TestEnabled(t *testing.T) { // Context with DefaultConfig c := config.Config{ Defaults: &config.Defaults{ - ClusterDefault: &config.ClassAndBrokerConfig{ - BrokerClass: "AValidBrokerClass", + ClusterDefaultConfig: &config.DefaultConfig{ + DefaultBrokerClass: "AValidBrokerClass", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + Name: "default-broker-config", + Kind: "ConfigMap", + Namespace: testNS, + APIVersion: "v1", + }, + }, }, }, } diff --git a/pkg/reconciler/sugar/trigger/trigger_test.go b/pkg/reconciler/sugar/trigger/trigger_test.go index 9fb439a5d54..922e84941ae 100644 --- a/pkg/reconciler/sugar/trigger/trigger_test.go +++ b/pkg/reconciler/sugar/trigger/trigger_test.go @@ -20,6 +20,8 @@ import ( "context" "testing" + duckv1 "knative.dev/pkg/apis/duck/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -70,8 +72,16 @@ func TestEnabled(t *testing.T) { // Context with DefaultConfig c := config.Config{ Defaults: &config.Defaults{ - ClusterDefault: &config.ClassAndBrokerConfig{ - BrokerClass: "AValidBrokerClass", + ClusterDefaultConfig: &config.DefaultConfig{ + DefaultBrokerClass: "AValidBrokerClass", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + Name: "default-broker-config", + Kind: "ConfigMap", + Namespace: testNS, + APIVersion: "v1", + }, + }, }, }, }