From 3b7190858030483b76c32ed59cdd052e8d8217e7 Mon Sep 17 00:00:00 2001 From: KevFan Date: Thu, 15 Feb 2024 11:53:22 +0000 Subject: [PATCH] refactor: OverriddenPolicyMap --- controllers/authpolicy_authconfig.go | 6 ++-- controllers/authpolicy_controller.go | 5 ++- controllers/authpolicy_status.go | 28 ++++----------- controllers/suite_test.go | 2 ++ main.go | 2 ++ pkg/common/apimachinery_status_conditions.go | 37 ++++++++++++++++++++ 6 files changed, 52 insertions(+), 28 deletions(-) diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 9c2f71f65..63f447e77 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -92,7 +92,7 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au if len(rules) == 0 { logger.V(1).Info("no httproutes attached to the targeted gateway, skipping authorino authconfig for the gateway authpolicy") common.TagObjectToDelete(authConfig) - r.setOverriddenPolicy(ap) + r.OverriddenPolicyMap.SetOverriddenPolicy(ap) return authConfig, nil } route = &gatewayapiv1.HTTPRoute{ @@ -103,8 +103,8 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au } } - // AuthPolicy is not overridden if code execution flow reaches here - r.removeOverriddenPolicy(ap) + // AuthPolicy is not overridden if we still need to create an AuthConfig for it + r.OverriddenPolicyMap.RemoveOverriddenPolicy(ap) // hosts authConfig.Spec.Hosts = hosts diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index 957f4f6be..5eec39b72 100644 --- a/controllers/authpolicy_controller.go +++ b/controllers/authpolicy_controller.go @@ -6,7 +6,6 @@ import ( "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -24,8 +23,8 @@ const authPolicyFinalizer = "authpolicy.kuadrant.io/finalizer" // AuthPolicyReconciler reconciles a AuthPolicy object type AuthPolicyReconciler struct { reconcilers.TargetRefReconciler - // OverriddenPolicies tracks the overridden policies to report their status. - OverriddenPolicies map[types.UID]bool + // OverriddenPolicyMap tracks the overridden policies to report their status. + OverriddenPolicyMap *common.OverriddenPolicyMap } //+kubebuilder:rbac:groups=kuadrant.io,resources=authpolicies,verbs=get;list;watch;create;update;patch;delete diff --git a/controllers/authpolicy_status.go b/controllers/authpolicy_status.go index 7e699becd..f36ce06f6 100644 --- a/controllers/authpolicy_status.go +++ b/controllers/authpolicy_status.go @@ -11,10 +11,9 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" 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/client" - v1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" @@ -89,7 +88,10 @@ func (r *AuthPolicyReconciler) enforcedCondition(ctx context.Context, policy *ap logger, _ := logr.FromContext(ctx) // Check if the policy is overridden - if r.isPolicyOverridden(policy) { + // Note: This logic assumes synchronous processing, where computing the desired AuthConfig, marking the AuthPolicy + // as overridden, and calculating the Enforced condition happen sequentially. + // Introducing a goroutine in this flow could break this assumption and lead to unexpected behavior. + if r.OverriddenPolicyMap.IsPolicyOverridden(policy) { logger.V(1).Info("Gateway Policy is overridden") return r.handleGatewayPolicyOverride(logger, policy, targetNetworkObject) } @@ -110,24 +112,6 @@ func (r *AuthPolicyReconciler) enforcedCondition(ctx context.Context, policy *ap return common.EnforcedCondition(policy, nil) } -// setOverriddenPolicy sets the overridden policy in the reconciler's tracking map. -func (r *AuthPolicyReconciler) setOverriddenPolicy(ap *api.AuthPolicy) { - if r.OverriddenPolicies == nil { - r.OverriddenPolicies = make(map[types.UID]bool) - } - r.OverriddenPolicies[ap.GetUID()] = true -} - -// removeOverriddenPolicy removes the overridden policy from the reconciler's tracking map. -func (r *AuthPolicyReconciler) removeOverriddenPolicy(ap *api.AuthPolicy) { - delete(r.OverriddenPolicies, ap.GetUID()) -} - -// isPolicyOverridden checks if the provided AuthPolicy is overridden based on the tracking map maintained by the reconciler. -func (r *AuthPolicyReconciler) isPolicyOverridden(ap *api.AuthPolicy) bool { - return r.OverriddenPolicies[ap.GetUID()] && common.IsTargetRefGateway(ap.GetTargetRef()) -} - // isAuthConfigReady checks if the AuthConfig is ready. func (r *AuthPolicyReconciler) isAuthConfigReady(ctx context.Context, policy *api.AuthPolicy) (bool, error) { apKey := client.ObjectKeyFromObject(policy) @@ -148,7 +132,7 @@ func (r *AuthPolicyReconciler) isAuthConfigReady(ctx context.Context, policy *ap // handleGatewayPolicyOverride handles the case where the Gateway Policy is overridden by filtering policy references // and creating a corresponding error condition. func (r *AuthPolicyReconciler) handleGatewayPolicyOverride(logger logr.Logger, policy *api.AuthPolicy, targetNetworkObject client.Object) *metav1.Condition { - obj := targetNetworkObject.(*v1.Gateway) + obj := targetNetworkObject.(*gatewayapiv1.Gateway) gatewayWrapper := common.GatewayWrapper{Gateway: obj, PolicyRefsConfig: &common.KuadrantAuthPolicyRefsConfig{}} refs := gatewayWrapper.PolicyRefs() filteredRef := common.Filter(refs, func(key client.ObjectKey) bool { diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 9f8ac9f30..0926efc89 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -25,6 +25,7 @@ import ( authorinoopapi "github.com/kuadrant/authorino-operator/api/v1beta1" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/common" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -131,6 +132,7 @@ var _ = BeforeSuite(func() { TargetRefReconciler: reconcilers.TargetRefReconciler{ BaseReconciler: authPolicyBaseReconciler, }, + OverriddenPolicyMap: common.NewOverriddenPolicyMap(), }).SetupWithManager(mgr) Expect(err).NotTo(HaveOccurred()) diff --git a/main.go b/main.go index c8d67118c..f7a605b6a 100644 --- a/main.go +++ b/main.go @@ -22,6 +22,7 @@ import ( "os" "runtime" + "github.com/kuadrant/kuadrant-operator/pkg/common" "k8s.io/utils/env" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -171,6 +172,7 @@ func main() { TargetRefReconciler: reconcilers.TargetRefReconciler{ BaseReconciler: authPolicyBaseReconciler, }, + OverriddenPolicyMap: common.NewOverriddenPolicyMap(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "AuthPolicy") os.Exit(1) diff --git a/pkg/common/apimachinery_status_conditions.go b/pkg/common/apimachinery_status_conditions.go index ee5364a1b..c500aaf0f 100644 --- a/pkg/common/apimachinery_status_conditions.go +++ b/pkg/common/apimachinery_status_conditions.go @@ -6,8 +6,10 @@ import ( "fmt" "slices" "sort" + "sync" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) @@ -19,6 +21,41 @@ const ( PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown" ) +func NewOverriddenPolicyMap() *OverriddenPolicyMap { + return &OverriddenPolicyMap{ + policies: make(map[types.UID]bool), + } +} + +type OverriddenPolicyMap struct { + policies map[types.UID]bool + mu sync.RWMutex +} + +// SetOverriddenPolicy sets the provided KuadrantPolicy as overridden in the tracking map. +func (o *OverriddenPolicyMap) SetOverriddenPolicy(p KuadrantPolicy) { + o.mu.Lock() + defer o.mu.Unlock() + + if o.policies == nil { + o.policies = make(map[types.UID]bool) + } + o.policies[p.GetUID()] = true +} + +// RemoveOverriddenPolicy removes the provided KuadrantPolicy from the tracking map of overridden policies. +func (o *OverriddenPolicyMap) RemoveOverriddenPolicy(p KuadrantPolicy) { + o.mu.Lock() + defer o.mu.Unlock() + + delete(o.policies, p.GetUID()) +} + +// IsPolicyOverridden checks if the provided KuadrantPolicy is overridden based on the tracking map maintained. +func (o *OverriddenPolicyMap) IsPolicyOverridden(p KuadrantPolicy) bool { + return o.policies[p.GetUID()] && IsTargetRefGateway(p.GetTargetRef()) +} + // ConditionMarshal marshals the set of conditions as a JSON array, sorted by condition type. func ConditionMarshal(conditions []metav1.Condition) ([]byte, error) { condCopy := slices.Clone(conditions)