Skip to content

Commit

Permalink
refactor: OverriddenPolicyMap
Browse files Browse the repository at this point in the history
  • Loading branch information
KevFan committed Feb 15, 2024
1 parent 522306e commit 3b71908
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 28 deletions.
6 changes: 3 additions & 3 deletions controllers/authpolicy_authconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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
Expand Down
5 changes: 2 additions & 3 deletions controllers/authpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
28 changes: 6 additions & 22 deletions controllers/authpolicy_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
Expand All @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -131,6 +132,7 @@ var _ = BeforeSuite(func() {
TargetRefReconciler: reconcilers.TargetRefReconciler{
BaseReconciler: authPolicyBaseReconciler,
},
OverriddenPolicyMap: common.NewOverriddenPolicyMap(),
}).SetupWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

Expand Down
2 changes: 2 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Expand Down
37 changes: 37 additions & 0 deletions pkg/common/apimachinery_status_conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -19,6 +21,41 @@ const (
PolicyReasonUnknown gatewayapiv1alpha2.PolicyConditionReason = "Unknown"
)

func NewOverriddenPolicyMap() *OverriddenPolicyMap {
return &OverriddenPolicyMap{
policies: make(map[types.UID]bool),
}

Check warning on line 27 in pkg/common/apimachinery_status_conditions.go

View check run for this annotation

Codecov / codecov/patch

pkg/common/apimachinery_status_conditions.go#L24-L27

Added lines #L24 - L27 were not covered by tests
}

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

Check warning on line 43 in pkg/common/apimachinery_status_conditions.go

View check run for this annotation

Codecov / codecov/patch

pkg/common/apimachinery_status_conditions.go#L36-L43

Added lines #L36 - L43 were not covered by tests
}

// 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())

Check warning on line 51 in pkg/common/apimachinery_status_conditions.go

View check run for this annotation

Codecov / codecov/patch

pkg/common/apimachinery_status_conditions.go#L47-L51

Added lines #L47 - L51 were not covered by tests
}

// 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())

Check warning on line 56 in pkg/common/apimachinery_status_conditions.go

View check run for this annotation

Codecov / codecov/patch

pkg/common/apimachinery_status_conditions.go#L55-L56

Added lines #L55 - L56 were not covered by tests
}

// 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)
Expand Down

0 comments on commit 3b71908

Please sign in to comment.