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

Broker class based defaults #5992

Open
pierDipi opened this issue Dec 13, 2021 · 4 comments · May be fixed by #7631
Open

Broker class based defaults #5992

pierDipi opened this issue Dec 13, 2021 · 4 comments · May be fixed by #7631
Assignees
@Leo6Leo @liuchangyan
Labels
area/brokers kind/feature-request priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Issues which should be fixed (post-triage)

Comments

@pierDipi
Copy link
Member

pierDipi commented Dec 13, 2021 •
edited

Problem

Operators can set defaults for brokers at the cluster or namespace level in the
config-br-defaults ConfigMap.

When there are multiple classes in the same cluster, it's not possible to set
a default for broker.spec fields based on the configured broker class.

A convenient configuration like the following would be perfect:

    clusterDefault:
      brokerClass: MTChannelBasedBroker
      apiVersion: v1
      kind: ConfigMap
      name: config-br-default-channel
      namespace: knative-eventing
      <broker_class>:
        spec:
          delivery: 
            retry: <...>
            # ...
          config: 
            apiVersion: <...>
            kind: <...>
            name: <...>
            namespace: <...>

Persona:
Which persona is this feature for?

Operator

Exit Criteria
A measurable (binary) test that would indicate that the problem has been resolved.

Time Estimate (optional):
How many developer-days do you think this may take to resolve?
2

Additional context (optional)
Add any other context about the feature request here.

/area brokers
/priority important-longterm
/kind feature-request
@knative-prow-robot knative-prow-robot added area/brokers priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Dec 13, 2021
@pierDipi pierDipi added the triage/accepted Issues which should be fixed (post-triage) label Dec 13, 2021
@liuchangyan
Copy link
Contributor

/assign

@liuchangyan
Copy link
Contributor

In this configuration, the <broker_class> is the unknown key in JSON, and in the original code it use parseEntry function to transfer YAML to JSON, if we want to unmarshal the known key in the whole YAML, we need to modify the NewDefaultsConfigFromMap, it will make our code became more complex, so I suggest we use the following Configuration:

  clusterDefault:
      brokerClass: MTChannelBasedBroker
      apiVersion: v1
      kind: ConfigMap
      name: config-br-default-channel
      namespace: knative-eventing
      brokerClassSpec:
        MTChannelBasedBroker:
          spec:
            delivery: 
              retry: <...>
              # ...
            config: 
              apiVersion: <...>
              kind: <...>
              name: <...>
              namespace: <...>
        KafkaBroker:
          spec:
            delivery: 
              retry: <...>
              # ...
            config: 
              apiVersion: <...>
              kind: <...>
              name: <...>
              namespace: <...>
  namespaceDefaults:
     namespace-1:
       brokerClass: MTChannelBasedBroker
     namespace-2:
       brokerClass: KafkaBroker

@pierDipi what do you think?

@liuchangyan liuchangyan mentioned this issue Nov 21, 2022
Closed
5 tasks
@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 3, 2024

/assign

@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 3, 2024

https://docs.google.com/document/d/1RKij-DYPmcbCTHF26hkXdrtWHqrksA-Fo5qYLRP7xVA/edit
Added some initial thoughts in this google doc.

@Leo6Leo Leo6Leo linked a pull request Jan 26, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Leo6Leo Leo6Leo

@liuchangyan liuchangyan

Labels
area/brokers kind/feature-request priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Issues which should be fixed (post-triage)
Projects
None yet
Milestone
No milestone
4 participants
@pierDipi @Leo6Leo @knative-prow-robot @liuchangyan