From 988439e0b8fba04643aac137fd32645094667cbe Mon Sep 17 00:00:00 2001 From: Matheus Macabu Date: Fri, 1 Aug 2025 14:00:01 +0200 Subject: [PATCH] Secrets: Simplify CanReference interface to only pass secure value names (#109030) --- pkg/registry/apis/secret/contracts/inline.go | 4 +- .../secret/service/inline_secure_value.go | 27 ++--- .../service/inline_secure_value_test.go | 102 +++--------------- 3 files changed, 27 insertions(+), 106 deletions(-) diff --git a/pkg/registry/apis/secret/contracts/inline.go b/pkg/registry/apis/secret/contracts/inline.go index 4f677fa5c79..f2f3de71568 100644 --- a/pkg/registry/apis/secret/contracts/inline.go +++ b/pkg/registry/apis/secret/contracts/inline.go @@ -7,8 +7,8 @@ import ( ) type InlineSecureValueSupport interface { - // Check that the request user can reference a secret in the context of a given resource (owner) - CanReference(ctx context.Context, owner common.ObjectReference, values common.InlineSecureValues) error + // Check that the request user can reference secure value names in the context of a given resource (owner) + CanReference(ctx context.Context, owner common.ObjectReference, names ...string) error // CreateInline creates a secret that is owned by the referenced object // returns the name of the created secret or an error diff --git a/pkg/registry/apis/secret/service/inline_secure_value.go b/pkg/registry/apis/secret/service/inline_secure_value.go index c2a6b97e451..9e0cb9fce41 100644 --- a/pkg/registry/apis/secret/service/inline_secure_value.go +++ b/pkg/registry/apis/secret/service/inline_secure_value.go @@ -38,13 +38,14 @@ func ProvideInlineSecureValueService( } } -func (s *inlineSecureValueService) CanReference(ctx context.Context, owner common.ObjectReference, values common.InlineSecureValues) error { +func (s *inlineSecureValueService) CanReference(ctx context.Context, owner common.ObjectReference, names ...string) error { ctx, span := s.tracer.Start(ctx, "InlineSecureValueService.CanReference", trace.WithAttributes( attribute.String("owner.namespace", owner.Namespace), attribute.String("owner.apiGroup", owner.APIGroup), attribute.String("owner.apiVersion", owner.APIVersion), attribute.String("owner.kind", owner.Kind), attribute.String("owner.name", owner.Name), + attribute.StringSlice("secureValueNames", names), )) defer span.End() @@ -61,31 +62,23 @@ func (s *inlineSecureValueService) CanReference(ctx context.Context, owner commo return fmt.Errorf("owner reference must have a valid API group, API version, kind and name") } - if len(values) == 0 { + if len(names) == 0 { return fmt.Errorf("no inline secure values provided") } - for field, value := range values { - if value.Name == "" { - return fmt.Errorf("field %s has an empty secure value name", field) + for _, name := range names { + if name == "" { + return fmt.Errorf("empty secure value name") } - if !value.Create.IsZero() { - return fmt.Errorf("field %s has 'create' set, which is not allowed", field) - } - - if value.Remove { - return fmt.Errorf("field %s has 'remove' set, which is not allowed", field) - } - - owned, err := s.isSecureValueOwnedByResource(ctx, owner, value.Name) + owned, err := s.isSecureValueOwnedByResource(ctx, owner, name) if err != nil { - return fmt.Errorf("field %s had an error checking secure value ownership: %w", field, err) + return err } if !owned { - if err := s.canIdentityReadSecureValue(ctx, xkube.Namespace(owner.Namespace), value.Name); err != nil { - return fmt.Errorf("field %s: identity cannot read secure value %s: %w", field, value.Name, err) + if err := s.canIdentityReadSecureValue(ctx, xkube.Namespace(owner.Namespace), name); err != nil { + return err } } } diff --git a/pkg/registry/apis/secret/service/inline_secure_value_test.go b/pkg/registry/apis/secret/service/inline_secure_value_test.go index fe486df5a9b..d108ce8f047 100644 --- a/pkg/registry/apis/secret/service/inline_secure_value_test.go +++ b/pkg/registry/apis/secret/service/inline_secure_value_test.go @@ -48,18 +48,13 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { require.NoError(t, err) require.NotNil(t, createdSv2) - values := common.InlineSecureValues{ - "fieldA": {Name: sv1}, - "fieldB": {Name: sv2}, - } - ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{ "securevalues:read": {"securevalues:uid:" + sv2}, }) svc := service.ProvideInlineSecureValueService(tracer, tu.SecureValueService, tu.AccessClient) - err = svc.CanReference(ctx, owner, values) + err = svc.CanReference(ctx, owner, sv1, sv2) require.NoError(t, err) }) @@ -67,7 +62,7 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { t.Parallel() svc := service.ProvideInlineSecureValueService(tracer, nil, nil) - err := svc.CanReference(t.Context(), common.ObjectReference{}, common.InlineSecureValues{}) + err := svc.CanReference(t.Context(), common.ObjectReference{}) require.Error(t, err) }) @@ -79,7 +74,7 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { reqNs := "org-2345" ctx := testutils.CreateUserAuthContext(t.Context(), reqNs, map[string][]string{}) - err := svc.CanReference(ctx, owner, common.InlineSecureValues{}) + err := svc.CanReference(ctx, owner) require.Error(t, err) }) @@ -90,7 +85,7 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - err := svc.CanReference(ctx, common.ObjectReference{}, common.InlineSecureValues{}) + err := svc.CanReference(ctx, common.ObjectReference{}) require.Error(t, err) }) @@ -105,23 +100,23 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - err := svc.CanReference(ctx, owner, common.InlineSecureValues{}) + err := svc.CanReference(ctx, owner) require.Error(t, err) owner.APIGroup = "prometheus.datasource.grafana.app" - require.Error(t, svc.CanReference(ctx, owner, common.InlineSecureValues{})) + require.Error(t, svc.CanReference(ctx, owner)) owner.APIGroup = "" owner.APIVersion = "v1alpha1" - require.Error(t, svc.CanReference(ctx, owner, common.InlineSecureValues{})) + require.Error(t, svc.CanReference(ctx, owner)) owner.APIVersion = "" owner.Kind = "DataSourceConfig" - require.Error(t, svc.CanReference(ctx, owner, common.InlineSecureValues{})) + require.Error(t, svc.CanReference(ctx, owner)) owner.Kind = "" owner.Name = "test-datasource" - require.Error(t, svc.CanReference(ctx, owner, common.InlineSecureValues{})) + require.Error(t, svc.CanReference(ctx, owner)) owner.Name = "" }) @@ -132,58 +127,7 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - err := svc.CanReference(ctx, owner, common.InlineSecureValues{}) - require.Error(t, err) - }) - - t.Run("when one of the secure values does not have a `name`, it returns an error", func(t *testing.T) { - t.Parallel() - - svc := service.ProvideInlineSecureValueService(tracer, nil, nil) - - values := common.InlineSecureValues{ - "fieldA": {Name: ""}, - } - - ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - - err := svc.CanReference(ctx, owner, values) - require.Error(t, err) - }) - - t.Run("when one of the secure values has `create` field set, it returns an error", func(t *testing.T) { - t.Parallel() - - svc := service.ProvideInlineSecureValueService(tracer, nil, nil) - - values := common.InlineSecureValues{ - "fieldA": { - Name: "test-sv", - Create: common.NewSecretValue("test"), - }, - } - - ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - - err := svc.CanReference(ctx, owner, values) - require.Error(t, err) - }) - - t.Run("when one of the secure values has `remove` field set, it returns an error", func(t *testing.T) { - t.Parallel() - - svc := service.ProvideInlineSecureValueService(tracer, nil, nil) - - values := common.InlineSecureValues{ - "fieldA": { - Name: "test-sv", - Remove: true, - }, - } - - ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - - err := svc.CanReference(ctx, owner, values) + err := svc.CanReference(ctx, owner) require.Error(t, err) }) @@ -193,13 +137,9 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { tu := testutils.Setup(t) svc := service.ProvideInlineSecureValueService(tracer, tu.SecureValueService, nil) - values := common.InlineSecureValues{ - "fieldA": {Name: "non-existent-sv"}, - } - ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) - err := svc.CanReference(ctx, owner, values) + err := svc.CanReference(ctx, owner, "non-existent-sv") require.Error(t, err) }) @@ -225,15 +165,11 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { require.NoError(t, err) require.NotNil(t, createdSv1) - values := common.InlineSecureValues{ - "fieldA": {Name: sv1}, - } - ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{}) svc := service.ProvideInlineSecureValueService(tracer, tu.SecureValueService, nil) - err = svc.CanReference(ctx, owner, values) + err = svc.CanReference(ctx, owner, sv1) require.Error(t, err) }) @@ -249,15 +185,11 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { }) require.NoError(t, err) - values := common.InlineSecureValues{ - "fieldA": {Name: sv1}, - } - ctx := identity.WithServiceIdentityContext(t.Context(), 1234) svc := service.ProvideInlineSecureValueService(tracer, tu.SecureValueService, nil) - err = svc.CanReference(ctx, owner, values) + err = svc.CanReference(ctx, owner, sv1) require.Error(t, err) }) @@ -273,22 +205,18 @@ func TestIntegration_InlineSecureValue_CanReference(t *testing.T) { }) require.NoError(t, err) - values := common.InlineSecureValues{ - "fieldA": {Name: sv1}, - } - svc := service.ProvideInlineSecureValueService(tracer, tu.SecureValueService, tu.AccessClient) ctx := testutils.CreateUserAuthContext(t.Context(), defaultNs, map[string][]string{ "securevalues:read": {"securevalues:uid:another-sv"}, // can read, but another resource! }) - err = svc.CanReference(ctx, owner, values) + err = svc.CanReference(ctx, owner, sv1) require.Error(t, err) ctx = testutils.CreateUserAuthContext(t.Context(), defaultNs, nil) - err = svc.CanReference(ctx, owner, values) + err = svc.CanReference(ctx, owner, sv1) require.Error(t, err) }) }