From 891d5f0625a8b6b1b5d2c3c3683c1c24ffa125d9 Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Mon, 20 Oct 2025 01:29:03 -0600 Subject: [PATCH] Unistore: Propagate DeprecatedLegacyID on upsert (#112619) Unistore: Fix label propogation on upsert --- pkg/registry/apis/dashboard/legacy/storage.go | 5 ++ .../apis/dashboard/legacy/storage_test.go | 5 ++ pkg/storage/legacysql/dualwrite/dualwriter.go | 46 +++++++++++- .../integration/api_validation_test.go | 72 +++++++++++++++++++ 4 files changed, 125 insertions(+), 3 deletions(-) diff --git a/pkg/registry/apis/dashboard/legacy/storage.go b/pkg/registry/apis/dashboard/legacy/storage.go index d17ae5ef231..d5947ab24f5 100644 --- a/pkg/registry/apis/dashboard/legacy/storage.go +++ b/pkg/registry/apis/dashboard/legacy/storage.go @@ -119,8 +119,13 @@ func (a *dashboardSqlAccess) WriteEvent(ctx context.Context, event resource.Writ } // dashboard version is the RV in legacy storage + // and deprecatedInternalID must be set here (as SaveDashboard does below for non-provisioned dashboards) if after != nil { rv = int64(after.Version) + access := GetLegacyAccess(ctx) + if access != nil { + access.DashboardID = after.ID + } } } else { failOnExisting := event.Type == resourcepb.WatchEvent_ADDED diff --git a/pkg/registry/apis/dashboard/legacy/storage_test.go b/pkg/registry/apis/dashboard/legacy/storage_test.go index 00168d2eee2..27ed87d4d43 100644 --- a/pkg/registry/apis/dashboard/legacy/storage_test.go +++ b/pkg/registry/apis/dashboard/legacy/storage_test.go @@ -83,6 +83,7 @@ func TestWriteProvisioningEvent(t *testing.T) { dashData := &dashboards.Dashboard{ Title: "Test Dashboard", Version: 2, + ID: 3, } dashBytes, err := json.Marshal(dashData) require.NoError(t, err) @@ -125,8 +126,12 @@ func TestWriteProvisioningEvent(t *testing.T) { } ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) + ctx = WithLegacyAccess(ctx) rv, err := access.WriteEvent(ctx, event) require.NoError(t, err) require.Equal(t, int64(2), rv) + a := GetLegacyAccess(ctx) + require.NotNil(t, a) + require.Equal(t, int64(3), a.DashboardID) mockStore.AssertExpectations(t) } diff --git a/pkg/storage/legacysql/dualwrite/dualwriter.go b/pkg/storage/legacysql/dualwrite/dualwriter.go index 7907db3af65..72c05d5ed4d 100644 --- a/pkg/storage/legacysql/dualwrite/dualwriter.go +++ b/pkg/storage/legacysql/dualwrite/dualwriter.go @@ -332,10 +332,10 @@ func (d *dualWriter) Update(ctx context.Context, name string, objInfo rest.Updat unifiedInfo := objInfo unifiedForceCreate := forceAllowCreate if d.readUnified { - legacyInfo = &wrappedUpdateInfo{objInfo} + legacyInfo = &wrappedUpdateInfo{objInfo: objInfo} legacyForceCreate = true } else { - unifiedInfo = &wrappedUpdateInfo{objInfo} + unifiedInfo = &wrappedUpdateInfo{objInfo: objInfo} unifiedForceCreate = true } @@ -345,6 +345,21 @@ func (d *dualWriter) Update(ctx context.Context, name string, objInfo rest.Updat return nil, false, err } + // add any metadata returned from legacy to what is saved in unified storage when forceCreate is used. + // this is especially needed for legacy internal IDs + if createdLegacy { + legacyMeta, err := utils.MetaAccessor(objFromLegacy) + if err != nil { + log.With("object", objFromLegacy).Error("could not get meta accessor for legacy object", "err", err) + return nil, false, err + } + unifiedInfo = &wrappedUpdateInfo{ + objInfo: objInfo, + legacyLabels: legacyMeta.GetLabels(), + legacyAnnotations: legacyMeta.GetAnnotations(), + } + } + if d.readUnified { return d.unified.Update(ctx, name, unifiedInfo, createValidation, updateValidation, unifiedForceCreate, options) } else if d.errorIsOK { @@ -427,7 +442,9 @@ func (d *dualWriter) ConvertToTable(ctx context.Context, object runtime.Object, } type wrappedUpdateInfo struct { - objInfo rest.UpdatedObjectInfo + objInfo rest.UpdatedObjectInfo + legacyLabels map[string]string + legacyAnnotations map[string]string } // Preconditions implements rest.UpdatedObjectInfo. @@ -445,6 +462,29 @@ func (w *wrappedUpdateInfo) UpdatedObject(ctx context.Context, oldObj runtime.Ob if err != nil { return nil, err } + + // add any labels or annotations set by legacy storage + if len(w.legacyLabels) > 0 { + existingLabels := meta.GetLabels() + if existingLabels == nil { + existingLabels = make(map[string]string) + } + for key, value := range w.legacyLabels { + existingLabels[key] = value + } + meta.SetLabels(existingLabels) + } + if len(w.legacyAnnotations) > 0 { + existingAnnotations := meta.GetAnnotations() + if existingAnnotations == nil { + existingAnnotations = make(map[string]string) + } + for key, value := range w.legacyAnnotations { + existingAnnotations[key] = value + } + meta.SetAnnotations(existingAnnotations) + } + meta.SetResourceVersion("") meta.SetUID("") return obj, err diff --git a/pkg/tests/apis/dashboard/integration/api_validation_test.go b/pkg/tests/apis/dashboard/integration/api_validation_test.go index 8e3ae229b68..e71825c6463 100644 --- a/pkg/tests/apis/dashboard/integration/api_validation_test.go +++ b/pkg/tests/apis/dashboard/integration/api_validation_test.go @@ -20,6 +20,7 @@ import ( dashboardV2beta1 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v2beta1" foldersV1 "github.com/grafana/grafana/apps/folder/pkg/apis/folder/v1beta1" "github.com/grafana/grafana/pkg/apiserver/rest" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/serviceaccounts" @@ -30,6 +31,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/services/dashboards" // TODO: Check if we can remove this import "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/util" @@ -779,6 +781,76 @@ func runDashboardValidationTests(t *testing.T, ctx TestContext) { require.NoError(t, err) }) }) + + t.Run("Dashboard upsert propagates legacy id", func(t *testing.T) { + // ensures that the internal ID is propogated from legacy to unified on upsert even in mode 3 + if ctx.DualWriterMode != rest.Mode3 { + t.Skip("Skipping upsert metadata test") + } + + // create via the service, so that upsert is used + specificUID := "upsert-metadata-test" + dashboardService := ctx.Helper.GetEnv().Server.HTTPServer.DashboardService + require.NotNil(t, dashboardService) + provisioningService, ok := dashboardService.(dashboards.DashboardProvisioningService) + require.True(t, ok, "DashboardService should also implement DashboardProvisioningService") + dashboardData := simplejson.NewFromAny(map[string]interface{}{ + "title": "Dashboard for Upsert Metadata Test", + "uid": specificUID, + }) + result, err := provisioningService.SaveProvisionedDashboard(context.Background(), &dashboards.SaveDashboardDTO{ + OrgID: ctx.OrgID, + Dashboard: &dashboards.Dashboard{ + Title: "Dashboard for Upsert Metadata Test", + UID: specificUID, + Data: dashboardData, + }, + }, &dashboards.DashboardProvisioning{ + Name: "test-provisioner", + ExternalID: "/test/path/dashboard.json", + CheckSum: "abc123", + }) + require.NoError(t, err) + require.NotNil(t, result) + + // get the internal id from legacy directly from the db + sqlStore := ctx.Helper.GetEnv().Server.HTTPServer.SQLStore + require.NotNil(t, sqlStore) + + var legacyID int64 + err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + has, innerErr := sess.Table("dashboard"). + Where("uid = ? AND org_id = ?", specificUID, ctx.OrgID). + Cols("id"). + Get(&legacyID) + if innerErr != nil { + return innerErr + } + if !has { + return fmt.Errorf("dashboard not found in legacy storage") + } + return nil + }) + require.NoError(t, err) + require.NotZero(t, legacyID) + + // then compare what unistore returns + dashboardObj, err := adminClient.Resource.Get(context.Background(), specificUID, v1.GetOptions{}) + require.NoError(t, err) + require.NotNil(t, dashboardObj) + labels := dashboardObj.GetLabels() + require.NotNil(t, labels) + deprecatedIDStr, exists := labels[utils.LabelKeyDeprecatedInternalID] + require.True(t, exists) + + legacyIDStr := strconv.FormatInt(legacyID, 10) + require.Equal(t, legacyIDStr, deprecatedIDStr) + + // clean up - with force so the provisioned dashboard deletion check is skipped + zeroPtr := int64(0) + err = adminClient.Resource.Delete(context.Background(), specificUID, v1.DeleteOptions{GracePeriodSeconds: &zeroPtr}) + require.NoError(t, err) + }) } // Run tests for quota validation