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

rfc: configuration of kuadrant sub components #25

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

Boomatang
Copy link
Contributor

@Boomatang Boomatang commented Sep 11, 2023

@alexsnaps
Copy link
Member

This might be of interest to this:
Kuadrant/limitador-operator#96

@eguzki eguzki mentioned this pull request Sep 12, 2023
@Boomatang Boomatang force-pushed the rfc/configuration_of_kuadrant_sub_components branch from dd8da75 to 155ea6e Compare September 19, 2023 09:16
@Boomatang Boomatang requested a review from a team September 28, 2023 14:05
@Boomatang Boomatang force-pushed the rfc/configuration_of_kuadrant_sub_components branch from 155ea6e to e8ce534 Compare October 20, 2023 10:03
@Boomatang Boomatang added the status/FCP Final Comment Period label Nov 9, 2023
#### Kubernetes native
- replicas

#### Observability
Copy link
Collaborator

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 ?

Copy link
Collaborator

@maleck13 maleck13 left a 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)

@Boomatang
Copy link
Contributor Author

@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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Feature Name: Configuration of Kuadrant Sub components.
- Feature Name: `sub-components-config`

Copy link
Contributor Author

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?

Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- eveluatorCacheSize
- evaluatorCacheSize

#### Unsupported
- clusterWide
- authConfigLabelSelectors
- secrtLabelSelectors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.)

@maleck13
Copy link
Collaborator

maleck13 commented Nov 14, 2023

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: ...
Copy link

Choose a reason for hiding this comment

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

Suggested change
eveluatorCacheSize: ...
evaluatorCacheSize: ...

@Boomatang Boomatang merged commit bfa229c into main Nov 16, 2023
1 check passed
@eguzki eguzki deleted the rfc/configuration_of_kuadrant_sub_components branch November 16, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments status/FCP Final Comment Period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants