Reconcile Sub Component Authorino CR #393

Boomatang · 2024-01-11T14:02:09Z

Part of: #307
RFC: https://github.com/Kuadrant/architecture/blob/main/rfcs/0006-kuadrant_sub_components_configurations.md

These changes allow a user to configure settings in the Authorino CR from the Kuadrant CR.

Verification

Deploy this branch using make local-setup.
Wait for deployment to complete and all resources to be a ready state.
Deploy the custom resources.
- kubectl apply -f https://raw.githubusercontent.com/Boomatang/k8s_configs/kuadrant-operator-PR393/manifest/kuadrant_spec_authorino.yaml
- The custom manifest will deploy all the resources required for testing including an instance of Jaeger for OpenTelemetry. Jaeger needs a number of resources set up so the deployment will fail on the first run but rerun the command again to get all resources installed.
- The Kuadrant CR has every spec that this PR allows configuring set.
- The manifest is linked to a tag kuadrant-operator-PR393 to ensure state does not change.
Wait for the deployment to complete
The spec that is being reconciled are listed in the RFC: Authorino Spec
Editing the spec directly in the Authorino CR should be reverted to the spec in the Kuadrant CR
Editing the spec in the Kuadrant CR should update the spec in the Authorino CR
- NOTE: fully removing a spec from the kuadrant CR will not remove it from the Authorino CR. This is done to allow upgrades. A user may have custom spec already in the Limitador CR. This change only overrides the Authorino CR spec if that spec is defined in Kuadrant CR.

codecov · 2024-01-11T14:05:35Z

Codecov Report

Attention: Patch coverage is 72.63158% with 26 lines in your changes missing coverage. Please review.

Project coverage is 69.26%. Comparing base (ece13e8) to head (eb909a5).
Report is 167 commits behind head on main.

Files	Patch %	Lines
controllers/kuadrant_controller.go	50.00%	24 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (ece13e8) and HEAD (eb909a5). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (ece13e8) HEAD (eb909a5)

integration 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #393       +/-   ##
===========================================
- Coverage   80.20%   69.26%   -10.94%     
===========================================
  Files          64       77       +13     
  Lines        4492     5968     +1476     
===========================================
+ Hits         3603     4134      +531     
- Misses        600     1512      +912     
- Partials      289      322       +33

Flag	Coverage Δ
bare-k8s-integration	`4.51% <0.00%> (?)`
gatewayapi-integration	`10.93% <45.26%> (?)`
integration	`?`
istio-integration	`56.42% <45.26%> (?)`
unit	`33.38% <45.26%> (+3.35%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	`71.42% <ø> (ø)`
api/v1beta2 (u)	`90.85% <90.62%> (-0.58%)`	⬇️
pkg/common (u)	`79.66% <ø> (-9.17%)`	⬇️
pkg/istio (u)	`73.88% <ø> (-0.03%)`	⬇️
pkg/log (u)	`94.73% <ø> (ø)`
pkg/reconcilers (u)	`∅ <ø> (∅)`
pkg/rlptools (u)	`83.33% <ø> (+3.87%)`	⬆️
controllers (i)	`64.27% <76.77%> (-12.53%)`	⬇️

Files	Coverage Δ
api/v1beta1/kuadrant_types.go	`71.42% <ø> (ø)`
pkg/common/common.go	`75.86% <ø> (-17.89%)`	⬇️
pkg/kuadranttools/authorino_tools.go	`100.00% <100.00%> (ø)`
controllers/kuadrant_controller.go	`56.43% <50.00%> (+2.86%)`	⬆️

... and 40 files with indirect coverage changes

adam-cattermole · 2024-01-16T14:29:13Z

controllers/kuadrant_controller.go

-					Enabled: &tmpFalse,
+	authorinoKey := client.ObjectKey{Name: common.AuthorinoName, Namespace: kObj.Namespace}
+	authorino := &authorinov1beta1.Authorino{}
+	err := r.Client().Get(ctx, authorinoKey, authorino)


I'm curious, do we have to do this Get? can't we generate the desired authorino CR using a default with values overridden by those set in kObj and let the mutator do the get/update on only the allowed fields?

Good question. This has to do with not overriding exist user settings if they are not defined in the kuadrant CR. An example is, the user has set the number of replicas in the Authorino CR before this feature was released. We do not want to over ride these existing setting. If we don't do the Get your questioning, the mutator function will always say there is a difference between the desired and existing. It may be possible to do all this work inside the mutator function but that would meant the desired configuration being past in would not be the whole truth as the mutator function would be able to change the desired configuration on the fly.

I believe it is better to pass the correct desired configuration to the mutator function, over allowing the mutator change the desired configuration on the fly.

Aha I see makes sense thanks, precedence is kuadrant CR > existing settings > defaults and it avoids defaults overriding existing.

I'd say we don't need to worry about rolling out changes that override user-defined configs in the Authorino and Limitador CR, especially now that those can be set at the level of the Kuadrant CR.

IMO, the controller should compute the desired state and rely on the ReconcileResource function (which dry-runs the command once to consider occasional modifications performed at the level of the API server.) Except for the differences enforced by the API server, the state computed by the controller should win over preexisting ones.

Sorry @guicassolato, I have to strongly disagree with you on this. If the user has configuration in the Authorino or Limitador CR, blank values in the Kuadrant CR should not override those values. Because if it does how does the user upgrade without bring down their service? That would be a breaking change to the Users API without any deprecation notice. I know currently today we do not have good support for upgrades but that does not mean we should break customers stuff if it can be avoided.

This behaviour follows what was outlined in the RFC. If this is unwanted behaviour a deprecation notice should be give and the behaviour removed in a later version allow user time to migrate their custom configurations from the sub components CRs to the Kuadrant CR.

For clarity, this only applies to fields the user is currently allow configure. If the Kuadrant operator requires a field to be Y after this change that field will still be Y.

It feels like what you're proposing inverts the relation between controller and controlled resources, IMO – in contrast to API Server >>> Controller >>> Controlled resources.

E.g., should we apply the same policy to Authorino AuthConfigs? One manually sets a field in a AuthConfig resource that is omitted in the AuthPolicy. Should the controller override what's in the AuthConfig or should it honour it? IMO, it should override because, in this case, the AuthPolicy is the API, not the AuthConfig. Similarly, the Kuadrant CRD is the API, not the Authorino one, nor Limitador.

I know see what you are getting at. Would you class this as a breaking change then? I would, if the user upgrades to the version here any setting that can now be configured in the Kuadrant CR would override any of the configuration in the sub component CRs. That would be introducing a breaking change without any deprecation notice and no way for the user to upgrade without bring down their service.

I would be happy if this change was added in and a deprecation notice give to say that in the future configuration of in sub components would be removed.

Yes, you're right. What I'm proposing would be a breaking change for the end-user.

adam-cattermole · 2024-01-19T11:01:24Z

pkg/kuadranttools/authorino_tools.go

+		return false, fmt.Errorf("existingObj %T is not a *authorinoauthorinov1beta1.Authorino", existingObj)
+	}
+	desired, ok := desiredObj.(*authorinov1beta1.Authorino)
+	if !ok {
+		return false, fmt.Errorf("desiredObj %T is not a *authorinoauthorinov1beta1.Authorino", desiredObj)


nit typo in log:

Suggested change

return false, fmt.Errorf("existingObj %T is not a *authorinoauthorinov1beta1.Authorino", existingObj)

}

desired, ok := desiredObj.(*authorinov1beta1.Authorino)

if !ok {

return false, fmt.Errorf("desiredObj %T is not a *authorinoauthorinov1beta1.Authorino", desiredObj)

return false, fmt.Errorf("existingObj %T is not a *authorinov1beta1.Authorino", existingObj)

}

desired, ok := desiredObj.(*authorinov1beta1.Authorino)

if !ok {

return false, fmt.Errorf("desiredObj %T is not a *authorinov1beta1.Authorino", desiredObj)

Going to blame copy and paste. Will fix.

adam-cattermole

Overall the changes here look good - just a couple things:

Do you think we should add an integration test where kuadrant CR has some authorino spec values applied?
I'm wondering if we should document the behaviour where items removed from the spec do not result in defaults in the authorino CR. I understand this behaviour is to avoid overriding existing settings but if I'm a new user of Kuadrant and all I want is to set my authorino settings through the Kuadrant CR I would find this confusing - I deploy with defaults and then override them and then remove the setting and I don't end up with defaults again.

guicassolato · 2024-01-19T11:34:58Z

controllers/kuadrant_controller.go

+	authorino := &authorinov1beta1.Authorino{}
+	err := r.Client().Get(ctx, authorinoKey, authorino)
+	if err != nil {
+		if apierrors.IsNotFound(err) {


Maybe get rid of the else clause.

Suggested change

if apierrors.IsNotFound(err) {

if !apierrors.IsNotFound(err) {

return err

}

Ya I can make this change. Personal I don't like negative conditionals and my natural instinct is not to use them.

guicassolato · 2024-01-19T11:42:32Z

controllers/kuadrant_controller.go

-			},
-		},
+				Spec: authorinov1beta1.AuthorinoSpec{
+					ClusterWide:            true,


There aren't many use cases for allowing multiple cluster-wide instances and yet do not to expose authConfigLabelSelectors. I can think of some, but in general it's not recommended.

This was part of the existing configuration. I made the assumption that the existing configuration was correct.

guicassolato · 2024-01-19T11:43:58Z

controllers/kuadrant_controller.go

+					SupersedingHostSubsets: true,
+					Listener: authorinov1beta1.Listener{
+						Tls: authorinov1beta1.Tls{
+							Enabled: &tmpFalse,


Can use ptr.To here.

This was part of the existing code. Happy to make the change as there are a few other changes needed.

guicassolato · 2024-05-06T13:55:25Z

api/v1beta1/kuadrant_types.go

@@ -19,6 +19,7 @@ package v1beta1
 import (
 	"github.com/go-logr/logr"
 	"github.com/google/go-cmp/cmp"
+	authorinov1beta1 "github.com/kuadrant/authorino-operator/api/v1beta1"


Let's make sure we use the latest version of the Authorino API here: #561

Once that PR is merged I will make sure to rebase to include it.

guicassolato · 2024-05-06T13:59:56Z

doc/reference/kuadrant.md

+| **Field** | **Type** | **Required** | **Description**                                                                                     |
+|-----------|----------|:------------:|-----------------------------------------------------------------------------------------------------|
+| endpoint  | String   |     Yes      | Full endpoint of the OpenTelemetry tracing collector service (e.g. http://jaegar:14268/api/traces). |
+| tags      | Map      |      No      | Key-value map of fixed tags to add to all OpenTelemetry traces emitted by Authorino.                |


Ref. #393 (comment)

The latest version of the Authorino API shall include another field here, insecure.

maleck13 · 2024-06-28T13:27:15Z

@Boomatang @guicassolato @adam-cattermole what is the story with this PR is it still relevant or should we close it?

Rebase to main

If the use sets any value for authorino in the Kuadrant CR all fields that can be managed are managed. This still does allow the user to set fields in the authorino CR that are out of scope of the Kuadrant CR.

Boomatang · 2024-07-09T08:37:43Z

Yes @maleck13, I do feel that this work is still relevant, it is part of the RFC006. After me being out sick for two months priorities shifted and this work did get left behind a bit. It is now harder to get reviews on the work.

If it is decide that this work is not relevant any more then it would make sense for the RFC to be removed and the existing work done around the limitador CR to be reverted.

Boomatang · 2024-08-19T14:12:14Z

Changes in RFC design means that will not be the final approach

Boomatang requested a review from a team as a code owner January 11, 2024 14:02

Boomatang added the kind/enhancement New feature or request label Jan 11, 2024

Boomatang added this to the v0.7.0 milestone Jan 11, 2024

KevFan assigned Boomatang Jan 15, 2024

adam-cattermole reviewed Jan 16, 2024

View reviewed changes

adam-cattermole reviewed Jan 19, 2024

View reviewed changes

Boomatang mentioned this pull request Jan 19, 2024

Existing Ownership of sub resoucres [Edge Case] #397

Open

guicassolato reviewed Jan 19, 2024

View reviewed changes

Boomatang mentioned this pull request Jan 22, 2024

fix: bump OpenTelemetry Schema URL to 1.21.0 Kuadrant/authorino#455

Merged

pehala mentioned this pull request Jan 22, 2024

Add test for settings Authorino configuration from Kuadrant CR Kuadrant/testsuite#327

Closed

Boomatang force-pushed the sub_component/authorino branch from 2863177 to ec7edf2 Compare January 23, 2024 11:34

Boomatang mentioned this pull request Jan 23, 2024

Reconcile Spec to sub components #307

Open

2 tasks

Boomatang force-pushed the sub_component/authorino branch from ec7edf2 to e57a091 Compare January 26, 2024 14:29

Boomatang force-pushed the sub_component/authorino branch 2 times, most recently from 77926e7 to 401de53 Compare May 1, 2024 14:29

guicassolato reviewed May 6, 2024

View reviewed changes

Boomatang force-pushed the sub_component/authorino branch 2 times, most recently from 2f3b6f3 to a42aeec Compare May 8, 2024 13:45

Boomatang force-pushed the sub_component/authorino branch from 3885190 to a7eb7fc Compare June 5, 2024 13:28

Boomatang added 5 commits July 8, 2024 10:35

Add Authorino types

ef30130

Reconcile authorino spec from kuadrant CR

b11526c

Add API documentation

f30c216

PR checks: Update code to pass

ac31040

PR comments fix

8799593

Boomatang added 5 commits July 8, 2024 10:35

Updates for rebase

817c1d8

Rebase to main

Add integration tests.

273b9aa

Update docs for rebase.

8da1d63

Change forces control to Kuadrant.

de6e57f

If the use sets any value for authorino in the Kuadrant CR all fields that can be managed are managed. This still does allow the user to set fields in the authorino CR that are out of scope of the Kuadrant CR.

Changes required after a rebase.

eb909a5

Boomatang force-pushed the sub_component/authorino branch from a7eb7fc to eb909a5 Compare July 8, 2024 10:38

Boomatang closed this Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile Sub Component Authorino CR #393

Reconcile Sub Component Authorino CR #393

Boomatang commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

adam-cattermole Jan 16, 2024 •

edited

Loading

Boomatang Jan 17, 2024

adam-cattermole Jan 19, 2024 •

edited

Loading

guicassolato Jan 19, 2024

Boomatang Jan 19, 2024

guicassolato Jan 23, 2024

Boomatang Jan 23, 2024

guicassolato Jan 23, 2024

guicassolato May 6, 2024

adam-cattermole Jan 19, 2024

Boomatang Jan 19, 2024

adam-cattermole left a comment •

edited

Loading

guicassolato Jan 19, 2024

Boomatang Jan 19, 2024

guicassolato Jan 19, 2024

Boomatang Jan 19, 2024

guicassolato Jan 19, 2024

Boomatang Jan 19, 2024

guicassolato May 6, 2024

Boomatang May 7, 2024

guicassolato May 7, 2024

guicassolato May 6, 2024

maleck13 commented Jun 28, 2024

Boomatang commented Jul 9, 2024

Boomatang commented Aug 19, 2024

-		if apierrors.IsNotFound(err) {
+		if !apierrors.IsNotFound(err) {
+			return err
+		}

Reconcile Sub Component Authorino CR #393

Reconcile Sub Component Authorino CR #393

Conversation

Boomatang commented Jan 11, 2024 • edited Loading

Verification

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

adam-cattermole Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-cattermole Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-cattermole left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 commented Jun 28, 2024

Boomatang commented Jul 9, 2024

Boomatang commented Aug 19, 2024

Boomatang commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

adam-cattermole Jan 16, 2024 •

edited

Loading

adam-cattermole Jan 19, 2024 •

edited

Loading

adam-cattermole left a comment •

edited

Loading