diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e180ecc3..9c1ea69ef 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/controllers/securitypolicy_controller.go b/controllers/securitypolicy_controller.go index 44446c3c9..ed1336382 100644 --- a/controllers/securitypolicy_controller.go +++ b/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) } diff --git a/integration/securitypolicy_test.go b/integration/securitypolicy_test.go index da25335da..49a1ac544 100644 --- a/integration/securitypolicy_test.go +++ b/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) +}