From 85b6a968bd8bbf990ae5be537857143beb822d98 Mon Sep 17 00:00:00 2001 From: KevFan Date: Mon, 22 Apr 2024 18:35:23 +0100 Subject: [PATCH] fix: invalid reason not deleting second ns after test & fix missing comments --- controllers/authpolicy_authconfig.go | 2 +- controllers/authpolicy_controller_test.go | 16 +++++++++++----- controllers/ratelimitpolicy_controller_test.go | 11 +++++++++-- controllers/ratelimitpolicy_limits.go | 2 +- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 15d0c95aa..dfd673cb4 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -195,7 +195,7 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au return mergeConditionsFromRouteSelectorsIntoConfigs(ap, route, authConfig) } -// routeGatewayAuthOverrides returns the GW auth policies that has a +// routeGatewayAuthOverrides returns the GW auth policies that has an override field set func routeGatewayAuthOverrides(t *kuadrantgatewayapi.Topology, ap *api.AuthPolicy) []client.ObjectKey { affectedPolicies := getAffectedPolicies(t, ap) diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index 68a7c43a8..39f756d4b 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -15,6 +15,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" secv1beta1resources "istio.io/client-go/pkg/apis/security/v1beta1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1136,10 +1137,15 @@ var _ = Describe("AuthPolicy controller", func() { ), 30*time.Second, 5*time.Second).Should(BeTrue()) }) - It("Invalid reason", func() { + It("Invalid reason", func(ctx SpecContext) { var otherNamespace string CreateNamespace(&otherNamespace) - defer DeleteNamespaceCallback(&otherNamespace) + defer func(k8sClient client.Client, ctx context.Context, obj client.Object) { + err := k8sClient.Delete(ctx, obj) + if err != nil { + logf.Log.V(1).Info("failed to delete namespace", "namespace", obj.GetNamespace(), "name", obj.GetName()) + } + }(k8sClient, ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: otherNamespace}}) policy := policyFactory(func(policy *api.AuthPolicy) { policy.Namespace = otherNamespace // create the policy in a different namespace than the target @@ -1147,10 +1153,10 @@ var _ = Describe("AuthPolicy controller", func() { policy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(testGatewayName) policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace(testNamespace)) }) - Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) - Eventually(assertAcceptedCondFalseAndEnforcedCondNil(policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("AuthPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace)), 30*time.Second, 5*time.Second).Should(BeTrue()) - }) + Eventually(assertAcceptedCondFalseAndEnforcedCondNil(policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("AuthPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace))).WithContext(ctx).Should(BeTrue()) + }, SpecTimeout(time.Minute)) }) Context("AuthPolicy enforced condition reasons", func() { diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 218779b77..50df4ac4c 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -13,11 +13,13 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -662,7 +664,12 @@ var _ = Describe("RateLimitPolicy controller", func() { It("Invalid reason", func(ctx SpecContext) { var otherNamespace string CreateNamespace(&otherNamespace) - defer DeleteNamespaceCallback(&otherNamespace) + defer func(k8sClient client.Client, ctx context.Context, obj client.Object) { + err := k8sClient.Delete(ctx, obj) + if err != nil { + logf.Log.V(1).Info("failed to delete namespace", "namespace", obj.GetNamespace(), "name", obj.GetName()) + } + }(k8sClient, ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: otherNamespace}}) policy := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Namespace = otherNamespace // create the policy in a different namespace than the target @@ -672,7 +679,7 @@ var _ = Describe("RateLimitPolicy controller", func() { }) Expect(k8sClient.Create(ctx, policy)).To(Succeed()) - Eventually(assertAcceptedConditionFalse(ctx, policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace)), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(assertAcceptedConditionFalse(ctx, policy, string(gatewayapiv1alpha2.PolicyReasonInvalid), fmt.Sprintf("RateLimitPolicy target is invalid: invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", testNamespace))).WithContext(ctx).Should(Succeed()) }, SpecTimeout(time.Minute)) }) diff --git a/controllers/ratelimitpolicy_limits.go b/controllers/ratelimitpolicy_limits.go index e248ee10f..a90817570 100644 --- a/controllers/ratelimitpolicy_limits.go +++ b/controllers/ratelimitpolicy_limits.go @@ -189,7 +189,7 @@ func (r *RateLimitPolicyReconciler) getAffectedPoliciesInfo(rlp *kuadrantv1beta2 } // applyGatewayOverrides a Gateway RLP is not affected if there is untargetted routes or affects other policies -// Otherwise, it +// Otherwise, it has no free routes, and therefore "overridden" by the route policies func (r *RateLimitPolicyReconciler) applyGatewayOverrides(logger logr.Logger, rlp *kuadrantv1beta2.RateLimitPolicy, numUnTargetedRoutes int, affectedPolicies []kuadrantgatewayapi.Policy) { if rlp.Spec.Overrides == nil && numUnTargetedRoutes == 0 { r.AffectedPolicyMap.SetAffectedPolicy(rlp, utils.Map(affectedPolicies, func(p kuadrantgatewayapi.Policy) client.ObjectKey {