Reconcile Sub Component Limitador CR #350

Boomatang · 2023-12-01T09:48:12Z

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

The changes here allow a user to configure the setting in the Limitador CR from the kuadrant CR.

Verification

Deploy this branch using make local-setup.
Wait for deploy to complete and all resources to be a ready state.
Deploy the custom manifest.
- kubectl apply -f https://raw.githubusercontent.com/Boomatang/k8s_configs/kuadrant-operator-PR350/manifest/kuadrant_spec_limitador.yaml
- The custom manifest will deploy an instance of redis along with all the resources to test the different configurations.
- The Kuadrant CR has every spec that this PR allows configuring set.
- The manifest is linked to a tag kuadrant-operator-PR350 to ensure state does not change.
Wait of deployment to complete
The spec that is being reconciled are listed in the RFC: Limitador Spec
Editing the spec directly in the Limitador CR should be reverted to the spec in the Kuadrant CR
Editing the spec in the Kuadrant CR should update the spec in the Limitador CR
- NOTE: fully removing a spec from the kuadrant CR will not remove it from the Limitador CR. This is do to allow upgrades. A user may have custom spec already in the Limitador CR. This change only overrides the Limitador CR spec if that spec is defined in Kuadrant CR.

Use Case:

Existing Limitador Users

It is possible that a user can be using limitador as a standalone product before using kuadrant. In the case where the user named the limitador CR limitador and places the kuadrant CR in the same namespace, the kuadrant operator will take ownership of the limitador CR. This will be done by setting the ownerReference in the limitador CR.

Validating this use case.

Follow the verification steps to get the environment set up.
Scale down the kuadrant operator
Edit the limitador CR
- Set spec as follows
```
spec:
  replicas: 1
```
- Remove the ownerReference block from the metadata.
Scale back up the kuadrant operator
Expected:
- The spec switches to that defined in the kuadrant CR
- The ownerReference is set to the kuadrant operator

codecov · 2023-12-01T09:50:20Z

Codecov Report

Merging #350 (1957813) into main (c9e8173) will increase coverage by 0.33%.
Report is 2 commits behind head on main.
The diff coverage is 69.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
+ Coverage   65.69%   66.03%   +0.33%     
==========================================
  Files          37       38       +1     
  Lines        3842     3901      +59     
==========================================
+ Hits         2524     2576      +52     
- Misses       1127     1135       +8     
+ Partials      191      190       -1

Flag	Coverage Δ
integration	`70.95% <52.50%> (+0.18%)`	⬆️
unit	`60.69% <88.88%> (+0.55%)`	⬆️

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

Components	Coverage Δ
api/v1beta1 (u)	`∅ <ø> (∅)`
pkg/common (u)	`78.59% <ø> (ø)`
pkg/istio (u)	`37.11% <ø> (ø)`
pkg/log (u)	`31.81% <ø> (ø)`
pkg/reconcilers (u)	`33.44% <ø> (ø)`
pkg/rlptools (u)	`56.46% <ø> (ø)`
controllers (i)	`70.95% <52.50%> (+0.18%)`	⬆️

Files	Coverage Δ
pkg/kuadranttools/limitador_tools.go	`88.88% <88.88%> (ø)`
controllers/kuadrant_controller.go	`50.77% <52.50%> (-1.56%)`	⬇️

... and 7 files with indirect coverage changes

guicassolato · 2024-01-08T09:43:15Z

api/v1beta1/kuadrant_types.go

@@ -29,6 +31,26 @@ import (

 // KuadrantSpec defines the desired state of Kuadrant
 type KuadrantSpec struct {
+	// +optional
+	Limitador *LimitadorSpec `json:"limitador,omitempty"`


I suppose we don't want the entire limitadorv1alpha1.LimitadorSpec type here, right?

Correct. Some spec may be forced by the operator and not user editable. While others are classified as dev/test and should not be easily set in the kuadrant operator.

KevFan · 2024-01-15T11:57:27Z

@Boomatang Can you rebase with main? Running make local-setup is failing with:

The Istio "istiocontrolplane" is invalid: spec.namespace: Required value
make[4]: *** [sail-install] Error 1
make[3]: *** [istio-install] Error 2
make[2]: *** [local-cluster-setup] Error 2
make[1]: *** [local-env-setup] Error 2
make: *** [local-setup] Error 2

KevFan · 2024-01-15T14:12:58Z

controllers/kuadrant_controller.go

+	if limitador.OwnerReferences != nil && !reflect.DeepEqual(tmp.OwnerReferences, limitador.OwnerReferences) {
+		return nil
 	}

-	err := r.SetOwnerReference(kObj, limitador)
+	err = r.SetOwnerReference(kObj, limitador)


Is this so that if there is an owner reference set on limitador (e.g. not created / managed by kuadrant), then the limitador spec in Kuadrant CR will not be reconciled to the Limitador CR ? 🤔

So, in the case of ownerReference is nil (e.g. I just create the Limitador CR manually), the spec defined, if set, in Kuadrant CR will be reconciled to the Limitador CR. Is this the expected behaviour ? 🤔

One weird thing from this setup, unless I'm missing something, is that I expect the ownerRefence to be set to the Limitador CR in this case, but it isn't on my setup, everything else is reconciled correctly 🤷

There is a bug here in my mutate function, working on the fix now, but should explain what is going on here.

Firstly the Kuadrant operator expects the limitador CR to be called limitador and in the same namespace as the Kuadrant CR. The code will not reconcile any limitador resources of a different name or in different namespace.

The reflect.DeepEqual on line 527 is me being extra safe. I would only expect that to true if the user has its own custom controller that is managing the limitador CR. I could see this being a thing if the user was to use limitador as standalone product before using kuadrant.

So the bug is in the setting of the owner reference if the user manually creates the limitador CR in the same namespace as the kuadrant CR when the kuadrant operator is scaled down for example. Or the real world use case would be if the user was using the limitador as a standalone product and then starts to use kuadrant. In this case you would expect kuadrant to take ownership of the limitador CR.

Yeah, this is an edge case. I guess to me, if I were to use limitador standalone first (using the limitador name) in the kuadrant namespace, having kuadrant take ownership of the CR afterwards may not necessary be expected, as this limitador could be used by other services 🤔 Deleting the kuadrant CR, will then delete this limitador instance, potentially affecting other services using this limitador instance 🤔

Anyhow, just a thought, I don't mind as long as this is expected flow, though maybe best documented somewhere that this will happen at least.

In the case where the limitador CR is managed by another custom controller (due to different owner references), I think a log or a even better a status on the kuadrant CR noting this would be nice, since it is not obvious to the user that the limitador spec defined in the Kuadrant CR will not be reconciled due to this

I agree with Kevin here, and to add a bit more, it could be a common scenario to have a limitador instance in the cluster with its default name limitador, thus Kuadrant should not take ownership if not explicitly declared in the Kuadrant CR.

I agree it could be common to have any existing limitador CRs called limitador on the cluster. But let me ask this question. If you have a limitador instance that you do not want kuadrant to manage, why would you create the kuadrant CR in the same namespace as the existing limitador CR? Next question I would ask, can limitador work with two limitador CRs in the same namespace?

KevFan

Looks good generally 👍 Have 2 comments 🧑‍💻

KevFan · 2024-01-17T10:32:36Z

controllers/kuadrant_controller.go

+	tmp := &limitadorv1alpha1.Limitador{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      common.LimitadorName,
+			Name:      "temp",
 			Namespace: kObj.Namespace,
 		},
-		Spec: limitadorv1alpha1.LimitadorSpec{
-			RateLimitHeaders: &[]limitadorv1alpha1.RateLimitHeadersType{limitadorv1alpha1.RateLimitHeadersTypeDraft03}[0],
-			Telemetry:        &[]limitadorv1alpha1.Telemetry{limitadorv1alpha1.TelemetryExhaustive}[0],
-		},
+	}
+	err = r.SetOwnerReference(kObj, tmp)
+	if err != nil {
+		return err
+	}
+
+	if limitador.OwnerReferences != nil && !reflect.DeepEqual(tmp.OwnerReferences, limitador.OwnerReferences) {
+		logger.Error(fmt.Errorf("limitador CR owned by different controller: %v", limitador.OwnerReferences[0].Name), "check limitador CR ownerReference")
+		return nil
 	}

-	err := r.SetOwnerReference(kObj, limitador)
+	err = r.SetOwnerReference(kObj, limitador)
 	if err != nil {
 		return err
 	}


I was looking at controllerutil SetControllerReference method, it seems to return an AlreadyOwnedError in the case where the obj is already owned by another controller.

https://github.com/kubernetes-sigs/controller-runtime/blob/v0.17.0/pkg/controller/controllerutil/controllerutil.go#L85-L87

So I wonder can this be simplified to this without needing to have a tmp obj here:

err = r.SetOwnerReference(kObj, limitador) if err != nil { var alreadyOwnedError *controllerutil.AlreadyOwnedError if errors.As(err, &alreadyOwnedError) { logger.Error(alreadyOwnedError, "check limitador CR ownerReference") return nil } return err }

Also, can you add a unit test here to test this behaviour ?

KevFan · 2024-01-17T10:51:10Z

controllers/kuadrant_status.go

@@ -94,7 +95,7 @@ func (r *KuadrantReconciler) readyCondition(ctx context.Context, kObj *kuadrantv
 		return cond, nil
 	}

-	reason, err := r.checkLimitadorReady(ctx, kObj)
+	reason, err := r.checkLimitador(ctx, kObj)


In the case of differing owner ref, condition will be set to LimitadorNotReady with message Unable to reconcile limitador CR, Own Reference belongs to different controller.

Do we view Kuadrant is ready or not in this state ? All components could be up and Limitador is technically ready in the "unmanaged state" but since the resource is owned by another controller, there is really nothing we can do 🤔 Though i realise this this a nitpick on an edge case 🤷

KevFan

Verified and code looks good to me as we've decided to have a follow up ticket instead on if the edge case of the limitador used is owned by another custom controller 👍

There's failing integration tests, but not sure it related to this PR 🤷

Boomatang · 2024-01-19T11:03:58Z

The failing integration tests where cause by the edge case that was found during this time. Where a second kuadrant CR was trying to take ownership of the limitador CR, which was owned by the first kuadrant CR. In this case the integration tests required a small refactor.

didierofrivia · 2024-01-22T14:51:20Z

I'm getting the following error when running make local-setup

error: no objects passed to apply
make[4]: *** [sail-install] Error 1
make[3]: *** [istio-install] Error 2
make[2]: *** [local-cluster-setup] Error 2
make[1]: *** [local-env-setup] Error 2
make: *** [local-setup] Error 2

Boomatang · 2024-01-22T15:00:32Z

@didierofrivia The istio-operator removed the manifests on Friday. Its an error on related to these changes. You can try ISTIO_INSTALL_SAIL=false make local-setup, it worked for me on a different PR this morning.

maistra/istio-operator#1576

adam-cattermole · 2024-01-22T15:00:55Z

I'm getting the following error when running make local-setup

error: no objects passed to apply
make[4]: *** [sail-install] Error 1
make[3]: *** [istio-install] Error 2
make[2]: *** [local-cluster-setup] Error 2
make[1]: *** [local-env-setup] Error 2
make: *** [local-setup] Error 2

This error is from sail (again) #400, addressed partially in #395. For now you can run with ISTIO_INSTALL_SAIL=false

didierofrivia

works as expected! noted regarding the edge case, and probably we might want to address limitador instance naming too in that following issue (?)
Good job! 🎖️

Boomatang requested a review from a team as a code owner December 1, 2023 09:48

Boomatang added the kind/enhancement New feature or request label Dec 1, 2023

Boomatang marked this pull request as draft December 1, 2023 09:48

Boomatang force-pushed the sub_component/limitador branch from 918f2a9 to 76290da Compare December 5, 2023 10:03

alexsnaps assigned Boomatang Dec 5, 2023

Boomatang force-pushed the sub_component/limitador branch from 76290da to 5b43903 Compare December 6, 2023 10:29

Boomatang marked this pull request as ready for review December 6, 2023 16:12

alexsnaps added this to the v0.6.0 milestone Dec 8, 2023

Boomatang force-pushed the sub_component/limitador branch from 5b43903 to 94bb3f3 Compare December 11, 2023 11:28

alexsnaps modified the milestones: v0.6.0, v0.7.0 Dec 13, 2023

Boomatang force-pushed the sub_component/limitador branch from 3a887fc to 6c45659 Compare December 14, 2023 12:50

guicassolato reviewed Jan 8, 2024

View reviewed changes

Add limitador types

86ba9c5

Boomatang force-pushed the sub_component/limitador branch from 6c45659 to c1ca4ec Compare January 15, 2024 12:03

KevFan reviewed Jan 15, 2024

View reviewed changes

Boomatang force-pushed the sub_component/limitador branch from ed429c6 to c029047 Compare January 16, 2024 16:42

KevFan reviewed Jan 17, 2024

View reviewed changes

Reconcile limitador spec from kuadrant CR

f428dc4

Boomatang force-pushed the sub_component/limitador branch from c029047 to f428dc4 Compare January 17, 2024 12:27

KevFan approved these changes Jan 17, 2024

View reviewed changes

Fix Integration Test

1957813

Boomatang mentioned this pull request Jan 19, 2024

Existing Ownership of sub resoucres [Edge Case] #397

Open

pehala mentioned this pull request Jan 22, 2024

Add test for settings Limitador configuration from Kuadrant CR Kuadrant/testsuite#326

Closed

didierofrivia approved these changes Jan 22, 2024

View reviewed changes

Boomatang merged commit 624f58b into main Jan 23, 2024
19 checks passed

Boomatang mentioned this pull request Jan 23, 2024

Reconcile Spec to sub components #307

Open

2 tasks

eguzki deleted the sub_component/limitador branch January 29, 2024 17:15

jsmolar mentioned this pull request May 6, 2024

Limitador CR should reconcile to default values for fields that are not defiend #617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile Sub Component Limitador CR #350

Reconcile Sub Component Limitador CR #350

Boomatang commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

guicassolato Jan 8, 2024

Boomatang Jan 10, 2024

KevFan commented Jan 15, 2024

KevFan Jan 15, 2024

Boomatang Jan 16, 2024

KevFan Jan 16, 2024

didierofrivia Jan 22, 2024 •

edited

Loading

Boomatang Jan 22, 2024

KevFan left a comment •

edited

Loading

KevFan Jan 17, 2024 •

edited

Loading

KevFan Jan 17, 2024 •

edited

Loading

KevFan left a comment

Boomatang commented Jan 19, 2024

didierofrivia commented Jan 22, 2024

Boomatang commented Jan 22, 2024

adam-cattermole commented Jan 22, 2024

didierofrivia left a comment

Reconcile Sub Component Limitador CR #350

Reconcile Sub Component Limitador CR #350

Conversation

Boomatang commented Dec 1, 2023 • edited Loading

Verification

Use Case:

Existing Limitador Users

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

guicassolato Jan 8, 2024

Choose a reason for hiding this comment

Boomatang Jan 10, 2024

Choose a reason for hiding this comment

KevFan commented Jan 15, 2024

KevFan Jan 15, 2024

Choose a reason for hiding this comment

Boomatang Jan 16, 2024

Choose a reason for hiding this comment

KevFan Jan 16, 2024

Choose a reason for hiding this comment

didierofrivia Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Boomatang Jan 22, 2024

Choose a reason for hiding this comment

KevFan left a comment • edited Loading

Choose a reason for hiding this comment

KevFan Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

KevFan Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

Boomatang commented Jan 19, 2024

didierofrivia commented Jan 22, 2024

Boomatang commented Jan 22, 2024

adam-cattermole commented Jan 22, 2024

didierofrivia left a comment

Choose a reason for hiding this comment

Boomatang commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

didierofrivia Jan 22, 2024 •

edited

Loading

KevFan left a comment •

edited

Loading

KevFan Jan 17, 2024 •

edited

Loading

KevFan Jan 17, 2024 •

edited

Loading