RFC: Standardize the Kuadrant Spec #112
Conversation
Start RFC that outlines an Standardized API design for Kuadrant Spec Signed-off-by: Jim Fitzpatrick <[email protected]>
| # Motivation | ||
| [motivation]: #motivation | ||
|
|
||
| As we are move forward with the development of Kuadrant, we are wanting to add new features to the product. |
There was a problem hiding this comment.
| As we are move forward with the development of Kuadrant, we are wanting to add new features to the product. | |
| As we move forward with the development of Kuadrant, we are wanting to add new features to the product. |
| spec: | ||
| mtls: | ||
| configure: true | ||
| enable: ["authorino"] |
There was a problem hiding this comment.
So from the start I was looking to see how this would be presented. I worry that this is overly verbose? If instead you had a default for mtls of not enabled this would be the equivalent of leaving it blank or more sepecifically
mtls:
enable: []
so I am wondering is the configure part needed?
There was a problem hiding this comment.
Being verbose is on propose, it gives a clearer picture the user about what they are doing. In your example, if there is more then one feature in the kuadrant CR, lets mtls and service monitors. One feature is enabled by default and the second is disabled. What would a empty enable list mean on both configurations?
The verbose nature shows the intend in a much clearer. It is not just about how easy it is to configure today, but also how easy would it be for a new system administrator to onboard after kuadrant is used by our users for years to come.
From a technical point, I think an empty list would be dropped from the spec as I think we would need to do as omit if empty so as not make breaking changes to API. If this is the case it would become harder to understand what is happening.
| `spec.feature_a.enable` is a list of strings that represent the components the configuration will be applied too. | ||
| Having it a list of strings allows us to extend or reduce the number of accepted values without needing to modify the CRD. | ||
| With the use of strings as values means keywords can also be defined, for example `"default"` could be shorthand for a sub set of components affected by the feature. | ||
| `spec.feature_a.disable` is a list of strings that repenting the components that the configuration will *not* be applied too. |
There was a problem hiding this comment.
I think this could be added at a later date if it was shown to be needed. We don't have that many components and so an enable list should not be too onerous on the user IMO
There was a problem hiding this comment.
You maybe right, but I would like to have a guide to what happens if the disable is required. I don't like the idea of updating the CRD with new fields in the future, if we can plan for it now.
| - What is the impact of not doing this? | ||
|
|
||
| # Prior art | ||
| [prior-art]: #prior-art |
There was a problem hiding this comment.
I took inspiration for the grafana config from the kiali CR https://kiali.io/docs/configuration/kialis.kiali.io/
I've used the prometheus and alertmanager CRs a lot https://github.com/prometheus-operator/prometheus-operator/blob/main/Documentation/api-reference/api.md#monitoringcoreoscomv1
Both seem like reasonable prior art in this space.
There was a problem hiding this comment.
I can add them is as prior art.
The kiali CR reference shows how out of control this can get. That is only for one product while we would need to be able to do that for many.
There was a problem hiding this comment.
how out of control this can get
What makes you say it's out of control?
From the developer docs, it looks like they have a good handle on managing configuration.
Perhaps it's different from those on the ground developing it, but from a user point of view, the docs have the entire spec in one place, and detailed docs for the different sections, for example, https://kiali.io/docs/configuration/p8s-jaeger-grafana/grafana/
That is only for one product while we would need to be able to do that for many.
Do you mean one component?
Kuadrant is one project, with multiple components in my view.
There was a problem hiding this comment.
The kiali configuration is the one that I am saying if we try that method it will get out of control. Kiali is a a product, there is a UI, it is what the user is expected to interact with. I agree that kuadrant is one project with multiple components. It is however not one product.
In the kaili configuration there is spec.deployment, which I am assuming allows you to change the deployment setting for the product, which the users sees as the UI. What would such a value mean in a kuadrant context. Is it configuring the kuadrant operator, console plugin, all the sub components? Would we need a similar section for every component? This is how I mean it gets very messy.
We shouldn't have to lift every configuration setting up into the kuadrant CR. Our components while they work together are not tied to each other. The user might want Authorino in a HA mode, but not the DNS operator. Where as I feel with the kiali product, putting that into a HA mode would also require its sub components to be in a matching HA mode.
There is also the issue where the kiali style does not show the intent of the configuration, such as zero-trust, as mentioned in Kuadrant/kuadrant-operator#1130. I like the verbosity of the configuration, but is that what we are trying to achieve.
| feature_a: | ||
| configure: true # optional, boolean, defaults to false | ||
| enable: [] # optional, list of strings | ||
| disable: [] # optional, list of strings |
There was a problem hiding this comment.
I think having both an 'enable' and 'disable' list would be confusing for the user.
Are they meant to be mutually exclusive?
Should only 1 or the other be set?
Or should I make sure all components are included across the union of both lists.
There was a problem hiding this comment.
When you say mutually exclusive, which do you mean?
- you can only have the
enableordisablelists. - a value can only be in
enableordisable, but not in both.
There was a problem hiding this comment.
Either.
Hence the confusion.
Also, what happens if i'm not exhaustive in what i put in both arrays?
For example:
enable: ['authorino']
disable: ['limitador']Do all other components have the feature enabled or disabled?
I'm not looking for an answer here (at least not yet).
Moreso highlighting that it's confusing to me.
There was a problem hiding this comment.
I will do a bit of rewording around this. I am guess that the # optional is also causing some of this confusion.
| One thing that would be left up to the feature design, and is out of scope of this RFC is how the elements should be prioritized. | ||
| For example if a component is listed as enable in two sets of configures, which configuration is added. | ||
| In the case of logging I believe it should be the more verbose configuration. | ||
| ```yaml | ||
| spec: | ||
| logging_level: | ||
| - level: "error" | ||
| configure: true | ||
| enable: ["kuadrant-operator"] | ||
| disable: [] | ||
| - level: "debug" | ||
| configure: true | ||
| enable: ["kuadrant-operator"] | ||
| disable: [] | ||
| ``` |
There was a problem hiding this comment.
Thinking about this again. There might be some thing that we can do with CEL to ensure a component is only configured in one listed object.
- Reword on section - Add prior art Signed-off-by: Jim Fitzpatrick <[email protected]>
|
Related conversation: https://kubernetes.slack.com/archives/C05J0D0V525/p1737380600067349 |
Start RFC that outlines an Standardized API design for Kuadrant Spec
There is a number of RFC attempting to add new features to Kuadrant that would be configured through the Kuadrant CR. There has being attempts to add feature before the V1 release, and these features got removed due to the messy API design that was used.
Before any new feature work that would be configured in the kuadrant CR, we should agree on a common API design as this will affect how features are designed to work.