Drop ProvenanceOrgAdapter and build into store API instead (#48137)

This commit is contained in:
Alexander Weaver 2022-04-26 10:30:57 -05:00 committed by GitHub
parent 9b95d77be9
commit 078a578803
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 56 additions and 112 deletions

View File

@ -505,8 +505,7 @@ func createRequestCtxInOrg(org int64) *models.ReqContext {
// setRouteProvenance marks an org's routing tree as provisioned.
func setRouteProvenance(t *testing.T, org int64, ps provisioning.ProvisioningStore) {
t.Helper()
adp := provisioning.ProvenanceOrgAdapter{Inner: &apimodels.Route{}, OrgID: org}
err := ps.SetProvenance(context.Background(), adp, ngmodels.ProvenanceAPI)
err := ps.SetProvenance(context.Background(), &apimodels.Route{}, org, ngmodels.ProvenanceAPI)
require.NoError(t, err)
}

View File

@ -14,11 +14,4 @@ const (
type Provisionable interface {
ResourceType() string
ResourceID() string
ResourceOrgID() int64
}
// ProvisionableInOrg represents a resource that can be provisioned, given external org-related information.
type ProvisionableInOrg interface {
ResourceType() string
ResourceID() string
}

View File

@ -8,7 +8,6 @@ import (
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/ngalert/provisioning"
"github.com/grafana/grafana/pkg/services/ngalert/store"
)
@ -129,11 +128,7 @@ func (moa *MultiOrgAlertmanager) ApplyAlertmanagerConfiguration(ctx context.Cont
func (moa *MultiOrgAlertmanager) mergeProvenance(ctx context.Context, config definitions.GettableApiAlertingConfig, org int64) (definitions.GettableApiAlertingConfig, error) {
if config.Route != nil {
adp := provisioning.ProvenanceOrgAdapter{
Inner: config.Route,
OrgID: org,
}
provenance, err := moa.ProvStore.GetProvenance(ctx, adp)
provenance, err := moa.ProvStore.GetProvenance(ctx, config.Route, org)
if err != nil {
return definitions.GettableApiAlertingConfig{}, err
}

View File

@ -177,11 +177,7 @@ func (ecp *ContactPointService) CreateContactPoint(ctx context.Context, orgID in
if err != nil {
return err
}
adapter := ProvenanceOrgAdapter{
Inner: &contactPoint,
OrgID: orgID,
}
err = ecp.provenanceStore.SetProvenance(ctx, adapter, provenance)
err = ecp.provenanceStore.SetProvenance(ctx, &contactPoint, orgID, provenance)
if err != nil {
return err
}
@ -218,10 +214,7 @@ func (ecp *ContactPointService) UpdateContactPoint(ctx context.Context, orgID in
return err
}
// check that provenance is not changed in a invalid way
storedProvenance, err := ecp.provenanceStore.GetProvenance(ctx, ProvenanceOrgAdapter{
Inner: &contactPoint,
OrgID: orgID,
})
storedProvenance, err := ecp.provenanceStore.GetProvenance(ctx, &contactPoint, orgID)
if err != nil {
return err
}
@ -283,11 +276,7 @@ func (ecp *ContactPointService) UpdateContactPoint(ctx context.Context, orgID in
if err != nil {
return err
}
adapter := ProvenanceOrgAdapter{
Inner: &contactPoint,
OrgID: orgID,
}
err = ecp.provenanceStore.SetProvenance(ctx, adapter, provenance)
err = ecp.provenanceStore.SetProvenance(ctx, &contactPoint, orgID, provenance)
if err != nil {
return err
}
@ -330,12 +319,10 @@ func (ecp *ContactPointService) DeleteContactPoint(ctx context.Context, orgID in
return err
}
return ecp.xact.InTransaction(ctx, func(ctx context.Context) error {
err := ecp.provenanceStore.DeleteProvenance(ctx, ProvenanceOrgAdapter{
Inner: &apimodels.EmbeddedContactPoint{
UID: uid,
},
OrgID: orgID,
})
target := &apimodels.EmbeddedContactPoint{
UID: uid,
}
err := ecp.provenanceStore.DeleteProvenance(ctx, target, orgID)
if err != nil {
return err
}

View File

@ -47,11 +47,7 @@ func (nps *NotificationPolicyService) GetPolicyTree(ctx context.Context, orgID i
return definitions.Route{}, fmt.Errorf("no route present in current alertmanager config")
}
adapter := ProvenanceOrgAdapter{
Inner: cfg.AlertmanagerConfig.Route,
OrgID: orgID,
}
provenance, err := nps.provenanceStore.GetProvenance(ctx, adapter)
provenance, err := nps.provenanceStore.GetProvenance(ctx, cfg.AlertmanagerConfig.Route, orgID)
if err != nil {
return definitions.Route{}, err
}
@ -95,11 +91,7 @@ func (nps *NotificationPolicyService) UpdatePolicyTree(ctx context.Context, orgI
if err != nil {
return err
}
adapter := ProvenanceOrgAdapter{
Inner: &tree,
OrgID: orgID,
}
err = nps.provenanceStore.SetProvenance(ctx, adapter, p)
err = nps.provenanceStore.SetProvenance(ctx, &tree, orgID, p)
if err != nil {
return err
}

View File

@ -14,30 +14,13 @@ type AMConfigStore interface {
// ProvisioningStore is a store of provisioning data for arbitrary objects.
type ProvisioningStore interface {
GetProvenance(ctx context.Context, o models.Provisionable) (models.Provenance, error)
GetProvenances(ctx context.Context, orgID int64, resourceType string) (map[string]models.Provenance, error)
SetProvenance(ctx context.Context, o models.Provisionable, p models.Provenance) error
DeleteProvenance(ctx context.Context, o models.Provisionable) error
GetProvenance(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error)
GetProvenances(ctx context.Context, org int64, resourceType string) (map[string]models.Provenance, error)
SetProvenance(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) error
DeleteProvenance(ctx context.Context, o models.Provisionable, org int64) error
}
// TransactionManager represents the ability to issue and close transactions through contexts.
type TransactionManager interface {
InTransaction(ctx context.Context, work func(ctx context.Context) error) error
}
type ProvenanceOrgAdapter struct {
Inner models.ProvisionableInOrg
OrgID int64
}
func (a ProvenanceOrgAdapter) ResourceType() string {
return a.Inner.ResourceType()
}
func (a ProvenanceOrgAdapter) ResourceID() string {
return a.Inner.ResourceID()
}
func (a ProvenanceOrgAdapter) ResourceOrgID() int64 {
return a.OrgID
}

View File

@ -98,8 +98,8 @@ func NewFakeProvisioningStore() *fakeProvisioningStore {
}
}
func (f *fakeProvisioningStore) GetProvenance(ctx context.Context, o models.Provisionable) (models.Provenance, error) {
if val, ok := f.records[o.ResourceOrgID()]; ok {
func (f *fakeProvisioningStore) GetProvenance(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error) {
if val, ok := f.records[org]; ok {
if prov, ok := val[o.ResourceID()+o.ResourceType()]; ok {
return prov, nil
}
@ -119,18 +119,17 @@ func (f *fakeProvisioningStore) GetProvenances(ctx context.Context, orgID int64,
return results, nil
}
func (f *fakeProvisioningStore) SetProvenance(ctx context.Context, o models.Provisionable, p models.Provenance) error {
orgID := o.ResourceOrgID()
if _, ok := f.records[orgID]; !ok {
f.records[orgID] = map[string]models.Provenance{}
func (f *fakeProvisioningStore) SetProvenance(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) error {
if _, ok := f.records[org]; !ok {
f.records[org] = map[string]models.Provenance{}
}
_ = f.DeleteProvenance(ctx, o) // delete old entries first
f.records[orgID][o.ResourceID()+o.ResourceType()] = p
_ = f.DeleteProvenance(ctx, o, org) // delete old entries first
f.records[org][o.ResourceID()+o.ResourceType()] = p
return nil
}
func (f *fakeProvisioningStore) DeleteProvenance(ctx context.Context, o models.Provisionable) error {
if val, ok := f.records[o.ResourceOrgID()]; ok {
func (f *fakeProvisioningStore) DeleteProvenance(ctx context.Context, o models.Provisionable, org int64) error {
if val, ok := f.records[org]; ok {
delete(val, o.ResourceID()+o.ResourceType())
}
return nil

View File

@ -21,16 +21,15 @@ func (pr provenanceRecord) TableName() string {
}
// GetProvenance gets the provenance status for a provisionable object.
func (st DBstore) GetProvenance(ctx context.Context, o models.Provisionable) (models.Provenance, error) {
func (st DBstore) GetProvenance(ctx context.Context, o models.Provisionable, org int64) (models.Provenance, error) {
recordType := o.ResourceType()
recordKey := o.ResourceID()
orgID := o.ResourceOrgID()
provenance := models.ProvenanceNone
err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
filter := "record_key = ? AND record_type = ? AND org_id = ?"
var result models.Provenance
has, err := sess.Table(provenanceRecord{}).Where(filter, recordKey, recordType, orgID).Desc("id").Cols("provenance").Get(&result)
has, err := sess.Table(provenanceRecord{}).Where(filter, recordKey, recordType, org).Desc("id").Cols("provenance").Get(&result)
if err != nil {
return fmt.Errorf("failed to query for existing provenance status: %w", err)
}
@ -46,11 +45,11 @@ func (st DBstore) GetProvenance(ctx context.Context, o models.Provisionable) (mo
}
// GetProvenance gets the provenance status for a provisionable object.
func (st DBstore) GetProvenances(ctx context.Context, orgID int64, resourceType string) (map[string]models.Provenance, error) {
func (st DBstore) GetProvenances(ctx context.Context, org int64, resourceType string) (map[string]models.Provenance, error) {
resultMap := make(map[string]models.Provenance)
err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
filter := "record_type = ? AND org_id = ?"
rawData, err := sess.Table(provenanceRecord{}).Where(filter, resourceType, orgID).Desc("id").Cols("record_key", "provenance").QueryString()
rawData, err := sess.Table(provenanceRecord{}).Where(filter, resourceType, org).Desc("id").Cols("record_key", "provenance").QueryString()
if err != nil {
return fmt.Errorf("failed to query for existing provenance status: %w", err)
}
@ -63,17 +62,16 @@ func (st DBstore) GetProvenances(ctx context.Context, orgID int64, resourceType
}
// SetProvenance changes the provenance status for a provisionable object.
func (st DBstore) SetProvenance(ctx context.Context, o models.Provisionable, p models.Provenance) error {
func (st DBstore) SetProvenance(ctx context.Context, o models.Provisionable, org int64, p models.Provenance) error {
recordType := o.ResourceType()
recordKey := o.ResourceID()
orgID := o.ResourceOrgID()
return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
// TODO: Add a unit-of-work pattern, so updating objects + provenance will happen consistently with rollbacks across stores.
// TODO: Need to make sure that writing a record where our concurrency key fails will also fail the whole transaction. That way, this gets rolled back too. can't just check that 0 updates happened inmemory. Check with jp. If not possible, we need our own concurrency key.
// TODO: Clean up stale provenance records periodically.
filter := "record_key = ? AND record_type = ? AND org_id = ?"
_, err := sess.Table(provenanceRecord{}).Where(filter, recordKey, recordType, orgID).Delete(provenanceRecord{})
_, err := sess.Table(provenanceRecord{}).Where(filter, recordKey, recordType, org).Delete(provenanceRecord{})
if err != nil {
return fmt.Errorf("failed to delete pre-existing provisioning status: %w", err)
@ -83,7 +81,7 @@ func (st DBstore) SetProvenance(ctx context.Context, o models.Provisionable, p m
RecordKey: recordKey,
RecordType: recordType,
Provenance: p,
OrgID: orgID,
OrgID: org,
}
if _, err := sess.Insert(record); err != nil {
@ -95,12 +93,12 @@ func (st DBstore) SetProvenance(ctx context.Context, o models.Provisionable, p m
}
// DeleteProvenance deletes the provenance record from the table
func (st DBstore) DeleteProvenance(ctx context.Context, o models.Provisionable) error {
func (st DBstore) DeleteProvenance(ctx context.Context, o models.Provisionable, org int64) error {
return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
_, err := sess.Delete(provenanceRecord{
RecordKey: o.ResourceID(),
RecordType: o.ResourceType(),
OrgID: o.ResourceOrgID(),
OrgID: org,
})
return err
})

View File

@ -22,7 +22,7 @@ func TestProvisioningStore(t *testing.T) {
UID: "asdf",
}
provenance, err := store.GetProvenance(context.Background(), &rule)
provenance, err := store.GetProvenance(context.Background(), &rule, 1)
require.NoError(t, err)
require.Equal(t, models.ProvenanceNone, provenance)
@ -32,10 +32,10 @@ func TestProvisioningStore(t *testing.T) {
rule := models.AlertRule{
UID: "123",
}
err := store.SetProvenance(context.Background(), &rule, models.ProvenanceFile)
err := store.SetProvenance(context.Background(), &rule, 1, models.ProvenanceFile)
require.NoError(t, err)
p, err := store.GetProvenance(context.Background(), &rule)
p, err := store.GetProvenance(context.Background(), &rule, 1)
require.NoError(t, err)
require.Equal(t, models.ProvenanceFile, p)
@ -43,17 +43,15 @@ func TestProvisioningStore(t *testing.T) {
t.Run("Store does not get provenance of record with different org ID", func(t *testing.T) {
ruleOrg2 := models.AlertRule{
UID: "456",
OrgID: 2,
UID: "456",
}
ruleOrg3 := models.AlertRule{
UID: "456",
OrgID: 3,
UID: "456",
}
err := store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceFile)
err := store.SetProvenance(context.Background(), &ruleOrg2, 2, models.ProvenanceFile)
require.NoError(t, err)
p, err := store.GetProvenance(context.Background(), &ruleOrg3)
p, err := store.GetProvenance(context.Background(), &ruleOrg3, 3)
require.NoError(t, err)
require.Equal(t, models.ProvenanceNone, p)
@ -68,42 +66,42 @@ func TestProvisioningStore(t *testing.T) {
UID: "789",
OrgID: 3,
}
err := store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceFile)
err := store.SetProvenance(context.Background(), &ruleOrg2, 2, models.ProvenanceFile)
require.NoError(t, err)
err = store.SetProvenance(context.Background(), &ruleOrg3, models.ProvenanceFile)
err = store.SetProvenance(context.Background(), &ruleOrg3, 3, models.ProvenanceFile)
require.NoError(t, err)
err = store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceAPI)
err = store.SetProvenance(context.Background(), &ruleOrg2, 2, models.ProvenanceAPI)
require.NoError(t, err)
p, err := store.GetProvenance(context.Background(), &ruleOrg2)
p, err := store.GetProvenance(context.Background(), &ruleOrg2, 2)
require.NoError(t, err)
require.Equal(t, models.ProvenanceAPI, p)
p, err = store.GetProvenance(context.Background(), &ruleOrg3)
p, err = store.GetProvenance(context.Background(), &ruleOrg3, 3)
require.NoError(t, err)
require.Equal(t, models.ProvenanceFile, p)
})
t.Run("Store should return all provenances by type", func(t *testing.T) {
const orgID = 123
ruleOrg1 := models.AlertRule{
rule1 := models.AlertRule{
UID: "789",
OrgID: orgID,
}
ruleOrg2 := models.AlertRule{
rule2 := models.AlertRule{
UID: "790",
OrgID: orgID,
}
err := store.SetProvenance(context.Background(), &ruleOrg1, models.ProvenanceFile)
err := store.SetProvenance(context.Background(), &rule1, orgID, models.ProvenanceFile)
require.NoError(t, err)
err = store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceAPI)
err = store.SetProvenance(context.Background(), &rule2, orgID, models.ProvenanceAPI)
require.NoError(t, err)
p, err := store.GetProvenances(context.Background(), orgID, ruleOrg1.ResourceType())
p, err := store.GetProvenances(context.Background(), orgID, rule1.ResourceType())
require.NoError(t, err)
require.Len(t, p, 2)
require.Equal(t, models.ProvenanceFile, p[ruleOrg1.UID])
require.Equal(t, models.ProvenanceAPI, p[ruleOrg2.UID])
require.Equal(t, models.ProvenanceFile, p[rule1.UID])
require.Equal(t, models.ProvenanceAPI, p[rule2.UID])
})
t.Run("Store should delete provenance correctly", func(t *testing.T) {
@ -112,16 +110,16 @@ func TestProvisioningStore(t *testing.T) {
UID: "7834539",
OrgID: orgID,
}
err := store.SetProvenance(context.Background(), &ruleOrg, models.ProvenanceFile)
err := store.SetProvenance(context.Background(), &ruleOrg, orgID, models.ProvenanceFile)
require.NoError(t, err)
p, err := store.GetProvenance(context.Background(), &ruleOrg)
p, err := store.GetProvenance(context.Background(), &ruleOrg, orgID)
require.NoError(t, err)
require.Equal(t, models.ProvenanceFile, p)
err = store.DeleteProvenance(context.Background(), &ruleOrg)
err = store.DeleteProvenance(context.Background(), &ruleOrg, orgID)
require.NoError(t, err)
p, err = store.GetProvenance(context.Background(), &ruleOrg)
p, err = store.GetProvenance(context.Background(), &ruleOrg, orgID)
require.NoError(t, err)
require.Equal(t, models.ProvenanceNone, p)
})