Extract binding logic into Binder #332

nathancoleman · 2022-08-30T18:37:08Z

Changes proposed in this PR:

This is a portion of the refactor changes from #163. The end result of this PR is that the logic for binding xRoutes to Gateways is abstracted out into Binder. You create a Binder using binder.NewBinder( <Gateway> ) and then use it to bind routes by calling <Binder>.Bind( <Route> ). Similar to other recent changes, K8sGateway.Bind( ) will proxy through to Binder.Bind().

How I've tested this PR:

🤖 tests pass

How I expect reviewers to test this PR:

Checklist:

Tests added
CHANGELOG entry added

Run make changelog-entry for guidance in authoring a changelog entry, and
commit the resulting file, which should have a name matching your PR number.
Entries should use imperative present tense (e.g. Add support for...)

Also restructures some code to favor early returns over nested blocks, adds some comments, etc.

When a resource is removed out-of-band, such as via the Consul API, the in-memory store is unaware of it. Since the store still contains the deleted resource, the background sync will always early-out and thus we lose the self-healing benefit that the background sync provides.

andrewstucki · 2022-09-01T19:51:26Z

internal/store/memory/store.go

@@ -222,7 +198,7 @@ func (s *Store) UpsertRoute(ctx context.Context, route store.Route, updateCondit

 	// bind to gateways
 	for _, gateway := range s.gateways {
-		gateway.TryBind(ctx, route)
+		gateway.Bind(ctx, route)
 	}

 	// sync the gateways to consul and route statuses to k8s


Just for posterity sake, the optimization of only syncing based off of ingress gateway changes comes from trying to ensure the Sync call below this doesn't hammer Consul every time any resource is reconciled. In the last part of the refactor we do some smarter dirty checking to make sure we're only updating gateways whose state values have actually changed from their last stored state, so even if we temporarily un-gate the syncs an over-sync to Consul right now, it should be fixed soon enough.

@nathancoleman and I talked about that offline.

nathancoleman

Personal review

nathancoleman · 2022-09-01T19:46:00Z

internal/adapters/consul/sync.go

 )

 type syncState struct {
+	ingress   *api.IngressGatewayConfigEntry


The changes in this file are mostly around storing the ingress in our in-memory state store

nathancoleman · 2022-09-01T19:46:43Z

internal/adapters/consul/sync.go

 	}

-	return nil
+	return true, nil


Return whether the sync actually completed or not, matches the changes to the interface in core/interfaces.go

nathancoleman · 2022-09-01T19:49:14Z

internal/envoy/handler.go

@@ -85,7 +86,12 @@ func (r *RequestHandler) OnStreamRequest(streamID int64, req *discovery.Discover
 	resources := req.GetResourceNames()

 	// check to make sure we're actually authorized to do this
-	allowed, err := r.registry.CanFetchSecrets(ctx, GatewayFromContext(ctx), resources)
+	gateway, err := r.store.GetGateway(ctx, GatewayFromContext(ctx))


This aligns with our store transitioning to just handling CRUD operations and nothing more

nathancoleman · 2022-09-01T20:03:45Z

internal/k8s/reconciler/gateway.go

 	}

-	return listeners
+	return newBinder(g.client, g.Gateway, g.GatewayState).Bind(ctx, k8sRoute)


Here's where we consume the new Binder to bind the route to the gateway

nathancoleman · 2022-09-01T20:05:42Z

internal/k8s/reconciler/route.go

@@ -197,32 +190,16 @@ func (r *K8sRoute) Validate(ctx context.Context) error {
 	return r.validator.Validate(ctx, r.RouteState, r.Route)
 }

-func (r *K8sRoute) OnBindFailed(err error, gateway store.Gateway) {


These moved onto the RouteState at internal/k8s/reconciler/state/route.go

nathancoleman · 2022-09-01T20:06:43Z

internal/store/memory/store.go

@@ -58,22 +57,6 @@ func (s *Store) GatewayExists(ctx context.Context, id core.GatewayID) (bool, err
 	return found, nil
 }

-func (s *Store) CanFetchSecrets(ctx context.Context, id core.GatewayID, secrets []string) (bool, error) {


This moved onto the store.Gateway interface to limit the amount of logic the store has that isn't related to storing things

nathancoleman · 2022-09-01T20:07:12Z

internal/store/memory/store.go

-	if err := syncGroup.Wait().ErrorOrNil(); err != nil {
-		return err
-	}
-	return nil
+
+	return syncGroup.Wait().ErrorOrNil()


Just an opportunity for simplification that I noticed

nathancoleman · 2022-09-01T20:07:53Z

internal/store/memory/store.go

-				// Force each gateway to sync its state even though listeners
-				// on the gateway may not be marked as needing a sync right now
-				for _, gateway := range s.gateways {
-					gateway.needsSync = true
-				}
-


The sync is not gated the way it was before, so there's no need to "force the sync gate open" as this was doing before. The loop just needs to call sync. As Andrew mentions in a comment below in this file, we'll be adding some dirty checking logic in a followup PR to prevent an endless loop of reconciliation.

andrewstucki

This is awesome that we're starting to get quite a bit closer, enough to actually drop some of the store implementation and old interfaces at this point. I can kind of see the end in sight 😅

Feel free to drop the Listener* stuff at this point in the store package, or you can in a subsequent PR 🤷

andrewstucki · 2022-09-01T20:14:12Z

internal/envoy/handler.go

@@ -85,7 +86,12 @@ func (r *RequestHandler) OnStreamRequest(streamID int64, req *discovery.Discover
 	resources := req.GetResourceNames()

 	// check to make sure we're actually authorized to do this
-	allowed, err := r.registry.CanFetchSecrets(ctx, GatewayFromContext(ctx), resources)
+	gateway, err := r.store.GetGateway(ctx, GatewayFromContext(ctx))


andrewstucki · 2022-09-01T20:15:09Z

internal/k8s/reconciler/binder.go

+// Bind will attempt to bind the provided route to all listeners on the Gateway and
+// remove the route from any listeners that the route should no longer be bound to.
+// The latter is important for scenarios such as the route's parent changing.
+func (b *binder) Bind(ctx context.Context, route *K8sRoute) []string {


I like this simplification 👍

andrewstucki · 2022-09-01T20:17:14Z

internal/store/interfaces.go

-	ShouldBind(route Route) bool
+	Bind(ctx context.Context, route Route) []string
+	Remove(ctx context.Context, id string) error
+	Resolve() core.ResolvedGateway


This is great that we're getting a lot closer

internal/store/interfaces.go

andrewstucki · 2022-09-01T20:18:45Z

internal/store/interfaces.go

 	OnGatewayRemoved(gateway Gateway)
 }

 // Route should be implemented by all route
 // source integrations
 type Route interface {
 	ID() string
-	Resolve(listener Listener) core.ResolvedRoute


andrewstucki · 2022-09-01T20:19:07Z

internal/store/memory/gateway.go

@@ -1,153 +0,0 @@
-package memory


🤣 🤣 🤣

internal/k8s/reconciler/listener.go

Extract binding logic into Binder

e95e861

nathancoleman added pr/no-changelog Skip the CI check that requires a changelog entry pr/run-conformance labels Aug 30, 2022

nathancoleman added 9 commits August 31, 2022 16:28

Remove unnecessary TODO

2db7d90

Restore prefix for route conversion

e6c4d47

Remove K8sRoute funcs that are no longer part of interface

04cce22

Update unit test to use RouteState.Bind

36fa4a8

Remove unused function

adf581b

Remove unused functions on K8sGateway

dfc8b5f

Include existing ingress on sync state for Consul adapter

b802b77

Rework binder.Bind() to handle routes that no longer reference gateway

cdc43cd

Also restructures some code to favor early returns over nested blocks, adds some comments, etc.

nathancoleman marked this pull request as ready for review September 1, 2022 19:41

nathancoleman requested review from andrewstucki, mikemorris and sarahalsmiller September 1, 2022 19:43

Remove unused function

b0e7f65

andrewstucki reviewed Sep 1, 2022

View reviewed changes

Un-export package-local binder, add docstrings

bdf0482

nathancoleman commented Sep 1, 2022

View reviewed changes

andrewstucki approved these changes Sep 1, 2022

View reviewed changes

andrewstucki reviewed Sep 1, 2022

View reviewed changes

internal/k8s/reconciler/listener.go Show resolved Hide resolved

nathancoleman merged commit 8f90401 into main Sep 1, 2022

nathancoleman deleted the binder branch September 1, 2022 21:07

nathancoleman added the refactor label Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract binding logic into Binder #332

Extract binding logic into Binder #332

nathancoleman commented Aug 30, 2022 •

edited

Loading

andrewstucki Sep 1, 2022

nathancoleman left a comment

nathancoleman Sep 1, 2022

nathancoleman Sep 1, 2022

nathancoleman Sep 1, 2022

andrewstucki Sep 1, 2022

nathancoleman Sep 1, 2022

nathancoleman Sep 1, 2022

nathancoleman Sep 1, 2022

nathancoleman Sep 1, 2022

nathancoleman Sep 1, 2022

andrewstucki left a comment •

edited

Loading

andrewstucki Sep 1, 2022

andrewstucki Sep 1, 2022

andrewstucki Sep 1, 2022

andrewstucki Sep 1, 2022

andrewstucki Sep 1, 2022

nathancoleman Sep 1, 2022

Extract binding logic into Binder #332

Extract binding logic into Binder #332

Conversation

nathancoleman commented Aug 30, 2022 • edited Loading

Changes proposed in this PR:

How I've tested this PR:

How I expect reviewers to test this PR:

Checklist:

Choose a reason for hiding this comment

nathancoleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewstucki left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathancoleman commented Aug 30, 2022 •

edited

Loading

andrewstucki left a comment •

edited

Loading