Storage: Reject duplicate deprecatedInternalID at admission (#125711)

This commit is contained in:
Ryan McKinley
2026-05-29 22:34:58 +03:00
committed by GitHub
parent bb674a5354
commit b571038316
11 changed files with 294 additions and 65 deletions

View File

@@ -199,6 +199,7 @@ func (m *grafanaMetaAccessor) SetAnnotation(key string, val string) {
if val == "" {
if anno != nil {
delete(anno, key)
anno = nilIfEmpty(anno)
}
} else {
if anno == nil {
@@ -316,7 +317,7 @@ func (m *grafanaMetaAccessor) SetDeprecatedInternalID(id int64) {
if id == 0 {
if labels != nil {
delete(labels, LabelKeyDeprecatedInternalID)
m.obj.SetLabels(labels)
m.obj.SetLabels(nilIfEmpty(labels))
}
return
}
@@ -941,6 +942,15 @@ func (m *grafanaMetaAccessor) SetSecureValues(vals common.InlineSecureValues) (e
return fmt.Errorf("unable to set secure values on (%T)", m.raw)
}
// nilIfEmpty returns nil for an empty map so the field is dropped from the
// serialized object rather than written as an empty {}.
func nilIfEmpty(m map[string]string) map[string]string {
if len(m) == 0 {
return nil
}
return m
}
func getJSONFieldName(f reflect.Value, idx int) string {
field := f.Type().Field(idx)
fname := field.Tag.Get("json")

View File

@@ -298,6 +298,53 @@ func TestMetaAccessor(t *testing.T) {
require.Equal(t, meta.GetDeprecatedInternalID(), int64(1))
})
t.Run("removing the last annotation drops the map (nil, not empty)", func(t *testing.T) {
res := &unstructured.Unstructured{Object: map[string]any{}}
meta, err := utils.MetaAccessor(res)
require.NoError(t, err)
meta.SetAnnotation("grafana.app/a", "1")
meta.SetAnnotation("grafana.app/b", "2")
// Removing one of several annotations leaves the rest untouched
meta.SetAnnotation("grafana.app/a", "")
require.Equal(t, map[string]string{"grafana.app/b": "2"}, res.GetAnnotations())
// Removing the final annotation clears the whole map so it is not
// serialized as an empty {}
meta.SetAnnotation("grafana.app/b", "")
require.Nil(t, res.GetAnnotations())
})
t.Run("clearing the internal ID preserves other labels", func(t *testing.T) {
res := &unstructured.Unstructured{Object: map[string]any{}}
meta, err := utils.MetaAccessor(res)
require.NoError(t, err)
res.SetLabels(map[string]string{
utils.LabelKeyDeprecatedInternalID: "7",
"example.com/keep": "yes",
})
// Only the internal ID label is removed; unrelated labels survive
meta.SetDeprecatedInternalID(0)
require.Equal(t, map[string]string{"example.com/keep": "yes"}, res.GetLabels())
})
t.Run("clearing the only label drops the map (nil, not empty)", func(t *testing.T) {
res := &unstructured.Unstructured{Object: map[string]any{}}
meta, err := utils.MetaAccessor(res)
require.NoError(t, err)
meta.SetDeprecatedInternalID(9)
require.Equal(t, map[string]string{
utils.LabelKeyDeprecatedInternalID: "9",
}, res.GetLabels())
meta.SetDeprecatedInternalID(0)
require.Nil(t, res.GetLabels())
})
t.Run("get and set grafana metadata (unstructured)", func(t *testing.T) {
// Error reading spec+status when missing
res := &unstructured.Unstructured{

View File

@@ -770,9 +770,10 @@ func validateDashboardTags(obj runtime.Object) error {
func (b *DashboardsAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupInfo, opts builder.APIGroupOptions) error {
storageOpts := apistore.StorageOptions{
Scheme: opts.Scheme,
EnableFolderSupport: true,
RequireDeprecatedInternalID: true,
Scheme: opts.Scheme,
Index: b.unified,
DeprecatedInternalID: apistore.DeprecatedID_Required,
EnableFolderSupport: true,
}
if b.isStandalone {

View File

@@ -240,7 +240,10 @@ func (b *DataSourceAPIBuilder) AllowedV0Alpha1Resources() []string {
func (b *DataSourceAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupInfo, opts builder.APIGroupOptions) error {
if opts.StorageOptsRegister != nil {
opts.StorageOptsRegister(b.datasourceResourceInfo.GroupResource(), apistore.StorageOptions{
EnableFolderSupport: false,
Index: nil, // TODO, required to check that they are unique
// Keep them for now, but we should get rid of them when possible
DeprecatedInternalID: apistore.DeprecatedID_Required,
// Setting the schema explicitly will force the apistore to explicitly marshal with a matching Group+version
// This is required because we map the same go type (DataSourceConfig) across multiple api groups

View File

@@ -218,10 +218,11 @@ func (b *FolderAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.API
opts.StorageOptsRegister(foldersv1.FolderResourceInfo.GroupResource(), apistore.StorageOptions{
// Preserve apiVersion/kind from the client on write. Without Scheme, apistore.encode
// uses the global LegacyCodec and converts to a single preferred external version.
Scheme: opts.Scheme,
EnableFolderSupport: true,
RequireDeprecatedInternalID: true,
Permissions: b.setDefaultFolderPermissions,
Scheme: opts.Scheme,
Index: b.searcher,
EnableFolderSupport: true,
DeprecatedInternalID: apistore.DeprecatedID_Required,
Permissions: b.setDefaultFolderPermissions,
})
// v1

View File

@@ -392,12 +392,22 @@ func (b *IdentityAccessManagementAPIBuilder) UpdateAPIGroupInfo(apiGroupInfo *ge
// teams + users must have shorter names because they are often used as part of another name
opts.StorageOptsRegister(iamv0.TeamResourceInfo.GroupResource(), apistore.StorageOptions{
MaximumNameLength: 80,
RequireDeprecatedInternalID: true,
MaximumNameLength: 80,
Index: b.unified,
DeprecatedInternalID: apistore.DeprecatedID_Required,
})
opts.StorageOptsRegister(iamv0.UserResourceInfo.GroupResource(), apistore.StorageOptions{
MaximumNameLength: 80,
RequireDeprecatedInternalID: true,
MaximumNameLength: 80,
Index: b.unified,
DeprecatedInternalID: apistore.DeprecatedID_Required,
})
opts.StorageOptsRegister(iamv0.ServiceAccountResourceInfo.GroupResource(), apistore.StorageOptions{
Index: b.unified,
DeprecatedInternalID: apistore.DeprecatedID_Required,
})
opts.StorageOptsRegister(iamv0.TeamBindingResourceInfo.GroupResource(), apistore.StorageOptions{
Index: b.unified,
DeprecatedInternalID: apistore.DeprecatedID_Optional,
})
// Cap the apiserver name at 253 characters so callers get a clear
// validation error instead of a silent truncation/error at the storage

View File

@@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"io"
"strconv"
"time"
"github.com/google/uuid"
@@ -14,6 +15,7 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/storage"
@@ -27,6 +29,7 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/utils"
secrets "github.com/grafana/grafana/pkg/registry/apis/secret/contracts"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/storage/unified/resourcepb"
)
type objectForStorage struct {
@@ -146,16 +149,24 @@ func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime
return v, err
}
if s.opts.RequireDeprecatedInternalID {
// nolint:staticcheck
id := obj.GetDeprecatedInternalID()
if id < 1 {
// the ID must be smaller than 9007199254740991, otherwise we will lose prescision
// on the frontend, which uses the number type to store ids. The largest safe number in
// javascript is 9007199254740991, compared to 9223372036854775807 as the max int64
// nolint:staticcheck
obj.SetDeprecatedInternalID(s.snowflake.Generate().Int64() & ((1 << 52) - 1))
// Make sure the deprecated internal ID is valid
id := obj.GetDeprecatedInternalID() // nolint:staticcheck
// nolint:staticcheck
switch {
case id > 0:
if s.opts.DeprecatedInternalID == DeprecatedID_None {
return v, apierrors.NewBadRequest("internal ID is not supported")
}
if err := s.ensureSingleDeprecatedInternalID(ctx, id, obj); err != nil {
return v, err
}
case s.opts.DeprecatedInternalID == DeprecatedID_Required:
// the ID must be smaller than 9007199254740991, otherwise we will lose precision
// on the frontend, which uses the number type to store ids. The largest safe number in
// javascript is 9007199254740991, compared to 9223372036854775807 as the max int64
obj.SetDeprecatedInternalID(s.snowflake.Generate().Int64() & ((1 << 52) - 1))
case s.opts.DeprecatedInternalID == DeprecatedID_None:
obj.SetDeprecatedInternalID(0) // remove it
}
obj.SetGenerateName("") // Clear the random name field
@@ -180,6 +191,42 @@ func (s *Storage) prepareObjectForStorage(ctx context.Context, newObject runtime
return v, err
}
// ensureSingleDeprecatedInternalID rejects a write when the requested internal
// ID is already in use. This is best effort, not a guarantee: the check queries
// an eventually-consistent search index and is not atomic with the write, so
// concurrent (or rapid sequential, before the index catches up) writes with the
// same ID can both pass. It catches the common accidental-duplicate case, not
// races.
func (s *Storage) ensureSingleDeprecatedInternalID(ctx context.Context, id int64, obj utils.GrafanaMetaAccessor) error {
if s.opts.Index == nil {
// The storage was not configured to verify uniqueness
return nil
}
rsp, err := s.opts.Index.Search(ctx, &resourcepb.ResourceSearchRequest{
Limit: 1, // we only need to know if any match exists
Options: &resourcepb.ListOptions{
Key: &resourcepb.ResourceKey{
Group: s.gr.Group,
Resource: s.gr.Resource,
Namespace: obj.GetNamespace(),
},
Labels: []*resourcepb.Requirement{{
Key: utils.LabelKeyDeprecatedInternalID,
Operator: string(selection.Equals),
Values: []string{strconv.FormatInt(id, 10)},
}},
},
})
if err != nil {
return err
}
if rsp.Results != nil && len(rsp.Results.Rows) > 0 {
return apierrors.NewConflict(s.gr, obj.GetName(),
fmt.Errorf("deprecatedInternalID=%d is already in use", id))
}
return nil
}
// Called on update
func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runtime.Object, previousObject runtime.Object) (objectForStorage, error) {
v := objectForStorage{}
@@ -224,12 +271,8 @@ func (s *Storage) prepareObjectForUpdate(ctx context.Context, updateObject runti
obj.SetResourceVersion("") // removed from saved JSON because the RV is not yet calculated
obj.SetAnnotation(utils.AnnoKeyGrantPermissions, "") // Grant is ignored for update requests
// for dashboards, a mutation hook will set it if it didn't exist on the previous obj
// avoid setting it back to 0
previousInternalID := previous.GetDeprecatedInternalID() // nolint:staticcheck
if previousInternalID != 0 {
obj.SetDeprecatedInternalID(previousInternalID) // nolint:staticcheck
}
// Make sure the deprecated internalID does not change
obj.SetDeprecatedInternalID(previous.GetDeprecatedInternalID()) // nolint:staticcheck
err = prepareSecureValues(ctx, s.opts.SecureValues, obj, previous, &v)
if err != nil {

View File

@@ -11,6 +11,7 @@ import (
"github.com/bwmarrin/snowflake"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
"k8s.io/apimachinery/pkg/api/apitesting"
apierrors "k8s.io/apimachinery/pkg/api/errors"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -26,6 +27,7 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/apimachinery/utils"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/storage/unified/resourcepb"
)
var rtscheme = runtime.NewScheme()
@@ -201,6 +203,37 @@ func TestPrepareObjectForStorage(t *testing.T) {
require.Equal(t, int64(2), meta2.GetGeneration())
})
t.Run("Update can not change the deprecated internal ID", func(t *testing.T) {
// The previously stored object owns internal ID 50
previous := &dashv1.Dashboard{ObjectMeta: v1.ObjectMeta{Name: "test-name"}}
prevMeta, err := utils.MetaAccessor(previous)
require.NoError(t, err)
prevMeta.SetDeprecatedInternalID(50) // nolint:staticcheck
assertStoredID := func(t *testing.T, updated *dashv1.Dashboard) {
t.Helper()
v, err := s.prepareObjectForUpdate(ctx, updated, previous)
require.NoError(t, err)
stored, _, err := s.codec.Decode(v.raw.Bytes(), nil, &dashv1.Dashboard{})
require.NoError(t, err)
storedMeta, err := utils.MetaAccessor(stored)
require.NoError(t, err)
// The update is ignored: the internal ID stays pinned to the previous value
require.Equal(t, int64(50), storedMeta.GetDeprecatedInternalID()) // nolint:staticcheck
}
// Attempting to change it to a different value is ignored
changed := &dashv1.Dashboard{ObjectMeta: v1.ObjectMeta{Name: "test-name"}}
changedMeta, err := utils.MetaAccessor(changed)
require.NoError(t, err)
changedMeta.SetDeprecatedInternalID(999) // nolint:staticcheck
assertStoredID(t, changed)
// Attempting to clear it is also ignored
cleared := &dashv1.Dashboard{ObjectMeta: v1.ObjectMeta{Name: "test-name"}}
assertStoredID(t, cleared)
})
t.Run("Update should skip incrementing generation when content is unchanged", func(t *testing.T) {
dashboard := dashv1.Dashboard{
ObjectMeta: v1.ObjectMeta{
@@ -237,7 +270,9 @@ func TestPrepareObjectForStorage(t *testing.T) {
require.Equal(t, "2025-12-17T01:01:00Z", tmp.GetAnnotation(utils.AnnoKeyUpdatedTimestamp))
})
s.opts.RequireDeprecatedInternalID = true
s.opts.DeprecatedInternalID = DeprecatedID_Required
s.opts.Index = &fakeSearchIndex{inUse: map[string]bool{"100": true}}
t.Run("Should generate internal id", func(t *testing.T) {
dashboard := dashv1.Dashboard{}
dashboard.Name = "test-name"
@@ -270,6 +305,18 @@ func TestPrepareObjectForStorage(t *testing.T) {
require.Equal(t, meta.GetDeprecatedInternalID(), int64(1)) // nolint:staticcheck
})
t.Run("Should fail if deprecated ID if already in use", func(t *testing.T) {
dashboard := dashv1.Dashboard{}
dashboard.Name = "test-name"
obj := dashboard.DeepCopyObject()
meta, err := utils.MetaAccessor(obj)
require.NoError(t, err)
meta.SetDeprecatedInternalID(100) // nolint:staticcheck
_, err = s.prepareObjectForStorage(ctx, obj)
require.True(t, apierrors.IsConflict(err))
})
t.Run("Should remove grant permissions annotation", func(t *testing.T) {
dashboard := dashv1.Dashboard{}
dashboard.Name = "test-name"
@@ -777,3 +824,26 @@ func TestPrepareObjectForStorage_FolderSupportDisabled(t *testing.T) {
require.NoError(t, err)
})
}
// fakeSearchIndex is a minimal resourcepb.ResourceIndexClient for tests. Search
// reports a hit when the request filters on a deprecatedInternalID label whose
// value is listed in inUse, and reports no hits otherwise.
type fakeSearchIndex struct {
resourcepb.ResourceIndexClient
inUse map[string]bool // deprecatedInternalID label values that already exist
}
func (f *fakeSearchIndex) Search(_ context.Context, req *resourcepb.ResourceSearchRequest, _ ...grpc.CallOption) (*resourcepb.ResourceSearchResponse, error) {
rsp := &resourcepb.ResourceSearchResponse{Results: &resourcepb.ResourceTable{}}
for _, label := range req.GetOptions().GetLabels() {
if label.GetKey() != utils.LabelKeyDeprecatedInternalID {
continue
}
for _, v := range label.GetValues() {
if f.inUse[v] {
rsp.Results.Rows = append(rsp.Results.Rows, &resourcepb.ResourceTableRow{})
}
}
}
return rsp, nil
}

View File

@@ -55,18 +55,32 @@ var (
type DefaultPermissionSetter = func(ctx context.Context, key *resourcepb.ResourceKey, id authtypes.AuthInfo, obj utils.GrafanaMetaAccessor) error
type DeprecatedIDState int
const (
// The ID property will always be dropped
DeprecatedID_None DeprecatedIDState = iota
// The ID will always be added
DeprecatedID_Required
// OK if the value is sent, but not added automatically
DeprecatedID_Optional
)
// Optional settings that apply to a single resource
type StorageOptions struct {
Scheme *runtime.Scheme
// Required to force unique constraints
Index resourcepb.ResourceIndexClient
// Allow writing objects with metadata.annotations[grafana.app/folder]
EnableFolderSupport bool
// Some resources should not allow the absolute maximum (254 characters)
MaximumNameLength int
// Add internalID label when missing
RequireDeprecatedInternalID bool
// How should we handle deprecated internal IDs
DeprecatedInternalID DeprecatedIDState
// Process inline secure values
SecureValues secrets.InlineSecureValueSupport
@@ -170,7 +184,7 @@ func NewStorage(
"resource", config.GroupResource.String())
}
if opts.RequireDeprecatedInternalID {
if opts.DeprecatedInternalID == DeprecatedID_Required {
node, err := snowflake.NewNode(rand.Int64N(1024))
if err != nil {
return nil, nil, err
@@ -246,16 +260,33 @@ func (s *Storage) convertToObject(ctx context.Context, data []byte, obj runtime.
func (s *Storage) Create(ctx context.Context, key string, obj runtime.Object, out runtime.Object, ttl uint64) error {
ctx, span := tracer.Start(ctx, "apistore.Storage.Create")
defer span.End()
rkey, err := s.getKey(key)
if err != nil {
return err
}
meta, err := utils.MetaAccessor(obj)
if err != nil {
return err
}
// Make sure we are looking at the correct namespace
if meta.GetNamespace() != rkey.Namespace {
if meta.GetNamespace() == "" {
meta.SetNamespace(rkey.Namespace)
} else {
return apierrors.NewBadRequest("namespace mismatch")
}
}
v, err := s.prepareObjectForStorage(ctx, obj)
if err != nil {
return s.handleManagedResourceRouting(ctx, err, resourcepb.WatchEvent_ADDED, key, obj, out)
}
req := &resourcepb.CreateRequest{
Value: v.raw.Bytes(),
}
req.Key, err = s.getKey(key)
if err != nil {
return err
Key: rkey,
}
v.permissionCreator, err = afterCreatePermissionCreator(ctx, req.Key, v.grantPermissions, obj, s.opts.Permissions)
@@ -279,7 +310,7 @@ func (s *Storage) Create(ctx context.Context, key string, obj runtime.Object, ou
return err
}
meta, err := utils.MetaAccessor(out)
meta, err = utils.MetaAccessor(out)
if err != nil {
return err
}

View File

@@ -226,48 +226,22 @@ func TestIntegrationDashboardServiceValidation(t *testing.T) {
require.NoError(t, err)
})
dashboardWithDuplicatedLegacyAnnotation := "new-uid"
t.Run("When saving a dashboard with an already used legacy ID", func(t *testing.T) {
resp, err := postDashboard(t, grafanaListedAddr, "admin", "admin", map[string]interface{}{
"dashboard": map[string]interface{}{
"id": savedDashInFolder.ID, // nolint:staticcheck
"uid": dashboardWithDuplicatedLegacyAnnotation,
"uid": "new-uid",
"title": "Updated title",
},
"folderUid": savedDashInFolder.FolderUID,
"overwrite": true,
})
require.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
assert.Equal(t, http.StatusConflict, resp.StatusCode)
err = resp.Body.Close()
require.NoError(t, err)
})
t.Run("When updating a dashboard with legacy ID in multiple dashboards", func(t *testing.T) {
resp, err := postDashboard(t, grafanaListedAddr, "admin", "admin", map[string]interface{}{
"dashboard": map[string]interface{}{
"id": savedDashInFolder.ID, // nolint:staticcheck
"uid": savedDashInGeneralFolder.UID,
"title": "Updated title",
},
"folderUid": savedDashInFolder.FolderUID,
"overwrite": true,
})
require.NoError(t, err)
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
err = resp.Body.Close()
require.NoError(t, err)
// Delete the dashboard with duplicated legacy ID annotation
u := fmt.Sprintf("http://admin:admin@%s/api/dashboards/uid/%s", grafanaListedAddr, dashboardWithDuplicatedLegacyAnnotation)
req, err := http.NewRequest("DELETE", u, nil)
require.NoError(t, err)
resp, err = http.DefaultClient.Do(req)
require.NoError(t, err)
err = resp.Body.Close()
require.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
})
t.Run("When updating a dashboard already using that uid", func(t *testing.T) {
resp, err := postDashboard(t, grafanaListedAddr, "admin", "admin", map[string]interface{}{
"dashboard": map[string]interface{}{

View File

@@ -514,6 +514,45 @@ func TestIntegrationLegacySupport(t *testing.T) {
}, &dtos.DashboardFullWithMeta{})
require.Equal(t, 200, rsp.Response.StatusCode)
require.Equal(t, dashboardV0.VERSION, rsp.Result.Meta.APIVersion)
//---------------------------------------------------------
// Reject creating a second dashboard that reuses an existing
// grafana.app/deprecatedInternalID. Without admission enforcement, two
// dashboards could end up sharing the same legacy id (the symptom was
// /api/dashboards/db returning 500 with "unexpected number of dashboards
// for id N. found: 2. desired: 1" on any later overwrite).
//---------------------------------------------------------
t.Run("reject duplicate deprecatedInternalID on create", func(t *testing.T) {
const sharedID = int64(99999)
newDashboardWithID := func(name string) *unstructured.Unstructured {
return &unstructured.Unstructured{
Object: map[string]any{
"apiVersion": "dashboard.grafana.app/v1",
"kind": "Dashboard",
"metadata": map[string]any{
"name": name,
},
"spec": map[string]any{
"id": sharedID,
"title": name,
"schemaVersion": int64(36),
},
},
}
}
first, err := clientV1.Resource.Create(ctx, newDashboardWithID("dup-id-a"), metav1.CreateOptions{})
require.NoError(t, err, "first create with spec.id should succeed")
require.Equal(t, "99999", first.GetLabels()[utils.LabelKeyDeprecatedInternalID], //nolint:staticcheck
"mutation hook should lift spec.id onto the label")
_, err = clientV1.Resource.Create(ctx, newDashboardWithID("dup-id-b"), metav1.CreateOptions{})
require.Error(t, err, "second create with same spec.id must be rejected")
require.True(t, errors.IsConflict(err),
"expected HTTP 409 Conflict, got %T: %v", err, err)
require.Contains(t, err.Error(), "deprecatedInternalID=99999")
})
}
func TestIntegrationListPagination(t *testing.T) {