-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: Filter managed KongPluginBinding in finalizer reconciler #802
fix: Filter managed KongPluginBinding in finalizer reconciler #802
Conversation
c5f5119
to
399e35f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to cover this in an envtest?
@@ -237,6 +243,14 @@ func (r *KonnectEntityPluginBindingFinalizerReconciler[T, TEnt]) getKongPluginBi | |||
} | |||
} | |||
|
|||
// isManagedKongPluginBinding returns true if the KongPluginBinding is managed by the controller. | |||
// REVIEW: Here we judge whether KongPluginBinding is managed by checking its owner references and treat it as managed if it has a plugin as owner reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me.
// REVIEW: Here we judge whether KongPluginBinding is managed by checking its owner references and treat it as managed if it has a plugin as owner reference. | |
// We judge whether KongPluginBinding is managed by checking its owner references and treat it as managed if it has a plugin as owner reference. |
Current envtest do the reverse. They create unmanaged plugin bindings and check for finalizers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should filter out the unmanaged KongPluginBinding
s earlier so that they are not even reconciled by this controller.
I'd suggest something like:
diff --git a/controller/konnect/reconciler_generic_pluginbindingfinalizer.go b/controller/konnect/reconciler_generic_pluginbindingfinalizer.go
index ac1d7526..8cbe4879 100644
--- a/controller/konnect/reconciler_generic_pluginbindingfinalizer.go
+++ b/controller/konnect/reconciler_generic_pluginbindingfinalizer.go
@@ -4,7 +4,9 @@ import (
"context"
"fmt"
+ "github.com/samber/lo"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -64,6 +66,14 @@ func (r *KonnectEntityPluginBindingFinalizerReconciler[T, TEnt]) SetupWithManage
return b.Complete(r)
}
+func ownerRefIsAnyKongPlugin() func(ownerRef metav1.OwnerReference) bool {
+ return func(ownerRef metav1.OwnerReference) bool {
+ return ownerRef.Kind == "KongPlugin" ||
+ // NOTE: We currently do not support KongClusterPlugin, but we keep this here for future use.
+ ownerRef.Kind == "KongClusterPlugin"
+ }
+}
+
// enqueueObjectReferencedByKongPluginBinding watches for KongPluginBinding objects
// that reference the given Konnect entity.
// It returns the reconcile.Request slice containing the entity that the KongPluginBinding references.
@@ -74,6 +84,11 @@ func (r *KonnectEntityPluginBindingFinalizerReconciler[T, TEnt]) enqueueObjectRe
return nil
}
+ // If the KongPluginBinding is unmanaged (created not using an annotation), skip it, do not delete it.
+ if !lo.ContainsBy(kpb.OwnerReferences, ownerRefIsAnyKongPlugin()) {
+ return nil
+ }
+
var (
name string
e T
@@ -125,6 +140,9 @@ func (r *KonnectEntityPluginBindingFinalizerReconciler[T, TEnt]) enqueueObjectRe
}
// Reconcile reconciles the Konnect entity that can be set as KongPluginBinding target.
+// It reconciles only entities that are referenced by managed KongPluginBindings,
+// i.e. those that are created by the controller out of konghq.com/plugins annotations.
+//
// Its purpose is to:
// - check if the entity is marked for deletion and mark KongPluginBindings
// that reference it.
WDYT?
Tao's right. https://github.com/Kong/gateway-operator/blob/e3cd706111df252197188ae89157fd811bab1364/test/envtest/kongplugincleanupfinalizer_test.go should use the WDYT? |
Solved in #805 |
What this PR does / why we need it:
Only add finalizers to target objects and remove
KongPluginBinding
on removal of targets for managedKongPluginBinding
s.Which issue this PR fixes
Fixes #786
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect significant changes