From c0021f686c5e3aaa787ce6f61ef6a94062f84471 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Fri, 2 Feb 2024 14:39:59 -0600 Subject: [PATCH 1/4] catalog: improve the bound workload identity encoding on services --- internal/catalog/exports.go | 10 +++ .../internal/controllers/endpoints/bound.go | 63 ++++++++++++++++ .../controllers/endpoints/bound_test.go | 73 +++++++++++++++++++ .../internal/controllers/endpoints/status.go | 11 ++- 4 files changed, 151 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 5bc889a1f50b..b9e680eebc6d 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(typ *pbresource.Type, res *pbresource.Resource) []*pbresource.Reference { + return endpoints.GetBoundIdentities(typ, 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 000000000000..22c6e516a98c --- /dev/null +++ b/internal/catalog/internal/controllers/endpoints/bound.go @@ -0,0 +1,63 @@ +// 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(typ *pbresource.Type, res *pbresource.Resource) []*pbresource.Reference { + 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 len(encoded) <= 2 { + return nil // it could only ever be [] which is nothing + } + + n := len(encoded) + + if encoded[0] != '[' || encoded[n-1] != ']' { + return nil + } + + trimmed := encoded[1 : n-1] + + identities := strings.Split(trimmed, ",") + + // Ensure determinstic sort so we don't get into infinite-reconcile + sort.Strings(identities) + + var out []*pbresource.Reference + for _, id := range identities { + out = append(out, &pbresource.Reference{ + Type: typ, + Name: id, + Tenancy: res.Id.Tenancy, + }) + } + + return out +} 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 000000000000..630b6dc74f9d --- /dev/null +++ b/internal/catalog/internal/controllers/endpoints/bound_test.go @@ -0,0 +1,73 @@ +// 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 := func(res *pbresource.Resource) []string { + got := endpoints.GetBoundIdentities(demo.TypeV2Album, res) + + var out []string + for _, id := range got { + require.True(t, resource.EqualType(demo.TypeV2Album, id.Type)) + require.True(t, resource.EqualTenancy(tenancy, id.Tenancy)) + out = append(out, id.Name) + } + return out + } + + 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 daf1428b518a..2bb88632d147 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, ",") + "]", } } From 0425833a763e5b812fe98725148bd62e7e18fdc2 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Fri, 2 Feb 2024 15:46:40 -0600 Subject: [PATCH 2/4] remove bracketing --- .../catalog/internal/controllers/endpoints/bound.go | 12 ++---------- .../internal/controllers/endpoints/bound_test.go | 10 +++++----- .../catalog/internal/controllers/endpoints/status.go | 4 ++-- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/internal/catalog/internal/controllers/endpoints/bound.go b/internal/catalog/internal/controllers/endpoints/bound.go index 22c6e516a98c..9a09652915a3 100644 --- a/internal/catalog/internal/controllers/endpoints/bound.go +++ b/internal/catalog/internal/controllers/endpoints/bound.go @@ -33,19 +33,11 @@ func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []*pbres break } } - if len(encoded) <= 2 { - return nil // it could only ever be [] which is nothing - } - - n := len(encoded) - - if encoded[0] != '[' || encoded[n-1] != ']' { + if encoded == "" { return nil } - trimmed := encoded[1 : n-1] - - identities := strings.Split(trimmed, ",") + identities := strings.Split(encoded, ",") // Ensure determinstic sort so we don't get into infinite-reconcile sort.Strings(identities) diff --git a/internal/catalog/internal/controllers/endpoints/bound_test.go b/internal/catalog/internal/controllers/endpoints/bound_test.go index 630b6dc74f9d..406ec0d66565 100644 --- a/internal/catalog/internal/controllers/endpoints/bound_test.go +++ b/internal/catalog/internal/controllers/endpoints/bound_test.go @@ -47,27 +47,27 @@ func TestGetBoundIdentities(t *testing.T) { require.Empty(t, run(build(&pbresource.Condition{ Type: endpoints.StatusConditionBoundIdentities, State: pbresource.Condition_STATE_TRUE, - Message: "[]", + Message: "", }))) require.Equal(t, []string{"foo"}, run(build(&pbresource.Condition{ Type: endpoints.StatusConditionBoundIdentities, State: pbresource.Condition_STATE_TRUE, - Message: "[foo]", + Message: "foo", }))) require.Empty(t, run(build(&pbresource.Condition{ Type: endpoints.StatusConditionBoundIdentities, State: pbresource.Condition_STATE_FALSE, - Message: "[foo]", + 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 + 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 + 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 2bb88632d147..90993ced770a 100644 --- a/internal/catalog/internal/controllers/endpoints/status.go +++ b/internal/catalog/internal/controllers/endpoints/status.go @@ -45,7 +45,7 @@ var ( Type: StatusConditionBoundIdentities, State: pbresource.Condition_STATE_FALSE, Reason: StatusReasonNoWorkloadIdentitiesFound, - Message: "[]", + Message: "", } ) @@ -56,6 +56,6 @@ func ConditionIdentitiesFound(identities []string) *pbresource.Condition { Type: StatusConditionBoundIdentities, State: pbresource.Condition_STATE_TRUE, Reason: StatusReasonWorkloadIdentitiesFound, - Message: "[" + strings.Join(identities, ",") + "]", + Message: strings.Join(identities, ","), } } From 895c0752544a1e44586849513f48a72e44b469a1 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Fri, 2 Feb 2024 15:49:56 -0600 Subject: [PATCH 3/4] return []string and let the caller wrap it in a reference if needed --- internal/catalog/exports.go | 2 +- .../catalog/internal/controllers/endpoints/bound.go | 13 ++----------- .../internal/controllers/endpoints/bound_test.go | 10 +--------- 3 files changed, 4 insertions(+), 21 deletions(-) diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go index b9e680eebc6d..44fb8a518b55 100644 --- a/internal/catalog/exports.go +++ b/internal/catalog/exports.go @@ -69,7 +69,7 @@ func SimplifyFailoverPolicy(svc *pbcatalog.Service, failover *pbcatalog.Failover // // This allows a controller to skip watching ServiceEndpoints (which is // expensive) to discover this data. -func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []*pbresource.Reference { +func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []string { return endpoints.GetBoundIdentities(typ, res) } diff --git a/internal/catalog/internal/controllers/endpoints/bound.go b/internal/catalog/internal/controllers/endpoints/bound.go index 9a09652915a3..3e76ac6b9551 100644 --- a/internal/catalog/internal/controllers/endpoints/bound.go +++ b/internal/catalog/internal/controllers/endpoints/bound.go @@ -16,7 +16,7 @@ import ( // // This allows a controller to skip watching ServiceEndpoints (which is // expensive) to discover this data. -func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []*pbresource.Reference { +func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []string { if res.GetStatus() == nil { return nil } @@ -42,14 +42,5 @@ func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []*pbres // Ensure determinstic sort so we don't get into infinite-reconcile sort.Strings(identities) - var out []*pbresource.Reference - for _, id := range identities { - out = append(out, &pbresource.Reference{ - Type: typ, - Name: id, - Tenancy: res.Id.Tenancy, - }) - } - - return out + return identities } diff --git a/internal/catalog/internal/controllers/endpoints/bound_test.go b/internal/catalog/internal/controllers/endpoints/bound_test.go index 406ec0d66565..4170266a9e34 100644 --- a/internal/catalog/internal/controllers/endpoints/bound_test.go +++ b/internal/catalog/internal/controllers/endpoints/bound_test.go @@ -32,15 +32,7 @@ func TestGetBoundIdentities(t *testing.T) { } run := func(res *pbresource.Resource) []string { - got := endpoints.GetBoundIdentities(demo.TypeV2Album, res) - - var out []string - for _, id := range got { - require.True(t, resource.EqualType(demo.TypeV2Album, id.Type)) - require.True(t, resource.EqualTenancy(tenancy, id.Tenancy)) - out = append(out, id.Name) - } - return out + return endpoints.GetBoundIdentities(demo.TypeV2Album, res) } require.Empty(t, run(build(nil))) From 2fe9dd8b78fa6aaf711e4ddb803aa25f275787f6 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Fri, 2 Feb 2024 16:00:29 -0600 Subject: [PATCH 4/4] remove unused var --- internal/catalog/exports.go | 4 ++-- internal/catalog/internal/controllers/endpoints/bound.go | 2 +- internal/catalog/internal/controllers/endpoints/bound_test.go | 4 +--- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go index 44fb8a518b55..4d8f69f67078 100644 --- a/internal/catalog/exports.go +++ b/internal/catalog/exports.go @@ -69,8 +69,8 @@ func SimplifyFailoverPolicy(svc *pbcatalog.Service, failover *pbcatalog.Failover // // This allows a controller to skip watching ServiceEndpoints (which is // expensive) to discover this data. -func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []string { - return endpoints.GetBoundIdentities(typ, res) +func GetBoundIdentities(res *pbresource.Resource) []string { + return endpoints.GetBoundIdentities(res) } // ValidateLocalServiceRefNoSection ensures the following: diff --git a/internal/catalog/internal/controllers/endpoints/bound.go b/internal/catalog/internal/controllers/endpoints/bound.go index 3e76ac6b9551..923a657de0da 100644 --- a/internal/catalog/internal/controllers/endpoints/bound.go +++ b/internal/catalog/internal/controllers/endpoints/bound.go @@ -16,7 +16,7 @@ import ( // // This allows a controller to skip watching ServiceEndpoints (which is // expensive) to discover this data. -func GetBoundIdentities(typ *pbresource.Type, res *pbresource.Resource) []string { +func GetBoundIdentities(res *pbresource.Resource) []string { if res.GetStatus() == nil { return nil } diff --git a/internal/catalog/internal/controllers/endpoints/bound_test.go b/internal/catalog/internal/controllers/endpoints/bound_test.go index 4170266a9e34..6fb36594f5f9 100644 --- a/internal/catalog/internal/controllers/endpoints/bound_test.go +++ b/internal/catalog/internal/controllers/endpoints/bound_test.go @@ -31,9 +31,7 @@ func TestGetBoundIdentities(t *testing.T) { return b.Build() } - run := func(res *pbresource.Resource) []string { - return endpoints.GetBoundIdentities(demo.TypeV2Album, res) - } + run := endpoints.GetBoundIdentities require.Empty(t, run(build(nil))) require.Empty(t, run(build(&pbresource.Condition{