Skip to content

fix: enqueue a single request for all resources #1813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zirain merged 3 commits into from
Aug 22, 2023
+43 −45
Merged

fix: enqueue a single request for all resources #1813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18483f4
Enqueue a single request for all resources
dboslee Aug 21, 2023
1c6e1e4
Remove namespace from log
dboslee Aug 21, 2023
aaa25af
Add comment and missing enqueue class
dboslee Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 36 additions & 29 deletions internal/provider/kubernetes/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,11 @@ func newResourceMapping() *resourceMappings {
}
}

func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
r.log.WithName(request.Name).Info("reconciling object", "namespace", request.Namespace, "name", request.Name)
// Reconcile handles reconciling all resources in a single call. Any resource event should enqueue the
// same reconcile.Request containing the gateway controller name. This allows multiple resource updates to
// be handled by a single call to Reconcile. The reconcile.Request DOES NOT map to a specific resource.
func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, _ reconcile.Request) (reconcile.Result, error) {
r.log.Info("reconciling gateways")

var gatewayClasses gwapiv1b1.GatewayClassList
if err := r.client.List(ctx, &gatewayClasses); err != nil {
Expand Down Expand Up @@ -313,7 +316,7 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, request reconcile.
// Store will be required to trigger a cleanup of envoy infra resources.
r.resources.GatewayAPIResources.Store(acceptedGC.Name, resourceTree)

r.log.WithName(request.Name).Info("reconciled gatewayAPI object successfully", "namespace", request.Namespace, "name", request.Name)
r.log.Info("reconciled gateways successfully")
return reconcile.Result{}, nil
}

Expand Down Expand Up @@ -1155,7 +1158,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Only enqueue GatewayClass objects that match this Envoy Gateway's controller name.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1b1.GatewayClass{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.hasMatchingController),
); err != nil {
return err
Expand All @@ -1164,16 +1167,17 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Only enqueue EnvoyProxy objects that match this Envoy Gateway's GatewayClass.
if err := c.Watch(
source.Kind(mgr.GetCache(), &egcfgv1a1.EnvoyProxy{}),
handler.EnqueueRequestsFromMapFunc(r.enqueueManagedClass),
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.ResourceVersionChangedPredicate{},
predicate.NewPredicateFuncs(r.hasManagedClass),
); err != nil {
return err
}

// Watch Gateway CRUDs and reconcile affected GatewayClass.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1b1.Gateway{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.validateGatewayForReconcile),
); err != nil {
return err
Expand All @@ -1185,7 +1189,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch HTTPRoute CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1b1.HTTPRoute{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
); err != nil {
return err
}
Expand All @@ -1196,7 +1200,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch GRPCRoute CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1a2.GRPCRoute{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
); err != nil {
return err
}
Expand All @@ -1207,7 +1211,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch TLSRoute CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1a2.TLSRoute{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
); err != nil {
return err
}
Expand All @@ -1218,7 +1222,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch UDPRoute CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1a2.UDPRoute{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
); err != nil {
return err
}
Expand All @@ -1229,7 +1233,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch TCPRoute CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1a2.TCPRoute{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
); err != nil {
return err
}
Expand All @@ -1240,15 +1244,15 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch Service CRUDs and process affected *Route objects.
if err := c.Watch(
source.Kind(mgr.GetCache(), &corev1.Service{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.validateServiceForReconcile)); err != nil {
return err
}

// Watch EndpointSlice CRUDs and process affected *Route objects.
if err := c.Watch(
source.Kind(mgr.GetCache(), &discoveryv1.EndpointSlice{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.validateEndpointSliceForReconcile)); err != nil {
return err
}
Expand All @@ -1258,7 +1262,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// resource address.
if err := c.Watch(
source.Kind(mgr.GetCache(), &corev1.Node{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.handleNode),
); err != nil {
return err
Expand All @@ -1267,7 +1271,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch Secret CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &corev1.Secret{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.validateSecretForReconcile),
); err != nil {
return err
Expand All @@ -1276,7 +1280,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch ReferenceGrant CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &gwapiv1a2.ReferenceGrant{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
); err != nil {
return err
}
Expand All @@ -1287,7 +1291,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch Deployment CRUDs and process affected Gateways.
if err := c.Watch(
source.Kind(mgr.GetCache(), &appsv1.Deployment{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.validateDeploymentForReconcile),
); err != nil {
return err
Expand All @@ -1296,15 +1300,15 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch AuthenticationFilter CRUDs and enqueue associated HTTPRoute objects.
if err := c.Watch(
source.Kind(mgr.GetCache(), &egv1a1.AuthenticationFilter{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
predicate.NewPredicateFuncs(r.httpRoutesForAuthenticationFilter)); err != nil {
return err
}

// Watch RateLimitFilter CRUDs and enqueue associated HTTPRoute objects.
if err := c.Watch(
source.Kind(mgr.GetCache(), &egv1a1.RateLimitFilter{}),
&handler.EnqueueRequestForObject{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass),
Copy link
Copy Markdown
Contributor

@arkodg arkodg Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also do this for L1318 ?

predicate.NewPredicateFuncs(r.httpRoutesForRateLimitFilter)); err != nil {
return err
}
Expand All @@ -1314,7 +1318,7 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
// Watch EnvoyPatchPolicy CRUDs
if err := c.Watch(
source.Kind(mgr.GetCache(), &egv1a1.EnvoyPatchPolicy{}),
&handler.EnqueueRequestForObject{}); err != nil {
handler.EnqueueRequestsFromMapFunc(r.enqueueClass)); err != nil {
return err
}
}
Expand All @@ -1326,15 +1330,21 @@ func (r *gatewayAPIReconciler) watchResources(ctx context.Context, mgr manager.M
u := &unstructured.Unstructured{}
u.SetGroupVersionKind(gvk)
if err := c.Watch(source.Kind(mgr.GetCache(), u),
&handler.EnqueueRequestForObject{}); err != nil {
handler.EnqueueRequestsFromMapFunc(r.enqueueClass)); err != nil {
return err
}
r.log.Info("Watching additional resource", "resource", gvk.String())
}
return nil
}

func (r *gatewayAPIReconciler) enqueueManagedClass(_ context.Context, obj client.Object) []reconcile.Request {
func (r *gatewayAPIReconciler) enqueueClass(_ context.Context, _ client.Object) []reconcile.Request {
return []reconcile.Request{{NamespacedName: types.NamespacedName{
Name: string(r.classController),
}}}
}

func (r *gatewayAPIReconciler) hasManagedClass(obj client.Object) bool {
ep, ok := obj.(*egcfgv1a1.EnvoyProxy)
if !ok {
panic(fmt.Sprintf("unsupported object type %T", obj))
Expand All @@ -1344,14 +1354,14 @@ func (r *gatewayAPIReconciler) enqueueManagedClass(_ context.Context, obj client
if ep.Namespace != r.namespace {
r.log.Info("envoyproxy namespace does not match Envoy Gateway's namespace",
"namespace", ep.Namespace, "name", ep.Name)
return []reconcile.Request{}
return false
}

gcList := new(gwapiv1b1.GatewayClassList)
err := r.client.List(context.TODO(), gcList)
if err != nil {
r.log.Error(err, "failed to list gatewayclasses")
return []reconcile.Request{}
return false
}

for i := range gcList.Items {
Expand All @@ -1360,14 +1370,11 @@ func (r *gatewayAPIReconciler) enqueueManagedClass(_ context.Context, obj client
if r.hasMatchingController(&gc) &&
classAccepted(&gc) &&
classRefsEnvoyProxy(&gc, ep) {
req := reconcile.Request{
NamespacedName: types.NamespacedName{Name: gc.Name},
}
return []reconcile.Request{req}
return true
}
}

return []reconcile.Request{}
return false
}

// processParamsRef processes the parametersRef of the provided GatewayClass.
Expand Down
23 changes: 7 additions & 16 deletions internal/provider/kubernetes/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
gwapiv1b1 "sigs.k8s.io/gateway-api/apis/v1beta1"

egcfgv1a1 "github.com/envoyproxy/gateway/api/config/v1alpha1"
Expand Down Expand Up @@ -152,14 +151,14 @@ func TestRemoveGatewayClassFinalizer(t *testing.T) {
}
}

func TestEnqueueManagedClass(t *testing.T) {
func TestHasManagedClass(t *testing.T) {
gcCtrlName := gwapiv1b1.GatewayController(egcfgv1a1.GatewayControllerName)

testCases := []struct {
name string
ep client.Object
classes []*gwapiv1b1.GatewayClass
expected []reconcile.Request
expected bool
}{
{
name: "no matching gatewayclasses",
Expand Down Expand Up @@ -193,7 +192,7 @@ func TestEnqueueManagedClass(t *testing.T) {
},
},
},
expected: []reconcile.Request{},
expected: false,
},
{
name: "match one gatewayclass",
Expand Down Expand Up @@ -227,11 +226,7 @@ func TestEnqueueManagedClass(t *testing.T) {
},
},
},
expected: []reconcile.Request{
{
NamespacedName: types.NamespacedName{Name: "test-gc"},
},
},
expected: true,
},
{
name: "envoyproxy in different namespace as eg",
Expand All @@ -249,7 +244,7 @@ func TestEnqueueManagedClass(t *testing.T) {
Spec: gwapiv1b1.GatewayClassSpec{ControllerName: gcCtrlName},
},
},
expected: []reconcile.Request{},
expected: false,
},
{
name: "multiple gatewayclasses one with accepted status",
Expand Down Expand Up @@ -305,11 +300,7 @@ func TestEnqueueManagedClass(t *testing.T) {
},
},
},
expected: []reconcile.Request{
{
NamespacedName: types.NamespacedName{Name: "test-gc1"},
},
},
expected: true,
},
}

Expand Down Expand Up @@ -339,7 +330,7 @@ func TestEnqueueManagedClass(t *testing.T) {
Build()

// Process the test case gatewayclasses.
results := r.enqueueManagedClass(context.TODO(), tc.ep)
results := r.hasManagedClass(tc.ep)
require.Equal(t, tc.expected, results)
})
}
Expand Down