[TT-9323] Improve policy reconcile (#639)

* Do not reconcile policy on status update

* Fix logic to update status of api definitions

* Add tests

* Fix lint errors

* Update changelog

* Rename variables to avoid confusion

* Declare eval at the test function scope

CHANGELOG.md

@@ -10,6 +10,7 @@ last reconciliation. Now, any error can be observed there instead of checking Ty
 
 **Fixed**
 - Check if certificate already exists on tyk before uploading
+- Operator throwing lots of errors "the object has been modified; please apply your changes to the latest version and try again" while reconciling security policy
 
 ## [0.14.2](https://github.com/TykTechnologies/tyk-operator/tree/v0.14.2)
 [Full Changelog](https://github.com/TykTechnologies/tyk-operator/compare/v0.14.1...v0.14.2)

controllers/securitypolicy_controller.go

@@ -34,6 +34,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	util "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
 const policyFinalizer = "finalizers.tyk.io/securitypolicy"
@@ -109,6 +110,7 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		}
 
 		policy.Spec.SecurityPolicySpec = *newSpec
+
 		return nil
 	})
 
@@ -260,6 +262,7 @@ func (r *SecurityPolicyReconciler) update(ctx context.Context,
 	specTyk, err := klient.Universal.Portal().Policy().Get(ctx, policy.Status.PolID)
 	if err == nil {
 		if isSame(policy.Status.LatestCRDSpecHash, spec) && isSame(policy.Status.LatestTykSpecHash, specTyk) {
+			r.Log.Info("SecurityPolicy is already up-to-date", "Policy", client.ObjectKeyFromObject(policy))
 			// TODO(buraksekili): needs refactoring - no need for code duplication.
 			err = r.updateStatusOfLinkedAPIs(ctx, policy, false)
 			if err != nil {
@@ -440,22 +443,37 @@ func (r *SecurityPolicyReconciler) updateStatusOfLinkedAPIs(ctx context.Context,
 ) error {
 	r.Log.Info("Updating linked api definitions")
 
-	namespace := policy.Namespace
-
+	polNS := policy.Namespace
 	target := model.Target{
-		Namespace: &namespace, Name: policy.Name,
+		Namespace: &polNS, Name: policy.Name,
+	}
+
+	oldLinks := map[string]bool{}
+	newLinks := map[string]bool{}
+
+	for _, t := range policy.Status.LinkedAPIs {
+		oldLinks[t.String()] = true
+	}
+
+	for _, t := range policy.Spec.AccessRightsArray {
+		name := types.NamespacedName{Name: t.Name, Namespace: t.Namespace}
+		newLinks[name.String()] = true
 	}
 
 	// Remove links from api definitions
 	for _, t := range policy.Status.LinkedAPIs {
+		if _, ok := newLinks[t.String()]; ok {
+			continue
+		}
+
 		api := &tykv1.ApiDefinition{}
 
-		namespace := ""
+		apiNS := ""
 		if t.Namespace != nil {
-			namespace = *t.Namespace
+			apiNS = *t.Namespace
 		}
 
-		if err := r.Get(ctx, types.NamespacedName{Name: t.Name, Namespace: namespace}, api); err != nil {
+		if err := r.Get(ctx, types.NamespacedName{Name: t.Name, Namespace: apiNS}, api); err != nil {
 			r.Log.Error(err, "Failed to get the linked API", "api", t.String())
 
 			return err
@@ -471,6 +489,11 @@ func (r *SecurityPolicyReconciler) updateStatusOfLinkedAPIs(ctx context.Context,
 	}
 
 	for _, a := range policy.Spec.AccessRightsArray {
+		ok := oldLinks[types.NamespacedName{Name: a.Name, Namespace: a.Namespace}.String()]
+		if ok && !policyDeleted {
+			continue
+		}
+
 		api := &tykv1.ApiDefinition{}
 
 		name := types.NamespacedName{Name: a.Name, Namespace: a.Namespace}
@@ -501,5 +524,6 @@ func (r *SecurityPolicyReconciler) updateStatusOfLinkedAPIs(ctx context.Context,
 func (r *SecurityPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&tykv1.SecurityPolicy{}).
+		WithEventFilter(predicate.GenerationChangedPredicate{}).
 		Complete(r)
 }

integration/securitypolicy_test.go

@@ -1082,3 +1082,136 @@ func TestSecurityPolicyWithContextRef(t *testing.T) {
 
 	testenv.Test(t, policyAndOperatorCtx)
 }
+
+func TestUpdateStatusOfLinkedAPIs(t *testing.T) {
+	var (
+		api1    = "api1"
+		api2    = "api2"
+		testNs  = ""
+		polName = ""
+		pol     *v1alpha1.SecurityPolicy
+		eval    = is.New(t)
+	)
+
+	f := features.New("UpdateStatusOfLinkedAPIs").
+		Setup(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
+			var ok bool
+			testNs, ok = ctx.Value(ctxNSKey).(string)
+			eval.True(ok)
+
+			// Obtain Environment configuration to be able to connect Tyk.
+			tykEnv, err := generateEnvConfig(ctx, c)
+			eval.NoErr(err)
+
+			verifyPolicyApiVersion(t, &tykEnv)
+
+			apiDef, err := createTestAPIDef(ctx, c, testNs, func(api *v1alpha1.ApiDefinition) {
+				api.Name = api1
+				api.Spec.Name = api1
+			})
+			eval.NoErr(err)
+
+			err = waitForTykResourceCreation(c, apiDef)
+			eval.NoErr(err)
+
+			apiDef, err = createTestAPIDef(ctx, c, testNs, func(api *v1alpha1.ApiDefinition) {
+				api.Name = api2
+				api.Spec.Name = api2
+			})
+			eval.NoErr(err)
+
+			err = waitForTykResourceCreation(c, apiDef)
+			eval.NoErr(err)
+
+			pol, err = createTestPolicy(ctx, c, testNs, func(p *v1alpha1.SecurityPolicy) {
+				p.Spec.AccessRightsArray = []*model.AccessDefinition{
+					{
+						Name:      api1,
+						Namespace: testNs,
+					},
+					{
+						Name:      api2,
+						Namespace: testNs,
+					},
+				}
+			})
+			eval.NoErr(err)
+
+			polName = pol.Name
+
+			err = waitForTykResourceCreation(c, pol)
+			eval.NoErr(err)
+
+			return ctx
+		}).Assess("Link to policy is added both apis",
+		func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
+			apiDef := v1alpha1.ApiDefinition{}
+
+			err := c.Client().Resources(testNs).Get(ctx, api1, testNs, &apiDef)
+			eval.NoErr(err)
+
+			eval.True(len(apiDef.Status.LinkedByPolicies) == 1)
+			eval.True(apiDef.Status.LinkedByPolicies[0].Name == polName)
+
+			err = c.Client().Resources(testNs).Get(ctx, api2, testNs, &apiDef)
+			eval.NoErr(err)
+
+			eval.True(len(apiDef.Status.LinkedByPolicies) == 1)
+			eval.True(apiDef.Status.LinkedByPolicies[0].Name == polName)
+
+			return ctx
+		}).Assess("Link to policy is removed from api2",
+		func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
+			pol.Spec.AccessRightsArray = []*model.AccessDefinition{
+				{
+					Name:      "api1",
+					Namespace: testNs,
+				},
+			}
+
+			err := c.Client().Resources(testNs).Update(ctx, pol)
+			eval.NoErr(err)
+
+			apiDef := v1alpha1.ApiDefinition{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      api2,
+					Namespace: testNs,
+				},
+			}
+
+			err = wait.For(conditions.New(c.Client().Resources()).ResourceMatch(&apiDef, func(object k8s.Object) bool {
+				if apiDef.Status.LinkedByPolicies == nil || len(apiDef.Status.LinkedByPolicies) == 0 {
+					return true
+				}
+
+				return false
+			}), wait.WithTimeout(defaultWaitTimeout), wait.WithInterval(defaultWaitInterval))
+			eval.NoErr(err)
+
+			return ctx
+		}).Assess("Link to policy is removed when policy is deleted",
+		func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
+			err := c.Client().Resources(testNs).Delete(ctx, pol)
+			eval.NoErr(err)
+
+			apiDef := v1alpha1.ApiDefinition{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      api1,
+					Namespace: testNs,
+				},
+			}
+
+			err = wait.For(conditions.New(c.Client().Resources()).ResourceMatch(&apiDef, func(object k8s.Object) bool {
+				if apiDef.Status.LinkedByPolicies == nil || len(apiDef.Status.LinkedByPolicies) == 0 {
+					return true
+				}
+
+				return false
+			}), wait.WithTimeout(defaultWaitTimeout), wait.WithInterval(defaultWaitInterval))
+			eval.NoErr(err)
+
+			return ctx
+		}).Feature()
+
+	testenv.Test(t, f)
+}