From d7ee0ca94cd5ccabe170ff5af9f21a8f239aeeb6 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 26 Jan 2024 02:01:57 -0500 Subject: [PATCH 01/36] First draft of the default broker class code Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 122 +++++++++++++++++++++++++++++------- 1 file changed, 98 insertions(+), 24 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index f12b83ab534..9f68edf86f8 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -71,20 +71,33 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) // 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 *NamespaceDefaultsConfig `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"` - *BrokerConfig `json:",inline"` +// 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 + // Users have to specify both of them + DefaultBrokerClass string `json:"brokerClass,omitempty"` + DefaultBrokerClassConfig *BrokerConfig `json:",inline"` + + // BrokerClasses are the default broker classes' config for the whole cluster + BrokerClasses map[string]*BrokerConfig `json:"brokerClasses,omitempty"` + + DisallowDifferentNamespaceConfig *bool `json:"disallowDifferentNamespaceConfig,omitempty"` +} + +// NamespaceDefaultsConfig struct contains the default configuration for the namespace. +type NamespaceDefaultsConfig struct { + + // BrokerClasses are the default broker classes' config for the namespace + NameSpaces map[string]*DefaultConfig `json:"brokerClasses,omitempty"` DisallowDifferentNamespaceConfig *bool `json:"disallowDifferentNamespaceConfig,omitempty"` } @@ -100,33 +113,94 @@ type BrokerConfig struct { // 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) { +func (d *Defaults) GetBrokerConfig(ns string, brokerClassName string) (*BrokerConfig, error) { + if d == nil { return nil, errors.New("Defaults are nil") } - value, present := d.NamespaceDefaultsConfig[ns] - if present && value.BrokerConfig != nil { - return value.BrokerConfig, nil + + value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] + if present && value != nil { + // We have the namespace specific config + // Now check whether the brokerClassName is the default broker class for this namespace + if value.DefaultBrokerClass == brokerClassName { + // return the default broker class config for this namespace + return value.DefaultBrokerClassConfig, nil + } else { + // If the brokerClassName is not the default broker class for this namespace, check if we have the config for this brokerclass + if value.BrokerClasses != nil { + brokerConfig, present := value.BrokerClasses[brokerClassName] + if present && brokerConfig != nil { + // We have the config specifically for this broker class in this namespace, just return the config + return brokerConfig, nil + } else { + // We don't have the config specifically for this broker class in this namespace, check the default broker class config for this namespace + if value.DefaultBrokerClassConfig != nil { + // return the default broker class config for this namespace + return value.DefaultBrokerClassConfig, nil + } else { + // We don't have the default broker class config for this namespace, check if we have the cluster default config + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { + return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil + } else { + return nil, errors.New("Defaults for Broker Configurations have not been set up.") + } + } + } + + } else { + // No brokerClasses config for this namespace, check the default broker class config for this namespace + if value.DefaultBrokerClassConfig != nil { + // return the default broker class config for this namespace + return value.DefaultBrokerClassConfig, nil + } else { + // We don't have the default broker class config for this namespace, check if we have the cluster default config + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { + return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil + } else { + return nil, errors.New("Defaults for Broker Configurations have not been set up. Both default broker class config for this namespace and cluster default config are nil.") + } + } + } + } } - if d.ClusterDefault != nil && d.ClusterDefault.BrokerConfig != nil { - return d.ClusterDefault.BrokerConfig, nil + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { + return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil } return nil, errors.New("Defaults for Broker Configurations have not been set up.") + } // GetBrokerClass returns a namespace specific Broker Class, and if // that doesn't exist, return a Cluster Default and if that doesn't exist // return an error. func (d *Defaults) GetBrokerClass(ns string) (string, error) { + if d == nil { return "", errors.New("Defaults are nil") } - value, present := d.NamespaceDefaultsConfig[ns] - if present && value.BrokerClass != "" { - return value.BrokerClass, nil - } - if d.ClusterDefault != nil && d.ClusterDefault.BrokerClass != "" { - return d.ClusterDefault.BrokerClass, nil + + value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] + if present && value != nil { + // We have the namespace specific config + // Now check whether the brokerClassName is the default broker class for this namespace + if value.DefaultBrokerClass != "" { + // return the default broker class for this namespace + return value.DefaultBrokerClass, nil + } else { + // If the brokerClassName is not set for this namespace, we will check for the cluster default config + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { + return d.ClusterDefaultConfig.DefaultBrokerClass, nil + } else { + return "", errors.New("Defaults for Broker Configurations have not been set up.") + } + } + } else { + // We don't have the namespace specific config, check for the cluster default config + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { + return d.ClusterDefaultConfig.DefaultBrokerClass, nil + } else { + return "", errors.New("Defaults for Broker Configurations have not been set up.") + } } - return "", errors.New("Defaults for Broker Configurations have not been set up.") } From 662c21499392c349c3217172cca0caa2e23bd899 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 29 Jan 2024 17:00:17 -0500 Subject: [PATCH 02/36] Change the default broker class config Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 29 +- pkg/apis/config/defaults_test.go | 823 ++++++++++-------- .../config/testdata/config-br-defaults.yaml | 91 +- 3 files changed, 567 insertions(+), 376 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 9f68edf86f8..ce114c8807c 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -119,6 +119,33 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName string) (*BrokerCo return nil, errors.New("Defaults are nil") } + // Check if the brokerClassName is empty + if brokerClassName == "" { + // We don't have the brokerClassName, check if we have the namespace default config + if d.NamespaceDefaultsConfig != nil { + // We have the namespace default config, check if we have the default broker class config for this namespace + value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] + if present && value != nil { + // We have the default broker class config for this namespace, return the config + brokerClassName = value.DefaultBrokerClass + } else { + // We don't have the default broker class config for this namespace, check if we have the cluster default config + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { + brokerClassName = d.ClusterDefaultConfig.DefaultBrokerClass + } else { + return nil, errors.New("Broker class name is empty and Defaults for Broker Configurations have not been set up. Cannot proceed further.") + } + } + } else { + // We don't have the namespace default config, check if we have the cluster default config + if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { + return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil + } else { + return nil, errors.New("Defaults for Broker Configurations have not been set up.") + } + } + } + value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] if present && value != nil { // We have the namespace specific config @@ -167,7 +194,7 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName string) (*BrokerCo if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil } - return nil, errors.New("Defaults for Broker Configurations have not been set up.") + return nil, errors.New("The given broker class name cannout be found in the namespace defaults config or cluster default config.") } diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 2a527a7c02d..57059572846 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -19,12 +19,6 @@ package config import ( "testing" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - duckv1 "knative.dev/pkg/apis/duck/v1" - "knative.dev/pkg/kmp" - "knative.dev/pkg/system" - . "knative.dev/pkg/configmap/testing" _ "knative.dev/pkg/system/testing" ) @@ -42,400 +36,495 @@ func TestGetBrokerConfig(t *testing.T) { if err != nil { t.Error("NewDefaultsConfigFromConfigMap(example) =", err) } - c, err := defaults.GetBrokerConfig("rando") + + // Test cluster default, without given broker class name + // Will use cluster default broker class, as no default namespace config for ns "rando" + // The name should be config-default-cluster-class + 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) + if c.Name != "config-default-cluster-class" { + t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) } - c, err = defaults.GetBrokerConfig("some-namespace") + + // Test cluster default, with given broker class name: cluster-class-2 + // Will use the given broker class name with the correspond config from cluster brokerClasses, as no default namespace config for ns "rando" + // The name for the config should be config-cluster-class-2 + c, err = defaults.GetBrokerConfig("rando", "cluster-class-2") if err != nil { t.Error("GetBrokerConfig Failed =", err) } - if c.Name != "someothername" { - t.Error("GetBrokerConfig Failed, wanted someothername, got:", c.Name) + if c.Name != "config-cluster-class-2" { + t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) } - // Nil and empty tests - var nilDefaults *Defaults - _, err = nilDefaults.GetBrokerConfig("rando") - if err == nil { - t.Errorf("GetBrokerConfig did not fail with nil") + // Test namespace default, without given broker class name + // Will use the namespace default broker class, and the config exist for this namespace + // The config name should be config-default-namespace-1-class + c, err = defaults.GetBrokerConfig("namespace-1", "") + if err != nil { + t.Error("GetBrokerConfig Failed =", err) } - if err.Error() != "Defaults are nil" { - t.Error("GetBrokerConfig did not fail with nil msg, got", err) + if c.Name != "config-default-namespace-1-class" { + t.Error("GetBrokerConfig Failed, wanted config-default-namespace-1-class, got:", c.Name) } - emptyDefaults := Defaults{} - _, err = emptyDefaults.GetBrokerConfig("rando") - if err == nil { - t.Errorf("GetBrokerConfig did not fail with empty") + + // Test namespace default, with given broker class name: namespace-1-class-2 + // Will use the given broker class name with the correspond config from namespace brokerClasses + // The config name should be config-namespace-1-class-2 + c, err = defaults.GetBrokerConfig("namespace-1", "namespace-1-class-2") + if err != nil { + t.Error("GetBrokerConfig Failed =", err) } - if err.Error() != "Defaults for Broker Configurations have not been set up." { - t.Error("GetBrokerConfig did not fail with non-setup msg, got", err) + if c.Name != "config-namespace-1-class-2" { + t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class-2, got:", c.Name) } -} -func TestGetBrokerClass(t *testing.T) { - _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) - defaults, err := NewDefaultsConfigFromConfigMap(example) + // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses + // Will use the cluster config for this broker class. i.e will looking for the config in cluster brokerClasses + // The config name should be config-cluster-class-2 + c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-2") if err != nil { - t.Error("NewDefaultsConfigFromConfigMap(example) =", err) + t.Error("GetBrokerConfig Failed =", err) + } + if c.Name != "config-cluster-class-2" { + t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) + } + + // Test namespace default, specify a broker class name that doesn't exist in this namespace's brokerClasses, but it is cluster's default broker class + // Will use the cluster default broker class config, as no default broker class config is set for this namespace + // The config name should be config-default-cluster-class + c, err = defaults.GetBrokerConfig("namespace-1", "default-cluster-class") + if err != nil { + t.Error("GetBrokerConfig Failed =", err) + } + if c.Name != "config-default-cluster-class" { + t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) } - c, err := defaults.GetBrokerClass("rando") + + // Shared config + // Test namespace default, with given broker class name both have config in this namespace's brokerClasses and cluster brokerClasses + // Will use the given broker class name with the correspond config from namespace brokerClasses. i.e namespace will override cluster config + // The config name should be config-shared-class-in-namespace-1 + c, err = defaults.GetBrokerConfig("namespace-1", "shared-class") + if err != nil { + t.Error("GetBrokerConfig Failed =", err) + } + if c.Name != "config-shared-class-in-namespace-1" { + t.Error("GetBrokerConfig Failed, wanted config-shared-class-in-namespace-1, got:", c.Name) + } + + // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses, and also doesn't have config in cluster brokerClasses + // Will return error, as no config found for this broker class in both namespace brokerClasses and cluster brokerClasses + _, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-3") + if err == nil { + t.Error("GetBrokerConfig did not fail with no config found") + } + + // Test namespace default, without specifying broker class name, and the namespace default broker class's config is not set + // Will use the cluster default broker class config, as no default broker class config is set for this namespace + // The config name should be config-default-cluster-class + c, err = defaults.GetBrokerConfig("namespace-2", "") if err != nil { - t.Error("GetBrokerClass Failed =", err) + t.Error("GetBrokerConfig Failed =", err) } - if c != "MTChannelBasedBroker" { - t.Error("GetBrokerClass Failed, wanted MTChannelBasedBroker, got:", c) + if c.Name != "config-default-cluster-class" { + t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) } - c, err = defaults.GetBrokerClass("some-namespace") + + // Test namespace default, without specifying broker class name, and nothing for this namespace is set + // Will use the cluster default broker class config, as nothing is setted for this namespace + // The config name should be config-default-cluster-class + c, err = defaults.GetBrokerConfig("namespace-3", "") if err != nil { - t.Error("GetBrokerClass Failed =", err) + t.Error("GetBrokerConfig Failed =", err) } - if c != "someotherbrokerclass" { - t.Error("GetBrokerClass Failed, wanted someothername, got:", c) + if c.Name != "config-default-cluster-class" { + t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) } // Nil and empty tests var nilDefaults *Defaults - _, err = nilDefaults.GetBrokerClass("rando") + _, err = nilDefaults.GetBrokerConfig("rando", "") if err == nil { - t.Errorf("GetBrokerClass did not fail with nil") + t.Errorf("GetBrokerConfig did not fail with nil") } if err.Error() != "Defaults are nil" { - t.Error("GetBrokerClass did not fail with nil msg, got", err) + t.Error("GetBrokerConfig did not fail with nil msg, got", err) } + emptyDefaults := Defaults{} - _, err = emptyDefaults.GetBrokerClass("rando") + _, err = emptyDefaults.GetBrokerConfig("rando", "") if err == nil { - t.Errorf("GetBrokerClass did not fail with empty") + t.Errorf("GetBrokerConfig did not fail with empty") } + if err.Error() != "Defaults for Broker Configurations have not been set up." { - t.Error("GetBrokerClass did not fail with non-setup msg, got", err) + t.Error("GetBrokerConfig did not fail with non-setup msg, got", err) } } -func TestDefaultsConfiguration(t *testing.T) { - configTests := []struct { - name string - wantErr bool - wantDefaults interface{} - config *corev1.ConfigMap - }{{ - name: "defaults configuration", - wantErr: true, - config: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), - Name: DefaultsConfigName, - }, - Data: map[string]string{}, - }, - }, { - name: "corrupt default-br-config", - wantErr: true, - config: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace(), - Name: DefaultsConfigName, - }, - Data: map[string]string{ - "default-br-config": ` - broken YAML -`, - }, - }, - }, { - name: "all specified values", - wantErr: false, - wantDefaults: &Defaults{ - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ - "some-namespace": { - BrokerClass: "somenamespaceclass", - BrokerConfig: &BrokerConfig{ - KReference: &duckv1.KReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "someothername", - Namespace: "someothernamespace", - }, - Delivery: nil, - }, - }, - "some-namespace-too": { - BrokerClass: "somenamespaceclasstoo", - BrokerConfig: &BrokerConfig{ - KReference: &duckv1.KReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "someothernametoo", - Namespace: "someothernamespacetoo", - }, - Delivery: nil, - }, - }, - }, - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "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 - 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: "only clusterdefault specified values", - wantErr: false, - wantDefaults: &Defaults{ - // NamespaceDefaultsConfig: map[string]*duckv1.KReference{}, - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "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 namespace defaults", - wantErr: false, - wantDefaults: &Defaults{ - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ - "some-namespace": { - BrokerClass: "brokerclassnamespace", - BrokerConfig: &BrokerConfig{ - KReference: &duckv1.KReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "someothername", - Namespace: "someothernamespace", - }, - Delivery: nil, - }, - }, - "some-namespace-too": { - BrokerClass: "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 -`, - }, - }, - }, { - name: "only namespace config default, cluster brokerclass", - wantErr: false, - wantDefaults: &Defaults{ - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "clusterbrokerclass", - }, - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ - "some-namespace": { - BrokerConfig: &BrokerConfig{ - KReference: &duckv1.KReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "someothername", - Namespace: "someothernamespace", - }, - Delivery: nil, - }, - }, - "some-namespace-too": { - 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": ` - 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 -`, - }, - }, - }, { - name: "one namespace config default, namespace config default with class, cluster brokerclass", - wantErr: false, - wantDefaults: &Defaults{ - ClusterDefault: &ClassAndBrokerConfig{ - BrokerClass: "clusterbrokerclass", - }, - NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ - "some-namespace": { - BrokerClass: "namespacebrokerclass", - BrokerConfig: &BrokerConfig{ - KReference: &duckv1.KReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Name: "someothername", - Namespace: "someothernamespace", - }, - Delivery: nil, - }, - }, - "some-namespace-too": { - 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": ` - 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 -`, - }, - }, - }} - - for _, tt := range configTests { - t.Run(tt.name, func(t *testing.T) { - actualDefaults, err := NewDefaultsConfigFromConfigMap(tt.config) - - if (err != nil) != tt.wantErr { - t.Fatalf("Test: %q; NewDefaultsConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) - } - if !tt.wantErr { - diff, err := kmp.ShortDiff(tt.wantDefaults, actualDefaults) - if err != nil { - t.Fatalf("Diff failed: %s %q", err, diff) - } - if diff != "" { - t.Fatal("diff", diff) - } - } - }) - } -} +//func TestGetBrokerClass(t *testing.T) { +// _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) +// defaults, err := NewDefaultsConfigFromConfigMap(example) +// if err != nil { +// t.Error("NewDefaultsConfigFromConfigMap(example) =", err) +// } +// c, err := defaults.GetBrokerClass("rando") +// if err != nil { +// t.Error("GetBrokerClass Failed =", err) +// } +// if c != "MTChannelBasedBroker" { +// t.Error("GetBrokerClass Failed, wanted MTChannelBasedBroker, got:", c) +// } +// c, err = defaults.GetBrokerClass("some-namespace") +// if err != nil { +// t.Error("GetBrokerClass Failed =", err) +// } +// if c != "someotherbrokerclass" { +// t.Error("GetBrokerClass Failed, wanted someothername, got:", c) +// } +// +// // Nil and empty tests +// var nilDefaults *Defaults +// _, err = nilDefaults.GetBrokerClass("rando") +// 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) +// } +// 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." { +// t.Error("GetBrokerClass did not fail with non-setup msg, got", err) +// } +//} +// +//func TestDefaultsConfiguration(t *testing.T) { +// configTests := []struct { +// name string +// wantErr bool +// wantDefaults interface{} +// config *corev1.ConfigMap +// }{{ +// name: "defaults configuration", +// wantErr: true, +// config: &corev1.ConfigMap{ +// ObjectMeta: metav1.ObjectMeta{ +// Namespace: system.Namespace(), +// Name: DefaultsConfigName, +// }, +// Data: map[string]string{}, +// }, +// }, { +// name: "corrupt default-br-config", +// wantErr: true, +// config: &corev1.ConfigMap{ +// ObjectMeta: metav1.ObjectMeta{ +// Namespace: system.Namespace(), +// Name: DefaultsConfigName, +// }, +// Data: map[string]string{ +// "default-br-config": ` +// broken YAML +//`, +// }, +// }, +// }, { +// name: "all specified values", +// wantErr: false, +// wantDefaults: &Defaults{ +// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ +// "some-namespace": { +// BrokerClass: "somenamespaceclass", +// BrokerConfig: &BrokerConfig{ +// KReference: &duckv1.KReference{ +// APIVersion: "v1", +// Kind: "ConfigMap", +// Name: "someothername", +// Namespace: "someothernamespace", +// }, +// Delivery: nil, +// }, +// }, +// "some-namespace-too": { +// BrokerClass: "somenamespaceclasstoo", +// BrokerConfig: &BrokerConfig{ +// KReference: &duckv1.KReference{ +// APIVersion: "v1", +// Kind: "ConfigMap", +// Name: "someothernametoo", +// Namespace: "someothernamespacetoo", +// }, +// Delivery: nil, +// }, +// }, +// }, +// ClusterDefault: &ClassAndBrokerConfig{ +// BrokerClass: "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 +// 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: "only clusterdefault specified values", +// wantErr: false, +// wantDefaults: &Defaults{ +// // NamespaceDefaultsConfig: map[string]*duckv1.KReference{}, +// ClusterDefault: &ClassAndBrokerConfig{ +// BrokerClass: "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 namespace defaults", +// wantErr: false, +// wantDefaults: &Defaults{ +// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ +// "some-namespace": { +// BrokerClass: "brokerclassnamespace", +// BrokerConfig: &BrokerConfig{ +// KReference: &duckv1.KReference{ +// APIVersion: "v1", +// Kind: "ConfigMap", +// Name: "someothername", +// Namespace: "someothernamespace", +// }, +// Delivery: nil, +// }, +// }, +// "some-namespace-too": { +// BrokerClass: "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 +//`, +// }, +// }, +// }, { +// name: "only namespace config default, cluster brokerclass", +// wantErr: false, +// wantDefaults: &Defaults{ +// ClusterDefault: &ClassAndBrokerConfig{ +// BrokerClass: "clusterbrokerclass", +// }, +// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ +// "some-namespace": { +// BrokerConfig: &BrokerConfig{ +// KReference: &duckv1.KReference{ +// APIVersion: "v1", +// Kind: "ConfigMap", +// Name: "someothername", +// Namespace: "someothernamespace", +// }, +// Delivery: nil, +// }, +// }, +// "some-namespace-too": { +// 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": ` +// 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 +//`, +// }, +// }, +// }, { +// name: "one namespace config default, namespace config default with class, cluster brokerclass", +// wantErr: false, +// wantDefaults: &Defaults{ +// ClusterDefault: &ClassAndBrokerConfig{ +// BrokerClass: "clusterbrokerclass", +// }, +// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ +// "some-namespace": { +// BrokerClass: "namespacebrokerclass", +// BrokerConfig: &BrokerConfig{ +// KReference: &duckv1.KReference{ +// APIVersion: "v1", +// Kind: "ConfigMap", +// Name: "someothername", +// Namespace: "someothernamespace", +// }, +// Delivery: nil, +// }, +// }, +// "some-namespace-too": { +// 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": ` +// 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 +//`, +// }, +// }, +// }} +// +// for _, tt := range configTests { +// t.Run(tt.name, func(t *testing.T) { +// actualDefaults, err := NewDefaultsConfigFromConfigMap(tt.config) +// +// if (err != nil) != tt.wantErr { +// t.Fatalf("Test: %q; NewDefaultsConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) +// } +// if !tt.wantErr { +// diff, err := kmp.ShortDiff(tt.wantDefaults, actualDefaults) +// if err != nil { +// t.Fatalf("Diff failed: %s %q", err, diff) +// } +// if diff != "" { +// t.Fatal("diff", diff) +// } +// } +// }) +// } +//} diff --git a/pkg/apis/config/testdata/config-br-defaults.yaml b/pkg/apis/config/testdata/config-br-defaults.yaml index 1e62c0fe5c3..2b4b74b5b6a 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: default-namespace-1-class apiVersion: v1 kind: ConfigMap - name: someothername - namespace: someothernamespace + name: config-default-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: + default-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-default-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: \ No newline at end of file From 3c11b024e10c56a8e1f1791b362aaf7a61fce62d Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 30 Jan 2024 13:57:22 -0500 Subject: [PATCH 03/36] Run codegen Signed-off-by: Leo Li --- pkg/apis/config/zz_generated.deepcopy.go | 73 ++++++++++++++++++------ 1 file changed, 57 insertions(+), 16 deletions(-) diff --git a/pkg/apis/config/zz_generated.deepcopy.go b/pkg/apis/config/zz_generated.deepcopy.go index e02665cc874..05ef91f4845 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 + if in.DefaultBrokerClassConfig != nil { + in, out := &in.DefaultBrokerClassConfig, &out.DefaultBrokerClassConfig *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,33 +98,59 @@ 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 = new(NamespaceDefaultsConfig) + (*in).DeepCopyInto(*out) + } + if in.ClusterDefaultConfig != nil { + in, out := &in.ClusterDefaultConfig, &out.ClusterDefaultConfig + *out = new(DefaultConfig) + (*in).DeepCopyInto(*out) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. +func (in *Defaults) DeepCopy() *Defaults { + if in == nil { + return nil + } + out := new(Defaults) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *NamespaceDefaultsConfig) DeepCopyInto(out *NamespaceDefaultsConfig) { + *out = *in + if in.NameSpaces != nil { + in, out := &in.NameSpaces, &out.NameSpaces + *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) - (*in).DeepCopyInto(*out) + if in.DisallowDifferentNamespaceConfig != nil { + in, out := &in.DisallowDifferentNamespaceConfig, &out.DisallowDifferentNamespaceConfig + *out = new(bool) + **out = **in } return } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. -func (in *Defaults) DeepCopy() *Defaults { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NamespaceDefaultsConfig. +func (in *NamespaceDefaultsConfig) DeepCopy() *NamespaceDefaultsConfig { if in == nil { return nil } - out := new(Defaults) + out := new(NamespaceDefaultsConfig) in.DeepCopyInto(out) return out } From 8145609c7b080b12944bdfff750d0bfa62754671 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 30 Jan 2024 17:05:24 -0500 Subject: [PATCH 04/36] Refactor the code Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 181 ++++++++---------- pkg/apis/config/defaults_test.go | 16 +- .../config/testdata/config-br-defaults.yaml | 8 +- 3 files changed, 89 insertions(+), 116 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index ce114c8807c..2e2921e4210 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -72,7 +72,7 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) type Defaults struct { // NamespaceDefaultsConfig are the default Broker Configs for each namespace. // Namespace is the key, the value is the NamespaceDefaultConfig - NamespaceDefaultsConfig *NamespaceDefaultsConfig `json:"namespaceDefaults,omitempty"` + NamespaceDefaultsConfig map[string]*DefaultConfig `json:"namespaceDefaults,omitempty"` // ClusterDefaultConfig is the default broker config for all the namespaces that // are not in NamespaceDefaultBrokerConfigs. Different broker class could have @@ -84,8 +84,8 @@ type Defaults struct { type DefaultConfig struct { // DefaultBrokerClass and DefaultBrokerClassConfig are the default broker class for the whole cluster // Users have to specify both of them - DefaultBrokerClass string `json:"brokerClass,omitempty"` - DefaultBrokerClassConfig *BrokerConfig `json:",inline"` + DefaultBrokerClass string `json:"brokerClass,omitempty"` + *BrokerConfig `json:",inline"` // BrokerClasses are the default broker classes' config for the whole cluster BrokerClasses map[string]*BrokerConfig `json:"brokerClasses,omitempty"` @@ -94,13 +94,6 @@ type DefaultConfig struct { } // NamespaceDefaultsConfig struct contains the default configuration for the namespace. -type NamespaceDefaultsConfig struct { - - // BrokerClasses are the default broker classes' config for the namespace - NameSpaces map[string]*DefaultConfig `json:"brokerClasses,omitempty"` - - DisallowDifferentNamespaceConfig *bool `json:"disallowDifferentNamespaceConfig,omitempty"` -} // BrokerConfig contains configuration for a given namespace for broker. Allows // configuring the reference to the @@ -110,92 +103,66 @@ 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, brokerClassName string) (*BrokerConfig, error) { - if d == nil { return nil, errors.New("Defaults are nil") } - // Check if the brokerClassName is empty - if brokerClassName == "" { - // We don't have the brokerClassName, check if we have the namespace default config - if d.NamespaceDefaultsConfig != nil { - // We have the namespace default config, check if we have the default broker class config for this namespace - value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] - if present && value != nil { - // We have the default broker class config for this namespace, return the config - brokerClassName = value.DefaultBrokerClass - } else { - // We don't have the default broker class config for this namespace, check if we have the cluster default config - if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { - brokerClassName = d.ClusterDefaultConfig.DefaultBrokerClass - } else { - return nil, errors.New("Broker class name is empty and Defaults for Broker Configurations have not been set up. Cannot proceed further.") - } - } - } else { - // We don't have the namespace default config, check if we have the cluster default config - if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { - return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil - } else { - return nil, errors.New("Defaults for Broker Configurations have not been set up.") + // Early return if brokerClassName is provided and valid + if brokerClassName != "" { + return d.getBrokerConfigByClassName(ns, brokerClassName) + } + + // Handling empty brokerClassName + return d.getBrokerConfigForEmptyClassName(ns) +} + +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, should get the cluster default config + return d.getClusterDefaultBrokerConfig(brokerClassName) } + return nsConfig.BrokerConfig, nil + } + if config, ok := nsConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil } } - value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] - if present && value != nil { - // We have the namespace specific config - // Now check whether the brokerClassName is the default broker class for this namespace - if value.DefaultBrokerClass == brokerClassName { - // return the default broker class config for this namespace - return value.DefaultBrokerClassConfig, nil - } else { - // If the brokerClassName is not the default broker class for this namespace, check if we have the config for this brokerclass - if value.BrokerClasses != nil { - brokerConfig, present := value.BrokerClasses[brokerClassName] - if present && brokerConfig != nil { - // We have the config specifically for this broker class in this namespace, just return the config - return brokerConfig, nil - } else { - // We don't have the config specifically for this broker class in this namespace, check the default broker class config for this namespace - if value.DefaultBrokerClassConfig != nil { - // return the default broker class config for this namespace - return value.DefaultBrokerClassConfig, nil - } else { - // We don't have the default broker class config for this namespace, check if we have the cluster default config - if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { - return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil - } else { - return nil, errors.New("Defaults for Broker Configurations have not been set up.") - } - } - } - - } else { - // No brokerClasses config for this namespace, check the default broker class config for this namespace - if value.DefaultBrokerClassConfig != nil { - // return the default broker class config for this namespace - return value.DefaultBrokerClassConfig, nil - } else { - // We don't have the default broker class config for this namespace, check if we have the cluster default config - if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { - return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil - } else { - return nil, errors.New("Defaults for Broker Configurations have not been set up. Both default broker class config for this namespace and cluster default config are nil.") - } - } - } + // Check cluster default configuration + return d.getClusterDefaultBrokerConfig(brokerClassName) +} + +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.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClassConfig != nil { - return d.ClusterDefaultConfig.DefaultBrokerClassConfig, nil + + // Fallback to cluster default configuration + return d.getClusterDefaultBrokerConfig("") +} + +func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*BrokerConfig, error) { + if d.ClusterDefaultConfig == nil || d.ClusterDefaultConfig.BrokerConfig == nil { + return nil, errors.New("Defaults for Broker Configurations have not been set up.") + } + + if brokerClassName == "" { + return d.ClusterDefaultConfig.BrokerConfig, nil + } + + if config, ok := d.ClusterDefaultConfig.BrokerClasses[brokerClassName]; ok && config != nil { + return config, nil } - return nil, errors.New("The given broker class name cannout be found in the namespace defaults config or cluster default config.") + return d.ClusterDefaultConfig.BrokerConfig, nil } // GetBrokerClass returns a namespace specific Broker Class, and if @@ -207,27 +174,29 @@ func (d *Defaults) GetBrokerClass(ns string) (string, error) { return "", errors.New("Defaults are nil") } - value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] - if present && value != nil { - // We have the namespace specific config - // Now check whether the brokerClassName is the default broker class for this namespace - if value.DefaultBrokerClass != "" { - // return the default broker class for this namespace - return value.DefaultBrokerClass, nil - } else { - // If the brokerClassName is not set for this namespace, we will check for the cluster default config - if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { - return d.ClusterDefaultConfig.DefaultBrokerClass, nil - } else { - return "", errors.New("Defaults for Broker Configurations have not been set up.") - } - } - } else { - // We don't have the namespace specific config, check for the cluster default config - if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { - return d.ClusterDefaultConfig.DefaultBrokerClass, nil - } else { - return "", errors.New("Defaults for Broker Configurations have not been set up.") - } - } + //value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] + //if present && value != nil { + // // We have the namespace specific config + // // Now check whether the brokerClassName is the default broker class for this namespace + // if value.DefaultBrokerClass != "" { + // // return the default broker class for this namespace + // return value.DefaultBrokerClass, nil + // } else { + // // If the brokerClassName is not set for this namespace, we will check for the cluster default config + // if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { + // return d.ClusterDefaultConfig.DefaultBrokerClass, nil + // } else { + // return "", errors.New("Defaults for Broker Configurations have not been set up.") + // } + // } + //} else { + // // We don't have the namespace specific config, check for the cluster default config + // if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { + // return d.ClusterDefaultConfig.DefaultBrokerClass, nil + // } else { + // return "", errors.New("Defaults for Broker Configurations have not been set up.") + // } + //} + + return "", errors.New("The given namespace cannout be found in the namespace defaults config or cluster default config.") } diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 57059572846..feb9d813ee7 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -66,8 +66,8 @@ func TestGetBrokerConfig(t *testing.T) { if err != nil { t.Error("GetBrokerConfig Failed =", err) } - if c.Name != "config-default-namespace-1-class" { - t.Error("GetBrokerConfig Failed, wanted config-default-namespace-1-class, got:", c.Name) + if c.Name != "config-namespace-1-class" { + t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class, got:", c.Name) } // Test namespace default, with given broker class name: namespace-1-class-2 @@ -116,10 +116,14 @@ func TestGetBrokerConfig(t *testing.T) { } // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses, and also doesn't have config in cluster brokerClasses - // Will return error, as no config found for this broker class in both namespace brokerClasses and cluster brokerClasses - _, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-3") - if err == nil { - t.Error("GetBrokerConfig did not fail with no config found") + // Should return the cluster default + // The config name should be config-default-cluster-class + c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-3") + if err != nil { + t.Error("GetBrokerConfig Failed =", err) + } + if c.Name != "config-default-cluster-class" { + t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) } // Test namespace default, without specifying broker class name, and the namespace default broker class's config is not set diff --git a/pkg/apis/config/testdata/config-br-defaults.yaml b/pkg/apis/config/testdata/config-br-defaults.yaml index 2b4b74b5b6a..bf1363bf87d 100644 --- a/pkg/apis/config/testdata/config-br-defaults.yaml +++ b/pkg/apis/config/testdata/config-br-defaults.yaml @@ -81,10 +81,10 @@ data: namespaceDefaults: namespace-1: - brokerClass: default-namespace-1-class + brokerClass: namespace-1-class apiVersion: v1 kind: ConfigMap - name: config-default-namespace-1-class + name: config-namespace-1-class namespace: namespace-1 delivery: retry: 5 @@ -97,7 +97,7 @@ data: backoffPolicy: linear backoffDelay: 5s brokerClasses: - default-namespace-1-class-2: + namespace-1-class-2: delivery: retry: 3 deadLetterSink: @@ -111,7 +111,7 @@ data: apiVersion: v1 kind: ConfigMap - name: config-default-namespace-1-class-2 + name: config-namespace-1-class-2 namespace: knative-eventing shared-class: From 0f3b4a23664a788c48af5dc500dfac50ed9443a5 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 30 Jan 2024 17:05:29 -0500 Subject: [PATCH 05/36] Codegen Signed-off-by: Leo Li --- pkg/apis/config/zz_generated.deepcopy.go | 44 +++++------------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/pkg/apis/config/zz_generated.deepcopy.go b/pkg/apis/config/zz_generated.deepcopy.go index 05ef91f4845..753fea32bea 100644 --- a/pkg/apis/config/zz_generated.deepcopy.go +++ b/pkg/apis/config/zz_generated.deepcopy.go @@ -55,8 +55,8 @@ func (in *BrokerConfig) DeepCopy() *BrokerConfig { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DefaultConfig) DeepCopyInto(out *DefaultConfig) { *out = *in - if in.DefaultBrokerClassConfig != nil { - in, out := &in.DefaultBrokerClassConfig, &out.DefaultBrokerClassConfig + if in.BrokerConfig != nil { + in, out := &in.BrokerConfig, &out.BrokerConfig *out = new(BrokerConfig) (*in).DeepCopyInto(*out) } @@ -98,32 +98,6 @@ func (in *Defaults) DeepCopyInto(out *Defaults) { *out = *in if in.NamespaceDefaultsConfig != nil { in, out := &in.NamespaceDefaultsConfig, &out.NamespaceDefaultsConfig - *out = new(NamespaceDefaultsConfig) - (*in).DeepCopyInto(*out) - } - if in.ClusterDefaultConfig != nil { - in, out := &in.ClusterDefaultConfig, &out.ClusterDefaultConfig - *out = new(DefaultConfig) - (*in).DeepCopyInto(*out) - } - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. -func (in *Defaults) DeepCopy() *Defaults { - if in == nil { - return nil - } - out := new(Defaults) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *NamespaceDefaultsConfig) DeepCopyInto(out *NamespaceDefaultsConfig) { - *out = *in - if in.NameSpaces != nil { - in, out := &in.NameSpaces, &out.NameSpaces *out = make(map[string]*DefaultConfig, len(*in)) for key, val := range *in { var outVal *DefaultConfig @@ -137,20 +111,20 @@ func (in *NamespaceDefaultsConfig) DeepCopyInto(out *NamespaceDefaultsConfig) { (*out)[key] = outVal } } - if in.DisallowDifferentNamespaceConfig != nil { - in, out := &in.DisallowDifferentNamespaceConfig, &out.DisallowDifferentNamespaceConfig - *out = new(bool) - **out = **in + if in.ClusterDefaultConfig != nil { + in, out := &in.ClusterDefaultConfig, &out.ClusterDefaultConfig + *out = new(DefaultConfig) + (*in).DeepCopyInto(*out) } return } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NamespaceDefaultsConfig. -func (in *NamespaceDefaultsConfig) DeepCopy() *NamespaceDefaultsConfig { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Defaults. +func (in *Defaults) DeepCopy() *Defaults { if in == nil { return nil } - out := new(NamespaceDefaultsConfig) + out := new(Defaults) in.DeepCopyInto(out) return out } From 5f61000846a68c8f224040f7d8721dade731c56c Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 31 Jan 2024 09:36:33 -0500 Subject: [PATCH 06/36] Fix the build error and run code gen Signed-off-by: Leo Li --- pkg/apis/config/testdata/config-br-defaults.yaml | 2 +- pkg/apis/eventing/v1/broker_defaults.go | 2 +- pkg/apis/eventing/v1/broker_validation.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/apis/config/testdata/config-br-defaults.yaml b/pkg/apis/config/testdata/config-br-defaults.yaml index bf1363bf87d..8a452f79b0c 100644 --- a/pkg/apis/config/testdata/config-br-defaults.yaml +++ b/pkg/apis/config/testdata/config-br-defaults.yaml @@ -135,4 +135,4 @@ data: brokerClass: default-namespace-2-class - namespace-3: \ No newline at end of file + namespace-3: diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index e101713e79a..3ab2293a2f4 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -36,7 +36,7 @@ func (b *Broker) SetDefaults(ctx context.Context) { func (bs *BrokerSpec) SetDefaults(ctx context.Context) { cfg := config.FromContextOrDefaults(ctx) - c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace) + c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, "") if err == nil { if bs.Config == nil { bs.Config = c.KReference diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index 1829872f9ee..a96a9fbf6b9 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -35,12 +35,12 @@ func (b *Broker) Validate(ctx context.Context) *apis.FieldError { ctx = apis.WithinParent(ctx, b.ObjectMeta) cfg := config.FromContextOrDefaults(ctx) - var brConfig *config.ClassAndBrokerConfig + var brConfig *config.DefaultConfig if cfg.Defaults != nil { if c, ok := cfg.Defaults.NamespaceDefaultsConfig[b.GetNamespace()]; ok { brConfig = c } else { - brConfig = cfg.Defaults.ClusterDefault + brConfig = cfg.Defaults.ClusterDefaultConfig } } From b8a649e2e16af8145a35b7ee2bd354dd6b48fd19 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 31 Jan 2024 11:21:15 -0500 Subject: [PATCH 07/36] Fix the build error in the tests Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults_test.go | 10 +++++----- pkg/apis/eventing/v1/broker_validation_test.go | 6 +++--- pkg/reconciler/sugar/namespace/namespace_test.go | 4 ++-- pkg/reconciler/sugar/trigger/trigger_test.go | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index 6fca4327b51..6af0ee494a7 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -38,7 +38,7 @@ 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": { BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ @@ -63,7 +63,7 @@ var ( }, }, "mynamespace2": { - BrokerClass: "mynamespace2class", + DefaultBrokerClass: "mynamespace2class", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -87,7 +87,7 @@ var ( }, }, "mynamespace3": { - BrokerClass: "mynamespace3class", + DefaultBrokerClass: "mynamespace3class", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", @@ -110,8 +110,8 @@ var ( }, }, }, - ClusterDefault: &config.ClassAndBrokerConfig{ - BrokerClass: eventing.MTChannelBrokerClassValue, + ClusterDefaultConfig: &config.DefaultConfig{ + DefaultBrokerClass: eventing.MTChannelBrokerClassValue, BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", diff --git a/pkg/apis/eventing/v1/broker_validation_test.go b/pkg/apis/eventing/v1/broker_validation_test.go index ae4dd7e25f9..1288d5295cd 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 00519766672..9ff47614428 100644 --- a/pkg/reconciler/sugar/namespace/namespace_test.go +++ b/pkg/reconciler/sugar/namespace/namespace_test.go @@ -69,8 +69,8 @@ func TestEnabled(t *testing.T) { // Context with DefaultConfig c := config.Config{ Defaults: &config.Defaults{ - ClusterDefault: &config.ClassAndBrokerConfig{ - BrokerClass: "AValidBrokerClass", + ClusterDefaultConfig: &config.DefaultConfig{ + DefaultBrokerClass: "AValidBrokerClass", }, }, } diff --git a/pkg/reconciler/sugar/trigger/trigger_test.go b/pkg/reconciler/sugar/trigger/trigger_test.go index d377e450ad1..5bc47a47137 100644 --- a/pkg/reconciler/sugar/trigger/trigger_test.go +++ b/pkg/reconciler/sugar/trigger/trigger_test.go @@ -70,8 +70,8 @@ func TestEnabled(t *testing.T) { // Context with DefaultConfig c := config.Config{ Defaults: &config.Defaults{ - ClusterDefault: &config.ClassAndBrokerConfig{ - BrokerClass: "AValidBrokerClass", + ClusterDefaultConfig: &config.DefaultConfig{ + DefaultBrokerClass: "AValidBrokerClass", }, }, } From 73eb74d78b420da096a80ff2ac761117e83b209a Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 31 Jan 2024 12:05:16 -0500 Subject: [PATCH 08/36] Fix the build error in the tests Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 40 +- pkg/apis/config/defaults_test.go | 739 ++++++++++--------- pkg/apis/eventing/v1/broker_defaults_test.go | 1 + 3 files changed, 391 insertions(+), 389 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 2e2921e4210..baab8e4cae1 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -169,34 +169,22 @@ func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*Broke // that doesn't exist, return a Cluster Default and if that doesn't exist // return an error. func (d *Defaults) GetBrokerClass(ns string) (string, error) { - if d == nil { return "", errors.New("Defaults are nil") } - //value, present := d.NamespaceDefaultsConfig.NameSpaces[ns] - //if present && value != nil { - // // We have the namespace specific config - // // Now check whether the brokerClassName is the default broker class for this namespace - // if value.DefaultBrokerClass != "" { - // // return the default broker class for this namespace - // return value.DefaultBrokerClass, nil - // } else { - // // If the brokerClassName is not set for this namespace, we will check for the cluster default config - // if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { - // return d.ClusterDefaultConfig.DefaultBrokerClass, nil - // } else { - // return "", errors.New("Defaults for Broker Configurations have not been set up.") - // } - // } - //} else { - // // We don't have the namespace specific config, check for the cluster default config - // if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" { - // return d.ClusterDefaultConfig.DefaultBrokerClass, nil - // } else { - // return "", errors.New("Defaults for Broker Configurations have not been set up.") - // } - //} - - return "", errors.New("The given namespace cannout be found in the namespace defaults config or cluster default config.") + // Check if the namespace has a specific configuration + if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil { + if nsConfig.DefaultBrokerClass != "" { + return nsConfig.DefaultBrokerClass, 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 + } + + // If neither namespace specific nor cluster default broker class is found + return "", fmt.Errorf("Defaults are nil") } diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index feb9d813ee7..66cb1eca03f 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -19,6 +19,12 @@ package config import ( "testing" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + duckv1 "knative.dev/pkg/apis/duck/v1" + "knative.dev/pkg/kmp" + "knative.dev/pkg/system" + . "knative.dev/pkg/configmap/testing" _ "knative.dev/pkg/system/testing" ) @@ -169,366 +175,373 @@ func TestGetBrokerConfig(t *testing.T) { } } -//func TestGetBrokerClass(t *testing.T) { -// _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) -// defaults, err := NewDefaultsConfigFromConfigMap(example) -// if err != nil { -// t.Error("NewDefaultsConfigFromConfigMap(example) =", err) -// } -// c, err := defaults.GetBrokerClass("rando") -// if err != nil { -// t.Error("GetBrokerClass Failed =", err) -// } -// if c != "MTChannelBasedBroker" { -// t.Error("GetBrokerClass Failed, wanted MTChannelBasedBroker, got:", c) -// } -// c, err = defaults.GetBrokerClass("some-namespace") -// if err != nil { -// t.Error("GetBrokerClass Failed =", err) -// } -// if c != "someotherbrokerclass" { -// t.Error("GetBrokerClass Failed, wanted someothername, got:", c) -// } -// -// // Nil and empty tests -// var nilDefaults *Defaults -// _, err = nilDefaults.GetBrokerClass("rando") -// 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) -// } -// 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." { -// t.Error("GetBrokerClass did not fail with non-setup msg, got", err) -// } -//} -// -//func TestDefaultsConfiguration(t *testing.T) { -// configTests := []struct { -// name string -// wantErr bool -// wantDefaults interface{} -// config *corev1.ConfigMap -// }{{ -// name: "defaults configuration", -// wantErr: true, -// config: &corev1.ConfigMap{ -// ObjectMeta: metav1.ObjectMeta{ -// Namespace: system.Namespace(), -// Name: DefaultsConfigName, -// }, -// Data: map[string]string{}, -// }, -// }, { -// name: "corrupt default-br-config", -// wantErr: true, -// config: &corev1.ConfigMap{ -// ObjectMeta: metav1.ObjectMeta{ -// Namespace: system.Namespace(), -// Name: DefaultsConfigName, -// }, -// Data: map[string]string{ -// "default-br-config": ` -// broken YAML -//`, -// }, -// }, -// }, { -// name: "all specified values", -// wantErr: false, -// wantDefaults: &Defaults{ -// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ -// "some-namespace": { -// BrokerClass: "somenamespaceclass", -// BrokerConfig: &BrokerConfig{ -// KReference: &duckv1.KReference{ -// APIVersion: "v1", -// Kind: "ConfigMap", -// Name: "someothername", -// Namespace: "someothernamespace", -// }, -// Delivery: nil, -// }, -// }, -// "some-namespace-too": { -// BrokerClass: "somenamespaceclasstoo", -// BrokerConfig: &BrokerConfig{ -// KReference: &duckv1.KReference{ -// APIVersion: "v1", -// Kind: "ConfigMap", -// Name: "someothernametoo", -// Namespace: "someothernamespacetoo", -// }, -// Delivery: nil, -// }, -// }, -// }, -// ClusterDefault: &ClassAndBrokerConfig{ -// BrokerClass: "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 -// 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: "only clusterdefault specified values", -// wantErr: false, -// wantDefaults: &Defaults{ -// // NamespaceDefaultsConfig: map[string]*duckv1.KReference{}, -// ClusterDefault: &ClassAndBrokerConfig{ -// BrokerClass: "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 namespace defaults", -// wantErr: false, -// wantDefaults: &Defaults{ -// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ -// "some-namespace": { -// BrokerClass: "brokerclassnamespace", -// BrokerConfig: &BrokerConfig{ -// KReference: &duckv1.KReference{ -// APIVersion: "v1", -// Kind: "ConfigMap", -// Name: "someothername", -// Namespace: "someothernamespace", -// }, -// Delivery: nil, -// }, -// }, -// "some-namespace-too": { -// BrokerClass: "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 -//`, -// }, -// }, -// }, { -// name: "only namespace config default, cluster brokerclass", -// wantErr: false, -// wantDefaults: &Defaults{ -// ClusterDefault: &ClassAndBrokerConfig{ -// BrokerClass: "clusterbrokerclass", -// }, -// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ -// "some-namespace": { -// BrokerConfig: &BrokerConfig{ -// KReference: &duckv1.KReference{ -// APIVersion: "v1", -// Kind: "ConfigMap", -// Name: "someothername", -// Namespace: "someothernamespace", -// }, -// Delivery: nil, -// }, -// }, -// "some-namespace-too": { -// 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": ` -// 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 -//`, -// }, -// }, -// }, { -// name: "one namespace config default, namespace config default with class, cluster brokerclass", -// wantErr: false, -// wantDefaults: &Defaults{ -// ClusterDefault: &ClassAndBrokerConfig{ -// BrokerClass: "clusterbrokerclass", -// }, -// NamespaceDefaultsConfig: map[string]*ClassAndBrokerConfig{ -// "some-namespace": { -// BrokerClass: "namespacebrokerclass", -// BrokerConfig: &BrokerConfig{ -// KReference: &duckv1.KReference{ -// APIVersion: "v1", -// Kind: "ConfigMap", -// Name: "someothername", -// Namespace: "someothernamespace", -// }, -// Delivery: nil, -// }, -// }, -// "some-namespace-too": { -// 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": ` -// 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 -//`, -// }, -// }, -// }} -// -// for _, tt := range configTests { -// t.Run(tt.name, func(t *testing.T) { -// actualDefaults, err := NewDefaultsConfigFromConfigMap(tt.config) -// -// if (err != nil) != tt.wantErr { -// t.Fatalf("Test: %q; NewDefaultsConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) -// } -// if !tt.wantErr { -// diff, err := kmp.ShortDiff(tt.wantDefaults, actualDefaults) -// if err != nil { -// t.Fatalf("Diff failed: %s %q", err, diff) -// } -// if diff != "" { -// t.Fatal("diff", diff) -// } -// } -// }) -// } -//} +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 != "default-cluster-class" { + t.Error("GetBrokerClass Failed, wanted default-cluster-class, got:", c) + } + + // 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 != "namespace-1-class" { + t.Error("GetBrokerClass Failed, wanted namespace-1-class, got:", c) + } + + // Nil and empty tests + var nilDefaults *Defaults + _, err = nilDefaults.GetBrokerClass("rando") + 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.Error()) + } + + emptyDefaults := Defaults{} + _, err = emptyDefaults.GetBrokerClass("rando") + if err == nil { + t.Errorf("GetBrokerClass did not fail with empty") + } + if err.Error() != "Defaults are nil" { + t.Error("GetBrokerClass did not fail with non-setup msg, got", err) + } +} + +func TestDefaultsConfiguration(t *testing.T) { + configTests := []struct { + name string + wantErr bool + wantDefaults interface{} + config *corev1.ConfigMap + }{{ + name: "defaults configuration", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{}, + }, + }, { + name: "corrupt default-br-config", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "default-br-config": ` + broken YAML +`, + }, + }, + }, { + name: "all specified values", + wantErr: false, + wantDefaults: &Defaults{ + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + DefaultBrokerClass: "somenamespaceclass", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + Delivery: nil, + }, + }, + "some-namespace-too": { + DefaultBrokerClass: "somenamespaceclasstoo", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothernametoo", + Namespace: "someothernamespacetoo", + }, + Delivery: nil, + }, + }, + }, + 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 + 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: "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, + }, + }, + }, + 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 namespace defaults", + 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 +`, + }, + }, + }, { + name: "only namespace config default, cluster brokerclass", + wantErr: false, + wantDefaults: &Defaults{ + 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, + }, + }, + "some-namespace-too": { + 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": ` + 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 +`, + }, + }, + }, { + name: "one namespace config default, namespace config default with class, cluster brokerclass", + wantErr: false, + wantDefaults: &Defaults{ + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", + }, + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + DefaultBrokerClass: "namespacebrokerclass", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + Delivery: nil, + }, + }, + "some-namespace-too": { + 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": ` + 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 +`, + }, + }, + }} + + for _, tt := range configTests { + t.Run(tt.name, func(t *testing.T) { + actualDefaults, err := NewDefaultsConfigFromConfigMap(tt.config) + + if (err != nil) != tt.wantErr { + t.Fatalf("Test: %q; NewDefaultsConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) + } + if !tt.wantErr { + diff, err := kmp.ShortDiff(tt.wantDefaults, actualDefaults) + if err != nil { + t.Fatalf("Diff failed: %s %q", err, diff) + } + if diff != "" { + t.Fatal("diff", diff) + } + } + }) + } +} diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index 6af0ee494a7..d8370af9909 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -40,6 +40,7 @@ var ( // Namespace is the key, the value is the KReference to the config. NamespaceDefaultsConfig: map[string]*config.DefaultConfig{ "mynamespace": { + DefaultBrokerClass: "MTChannelBasedBroker", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", From 03f8cab425070bfb7f02ef1997c831115395b927 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 31 Jan 2024 12:56:31 -0500 Subject: [PATCH 09/36] Modify the current existing test, and add more test cases Signed-off-by: Leo Li --- pkg/apis/config/defaults_test.go | 347 +++++++++++++++++++++++++++---- 1 file changed, 307 insertions(+), 40 deletions(-) diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 66cb1eca03f..98fbb52d554 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -268,6 +268,16 @@ 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": { DefaultBrokerClass: "somenamespaceclasstoo", @@ -293,8 +303,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(), @@ -302,25 +324,71 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - clusterDefault: - brokerClass: clusterbrokerclass - 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 +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: someothernsname + namespace: somenamespace + some-namespace-too: + brokerClass: somenamespaceclasstoo + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo +`, + }, + }, + }, { + 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 `, }, }, @@ -340,6 +408,16 @@ func TestDefaultsConfiguration(t *testing.T) { }, Delivery: nil, }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + Kind: "ConfigMap", + APIVersion: "v1", + Namespace: "someothernamespace", + Name: "someothername", + }, + }, + }, }, }, config: &corev1.ConfigMap{ @@ -349,17 +427,23 @@ func TestDefaultsConfiguration(t *testing.T) { }, Data: map[string]string{ "default-br-config": ` - clusterDefault: - brokerClass: clusterbrokerclass - apiVersion: v1 - kind: ConfigMap - name: somename - namespace: knative-eventing +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", + name: "only namespace defaults without brokerClasses", wantErr: false, wantDefaults: &Defaults{ NamespaceDefaultsConfig: map[string]*DefaultConfig{ @@ -409,6 +493,76 @@ func TestDefaultsConfiguration(t *testing.T) { kind: ConfigMap name: someothernametoo namespace: someothernamespacetoo +`, + }, + }, + }, { + name: "only namespace defaults", + 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, + }, + BrokerClasses: map[string]*BrokerConfig{ + "somebrokerclass": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + }, + }, + }, + "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 + brokerClasses: + somebrokerclass: + apiVersion: v1 + kind: ConfigMap + name: someothername + namespace: someothernamespace + some-namespace-too: + brokerClass: brokerclassnamespacetoo + apiVersion: v1 + kind: ConfigMap + name: someothernametoo + namespace: someothernamespacetoo `, }, }, @@ -430,6 +584,94 @@ 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": { + 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: "one namespace config default, namespace config default with class, cluster brokerclass, without brokerClasses", + wantErr: false, + wantDefaults: &Defaults{ + ClusterDefaultConfig: &DefaultConfig{ + DefaultBrokerClass: "clusterbrokerclass", + }, + NamespaceDefaultsConfig: map[string]*DefaultConfig{ + "some-namespace": { + DefaultBrokerClass: "namespacebrokerclass", + BrokerConfig: &BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "someothername", + Namespace: "someothernamespace", + }, + Delivery: nil, + }, }, "some-namespace-too": { BrokerConfig: &BrokerConfig{ @@ -455,6 +697,7 @@ func TestDefaultsConfiguration(t *testing.T) { brokerClass: clusterbrokerclass namespaceDefaults: some-namespace: + brokerClass: namespacebrokerclass apiVersion: v1 kind: ConfigMap name: someothername @@ -468,21 +711,44 @@ func TestDefaultsConfiguration(t *testing.T) { }, }, }, { - name: "one namespace config default, namespace config default with class, cluster brokerclass", + 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: "clusterbrokerclass", + DefaultBrokerClass: "", }, NamespaceDefaultsConfig: map[string]*DefaultConfig{ "some-namespace": { - DefaultBrokerClass: "namespacebrokerclass", + DefaultBrokerClass: "", BrokerConfig: &BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", Kind: "ConfigMap", - Name: "someothername", - Namespace: "someothernamespace", + Name: "", + Namespace: "", }, Delivery: nil, }, @@ -492,8 +758,8 @@ func TestDefaultsConfiguration(t *testing.T) { KReference: &duckv1.KReference{ APIVersion: "v1", Kind: "ConfigMap", - Name: "someothernametoo", - Namespace: "someothernamespacetoo", + Name: "", + Namespace: "", }, Delivery: nil, }, @@ -508,23 +774,24 @@ func TestDefaultsConfiguration(t *testing.T) { Data: map[string]string{ "default-br-config": ` clusterDefault: - brokerClass: clusterbrokerclass + brokerClass: namespaceDefaults: some-namespace: - brokerClass: namespacebrokerclass + brokerClass: apiVersion: v1 kind: ConfigMap - name: someothername - namespace: someothernamespace + name: + namespace: some-namespace-too: apiVersion: v1 kind: ConfigMap - name: someothernametoo - namespace: someothernamespacetoo + name: + namespace: `, }, }, - }} + }, + } for _, tt := range configTests { t.Run(tt.name, func(t *testing.T) { From 63954533b2b81af9ce89b0e8a2c17a08bd1c1111 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 31 Jan 2024 15:13:24 -0500 Subject: [PATCH 10/36] Modify the current existing test, and add more test cases Signed-off-by: Leo Li --- pkg/apis/config/defaults_test.go | 274 +++++++++++++++++++ pkg/apis/eventing/v1/broker_defaults_test.go | 30 ++ 2 files changed, 304 insertions(+) diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 98fbb52d554..371d4214630 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -355,6 +355,167 @@ namespaceDefaults: kind: ConfigMap name: someothernametoo namespace: someothernamespacetoo +`, + }, + }, + }, { + name: "all specified values with multiple brokerClasses", + wantErr: false, + wantDefaults: &Defaults{ + 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", + 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", + }, + Delivery: nil, + }, + "somebrokerclass2": { + 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 + 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: someothername + namespace: someothernamespace `, }, }, @@ -650,6 +811,119 @@ namespaceDefaults: 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]*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", + }, + }, + "somebrokerclass2": { + 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", + }, + }, + "somebrokerclass2": { + 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 + 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 `, }, }, diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index d8370af9909..5ded37ae07f 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -62,6 +62,24 @@ 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": { DefaultBrokerClass: "mynamespace2class", @@ -86,6 +104,16 @@ 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": { DefaultBrokerClass: "mynamespace3class", @@ -139,6 +167,8 @@ var ( } ) +/// FIXME: add the broker classes consideration into the test cases in this file + func TestBrokerSetDefaults(t *testing.T) { testCases := map[string]struct { initial Broker From 2480d40bc6f5db105aff231eee67e1d93b644b15 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 2 Feb 2024 15:16:10 -0500 Subject: [PATCH 11/36] Pass in the actual broker name instead of leaving it empty Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index 3ab2293a2f4..feb4b1cbd39 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -36,7 +36,7 @@ func (b *Broker) SetDefaults(ctx context.Context) { func (bs *BrokerSpec) SetDefaults(ctx context.Context) { cfg := config.FromContextOrDefaults(ctx) - c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, "") + c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, bs.Config.Name) if err == nil { if bs.Config == nil { bs.Config = c.KReference From a70189f7d2c47a093868117ffdcca84375bc46a6 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 2 Feb 2024 15:16:53 -0500 Subject: [PATCH 12/36] Modify the condition to check whether Different Namespace is allowed according to the default configs Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_validation.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index a96a9fbf6b9..539be9acfc4 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -35,17 +35,14 @@ func (b *Broker) Validate(ctx context.Context) *apis.FieldError { ctx = apis.WithinParent(ctx, b.ObjectMeta) cfg := config.FromContextOrDefaults(ctx) - var brConfig *config.DefaultConfig + var brConfig *config.Defaults if cfg.Defaults != nil { - if c, ok := cfg.Defaults.NamespaceDefaultsConfig[b.GetNamespace()]; ok { - brConfig = c - } else { - brConfig = cfg.Defaults.ClusterDefaultConfig - } + brConfig = cfg.Defaults } withNS := ctx - if brConfig == nil || brConfig.DisallowDifferentNamespaceConfig == nil || !*brConfig.DisallowDifferentNamespaceConfig { + if !(brConfig != nil && ((brConfig.NamespaceDefaultsConfig != nil && brConfig.NamespaceDefaultsConfig[b.GetNamespace()] != nil && *brConfig.NamespaceDefaultsConfig[b.GetNamespace()].DisallowDifferentNamespaceConfig) || (brConfig.ClusterDefaultConfig != nil && *brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig))) { + // Apply the AllowDifferentNamespace setting if different namespace configurations are allowed. withNS = apis.AllowDifferentNamespace(ctx) } From 76592bb537f72fbd99e059dd3cbcefb69b58a5ea Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 2 Feb 2024 15:53:04 -0500 Subject: [PATCH 13/36] Revert the brokerSpec.Name change. Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index feb4b1cbd39..6c50d285679 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -36,7 +36,8 @@ func (b *Broker) SetDefaults(ctx context.Context) { func (bs *BrokerSpec) SetDefaults(ctx context.Context) { cfg := config.FromContextOrDefaults(ctx) - c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, bs.Config.Name) + + c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, "") if err == nil { if bs.Config == nil { bs.Config = c.KReference From 9b5abf3bc6ad4ad2ebd116c1d5eac1fd89f5dfd4 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 5 Feb 2024 11:35:32 -0500 Subject: [PATCH 14/36] Fix the failing unit tests by adding the default config for the defaults Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 14 +++++++++----- pkg/apis/eventing/v1/broker_validation.go | 11 +++++++++-- pkg/reconciler/sugar/namespace/namespace_test.go | 9 +++++++++ pkg/reconciler/sugar/trigger/trigger_test.go | 10 ++++++++++ 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index 6c50d285679..ba5e5ccaf60 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -50,10 +50,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) + } else { + // raise error panic + panic(err) } - // 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_validation.go b/pkg/apis/eventing/v1/broker_validation.go index 539be9acfc4..573c739b693 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -41,8 +41,15 @@ func (b *Broker) Validate(ctx context.Context) *apis.FieldError { } withNS := ctx - if !(brConfig != nil && ((brConfig.NamespaceDefaultsConfig != nil && brConfig.NamespaceDefaultsConfig[b.GetNamespace()] != nil && *brConfig.NamespaceDefaultsConfig[b.GetNamespace()].DisallowDifferentNamespaceConfig) || (brConfig.ClusterDefaultConfig != nil && *brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig))) { - // Apply the AllowDifferentNamespace setting if different namespace configurations are allowed. + + // Apply the DisallowDifferentNamespaceConfig setting if different namespace configurations are disallowed. + // Do the pre-screening to avoid the cost of the AllowDifferentNamespace setting. + // if no NameSpace default or Cluster default is set, then the default is to allow different namespace, we will not check the AllowDifferentNamespace setting. + if brConfig == nil || (brConfig.NamespaceDefaultsConfig == nil && brConfig.ClusterDefaultConfig == nil) { + withNS = apis.AllowDifferentNamespace(ctx) + } else if !(brConfig.NamespaceDefaultsConfig[apis.ParentMeta(ctx).Namespace] != nil && brConfig.NamespaceDefaultsConfig[apis.ParentMeta(ctx).Namespace].DisallowDifferentNamespaceConfig != nil && *brConfig.NamespaceDefaultsConfig[apis.ParentMeta(ctx).Namespace].DisallowDifferentNamespaceConfig) { + withNS = apis.AllowDifferentNamespace(ctx) + } else if !(brConfig.ClusterDefaultConfig != nil && brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig != nil && *brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig) { withNS = apis.AllowDifferentNamespace(ctx) } diff --git a/pkg/reconciler/sugar/namespace/namespace_test.go b/pkg/reconciler/sugar/namespace/namespace_test.go index 9ff47614428..5e7043637c7 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" @@ -71,6 +72,14 @@ func TestEnabled(t *testing.T) { Defaults: &config.Defaults{ 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 5bc47a47137..d5154a015dc 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" @@ -72,6 +74,14 @@ func TestEnabled(t *testing.T) { Defaults: &config.Defaults{ ClusterDefaultConfig: &config.DefaultConfig{ DefaultBrokerClass: "AValidBrokerClass", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + Name: "default-broker-config", + Kind: "ConfigMap", + Namespace: testNS, + APIVersion: "v1", + }, + }, }, }, } From 4a2da5f67fb9bba297c42abf3ac0b96f0a791536 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 5 Feb 2024 16:35:32 -0500 Subject: [PATCH 15/36] Fix the failing unit tests Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 3 -- pkg/apis/eventing/v1/broker_validation.go | 38 ++++++++++++----------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index ba5e5ccaf60..fde1228210f 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -55,9 +55,6 @@ func (bs *BrokerSpec) SetDefaults(ctx context.Context) { bs.Config.SetDefaults(ctx) } bs.Delivery.SetDefaults(ctx) - } else { - // raise error panic - panic(err) } } diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index 573c739b693..ff9fc344918 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -33,40 +33,42 @@ const ( func (b *Broker) Validate(ctx context.Context) *apis.FieldError { ctx = apis.WithinParent(ctx, b.ObjectMeta) - cfg := config.FromContextOrDefaults(ctx) - var brConfig *config.Defaults - if cfg.Defaults != nil { - brConfig = cfg.Defaults - } - withNS := ctx - - // Apply the DisallowDifferentNamespaceConfig setting if different namespace configurations are disallowed. - // Do the pre-screening to avoid the cost of the AllowDifferentNamespace setting. - // if no NameSpace default or Cluster default is set, then the default is to allow different namespace, we will not check the AllowDifferentNamespace setting. - if brConfig == nil || (brConfig.NamespaceDefaultsConfig == nil && brConfig.ClusterDefaultConfig == nil) { - withNS = apis.AllowDifferentNamespace(ctx) - } else if !(brConfig.NamespaceDefaultsConfig[apis.ParentMeta(ctx).Namespace] != nil && brConfig.NamespaceDefaultsConfig[apis.ParentMeta(ctx).Namespace].DisallowDifferentNamespaceConfig != nil && *brConfig.NamespaceDefaultsConfig[apis.ParentMeta(ctx).Namespace].DisallowDifferentNamespaceConfig) { - withNS = apis.AllowDifferentNamespace(ctx) - } else if !(brConfig.ClusterDefaultConfig != nil && brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig != nil && *brConfig.ClusterDefaultConfig.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 brConfig == nil || (brConfig.NamespaceDefaultsConfig == nil && brConfig.ClusterDefaultConfig == nil) { + return apis.AllowDifferentNamespace(ctx) + } + namespace := apis.ParentMeta(ctx).Namespace + nsConfig := brConfig.NamespaceDefaultsConfig[namespace] + if nsConfig == nil || nsConfig.DisallowDifferentNamespaceConfig == nil || !*nsConfig.DisallowDifferentNamespaceConfig { + if brConfig.ClusterDefaultConfig == nil || brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig == nil || !*brConfig.ClusterDefaultConfig.DisallowDifferentNamespaceConfig { + 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 From d0a166081ae657a2e1a5fb8f91cf4abed6ccab63 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 7 Feb 2024 01:24:32 -0500 Subject: [PATCH 16/36] Fix the review comments Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 6 +++++- pkg/apis/eventing/v1/broker_defaults_test.go | 2 -- pkg/apis/eventing/v1/broker_validation.go | 6 ++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index fde1228210f..a629136714c 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -36,8 +36,12 @@ func (b *Broker) SetDefaults(ctx context.Context) { func (bs *BrokerSpec) SetDefaults(ctx context.Context) { cfg := config.FromContextOrDefaults(ctx) - c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, "") + + 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 diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index 5ded37ae07f..070ded3d183 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -167,8 +167,6 @@ var ( } ) -/// FIXME: add the broker classes consideration into the test cases in this file - func TestBrokerSetDefaults(t *testing.T) { testCases := map[string]struct { initial Broker diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index ff9fc344918..a453878f156 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -55,13 +55,19 @@ func (b *Broker) Validate(ctx context.Context) *apis.FieldError { // 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) } } From 28c30e60e5bf2b38ad43af446fd71303d1cb383c Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 9 Feb 2024 01:40:54 -0500 Subject: [PATCH 17/36] Update pkg/apis/config/defaults.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Christoph Stäbler --- pkg/apis/config/defaults.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index baab8e4cae1..6adc8c4878b 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -103,7 +103,7 @@ type BrokerConfig struct { Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"` } -func (d *Defaults) GetBrokerConfig(ns string, brokerClassName string) (*BrokerConfig, error) { +func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerConfig, error) { if d == nil { return nil, errors.New("Defaults are nil") } From d37c5e343a49ceefa21aa378a51137be7dfd467d Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 9 Feb 2024 02:03:03 -0500 Subject: [PATCH 18/36] Change the tests to be table form Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 4 +- pkg/apis/config/defaults_test.go | 369 ++++++++++++++++++++----------- 2 files changed, 248 insertions(+), 125 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 6adc8c4878b..7967d8b44f4 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -109,8 +109,8 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerC } // Early return if brokerClassName is provided and valid - if brokerClassName != "" { - return d.getBrokerConfigByClassName(ns, brokerClassName) + if brokerClassName != nil { + return d.getBrokerConfigByClassName(ns, *brokerClassName) } // Handling empty brokerClassName diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 371d4214630..b309ebf8f8e 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -36,143 +36,266 @@ func TestDefaultsConfigurationFromFile(t *testing.T) { } } +//func TestGetBrokerConfig(t *testing.T) { +// _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) +// defaults, err := NewDefaultsConfigFromConfigMap(example) +// if err != nil { +// t.Error("NewDefaultsConfigFromConfigMap(example) =", err) +// } +// +// // Test cluster default, without given broker class name +// // Will use cluster default broker class, as no default namespace config for ns "rando" +// // The name should be config-default-cluster-class +// c, err := defaults.GetBrokerConfig("rando", nil) +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-default-cluster-class" { +// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) +// } +// +// // Test cluster default, with given broker class name: cluster-class-2 +// // Will use the given broker class name with the correspond config from cluster brokerClasses, as no default namespace config for ns "rando" +// // The name for the config should be config-cluster-class-2 +// c, err = defaults.GetBrokerConfig("rando", "cluster-class-2") +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-cluster-class-2" { +// t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) +// } +// +// // Test namespace default, without given broker class name +// // Will use the namespace default broker class, and the config exist for this namespace +// // The config name should be config-default-namespace-1-class +// c, err = defaults.GetBrokerConfig("namespace-1", nil) +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-namespace-1-class" { +// t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class, got:", c.Name) +// } +// +// // Test namespace default, with given broker class name: namespace-1-class-2 +// // Will use the given broker class name with the correspond config from namespace brokerClasses +// // The config name should be config-namespace-1-class-2 +// c, err = defaults.GetBrokerConfig("namespace-1", "namespace-1-class-2") +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-namespace-1-class-2" { +// t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class-2, got:", c.Name) +// } +// +// // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses +// // Will use the cluster config for this broker class. i.e will looking for the config in cluster brokerClasses +// // The config name should be config-cluster-class-2 +// c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-2") +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-cluster-class-2" { +// t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) +// } +// +// // Test namespace default, specify a broker class name that doesn't exist in this namespace's brokerClasses, but it is cluster's default broker class +// // Will use the cluster default broker class config, as no default broker class config is set for this namespace +// // The config name should be config-default-cluster-class +// c, err = defaults.GetBrokerConfig("namespace-1", "default-cluster-class") +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-default-cluster-class" { +// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) +// } +// +// // Shared config +// // Test namespace default, with given broker class name both have config in this namespace's brokerClasses and cluster brokerClasses +// // Will use the given broker class name with the correspond config from namespace brokerClasses. i.e namespace will override cluster config +// // The config name should be config-shared-class-in-namespace-1 +// c, err = defaults.GetBrokerConfig("namespace-1", "shared-class") +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-shared-class-in-namespace-1" { +// t.Error("GetBrokerConfig Failed, wanted config-shared-class-in-namespace-1, got:", c.Name) +// } +// +// // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses, and also doesn't have config in cluster brokerClasses +// // Should return the cluster default +// // The config name should be config-default-cluster-class +// c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-3") +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-default-cluster-class" { +// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) +// } +// +// // Test namespace default, without specifying broker class name, and the namespace default broker class's config is not set +// // Will use the cluster default broker class config, as no default broker class config is set for this namespace +// // The config name should be config-default-cluster-class +// c, err = defaults.GetBrokerConfig("namespace-2", nil) +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-default-cluster-class" { +// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) +// } +// +// // Test namespace default, without specifying broker class name, and nothing for this namespace is set +// // Will use the cluster default broker class config, as nothing is setted for this namespace +// // The config name should be config-default-cluster-class +// c, err = defaults.GetBrokerConfig("namespace-3", nil) +// if err != nil { +// t.Error("GetBrokerConfig Failed =", err) +// } +// if c.Name != "config-default-cluster-class" { +// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) +// } +// +// // Nil and empty tests +// var nilDefaults *Defaults +// _, err = nilDefaults.GetBrokerConfig("rando", nil) +// 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", nil) +// if err == nil { +// t.Errorf("GetBrokerConfig did not fail with empty") +// } +// +// if err.Error() != "Defaults for Broker Configurations have not been set up." { +// t.Error("GetBrokerConfig did not fail with non-setup msg, got", err) +// } +//} + func TestGetBrokerConfig(t *testing.T) { _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) defaults, err := NewDefaultsConfigFromConfigMap(example) if err != nil { - t.Error("NewDefaultsConfigFromConfigMap(example) =", err) - } - - // Test cluster default, without given broker class name - // Will use cluster default broker class, as no default namespace config for ns "rando" - // The name should be config-default-cluster-class - c, err := defaults.GetBrokerConfig("rando", "") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-default-cluster-class" { - t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) - } - - // Test cluster default, with given broker class name: cluster-class-2 - // Will use the given broker class name with the correspond config from cluster brokerClasses, as no default namespace config for ns "rando" - // The name for the config should be config-cluster-class-2 - c, err = defaults.GetBrokerConfig("rando", "cluster-class-2") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-cluster-class-2" { - t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) - } - - // Test namespace default, without given broker class name - // Will use the namespace default broker class, and the config exist for this namespace - // The config name should be config-default-namespace-1-class - c, err = defaults.GetBrokerConfig("namespace-1", "") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-namespace-1-class" { - t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class, got:", c.Name) - } - - // Test namespace default, with given broker class name: namespace-1-class-2 - // Will use the given broker class name with the correspond config from namespace brokerClasses - // The config name should be config-namespace-1-class-2 - c, err = defaults.GetBrokerConfig("namespace-1", "namespace-1-class-2") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-namespace-1-class-2" { - t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class-2, got:", c.Name) - } - - // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses - // Will use the cluster config for this broker class. i.e will looking for the config in cluster brokerClasses - // The config name should be config-cluster-class-2 - c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-2") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-cluster-class-2" { - t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) - } - - // Test namespace default, specify a broker class name that doesn't exist in this namespace's brokerClasses, but it is cluster's default broker class - // Will use the cluster default broker class config, as no default broker class config is set for this namespace - // The config name should be config-default-cluster-class - c, err = defaults.GetBrokerConfig("namespace-1", "default-cluster-class") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-default-cluster-class" { - t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) + t.Fatal("NewDefaultsConfigFromConfigMap(example) failed:", err) } - // Shared config - // Test namespace default, with given broker class name both have config in this namespace's brokerClasses and cluster brokerClasses - // Will use the given broker class name with the correspond config from namespace brokerClasses. i.e namespace will override cluster config - // The config name should be config-shared-class-in-namespace-1 - c, err = defaults.GetBrokerConfig("namespace-1", "shared-class") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-shared-class-in-namespace-1" { - t.Error("GetBrokerConfig Failed, wanted config-shared-class-in-namespace-1, got:", c.Name) + 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 are nil", + }, + { + name: "Empty Defaults", + namespace: "rando", + wantErrMessage: "Defaults for Broker Configurations have not been set up.", + }, } - // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses, and also doesn't have config in cluster brokerClasses - // Should return the cluster default - // The config name should be config-default-cluster-class - c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-3") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-default-cluster-class" { - t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var c *BrokerConfig + var err error - // Test namespace default, without specifying broker class name, and the namespace default broker class's config is not set - // Will use the cluster default broker class config, as no default broker class config is set for this namespace - // The config name should be config-default-cluster-class - c, err = defaults.GetBrokerConfig("namespace-2", "") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-default-cluster-class" { - t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) - } + // 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) + } - // Test namespace default, without specifying broker class name, and nothing for this namespace is set - // Will use the cluster default broker class config, as nothing is setted for this namespace - // The config name should be config-default-cluster-class - c, err = defaults.GetBrokerConfig("namespace-3", "") - if err != nil { - t.Error("GetBrokerConfig Failed =", err) - } - if c.Name != "config-default-cluster-class" { - t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) - } + // 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 + } - // 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) - } + if err != nil { + t.Errorf("Unexpected error: %v", err) + return + } - emptyDefaults := Defaults{} - _, err = emptyDefaults.GetBrokerConfig("rando", "") - if err == nil { - t.Errorf("GetBrokerConfig did not fail with empty") + if c.Name != tt.wantConfigName { + t.Errorf("Got config name %q, want %q", c.Name, tt.wantConfigName) + } + }) } +} - if err.Error() != "Defaults for Broker Configurations have not been set up." { - t.Error("GetBrokerConfig did not fail with non-setup msg, got", err) - } +func stringPtr(s string) *string { + return &s } func TestGetBrokerClass(t *testing.T) { From f6024571783d0ebe937951f4c264ed13082826f6 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 9 Feb 2024 02:14:02 -0500 Subject: [PATCH 19/36] Check if the brokerClassName is the default broker class for the whole cluster Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 7967d8b44f4..8e1eee27dd0 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -158,6 +158,11 @@ func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*Broke return d.ClusterDefaultConfig.BrokerConfig, nil } + // Check if the brokerClassName is the default broker class for the whole cluster + if d.ClusterDefaultConfig.DefaultBrokerClass == brokerClassName { + return d.ClusterDefaultConfig.BrokerConfig, nil + } + if config, ok := d.ClusterDefaultConfig.BrokerClasses[brokerClassName]; ok && config != nil { return config, nil } From b5a686381e08c5f357c350ca6bb0cff40808e9ec Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 9 Feb 2024 02:15:44 -0500 Subject: [PATCH 20/36] Fixed the build error Signed-off-by: Leo Li --- pkg/apis/config/defaults_test.go | 139 ------------------------ pkg/apis/eventing/v1/broker_defaults.go | 4 +- 2 files changed, 2 insertions(+), 141 deletions(-) diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index b309ebf8f8e..a1e5aa474f6 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -36,145 +36,6 @@ func TestDefaultsConfigurationFromFile(t *testing.T) { } } -//func TestGetBrokerConfig(t *testing.T) { -// _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) -// defaults, err := NewDefaultsConfigFromConfigMap(example) -// if err != nil { -// t.Error("NewDefaultsConfigFromConfigMap(example) =", err) -// } -// -// // Test cluster default, without given broker class name -// // Will use cluster default broker class, as no default namespace config for ns "rando" -// // The name should be config-default-cluster-class -// c, err := defaults.GetBrokerConfig("rando", nil) -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-default-cluster-class" { -// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) -// } -// -// // Test cluster default, with given broker class name: cluster-class-2 -// // Will use the given broker class name with the correspond config from cluster brokerClasses, as no default namespace config for ns "rando" -// // The name for the config should be config-cluster-class-2 -// c, err = defaults.GetBrokerConfig("rando", "cluster-class-2") -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-cluster-class-2" { -// t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) -// } -// -// // Test namespace default, without given broker class name -// // Will use the namespace default broker class, and the config exist for this namespace -// // The config name should be config-default-namespace-1-class -// c, err = defaults.GetBrokerConfig("namespace-1", nil) -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-namespace-1-class" { -// t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class, got:", c.Name) -// } -// -// // Test namespace default, with given broker class name: namespace-1-class-2 -// // Will use the given broker class name with the correspond config from namespace brokerClasses -// // The config name should be config-namespace-1-class-2 -// c, err = defaults.GetBrokerConfig("namespace-1", "namespace-1-class-2") -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-namespace-1-class-2" { -// t.Error("GetBrokerConfig Failed, wanted config-namespace-1-class-2, got:", c.Name) -// } -// -// // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses -// // Will use the cluster config for this broker class. i.e will looking for the config in cluster brokerClasses -// // The config name should be config-cluster-class-2 -// c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-2") -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-cluster-class-2" { -// t.Error("GetBrokerConfig Failed, wanted config-cluster-class-2, got:", c.Name) -// } -// -// // Test namespace default, specify a broker class name that doesn't exist in this namespace's brokerClasses, but it is cluster's default broker class -// // Will use the cluster default broker class config, as no default broker class config is set for this namespace -// // The config name should be config-default-cluster-class -// c, err = defaults.GetBrokerConfig("namespace-1", "default-cluster-class") -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-default-cluster-class" { -// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) -// } -// -// // Shared config -// // Test namespace default, with given broker class name both have config in this namespace's brokerClasses and cluster brokerClasses -// // Will use the given broker class name with the correspond config from namespace brokerClasses. i.e namespace will override cluster config -// // The config name should be config-shared-class-in-namespace-1 -// c, err = defaults.GetBrokerConfig("namespace-1", "shared-class") -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-shared-class-in-namespace-1" { -// t.Error("GetBrokerConfig Failed, wanted config-shared-class-in-namespace-1, got:", c.Name) -// } -// -// // Test namespace default, with given broker class name that doesn't have config in this namespace's brokerClasses, and also doesn't have config in cluster brokerClasses -// // Should return the cluster default -// // The config name should be config-default-cluster-class -// c, err = defaults.GetBrokerConfig("namespace-1", "cluster-class-3") -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-default-cluster-class" { -// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) -// } -// -// // Test namespace default, without specifying broker class name, and the namespace default broker class's config is not set -// // Will use the cluster default broker class config, as no default broker class config is set for this namespace -// // The config name should be config-default-cluster-class -// c, err = defaults.GetBrokerConfig("namespace-2", nil) -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-default-cluster-class" { -// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) -// } -// -// // Test namespace default, without specifying broker class name, and nothing for this namespace is set -// // Will use the cluster default broker class config, as nothing is setted for this namespace -// // The config name should be config-default-cluster-class -// c, err = defaults.GetBrokerConfig("namespace-3", nil) -// if err != nil { -// t.Error("GetBrokerConfig Failed =", err) -// } -// if c.Name != "config-default-cluster-class" { -// t.Error("GetBrokerConfig Failed, wanted config-default-cluster-class, got:", c.Name) -// } -// -// // Nil and empty tests -// var nilDefaults *Defaults -// _, err = nilDefaults.GetBrokerConfig("rando", nil) -// 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", nil) -// if err == nil { -// t.Errorf("GetBrokerConfig did not fail with empty") -// } -// -// if err.Error() != "Defaults for Broker Configurations have not been set up." { -// t.Error("GetBrokerConfig did not fail with non-setup msg, got", err) -// } -//} - func TestGetBrokerConfig(t *testing.T) { _, example := ConfigMapsFromTestFile(t, DefaultsConfigName) defaults, err := NewDefaultsConfigFromConfigMap(example) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index a629136714c..47a69e2f987 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -36,10 +36,10 @@ func (b *Broker) SetDefaults(ctx context.Context) { func (bs *BrokerSpec) SetDefaults(ctx context.Context) { cfg := config.FromContextOrDefaults(ctx) - c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, "") + c, err := cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, nil) if bs.Config != nil { - c, err = cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, bs.Config.Kind) + c, err = cfg.Defaults.GetBrokerConfig(apis.ParentMeta(ctx).Namespace, &bs.Config.Kind) } if err == nil { From ef6c1c5dd38f406c86ffacff7936283a6173ce7b Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 10 May 2024 17:21:37 -0400 Subject: [PATCH 21/36] fix: instead of always passing the empty className, pass in the variable instead Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index 47a69e2f987..8d577b11c1a 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -30,13 +30,13 @@ 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, nil) + 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) From 4106cbc569c946b8afcc8daf3db0adc1e16627f2 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 10 May 2024 17:22:22 -0400 Subject: [PATCH 22/36] 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 --- pkg/apis/config/defaults.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 8e1eee27dd0..9a0d4c075a5 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -109,7 +109,8 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerC } // Early return if brokerClassName is provided and valid - if brokerClassName != nil { + // FIXME: might cause segfault + if *brokerClassName != "" { return d.getBrokerConfigByClassName(ns, *brokerClassName) } From 77779fd133f973b1c22c3f02305a18d9b16747f9 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 10 May 2024 17:22:46 -0400 Subject: [PATCH 23/36] feat: adding some temporary test cases for testing purposes Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults_test.go | 87 ++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index 070ded3d183..8c20bafe401 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -138,6 +138,35 @@ var ( }, }, }, + "custom": { + DefaultBrokerClass: "haha-broker", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "haha-ns", + Name: "haha-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", + }, + }, + }, + }, }, ClusterDefaultConfig: &config.DefaultConfig{ DefaultBrokerClass: eventing.MTChannelBrokerClassValue, @@ -162,6 +191,17 @@ var ( BackoffDelay: pointer.String("5s"), }, }, + BrokerClasses: map[string]*config.BrokerConfig{ + "clusterBrokerClass1": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-haha", + }, + + }, + }, }, }, } @@ -490,6 +530,53 @@ 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-haha", + 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: "haha-broker", + }, + }, + Spec: BrokerSpec{ + Config: &duckv1.KReference{ + Kind: "ConfigMap", + Namespace: "haha-ns", + Name: "haha-config", + APIVersion: "v1", + }, + }, + }, + }, } for n, tc := range testCases { t.Run(n, func(t *testing.T) { From fcf9916865336bb15d2ebfd8c5104e2549f3bc55 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 10 May 2024 17:23:07 -0400 Subject: [PATCH 24/36] feat: adding some temporary test cases for testing purposes Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults_test.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index 8c20bafe401..d4241596ad5 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -192,15 +192,14 @@ var ( }, }, BrokerClasses: map[string]*config.BrokerConfig{ - "clusterBrokerClass1": { - KReference: &duckv1.KReference{ - APIVersion: "v1", - Kind: "ConfigMap", - Namespace: "knative-eventing", - Name: "imc-channel-haha", - }, - - }, + "clusterBrokerClass1": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-haha", + }, + }, }, }, }, From 606578c5a62285f23b573c06b6d7195a4861f6dc Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 10 May 2024 17:47:53 -0400 Subject: [PATCH 25/36] fix: fix the failing tests Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults_test.go | 35 ++++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index d4241596ad5..cf1da8a2e83 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -138,6 +138,35 @@ var ( }, }, }, + "customns": { + DefaultBrokerClass: "MTChannelBasedBroker", + BrokerConfig: &config.BrokerConfig{ + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "haha-ns", + Name: "haha-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: "haha-broker", BrokerConfig: &config.BrokerConfig{ @@ -476,7 +505,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", @@ -501,7 +530,7 @@ func TestBrokerSetDefaults(t *testing.T) { expected: Broker{ ObjectMeta: metav1.ObjectMeta{ Name: "broker", - Namespace: "custom", + Namespace: "customns", Annotations: map[string]string{ eventing.BrokerClassKey: "MTChannelBasedBroker", }, @@ -517,7 +546,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", }, From aef75f548900ee34778486092dfaadbef3b2794a Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 14 May 2024 13:19:42 -0400 Subject: [PATCH 26/36] fix: fix FIXME, the issue that can potentially cause SegFault Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 9a0d4c075a5..7b8d741f25e 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -109,8 +109,7 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerC } // Early return if brokerClassName is provided and valid - // FIXME: might cause segfault - if *brokerClassName != "" { + if brokerClassName != nil && *brokerClassName != "" { return d.getBrokerConfigByClassName(ns, *brokerClassName) } From 602427d670197d16caf6220b562986fab62fcb2b Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 17 Jun 2024 16:15:56 -0400 Subject: [PATCH 27/36] fix: fix the failing rabbitmq issue Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults.go b/pkg/apis/eventing/v1/broker_defaults.go index 8d577b11c1a..2867be948e5 100644 --- a/pkg/apis/eventing/v1/broker_defaults.go +++ b/pkg/apis/eventing/v1/broker_defaults.go @@ -54,11 +54,14 @@ func (bs *BrokerSpec) SetDefaults(ctx context.Context, brokerClass string) { BackoffDelay: c.Delivery.BackoffDelay, } } - // Default the namespace if not given - if bs.Config != nil { - bs.Config.SetDefaults(ctx) - } - bs.Delivery.SetDefaults(ctx) + + } + + // Default the namespace if not given + if bs.Config != nil { + bs.Config.SetDefaults(ctx) } + bs.Delivery.SetDefaults(ctx) + } From 8c1e77f8825df6e3c18d508cfa8f91f779893995 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 18 Jun 2024 09:38:01 -0400 Subject: [PATCH 28/36] fix: remove the trailing whitespace Signed-off-by: Leo Li --- pkg/apis/config/defaults_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index a1e5aa474f6..57e65c69df1 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -1032,19 +1032,19 @@ namespaceDefaults: Data: map[string]string{ "default-br-config": ` clusterDefault: - brokerClass: + brokerClass: namespaceDefaults: some-namespace: - brokerClass: + brokerClass: apiVersion: v1 kind: ConfigMap - name: - namespace: + name: + namespace: some-namespace-too: apiVersion: v1 kind: ConfigMap - name: - namespace: + name: + namespace: `, }, }, From 2f364664d25434f6b9233d8157f1887664093c48 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Thu, 20 Jun 2024 01:33:58 -0400 Subject: [PATCH 29/36] fix: update the comments Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 7b8d741f25e..51a39de6865 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -82,20 +82,18 @@ type Defaults struct { // 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 + // 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"` *BrokerConfig `json:",inline"` - // BrokerClasses are the default broker classes' config for the whole cluster + // Optional: BrokerClasses are the default broker classes' config for the whole cluster BrokerClasses map[string]*BrokerConfig `json:"brokerClasses,omitempty"` DisallowDifferentNamespaceConfig *bool `json:"disallowDifferentNamespaceConfig,omitempty"` } -// NamespaceDefaultsConfig struct contains the default configuration for the namespace. - -// 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 { @@ -103,6 +101,8 @@ type BrokerConfig struct { Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"` } +// GetBrokerConfig returns a namespace specific Broker Config, and if +// that doesn't exist, return a Cluster Default and if that doesn't exist func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerConfig, error) { if d == nil { return nil, errors.New("Defaults are nil") @@ -117,6 +117,8 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerC 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 { @@ -137,6 +139,8 @@ func (d *Defaults) getBrokerConfigByClassName(ns string, brokerClassName string) 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 { @@ -149,6 +153,7 @@ func (d *Defaults) getBrokerConfigForEmptyClassName(ns string) (*BrokerConfig, e 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 have not been set up.") @@ -171,7 +176,7 @@ func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*Broke } // 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 { From d9df59fd65066ec90cf495cd7978820a183aeba8 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 21 Jun 2024 13:50:02 -0400 Subject: [PATCH 30/36] feat: add the high level summary for the code Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 58 +++++++++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 51a39de6865..330b1baa347 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -68,6 +68,58 @@ 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 + +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. @@ -87,7 +139,7 @@ type DefaultConfig struct { DefaultBrokerClass string `json:"brokerClass,omitempty"` *BrokerConfig `json:",inline"` - // Optional: BrokerClasses are the default broker classes' config for the whole cluster + // 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"` @@ -117,8 +169,8 @@ func (d *Defaults) GetBrokerConfig(ns string, brokerClassName *string) (*BrokerC return d.getBrokerConfigForEmptyClassName(ns) } -// getBrokerConfigByClassName returns the BrokerConfig for the given brokerClassName. -// It first checks the namespace specific configuration, then the cluster default configuration. +// 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 { From b9280211b8468da99f5c3ebaedde1e54a8329a2a Mon Sep 17 00:00:00 2001 From: Leo Li Date: Thu, 11 Jul 2024 16:38:33 -0400 Subject: [PATCH 31/36] fix: fix the review comments Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 330b1baa347..7d573f630db 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -154,7 +154,7 @@ type BrokerConfig struct { } // GetBrokerConfig returns a namespace specific Broker Config, 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 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") @@ -177,14 +177,15 @@ func (d *Defaults) getBrokerConfigByClassName(ns string, brokerClassName string) // 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, should get the cluster default config + // 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) } return nsConfig.BrokerConfig, nil } - if config, ok := nsConfig.BrokerClasses[brokerClassName]; ok && config != nil { - return config, nil - } } // Check cluster default configuration @@ -211,12 +212,8 @@ func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*Broke return nil, errors.New("Defaults for Broker Configurations have not been set up.") } - if brokerClassName == "" { - return d.ClusterDefaultConfig.BrokerConfig, nil - } - // Check if the brokerClassName is the default broker class for the whole cluster - if d.ClusterDefaultConfig.DefaultBrokerClass == brokerClassName { + if brokerClassName == "" || d.ClusterDefaultConfig.DefaultBrokerClass == brokerClassName { return d.ClusterDefaultConfig.BrokerConfig, nil } @@ -248,5 +245,5 @@ func (d *Defaults) GetBrokerClass(ns string) (string, error) { } // If neither namespace specific nor cluster default broker class is found - return "", fmt.Errorf("Defaults are nil") + 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) } From 27eb33c056f1c51991e6446961a1674e8f75628d Mon Sep 17 00:00:00 2001 From: Leo Li Date: Thu, 11 Jul 2024 16:49:38 -0400 Subject: [PATCH 32/36] fix: fix the review comments Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 7d573f630db..b37a0dee226 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -137,7 +137,9 @@ 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"` - *BrokerConfig `json:",inline"` + + //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"` @@ -183,8 +185,16 @@ func (d *Defaults) getBrokerConfigByClassName(ns string, brokerClassName string) } // 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 + } } - return nsConfig.BrokerConfig, nil + } } @@ -209,12 +219,18 @@ func (d *Defaults) getBrokerConfigForEmptyClassName(ns string) (*BrokerConfig, e // 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 have not been set up.") + return nil, errors.New("Defaults for Broker Configurations for cluster have not been set up.") } // Check if the brokerClassName is the default broker class for the whole cluster if brokerClassName == "" || d.ClusterDefaultConfig.DefaultBrokerClass == brokerClassName { - return d.ClusterDefaultConfig.BrokerConfig, nil + // 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 { From fe6d9dc396eb352796ac583f8b0e0b9db32ca04d Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 22 Jul 2024 08:25:33 -0400 Subject: [PATCH 33/36] fix: fix Calum's review comments Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index b37a0dee226..b9411bf964e 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -159,7 +159,7 @@ type BrokerConfig struct { // 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 @@ -219,7 +219,7 @@ func (d *Defaults) getBrokerConfigForEmptyClassName(ns string) (*BrokerConfig, e // 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.") + return nil, errors.New("Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.") } // Check if the brokerClassName is the default broker class for the whole cluster @@ -245,7 +245,7 @@ func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*Broke // 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.") } // Check if the namespace has a specific configuration From b4f7060384840b83dde301e3f0d1757bf289c0e8 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 22 Jul 2024 10:16:46 -0400 Subject: [PATCH 34/36] fix: fix failed unit test caused by the latest review comment fix Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 2 ++ pkg/apis/config/defaults_test.go | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index b9411bf964e..a42e9083c9b 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -83,6 +83,8 @@ The priority precedence for determining the broker class and configuration is as 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 diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 57e65c69df1..5a1fff38f23 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -192,7 +192,7 @@ func TestGetBrokerClass(t *testing.T) { if err == nil { t.Errorf("GetBrokerClass did not fail with nil") } - if err.Error() != "Defaults are nil" { + 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()) } @@ -201,7 +201,8 @@ func TestGetBrokerClass(t *testing.T) { if err == nil { t.Errorf("GetBrokerClass did not fail with empty") } - if err.Error() != "Defaults are nil" { + // 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) } } From 7a7260f6c1c9534f9cb54931c24245c99e4fd3d4 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 22 Jul 2024 10:37:49 -0400 Subject: [PATCH 35/36] fix: fix failed unit test caused by the latest review comment fix Signed-off-by: Leo Li --- pkg/apis/config/defaults.go | 6 +++++- pkg/apis/config/defaults_test.go | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index a42e9083c9b..fcbdb50a521 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -196,7 +196,11 @@ func (d *Defaults) getBrokerConfigByClassName(ns string, brokerClassName string) 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 + } } } diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 5a1fff38f23..9bf87fe96be 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -108,12 +108,12 @@ func TestGetBrokerConfig(t *testing.T) { }, { name: "Nil Defaults", - wantErrMessage: "Defaults are nil", + 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 have not been set up.", + wantErrMessage: "Defaults for Broker Configurations for cluster have not been set up. You can set them via ConfigMap config-br-defaults.", }, } From 0e62b00c32a7f55a7afe2ad12f5df5642da638b0 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 22 Jul 2024 10:59:03 -0400 Subject: [PATCH 36/36] feat: add the test for the deprecating default broker config feature Signed-off-by: Leo Li --- pkg/apis/eventing/v1/broker_defaults_test.go | 53 ++++++++++++++++---- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_defaults_test.go b/pkg/apis/eventing/v1/broker_defaults_test.go index cf1da8a2e83..6232a9069d3 100644 --- a/pkg/apis/eventing/v1/broker_defaults_test.go +++ b/pkg/apis/eventing/v1/broker_defaults_test.go @@ -144,8 +144,8 @@ var ( KReference: &duckv1.KReference{ APIVersion: "v1", Kind: "ConfigMap", - Namespace: "haha-ns", - Name: "haha-config", + Namespace: "test-ns", + Name: "test-config", }, }, BrokerClasses: map[string]*config.BrokerConfig{ @@ -168,13 +168,13 @@ var ( }, }, "custom": { - DefaultBrokerClass: "haha-broker", + DefaultBrokerClass: "test-broker", BrokerConfig: &config.BrokerConfig{ KReference: &duckv1.KReference{ APIVersion: "v1", Kind: "ConfigMap", - Namespace: "haha-ns", - Name: "haha-config", + Namespace: "test-ns", + Name: "test-config", }, }, BrokerClasses: map[string]*config.BrokerConfig{ @@ -226,7 +226,15 @@ var ( APIVersion: "v1", Kind: "ConfigMap", Namespace: "knative-eventing", - Name: "imc-channel-haha", + Name: "imc-channel-test", + }, + }, + "MTChannelBasedBroker": { + KReference: &duckv1.KReference{ + APIVersion: "v1", + Kind: "ConfigMap", + Namespace: "knative-eventing", + Name: "imc-channel-in-broker-classes", }, }, }, @@ -577,7 +585,32 @@ func TestBrokerSetDefaults(t *testing.T) { Config: &duckv1.KReference{ Kind: "ConfigMap", Namespace: "knative-eventing", - Name: "imc-channel-haha", + 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", }, }, @@ -592,14 +625,14 @@ func TestBrokerSetDefaults(t *testing.T) { Name: "broker", Namespace: "custom", Annotations: map[string]string{ - eventing.BrokerClassKey: "haha-broker", + eventing.BrokerClassKey: "test-broker", }, }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Kind: "ConfigMap", - Namespace: "haha-ns", - Name: "haha-config", + Namespace: "test-ns", + Name: "test-config", APIVersion: "v1", }, },