-
Notifications
You must be signed in to change notification settings - Fork 11
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
rfc: configuration of kuadrant sub components #25
Conversation
This might be of interest to this: |
dd8da75
to
155ea6e
Compare
155ea6e
to
e8ce534
Compare
#### Kubernetes native | ||
- replicas | ||
|
||
#### Observability |
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.
what am I setting when configuring these? LogLevel is obvious, but what is the value to exposing Healthz, Metrics, Tracing ? Should these be things we set as part of the kaudrant install. I guess I am struggling to see the use case for setting some of these and perhaps we should limit down the set we want to expose further @alexsnaps ?
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.
I think it is reasonable to allow some of the configuration options to bubble up to the kuadrant CR. I have concerns with how many we are proposing. I am not sure we have clear use case for each of them. The redis url has a clear use case for example, but I don't know why I am setting the number replicas for example (why can't I just scale up the deployment) this seems more complex than what is needed. I worry a little about leaking things that exist in established APIs such as deployments up through not one but two CRs. Personally I have become more of a fan of you get the default deployment and there are things you can chane about that if you want (IE numbr of replicas)
@maleck13 On the concern about the number of configuration options that are bubbling up to the Kuadrant CR. I am happy to reduce the number, it would be better to start with a lesser number and expend the number options at a later date. As for the number of replicas being configured, in the case of limitador it is not recommend to change this in the deployment as depending on what storage option is selected the number of replicas will be restricted. We have also had a number of issues opened against limitador-operator asking to move configuration options into the limitador CR, this is extending their use case. |
@@ -0,0 +1,237 @@ | |||
# Configuration of Kuadrant Sub Components | |||
|
|||
- Feature Name: Configuration of Kuadrant Sub components. |
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.
- Feature Name: Configuration of Kuadrant Sub components. | |
- Feature Name: `sub-components-config` |
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.
What is the reasoning behind this change?
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.
Following the pattern from other RFCs. I think the idea for this is to be like a unique identifier that can be used to track the feature unequivocally across, e.g., file names, etc. It helps avoid overloading words and related confusion of such of when relying otherwise only on the human version of it (which is already in the title, BTW.)
[motivation]: #motivation | ||
|
||
The initial request comes from MGC to configure Redis for Limitador by the following issue [#163](https://github.com/Kuadrant/kuadrant-operator/issues/163). | ||
MGCs current work around is to update the Limitador CR after the deployment with the configuration setting for Redis Instance. |
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.
MGCs current work around is to update the Limitador CR after the deployment with the configuration setting for Redis Instance. | |
MGC's current work around is to update the Limitador CR after the deployment with the configuration setting for Redis Instance. |
This change would allow for the configuration sub components before the Kuadrant is deployed. | ||
|
||
As a kuadrant user this reduces the number of CRs that they are required to modify to get the installation they require. | ||
The sub components CRDs (Authorino, Limitador) never have to be modified by a Kuadrant user (and should never be modified by a Kuadrant User). |
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.
The sub components CRDs (Authorino, Limitador) never have to be modified by a Kuadrant user (and should never be modified by a Kuadrant User). | |
The sub components CRs (Authorino, Limitador) never have to be modified by a Kuadrant user (and should never be modified by a Kuadrant User). |
|
||
The initial request comes from MGC to configure Redis for Limitador by the following issue [#163](https://github.com/Kuadrant/kuadrant-operator/issues/163). | ||
MGCs current work around is to update the Limitador CR after the deployment with the configuration setting for Redis Instance. | ||
This change would allow for the configuration sub components before the Kuadrant is deployed. |
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.
This change would allow for the configuration sub components before the Kuadrant is deployed. | |
This change would allow for the configuration of sub components before the Kuadrant is deployed. |
MGCs current work around is to update the Limitador CR after the deployment with the configuration setting for Redis Instance. | ||
This change would allow for the configuration sub components before the Kuadrant is deployed. | ||
|
||
As a kuadrant user this reduces the number of CRs that they are required to modify to get the installation they require. |
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.
As a kuadrant user this reduces the number of CRs that they are required to modify to get the installation they require. | |
This reduces the number of CRs that users of Kuadrant are required to modify to get the installation they require. |
- tracing | ||
|
||
#### Application Settings | ||
- eveluatorCacheSize |
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.
- eveluatorCacheSize | |
- evaluatorCacheSize |
#### Unsupported | ||
- clusterWide | ||
- authConfigLabelSelectors | ||
- secrtLabelSelectors |
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.
- secrtLabelSelectors | |
- secretLabelSelectors |
If a user wishes to use a different image, they can. | ||
Kuadrant assumes they know what they are doing but requires the user to set the change on the component directly. | ||
|
||
Only the sub component operator will be responsible for actioning the configurations pasted form the Kuadrant CR to the sub components CR. |
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.
Only the sub component operator will be responsible for actioning the configurations pasted form the Kuadrant CR to the sub components CR. | |
Only the sub component operator will be responsible for actioning the configurations pasted from the Kuadrant CR to the sub components CR. |
Fields being reconcile can be classified into different groups. | ||
These classifications are based around the tasks a user is achieve. | ||
|
||
- Kubernetes native, setting that affect how Kubernetes handles the resource. |
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.
I wonder about labels. Are those in this group? Or maybe they could be propagated from the labels of the Kuadrant CR itself. If this is the case, are labels out of scope of the RFC?
The ability to set labels in the components' CRs can be useful for selecting not only these custom resources in particular, but also other resources that the corresponding component operator creates based on those. This is the case of Authorino, whose operator creates services propagating the same labels applied to the Authorino CR, including, e.g., Authorino's metrics service, later used by the scraper to find the service.
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.
Labels are interesting, and would be out of scope of the RFC. This RFC is focusing on the spec of CRs.
If the labels were to be managed you would also need to take into account other updater's of the labels on the resources. The source of truth for the labels becomes a bit muddy.
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.
One thing is for sure. We need a way for users to be able to propagate labels across resources initially created by our controllers.
That said, I can live with labels being out of the scope of this RFC. It should be clear then for one who implements this RFC that reconciling the resources back to the desired state declared in the Kuadrant
CR should NOT include the labels (at least not because of this RFC.)
I am happy to approve as long as we give careful consideration to each value we choose to surface as you mention @Boomatang it is easier to add more than to have to start depreciating and removing |
resourceRequirements: ... | ||
storage: ... | ||
authorino: | ||
eveluatorCacheSize: ... |
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.
eveluatorCacheSize: ... | |
evaluatorCacheSize: ... |
Address needs of Kuadrant/kuadrant-operator#163
Related issue: Kuadrant/kuadrant-operator#164