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.

Sign up for GitHub

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

Propagating Limitador's env vars #22

Merged
merged 2 commits into from
Aug 8, 2022
Merged

Propagating Limitador's env vars #22

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a49a69c
[controller] Propagating Limitador's env vars
didierofrivia May 31, 2022
52402f4
[controller] Patching the (Cluster)RoleBinding to match Kuadrant NS
didierofrivia Jul 28, 2022
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
56 changes: 48 additions & 8 deletions controllers/kuadrant_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ import (
"encoding/json"
"errors"
"fmt"

"github.com/go-logr/logr"
authorinov1beta1 "github.com/kuadrant/authorino-operator/api/v1beta1"
limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1"
istioapiv1alpha1 "istio.io/api/operator/v1alpha1"
iopv1alpha1 "istio.io/istio/operator/pkg/apis/istio/v1alpha1"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -43,8 +44,14 @@ import (
)

const (
kuadrantFinalizer = "kuadrant.kuadrant.io/finalizer"
extAuthorizerName = "kuadrant-authorization"
kuadrantFinalizer = "kuadrant.kuadrant.io/finalizer"
extAuthorizerName = "kuadrant-authorization"
envLimitadorNamespace = "LIMITADOR_NAMESPACE"
envLimitadorName = "LIMITADOR_NAME"
)

var (
limitadorName = common.FetchEnv(envLimitadorName, "limitador")
)

// KuadrantReconciler reconciles a Kuadrant object
Expand Down Expand Up @@ -344,7 +351,7 @@ func (r *KuadrantReconciler) reconcileLimitador(ctx context.Context, kObj *kuadr
APIVersion: "limitador.kuadrant.io/v1alpha1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "limitador",
Name: limitadorName,
Namespace: kObj.Namespace,
},
Spec: limitadorv1alpha1.LimitadorSpec{},
Expand Down Expand Up @@ -389,14 +396,47 @@ func (r *KuadrantReconciler) createOnlyInKuadrantNSCb(ctx context.Context, kObj
return err
}

k8sObjKind := k8sObj.DeepCopyObject().GetObjectKind()
var newObj client.Object
newObj = k8sObj
Copy link
Member

Choose a reason for hiding this comment

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

I must be missing something here, but there is a lot of copying going on here.
Is the switch obj := k8sObj.(type) a copy itself? Trying to understand… but I wonder why not use the default: if a "default" copy has to happen? That's if they are all different… I guess I need to refresh my golang

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, it's a bit misleading the default case, its block will be executed if none of the other cases match.

Copy link
Member Author

Choose a reason for hiding this comment

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

switch obj := k8sObj.(type) will only live within the switch block, so we need to copy it to a different var.

I'd love the swith case to include a finally clause, that will be executed at the very end, thus we could avoid a lot of repetition


switch obj := k8sObj.(type) {
case *appsv1.Deployment: // If it's a Deployment obj, it adds the required env vars
obj.Spec.Template.Spec.Containers[0].Env = append(
obj.Spec.Template.Spec.Containers[0].Env,
v1.EnvVar{Name: envLimitadorNamespace, Value: kObj.Namespace},
v1.EnvVar{Name: envLimitadorName, Value: limitadorName},
)
newObj = obj
// TODO: DRY the following 2 case switches
case *rbacv1.RoleBinding:
if obj.Name == "kuadrant-leader-election-rolebinding" {
for i, subject := range obj.Subjects {
if subject.Name == "kuadrant-controller-manager" {
obj.Subjects[i].Namespace = kObj.Namespace
}
}
}
newObj = obj
case *rbacv1.ClusterRoleBinding:
if obj.Name == "kuadrant-manager-rolebinding" {
for i, subject := range obj.Subjects {
if subject.Name == "kuadrant-controller-manager" {
obj.Subjects[i].Namespace = kObj.Namespace
}
}
}
newObj = obj
default:
Copy link
Member

Choose a reason for hiding this comment

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

This some code style we're following?

}
newObjCloned := newObj.DeepCopyObject()
err = r.Client().Create(ctx, newObj)

err = r.Client().Create(ctx, k8sObj)
logger.V(1).Info("create resource", "GKV", k8sObjKind.GroupVersionKind(), "name", k8sObj.GetName(), "error", err)
k8sObjKind := newObjCloned.GetObjectKind()
logger.V(1).Info("create resource", "GKV", k8sObjKind.GroupVersionKind(), "name", newObj.GetName(), "error", err)
if err != nil {
if apierrors.IsAlreadyExists(err) {
// Omit error
logger.Info("Already exists", "GKV", k8sObjKind.GroupVersionKind(), "name", k8sObj.GetName())
logger.Info("Already exists", "GKV", k8sObjKind.GroupVersionKind(), "name", newObj.GetName())
} else {
return err
}
Expand Down