From e92b91df70f7fc83d3baf85daa38732352300255 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Mon, 28 Oct 2024 19:54:08 +0100 Subject: [PATCH] refactor: improved tracking of the origin of a policy rule throughout merges Signed-off-by: Guilherme Cassolato --- api/v1/merge_strategies.go | 38 ++++++++++++++----- api/v1beta3/authpolicy_types.go | 5 +++ api/v1beta3/ratelimitpolicy_types.go | 5 ++- controllers/auth_policy_status_updater.go | 2 +- .../ratelimit_policy_status_updater.go | 2 +- 5 files changed, 39 insertions(+), 13 deletions(-) diff --git a/api/v1/merge_strategies.go b/api/v1/merge_strategies.go index f169c5f73..779c9f157 100644 --- a/api/v1/merge_strategies.go +++ b/api/v1/merge_strategies.go @@ -69,13 +69,11 @@ func AtomicDefaultsMergeStrategy(source, target machinery.Policy) machinery.Poli return source } - mergeableTargetPolicy := target.(MergeablePolicy) - - if !mergeableTargetPolicy.Empty() { - return mergeableTargetPolicy.DeepCopyObject().(machinery.Policy) + if mergeableTarget := target.(MergeablePolicy); !mergeableTarget.Empty() { + return copyMergeablePolicy(mergeableTarget) } - return source.(MergeablePolicy).DeepCopyObject().(machinery.Policy) + return copyMergeablePolicy(source.(MergeablePolicy)) } var _ machinery.MergeStrategy = AtomicDefaultsMergeStrategy @@ -86,7 +84,7 @@ func AtomicOverridesMergeStrategy(source, _ machinery.Policy) machinery.Policy { if source == nil { return nil } - return source.(MergeablePolicy).DeepCopyObject().(machinery.Policy) + return copyMergeablePolicy(source.(MergeablePolicy)) } var _ machinery.MergeStrategy = AtomicOverridesMergeStrategy @@ -105,12 +103,16 @@ func PolicyRuleDefaultsMergeStrategy(source, target machinery.Policy) machinery. targetMergeablePolicy := target.(MergeablePolicy) // copy rules from the target - rules := targetMergeablePolicy.Rules() + rules := lo.MapValues(targetMergeablePolicy.Rules(), mapRuleWithSourceFunc(target)) // add extra rules from the source for ruleID, rule := range sourceMergeablePolicy.Rules() { if _, ok := targetMergeablePolicy.Rules()[ruleID]; !ok { - rules[ruleID] = rule.WithSource(source.GetLocator()) + origin := rule.GetSource() + if origin == "" { + origin = source.GetLocator() + } + rules[ruleID] = rule.WithSource(origin) } } @@ -129,12 +131,16 @@ func PolicyRuleOverridesMergeStrategy(source, target machinery.Policy) machinery targetMergeablePolicy := target.(MergeablePolicy) // copy rules from the source - rules := sourceMergeablePolicy.Rules() + rules := lo.MapValues(sourceMergeablePolicy.Rules(), mapRuleWithSourceFunc(source)) // add extra rules from the target for ruleID, rule := range targetMergeablePolicy.Rules() { if _, ok := sourceMergeablePolicy.Rules()[ruleID]; !ok { - rules[ruleID] = rule + origin := rule.GetSource() + if origin == "" { + origin = target.GetLocator() + } + rules[ruleID] = rule.WithSource(origin) } } @@ -206,3 +212,15 @@ func PathID(path []machinery.Targetable) string { return strings.TrimPrefix(k8stypes.NamespacedName{Namespace: t.GetNamespace(), Name: t.GetName()}.String(), string(k8stypes.Separator)) }), "|") } + +func mapRuleWithSourceFunc(source machinery.Policy) func(MergeableRule, string) MergeableRule { + return func(rule MergeableRule, _ string) MergeableRule { + return rule.WithSource(source.GetLocator()) + } +} + +func copyMergeablePolicy(policy MergeablePolicy) MergeablePolicy { + dup := policy.DeepCopyObject().(MergeablePolicy) + dup.SetRules(lo.MapValues(dup.Rules(), mapRuleWithSourceFunc(policy))) + return dup +} diff --git a/api/v1beta3/authpolicy_types.go b/api/v1beta3/authpolicy_types.go index 27aba8cf3..8aae9cda9 100644 --- a/api/v1beta3/authpolicy_types.go +++ b/api/v1beta3/authpolicy_types.go @@ -189,6 +189,11 @@ func (p *AuthPolicy) Rules() map[string]kuadrantv1.MergeableRule { } func (p *AuthPolicy) SetRules(rules map[string]kuadrantv1.MergeableRule) { + // clear all rules of the policy before setting new ones + p.Spec.Proper().NamedPatterns = nil + p.Spec.Proper().Conditions = nil + p.Spec.Proper().AuthScheme = nil + ensureNamedPatterns := func() { if p.Spec.Proper().NamedPatterns == nil { p.Spec.Proper().NamedPatterns = make(map[string]MergeablePatternExpressions) diff --git a/api/v1beta3/ratelimitpolicy_types.go b/api/v1beta3/ratelimitpolicy_types.go index 796f911c7..0d058291b 100644 --- a/api/v1beta3/ratelimitpolicy_types.go +++ b/api/v1beta3/ratelimitpolicy_types.go @@ -132,7 +132,10 @@ func (p *RateLimitPolicy) Rules() map[string]kuadrantv1.MergeableRule { } func (p *RateLimitPolicy) SetRules(rules map[string]kuadrantv1.MergeableRule) { - if len(rules) > 0 && p.Spec.Proper().Limits == nil { + // clear all rules of the policy before setting new ones + p.Spec.Proper().Limits = nil + + if len(rules) > 0 { p.Spec.Proper().Limits = make(map[string]Limit) } diff --git a/controllers/auth_policy_status_updater.go b/controllers/auth_policy_status_updater.go index 113713d9a..f1acbcd05 100644 --- a/controllers/auth_policy_status_updater.go +++ b/controllers/auth_policy_status_updater.go @@ -152,7 +152,7 @@ func (r *AuthPolicyStatusUpdater) enforcedCondition(policy *kuadrantv1beta3.Auth for _, policyRuleKey := range policyRuleKeys { if effectivePolicyRule, ok := effectivePolicyRules[policyRuleKey]; !ok || (ok && effectivePolicyRule.GetSource() != policy.GetLocator()) { // policy rule has been overridden by another policy var overriddenBy string - if ok { // TODO(guicassolato): !ok → we cannot tell which policy is overriding the rule, this information is lost when the policy rule is dropped during an atomic override + if ok { overriddenBy = effectivePolicyRule.GetSource() } overridingPolicies[policyRuleKey] = append(overridingPolicies[policyRuleKey], overriddenBy) diff --git a/controllers/ratelimit_policy_status_updater.go b/controllers/ratelimit_policy_status_updater.go index ecc7a0a22..4ac9f23f2 100644 --- a/controllers/ratelimit_policy_status_updater.go +++ b/controllers/ratelimit_policy_status_updater.go @@ -142,7 +142,7 @@ func (r *RateLimitPolicyStatusUpdater) enforcedCondition(policy *kuadrantv1beta3 for _, policyRuleKey := range policyRuleKeys { if effectivePolicyRule, ok := effectivePolicyRules[policyRuleKey]; !ok || (ok && effectivePolicyRule.GetSource() != policy.GetLocator()) { // policy rule has been overridden by another policy var overriddenBy string - if ok { // TODO(guicassolato): !ok → we cannot tell which policy is overriding the rule, this information is lost when the policy rule is dropped during an atomic override + if ok { overriddenBy = effectivePolicyRule.GetSource() } overridingPolicies[policyRuleKey] = append(overridingPolicies[policyRuleKey], overriddenBy)