From e9212744a4e66b63727986284f0225df74e890e4 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Thu, 11 Apr 2024 16:47:12 +0200 Subject: [PATCH 1/5] PolicyAffected status condition added to Gateway and HTTPRoute objects for AuthPolicy and RateLimitPolicy --- api/v1alpha1/dnspolicy_types.go | 24 + api/v1alpha1/tlspolicy_types.go | 24 + api/v1beta2/authpolicy_types.go | 26 +- api/v1beta2/authpolicy_types_test.go | 28 - api/v1beta2/ratelimitpolicy_types.go | 25 +- ...adrant-operator.clusterserviceversion.yaml | 18 +- config/rbac/role.yaml | 16 + controllers/authpolicy_authconfig.go | 2 +- controllers/authpolicy_controller.go | 16 +- controllers/authpolicy_controller_test.go | 6 + .../authpolicy_istio_authorizationpolicy.go | 2 +- controllers/dnspolicy_controller.go | 68 +-- controllers/dnspolicy_controller_test.go | 23 - controllers/helper_test.go | 8 +- controllers/kuadrant_controller.go | 12 +- controllers/kuadrant_status.go | 28 - controllers/ratelimitpolicy_controller.go | 15 +- .../ratelimitpolicy_controller_test.go | 15 + controllers/suite_test.go | 12 + controllers/target_status_controller.go | 371 ++++++++++++ controllers/target_status_controller_test.go | 560 ++++++++++++++++++ controllers/tlspolicy_controller.go | 65 +- controllers/tlspolicy_controller_test.go | 8 - main.go | 12 + pkg/library/gatewayapi/types.go | 14 + pkg/library/gatewayapi/types_test.go | 21 +- pkg/library/gatewayapi/utils.go | 43 +- pkg/library/kuadrant/kuadrant.go | 1 + pkg/library/kuadrant/test_utils.go | 17 + pkg/library/mappers/policy_to_gateway.go | 2 +- .../reconcilers/target_ref_reconciler.go | 14 +- pkg/library/utils/slice_utils.go | 13 +- 32 files changed, 1223 insertions(+), 286 deletions(-) create mode 100644 controllers/target_status_controller.go create mode 100644 controllers/target_status_controller_test.go diff --git a/api/v1alpha1/dnspolicy_types.go b/api/v1alpha1/dnspolicy_types.go index db7ab2030..0ff26b8a8 100644 --- a/api/v1alpha1/dnspolicy_types.go +++ b/api/v1alpha1/dnspolicy_types.go @@ -24,7 +24,9 @@ import ( gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) type RoutingStrategy string @@ -128,6 +130,10 @@ type DNSPolicyStatus struct { HealthCheck *HealthCheckStatus `json:"healthCheck,omitempty"` } +func (s *DNSPolicyStatus) GetConditions() []metav1.Condition { + return s.Conditions +} + var _ kuadrant.Policy = &DNSPolicy{} var _ kuadrant.Referrer = &DNSPolicy{} @@ -160,8 +166,16 @@ func (p *DNSPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { return p.Spec.TargetRef } +func (p *DNSPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { + return &p.Status +} + func (p *DNSPolicy) Kind() string { return p.TypeMeta.Kind } +func (p *DNSPolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { + return kuadrantgatewayapi.DirectPolicy +} + func (p *DNSPolicy) BackReferenceAnnotationName() string { return DNSPolicyBackReferenceAnnotationName } @@ -209,6 +223,12 @@ type DNSPolicyList struct { Items []DNSPolicy `json:"items"` } +func (l *DNSPolicyList) GetItems() []kuadrant.Policy { + return utils.Map(l.Items, func(item DNSPolicy) kuadrant.Policy { + return &item + }) +} + // HealthCheckSpec configures health checks in the DNS provider. // By default, this health check will be applied to each unique DNS A Record for // the listeners assigned to the target gateway @@ -267,6 +287,10 @@ func init() { func NewDNSPolicy(name, ns string) *DNSPolicy { return &DNSPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "DNSPolicy", + APIVersion: GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: ns, diff --git a/api/v1alpha1/tlspolicy_types.go b/api/v1alpha1/tlspolicy_types.go index 6952b0954..ddfe7e309 100644 --- a/api/v1alpha1/tlspolicy_types.go +++ b/api/v1alpha1/tlspolicy_types.go @@ -25,7 +25,9 @@ import ( gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) const ( @@ -116,6 +118,10 @@ type TLSPolicyStatus struct { ObservedGeneration int64 `json:"observedGeneration,omitempty"` } +func (s *TLSPolicyStatus) GetConditions() []metav1.Condition { + return s.Conditions +} + var _ kuadrant.Policy = &TLSPolicy{} var _ kuadrant.Referrer = &TLSPolicy{} @@ -138,6 +144,10 @@ type TLSPolicy struct { func (p *TLSPolicy) Kind() string { return p.TypeMeta.Kind } +func (p *TLSPolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { + return kuadrantgatewayapi.DirectPolicy +} + func (p *TLSPolicy) GetWrappedNamespace() gatewayapiv1.Namespace { return gatewayapiv1.Namespace(p.Namespace) } @@ -150,6 +160,10 @@ func (p *TLSPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { return p.Spec.TargetRef } +func (p *TLSPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { + return &p.Status +} + func (p *TLSPolicy) BackReferenceAnnotationName() string { return TLSPolicyBackReferenceAnnotationName } @@ -183,6 +197,12 @@ type TLSPolicyList struct { Items []TLSPolicy `json:"items"` } +func (l *TLSPolicyList) GetItems() []kuadrant.Policy { + return utils.Map(l.Items, func(item TLSPolicy) kuadrant.Policy { + return &item + }) +} + func init() { SchemeBuilder.Register(&TLSPolicy{}, &TLSPolicyList{}) } @@ -191,6 +211,10 @@ func init() { func NewTLSPolicy(policyName, ns string) *TLSPolicy { return &TLSPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "TLSPolicy", + APIVersion: GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: policyName, Namespace: ns, diff --git a/api/v1beta2/authpolicy_types.go b/api/v1beta2/authpolicy_types.go index 20deb2511..8bef0595a 100644 --- a/api/v1beta2/authpolicy_types.go +++ b/api/v1beta2/authpolicy_types.go @@ -7,10 +7,10 @@ import ( "github.com/google/go-cmp/cmp" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) @@ -242,6 +242,10 @@ func (s *AuthPolicyStatus) Equals(other *AuthPolicyStatus, logger logr.Logger) b return true } +func (s *AuthPolicyStatus) GetConditions() []metav1.Condition { + return s.Conditions +} + var _ kuadrant.Policy = &AuthPolicy{} var _ kuadrant.Referrer = &AuthPolicy{} @@ -267,18 +271,6 @@ func (ap *AuthPolicy) IsAtomicOverride() bool { return ap.Spec.Overrides != nil } -func (ap *AuthPolicy) TargetKey() client.ObjectKey { - ns := ap.Namespace - if ap.Spec.TargetRef.Namespace != nil { - ns = string(*ap.Spec.TargetRef.Namespace) - } - - return client.ObjectKey{ - Name: string(ap.Spec.TargetRef.Name), - Namespace: ns, - } -} - func (ap *AuthPolicy) Validate() error { if ap.Spec.TargetRef.Namespace != nil && string(*ap.Spec.TargetRef.Namespace) != ap.Namespace { return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *ap.Spec.TargetRef.Namespace) @@ -291,6 +283,10 @@ func (ap *AuthPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { return ap.Spec.TargetRef } +func (ap *AuthPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { + return &ap.Status +} + func (ap *AuthPolicy) GetWrappedNamespace() gatewayapiv1.Namespace { return gatewayapiv1.Namespace(ap.Namespace) } @@ -342,6 +338,10 @@ func (ap *AuthPolicy) Kind() string { return ap.TypeMeta.Kind } +func (ap *AuthPolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { + return kuadrantgatewayapi.InheritedPolicy +} + func (ap *AuthPolicy) BackReferenceAnnotationName() string { return AuthPolicyBackReferenceAnnotationName } diff --git a/api/v1beta2/authpolicy_types_test.go b/api/v1beta2/authpolicy_types_test.go index c0fb1df24..75e38ad1f 100644 --- a/api/v1beta2/authpolicy_types_test.go +++ b/api/v1beta2/authpolicy_types_test.go @@ -47,34 +47,6 @@ func TestAuthPolicySpecGetRouteSelectors(t *testing.T) { } } -func TestAuthPolicyTargetKey(t *testing.T) { - policy := &AuthPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "my-policy", - Namespace: "my-namespace", - }, - Spec: AuthPolicySpec{ - TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ - Group: gatewayapiv1.GroupName, - Kind: "HTTPRoute", - Name: "my-route", - }, - }, - } - // targetRef missing namespace - expected := "my-namespace/my-route" - if result := policy.TargetKey().String(); result != expected { - t.Errorf("Expected target key %s, got %s", expected, result) - } - - // targetRef with namespace - policy.Spec.TargetRef.Namespace = ptr.To(gatewayapiv1.Namespace("route-namespace")) - expected = "route-namespace/my-route" - if result := policy.TargetKey().String(); result != expected { - t.Errorf("Expected target key %s, got %s", expected, result) - } -} - func TestAuthPolicyListGetItems(t *testing.T) { list := &AuthPolicyList{} if len(list.GetItems()) != 0 { diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go index 7b509c027..306e26ab0 100644 --- a/api/v1beta2/ratelimitpolicy_types.go +++ b/api/v1beta2/ratelimitpolicy_types.go @@ -22,7 +22,6 @@ import ( "github.com/go-logr/logr" "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -190,6 +189,10 @@ func (s *RateLimitPolicyStatus) Equals(other *RateLimitPolicyStatus, logger logr return true } +func (s *RateLimitPolicyStatus) GetConditions() []metav1.Condition { + return s.Conditions +} + var _ kuadrant.Policy = &RateLimitPolicy{} var _ kuadrant.Referrer = &RateLimitPolicy{} @@ -221,18 +224,6 @@ func (r *RateLimitPolicy) Validate() error { return nil } -func (r *RateLimitPolicy) TargetKey() client.ObjectKey { - tmpNS := r.Namespace - if r.Spec.TargetRef.Namespace != nil { - tmpNS = string(*r.Spec.TargetRef.Namespace) - } - - return client.ObjectKey{ - Name: string(r.Spec.TargetRef.Name), - Namespace: tmpNS, - } -} - //+kubebuilder:object:root=true // RateLimitPolicyList contains a list of RateLimitPolicy @@ -252,6 +243,10 @@ func (r *RateLimitPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReferenc return r.Spec.TargetRef } +func (r *RateLimitPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { + return &r.Status +} + func (r *RateLimitPolicy) GetWrappedNamespace() gatewayapiv1.Namespace { return gatewayapiv1.Namespace(r.Namespace) } @@ -277,6 +272,10 @@ func (r *RateLimitPolicy) Kind() string { return r.TypeMeta.Kind } +func (r *RateLimitPolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { + return kuadrantgatewayapi.InheritedPolicy +} + func (r *RateLimitPolicy) BackReferenceAnnotationName() string { return RateLimitPolicyBackReferenceAnnotationName } diff --git a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml index fcf3e22b5..b75cfe43e 100644 --- a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml +++ b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml @@ -106,7 +106,7 @@ metadata: capabilities: Basic Install categories: Integration & Delivery containerImage: quay.io/kuadrant/kuadrant-operator:latest - createdAt: "2024-04-22T07:53:19Z" + createdAt: "2024-04-22T18:16:55Z" operators.operatorframework.io/builder: operator-sdk-v1.32.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 repository: https://github.com/Kuadrant/kuadrant-operator @@ -282,6 +282,14 @@ spec: - patch - update - watch + - apiGroups: + - gateway.networking.k8s.io + resources: + - gatewayclasses + verbs: + - get + - list + - watch - apiGroups: - gateway.networking.k8s.io resources: @@ -318,6 +326,14 @@ spec: - patch - update - watch + - apiGroups: + - gateway.networking.k8s.io + resources: + - httproutes/status + verbs: + - get + - patch + - update - apiGroups: - install.istio.io resources: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index b390602b2..7afd7f41a 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -130,6 +130,14 @@ rules: - patch - update - watch +- apiGroups: + - gateway.networking.k8s.io + resources: + - gatewayclasses + verbs: + - get + - list + - watch - apiGroups: - gateway.networking.k8s.io resources: @@ -166,6 +174,14 @@ rules: - patch - update - watch +- apiGroups: + - gateway.networking.k8s.io + resources: + - httproutes/status + verbs: + - get + - patch + - update - apiGroups: - install.istio.io resources: diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index dfd673cb4..ba64a9342 100644 --- a/controllers/authpolicy_authconfig.go +++ b/controllers/authpolicy_authconfig.go @@ -96,7 +96,7 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au hosts = utils.HostnamesToStrings(gwHostnames) rules := make([]gatewayapiv1.HTTPRouteRule, 0) - routes := r.TargetRefReconciler.FetchAcceptedGatewayHTTPRoutes(ctx, ap.TargetKey()) + routes := r.TargetRefReconciler.FetchAcceptedGatewayHTTPRoutes(ctx, obj) for idx := range routes { route := routes[idx] // skip routes that have an authpolicy of its own and Gateway authpolicy does not define atomic overrides diff --git a/controllers/authpolicy_controller.go b/controllers/authpolicy_controller.go index 8fb053676..9858cfe13 100644 --- a/controllers/authpolicy_controller.go +++ b/controllers/authpolicy_controller.go @@ -3,6 +3,7 @@ package controllers import ( "context" "encoding/json" + "fmt" "github.com/go-logr/logr" authorinoapi "github.com/kuadrant/authorino/api/v1beta2" @@ -160,11 +161,11 @@ func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.A } if err := r.reconcileIstioAuthorizationPolicies(ctx, ap, targetNetworkObject, gatewayDiffObj); err != nil { - return err + return fmt.Errorf("reconcile AuthorizationPolicy error %w", err) } if err := r.reconcileAuthConfigs(ctx, ap, targetNetworkObject); err != nil { - return err + return fmt.Errorf("reconcile AuthConfig error %w", err) } // if the AuthPolicy(ap) targets a Gateway then all policies attached to that Gateway need to be checked. @@ -184,8 +185,7 @@ func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.A return err } - refNetworkObject := &gatewayapiv1.HTTPRoute{} - err = r.Client().Get(ctx, ref.TargetKey(), refNetworkObject) + refNetworkObject, err := reconcilers.FetchTargetRefObject(ctx, r.Client(), ref.GetTargetRef(), ref.Namespace) if err != nil { return err } @@ -198,11 +198,15 @@ func (r *AuthPolicyReconciler) reconcileResources(ctx context.Context, ap *api.A // set direct back ref - i.e. claim the target network object as taken asap if err := r.reconcileNetworkResourceDirectBackReference(ctx, ap, targetNetworkObject); err != nil { - return err + return fmt.Errorf("reconcile TargetBackReference error %w", err) } // set annotation of policies affecting the gateway - should be the last step, only when all the reconciliation steps succeed - return r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, ap, gatewayDiffObj) + if err := r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, ap, gatewayDiffObj); err != nil { + return fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err) + } + + return nil } func (r *AuthPolicyReconciler) deleteResources(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object) error { diff --git a/controllers/authpolicy_controller_test.go b/controllers/authpolicy_controller_test.go index 72e718298..a5767d842 100644 --- a/controllers/authpolicy_controller_test.go +++ b/controllers/authpolicy_controller_test.go @@ -57,6 +57,10 @@ var _ = Describe("AuthPolicy controller", func() { policyFactory := func(mutateFns ...func(policy *api.AuthPolicy)) *api.AuthPolicy { policy := &api.AuthPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "AuthPolicy", + APIVersion: api.GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "toystore", Namespace: testNamespace, @@ -1123,6 +1127,8 @@ var _ = Describe("AuthPolicy controller", func() { logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) Expect(err).ToNot(HaveOccurred()) + Eventually(isAuthPolicyAccepted(policy), time.Minute, 5*time.Second).Should(BeTrue()) + policy2 := policyFactory(func(policy *api.AuthPolicy) { policy.Name = "conflicting-ap" }) diff --git a/controllers/authpolicy_istio_authorizationpolicy.go b/controllers/authpolicy_istio_authorizationpolicy.go index fb20993d8..c9d9471c2 100644 --- a/controllers/authpolicy_istio_authorizationpolicy.go +++ b/controllers/authpolicy_istio_authorizationpolicy.go @@ -124,7 +124,7 @@ func (r *AuthPolicyReconciler) istioAuthorizationPolicy(ctx context.Context, ap // fake a single httproute with all rules from all httproutes accepted by the gateway, // that do not have an authpolicy of its own, so we can generate wasm rules for those cases rules := make([]gatewayapiv1.HTTPRouteRule, 0) - routes := r.TargetRefReconciler.FetchAcceptedGatewayHTTPRoutes(ctx, ap.TargetKey()) + routes := r.TargetRefReconciler.FetchAcceptedGatewayHTTPRoutes(ctx, obj) for idx := range routes { route := routes[idx] // skip routes that have an authpolicy of its own diff --git a/controllers/dnspolicy_controller.go b/controllers/dnspolicy_controller.go index 3a6806431..dcda01292 100644 --- a/controllers/dnspolicy_controller.go +++ b/controllers/dnspolicy_controller.go @@ -18,13 +18,9 @@ package controllers import ( "context" - "errors" "fmt" - "reflect" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -32,7 +28,6 @@ import ( crlog "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1" @@ -42,10 +37,7 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" ) -const ( - DNSPolicyFinalizer = "kuadrant.io/dns-policy" - DNSPolicyAffected string = "kuadrant.io/DNSPolicyAffected" -) +const DNSPolicyFinalizer = "kuadrant.io/dns-policy" type DNSPolicyRefsConfig struct{} @@ -60,9 +52,6 @@ type DNSPolicyReconciler struct { //+kubebuilder:rbac:groups=kuadrant.io,resources=dnspolicies,verbs=get;list;watch;update;patch;delete //+kubebuilder:rbac:groups=kuadrant.io,resources=dnspolicies/status,verbs=get;update;patch //+kubebuilder:rbac:groups=kuadrant.io,resources=dnspolicies/finalizers,verbs=update -//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways,verbs=get;list;watch;update;patch -//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways/status,verbs=get;update;patch -//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways/finalizers,verbs=update //+kubebuilder:rbac:groups=kuadrant.io,resources=dnsrecords,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=kuadrant.io,resources=dnsrecords/status,verbs=get @@ -136,8 +125,6 @@ func (r *DNSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } func (r *DNSPolicyReconciler) reconcileResources(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, targetNetworkObject client.Object) error { - gatewayCondition := BuildPolicyAffectedCondition(DNSPolicyAffected, dnsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyReasonAccepted, nil) - // validate err := dnsPolicy.Validate() if err != nil { @@ -153,29 +140,17 @@ func (r *DNSPolicyReconciler) reconcileResources(ctx context.Context, dnsPolicy } if err = r.reconcileDNSRecords(ctx, dnsPolicy, gatewayDiffObj); err != nil { - gatewayCondition = BuildPolicyAffectedCondition(DNSPolicyAffected, dnsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyReasonInvalid, err) - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - return errors.Join(fmt.Errorf("reconcile DNSRecords error %w", err), updateErr) + return fmt.Errorf("reconcile DNSRecords error %w", err) } // set direct back ref - i.e. claim the target network object as taken asap if err = r.TargetRefReconciler.ReconcileTargetBackReference(ctx, dnsPolicy, targetNetworkObject, dnsPolicy.DirectReferenceAnnotationName()); err != nil { - gatewayCondition = BuildPolicyAffectedCondition(DNSPolicyAffected, dnsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyReasonConflicted, err) - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - return errors.Join(fmt.Errorf("reconcile TargetBackReference error %w", err), updateErr) + return fmt.Errorf("reconcile TargetBackReference error %w", err) } // set annotation of policies affecting the gateway if err := r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, dnsPolicy, gatewayDiffObj); err != nil { - gatewayCondition = BuildPolicyAffectedCondition(DNSPolicyAffected, dnsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyConditionReason(PolicyReasonUnknown), err) - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - return errors.Join(fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err), updateErr) - } - - // set gateway policy affected condition status - should be the last step, only when all the reconciliation steps succeed - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - if updateErr != nil { - return fmt.Errorf("failed to update gateway conditions %w ", updateErr) + return fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err) } return nil @@ -200,40 +175,7 @@ func (r *DNSPolicyReconciler) deleteResources(ctx context.Context, dnsPolicy *v1 } // update annotation of policies affecting the gateway - if err := r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, dnsPolicy, gatewayDiffObj); err != nil { - return err - } - - // remove gateway policy affected condition status - return r.updateGatewayCondition(ctx, metav1.Condition{Type: DNSPolicyAffected}, gatewayDiffObj) -} - -func (r *DNSPolicyReconciler) updateGatewayCondition(ctx context.Context, condition metav1.Condition, gatewayDiff *reconcilers.GatewayDiffs) error { - // update condition if needed - gatewayDiffs := append(gatewayDiff.GatewaysWithValidPolicyRef, gatewayDiff.GatewaysMissingPolicyRef...) - for i, gw := range gatewayDiffs { - previous := gw.DeepCopy() - meta.SetStatusCondition(&gatewayDiffs[i].Status.Conditions, condition) - if !reflect.DeepEqual(previous.Status.Conditions, gw.Status.Conditions) { - if err := r.Client().Status().Update(ctx, gw.Gateway); err != nil { - return err - } - } - } - - // remove condition from gateway that is no longer referenced - gatewayDiffs = gatewayDiff.GatewaysWithInvalidPolicyRef - for i, gw := range gatewayDiff.GatewaysWithInvalidPolicyRef { - previous := gw.DeepCopy() - meta.RemoveStatusCondition(&gatewayDiffs[i].Status.Conditions, condition.Type) - if !reflect.DeepEqual(previous.Status.Conditions, gw.Status.Conditions) { - if err := r.Client().Status().Update(ctx, gw.Gateway); err != nil { - return err - } - } - } - - return nil + return r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, dnsPolicy, gatewayDiffObj) } func (r *DNSPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { diff --git a/controllers/dnspolicy_controller_test.go b/controllers/dnspolicy_controller_test.go index 8c31c21ef..3af3114a1 100644 --- a/controllers/dnspolicy_controller_test.go +++ b/controllers/dnspolicy_controller_test.go @@ -325,16 +325,6 @@ var _ = Describe("DNSPolicy controller", func() { "Message": Equal("DNSPolicy has been successfully enforced"), })), ) - err = k8sClient.Get(ctx, client.ObjectKeyFromObject(gateway), gateway) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(gateway.Status.Conditions).To( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(DNSPolicyAffected), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal(string(gatewayapiv1alpha2.PolicyReasonAccepted)), - "ObservedGeneration": Equal(gateway.Generation), - })), - ) }, TestTimeoutMedium, time.Second).Should(Succeed()) // ensure there are no policies with not accepted condition @@ -402,14 +392,6 @@ var _ = Describe("DNSPolicy controller", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(gateway.Annotations).To(HaveKeyWithValue(v1alpha1.DNSPolicyDirectReferenceAnnotationName, policyBackRefValue)) g.Expect(gateway.Annotations).To(HaveKeyWithValue(v1alpha1.DNSPolicyBackReferenceAnnotationName, policiesBackRefValue)) - g.Expect(gateway.Status.Conditions).To( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(DNSPolicyAffected), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal(string(gatewayapiv1alpha2.PolicyReasonAccepted)), - "ObservedGeneration": Equal(gateway.Generation), - })), - ) err = k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsPolicy), dnsPolicy) g.Expect(err).NotTo(HaveOccurred()) @@ -424,11 +406,6 @@ var _ = Describe("DNSPolicy controller", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(gateway.Annotations).ToNot(HaveKey(v1alpha1.DNSPolicyDirectReferenceAnnotationName)) g.Expect(gateway.Annotations).ToNot(HaveKeyWithValue(v1alpha1.DNSPolicyBackReferenceAnnotationName, policiesBackRefValue)) - g.Expect(gateway.Status.Conditions).ToNot( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(DNSPolicyAffected)), - })), - ) }, TestTimeoutMedium, time.Second).Should(Succeed()) }) diff --git a/controllers/helper_test.go b/controllers/helper_test.go index cc1ca51fd..1cc96c5b8 100644 --- a/controllers/helper_test.go +++ b/controllers/helper_test.go @@ -240,8 +240,8 @@ func CreateOrUpdateK8SObject(obj runtime.Object, k8sClient client.Client) error return k8sClient.Update(context.Background(), k8sObjCopy) } -func testBuildBasicGateway(gwName, ns string) *gatewayapiv1.Gateway { - return &gatewayapiv1.Gateway{ +func testBuildBasicGateway(gwName, ns string, mutateFns ...func(*gatewayapiv1.Gateway)) *gatewayapiv1.Gateway { + gateway := &gatewayapiv1.Gateway{ TypeMeta: metav1.TypeMeta{ Kind: "Gateway", APIVersion: gatewayapiv1.GroupVersion.String(), @@ -263,6 +263,10 @@ func testBuildBasicGateway(gwName, ns string) *gatewayapiv1.Gateway { }, }, } + for _, mutateFn := range mutateFns { + mutateFn(gateway) + } + return gateway } func testBuildBasicHttpRoute(routeName, gwName, ns string, hostnames []string) *gatewayapiv1.HTTPRoute { diff --git a/controllers/kuadrant_controller.go b/controllers/kuadrant_controller.go index 8298ad37a..77a55702a 100644 --- a/controllers/kuadrant_controller.go +++ b/controllers/kuadrant_controller.go @@ -60,19 +60,27 @@ type KuadrantReconciler struct { //+kubebuilder:rbac:groups=kuadrant.io,resources=kuadrants/status,verbs=get;update;patch //+kubebuilder:rbac:groups=kuadrant.io,resources=kuadrants/finalizers,verbs=update //+kubebuilder:rbac:groups=limitador.kuadrant.io,resources=limitadors,verbs=get;list;watch;create;update;delete;patch + //+kubebuilder:rbac:groups=core,resources=serviceaccounts;configmaps;services,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=coordination.k8s.io,resources=configmaps;leases,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups="",resources=events,verbs=create;patch //+kubebuilder:rbac:groups="",resources=leases,verbs=get;list;watch;create;update;patch;delete -//+kubebuilder:rbac:groups="gateway.networking.k8s.io",resources=gateways,verbs=get;list;watch;create;update;delete;patch -//+kubebuilder:rbac:groups="gateway.networking.k8s.io",resources=httproutes,verbs=get;list;patch;update;watch + //+kubebuilder:rbac:groups=operator.authorino.kuadrant.io,resources=authorinos,verbs=get;list;watch;create;update;delete;patch //+kubebuilder:rbac:groups=install.istio.io,resources=istiooperators,verbs=get;list;watch;create;update;patch //+kubebuilder:rbac:groups=operator.istio.io,resources=istios,verbs=get;list;watch;create;update;patch //+kubebuilder:rbac:groups=maistra.io,resources=servicemeshcontrolplanes,verbs=get;list;watch;update;use;patch //+kubebuilder:rbac:groups=maistra.io,resources=servicemeshmembers,verbs=get;list;watch;create;update;delete;patch +// Common permissions required by policy controllers +//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gatewayclasses,verbs=get;list;watch +//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways,verbs=get;list;watch;update;patch +//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways/status,verbs=get;update;patch +//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways/finalizers,verbs=update +//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=httproutes,verbs=get;list;watch;update;patch +//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=httproutes/status,verbs=get;update;patch + // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. // For more details, check Reconcile and its Result here: diff --git a/controllers/kuadrant_status.go b/controllers/kuadrant_status.go index dec6f1721..60c776009 100644 --- a/controllers/kuadrant_status.go +++ b/controllers/kuadrant_status.go @@ -10,11 +10,9 @@ import ( "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" authorinov1beta1 "github.com/kuadrant/authorino-operator/api/v1beta1" kuadrantv1beta1 "github.com/kuadrant/kuadrant-operator/api/v1beta1" @@ -174,29 +172,3 @@ func (r *KuadrantReconciler) checkAuthorinoAvailable(ctx context.Context, kObj * return nil, nil } - -func BuildPolicyAffectedCondition(conditionType string, policyObject runtime.Object, targetRef metav1.Object, reason gatewayapiv1alpha2.PolicyConditionReason, err error) metav1.Condition { - condition := metav1.Condition{ - Type: conditionType, - Status: metav1.ConditionTrue, - Reason: string(reason), - ObservedGeneration: targetRef.GetGeneration(), - } - - objectMeta, metaErr := meta.Accessor(policyObject) - if metaErr != nil { - condition.Status = metav1.ConditionFalse - condition.Message = fmt.Sprintf("failed to get metadata about policy object %s", policyObject.GetObjectKind().GroupVersionKind().String()) - condition.Reason = PolicyReasonUnknown - return condition - } - if err != nil { - condition.Status = metav1.ConditionFalse - condition.Message = fmt.Sprintf("policy failed. Object unaffected by policy %s in namespace %s with name %s with error %s", policyObject.GetObjectKind().GroupVersionKind().String(), objectMeta.GetNamespace(), objectMeta.GetName(), err) - return condition - } - - condition.Message = fmt.Sprintf("policy success. Object affected by policy %s in namespace %s with name %s ", policyObject.GetObjectKind().GroupVersionKind().String(), objectMeta.GetNamespace(), objectMeta.GetName()) - - return condition -} diff --git a/controllers/ratelimitpolicy_controller.go b/controllers/ratelimitpolicy_controller.go index b5a973c3c..2b4d7d11a 100644 --- a/controllers/ratelimitpolicy_controller.go +++ b/controllers/ratelimitpolicy_controller.go @@ -19,6 +19,7 @@ package controllers import ( "context" "encoding/json" + "fmt" "github.com/go-logr/logr" limitadorv1alpha1 "github.com/kuadrant/limitador-operator/api/v1alpha1" @@ -50,8 +51,6 @@ type RateLimitPolicyReconciler struct { //+kubebuilder:rbac:groups=kuadrant.io,resources=ratelimitpolicies/status,verbs=get;update;patch //+kubebuilder:rbac:groups=kuadrant.io,resources=ratelimitpolicies/finalizers,verbs=update //+kubebuilder:rbac:groups=limitador.kuadrant.io,resources=limitadors,verbs=get;list;watch;create;update;patch;delete -//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=httproutes,verbs=get;list;watch;update;patch -//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways,verbs=get;list;watch;update;patch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -105,7 +104,7 @@ func (r *RateLimitPolicyReconciler) Reconcile(eventCtx context.Context, req ctrl targetNetworkObject = nil // we need the object set to nil when there's an error, otherwise deleting the resources (when marked for deletion) will panic } - // handle authpolicy marked for deletion + // handle ratelimitpolicy marked for deletion if markedForDeletion { if controllerutil.ContainsFinalizer(rlp, rateLimitPolicyFinalizer) { logger.V(1).Info("Handling removal of ratelimitpolicy object") @@ -179,16 +178,20 @@ func (r *RateLimitPolicyReconciler) reconcileResources(ctx context.Context, rlp } if err := r.reconcileLimits(ctx, rlp); err != nil { - return err + return fmt.Errorf("reconcile Limitador error %w", err) } // set direct back ref - i.e. claim the target network object as taken asap if err := r.reconcileNetworkResourceDirectBackReference(ctx, rlp, targetNetworkObject); err != nil { - return err + return fmt.Errorf("reconcile TargetBackReference error %w", err) } // set annotation of policies affecting the gateway - should be the last step, only when all the reconciliation steps succeed - return r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, rlp, gatewayDiffObj) + if err := r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, rlp, gatewayDiffObj); err != nil { + return fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err) + } + + return nil } func (r *RateLimitPolicyReconciler) deleteResources(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, targetNetworkObject client.Object) error { diff --git a/controllers/ratelimitpolicy_controller_test.go b/controllers/ratelimitpolicy_controller_test.go index 014f08f26..800f737ed 100644 --- a/controllers/ratelimitpolicy_controller_test.go +++ b/controllers/ratelimitpolicy_controller_test.go @@ -617,6 +617,19 @@ var _ = Describe("RateLimitPolicy controller", func() { }) Context("RLP accepted condition reasons", func() { + assertAcceptedConditionTrue := func(rlp *kuadrantv1beta2.RateLimitPolicy) func() bool { + return func() bool { + rlpKey := client.ObjectKeyFromObject(rlp) + existingRLP := &kuadrantv1beta2.RateLimitPolicy{} + err := k8sClient.Get(context.Background(), rlpKey, existingRLP) + if err != nil { + return false + } + + return meta.IsStatusConditionTrue(existingRLP.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + } + } + assertAcceptedConditionFalse := func(ctx context.Context, rlp *kuadrantv1beta2.RateLimitPolicy, reason, message string) func(g Gomega) { return func(g Gomega) { rlpKey := client.ObjectKeyFromObject(rlp) @@ -649,6 +662,8 @@ var _ = Describe("RateLimitPolicy controller", func() { Expect(k8sClient.Create(ctx, rlp)).To(Succeed()) Eventually(testRLPIsAccepted(client.ObjectKeyFromObject(rlp))).WithContext(ctx).Should(BeTrue()) + Eventually(assertAcceptedConditionTrue(rlp), time.Minute, 5*time.Second).Should(BeTrue()) + rlp2 := policyFactory(func(policy *kuadrantv1beta2.RateLimitPolicy) { policy.Name = "conflicting-rlp" }) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 6a3033b2a..e01720770 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -239,6 +239,18 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) + targetStatusBaseReconciler := reconcilers.NewBaseReconciler( + mgr.GetClient(), mgr.GetScheme(), mgr.GetAPIReader(), + log.Log.WithName("targetstatus"), + mgr.GetEventRecorderFor("PolicyTargetStatus"), + ) + + err = (&TargetStatusReconciler{ + BaseReconciler: targetStatusBaseReconciler, + }).SetupWithManager(mgr) + + Expect(err).NotTo(HaveOccurred()) + go func() { defer GinkgoRecover() err = mgr.Start(ctrl.SetupSignalHandler()) diff --git a/controllers/target_status_controller.go b/controllers/target_status_controller.go new file mode 100644 index 000000000..0127e7cc5 --- /dev/null +++ b/controllers/target_status_controller.go @@ -0,0 +1,371 @@ +package controllers + +/* +Copyright 2021 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import ( + "context" + "errors" + "fmt" + + "github.com/go-logr/logr" + "github.com/google/uuid" + 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" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + kuadrantv1beta2 "github.com/kuadrant/kuadrant-operator/api/v1beta2" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" + "github.com/kuadrant/kuadrant-operator/pkg/library/mappers" + "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" +) + +const PolicyAffectedConditionPattern = "kuadrant.io/%sAffected" // Policy kinds are expected to be named XPolicy + +// TargetStatusReconciler reconciles a the status stanzas of objects targeted by Kuadrant policies +type TargetStatusReconciler struct { + *reconcilers.BaseReconciler +} + +func (r *TargetStatusReconciler) Reconcile(eventCtx context.Context, req ctrl.Request) (ctrl.Result, error) { + logger := r.Logger().WithValues("gateway", req.NamespacedName, "request id", uuid.NewString()) + logger.Info("Reconciling target status") + ctx := logr.NewContext(eventCtx, logger) + + gw := &gatewayapiv1.Gateway{} + if err := r.Client().Get(ctx, req.NamespacedName, gw); err != nil { + if apierrors.IsNotFound(err) { + logger.Info("no gateway found") + return ctrl.Result{}, nil + } + logger.Error(err, "failed to get gateway") + return ctrl.Result{}, err + } + + if err := r.reconcileResources(ctx, gw); err != nil { + return ctrl.Result{}, err + } + + logger.Info("Target status reconciled successfully") + return ctrl.Result{}, nil +} + +func (r *TargetStatusReconciler) reconcileResources(ctx context.Context, gw *gatewayapiv1.Gateway) error { + policyKinds := map[kuadrantgatewayapi.Policy]client.ObjectList{ + &kuadrantv1beta2.AuthPolicy{TypeMeta: ctrl.TypeMeta{Kind: "AuthPolicy"}}: &kuadrantv1beta2.AuthPolicyList{}, + &kuadrantv1alpha1.DNSPolicy{TypeMeta: ctrl.TypeMeta{Kind: "DNSPolicy"}}: &kuadrantv1alpha1.DNSPolicyList{}, + &kuadrantv1alpha1.TLSPolicy{TypeMeta: ctrl.TypeMeta{Kind: "TLSPolicy"}}: &kuadrantv1alpha1.TLSPolicyList{}, + &kuadrantv1beta2.RateLimitPolicy{TypeMeta: ctrl.TypeMeta{Kind: "RateLimitPolicy"}}: &kuadrantv1beta2.RateLimitPolicyList{}, + } + + var errs error + + for policy, policyListKind := range policyKinds { + err := r.reconcileResourcesForPolicyKind(ctx, gw, policy, policyListKind) + if err != nil { + errs = errors.Join(errs, err) + } + } + + return errs +} + +func (r *TargetStatusReconciler) reconcileResourcesForPolicyKind(parentCtx context.Context, gw *gatewayapiv1.Gateway, policy kuadrantgatewayapi.Policy, listPolicyKind client.ObjectList) error { + logger, err := logr.FromContext(parentCtx) + if err != nil { + return err + } + gatewayKey := client.ObjectKeyFromObject(gw) + policyKind := policy.GetObjectKind().GroupVersionKind().Kind + ctx := logr.NewContext(parentCtx, logger.WithValues("kind", policyKind)) + + topology, err := r.buildTopology(ctx, gw, policyKind, listPolicyKind) + if err != nil { + return err + } + policies := topology.PoliciesFromGateway(gw) + gatewayPolicyExists := len(policies) > 0 && utils.Index(policies, func(p kuadrantgatewayapi.Policy) bool { return kuadrantgatewayapi.IsTargetRefGateway(p.GetTargetRef()) }) >= 0 + + var errs error + + // if no policies of a kind affecting the gateway → remove condition from the gateway and routes + if !gatewayPolicyExists { + // remove the condition from the gateway + conditionType := policyAffectedConditionType(policyKind) + if c := meta.FindStatusCondition(gw.Status.Conditions, conditionType); c == nil { + logger.V(1).Info("condition already absent, skipping", "condition", conditionType) + } else { + meta.RemoveStatusCondition(&gw.Status.Conditions, conditionType) + logger.V(1).Info("removing condition from gateway", "condition", conditionType) + if err := r.Client().Status().Update(ctx, gw); err != nil { + errs = errors.Join(errs, err) + } + } + + // remove the condition from the routes not targeted by any policy + if policy.PolicyClass() == kuadrantgatewayapi.InheritedPolicy { + if err := r.updateInheritedGatewayCondition(ctx, topology.GetUntargetedRoutes(gw), metav1.Condition{Type: conditionType}, gatewayKey, r.removeRouteCondition); err != nil { + errs = errors.Join(errs, err) + } + } + } + + var gatewayStatusUpdated bool + + // update the status of the gateway and its routes + for i := range policies { + policy := policies[i] + condition := buildPolicyAffectedCondition(policy) + + // update status of targeted route + if route := topology.GetPolicyHTTPRoute(policy); route != nil { + if err := r.addRouteCondition(ctx, route, condition, gatewayKey, kuadrant.ControllerName); err != nil { + errs = errors.Join(errs, err) + } + continue + } + + // update status of the gateway if not already updated before after observing another policy of same kind + // this assumes that the gateway is targeted by at most one policy of each kind + if gatewayStatusUpdated { + continue + } + if c := meta.FindStatusCondition(gw.Status.Conditions, condition.Type); c != nil && c.Status == condition.Status && c.Reason == condition.Reason && c.Message == condition.Message && c.ObservedGeneration == gw.GetGeneration() { + logger.V(1).Info("condition already up-to-date, skipping", "condition", condition.Type, "status", condition.Status) + } else { + gwCondition := condition.DeepCopy() + gwCondition.ObservedGeneration = gw.GetGeneration() + meta.SetStatusCondition(&gw.Status.Conditions, *gwCondition) + logger.V(1).Info("adding condition to gateway", "condition", condition.Type, "status", condition.Status) + if err := r.Client().Status().Update(ctx, gw); err != nil { + return err + } + } + // update status of all untargeted routes accepted by the gateway + if policy.PolicyClass() == kuadrantgatewayapi.InheritedPolicy { + if err := r.updateInheritedGatewayCondition(ctx, topology.GetUntargetedRoutes(gw), condition, gatewayKey, r.addRouteCondition); err != nil { + return err + } + } + gatewayStatusUpdated = true + } + + return errs +} + +func (r *TargetStatusReconciler) buildTopology(ctx context.Context, gw *gatewayapiv1.Gateway, policyKind string, listPolicyKind client.ObjectList) (*kuadrantgatewayapi.TopologyIndexes, error) { + logger, err := logr.FromContext(ctx) + if err != nil { + return nil, err + } + + routeList := &gatewayapiv1.HTTPRouteList{} + // Get all the routes having the gateway as parent + err = r.Client().List(ctx, routeList, client.MatchingFields{HTTPRouteGatewayParentField: client.ObjectKeyFromObject(gw).String()}) + logger.V(1).Info("list routes by gateway", "#routes", len(routeList.Items), "err", err) + if err != nil { + return nil, err + } + + policies, err := r.getPoliciesByKind(ctx, policyKind, listPolicyKind) + if err != nil { + return nil, err + } + + t, err := kuadrantgatewayapi.NewTopology( + kuadrantgatewayapi.WithGateways([]*gatewayapiv1.Gateway{gw}), + kuadrantgatewayapi.WithRoutes(utils.Map(routeList.Items, ptr.To[gatewayapiv1.HTTPRoute])), + kuadrantgatewayapi.WithPolicies(policies), + kuadrantgatewayapi.WithLogger(logger), + ) + if err != nil { + return nil, err + } + + return kuadrantgatewayapi.NewTopologyIndexes(t), nil +} + +func (r *TargetStatusReconciler) getPoliciesByKind(ctx context.Context, policyKind string, listKind client.ObjectList) ([]kuadrantgatewayapi.Policy, error) { + logger, _ := logr.FromContext(ctx) + logger = logger.WithValues("kind", policyKind) + + // Get all policies of the given kind + err := r.Client().List(ctx, listKind) + policyList, ok := listKind.(kuadrant.PolicyList) + if !ok { + return nil, fmt.Errorf("%T is not a kuadrant.PolicyList", listKind) + } + logger.V(1).Info("list policies by kind", "#policies", len(policyList.GetItems()), "err", err) + if err != nil { + return nil, err + } + + return utils.Map(policyList.GetItems(), func(p kuadrant.Policy) kuadrantgatewayapi.Policy { return p }), nil +} + +func (r *TargetStatusReconciler) updateInheritedGatewayCondition(ctx context.Context, routes []*gatewayapiv1.HTTPRoute, condition metav1.Condition, gatewayKey client.ObjectKey, update updateRouteConditionFunc) error { + logger, _ := logr.FromContext(ctx) + logger = logger.WithValues("condition", condition.Type, "status", condition.Status) + + logger.V(1).Info("update inherited gateway condition", "#routes", len(routes)) + + var errs error + + for i := range routes { + route := routes[i] + if err := update(ctx, route, condition, gatewayKey, kuadrant.ControllerName); err != nil { + errs = errors.Join(errs, err) + } + } + + return errs +} + +type updateRouteConditionFunc func(ctx context.Context, route *gatewayapiv1.HTTPRoute, condition metav1.Condition, gatewayKey client.ObjectKey, controllerName gatewayapiv1.GatewayController) error + +func (r *TargetStatusReconciler) addRouteCondition(ctx context.Context, route *gatewayapiv1.HTTPRoute, condition metav1.Condition, gatewayKey client.ObjectKey, controllerName gatewayapiv1.GatewayController) error { + logger, _ := logr.FromContext(ctx) + logger = logger.WithValues("route", client.ObjectKeyFromObject(route), "condition", condition.Type, "status", condition.Status) + + i := utils.Index(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, gatewayKey, controllerName)) + if i < 0 { + logger.V(1).Info("cannot find parent status, creating new one") + route.Status.RouteStatus.Parents = append(route.Status.RouteStatus.Parents, gatewayapiv1.RouteParentStatus{ + ControllerName: controllerName, + ParentRef: gatewayapiv1.ParentReference{ + Kind: ptr.To(gatewayapiv1.Kind("Gateway")), + Name: gatewayapiv1.ObjectName(gatewayKey.Name), + Namespace: ptr.To(gatewayapiv1.Namespace(gatewayKey.Namespace)), + }, + Conditions: []metav1.Condition{}, + }) + i = utils.Index(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, gatewayKey, controllerName)) + } + + if c := meta.FindStatusCondition(route.Status.RouteStatus.Parents[i].Conditions, condition.Type); c != nil && c.Status == condition.Status && c.Reason == condition.Reason && c.Message == condition.Message && c.ObservedGeneration == route.GetGeneration() { + logger.V(1).Info("condition already up-to-date, skipping") + return nil + } + + routeCondition := condition.DeepCopy() + routeCondition.ObservedGeneration = route.GetGeneration() + meta.SetStatusCondition(&(route.Status.RouteStatus.Parents[i].Conditions), *routeCondition) // Istio will merge the conditions from Kuadrant's parent status into its own parent status. See https://github.com/istio/istio/issues/50484 + logger.V(1).Info("adding condition to route") + return r.Client().Status().Update(ctx, route) +} + +func (r *TargetStatusReconciler) removeRouteCondition(ctx context.Context, route *gatewayapiv1.HTTPRoute, condition metav1.Condition, gatewayKey client.ObjectKey, controllerName gatewayapiv1.GatewayController) error { + logger, _ := logr.FromContext(ctx) + logger = logger.WithValues("route", client.ObjectKeyFromObject(route), "condition", condition.Type, "status", condition.Status) + + i := utils.Index(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, gatewayKey, controllerName)) + if i < 0 { + logger.V(1).Info("cannot find parent status, skipping") + return nil + } + + if c := meta.FindStatusCondition(route.Status.RouteStatus.Parents[i].Conditions, condition.Type); c == nil { + logger.V(1).Info("condition already absent, skipping") + return nil + } + + logger.V(1).Info("removing condition from route") + meta.RemoveStatusCondition(&(route.Status.RouteStatus.Parents[i].Conditions), condition.Type) + if len(route.Status.RouteStatus.Parents[i].Conditions) == 0 { + route.Status.RouteStatus.Parents = append(route.Status.RouteStatus.Parents[:i], route.Status.RouteStatus.Parents[i+1:]...) + } + return r.Client().Status().Update(ctx, route) +} + +func findRouteParentStatusFunc(route *gatewayapiv1.HTTPRoute, gatewayKey client.ObjectKey, controllerName gatewayapiv1.GatewayController) func(gatewayapiv1.RouteParentStatus) bool { + return func(p gatewayapiv1.RouteParentStatus) bool { + return *p.ParentRef.Kind == ("Gateway") && + p.ControllerName == controllerName && + ((p.ParentRef.Namespace == nil && route.GetNamespace() == gatewayKey.Namespace) || string(*p.ParentRef.Namespace) == gatewayKey.Namespace) && + string(p.ParentRef.Name) == gatewayKey.Name + } +} + +// SetupWithManager sets up the controller with the Manager. +func (r *TargetStatusReconciler) SetupWithManager(mgr ctrl.Manager) error { + httpRouteToParentGatewaysEventMapper := mappers.NewHTTPRouteToParentGatewaysEventMapper( + mappers.WithLogger(r.Logger().WithName("httpRouteToParentGatewaysEventMapper")), + ) + + policyToParentGatewaysEventMapper := mappers.NewPolicyToParentGatewaysEventMapper( + mappers.WithLogger(r.Logger().WithName("policyToParentGatewaysEventMapper")), + mappers.WithClient(r.Client()), + ) + + return ctrl.NewControllerManagedBy(mgr). + For(&gatewayapiv1.Gateway{}). + Watches( + &gatewayapiv1.HTTPRoute{}, + handler.EnqueueRequestsFromMapFunc(httpRouteToParentGatewaysEventMapper.Map), + ). + Watches( + &kuadrantv1beta2.AuthPolicy{}, + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + ). + Watches( + &kuadrantv1alpha1.DNSPolicy{}, + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + ). + Watches( + &kuadrantv1beta2.RateLimitPolicy{}, + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + ). + Watches( + &kuadrantv1alpha1.TLSPolicy{}, + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + ). + Complete(r) +} + +func buildPolicyAffectedCondition(policy kuadrantgatewayapi.Policy) metav1.Condition { + policyKind := policy.GetObjectKind().GroupVersionKind().Kind + + condition := metav1.Condition{ + Type: policyAffectedConditionType(policyKind), + Status: metav1.ConditionTrue, + Reason: string(gatewayapiv1alpha2.PolicyReasonAccepted), + Message: fmt.Sprintf("Object affected by %s %s", policyKind, client.ObjectKeyFromObject(policy)), + } + + if c := meta.FindStatusCondition(policy.GetStatus().GetConditions(), string(gatewayapiv1alpha2.PolicyConditionAccepted)); c == nil || c.Status != metav1.ConditionTrue { // should we aim for 'Enforced' instead? + condition.Status = metav1.ConditionFalse + condition.Message = fmt.Sprintf("Object unaffected by %s %s, policy is not accepted", policyKind, client.ObjectKeyFromObject(policy)) + condition.Reason = PolicyReasonUnknown + if c != nil { + condition.Reason = c.Reason + } + } + + return condition +} + +func policyAffectedConditionType(policyKind string) string { + return fmt.Sprintf(PolicyAffectedConditionPattern, policyKind) +} diff --git a/controllers/target_status_controller_test.go b/controllers/target_status_controller_test.go new file mode 100644 index 000000000..8f994240d --- /dev/null +++ b/controllers/target_status_controller_test.go @@ -0,0 +1,560 @@ +//go:build integration + +package controllers + +import ( + "context" + "path/filepath" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + certmanv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + certmanmetav1 "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + authorinoapi "github.com/kuadrant/authorino/api/v1beta2" + kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1" + "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" + gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + "github.com/kuadrant/kuadrant-operator/api/v1alpha1" + "github.com/kuadrant/kuadrant-operator/api/v1beta2" + "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" + "github.com/kuadrant/kuadrant-operator/pkg/library/utils" +) + +var _ = Describe("Target status reconciler", func() { + var testNamespace string + + BeforeEach(func() { + // create namespace + CreateNamespace(&testNamespace) + + // create gateway + gateway := testBuildBasicGateway(testGatewayName, testNamespace, func(gateway *gatewayapiv1.Gateway) { + gateway.Spec.Listeners = []gatewayapiv1.Listener{{ + Name: gatewayapiv1.SectionName("test-listener-toystore-com"), + Hostname: ptr.To(gatewayapiv1.Hostname("*.toystore.com")), + Port: gatewayapiv1.PortNumber(80), + Protocol: gatewayapiv1.HTTPProtocolType, + }} + }) + err := k8sClient.Create(context.Background(), gateway) + Expect(err).ToNot(HaveOccurred()) + + Eventually(testGatewayIsReady(gateway), 15*time.Second, 5*time.Second).Should(BeTrue()) + + // create kuadrant instance + ApplyKuadrantCR(testNamespace) + + // create application + err = ApplyResources(filepath.Join("..", "examples", "toystore", "toystore.yaml"), k8sClient, testNamespace) + Expect(err).ToNot(HaveOccurred()) + route := testBuildBasicHttpRoute(testHTTPRouteName, testGatewayName, testNamespace, []string{"*.toystore.com"}) + err = k8sClient.Create(context.Background(), route) + Expect(err).ToNot(HaveOccurred()) + Eventually(testRouteIsAccepted(client.ObjectKeyFromObject(route)), time.Minute, 5*time.Second).Should(BeTrue()) + }) + + AfterEach(DeleteNamespaceCallback(&testNamespace)) + + gatewayAffected := func(gatewayName, conditionType string, policyKey client.ObjectKey) bool { + gateway := &gatewayapiv1.Gateway{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: gatewayName, Namespace: testNamespace}, gateway) + if err != nil { + return false + } + condition := meta.FindStatusCondition(gateway.Status.Conditions, conditionType) + return condition != nil && condition.Status == metav1.ConditionTrue && strings.Contains(condition.Message, policyKey.String()) + } + + routeAffected := func(routeName, conditionType string, policyKey client.ObjectKey) bool { + route := &gatewayapiv1.HTTPRoute{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: routeName, Namespace: testNamespace}, route) + if err != nil { + return false + } + routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName)) + if !found { + return false + } + condition := meta.FindStatusCondition(routeParentStatus.Conditions, conditionType) + return condition.Status == metav1.ConditionTrue && strings.Contains(condition.Message, policyKey.String()) + } + + targetsAffected := func(policyKey client.ObjectKey, conditionType string, targetRef gatewayapiv1alpha2.PolicyTargetReference, routeNames ...string) bool { + switch string(targetRef.Kind) { + case "Gateway": + if !gatewayAffected(string(targetRef.Name), conditionType, policyKey) { + return false + } + case "HTTPRoute": + routeNames = append(routeNames, string(targetRef.Name)) + } + + for _, routeName := range routeNames { + if !routeAffected(routeName, conditionType, policyKey) { + return false + } + } + + return true + } + + Context("AuthPolicy", func() { + policyAffectedCondition := policyAffectedConditionType("AuthPolicy") + + // policyFactory builds a standards AuthPolicy object that targets the test HTTPRoute by default, with the given mutate functions applied + policyFactory := func(mutateFns ...func(policy *v1beta2.AuthPolicy)) *v1beta2.AuthPolicy { + policy := &v1beta2.AuthPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "AuthPolicy", + APIVersion: v1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "toystore", + Namespace: testNamespace, + }, + Spec: v1beta2.AuthPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "HTTPRoute", + Name: testHTTPRouteName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + }, + Defaults: &v1beta2.AuthPolicyCommonSpec{ + AuthScheme: &v1beta2.AuthSchemeSpec{ + Authentication: map[string]v1beta2.AuthenticationSpec{ + "anonymous": { + AuthenticationSpec: authorinoapi.AuthenticationSpec{ + AuthenticationMethodSpec: authorinoapi.AuthenticationMethodSpec{ + AnonymousAccess: &authorinoapi.AnonymousAccessSpec{}, + }, + }, + }, + }, + }, + }, + }, + } + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + return policy + } + + // policyAcceptedAndTargetsAffected returns an assertion function that checks if an AuthPolicy is accepted + // and the statuses of its target object and other optional route objects have been all updated as affected by the policy + policyAcceptedAndTargetsAffected := func(policy *v1beta2.AuthPolicy, routeNames ...string) func() bool { + return func() bool { + if !isAuthPolicyAccepted(policy)() { + return false + } + return targetsAffected(client.ObjectKeyFromObject(policy), policyAffectedCondition, policy.Spec.TargetRef, routeNames...) + } + } + + It("adds PolicyAffected status condition to the targeted route", func() { + policy := policyFactory() + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("removes PolicyAffected status condition from the targeted route when the policy is deleted", func() { + policy := policyFactory() + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed()) + + Eventually(func() bool { // route is not affected by the policy + route := &gatewayapiv1.HTTPRoute{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: testHTTPRouteName, Namespace: testNamespace}, route) + if err != nil { + return false + } + routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName)) + return !found || meta.IsStatusConditionFalse(routeParentStatus.Conditions, policyAffectedCondition) + }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("adds PolicyAffected status condition to the targeted gateway and routes", func() { + policy := policyFactory(func(policy *v1beta2.AuthPolicy) { + policy.Name = "gateway-auth" + policy.Spec.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: testGatewayName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + } + }) + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("removes PolicyAffected status condition from the targeted gateway and routes when the policy is deleted", func() { + policy := policyFactory(func(policy *v1beta2.AuthPolicy) { + policy.Name = "gateway-auth" + policy.Spec.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: testGatewayName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + } + }) + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed()) + + Eventually(func() bool { // gateway and route not affected by the policy + gateway := &gatewayapiv1.Gateway{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, gateway) + if err != nil || meta.IsStatusConditionTrue(gateway.Status.Conditions, policyAffectedCondition) { + return false + } + + route := &gatewayapiv1.HTTPRoute{} + err = k8sClient.Get(context.Background(), client.ObjectKey{Name: testHTTPRouteName, Namespace: testNamespace}, route) + if err != nil { + return false + } + routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName)) + return !found || meta.IsStatusConditionFalse(routeParentStatus.Conditions, policyAffectedCondition) + }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("adds PolicyAffected status condition to the targeted gateway and non-targeted routes", func() { + routePolicy := policyFactory() + Expect(k8sClient.Create(context.Background(), routePolicy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + otherRouteName := testHTTPRouteName + "-other" + otherRoute := testBuildBasicHttpRoute(otherRouteName, testGatewayName, testNamespace, []string{"other.toystore.com"}) + Expect(k8sClient.Create(context.Background(), otherRoute)).To(Succeed()) + + gatewayPolicy := policyFactory(func(policy *v1beta2.AuthPolicy) { + policy.Name = "gateway-auth" + policy.Spec.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: testGatewayName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + } + }) + Expect(k8sClient.Create(context.Background(), gatewayPolicy)).To(Succeed()) + + Eventually(func() bool { + return testRouteIsAccepted(client.ObjectKeyFromObject(otherRoute))() && + policyAcceptedAndTargetsAffected(routePolicy)() && + policyAcceptedAndTargetsAffected(gatewayPolicy, otherRouteName)() + }, time.Minute, 5*time.Second).Should(BeTrue()) + + // remove route policy and check if the gateway policy has been rolled out to the status of the newly non-targeted route + Expect(k8sClient.Delete(context.Background(), routePolicy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(gatewayPolicy, otherRouteName, testHTTPRouteName), time.Minute, 5*time.Second).Should(BeTrue()) + }) + }) + + Context("RateLimitPolicy", func() { + policyAffectedCondition := policyAffectedConditionType("RateLimitPolicy") + + // policyFactory builds a standards RateLimitPolicy object that targets the test HTTPRoute by default, with the given mutate functions applied + policyFactory := func(mutateFns ...func(policy *v1beta2.RateLimitPolicy)) *v1beta2.RateLimitPolicy { + policy := &v1beta2.RateLimitPolicy{ + TypeMeta: metav1.TypeMeta{ + Kind: "RateLimitPolicy", + APIVersion: v1beta2.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "toystore", + Namespace: testNamespace, + }, + Spec: v1beta2.RateLimitPolicySpec{ + TargetRef: gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "HTTPRoute", + Name: gatewayapiv1.ObjectName(testHTTPRouteName), + }, + Defaults: &v1beta2.RateLimitPolicyCommonSpec{ + Limits: map[string]v1beta2.Limit{ + "l1": { + Rates: []v1beta2.Rate{ + { + Limit: 1, Duration: 3, Unit: v1beta2.TimeUnit("minute"), + }, + }, + }, + }, + }, + }, + } + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + return policy + } + + // policyAcceptedAndTargetsAffected returns an assertion function that checks if an RateLimitPolicy is accepted + // and the statuses of its target object and other optional route objects have been all updated as affected by the policy + policyAcceptedAndTargetsAffected := func(policy *v1beta2.RateLimitPolicy, routeNames ...string) func() bool { + return func() bool { + policyKey := client.ObjectKeyFromObject(policy) + if !testRLPIsAccepted(policyKey)() { + return false + } + return targetsAffected(policyKey, policyAffectedCondition, policy.Spec.TargetRef, routeNames...) + } + } + + It("adds PolicyAffected status condition to the targeted route", func() { + policy := policyFactory() + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("removes PolicyAffected status condition from the targeted route when the policy is deleted", func() { + policy := policyFactory() + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed()) + + Eventually(func() bool { // route is not affected by the policy + route := &gatewayapiv1.HTTPRoute{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: testHTTPRouteName, Namespace: testNamespace}, route) + if err != nil { + return false + } + routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName)) + return !found || meta.IsStatusConditionFalse(routeParentStatus.Conditions, policyAffectedCondition) + }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("adds PolicyAffected status condition to the targeted gateway and routes", func() { + policy := policyFactory(func(policy *v1beta2.RateLimitPolicy) { + policy.Name = "gateway-rlp" + policy.Spec.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: testGatewayName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + } + }) + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("removes PolicyAffected status condition from the targeted gateway and routes when the policy is deleted", func() { + policy := policyFactory(func(policy *v1beta2.RateLimitPolicy) { + policy.Name = "gateway-rlp" + policy.Spec.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: testGatewayName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + } + }) + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed()) + + Eventually(func() bool { // gateway and route not affected by the policy + gateway := &gatewayapiv1.Gateway{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, gateway) + if err != nil || meta.IsStatusConditionTrue(gateway.Status.Conditions, policyAffectedCondition) { + return false + } + + route := &gatewayapiv1.HTTPRoute{} + err = k8sClient.Get(context.Background(), client.ObjectKey{Name: testHTTPRouteName, Namespace: testNamespace}, route) + if err != nil { + return false + } + routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, findRouteParentStatusFunc(route, client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, kuadrant.ControllerName)) + return !found || meta.IsStatusConditionFalse(routeParentStatus.Conditions, policyAffectedCondition) + }, 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("adds PolicyAffected status condition to the targeted gateway and non-targeted routes", func() { + routePolicy := policyFactory() + Expect(k8sClient.Create(context.Background(), routePolicy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(routePolicy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + otherRouteName := testHTTPRouteName + "-other" + otherRoute := testBuildBasicHttpRoute(otherRouteName, testGatewayName, testNamespace, []string{"other.toystore.com"}) + Expect(k8sClient.Create(context.Background(), otherRoute)).To(Succeed()) + + gatewayPolicy := policyFactory(func(policy *v1beta2.RateLimitPolicy) { + policy.Name = "gateway-rlp" + policy.Spec.TargetRef = gatewayapiv1alpha2.PolicyTargetReference{ + Group: gatewayapiv1.GroupName, + Kind: "Gateway", + Name: testGatewayName, + Namespace: ptr.To(gatewayapiv1.Namespace(testNamespace)), + } + }) + Expect(k8sClient.Create(context.Background(), gatewayPolicy)).To(Succeed()) + + Eventually(func() bool { + return testRouteIsAccepted(client.ObjectKeyFromObject(otherRoute))() && + policyAcceptedAndTargetsAffected(routePolicy)() && + policyAcceptedAndTargetsAffected(gatewayPolicy, otherRouteName)() + }, time.Minute, 5*time.Second).Should(BeTrue()) + + // remove route policy and check if the gateway policy has been rolled out to the status of the newly non-targeted route + Expect(k8sClient.Delete(context.Background(), routePolicy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(gatewayPolicy, otherRouteName, testHTTPRouteName), time.Minute, 5*time.Second).Should(BeTrue()) + }) + }) + + Context("DNSPolicy", func() { + policyAffectedCondition := policyAffectedConditionType("DNSPolicy") + + // policyFactory builds a standards DNSPolicy object that targets the test gateway by default, with the given mutate functions applied + policyFactory := func(mutateFns ...func(policy *v1alpha1.DNSPolicy)) *v1alpha1.DNSPolicy { + policy := v1alpha1.NewDNSPolicy("test-dns-policy", testNamespace).WithTargetGateway(testGatewayName).WithRoutingStrategy(v1alpha1.SimpleRoutingStrategy) + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + return policy + } + + isDNSPolicyAccepted := func(policyKey client.ObjectKey) bool { + policy := &v1alpha1.DNSPolicy{} + err := k8sClient.Get(context.Background(), policyKey, policy) + if err != nil { + return false + } + return meta.IsStatusConditionTrue(policy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + } + + // policyAcceptedAndTargetsAffected returns an assertion function that checks if a DNSPolicy is accepted + // and the statuses of its target object has been all updated as affected by the policy + policyAcceptedAndTargetsAffected := func(policy *v1alpha1.DNSPolicy) func() bool { + return func() bool { + policyKey := client.ObjectKeyFromObject(policy) + if !isDNSPolicyAccepted(policyKey) { + return false + } + return targetsAffected(policyKey, policyAffectedCondition, policy.Spec.TargetRef) + } + } + + var managedZone *kuadrantdnsv1alpha1.ManagedZone + + BeforeEach(func() { + managedZone = testBuildManagedZone("mz-toystore-com", testNamespace, "toystore.com") + Expect(k8sClient.Create(context.Background(), managedZone)).To(Succeed()) + }) + + AfterEach(func() { + Expect(k8sClient.Delete(context.Background(), managedZone)).To(Succeed()) + }) + + It("adds PolicyAffected status condition to the targeted gateway", func() { + policy := policyFactory() + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("removes PolicyAffected status condition from the targeted gateway when the policy is deleted", func() { + policy := policyFactory() + policyKey := client.ObjectKeyFromObject(policy) + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed()) + + Eventually(func() bool { + gateway := &gatewayapiv1.Gateway{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, gateway) + if err != nil { + return false + } + condition := meta.FindStatusCondition(gateway.Status.Conditions, testGatewayName) + return condition == nil || !strings.Contains(condition.Message, policyKey.String()) || condition.Status == metav1.ConditionFalse + }) + }) + }) + + Context("TLSPolicy", func() { + policyAffectedCondition := policyAffectedConditionType("TLSPolicy") + + var issuer *certmanv1.Issuer + var issuerRef *certmanmetav1.ObjectReference + + // policyFactory builds a standards TLSPolicy object that targets the test gateway by default, with the given mutate functions applied + policyFactory := func(mutateFns ...func(policy *v1alpha1.TLSPolicy)) *v1alpha1.TLSPolicy { + policy := v1alpha1.NewTLSPolicy("test-tls-policy", testNamespace).WithTargetGateway(testGatewayName).WithIssuerRef(*issuerRef) + for _, mutateFn := range mutateFns { + mutateFn(policy) + } + return policy + } + + isTLSPolicyAccepted := func(policyKey client.ObjectKey) bool { + policy := &v1alpha1.TLSPolicy{} + err := k8sClient.Get(context.Background(), policyKey, policy) + if err != nil { + return false + } + return meta.IsStatusConditionTrue(policy.Status.Conditions, string(gatewayapiv1alpha2.PolicyConditionAccepted)) + } + + // policyAcceptedAndTargetsAffected returns an assertion function that checks if a TLSPolicy is accepted + // and the statuses of its target object has been all updated as affected by the policy + policyAcceptedAndTargetsAffected := func(policy *v1alpha1.TLSPolicy) func() bool { + return func() bool { + policyKey := client.ObjectKeyFromObject(policy) + if !isTLSPolicyAccepted(policyKey) { + return false + } + return targetsAffected(policyKey, policyAffectedCondition, policy.Spec.TargetRef) + } + } + + BeforeEach(func() { + issuer, issuerRef = testBuildSelfSignedIssuer("testissuer", testNamespace) + Expect(k8sClient.Create(context.Background(), issuer)).To(BeNil()) + }) + + AfterEach(func() { + if issuer != nil { + err := k8sClient.Delete(context.Background(), issuer) + Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred()) + } + }) + + It("adds PolicyAffected status condition to the targeted gateway", func() { + policy := policyFactory() + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + }) + + It("removes PolicyAffected status condition from the targeted gateway when the policy is deleted", func() { + policy := policyFactory() + policyKey := client.ObjectKeyFromObject(policy) + Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) + Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + + Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed()) + + Eventually(func() bool { + gateway := &gatewayapiv1.Gateway{} + err := k8sClient.Get(context.Background(), client.ObjectKey{Name: testGatewayName, Namespace: testNamespace}, gateway) + if err != nil { + return false + } + condition := meta.FindStatusCondition(gateway.Status.Conditions, testGatewayName) + return condition == nil || !strings.Contains(condition.Message, policyKey.String()) || condition.Status == metav1.ConditionFalse + }) + }) + }) +}) diff --git a/controllers/tlspolicy_controller.go b/controllers/tlspolicy_controller.go index a935afbaf..6f9156642 100644 --- a/controllers/tlspolicy_controller.go +++ b/controllers/tlspolicy_controller.go @@ -18,14 +18,10 @@ package controllers import ( "context" - "errors" "fmt" - "reflect" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -33,7 +29,6 @@ import ( crlog "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/kuadrant/kuadrant-operator/api/v1alpha1" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" @@ -41,10 +36,7 @@ import ( "github.com/kuadrant/kuadrant-operator/pkg/library/reconcilers" ) -const ( - TLSPolicyFinalizer = "kuadrant.io/tls-policy" - TLSPolicyAffected = "kuadrant.io/TLSPolicyAffected" -) +const TLSPolicyFinalizer = "kuadrant.io/tls-policy" // TLSPolicyReconciler reconciles a TLSPolicy object type TLSPolicyReconciler struct { @@ -136,8 +128,6 @@ func (r *TLSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } func (r *TLSPolicyReconciler) reconcileResources(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) error { - gatewayCondition := BuildPolicyAffectedCondition(TLSPolicyAffected, tlsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyReasonAccepted, nil) - // validate err := tlsPolicy.Validate() if err != nil { @@ -156,29 +146,17 @@ func (r *TLSPolicyReconciler) reconcileResources(ctx context.Context, tlsPolicy } if err = r.reconcileCertificates(ctx, tlsPolicy, gatewayDiffObj); err != nil { - gatewayCondition = BuildPolicyAffectedCondition(TLSPolicyAffected, tlsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyReasonInvalid, err) - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - return errors.Join(fmt.Errorf("reconcile Certificates error %w", err), updateErr) + return fmt.Errorf("reconcile Certificates error %w", err) } // set direct back ref - i.e. claim the target network object as taken asap if err = r.TargetRefReconciler.ReconcileTargetBackReference(ctx, tlsPolicy, targetNetworkObject, tlsPolicy.DirectReferenceAnnotationName()); err != nil { - gatewayCondition = BuildPolicyAffectedCondition(TLSPolicyAffected, tlsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyReasonConflicted, err) - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - return errors.Join(fmt.Errorf("reconcile TargetBackReference error %w", err), updateErr) + return fmt.Errorf("reconcile TargetBackReference error %w", err) } // set annotation of policies affecting the gateway if err = r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, tlsPolicy, gatewayDiffObj); err != nil { - gatewayCondition = BuildPolicyAffectedCondition(TLSPolicyAffected, tlsPolicy, targetNetworkObject, gatewayapiv1alpha2.PolicyConditionReason(PolicyReasonUnknown), err) - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - return errors.Join(fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err), updateErr) - } - - // set gateway policy affected condition status - should be the last step, only when all the reconciliation steps succeed - updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) - if updateErr != nil { - return fmt.Errorf("failed to update gateway conditions %w ", updateErr) + return fmt.Errorf("ReconcileGatewayPolicyReferences error %w", err) } return nil @@ -203,40 +181,7 @@ func (r *TLSPolicyReconciler) deleteResources(ctx context.Context, tlsPolicy *v1 } // update annotation of policies affecting the gateway - if err := r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, tlsPolicy, gatewayDiffObj); err != nil { - return err - } - - // remove gateway policy affected condition status - return r.updateGatewayCondition(ctx, metav1.Condition{Type: string(TLSPolicyAffected)}, gatewayDiffObj) -} - -func (r *TLSPolicyReconciler) updateGatewayCondition(ctx context.Context, condition metav1.Condition, gatewayDiff *reconcilers.GatewayDiffs) error { - // update condition if needed - gatewayDiffs := append(gatewayDiff.GatewaysWithValidPolicyRef, gatewayDiff.GatewaysMissingPolicyRef...) - for i, gw := range gatewayDiffs { - previous := gw.DeepCopy() - meta.SetStatusCondition(&gatewayDiffs[i].Status.Conditions, condition) - if !reflect.DeepEqual(previous.Status.Conditions, gw.Status.Conditions) { - if err := r.Client().Status().Update(ctx, gw.Gateway); err != nil { - return err - } - } - } - - // remove condition from gateway that is no longer referenced - gatewayDiffs = gatewayDiff.GatewaysWithInvalidPolicyRef - for i, gw := range gatewayDiffs { - previous := gw.DeepCopy() - meta.RemoveStatusCondition(&gatewayDiffs[i].Status.Conditions, condition.Type) - if !reflect.DeepEqual(previous.Status.Conditions, gw.Status.Conditions) { - if err := r.Client().Status().Update(ctx, gw.Gateway); err != nil { - return err - } - } - } - - return nil + return r.TargetRefReconciler.ReconcileGatewayPolicyReferences(ctx, tlsPolicy, gatewayDiffObj) } // SetupWithManager sets up the controller with the Manager. diff --git a/controllers/tlspolicy_controller_test.go b/controllers/tlspolicy_controller_test.go index 8251356b1..9392578bc 100644 --- a/controllers/tlspolicy_controller_test.go +++ b/controllers/tlspolicy_controller_test.go @@ -168,14 +168,6 @@ var _ = Describe("TLSPolicy controller", Ordered, func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(gw.Annotations).To(HaveKeyWithValue(v1alpha1.TLSPolicyDirectReferenceAnnotationName, policyBackRefValue)) g.Expect(gw.Annotations).To(HaveKeyWithValue(v1alpha1.TLSPolicyBackReferenceAnnotationName, policiesBackRefValue)) - //Check status - g.Expect(gw.Status.Conditions).To( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(TLSPolicyAffected), - "Status": Equal(metav1.ConditionTrue), - "Reason": Equal(string(gatewayapiv1alpha2.PolicyConditionAccepted)), - })), - ) }, TestTimeoutMedium, time.Second).Should(Succeed()) }) }) diff --git a/main.go b/main.go index 9cdba3237..3a86d237b 100644 --- a/main.go +++ b/main.go @@ -242,6 +242,18 @@ func main() { os.Exit(1) } + targetStatusBaseReconciler := reconcilers.NewBaseReconciler( + mgr.GetClient(), mgr.GetScheme(), mgr.GetAPIReader(), + log.Log.WithName("targetstatus"), + mgr.GetEventRecorderFor("PolicyTargetStatus"), + ) + if err = (&controllers.TargetStatusReconciler{ + BaseReconciler: targetStatusBaseReconciler, + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "TargetStatusReconciler") + os.Exit(1) + } + //+kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/pkg/library/gatewayapi/types.go b/pkg/library/gatewayapi/types.go index 751f37bfd..76784acf1 100644 --- a/pkg/library/gatewayapi/types.go +++ b/pkg/library/gatewayapi/types.go @@ -1,14 +1,28 @@ package gatewayapi import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) +type PolicyClass int + +const ( + DirectPolicy PolicyClass = iota + InheritedPolicy +) + type Policy interface { client.Object + PolicyClass() PolicyClass GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference + GetStatus() PolicyStatus +} + +type PolicyStatus interface { + GetConditions() []metav1.Condition } type PolicyByCreationTimestamp []Policy diff --git a/pkg/library/gatewayapi/types_test.go b/pkg/library/gatewayapi/types_test.go index 5c500993f..35addb4b3 100644 --- a/pkg/library/gatewayapi/types_test.go +++ b/pkg/library/gatewayapi/types_test.go @@ -14,6 +14,11 @@ import ( gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" ) +var ( + _ Policy = &TestPolicy{} + _ PolicyStatus = &FakePolicyStatus{} +) + type TestPolicy struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` @@ -21,14 +26,18 @@ type TestPolicy struct { TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` } -var ( - _ Policy = &TestPolicy{} -) +func (p *TestPolicy) PolicyClass() PolicyClass { + return DirectPolicy +} func (p *TestPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { return p.TargetRef } +func (p *TestPolicy) GetStatus() PolicyStatus { + return &FakePolicyStatus{} +} + func (p *TestPolicy) DeepCopyObject() runtime.Object { if c := p.DeepCopy(); c != nil { return c @@ -52,6 +61,12 @@ func (p *TestPolicy) DeepCopyInto(out *TestPolicy) { p.TargetRef.DeepCopyInto(&out.TargetRef) } +type FakePolicyStatus struct{} + +func (s *FakePolicyStatus) GetConditions() []metav1.Condition { + return nil +} + func TestPolicyByCreationTimestamp(t *testing.T) { testCases := []struct { name string diff --git a/pkg/library/gatewayapi/utils.go b/pkg/library/gatewayapi/utils.go index 4436e0f66..21f7ec620 100644 --- a/pkg/library/gatewayapi/utils.go +++ b/pkg/library/gatewayapi/utils.go @@ -79,6 +79,8 @@ func GetGatewayWorkloadSelector(ctx context.Context, cli client.Client, gateway return utils.GetServiceWorkloadSelector(ctx, cli, serviceKey) } +// IsHTTPRouteAccepted returns true if a given HTTPRoute has the Accepted status condition added by any of its +// parentRefs; otherwise, it returns false func IsHTTPRouteAccepted(httpRoute *gatewayapiv1.HTTPRoute) bool { acceptedParentRefs := GetRouteAcceptedParentRefs(httpRoute) @@ -89,39 +91,38 @@ func IsHTTPRouteAccepted(httpRoute *gatewayapiv1.HTTPRoute) bool { return len(acceptedParentRefs) == len(httpRoute.Spec.ParentRefs) } -func IsParentGateway(ref gatewayapiv1.ParentReference) bool { - return (ref.Kind == nil || *ref.Kind == "Gateway") && (ref.Group == nil || *ref.Group == gatewayapiv1.GroupName) +// GetRouteAcceptedGatewayParentKeys returns the object keys of all gateways that have accepted a given route +func GetRouteAcceptedGatewayParentKeys(route *gatewayapiv1.HTTPRoute) []client.ObjectKey { + acceptedParentRefs := GetRouteAcceptedParentRefs(route) + + gatewayParentRefs := utils.Filter(acceptedParentRefs, IsParentGateway) + + return utils.Map(gatewayParentRefs, func(p gatewayapiv1.ParentReference) client.ObjectKey { + return client.ObjectKey{ + Name: string(p.Name), + Namespace: string(ptr.Deref(p.Namespace, gatewayapiv1.Namespace(route.Namespace))), + } + }) } +// GetRouteAcceptedParentRefs returns the list of parentRefs for which a given route has the Accepted status condition func GetRouteAcceptedParentRefs(route *gatewayapiv1.HTTPRoute) []gatewayapiv1.ParentReference { if route == nil { return nil } return utils.Filter(route.Spec.ParentRefs, func(p gatewayapiv1.ParentReference) bool { - parentStatus, found := utils.Find(route.Status.RouteStatus.Parents, func(pStatus gatewayapiv1.RouteParentStatus) bool { - return reflect.DeepEqual(pStatus.ParentRef, p) - }) - - if !found { - return false + for _, parentStatus := range route.Status.RouteStatus.Parents { + if reflect.DeepEqual(parentStatus.ParentRef, p) && meta.IsStatusConditionTrue(parentStatus.Conditions, string(gatewayapiv1.RouteConditionAccepted)) { + return true + } } - - return meta.IsStatusConditionTrue(parentStatus.Conditions, "Accepted") + return false }) } -func GetRouteAcceptedGatewayParentKeys(route *gatewayapiv1.HTTPRoute) []client.ObjectKey { - acceptedParentRefs := GetRouteAcceptedParentRefs(route) - - gatewayParentRefs := utils.Filter(acceptedParentRefs, IsParentGateway) - - return utils.Map(gatewayParentRefs, func(p gatewayapiv1.ParentReference) client.ObjectKey { - return client.ObjectKey{ - Name: string(p.Name), - Namespace: string(ptr.Deref(p.Namespace, gatewayapiv1.Namespace(route.Namespace))), - } - }) +func IsParentGateway(ref gatewayapiv1.ParentReference) bool { + return (ref.Kind == nil || *ref.Kind == "Gateway") && (ref.Group == nil || *ref.Group == gatewayapiv1.GroupName) } // FilterValidSubdomains returns every subdomain that is a subset of at least one of the (super) domains specified in the first argument. diff --git a/pkg/library/kuadrant/kuadrant.go b/pkg/library/kuadrant/kuadrant.go index 8163f6896..5518ea94c 100644 --- a/pkg/library/kuadrant/kuadrant.go +++ b/pkg/library/kuadrant/kuadrant.go @@ -18,6 +18,7 @@ import ( const ( KuadrantNamespaceAnnotation = "kuadrant.io/namespace" + ControllerName = "kuadrant.io/policy-controller" ) type Policy interface { diff --git a/pkg/library/kuadrant/test_utils.go b/pkg/library/kuadrant/test_utils.go index 5a4cac7f2..8371c1aec 100644 --- a/pkg/library/kuadrant/test_utils.go +++ b/pkg/library/kuadrant/test_utils.go @@ -3,9 +3,12 @@ package kuadrant import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" ) var _ Referrer = &PolicyKindStub{} @@ -38,6 +41,10 @@ func (p *FakePolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { return p.targetRef } +func (p *FakePolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { + return &FakePolicyStatus{} +} + func (p *FakePolicy) GetWrappedNamespace() gatewayapiv1.Namespace { return gatewayapiv1.Namespace(p.GetNamespace()) } @@ -49,3 +56,13 @@ func (p *FakePolicy) GetRulesHostnames() []string { func (p *FakePolicy) Kind() string { return "FakePolicy" } + +func (_ *FakePolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { + return kuadrantgatewayapi.DirectPolicy +} + +type FakePolicyStatus struct{} + +func (s *FakePolicyStatus) GetConditions() []metav1.Condition { + return nil +} diff --git a/pkg/library/mappers/policy_to_gateway.go b/pkg/library/mappers/policy_to_gateway.go index de7f6f1c8..ba98adaa8 100644 --- a/pkg/library/mappers/policy_to_gateway.go +++ b/pkg/library/mappers/policy_to_gateway.go @@ -26,7 +26,7 @@ func NewPolicyToParentGatewaysEventMapper(o ...MapperOption) *PolicyToParentGate } func (k *PolicyToParentGatewaysEventMapper) Map(ctx context.Context, obj client.Object) []reconcile.Request { - logger := k.opts.Logger.WithValues("object", client.ObjectKeyFromObject(obj)) + logger := k.opts.Logger.WithValues("object", client.ObjectKeyFromObject(obj), "kind", obj.GetObjectKind().GroupVersionKind().Kind) policy, ok := obj.(kuadrantgatewayapi.Policy) if !ok { diff --git a/pkg/library/reconcilers/target_ref_reconciler.go b/pkg/library/reconcilers/target_ref_reconciler.go index 79ddd2f15..b50c76384 100644 --- a/pkg/library/reconcilers/target_ref_reconciler.go +++ b/pkg/library/reconcilers/target_ref_reconciler.go @@ -35,10 +35,17 @@ type TargetRefReconciler struct { } // FetchAcceptedGatewayHTTPRoutes returns the list of HTTPRoutes that have been accepted as children of a gateway. -func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context, gwKey client.ObjectKey) (routes []gatewayapiv1.HTTPRoute) { +func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context, gateway *gatewayapiv1.Gateway) (routes []gatewayapiv1.HTTPRoute) { + gwKey := client.ObjectKeyFromObject(gateway) logger, _ := logr.FromContext(ctx) logger = logger.WithName("FetchAcceptedGatewayHTTPRoutes").WithValues("gateway", gwKey) + gatewayClass := &gatewayapiv1.GatewayClass{} + if err := r.Client.Get(ctx, client.ObjectKey{Name: string(gateway.Spec.GatewayClassName)}, gatewayClass); err != nil { + logger.V(1).Info("failed to get controller name", "err", err) + return + } + routeList := &gatewayapiv1.HTTPRouteList{} err := r.Client.List(ctx, routeList) if err != nil { @@ -50,10 +57,11 @@ func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context route := routeList.Items[idx] routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, func(p gatewayapiv1.RouteParentStatus) bool { return *p.ParentRef.Kind == ("Gateway") && + p.ControllerName == gatewayClass.Spec.ControllerName && ((p.ParentRef.Namespace == nil && route.GetNamespace() == gwKey.Namespace) || string(*p.ParentRef.Namespace) == gwKey.Namespace) && string(p.ParentRef.Name) == gwKey.Name }) - if found && meta.IsStatusConditionTrue(routeParentStatus.Conditions, "Accepted") { + if found && meta.IsStatusConditionTrue(routeParentStatus.Conditions, string(gatewayapiv1.RouteConditionAccepted)) { logger.V(1).Info("found route attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) routes = append(routes, route) continue @@ -62,7 +70,7 @@ func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context logger.V(1).Info("skipping route, not attached to gateway", "httproute", client.ObjectKeyFromObject(&route), "isChildRoute", found, - "isAccepted", routeParentStatus != nil && meta.IsStatusConditionTrue(routeParentStatus.Conditions, "Accepted")) + "isAccepted", routeParentStatus != nil && meta.IsStatusConditionTrue(routeParentStatus.Conditions, string(gatewayapiv1.RouteConditionAccepted))) } return diff --git a/pkg/library/utils/slice_utils.go b/pkg/library/utils/slice_utils.go index de5f8793f..34dacaf92 100644 --- a/pkg/library/utils/slice_utils.go +++ b/pkg/library/utils/slice_utils.go @@ -48,12 +48,19 @@ func Intersection[T comparable](slice1, slice2 []T) []T { return result } -func Find[T any](slice []T, match func(T) bool) (*T, bool) { - for _, item := range slice { +func Index[T any](slice []T, match func(T) bool) int { + for i, item := range slice { if match(item) { - return &item, true + return i } } + return -1 +} + +func Find[T any](slice []T, match func(T) bool) (*T, bool) { + if i := Index(slice, match); i >= 0 { + return &slice[i], true + } return nil, false } From 5609855c4b8592e3f2cbc402eaf6a380cfffdbf2 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Fri, 19 Apr 2024 20:32:53 +0200 Subject: [PATCH 2/5] refactor: fetch routes accepted by a given gateway without specifying the gateway controller name Removes the need for authorizing reading gateway classes --- ...adrant-operator.clusterserviceversion.yaml | 10 +-------- config/rbac/role.yaml | 8 ------- controllers/kuadrant_controller.go | 1 - .../reconcilers/target_ref_reconciler.go | 21 +++---------------- 4 files changed, 4 insertions(+), 36 deletions(-) diff --git a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml index b75cfe43e..1238b6533 100644 --- a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml +++ b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml @@ -106,7 +106,7 @@ metadata: capabilities: Basic Install categories: Integration & Delivery containerImage: quay.io/kuadrant/kuadrant-operator:latest - createdAt: "2024-04-22T18:16:55Z" + createdAt: "2024-04-22T18:17:29Z" operators.operatorframework.io/builder: operator-sdk-v1.32.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 repository: https://github.com/Kuadrant/kuadrant-operator @@ -282,14 +282,6 @@ spec: - patch - update - watch - - apiGroups: - - gateway.networking.k8s.io - resources: - - gatewayclasses - verbs: - - get - - list - - watch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 7afd7f41a..7d89b1790 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -130,14 +130,6 @@ rules: - patch - update - watch -- apiGroups: - - gateway.networking.k8s.io - resources: - - gatewayclasses - verbs: - - get - - list - - watch - apiGroups: - gateway.networking.k8s.io resources: diff --git a/controllers/kuadrant_controller.go b/controllers/kuadrant_controller.go index 77a55702a..0adfe9b28 100644 --- a/controllers/kuadrant_controller.go +++ b/controllers/kuadrant_controller.go @@ -74,7 +74,6 @@ type KuadrantReconciler struct { //+kubebuilder:rbac:groups=maistra.io,resources=servicemeshmembers,verbs=get;list;watch;create;update;delete;patch // Common permissions required by policy controllers -//+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gatewayclasses,verbs=get;list;watch //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways,verbs=get;list;watch;update;patch //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways/status,verbs=get;update;patch //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=gateways/finalizers,verbs=update diff --git a/pkg/library/reconcilers/target_ref_reconciler.go b/pkg/library/reconcilers/target_ref_reconciler.go index b50c76384..b8ec58aca 100644 --- a/pkg/library/reconcilers/target_ref_reconciler.go +++ b/pkg/library/reconcilers/target_ref_reconciler.go @@ -26,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" "github.com/kuadrant/kuadrant-operator/pkg/library/kuadrant" "github.com/kuadrant/kuadrant-operator/pkg/library/utils" ) @@ -40,12 +41,6 @@ func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context logger, _ := logr.FromContext(ctx) logger = logger.WithName("FetchAcceptedGatewayHTTPRoutes").WithValues("gateway", gwKey) - gatewayClass := &gatewayapiv1.GatewayClass{} - if err := r.Client.Get(ctx, client.ObjectKey{Name: string(gateway.Spec.GatewayClassName)}, gatewayClass); err != nil { - logger.V(1).Info("failed to get controller name", "err", err) - return - } - routeList := &gatewayapiv1.HTTPRouteList{} err := r.Client.List(ctx, routeList) if err != nil { @@ -55,22 +50,12 @@ func (r *TargetRefReconciler) FetchAcceptedGatewayHTTPRoutes(ctx context.Context for idx := range routeList.Items { route := routeList.Items[idx] - routeParentStatus, found := utils.Find(route.Status.RouteStatus.Parents, func(p gatewayapiv1.RouteParentStatus) bool { - return *p.ParentRef.Kind == ("Gateway") && - p.ControllerName == gatewayClass.Spec.ControllerName && - ((p.ParentRef.Namespace == nil && route.GetNamespace() == gwKey.Namespace) || string(*p.ParentRef.Namespace) == gwKey.Namespace) && - string(p.ParentRef.Name) == gwKey.Name - }) - if found && meta.IsStatusConditionTrue(routeParentStatus.Conditions, string(gatewayapiv1.RouteConditionAccepted)) { + if utils.Index(kuadrantgatewayapi.GetRouteAcceptedGatewayParentKeys(&route), func(parentGatewayKey client.ObjectKey) bool { return parentGatewayKey == gwKey }) >= 0 { logger.V(1).Info("found route attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) routes = append(routes, route) continue } - - logger.V(1).Info("skipping route, not attached to gateway", - "httproute", client.ObjectKeyFromObject(&route), - "isChildRoute", found, - "isAccepted", routeParentStatus != nil && meta.IsStatusConditionTrue(routeParentStatus.Conditions, string(gatewayapiv1.RouteConditionAccepted))) + logger.V(1).Info("skipping route, not attached to gateway", "httproute", client.ObjectKeyFromObject(&route)) } return From 6aac8a3acbc18efd68402860b5d349f6a608a9fa Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Sat, 20 Apr 2024 15:19:28 +0200 Subject: [PATCH 3/5] Trigger target status reconciliation for policies when the policy status changed only --- controllers/policy_status_event_handler.go | 96 ++++++++ .../policy_status_event_handler_test.go | 205 ++++++++++++++++++ controllers/target_status_controller.go | 8 +- 3 files changed, 305 insertions(+), 4 deletions(-) create mode 100644 controllers/policy_status_event_handler.go create mode 100644 controllers/policy_status_event_handler_test.go diff --git a/controllers/policy_status_event_handler.go b/controllers/policy_status_event_handler.go new file mode 100644 index 000000000..e7bbaf6bc --- /dev/null +++ b/controllers/policy_status_event_handler.go @@ -0,0 +1,96 @@ +package controllers + +import ( + "context" + "reflect" + + "k8s.io/client-go/util/workqueue" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" +) + +// PolicyStatusEventHandlerFromMapFunc returns a PolicyStatusEventHandler that handles events from a mapping function. +func PolicyStatusEventHandlerFromMapFunc(mapFunc handler.MapFunc) handler.EventHandler { + return NewPolicyStatusEventHandler(WithHandler(handler.EnqueueRequestsFromMapFunc(mapFunc))) +} + +// NewPolicyStatusEventHandler returns a new PolicyStatusEventHandler. +func NewPolicyStatusEventHandler(opts ...PolicyStatusEventHandlerOption) handler.EventHandler { + h := &PolicyStatusEventHandler{} + for _, opt := range opts { + opt(h) + } + return h +} + +type PolicyStatusEventHandlerOption func(*PolicyStatusEventHandler) + +func WithHandler(h handler.EventHandler) PolicyStatusEventHandlerOption { + return func(p *PolicyStatusEventHandler) { + p.handler = h + } +} + +var _ handler.EventHandler = &PolicyStatusEventHandler{} + +// PolicyStatusEventHandler enqueues reconcile.Requests in response to events for Policy objects +// whose status blocks have changed. +// The handling of the events is delegated to the provided handler. +type PolicyStatusEventHandler struct { + handler handler.EventHandler +} + +// Create implements EventHandler. +func (h *PolicyStatusEventHandler) Create(ctx context.Context, evt event.CreateEvent, q workqueue.RateLimitingInterface) { + if h.handler == nil { + return + } + h.handler.Create(ctx, evt, q) +} + +// Update implements EventHandler. +func (h *PolicyStatusEventHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.RateLimitingInterface) { + if h.handler == nil { + return + } + oldPolicy, ok := evt.ObjectOld.(kuadrantgatewayapi.Policy) + if !ok { + return + } + newPolicy, ok := evt.ObjectNew.(kuadrantgatewayapi.Policy) + if !ok { + return + } + if statusChanged(oldPolicy, newPolicy) { + h.handler.Update(ctx, evt, q) + } +} + +// Delete implements EventHandler. +func (h *PolicyStatusEventHandler) Delete(ctx context.Context, evt event.DeleteEvent, q workqueue.RateLimitingInterface) { + if h.handler == nil { + return + } + h.handler.Delete(ctx, evt, q) +} + +// Generic implements EventHandler. +func (h *PolicyStatusEventHandler) Generic(ctx context.Context, evt event.GenericEvent, q workqueue.RateLimitingInterface) { + if h.handler == nil { + return + } + h.handler.Generic(ctx, evt, q) +} + +func statusChanged(old, new kuadrantgatewayapi.Policy) bool { + if old == nil || new == nil { + return false + } + + oldStatus := old.GetStatus() + newStatus := new.GetStatus() + + return !reflect.DeepEqual(oldStatus, newStatus) +} diff --git a/controllers/policy_status_event_handler_test.go b/controllers/policy_status_event_handler_test.go new file mode 100644 index 000000000..65a2838cd --- /dev/null +++ b/controllers/policy_status_event_handler_test.go @@ -0,0 +1,205 @@ +//go:build unit + +package controllers + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/util/workqueue" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + + kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" +) + +var _ handler.EventHandler = &testEventHandler{} + +type testEventHandler struct { + lastEventFunc string +} + +func (h *testEventHandler) Create(_ context.Context, _ event.CreateEvent, _ workqueue.RateLimitingInterface) { + h.lastEventFunc = "Create" +} +func (h *testEventHandler) Update(_ context.Context, _ event.UpdateEvent, _ workqueue.RateLimitingInterface) { + h.lastEventFunc = "Update" +} +func (h *testEventHandler) Delete(_ context.Context, _ event.DeleteEvent, _ workqueue.RateLimitingInterface) { + h.lastEventFunc = "Delete" +} +func (h *testEventHandler) Generic(_ context.Context, _ event.GenericEvent, _ workqueue.RateLimitingInterface) { + h.lastEventFunc = "Generic" +} + +// Test policy that implements kuadrantgatewayapi.Policy + +var ( + _ kuadrantgatewayapi.Policy = &TestPolicy{} + _ kuadrantgatewayapi.PolicyStatus = &FakePolicyStatus{} +) + +type TestPolicy struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` + Status FakePolicyStatus `json:"status,omitempty"` +} + +func (p *TestPolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { + return kuadrantgatewayapi.DirectPolicy +} + +func (p *TestPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { + return p.TargetRef +} + +func (p *TestPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { + return &p.Status +} + +func (p *TestPolicy) DeepCopyObject() runtime.Object { + if c := p.DeepCopy(); c != nil { + return c + } + return nil +} + +func (p *TestPolicy) DeepCopy() *TestPolicy { + if p == nil { + return nil + } + out := new(TestPolicy) + p.DeepCopyInto(out) + return out +} + +func (p *TestPolicy) DeepCopyInto(out *TestPolicy) { + *out = *p + out.TypeMeta = p.TypeMeta + p.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + p.TargetRef.DeepCopyInto(&out.TargetRef) + out.Status = p.Status +} + +type FakePolicyStatus struct { + Conditions []metav1.Condition +} + +func (s *FakePolicyStatus) GetConditions() []metav1.Condition { + return s.Conditions +} + +func TestPolicyStatusEventHandler(t *testing.T) { + tests := []struct { + name string + lastEventFunc func() string + expected string + }{ + { + name: "Create event", + lastEventFunc: func() string { + testHandler := &testEventHandler{} + h := NewPolicyStatusEventHandler(WithHandler(testHandler)) + h.Create(context.Background(), event.CreateEvent{}, nil) + return testHandler.lastEventFunc + }, + expected: "Create", + }, + { + name: "Update event with different status", + lastEventFunc: func() string { + testHandler := &testEventHandler{} + h := NewPolicyStatusEventHandler(WithHandler(testHandler)) + ev := event.UpdateEvent{ + ObjectOld: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{}, + }, + }, + ObjectNew: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "ValidPolicy", + }, + }, + }, + }, + } + h.Update(context.Background(), ev, nil) + return testHandler.lastEventFunc + }, + expected: "Update", + }, + { + name: "Update event without different status", + lastEventFunc: func() string { + testHandler := &testEventHandler{} + h := NewPolicyStatusEventHandler(WithHandler(testHandler)) + ev := event.UpdateEvent{ + ObjectOld: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "ValidPolicy", + }, + }, + }, + }, + ObjectNew: &TestPolicy{ + Status: FakePolicyStatus{ + Conditions: []metav1.Condition{ + { + Type: "Accepted", + Status: metav1.ConditionTrue, + Reason: "ValidPolicy", + }, + }, + }, + }, + } + h.Update(context.Background(), ev, nil) + return testHandler.lastEventFunc + }, + expected: "", + }, + { + name: "Delete event", + lastEventFunc: func() string { + testHandler := &testEventHandler{} + h := NewPolicyStatusEventHandler(WithHandler(testHandler)) + h.Delete(context.Background(), event.DeleteEvent{}, nil) + return testHandler.lastEventFunc + }, + expected: "Delete", + }, + { + name: "Generic event", + lastEventFunc: func() string { + testHandler := &testEventHandler{} + h := NewPolicyStatusEventHandler(WithHandler(testHandler)) + h.Generic(context.Background(), event.GenericEvent{}, nil) + return testHandler.lastEventFunc + }, + expected: "Generic", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.lastEventFunc() + if got != tt.expected { + t.Errorf("%s failed. Expected %s, got %s", tt.name, tt.expected, got) + } + }) + } +} diff --git a/controllers/target_status_controller.go b/controllers/target_status_controller.go index 0127e7cc5..f7a4a7965 100644 --- a/controllers/target_status_controller.go +++ b/controllers/target_status_controller.go @@ -327,19 +327,19 @@ func (r *TargetStatusReconciler) SetupWithManager(mgr ctrl.Manager) error { ). Watches( &kuadrantv1beta2.AuthPolicy{}, - handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), ). Watches( &kuadrantv1alpha1.DNSPolicy{}, - handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), ). Watches( &kuadrantv1beta2.RateLimitPolicy{}, - handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), ). Watches( &kuadrantv1alpha1.TLSPolicy{}, - handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), ). Complete(r) } From e7e8b3838df3bb8c2783db879cd0dd70933015e3 Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Mon, 22 Apr 2024 13:10:20 +0200 Subject: [PATCH 4/5] refactor: trigger target status reconciliation for policies when the policy status changed only based on simple predicate function instead of custom handler --- controllers/policy_status_event_handler.go | 96 -------- .../policy_status_event_handler_test.go | 205 ------------------ controllers/target_status_controller.go | 32 ++- 3 files changed, 28 insertions(+), 305 deletions(-) delete mode 100644 controllers/policy_status_event_handler.go delete mode 100644 controllers/policy_status_event_handler_test.go diff --git a/controllers/policy_status_event_handler.go b/controllers/policy_status_event_handler.go deleted file mode 100644 index e7bbaf6bc..000000000 --- a/controllers/policy_status_event_handler.go +++ /dev/null @@ -1,96 +0,0 @@ -package controllers - -import ( - "context" - "reflect" - - "k8s.io/client-go/util/workqueue" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" - - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" -) - -// PolicyStatusEventHandlerFromMapFunc returns a PolicyStatusEventHandler that handles events from a mapping function. -func PolicyStatusEventHandlerFromMapFunc(mapFunc handler.MapFunc) handler.EventHandler { - return NewPolicyStatusEventHandler(WithHandler(handler.EnqueueRequestsFromMapFunc(mapFunc))) -} - -// NewPolicyStatusEventHandler returns a new PolicyStatusEventHandler. -func NewPolicyStatusEventHandler(opts ...PolicyStatusEventHandlerOption) handler.EventHandler { - h := &PolicyStatusEventHandler{} - for _, opt := range opts { - opt(h) - } - return h -} - -type PolicyStatusEventHandlerOption func(*PolicyStatusEventHandler) - -func WithHandler(h handler.EventHandler) PolicyStatusEventHandlerOption { - return func(p *PolicyStatusEventHandler) { - p.handler = h - } -} - -var _ handler.EventHandler = &PolicyStatusEventHandler{} - -// PolicyStatusEventHandler enqueues reconcile.Requests in response to events for Policy objects -// whose status blocks have changed. -// The handling of the events is delegated to the provided handler. -type PolicyStatusEventHandler struct { - handler handler.EventHandler -} - -// Create implements EventHandler. -func (h *PolicyStatusEventHandler) Create(ctx context.Context, evt event.CreateEvent, q workqueue.RateLimitingInterface) { - if h.handler == nil { - return - } - h.handler.Create(ctx, evt, q) -} - -// Update implements EventHandler. -func (h *PolicyStatusEventHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.RateLimitingInterface) { - if h.handler == nil { - return - } - oldPolicy, ok := evt.ObjectOld.(kuadrantgatewayapi.Policy) - if !ok { - return - } - newPolicy, ok := evt.ObjectNew.(kuadrantgatewayapi.Policy) - if !ok { - return - } - if statusChanged(oldPolicy, newPolicy) { - h.handler.Update(ctx, evt, q) - } -} - -// Delete implements EventHandler. -func (h *PolicyStatusEventHandler) Delete(ctx context.Context, evt event.DeleteEvent, q workqueue.RateLimitingInterface) { - if h.handler == nil { - return - } - h.handler.Delete(ctx, evt, q) -} - -// Generic implements EventHandler. -func (h *PolicyStatusEventHandler) Generic(ctx context.Context, evt event.GenericEvent, q workqueue.RateLimitingInterface) { - if h.handler == nil { - return - } - h.handler.Generic(ctx, evt, q) -} - -func statusChanged(old, new kuadrantgatewayapi.Policy) bool { - if old == nil || new == nil { - return false - } - - oldStatus := old.GetStatus() - newStatus := new.GetStatus() - - return !reflect.DeepEqual(oldStatus, newStatus) -} diff --git a/controllers/policy_status_event_handler_test.go b/controllers/policy_status_event_handler_test.go deleted file mode 100644 index 65a2838cd..000000000 --- a/controllers/policy_status_event_handler_test.go +++ /dev/null @@ -1,205 +0,0 @@ -//go:build unit - -package controllers - -import ( - "context" - "testing" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/util/workqueue" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" - gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - - kuadrantgatewayapi "github.com/kuadrant/kuadrant-operator/pkg/library/gatewayapi" -) - -var _ handler.EventHandler = &testEventHandler{} - -type testEventHandler struct { - lastEventFunc string -} - -func (h *testEventHandler) Create(_ context.Context, _ event.CreateEvent, _ workqueue.RateLimitingInterface) { - h.lastEventFunc = "Create" -} -func (h *testEventHandler) Update(_ context.Context, _ event.UpdateEvent, _ workqueue.RateLimitingInterface) { - h.lastEventFunc = "Update" -} -func (h *testEventHandler) Delete(_ context.Context, _ event.DeleteEvent, _ workqueue.RateLimitingInterface) { - h.lastEventFunc = "Delete" -} -func (h *testEventHandler) Generic(_ context.Context, _ event.GenericEvent, _ workqueue.RateLimitingInterface) { - h.lastEventFunc = "Generic" -} - -// Test policy that implements kuadrantgatewayapi.Policy - -var ( - _ kuadrantgatewayapi.Policy = &TestPolicy{} - _ kuadrantgatewayapi.PolicyStatus = &FakePolicyStatus{} -) - -type TestPolicy struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` - Status FakePolicyStatus `json:"status,omitempty"` -} - -func (p *TestPolicy) PolicyClass() kuadrantgatewayapi.PolicyClass { - return kuadrantgatewayapi.DirectPolicy -} - -func (p *TestPolicy) GetTargetRef() gatewayapiv1alpha2.PolicyTargetReference { - return p.TargetRef -} - -func (p *TestPolicy) GetStatus() kuadrantgatewayapi.PolicyStatus { - return &p.Status -} - -func (p *TestPolicy) DeepCopyObject() runtime.Object { - if c := p.DeepCopy(); c != nil { - return c - } - return nil -} - -func (p *TestPolicy) DeepCopy() *TestPolicy { - if p == nil { - return nil - } - out := new(TestPolicy) - p.DeepCopyInto(out) - return out -} - -func (p *TestPolicy) DeepCopyInto(out *TestPolicy) { - *out = *p - out.TypeMeta = p.TypeMeta - p.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - p.TargetRef.DeepCopyInto(&out.TargetRef) - out.Status = p.Status -} - -type FakePolicyStatus struct { - Conditions []metav1.Condition -} - -func (s *FakePolicyStatus) GetConditions() []metav1.Condition { - return s.Conditions -} - -func TestPolicyStatusEventHandler(t *testing.T) { - tests := []struct { - name string - lastEventFunc func() string - expected string - }{ - { - name: "Create event", - lastEventFunc: func() string { - testHandler := &testEventHandler{} - h := NewPolicyStatusEventHandler(WithHandler(testHandler)) - h.Create(context.Background(), event.CreateEvent{}, nil) - return testHandler.lastEventFunc - }, - expected: "Create", - }, - { - name: "Update event with different status", - lastEventFunc: func() string { - testHandler := &testEventHandler{} - h := NewPolicyStatusEventHandler(WithHandler(testHandler)) - ev := event.UpdateEvent{ - ObjectOld: &TestPolicy{ - Status: FakePolicyStatus{ - Conditions: []metav1.Condition{}, - }, - }, - ObjectNew: &TestPolicy{ - Status: FakePolicyStatus{ - Conditions: []metav1.Condition{ - { - Type: "Accepted", - Status: metav1.ConditionTrue, - Reason: "ValidPolicy", - }, - }, - }, - }, - } - h.Update(context.Background(), ev, nil) - return testHandler.lastEventFunc - }, - expected: "Update", - }, - { - name: "Update event without different status", - lastEventFunc: func() string { - testHandler := &testEventHandler{} - h := NewPolicyStatusEventHandler(WithHandler(testHandler)) - ev := event.UpdateEvent{ - ObjectOld: &TestPolicy{ - Status: FakePolicyStatus{ - Conditions: []metav1.Condition{ - { - Type: "Accepted", - Status: metav1.ConditionTrue, - Reason: "ValidPolicy", - }, - }, - }, - }, - ObjectNew: &TestPolicy{ - Status: FakePolicyStatus{ - Conditions: []metav1.Condition{ - { - Type: "Accepted", - Status: metav1.ConditionTrue, - Reason: "ValidPolicy", - }, - }, - }, - }, - } - h.Update(context.Background(), ev, nil) - return testHandler.lastEventFunc - }, - expected: "", - }, - { - name: "Delete event", - lastEventFunc: func() string { - testHandler := &testEventHandler{} - h := NewPolicyStatusEventHandler(WithHandler(testHandler)) - h.Delete(context.Background(), event.DeleteEvent{}, nil) - return testHandler.lastEventFunc - }, - expected: "Delete", - }, - { - name: "Generic event", - lastEventFunc: func() string { - testHandler := &testEventHandler{} - h := NewPolicyStatusEventHandler(WithHandler(testHandler)) - h.Generic(context.Background(), event.GenericEvent{}, nil) - return testHandler.lastEventFunc - }, - expected: "Generic", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := tt.lastEventFunc() - if got != tt.expected { - t.Errorf("%s failed. Expected %s, got %s", tt.name, tt.expected, got) - } - }) - } -} diff --git a/controllers/target_status_controller.go b/controllers/target_status_controller.go index f7a4a7965..d7cafd6b0 100644 --- a/controllers/target_status_controller.go +++ b/controllers/target_status_controller.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "reflect" "github.com/go-logr/logr" "github.com/google/uuid" @@ -28,8 +29,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -319,6 +323,22 @@ func (r *TargetStatusReconciler) SetupWithManager(mgr ctrl.Manager) error { mappers.WithClient(r.Client()), ) + policyStatusChangedPredicate := predicate.Funcs{ + UpdateFunc: func(ev event.UpdateEvent) bool { + oldPolicy, ok := ev.ObjectOld.(kuadrantgatewayapi.Policy) + if !ok { + return false + } + newPolicy, ok := ev.ObjectNew.(kuadrantgatewayapi.Policy) + if !ok { + return false + } + oldStatus := oldPolicy.GetStatus() + newStatus := newPolicy.GetStatus() + return !reflect.DeepEqual(oldStatus, newStatus) + }, + } + return ctrl.NewControllerManagedBy(mgr). For(&gatewayapiv1.Gateway{}). Watches( @@ -327,19 +347,23 @@ func (r *TargetStatusReconciler) SetupWithManager(mgr ctrl.Manager) error { ). Watches( &kuadrantv1beta2.AuthPolicy{}, - PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + builder.WithPredicates(policyStatusChangedPredicate), ). Watches( &kuadrantv1alpha1.DNSPolicy{}, - PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + builder.WithPredicates(policyStatusChangedPredicate), ). Watches( &kuadrantv1beta2.RateLimitPolicy{}, - PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + builder.WithPredicates(policyStatusChangedPredicate), ). Watches( &kuadrantv1alpha1.TLSPolicy{}, - PolicyStatusEventHandlerFromMapFunc(policyToParentGatewaysEventMapper.Map), + handler.EnqueueRequestsFromMapFunc(policyToParentGatewaysEventMapper.Map), + builder.WithPredicates(policyStatusChangedPredicate), ). Complete(r) } From 30cd6be417d23b3000a4f5bce5b83bc14319de5b Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Tue, 23 Apr 2024 12:39:15 +0200 Subject: [PATCH 5/5] tests: fixed integration test for policyaffected condition inherited by the routes --- controllers/target_status_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/target_status_controller_test.go b/controllers/target_status_controller_test.go index 8f994240d..43ce81084 100644 --- a/controllers/target_status_controller_test.go +++ b/controllers/target_status_controller_test.go @@ -194,7 +194,7 @@ var _ = Describe("Target status reconciler", func() { } }) Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) - Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(policyAcceptedAndTargetsAffected(policy, testHTTPRouteName), 30*time.Second, 5*time.Second).Should(BeTrue()) }) It("removes PolicyAffected status condition from the targeted gateway and routes when the policy is deleted", func() { @@ -347,7 +347,7 @@ var _ = Describe("Target status reconciler", func() { } }) Expect(k8sClient.Create(context.Background(), policy)).To(Succeed()) - Eventually(policyAcceptedAndTargetsAffected(policy), 30*time.Second, 5*time.Second).Should(BeTrue()) + Eventually(policyAcceptedAndTargetsAffected(policy, testHTTPRouteName), 30*time.Second, 5*time.Second).Should(BeTrue()) }) It("removes PolicyAffected status condition from the targeted gateway and routes when the policy is deleted", func() {