From f1661166b291fc08d46ee9a3439ba7f7b106cb27 Mon Sep 17 00:00:00 2001 From: Vardan Torosyan Date: Mon, 27 Jun 2022 10:22:49 +0200 Subject: [PATCH] Service accounts: Grafana service accounts are enabled by default (#51402) * Remove feature flag for service accounts * Fix failing tests and remove remaining usage * Fix failing tests and remove remaining usage --- .../src/types/featureToggles.gen.ts | 1 - pkg/api/index.go | 4 --- pkg/services/featuremgmt/registry.go | 5 --- pkg/services/featuremgmt/toggles_gen.go | 4 --- pkg/services/featuremgmt/usage_stats_test.go | 2 -- pkg/services/serviceaccounts/api/api.go | 9 +---- pkg/services/serviceaccounts/api/api_test.go | 3 +- .../serviceaccounts/manager/service.go | 36 +++++-------------- .../serviceaccounts/manager/service_test.go | 20 ++--------- 9 files changed, 13 insertions(+), 71 deletions(-) diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 51ab303baf6..0320e389628 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -18,7 +18,6 @@ export interface FeatureToggles { trimDefaults?: boolean; disableEnvelopeEncryption?: boolean; - serviceAccounts?: boolean; database_metrics?: boolean; dashboardPreviews?: boolean; dashboardPreviewsAdmin?: boolean; diff --git a/pkg/api/index.go b/pkg/api/index.go index eedc353334b..f3018d79c01 100644 --- a/pkg/api/index.go +++ b/pkg/api/index.go @@ -151,9 +151,6 @@ func (hs *HTTPServer) getAppLinks(c *models.ReqContext) ([]*dtos.NavLink, error) } func enableServiceAccount(hs *HTTPServer, c *models.ReqContext) bool { - if !hs.Features.IsEnabled(featuremgmt.FlagServiceAccounts) { - return false - } hasAccess := ac.HasAccess(hs.AccessControl, c) return hasAccess(ac.ReqOrgAdmin, serviceAccountAccessEvaluator) } @@ -298,7 +295,6 @@ func (hs *HTTPServer) getNavTree(c *models.ReqContext, hasEditPerm bool, prefs * Url: hs.Cfg.AppSubURL + "/org/apikeys", }) } - // needs both feature flag and migration to be able to show service accounts if enableServiceAccount(hs, c) { configNodes = append(configNodes, &dtos.NavLink{ Text: "Service accounts", diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 8d62cb44d0a..7c16891a841 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -19,11 +19,6 @@ var ( Description: "Disable envelope encryption (emergency only)", State: FeatureStateStable, }, - { - Name: "serviceAccounts", - Description: "support service accounts", - State: FeatureStateBeta, - }, { Name: "database_metrics", Description: "Add prometheus metrics for database tables", diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index 236c12f1349..b7d14834b5d 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -15,10 +15,6 @@ const ( // Disable envelope encryption (emergency only) FlagDisableEnvelopeEncryption = "disableEnvelopeEncryption" - // FlagServiceAccounts - // support service accounts - FlagServiceAccounts = "serviceAccounts" - // FlagDatabaseMetrics // Add prometheus metrics for database tables FlagDatabaseMetrics = "database_metrics" diff --git a/pkg/services/featuremgmt/usage_stats_test.go b/pkg/services/featuremgmt/usage_stats_test.go index ca044aaece2..be25a8f54ba 100644 --- a/pkg/services/featuremgmt/usage_stats_test.go +++ b/pkg/services/featuremgmt/usage_stats_test.go @@ -11,7 +11,6 @@ func TestFeatureUsageStats(t *testing.T) { featureManagerWithAllFeatures := WithFeatures( "trimDefaults", "httpclientprovider_azure_auth", - "serviceAccounts", "database_metrics", "dashboardPreviews", "live-config", @@ -23,7 +22,6 @@ func TestFeatureUsageStats(t *testing.T) { require.Equal(t, map[string]interface{}{ "stats.features.trim_defaults.count": 1, "stats.features.httpclientprovider_azure_auth.count": 1, - "stats.features.service_accounts.count": 1, "stats.features.database_metrics.count": 1, "stats.features.dashboard_previews.count": 1, "stats.features.live_config.count": 1, diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index dd44dc2f359..ce185eae024 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -12,7 +12,6 @@ import ( "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/database" "github.com/grafana/grafana/pkg/setting" @@ -46,13 +45,7 @@ func NewServiceAccountsAPI( } } -func (api *ServiceAccountsAPI) RegisterAPIEndpoints( - features featuremgmt.FeatureToggles, -) { - if !features.IsEnabled(featuremgmt.FlagServiceAccounts) { - return - } - +func (api *ServiceAccountsAPI) RegisterAPIEndpoints() { auth := accesscontrol.Middleware(api.accesscontrol) api.RouterRegister.Group("/api/serviceaccounts", func(serviceAccountsRoute routing.RouteRegister) { serviceAccountsRoute.Get("/search", auth(middleware.ReqOrgAdmin, diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index cd4a3b42523..60d45ad36e7 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/pkg/services/serviceaccounts/api/api_test.go @@ -17,7 +17,6 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/contexthandler/ctxkey" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/database" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" @@ -226,7 +225,7 @@ func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock, acmock *accesscontrolmock.Mock, sqlStore *sqlstore.SQLStore, saStore serviceaccounts.Store) (*web.Mux, *ServiceAccountsAPI) { a := NewServiceAccountsAPI(setting.NewCfg(), svc, acmock, routerRegister, saStore) - a.RegisterAPIEndpoints(featuremgmt.WithFeatures(featuremgmt.FlagServiceAccounts)) + a.RegisterAPIEndpoints() a.cfg.ApiKeyMaxSecondsToLive = -1 // disable api key expiration diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index 583040c52f8..5049a8c2b28 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -8,7 +8,6 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/usagestats" "github.com/grafana/grafana/pkg/services/accesscontrol" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/services/serviceaccounts/api" "github.com/grafana/grafana/pkg/services/serviceaccounts/database" @@ -21,14 +20,12 @@ var ( ) type ServiceAccountsService struct { - store serviceaccounts.Store - features featuremgmt.FeatureToggles - log log.Logger + store serviceaccounts.Store + log log.Logger } func ProvideServiceAccountsService( cfg *setting.Cfg, - features featuremgmt.FeatureToggles, store *sqlstore.SQLStore, kvStore kvstore.KVStore, ac accesscontrol.AccessControl, @@ -36,45 +33,30 @@ func ProvideServiceAccountsService( usageStats usagestats.Service, ) (*ServiceAccountsService, error) { s := &ServiceAccountsService{ - features: features, - store: database.NewServiceAccountsStore(store, kvStore), - log: log.New("serviceaccounts"), + store: database.NewServiceAccountsStore(store, kvStore), + log: log.New("serviceaccounts"), } - if features.IsEnabled(featuremgmt.FlagServiceAccounts) { - if err := RegisterRoles(ac); err != nil { - s.log.Error("Failed to register roles", "error", err) - } - - usageStats.RegisterMetricsFunc(s.store.GetUsageMetrics) + if err := RegisterRoles(ac); err != nil { + s.log.Error("Failed to register roles", "error", err) } + usageStats.RegisterMetricsFunc(s.store.GetUsageMetrics) + serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, routeRegister, s.store) - serviceaccountsAPI.RegisterAPIEndpoints(features) + serviceaccountsAPI.RegisterAPIEndpoints() return s, nil } func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) { - if !sa.features.IsEnabled(featuremgmt.FlagServiceAccounts) { - sa.log.Debug(ServiceAccountFeatureToggleNotFound) - return nil, nil - } return sa.store.CreateServiceAccount(ctx, orgID, name) } func (sa *ServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error { - if !sa.features.IsEnabled(featuremgmt.FlagServiceAccounts) { - sa.log.Debug(ServiceAccountFeatureToggleNotFound) - return nil - } return sa.store.DeleteServiceAccount(ctx, orgID, serviceAccountID) } func (sa *ServiceAccountsService) RetrieveServiceAccountIdByName(ctx context.Context, orgID int64, name string) (int64, error) { - if !sa.features.IsEnabled(featuremgmt.FlagServiceAccounts) { - sa.log.Debug(ServiceAccountFeatureToggleNotFound) - return 0, nil - } return sa.store.RetrieveServiceAccountIdByName(ctx, orgID, name) } diff --git a/pkg/services/serviceaccounts/manager/service_test.go b/pkg/services/serviceaccounts/manager/service_test.go index f338e1f4b39..7e80399d500 100644 --- a/pkg/services/serviceaccounts/manager/service_test.go +++ b/pkg/services/serviceaccounts/manager/service_test.go @@ -4,33 +4,17 @@ import ( "context" "testing" - "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestProvideServiceAccount_DeleteServiceAccount(t *testing.T) { - t.Run("feature toggle present, should call store function", func(t *testing.T) { + t.Run("should call store function", func(t *testing.T) { storeMock := &tests.ServiceAccountsStoreMock{Calls: tests.Calls{}} - svc := ServiceAccountsService{ - features: featuremgmt.WithFeatures("serviceAccounts", true), - store: storeMock} + svc := ServiceAccountsService{store: storeMock} err := svc.DeleteServiceAccount(context.Background(), 1, 1) require.NoError(t, err) assert.Len(t, storeMock.Calls.DeleteServiceAccount, 1) }) - - t.Run("no feature toggle present, should not call store function", func(t *testing.T) { - svcMock := &tests.ServiceAccountsStoreMock{Calls: tests.Calls{}} - svc := ServiceAccountsService{ - features: featuremgmt.WithFeatures("serviceAccounts", false), - store: svcMock, - log: log.New("serviceaccounts-manager-test"), - } - err := svc.DeleteServiceAccount(context.Background(), 1, 1) - require.NoError(t, err) - assert.Len(t, svcMock.Calls.DeleteServiceAccount, 0) - }) }