-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
mesh: create new routes-controller to reconcile xRoute types into a ComputedRoutes resource #18460
Merged
Merged
Changes from all commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
cd28e66
wip: routes controller etc
rboyer 4c180d7
add more tests and fix a few bugs
rboyer b328d3a
rename troublesome function
rboyer 0e07b43
clarify how null routing works
rboyer a8ad665
remove debugging string
rboyer 9cb6287
test overlap
rboyer d7de729
fix more bugs with conflict
rboyer f4f0230
remove stray comment
rboyer 7d04781
remove old todo
rboyer d218235
remove stray logs
rboyer 3ad8e8a
update messages for new name
rboyer edff762
use loggerFor in both places
rboyer aa58b8b
cleanup naming and export similarly to failover controller
rboyer 3b28e46
remove debuggin
rboyer cd69033
remove todos
rboyer 091cc12
rename protobuf message types
rboyer cec0174
extract shared blob
rboyer 9772251
appease the linter
rboyer e240ec7
remove todo
rboyer 4e0da78
code gen helpers
rboyer 58a692c
fix go.mod
rboyer fba64fd
fix signatures
rboyer 9570170
remove unnecessary meta fields
rboyer b189c94
clarify comment
rboyer 0430838
use default printing format
rboyer 82538b4
early return
rboyer b1f1c91
update commentary
rboyer 5383230
checkpoint
rboyer 2de11ed
adding some generation tests outside of controller logic
rboyer 57e460e
move deduplication into a test and rely upon controller framework to …
rboyer 9dfdde7
stuff
rboyer bb3dca3
more tests
rboyer 95be76d
change indent
rboyer fc61ea7
add more coverage to just the generate.go file
rboyer 547ff46
lint
rboyer 2516db6
clean up this plumbing
rboyer 0f7466d
test initial sort
rboyer c946c95
More sort
rboyer aef3c44
reorder to appease lint
rboyer 14d345f
update signatures to be friendlier to testing
rboyer 6864c73
fixup for scope
rboyer cb7e073
fix enumcover lint
rboyer 4ec9640
use generics instead of generated methods
rboyer 5f8c937
switch to resource.ReplaceType
rboyer 4f81494
use existing interface
rboyer f3cac93
unexport
rboyer 94cb21b
add shared helper
rboyer f64b243
remove extra suspenders
rboyer b4e15ab
this was dead code
rboyer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: BUSL-1.1 | ||
|
||
package controller | ||
|
||
import ( | ||
"github.com/hashicorp/consul/internal/resource" | ||
"github.com/hashicorp/consul/proto-public/pbresource" | ||
) | ||
|
||
// MakeRequests accepts a list of pbresource.ID and pbresource.Reference items, | ||
// and mirrors them into a slice of []controller.Request items where the Type | ||
// of of the items has replaced by 'typ'. | ||
func MakeRequests[V resource.ReferenceOrID]( | ||
typ *pbresource.Type, | ||
refs []V, | ||
) []Request { | ||
if len(refs) == 0 { | ||
return nil | ||
} | ||
|
||
out := make([]Request, 0, len(refs)) | ||
for _, ref := range refs { | ||
out = append(out, Request{ | ||
ID: &pbresource.ID{ | ||
Type: typ, | ||
Tenancy: ref.GetTenancy(), | ||
Name: ref.GetName(), | ||
}, | ||
}) | ||
} | ||
|
||
return out | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: BUSL-1.1 | ||
|
||
package controller | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/hashicorp/consul/internal/resource" | ||
"github.com/hashicorp/consul/proto-public/pbresource" | ||
"github.com/hashicorp/consul/proto/private/prototest" | ||
) | ||
|
||
func TestMakeRequests(t *testing.T) { | ||
redType := &pbresource.Type{ | ||
Group: "colors", | ||
GroupVersion: "vfake", | ||
Kind: "red", | ||
} | ||
blueType := &pbresource.Type{ | ||
Group: "colors", | ||
GroupVersion: "vfake", | ||
Kind: "blue", | ||
} | ||
|
||
casparID := &pbresource.ID{ | ||
Type: redType, | ||
Tenancy: resource.DefaultNamespacedTenancy(), | ||
Name: "caspar", | ||
Uid: "ignored", | ||
} | ||
babypantsID := &pbresource.ID{ | ||
Type: redType, | ||
Tenancy: resource.DefaultNamespacedTenancy(), | ||
Name: "babypants", | ||
Uid: "ignored", | ||
} | ||
zimRef := &pbresource.Reference{ | ||
Type: redType, | ||
Tenancy: resource.DefaultNamespacedTenancy(), | ||
Name: "zim", | ||
Section: "ignored", | ||
} | ||
girRef := &pbresource.Reference{ | ||
Type: redType, | ||
Tenancy: resource.DefaultNamespacedTenancy(), | ||
Name: "gir", | ||
Section: "ignored", | ||
} | ||
|
||
newBlueReq := func(name string) Request { | ||
return Request{ | ||
ID: &pbresource.ID{ | ||
Type: blueType, | ||
Tenancy: resource.DefaultNamespacedTenancy(), | ||
Name: name, | ||
}, | ||
} | ||
} | ||
|
||
require.Nil(t, MakeRequests[*pbresource.ID](blueType, nil)) | ||
require.Nil(t, MakeRequests[*pbresource.Reference](blueType, nil)) | ||
|
||
prototest.AssertElementsMatch(t, []Request{ | ||
newBlueReq("caspar"), newBlueReq("babypants"), | ||
}, MakeRequests[*pbresource.ID](blueType, []*pbresource.ID{ | ||
casparID, babypantsID, | ||
})) | ||
|
||
prototest.AssertElementsMatch(t, []Request{ | ||
newBlueReq("gir"), newBlueReq("zim"), | ||
}, MakeRequests[*pbresource.Reference](blueType, []*pbresource.Reference{ | ||
girRef, zimRef, | ||
})) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
178 changes: 178 additions & 0 deletions
178
internal/mesh/internal/controllers/routes/controller.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,178 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: BUSL-1.1 | ||
|
||
package routes | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/hashicorp/go-hclog" | ||
"google.golang.org/protobuf/proto" | ||
"google.golang.org/protobuf/types/known/anypb" | ||
|
||
"github.com/hashicorp/consul/internal/catalog" | ||
"github.com/hashicorp/consul/internal/controller" | ||
"github.com/hashicorp/consul/internal/mesh/internal/controllers/routes/loader" | ||
"github.com/hashicorp/consul/internal/mesh/internal/controllers/routes/xroutemapper" | ||
"github.com/hashicorp/consul/internal/mesh/internal/types" | ||
"github.com/hashicorp/consul/internal/resource" | ||
pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v1alpha1" | ||
"github.com/hashicorp/consul/proto-public/pbresource" | ||
) | ||
|
||
func Controller() controller.Controller { | ||
mapper := xroutemapper.New() | ||
|
||
r := &routesReconciler{ | ||
mapper: mapper, | ||
} | ||
return controller.ForType(types.ComputedRoutesType). | ||
WithWatch(types.HTTPRouteType, mapper.MapHTTPRoute). | ||
WithWatch(types.GRPCRouteType, mapper.MapGRPCRoute). | ||
WithWatch(types.TCPRouteType, mapper.MapTCPRoute). | ||
WithWatch(types.DestinationPolicyType, mapper.MapDestinationPolicy). | ||
WithWatch(catalog.FailoverPolicyType, mapper.MapFailoverPolicy). | ||
WithWatch(catalog.ServiceType, mapper.MapService). | ||
WithReconciler(r) | ||
} | ||
|
||
type routesReconciler struct { | ||
mapper *xroutemapper.Mapper | ||
} | ||
|
||
func (r *routesReconciler) Reconcile(ctx context.Context, rt controller.Runtime, req controller.Request) error { | ||
// Notably don't inject the resource-id here into the logger, since we have | ||
// to do a fan-out to multiple resources due to xRoutes having multiple | ||
// parent refs. | ||
rt.Logger = rt.Logger.With("controller", StatusKey) | ||
|
||
rt.Logger.Trace("reconciling computed routes") | ||
|
||
loggerFor := func(id *pbresource.ID) hclog.Logger { | ||
return rt.Logger.With("resource-id", id) | ||
} | ||
related, err := loader.LoadResourcesForComputedRoutes(ctx, loggerFor, rt.Client, r.mapper, req.ID) | ||
if err != nil { | ||
rt.Logger.Error("error loading relevant resources", "error", err) | ||
return err | ||
} | ||
|
||
pending := make(PendingStatuses) | ||
|
||
ValidateXRouteReferences(related, pending) | ||
|
||
generatedResults := GenerateComputedRoutes(related, pending) | ||
|
||
if err := UpdatePendingStatuses(ctx, rt, pending); err != nil { | ||
rt.Logger.Error("error updating statuses for affected relevant resources", "error", err) | ||
return err | ||
} | ||
|
||
for _, result := range generatedResults { | ||
computedRoutesID := result.ID | ||
|
||
logger := rt.Logger.With("resource-id", computedRoutesID) | ||
|
||
prev, err := resource.GetDecodedResource[*pbmesh.ComputedRoutes](ctx, rt.Client, computedRoutesID) | ||
if err != nil { | ||
logger.Error("error loading previous computed routes", "error", err) | ||
return err | ||
} | ||
|
||
if err := ensureComputedRoutesIsSynced(ctx, logger, rt.Client, result, prev); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func ensureComputedRoutesIsSynced( | ||
ctx context.Context, | ||
logger hclog.Logger, | ||
client pbresource.ResourceServiceClient, | ||
result *ComputedRoutesResult, | ||
prev *types.DecodedComputedRoutes, | ||
) error { | ||
if result.Data == nil { | ||
return deleteComputedRoutes(ctx, logger, client, prev) | ||
} | ||
|
||
// Upsert the resource if changed. | ||
if prev != nil { | ||
if proto.Equal(prev.Data, result.Data) { | ||
return nil // no change | ||
} | ||
result.ID = prev.Resource.Id | ||
} | ||
|
||
return upsertComputedRoutes(ctx, logger, client, result.ID, result.OwnerID, result.Data) | ||
} | ||
|
||
func upsertComputedRoutes( | ||
ctx context.Context, | ||
logger hclog.Logger, | ||
client pbresource.ResourceServiceClient, | ||
id *pbresource.ID, | ||
ownerID *pbresource.ID, | ||
data *pbmesh.ComputedRoutes, | ||
) error { | ||
mcData, err := anypb.New(data) | ||
if err != nil { | ||
logger.Error("error marshalling new computed routes payload", "error", err) | ||
return err | ||
} | ||
|
||
// Now perform the write. The computed routes resource should be owned | ||
// by the service so that it will automatically be deleted upon service | ||
// deletion. | ||
|
||
_, err = client.Write(ctx, &pbresource.WriteRequest{ | ||
Resource: &pbresource.Resource{ | ||
Id: id, | ||
Owner: ownerID, | ||
Data: mcData, | ||
}, | ||
}) | ||
if err != nil { | ||
logger.Error("error writing computed routes", "error", err) | ||
return err | ||
} | ||
|
||
logger.Trace("updated computed routes resource was successfully written") | ||
|
||
return nil | ||
} | ||
|
||
func deleteComputedRoutes( | ||
ctx context.Context, | ||
logger hclog.Logger, | ||
client pbresource.ResourceServiceClient, | ||
prev *types.DecodedComputedRoutes, | ||
) error { | ||
if prev == nil { | ||
return nil | ||
} | ||
|
||
// The service the computed routes controls no longer participates in the | ||
// mesh at all. | ||
|
||
logger.Trace("removing previous computed routes") | ||
|
||
// This performs a CAS deletion. | ||
_, err := client.Delete(ctx, &pbresource.DeleteRequest{ | ||
Id: prev.Resource.Id, | ||
Version: prev.Resource.Version, | ||
}) | ||
// Potentially we could look for CAS failures by checking if the gRPC | ||
// status code is Aborted. However its an edge case and there could | ||
// possibly be other reasons why the gRPC status code would be aborted | ||
// besides CAS version mismatches. The simplest thing to do is to just | ||
// propagate the error and retry reconciliation later. | ||
if err != nil { | ||
logger.Error("error deleting previous computed routes resource", "error", err) | ||
return err | ||
} | ||
|
||
return nil | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me wonder if the resource service should have an endpoint to
WriteIfDataDiffers
(or maybe just some gRPC metadata to opt-in to the behavior of eliding unnecessary writes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the server does it, it would only benefit the removal of some raft writes. If the client does it then you get the benefit of also avoiding sending the data over the network at all.