From 344382823ca77566d257ce1592a699f47f741854 Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Wed, 5 Jul 2023 16:37:19 +0530 Subject: [PATCH 1/7] Do not reconcile policy on status update --- controllers/securitypolicy_controller.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/controllers/securitypolicy_controller.go b/controllers/securitypolicy_controller.go index 44446c3c9..14495108f 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" @@ -501,5 +502,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) } From b2524e2c50ed0483448b3d960f37d5f1e79c01dc Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Wed, 5 Jul 2023 19:13:02 +0530 Subject: [PATCH 2/7] Fix logic to update status of api definitions --- controllers/securitypolicy_controller.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/controllers/securitypolicy_controller.go b/controllers/securitypolicy_controller.go index 14495108f..daa9427e5 100644 --- a/controllers/securitypolicy_controller.go +++ b/controllers/securitypolicy_controller.go @@ -110,6 +110,7 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque } policy.Spec.SecurityPolicySpec = *newSpec + return nil }) @@ -261,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 { @@ -447,8 +449,24 @@ func (r *SecurityPolicyReconciler) updateStatusOfLinkedAPIs(ctx context.Context, Namespace: &namespace, 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 := "" @@ -472,6 +490,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} From 320b8b644a9d455b20943fbefe5b1ffb8deffc4e Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Wed, 5 Jul 2023 19:13:35 +0530 Subject: [PATCH 3/7] Add tests --- integration/securitypolicy_test.go | 133 +++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/integration/securitypolicy_test.go b/integration/securitypolicy_test.go index da25335da..b00db6018 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 + ) + + f := features.New("UpdateStatusOfLinkedAPIs"). + Setup(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + eval := is.New(t) + + 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) + + waitForTykResourceCreation(c, apiDef) + + apiDef, err = createTestAPIDef(ctx, c, testNs, func(api *v1alpha1.ApiDefinition) { + api.Name = api2 + api.Spec.Name = api2 + }) + eval.NoErr(err) + + waitForTykResourceCreation(c, apiDef) + + 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 + + waitForTykResourceCreation(c, pol) + + 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{} + eval := is.New(t) + + 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 { + eval := is.New(t) + + pol.Spec.AccessRightsArray = []*model.AccessDefinition{ + { + Name: "api1", + Namespace: testNs, + }, + } + + err := c.Client().Resources(testNs).Update(ctx, pol) + eval.NoErr(err) + + var 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 { + eval := is.New(t) + + err := c.Client().Resources(testNs).Delete(ctx, pol) + eval.NoErr(err) + + var 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) +} From 9bb1444743ef1230889fde0a168a59c885fe82e3 Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Wed, 5 Jul 2023 19:30:04 +0530 Subject: [PATCH 4/7] Fix lint errors --- integration/securitypolicy_test.go | 120 +++++++++++++++-------------- 1 file changed, 63 insertions(+), 57 deletions(-) diff --git a/integration/securitypolicy_test.go b/integration/securitypolicy_test.go index b00db6018..806291ada 100644 --- a/integration/securitypolicy_test.go +++ b/integration/securitypolicy_test.go @@ -1112,7 +1112,8 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { }) eval.NoErr(err) - waitForTykResourceCreation(c, apiDef) + err = waitForTykResourceCreation(c, apiDef) + eval.NoErr(err) apiDef, err = createTestAPIDef(ctx, c, testNs, func(api *v1alpha1.ApiDefinition) { api.Name = api2 @@ -1120,7 +1121,8 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { }) eval.NoErr(err) - waitForTykResourceCreation(c, apiDef) + err = waitForTykResourceCreation(c, apiDef) + eval.NoErr(err) pol, err = createTestPolicy(ctx, c, testNs, func(p *v1alpha1.SecurityPolicy) { p.Spec.AccessRightsArray = []*model.AccessDefinition{ @@ -1138,80 +1140,84 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { polName = pol.Name - waitForTykResourceCreation(c, pol) + 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{} - eval := is.New(t) - - err := c.Client().Resources(testNs).Get(ctx, api1, testNs, &apiDef) - eval.NoErr(err) + }).Assess("Link to policy is added both apis", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + apiDef := v1alpha1.ApiDefinition{} + eval := is.New(t) - eval.True(len(apiDef.Status.LinkedByPolicies) == 1) - eval.True(apiDef.Status.LinkedByPolicies[0].Name == polName) + err := c.Client().Resources(testNs).Get(ctx, api1, testNs, &apiDef) + eval.NoErr(err) - 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) - 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) - return ctx - }).Assess("Link to policy is removed from api2", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { - eval := is.New(t) + eval.True(len(apiDef.Status.LinkedByPolicies) == 1) + eval.True(apiDef.Status.LinkedByPolicies[0].Name == polName) - pol.Spec.AccessRightsArray = []*model.AccessDefinition{ - { - Name: "api1", - Namespace: testNs, - }, - } + return ctx + }).Assess("Link to policy is removed from api2", + func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { + eval := is.New(t) - err := c.Client().Resources(testNs).Update(ctx, pol) - eval.NoErr(err) + pol.Spec.AccessRightsArray = []*model.AccessDefinition{ + { + Name: "api1", + Namespace: testNs, + }, + } - var apiDef = v1alpha1.ApiDefinition{ - ObjectMeta: metav1.ObjectMeta{ - Name: api2, - Namespace: testNs, - }, - } + err := c.Client().Resources(testNs).Update(ctx, pol) + eval.NoErr(err) - 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 + apiDef := v1alpha1.ApiDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: api2, + Namespace: testNs, + }, } - return false - }), wait.WithTimeout(defaultWaitTimeout), wait.WithInterval(defaultWaitInterval)) - eval.NoErr(err) + 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 ctx - }).Assess("Link to policy is removed when policy is deleted", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { - eval := is.New(t) + return false + }), wait.WithTimeout(defaultWaitTimeout), wait.WithInterval(defaultWaitInterval)) + eval.NoErr(err) - err := c.Client().Resources(testNs).Delete(ctx, pol) - 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 { + eval := is.New(t) - var apiDef = v1alpha1.ApiDefinition{ - ObjectMeta: metav1.ObjectMeta{ - Name: api1, - Namespace: testNs, - }, - } + err := c.Client().Resources(testNs).Delete(ctx, pol) + eval.NoErr(err) - 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 + apiDef := v1alpha1.ApiDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: api1, + Namespace: testNs, + }, } - return false - }), wait.WithTimeout(defaultWaitTimeout), wait.WithInterval(defaultWaitInterval)) - eval.NoErr(err) + 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 ctx - }).Feature() + return false + }), wait.WithTimeout(defaultWaitTimeout), wait.WithInterval(defaultWaitInterval)) + eval.NoErr(err) + + return ctx + }).Feature() testenv.Test(t, f) } From bf323283c5c0ed6f6a2facb581488f8f74bd5d42 Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Thu, 6 Jul 2023 12:05:20 +0530 Subject: [PATCH 5/7] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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) From a8b8161d4b684cb89eb02cd9490d585052bfa2c8 Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Mon, 10 Jul 2023 16:48:50 +0530 Subject: [PATCH 6/7] Rename variables to avoid confusion --- controllers/securitypolicy_controller.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/controllers/securitypolicy_controller.go b/controllers/securitypolicy_controller.go index daa9427e5..ed1336382 100644 --- a/controllers/securitypolicy_controller.go +++ b/controllers/securitypolicy_controller.go @@ -443,10 +443,9 @@ 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{} @@ -469,12 +468,12 @@ func (r *SecurityPolicyReconciler) updateStatusOfLinkedAPIs(ctx context.Context, 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 From c27ff1384bde7e56c25b4208040e65fd4333bc8d Mon Sep 17 00:00:00 2001 From: Komal Sukhani Date: Mon, 10 Jul 2023 16:51:26 +0530 Subject: [PATCH 7/7] Declare eval at the test function scope --- integration/securitypolicy_test.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/integration/securitypolicy_test.go b/integration/securitypolicy_test.go index 806291ada..49a1ac544 100644 --- a/integration/securitypolicy_test.go +++ b/integration/securitypolicy_test.go @@ -1090,12 +1090,11 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { 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 { - eval := is.New(t) - var ok bool testNs, ok = ctx.Value(ctxNSKey).(string) eval.True(ok) @@ -1147,7 +1146,6 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { }).Assess("Link to policy is added both apis", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { apiDef := v1alpha1.ApiDefinition{} - eval := is.New(t) err := c.Client().Resources(testNs).Get(ctx, api1, testNs, &apiDef) eval.NoErr(err) @@ -1164,8 +1162,6 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { return ctx }).Assess("Link to policy is removed from api2", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { - eval := is.New(t) - pol.Spec.AccessRightsArray = []*model.AccessDefinition{ { Name: "api1", @@ -1195,8 +1191,6 @@ func TestUpdateStatusOfLinkedAPIs(t *testing.T) { return ctx }).Assess("Link to policy is removed when policy is deleted", func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { - eval := is.New(t) - err := c.Client().Resources(testNs).Delete(ctx, pol) eval.NoErr(err)