From deca6a49bd8153d3bf661f99bb7f02380aa4d39b Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Fri, 2 Feb 2024 16:28:39 -0600 Subject: [PATCH] catalog: improve the bound workload identity encoding on services (#20458) The endpoints controller currently encodes the list of unique workload identities referenced by all workload matched by a Service into a special data-bearing status condition on that Service. This allows a downstream controller to avoid an expensive watch on the ServiceEndpoints type just to get this data. The current encoding does not lend itself well to machine parsing, which is what the field is meant for, so this PR simplifies the encoding from: "blah blah: " + strings.Join(ids, ",") + "." to strings.Join(ids, ",") It also provides an exported utility function to easily extract this data. --- internal/catalog/exports.go | 10 +++ .../internal/controllers/endpoints/bound.go | 46 ++++++++++++++ .../controllers/endpoints/bound_test.go | 63 +++++++++++++++++++ .../internal/controllers/endpoints/status.go | 11 ++-- 4 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 internal/catalog/internal/controllers/endpoints/bound.go create mode 100644 internal/catalog/internal/controllers/endpoints/bound_test.go diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go index 5bc889a1f5..4d8f69f670 100644 --- a/internal/catalog/exports.go +++ b/internal/catalog/exports.go @@ -63,6 +63,16 @@ func SimplifyFailoverPolicy(svc *pbcatalog.Service, failover *pbcatalog.Failover return types.SimplifyFailoverPolicy(svc, failover) } +// GetBoundIdentities returns the unique list of workload identity references +// encoded into a data-bearing status condition on a Service resource by the +// endpoints controller. +// +// This allows a controller to skip watching ServiceEndpoints (which is +// expensive) to discover this data. +func GetBoundIdentities(res *pbresource.Resource) []string { + return endpoints.GetBoundIdentities(res) +} + // ValidateLocalServiceRefNoSection ensures the following: // // - ref is non-nil diff --git a/internal/catalog/internal/controllers/endpoints/bound.go b/internal/catalog/internal/controllers/endpoints/bound.go new file mode 100644 index 0000000000..923a657de0 --- /dev/null +++ b/internal/catalog/internal/controllers/endpoints/bound.go @@ -0,0 +1,46 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package endpoints + +import ( + "sort" + "strings" + + "github.com/hashicorp/consul/proto-public/pbresource" +) + +// GetBoundIdentities returns the unique list of workload identity references +// encoded into a data-bearing status condition on a Service resource by the +// endpoints controller. +// +// This allows a controller to skip watching ServiceEndpoints (which is +// expensive) to discover this data. +func GetBoundIdentities(res *pbresource.Resource) []string { + if res.GetStatus() == nil { + return nil + } + + status, ok := res.GetStatus()[ControllerID] + if !ok { + return nil + } + + var encoded string + for _, cond := range status.GetConditions() { + if cond.GetType() == StatusConditionBoundIdentities && cond.GetState() == pbresource.Condition_STATE_TRUE { + encoded = cond.GetMessage() + break + } + } + if encoded == "" { + return nil + } + + identities := strings.Split(encoded, ",") + + // Ensure determinstic sort so we don't get into infinite-reconcile + sort.Strings(identities) + + return identities +} diff --git a/internal/catalog/internal/controllers/endpoints/bound_test.go b/internal/catalog/internal/controllers/endpoints/bound_test.go new file mode 100644 index 0000000000..6fb36594f5 --- /dev/null +++ b/internal/catalog/internal/controllers/endpoints/bound_test.go @@ -0,0 +1,63 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package endpoints_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/internal/catalog/internal/controllers/endpoints" + "github.com/hashicorp/consul/internal/resource" + "github.com/hashicorp/consul/internal/resource/demo" + rtest "github.com/hashicorp/consul/internal/resource/resourcetest" + "github.com/hashicorp/consul/proto-public/pbresource" + pbdemo "github.com/hashicorp/consul/proto/private/pbdemo/v2" +) + +func TestGetBoundIdentities(t *testing.T) { + tenancy := resource.DefaultNamespacedTenancy() + + build := func(conds ...*pbresource.Condition) *pbresource.Resource { + b := rtest.Resource(demo.TypeV2Artist, "artist"). + WithTenancy(tenancy). + WithData(t, &pbdemo.Artist{Name: "very arty"}) + if len(conds) > 0 { + b.WithStatus(endpoints.ControllerID, &pbresource.Status{ + Conditions: conds, + }) + } + return b.Build() + } + + run := endpoints.GetBoundIdentities + + require.Empty(t, run(build(nil))) + require.Empty(t, run(build(&pbresource.Condition{ + Type: endpoints.StatusConditionBoundIdentities, + State: pbresource.Condition_STATE_TRUE, + Message: "", + }))) + require.Equal(t, []string{"foo"}, run(build(&pbresource.Condition{ + Type: endpoints.StatusConditionBoundIdentities, + State: pbresource.Condition_STATE_TRUE, + Message: "foo", + }))) + require.Empty(t, run(build(&pbresource.Condition{ + Type: endpoints.StatusConditionBoundIdentities, + State: pbresource.Condition_STATE_FALSE, + Message: "foo", + }))) + require.Equal(t, []string{"bar", "foo"}, run(build(&pbresource.Condition{ + Type: endpoints.StatusConditionBoundIdentities, + State: pbresource.Condition_STATE_TRUE, + Message: "bar,foo", // proper order + }))) + require.Equal(t, []string{"bar", "foo"}, run(build(&pbresource.Condition{ + Type: endpoints.StatusConditionBoundIdentities, + State: pbresource.Condition_STATE_TRUE, + Message: "foo,bar", // incorrect order gets fixed + }))) + +} diff --git a/internal/catalog/internal/controllers/endpoints/status.go b/internal/catalog/internal/controllers/endpoints/status.go index daf1428b51..90993ced77 100644 --- a/internal/catalog/internal/controllers/endpoints/status.go +++ b/internal/catalog/internal/controllers/endpoints/status.go @@ -4,7 +4,7 @@ package endpoints import ( - "fmt" + "sort" "strings" "github.com/hashicorp/consul/proto-public/pbresource" @@ -24,9 +24,6 @@ const ( StatusReasonWorkloadIdentitiesFound = "WorkloadIdentitiesFound" StatusReasonNoWorkloadIdentitiesFound = "NoWorkloadIdentitiesFound" - - identitiesFoundMessageFormat = "Found workload identities associated with this service: %q." - identitiesNotFoundChangedMessage = "No associated workload identities found." ) var ( @@ -48,15 +45,17 @@ var ( Type: StatusConditionBoundIdentities, State: pbresource.Condition_STATE_FALSE, Reason: StatusReasonNoWorkloadIdentitiesFound, - Message: identitiesNotFoundChangedMessage, + Message: "", } ) func ConditionIdentitiesFound(identities []string) *pbresource.Condition { + sort.Strings(identities) + return &pbresource.Condition{ Type: StatusConditionBoundIdentities, State: pbresource.Condition_STATE_TRUE, Reason: StatusReasonWorkloadIdentitiesFound, - Message: fmt.Sprintf(identitiesFoundMessageFormat, strings.Join(identities, ",")), + Message: strings.Join(identities, ","), } }