Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Broker class spec based default #6621

Closed
wants to merge 9 commits into from
Closed

Add Broker class spec based default #6621

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
520c7cb
Add Broker class spec based default
liuchangyan Nov 21, 2022
3ef5c30
code cleanup and bug fix
liuchangyan Nov 21, 2022
ca0f0e7
unit test bug fix
liuchangyan Nov 22, 2022
2ad62d2
Update codegen
liuchangyan Nov 28, 2022
e942ada
broker setDefault unit test add
liuchangyan Dec 12, 2022
1fa7225
remove Deprecated StringPtr/Int32Ptr to String/Int32
liuchangyan Dec 12, 2022
d247b0f
fix codegen error
liuchangyan Dec 12, 2022
375ef8e
broker setDefault unit test add
liuchangyan Dec 16, 2022
3ba0387
unit tests cleanup
liuchangyan Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions pkg/apis/config/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,22 @@ type Defaults struct {
type ClassAndBrokerConfig struct {
BrokerClass string `json:"brokerClass,omitempty"`
*BrokerConfig `json:",inline"`
// BrokerClasses set the multiple configurations of different brokerClass.
// Different brokerClass use corresponding brokerConfig for all the namespaces.
BrokerClasses map[string]*BrokerConfigSpec `json:"brokerClasses,omitempty"`
}

// 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
Expand All @@ -103,12 +119,24 @@ func (d *Defaults) GetBrokerConfig(ns string) (*BrokerConfig, error) {
return nil, errors.New("Defaults are nil")
}
value, present := d.NamespaceDefaultsConfig[ns]
// get namespace default config
if present && value.BrokerConfig != nil {
return value.BrokerConfig, nil
}
Comment on lines +122 to 125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I specify broker-class based default at the namespace level value.BrokerClasses I don't think this would work because we returning always the single value.BrokerConfig

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and that makes me think we need a test for this case as well.

Copy link
Contributor Author

@liuchangyan liuchangyan Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I specify broker-class based default at the namespace level value.BrokerClasses I don't think this would work because we returning always the single value.BrokerConfig

I see your point, I think the broker-class based default at the namespace level we may not use it , but once we set the the single value.BrokerConfig we surely want to use it. And is it necessary to set broker-class based default at the namespace level ? Because we already set it at the cluster level, and in namespace we use the config of cluster is ok, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to set broker-class based default at the namespace level ?

Yes, I think it's a valid use case and also for consistency with the cluster-level defaults

ndConfig := getClassConfig(d, value)
if ndConfig != nil {
return ndConfig, nil
}

// get cluster default config
cdConfig := getClassConfig(d, d.ClusterDefault)
if cdConfig != nil {
return cdConfig, nil
}
if d.ClusterDefault != nil && d.ClusterDefault.BrokerConfig != nil {
return d.ClusterDefault.BrokerConfig, nil
}

return nil, errors.New("Defaults for Broker Configurations have not been set up.")
}

Expand All @@ -128,3 +156,36 @@ func (d *Defaults) GetBrokerClass(ns string) (string, error) {
}
return "", errors.New("Defaults for Broker Configurations have not been set up.")
}

// getClassConfig get the corresponding class Config in multiple classes
func getClassConfig(d *Defaults, cb *ClassAndBrokerConfig) *BrokerConfig {
if cb == nil || cb.BrokerClass == "" {
return nil
}
if bCSpec := getBrokerClasses(d); bCSpec != nil {
bConfig := matchBrokerClass(cb.BrokerClass, bCSpec)
Copy link
Member

@pierDipi pierDipi Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we are always using cb.BrokerClass which is equal to ClusterDefault.BrokerClass but if I create a Broker with an already set Broker class, we won't chose the correct configuration

Copy link
Contributor Author

@liuchangyan liuchangyan Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the brokerclass we set is in cluster level right? We always need to set a BrokerClass in ClusterDefault, and then we choose the corresponding BrokerConfig

if bConfig != nil {
return bConfig
}
}
return nil
}

// getBrokerClasses get the configurations of multiple brokerClass
func getBrokerClasses(d *Defaults) map[string]*BrokerConfigSpec {
if d.ClusterDefault != nil && d.ClusterDefault.BrokerClasses != nil {
return d.ClusterDefault.BrokerClasses
}
return nil
}

// matchBrokerClass find the corresponding brokerConfig for a given brokerClass
func matchBrokerClass(brokerClass string, brokerClasses map[string]*BrokerConfigSpec) *BrokerConfig {
var bConfig BrokerConfig
if bCSpec, ok := brokerClasses[brokerClass]; ok {
bConfig.KReference = bCSpec.Spec.Config
bConfig.Delivery = bCSpec.Spec.Delivery
return &bConfig
}
return nil
}
106 changes: 104 additions & 2 deletions pkg/apis/config/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -117,6 +142,34 @@ func TestGetBrokerClass(t *testing.T) {
}

func TestDefaultsConfiguration(t *testing.T) {
brokerClasses := make(map[string]*BrokerConfigSpec)
brokerSpec1 := &BrokerSpec{
Copy link
Member

Choose a reason for hiding this comment

The 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,
}
brokerClasses["MTChannelBasedBroker"] = brokerConfigSpec1
brokerClasses["KafkaBroker"] = brokerConfigSpec2

configTests := []struct {
name string
wantErr bool
Expand Down Expand Up @@ -415,6 +468,55 @@ func TestDefaultsConfiguration(t *testing.T) {
kind: ConfigMap
name: someothernametoo
namespace: someothernamespacetoo
`,
},
},
}, {
name: "only clusterdefault specified values add brokerClasses",
wantErr: false,
wantDefaults: &Defaults{
ClusterDefault: &ClassAndBrokerConfig{
BrokerClass: "clusterbrokerclass",
BrokerConfig: &BrokerConfig{
KReference: &duckv1.KReference{
Kind: "ConfigMap",
APIVersion: "v1",
Namespace: "knative-eventing",
Name: "somename",
},
Delivery: nil,
},
BrokerClasses: brokerClasses,
},
},
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:
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
`,
},
},
Expand Down
41 changes: 40 additions & 1 deletion pkg/apis/config/testdata/config-br-defaults.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,45 @@ data:
ref:
apiVersion: serving.knative.dev/v1
kind: Service
name: handle-error
name: cluster-handle-error
namespace: knative-eventing
backoffPolicy: exponential
backoffDelay: 3s
brokerClasses:
MTChannelBasedBroker:
spec:
delivery:
retry: 3
deadLetterSink:
ref:
apiVersion: serving.knative.dev/v1
kind: Service
name: mt-handle-error
namespace: knative-eventing
backoffPolicy: exponential
backoffDelay: 3s
config:
apiVersion: v1
kind: ConfigMap
name: mt-test
namespace: knative-eventing
KafkaBroker:
spec:
delivery:
retry: 3
deadLetterSink:
ref:
apiVersion: serving.knative.
kind: Service
name: kafka-handle-error
namespace: knative-eventing
backoffPolicy: exponential
backoffDelay: 3s
config:
apiVersion: v1
kind: ConfigMap
name: kafka-test
namespace: knative-eventing

namespaceDefaults:
some-namespace:
Expand All @@ -62,3 +97,7 @@ data:
namespace: someothernamespace
backoffPolicy: linear
backoffDelay: 5s
some-namespace-two:
brokerClass: KafkaBroker
some-namespace-three:
brokerClass: KafkaBroker
62 changes: 62 additions & 0 deletions pkg/apis/config/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading