diff --git a/pkg/services/ngalert/api/api_alertmanager_test.go b/pkg/services/ngalert/api/api_alertmanager_test.go index 45c886cd1e4..d9af081ac64 100644 --- a/pkg/services/ngalert/api/api_alertmanager_test.go +++ b/pkg/services/ngalert/api/api_alertmanager_test.go @@ -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) } diff --git a/pkg/services/ngalert/models/provisioning.go b/pkg/services/ngalert/models/provisioning.go index c3655edfc5b..1a68cd59aa6 100644 --- a/pkg/services/ngalert/models/provisioning.go +++ b/pkg/services/ngalert/models/provisioning.go @@ -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 } diff --git a/pkg/services/ngalert/notifier/alertmanager_config.go b/pkg/services/ngalert/notifier/alertmanager_config.go index f3d873a9c98..7dd70647f2d 100644 --- a/pkg/services/ngalert/notifier/alertmanager_config.go +++ b/pkg/services/ngalert/notifier/alertmanager_config.go @@ -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 } diff --git a/pkg/services/ngalert/provisioning/contactpoints.go b/pkg/services/ngalert/provisioning/contactpoints.go index 83922021f0c..1bd94d74b78 100644 --- a/pkg/services/ngalert/provisioning/contactpoints.go +++ b/pkg/services/ngalert/provisioning/contactpoints.go @@ -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 } diff --git a/pkg/services/ngalert/provisioning/notification_policies.go b/pkg/services/ngalert/provisioning/notification_policies.go index 87582b012f8..f17e6a72e09 100644 --- a/pkg/services/ngalert/provisioning/notification_policies.go +++ b/pkg/services/ngalert/provisioning/notification_policies.go @@ -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 } diff --git a/pkg/services/ngalert/provisioning/persist.go b/pkg/services/ngalert/provisioning/persist.go index 0be6e093dd3..9368d79f0b7 100644 --- a/pkg/services/ngalert/provisioning/persist.go +++ b/pkg/services/ngalert/provisioning/persist.go @@ -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 -} diff --git a/pkg/services/ngalert/provisioning/testing.go b/pkg/services/ngalert/provisioning/testing.go index 4693460c1c8..ea8df1c666b 100644 --- a/pkg/services/ngalert/provisioning/testing.go +++ b/pkg/services/ngalert/provisioning/testing.go @@ -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 diff --git a/pkg/services/ngalert/store/provisioning_store.go b/pkg/services/ngalert/store/provisioning_store.go index 6710b702a84..2e59d68de62 100644 --- a/pkg/services/ngalert/store/provisioning_store.go +++ b/pkg/services/ngalert/store/provisioning_store.go @@ -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 }) diff --git a/pkg/services/ngalert/store/provisioning_store_test.go b/pkg/services/ngalert/store/provisioning_store_test.go index 886ca7f6884..f8c8cac2cc6 100644 --- a/pkg/services/ngalert/store/provisioning_store_test.go +++ b/pkg/services/ngalert/store/provisioning_store_test.go @@ -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) })