-
Notifications
You must be signed in to change notification settings - Fork 590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Broker class spec based default #6621
Changes from 1 commit
520c7cb
3ef5c30
ca0f0e7
2ad62d2
e942ada
1fa7225
d247b0f
375ef8e
3ba0387
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,23 @@ type Defaults struct { | |
type ClassAndBrokerConfig struct { | ||
BrokerClass string `json:"brokerClass,omitempty"` | ||
*BrokerConfig `json:",inline"` | ||
// BrokerClassSpec set the multiple configurations of different brokerClass. | ||
// Different brokerClass use corresponding brokerConfig for all the namespaces that | ||
// are not in NamespaceDefaultBrokerConfigs. | ||
BrokerClassSpec map[string]*BrokerConfigSpec `json:"brokerClassSpec,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sure |
||
} | ||
|
||
// BrokerConfigSpec contains concrete spec for broker. | ||
type BrokerConfigSpec struct { | ||
Spec *BrokerSpec `json:"spec,omitempty"` | ||
} | ||
|
||
// BrokerSpec contains the Config and Delivery for broker. Allows configuring the Config | ||
// which is a KReference that specifies configuration options for this Broker. | ||
// Delivery contains the delivery spec for each trigger to this Broker. | ||
type BrokerSpec struct { | ||
Config *duckv1.KReference `json:"config,omitempty"` | ||
Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"` | ||
} | ||
|
||
// BrokerConfig contains configuration for a given namespace for broker. Allows | ||
|
@@ -102,12 +119,30 @@ func (d *Defaults) GetBrokerConfig(ns 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 | ||
} | ||
if d.ClusterDefault != nil && d.ClusterDefault.BrokerConfig != nil { | ||
return d.ClusterDefault.BrokerConfig, nil | ||
if present && value.BrokerClass != "" { | ||
bCSpec := getBrokerClassSpec(d) | ||
bConfig := matchBrokerSpec(value.BrokerClass, bCSpec) | ||
if bConfig != nil { | ||
return bConfig, nil | ||
} | ||
} | ||
|
||
if d.ClusterDefault != nil { | ||
if d.ClusterDefault.BrokerClass != "" { | ||
bCSpec := getBrokerClassSpec(d) | ||
bConfig := matchBrokerSpec(d.ClusterDefault.BrokerClass, bCSpec) | ||
if bConfig != nil { | ||
return bConfig, nil | ||
} | ||
} | ||
if d.ClusterDefault.BrokerConfig != nil { | ||
return d.ClusterDefault.BrokerConfig, nil | ||
} | ||
} | ||
return nil, errors.New("Defaults for Broker Configurations have not been set up.") | ||
} | ||
|
@@ -128,3 +163,26 @@ func (d *Defaults) GetBrokerClass(ns string) (string, error) { | |
} | ||
return "", errors.New("Defaults for Broker Configurations have not been set up.") | ||
} | ||
|
||
// getBrokerClassSpec get the configurations of multiple brokerClass | ||
func getBrokerClassSpec(d *Defaults) map[string]*BrokerConfigSpec { | ||
if d.ClusterDefault != nil && d.ClusterDefault.BrokerClassSpec != nil { | ||
return d.ClusterDefault.BrokerClassSpec | ||
} | ||
return nil | ||
} | ||
|
||
// matchBrokerSpec find the corresponding brokerConfig for a given brokerClass | ||
func matchBrokerSpec(brokerClass string, brokerClassSpec map[string]*BrokerConfigSpec) *BrokerConfig { | ||
if brokerClassSpec != nil { | ||
for bClass, bCSpec := range brokerClassSpec { | ||
if bClass == brokerClass { | ||
var bConfig BrokerConfig | ||
bConfig.KReference = bCSpec.Spec.Config | ||
bConfig.Delivery = bCSpec.Spec.Delivery | ||
return &bConfig | ||
} | ||
} | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,8 +46,11 @@ func TestGetBrokerConfig(t *testing.T) { | |
if err != nil { | ||
t.Error("GetBrokerConfig Failed =", err) | ||
} | ||
if c.Name != "somename" { | ||
t.Error("GetBrokerConfig Failed, wanted somename, got:", c.Name) | ||
if c.Name != "mt-test" { | ||
t.Error("GetBrokerConfig Failed, wanted mt-test, got:", c.Name) | ||
} | ||
if c.Delivery.DeadLetterSink.Ref.Name != "mt-handle-error" { | ||
t.Error("GetBrokerConfig Failed, wanted mt-handle-error, got:", c.Delivery.DeadLetterSink.Ref.Name) | ||
} | ||
c, err = defaults.GetBrokerConfig("some-namespace") | ||
if err != nil { | ||
|
@@ -56,6 +59,28 @@ func TestGetBrokerConfig(t *testing.T) { | |
if c.Name != "someothername" { | ||
t.Error("GetBrokerConfig Failed, wanted someothername, got:", c.Name) | ||
} | ||
// Test GetBrokerConfig in different namespace | ||
c, err = defaults.GetBrokerConfig("some-namespace-two") | ||
if err != nil { | ||
t.Error("GetBrokerConfig Failed =", err) | ||
} | ||
if c.Name != "kafka-test" { | ||
t.Error("GetBrokerConfig Failed, wanted kafka-test, got:", c.Name) | ||
} | ||
if c.Delivery.DeadLetterSink.Ref.Name != "kafka-handle-error" { | ||
t.Error("GetBrokerConfig Failed, wanted kafka-handle-error, got:", c.Delivery.DeadLetterSink.Ref.Name) | ||
} | ||
|
||
c, err = defaults.GetBrokerConfig("some-namespace-three") | ||
if err != nil { | ||
t.Error("GetBrokerConfig Failed =", err) | ||
} | ||
if c.Name != "kafka-test" { | ||
t.Error("GetBrokerConfig Failed, wanted kafka-test, got:", c.Name) | ||
} | ||
if c.Delivery.DeadLetterSink.Ref.Name != "kafka-handle-error" { | ||
t.Error("GetBrokerConfig Failed, wanted kafka-handle-error, got:", c.Delivery.DeadLetterSink.Ref.Name) | ||
} | ||
|
||
// Nil and empty tests | ||
var nilDefaults *Defaults | ||
|
@@ -117,6 +142,34 @@ func TestGetBrokerClass(t *testing.T) { | |
} | ||
|
||
func TestDefaultsConfiguration(t *testing.T) { | ||
brokerClassSpec := make(map[string]*BrokerConfigSpec) | ||
brokerSpec1 := &BrokerSpec{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here on the naming |
||
Config: &duckv1.KReference{ | ||
Kind: "ConfigMap", | ||
APIVersion: "v1", | ||
Namespace: "knative-eventing", | ||
Name: "mt-test", | ||
}, | ||
Delivery: nil, | ||
} | ||
brokerConfigSpec1 := &BrokerConfigSpec{ | ||
Spec: brokerSpec1, | ||
} | ||
brokerSpec2 := &BrokerSpec{ | ||
Config: &duckv1.KReference{ | ||
Kind: "ConfigMap", | ||
APIVersion: "v1", | ||
Namespace: "knative-eventing", | ||
Name: "kafka-test", | ||
}, | ||
Delivery: nil, | ||
} | ||
brokerConfigSpec2 := &BrokerConfigSpec{ | ||
Spec: brokerSpec2, | ||
} | ||
brokerClassSpec["MTChannelBasedBroker"] = brokerConfigSpec1 | ||
brokerClassSpec["KafkaBroker"] = brokerConfigSpec2 | ||
|
||
configTests := []struct { | ||
name string | ||
wantErr bool | ||
|
@@ -415,6 +468,55 @@ func TestDefaultsConfiguration(t *testing.T) { | |
kind: ConfigMap | ||
name: someothernametoo | ||
namespace: someothernamespacetoo | ||
`, | ||
}, | ||
}, | ||
}, { | ||
name: "only clusterdefault specified values add brokerClassSpec", | ||
wantErr: false, | ||
wantDefaults: &Defaults{ | ||
ClusterDefault: &ClassAndBrokerConfig{ | ||
BrokerClass: "clusterbrokerclass", | ||
BrokerConfig: &BrokerConfig{ | ||
KReference: &duckv1.KReference{ | ||
Kind: "ConfigMap", | ||
APIVersion: "v1", | ||
Namespace: "knative-eventing", | ||
Name: "somename", | ||
}, | ||
Delivery: nil, | ||
}, | ||
BrokerClassSpec: brokerClassSpec, | ||
}, | ||
}, | ||
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 | ||
brokerClassSpec: | ||
MTChannelBasedBroker: | ||
spec: | ||
config: | ||
apiVersion: v1 | ||
kind: ConfigMap | ||
name: mt-test | ||
namespace: knative-eventing | ||
KafkaBroker: | ||
spec: | ||
config: | ||
apiVersion: v1 | ||
kind: ConfigMap | ||
name: kafka-test | ||
namespace: knative-eventing | ||
`, | ||
}, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we leaving broker-class based defaults for namespace defaults out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no, we are not leaving, the annotation description is wrong