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.

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

Broker class based defaults #7631

Merged
merged 48 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
d7ee0ca
First draft of the default broker class code
Leo6Leo Jan 26, 2024
66dfc36
Merge branch 'main' into default-broker-class
Leo6Leo Jan 29, 2024
662c214
Change the default broker class config
Leo6Leo Jan 29, 2024
3c11b02
Run codegen
Leo6Leo Jan 30, 2024
8145609
Refactor the code
Leo6Leo Jan 30, 2024
0f3b4a2
Codegen
Leo6Leo Jan 30, 2024
eca570c
Merge branch 'main' into default-broker-class
Leo6Leo Jan 31, 2024
5f61000
Fix the build error and run code gen
Leo6Leo Jan 31, 2024
b8a649e
Fix the build error in the tests
Leo6Leo Jan 31, 2024
73eb74d
Fix the build error in the tests
Leo6Leo Jan 31, 2024
03f8cab
Modify the current existing test, and add more test cases
Leo6Leo Jan 31, 2024
6395453
Modify the current existing test, and add more test cases
Leo6Leo Jan 31, 2024
15fa1c4
Merge branch 'main' into default-broker-class
Leo6Leo Feb 2, 2024
2480d40
Pass in the actual broker name instead of leaving it empty
Leo6Leo Feb 2, 2024
a70189f
Modify the condition to check whether Different Namespace is allowed …
Leo6Leo Feb 2, 2024
76592bb
Revert the brokerSpec.Name change.
Leo6Leo Feb 2, 2024
9b5abf3
Fix the failing unit tests by adding the default config for the defaults
Leo6Leo Feb 5, 2024
4a2da5f
Fix the failing unit tests
Leo6Leo Feb 5, 2024
3813fa6
Merge branch 'main' into default-broker-class
Leo6Leo Feb 6, 2024
d0a1660
Fix the review comments
Leo6Leo Feb 7, 2024
51c88f7
Merge branch 'main' into default-broker-class
Leo6Leo Feb 7, 2024
c678829
Merge branch 'main' into default-broker-class
Leo6Leo Feb 9, 2024
28c30e6
Update pkg/apis/config/defaults.go
Leo6Leo Feb 9, 2024
d37c5e3
Change the tests to be table form
Leo6Leo Feb 9, 2024
f602457
Check if the brokerClassName is the default broker class for the whol…
Leo6Leo Feb 9, 2024
b5a6863
Fixed the build error
Leo6Leo Feb 9, 2024
37c637a
Merge branch 'main' into default-broker-class
Leo6Leo Mar 8, 2024
5f6d615
Merge remote-tracking branch 'origin/main' into default-broker-class
Leo6Leo May 10, 2024
ef6c1c5
fix: instead of always passing the empty className, pass in the varia…
Leo6Leo May 10, 2024
4106cbc
fix: brokerClass is a string pointer. If the value is null but pointe…
Leo6Leo May 10, 2024
77779fd
feat: adding some temporary test cases for testing purposes
Leo6Leo May 10, 2024
fcf9916
feat: adding some temporary test cases for testing purposes
Leo6Leo May 10, 2024
606578c
fix: fix the failing tests
Leo6Leo May 10, 2024
3bcdf2f
Merge remote-tracking branch 'origin/main' into default-broker-class
Leo6Leo May 13, 2024
78c73e2
Merge remote-tracking branch 'origin/main' into default-broker-class
Leo6Leo May 14, 2024
aef75f5
fix: fix FIXME, the issue that can potentially cause SegFault
Leo6Leo May 14, 2024
5333a8a
Merge remote-tracking branch 'upstream/main' into default-broker-class
Leo6Leo Jun 10, 2024
602427d
fix: fix the failing rabbitmq issue
Leo6Leo Jun 17, 2024
8c1e77f
fix: remove the trailing whitespace
Leo6Leo Jun 18, 2024
2f36466
fix: update the comments
Leo6Leo Jun 20, 2024
d9df59f
feat: add the high level summary for the code
Leo6Leo Jun 21, 2024
96ae24c
Merge branch 'main' into default-broker-class
Leo6Leo Jul 11, 2024
b928021
fix: fix the review comments
Leo6Leo Jul 11, 2024
27eb33c
fix: fix the review comments
Leo6Leo Jul 11, 2024
fe6d9dc
fix: fix Calum's review comments
Leo6Leo Jul 22, 2024
b4f7060
fix: fix failed unit test caused by the latest review comment fix
Leo6Leo Jul 22, 2024
7a7260f
fix: fix failed unit test caused by the latest review comment fix
Leo6Leo Jul 22, 2024
0e62b00
feat: add the test for the deprecating default broker config feature
Leo6Leo Jul 22, 2024
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
124 changes: 96 additions & 28 deletions pkg/apis/config/defaults.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check failure on line 1 in pkg/apis/config/defaults.go

View workflow job for this annotation

GitHub Actions / style / Golang / Auto-format and Check

Please run goimports. diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 51a39de..1ca836c 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -117,8 +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. +// 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 {
Copyright 2020 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -71,62 +71,130 @@
// Defaults includes the default values to be populated by the webhook.
type Defaults struct {
// NamespaceDefaultsConfig are the default Broker Configs for each namespace.
// Namespace is the key, the value is the KReference to the config.
NamespaceDefaultsConfig map[string]*ClassAndBrokerConfig `json:"namespaceDefaults,omitempty"`
// Namespace is the key, the value is the NamespaceDefaultConfig
NamespaceDefaultsConfig map[string]*DefaultConfig `json:"namespaceDefaults,omitempty"`

// ClusterDefaultBrokerConfig is the default broker config for all the namespaces that
// are not in NamespaceDefaultBrokerConfigs.
ClusterDefault *ClassAndBrokerConfig `json:"clusterDefault,omitempty"`
// ClusterDefaultConfig is the default broker config for all the namespaces that
// are not in NamespaceDefaultBrokerConfigs. Different broker class could have
// different default config.
ClusterDefaultConfig *DefaultConfig `json:"clusterDefault,omitempty"`
}

// ClassAndBrokerConfig contains configuration for a given namespace for broker. Allows
// configuring the Class of the Broker, the reference to the
// config it should use and it's delivery.
type ClassAndBrokerConfig struct {
BrokerClass string `json:"brokerClass,omitempty"`
*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/namespace.
// Users have to specify both of them
DefaultBrokerClass string `json:"brokerClass,omitempty"`
*BrokerConfig `json:",inline"`
Leo6Leo marked this conversation as resolved.
Show resolved Hide resolved

// Optional: BrokerClasses are the default broker classes' config for the whole cluster
BrokerClasses map[string]*BrokerConfig `json:"brokerClasses,omitempty"`

DisallowDifferentNamespaceConfig *bool `json:"disallowDifferentNamespaceConfig,omitempty"`
}

// BrokerConfig contains configuration for a given namespace for broker. Allows
// BrokerConfig contains configuration for a given broker. Allows
// configuring the reference to the
// config it should use and it's delivery.
type BrokerConfig struct {
*duckv1.KReference `json:",inline"`
Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"`
}

// GetBrokerConfig returns a namespace specific Broker Configuration, and if
// GetBrokerConfig returns a namespace specific Broker Config, and if
// that doesn't exist, return a Cluster Default and if that doesn't exist
// return an error.
Leo6Leo marked this conversation as resolved.
Show resolved Hide resolved
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

// Early return if brokerClassName is provided and valid
if brokerClassName != nil && *brokerClassName != "" {
return d.getBrokerConfigByClassName(ns, *brokerClassName)
}

// Handling empty brokerClassName
return d.getBrokerConfigForEmptyClassName(ns)
}

// getBrokerConfigByClassName returns the BrokerConfig for the given brokerClassName.
// It first checks the namespace specific configuration, then the cluster default configuration.
func (d *Defaults) getBrokerConfigByClassName(ns string, brokerClassName string) (*BrokerConfig, error) {
// Check namespace specific configuration
if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil {
// check if the brokerClassName is the default broker class for this namespace
if nsConfig.DefaultBrokerClass == brokerClassName {
if nsConfig.BrokerConfig == nil {
// as no default broker class config is set for this namespace, should get the cluster default config
return d.getClusterDefaultBrokerConfig(brokerClassName)
}
Leo6Leo marked this conversation as resolved.
Show resolved Hide resolved
return nsConfig.BrokerConfig, nil
}
if config, ok := nsConfig.BrokerClasses[brokerClassName]; ok && config != nil {
return config, nil
}
}

// Check cluster default configuration
return d.getClusterDefaultBrokerConfig(brokerClassName)
}

// getBrokerConfigForEmptyClassName returns the BrokerConfig for the given namespace when brokerClassName is empty.
// It first checks the namespace specific configuration, then the cluster default configuration.
func (d *Defaults) getBrokerConfigForEmptyClassName(ns string) (*BrokerConfig, error) {
// Check if namespace has a default broker class
if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil {
if nsConfig.DefaultBrokerClass != "" {
return d.getBrokerConfigByClassName(ns, nsConfig.DefaultBrokerClass)
}
}

// Fallback to cluster default configuration
return d.getClusterDefaultBrokerConfig("")
}

// getClusterDefaultBrokerConfig returns the BrokerConfig for the given brokerClassName.
func (d *Defaults) getClusterDefaultBrokerConfig(brokerClassName string) (*BrokerConfig, error) {
if d.ClusterDefaultConfig == nil || d.ClusterDefaultConfig.BrokerConfig == nil {
return nil, errors.New("Defaults for Broker Configurations have not been set up.")
}
if d.ClusterDefault != nil && d.ClusterDefault.BrokerConfig != nil {
return d.ClusterDefault.BrokerConfig, nil

if brokerClassName == "" {
return d.ClusterDefaultConfig.BrokerConfig, nil
}
return nil, errors.New("Defaults for Broker Configurations have not been set up.")

// Check if the brokerClassName is the default broker class for the whole cluster
if d.ClusterDefaultConfig.DefaultBrokerClass == brokerClassName {
return d.ClusterDefaultConfig.BrokerConfig, nil
}

if config, ok := d.ClusterDefaultConfig.BrokerClasses[brokerClassName]; ok && config != nil {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we check before, if d.ClusterDefaultConfig.BrokerClass == brokerClassName and if this is the case return d.ClusterDefaultConfig.BrokerConfig, as you wrote in https://docs.google.com/document/d/1RKij-DYPmcbCTHF26hkXdrtWHqrksA-Fo5qYLRP7xVA/edit

# The brokerClasses contain the configmap, delivery spec for other brokerClasses.
# The one specified above is NOT included.
# Reason: to maintain compatibility, we are not removing those old fields. And it is not 
# necessary to add it again here under brokerClasses.

Otherwise d.ClusterDefaultConfig.BrokerConfig does not take preference over d.ClusterDefaultConfig.BrokerClasses[] (what I meant in #7631 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the logic here is correct, prob I didn't explain clearly in the Doc, sorry about that! @creydr

if d.ClusterDefaultConfig.BrokerClass == brokerClassName, then we will directly use d.ClusterDefaultConfig.BrokerConfig. And this brokerClass shouldn't appear in d.ClusterDefaultConfig.BrokerClasses.

When I was referring to:

The one specified above is NOT included.

It means the brokerConfig for d.ClusterDefaultConfig.BrokerClass will not be included in the d.ClusterDefaultConfig.BrokerClasses

return config, nil
}

return d.ClusterDefaultConfig.BrokerConfig, nil
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to return an error here instead? At this point we know there is no config for the requested broker class, so we might get weird behaviour for brokers with a random config that doesn't match their class. @pierDipi @creydr any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Ping @Leo6Leo - wdyt here? From the previous comment, my intuition is that this should return an error, but I'm not sure

Copy link
Member Author

Choose a reason for hiding this comment

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

If the requested brokerClass lacks a specific configuration, the system should fallback to using the default brokerClass and its configuration, rather than throwing an error. This behavior aligns with the purpose of having a default configuration in place in my opinion. But returning the error also makes sense to me.

}

// GetBrokerClass returns a namespace specific Broker Class, and if
// that doesn't exist, return a Cluster Default and if that doesn't exist
// that doesn't exist, return a Cluster Default and if the defaults doesn't exist
// return an error.
func (d *Defaults) GetBrokerClass(ns string) (string, error) {
if d == nil {
return "", errors.New("Defaults are nil")
}
value, present := d.NamespaceDefaultsConfig[ns]
if present && value.BrokerClass != "" {
return value.BrokerClass, nil

// Check if the namespace has a specific configuration
if nsConfig, ok := d.NamespaceDefaultsConfig[ns]; ok && nsConfig != nil {
if nsConfig.DefaultBrokerClass != "" {
return nsConfig.DefaultBrokerClass, nil
}
}
if d.ClusterDefault != nil && d.ClusterDefault.BrokerClass != "" {
return d.ClusterDefault.BrokerClass, nil

// Fallback to cluster default configuration if namespace specific configuration is not set
if d.ClusterDefaultConfig != nil && d.ClusterDefaultConfig.DefaultBrokerClass != "" {
return d.ClusterDefaultConfig.DefaultBrokerClass, nil
}
return "", errors.New("Defaults for Broker Configurations have not been set up.")

// If neither namespace specific nor cluster default broker class is found
return "", fmt.Errorf("Defaults are nil")
Leo6Leo marked this conversation as resolved.
Show resolved Hide resolved
}
Loading
Loading