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

Update limitador to 0.9.0 #608

Merged
merged 7 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ metadata:
capabilities: Basic Install
categories: Integration & Delivery
containerImage: quay.io/kuadrant/kuadrant-operator:latest
createdAt: "2024-07-05T16:23:56Z"
createdAt: "2024-07-10T15:04:27Z"
operators.operatorframework.io/builder: operator-sdk-v1.32.0
operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
repository: https://github.com/Kuadrant/kuadrant-operator
Expand Down
12 changes: 4 additions & 8 deletions bundle/manifests/kuadrant.io_kuadrants.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,10 @@ spec:
x-kubernetes-map-type: atomic
options:
properties:
batch-size:
description: 'BatchSize defines the size of entries
to flush in as single flush [default: 100]'
type: integer
flush-period:
description: 'FlushPeriod for counters in milliseconds
[default: 1000]'
Expand All @@ -991,18 +995,10 @@ spec:
description: 'MaxCached refers to the maximum amount
of counters cached [default: 10000]'
type: integer
ratio:
description: 'Ratio to apply to the TTL from Redis
on cached counters [default: 10]'
type: integer
response-timeout:
description: 'ResponseTimeout defines the timeout
for Redis commands in milliseconds [default: 350]'
type: integer
ttl:
description: 'TTL for cached counters in milliseconds
[default: 5000]'
type: integer
type: object
type: object
type: object
Expand Down
12 changes: 4 additions & 8 deletions config/crd/bases/kuadrant.io_kuadrants.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,10 @@ spec:
x-kubernetes-map-type: atomic
options:
properties:
batch-size:
description: 'BatchSize defines the size of entries
to flush in as single flush [default: 100]'
type: integer
flush-period:
description: 'FlushPeriod for counters in milliseconds
[default: 1000]'
Expand All @@ -989,18 +993,10 @@ spec:
description: 'MaxCached refers to the maximum amount
of counters cached [default: 10000]'
type: integer
ratio:
description: 'Ratio to apply to the TTL from Redis
on cached counters [default: 10]'
type: integer
response-timeout:
description: 'ResponseTimeout defines the timeout
for Redis commands in milliseconds [default: 350]'
type: integer
ttl:
description: 'TTL for cached counters in milliseconds
[default: 5000]'
type: integer
type: object
type: object
type: object
Expand Down
31 changes: 30 additions & 1 deletion controllers/kuadrant_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -408,35 +408,64 @@
}

func (r *KuadrantReconciler) reconcileLimitador(ctx context.Context, kObj *kuadrantv1beta1.Kuadrant) error {
limitador := &limitadorv1alpha1.Limitador{
TypeMeta: metav1.TypeMeta{
Kind: "Limitador",
APIVersion: "limitador.kuadrant.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: common.LimitadorName,
Namespace: kObj.Namespace,
},
Spec: limitadorv1alpha1.LimitadorSpec{},

Check warning on line 420 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L411-L420

Added lines #L411 - L420 were not covered by tests
}

if kObj.Spec.Limitador != nil {
limitador.Spec.Affinity = kObj.Spec.Limitador.Affinity
limitador.Spec.Replicas = kObj.Spec.Limitador.Replicas
limitador.Spec.Storage = kObj.Spec.Limitador.Storage
limitador.Spec.RateLimitHeaders = kObj.Spec.Limitador.RateLimitHeaders
limitador.Spec.Telemetry = kObj.Spec.Limitador.Telemetry
limitador.Spec.PodDisruptionBudget = kObj.Spec.Limitador.PodDisruptionBudget
limitador.Spec.ResourceRequirements = kObj.Spec.Limitador.ResourceRequirements
limitador.Spec.Verbosity = kObj.Spec.Limitador.Verbosity

Check warning on line 431 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L424-L431

Added lines #L424 - L431 were not covered by tests
}

err := r.SetOwnerReference(kObj, limitador)

Check warning on line 434 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L434

Added line #L434 was not covered by tests
if err != nil {
return err
}

return r.ReconcileResource(ctx, &limitadorv1alpha1.Limitador{}, limitador, kuadranttools.LimitadorMutator)
limitadorMutators := make([]kuadranttools.LimitadorMutateFn, 0)

Check warning on line 439 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L439

Added line #L439 was not covered by tests

limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorOwnerRefsMutator)

Check warning on line 441 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L441

Added line #L441 was not covered by tests

if kObj.Spec.Limitador.Affinity != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorAffinityMutator)

Check warning on line 444 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L443-L444

Added lines #L443 - L444 were not covered by tests
}
if kObj.Spec.Limitador.Replicas != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorReplicasMutator)

Check warning on line 447 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L446-L447

Added lines #L446 - L447 were not covered by tests
}
if kObj.Spec.Limitador.Storage != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorStorageMutator)

Check warning on line 450 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L449-L450

Added lines #L449 - L450 were not covered by tests
}
if kObj.Spec.Limitador.RateLimitHeaders != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorRateLimitHeadersMutator)

Check warning on line 453 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L452-L453

Added lines #L452 - L453 were not covered by tests
}
if kObj.Spec.Limitador.Telemetry != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorTelemetryMutator)

Check warning on line 456 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L455-L456

Added lines #L455 - L456 were not covered by tests
}
if kObj.Spec.Limitador.PodDisruptionBudget != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorPodDisruptionBudgetMutator)

Check warning on line 459 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L458-L459

Added lines #L458 - L459 were not covered by tests
}
if kObj.Spec.Limitador.ResourceRequirements != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorResourceRequirementsMutator)

Check warning on line 462 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L461-L462

Added lines #L461 - L462 were not covered by tests
}
if kObj.Spec.Limitador.Verbosity != nil {
limitadorMutators = append(limitadorMutators, kuadranttools.LimitadorVerbosityMutator)

Check warning on line 465 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L464-L465

Added lines #L464 - L465 were not covered by tests
}

return r.ReconcileResource(ctx, &limitadorv1alpha1.Limitador{}, limitador, kuadranttools.LimitadorMutator(limitadorMutators...))

Check warning on line 468 in controllers/kuadrant_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/kuadrant_controller.go#L468

Added line #L468 was not covered by tests
}

func (r *KuadrantReconciler) reconcileAuthorino(ctx context.Context, kObj *kuadrantv1beta1.Kuadrant) error {
Expand Down
86 changes: 0 additions & 86 deletions controllers/suite_test.go

This file was deleted.

4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ require (
github.com/kuadrant/authorino v0.17.2
github.com/kuadrant/authorino-operator v0.11.1
github.com/kuadrant/dns-operator v0.0.0-20240627145308-8a571eaae927
github.com/kuadrant/limitador-operator v0.8.0
github.com/kuadrant/limitador-operator v0.9.0
github.com/martinlindhe/base36 v1.1.1
github.com/onsi/ginkgo/v2 v2.13.2
github.com/onsi/gomega v1.30.0
Expand All @@ -28,7 +28,7 @@ require (
k8s.io/apimachinery v0.28.4
k8s.io/client-go v0.28.4
k8s.io/klog/v2 v2.110.1
k8s.io/utils v0.0.0-20231127182322-b307cd553661
k8s.io/utils v0.0.0-20240310230437-4693a0247e57
maistra.io/istio-operator v0.0.0-20231214211859-76e404c8df41
sigs.k8s.io/controller-runtime v0.16.3
sigs.k8s.io/external-dns v0.14.0
Expand Down
8 changes: 4 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ github.com/kuadrant/authorino-operator v0.11.1 h1:jndTZhiHMU+2Dk0NU+KP2+MUSfvclr
github.com/kuadrant/authorino-operator v0.11.1/go.mod h1:TeFFdX477vUTMushCojaHpvwPLga4DpErGI2oQbqFIs=
github.com/kuadrant/dns-operator v0.0.0-20240627145308-8a571eaae927 h1:T/pEIu+l13ecaJhCKNBlVu8YELcye4qM6m8b0u9rTbA=
github.com/kuadrant/dns-operator v0.0.0-20240627145308-8a571eaae927/go.mod h1:XquyzShT4VAew+2Kfg7zuQ1SrhbohyGgXIFZh2udTa0=
github.com/kuadrant/limitador-operator v0.7.0 h1:pLIpM6vUxAY/Jn6ny61IGpqS7Oti786duBzJ67DJOuA=
github.com/kuadrant/limitador-operator v0.7.0/go.mod h1:tg+G+3eTzUUfvUmdbiqH3FnScEPSWZ3DmorD1ZAx1bo=
github.com/kuadrant/limitador-operator v0.9.0 h1:hTQ6CFPayf/sL7cIzwWjCoU8uTn6fzWdsJgKbDlnFts=
github.com/kuadrant/limitador-operator v0.9.0/go.mod h1:DQOlg9qFOcnWPrwO529JRCMLLOEXJQxkmOes952S/Hw=
github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=
github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=
github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 h1:SOEGU9fKiNWd/HOJuq6+3iTQz8KNCLtVX6idSoTLdUw=
Expand Down Expand Up @@ -469,8 +469,8 @@ go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE=
go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A=
go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=
go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
Expand Down
133 changes: 133 additions & 0 deletions pkg/kuadranttools/limitador_mutators.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
package kuadranttools

import (
"fmt"
"reflect"

"sigs.k8s.io/controller-runtime/pkg/client"

limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"

"github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers"
)

// DeploymentMutateFn is a function which mutates the existing Deployment into it's desired state.
type LimitadorMutateFn func(desired, existing *limitadorv1alpha1.Limitador) bool

func LimitadorMutator(opts ...LimitadorMutateFn) reconcilers.MutateFn {
return func(existingObj, desiredObj client.Object) (bool, error) {
existing, ok := existingObj.(*limitadorv1alpha1.Limitador)
if !ok {
return false, fmt.Errorf("existingObj %T is not a *limitadorv1alpha1.Limitador", existingObj)
}
desired, ok := desiredObj.(*limitadorv1alpha1.Limitador)
if !ok {
return false, fmt.Errorf("desiredObj %T is not a *limitadorv1alpha1.Limitador", desiredObj)
}

update := false

// Loop through each option
for _, opt := range opts {
tmpUpdate := opt(desired, existing)
update = update || tmpUpdate
}

return update, nil
}
}

func LimitadorAffinityMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.Affinity, desired.Spec.Affinity) {
existing.Spec.Affinity = desired.Spec.Affinity
update = true

Check warning on line 44 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L43-L44

Added lines #L43 - L44 were not covered by tests
}
return update
}

func LimitadorReplicasMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false

var existingReplicas = 1

if existing.Spec.Replicas != nil {
existingReplicas = *existing.Spec.Replicas
}

var desiredReplicas = 1

if desired.Spec.Replicas != nil {
desiredReplicas = *desired.Spec.Replicas
}

if desiredReplicas != existingReplicas {
existing.Spec.Replicas = &desiredReplicas
update = true
}

return update
}

func LimitadorStorageMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.Storage, desired.Spec.Storage) {
existing.Spec.Storage = desired.Spec.Storage
update = true

Check warning on line 76 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L75-L76

Added lines #L75 - L76 were not covered by tests
}
return update
}

func LimitadorOwnerRefsMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.OwnerReferences, desired.OwnerReferences) {
existing.OwnerReferences = desired.OwnerReferences
update = true

Check warning on line 85 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L84-L85

Added lines #L84 - L85 were not covered by tests
}
return update
}

func LimitadorRateLimitHeadersMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.RateLimitHeaders, desired.Spec.RateLimitHeaders) {
existing.Spec.RateLimitHeaders = desired.Spec.RateLimitHeaders
update = true

Check warning on line 94 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L93-L94

Added lines #L93 - L94 were not covered by tests
}
return update
}

func LimitadorTelemetryMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.Telemetry, desired.Spec.Telemetry) {
existing.Spec.Telemetry = desired.Spec.Telemetry
update = true

Check warning on line 103 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L102-L103

Added lines #L102 - L103 were not covered by tests
}
return update
}

func LimitadorPodDisruptionBudgetMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.PodDisruptionBudget, desired.Spec.PodDisruptionBudget) {
existing.Spec.PodDisruptionBudget = desired.Spec.PodDisruptionBudget
update = true

Check warning on line 112 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L111-L112

Added lines #L111 - L112 were not covered by tests
}
return update
}

func LimitadorResourceRequirementsMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.ResourceRequirements, desired.Spec.ResourceRequirements) {
existing.Spec.ResourceRequirements = desired.Spec.ResourceRequirements
update = true

Check warning on line 121 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L120-L121

Added lines #L120 - L121 were not covered by tests
}
return update
}

func LimitadorVerbosityMutator(desired, existing *limitadorv1alpha1.Limitador) bool {
update := false
if !reflect.DeepEqual(existing.Spec.Verbosity, desired.Spec.Verbosity) {
existing.Spec.Verbosity = desired.Spec.Verbosity
update = true

Check warning on line 130 in pkg/kuadranttools/limitador_mutators.go

View check run for this annotation

Codecov / codecov/patch

pkg/kuadranttools/limitador_mutators.go#L129-L130

Added lines #L129 - L130 were not covered by tests
}
return update
}
Comment on lines +112 to +119
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's possible to DRY these funcs a bit, with something like...

func UpdateFieldIfNotEqual(existing, desired interface{}, fieldName string) bool {
	existingVal := reflect.ValueOf(existing).Elem().FieldByName(fieldName)
	desiredVal := reflect.ValueOf(desired).Elem().FieldByName(fieldName)

	if existingVal.IsValid() && desiredVal.IsValid() {
		if !existingVal.Equal(desiredVal) {
			existingVal.Set(desiredVal)
			return true
		}
	}
	return false
}

Since we are updating fields when are not matching the desired state, then we can populate the fields in an array and call it either in a for loop or reducer fn like...

limitadorSpecFieldNames := make(string, 0)
limitadorSpecFieldNames = append(limitadorSpecFieldNames,"Affinity")

....

for _, fieldName := range limitadorSpecFieldNames {
		if UpdateFieldIfNotEqual(existing, desired, fieldName) {
			update = true
		}
	}

not sure if we can even use the limitador spec types for the iteration instead of strings and interface{}

Maybe worth to try... maybe not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems a great idea to avoid DRY. However, I have few little issues with the proposed implementation

  • What is the semantics of that Equal method? Each field may need different comparing method.
  • The decision call of which fields needs to be "updated if not equal", belongs to the caller, not to the mutator.
  • TBH: I am not very fan of the reflection lib, only when strictly needed.

Loading
Loading