From 03070e5b3fab764602c49b6c0e07e369f3e3f8a2 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Thu, 5 Dec 2024 16:57:38 +0100 Subject: [PATCH] remove controller config --- internal/db/controller.go | 48 ----- internal/db/controller_test.go | 60 ------- internal/dbmodel/controller.go | 10 -- internal/dbmodel/controller_test.go | 22 --- internal/dbmodel/sql/postgres/1_15.sql | 5 + internal/dbmodel/version.go | 2 +- internal/jimm/controller.go | 48 ----- internal/jimm/controller_test.go | 166 ------------------ internal/jujuapi/controller.go | 39 +--- internal/jujuapi/controller_test.go | 50 ++---- .../jimmtest/mocks/jimm_controller_mock.go | 17 -- 11 files changed, 18 insertions(+), 449 deletions(-) create mode 100644 internal/dbmodel/sql/postgres/1_15.sql diff --git a/internal/db/controller.go b/internal/db/controller.go index fe4393afe..8d1b27192 100644 --- a/internal/db/controller.go +++ b/internal/db/controller.go @@ -179,51 +179,3 @@ func (d *Database) ForEachControllerModel(ctx context.Context, ctl *dbmodel.Cont } return nil } - -// UpsertControllerConfig upserts the controller config. -func (d *Database) UpsertControllerConfig(ctx context.Context, cfg *dbmodel.ControllerConfig) (err error) { - const op = errors.Op("db.UpsertControllerConfig") - - if err := d.ready(); err != nil { - return errors.E(op, err) - } - - durationObserver := servermon.DurationObserver(servermon.DBQueryDurationHistogram, string(op)) - defer durationObserver() - defer servermon.ErrorCounter(servermon.DBQueryErrorCount, &err, string(op)) - - if cfg.Name == "" { - return errors.E(op, errors.CodeBadRequest, `invalid config name ""`) - } - - db := d.DB.WithContext(ctx) - if err := db.Save(cfg).Error; err != nil { - return errors.E(op) - } - return nil -} - -func (d *Database) GetControllerConfig(ctx context.Context, cfg *dbmodel.ControllerConfig) (err error) { - const op = errors.Op("db.GetControllerConfig") - if err := d.ready(); err != nil { - return errors.E(op, err) - } - - durationObserver := servermon.DurationObserver(servermon.DBQueryDurationHistogram, string(op)) - defer durationObserver() - defer servermon.ErrorCounter(servermon.DBQueryErrorCount, &err, string(op)) - - if cfg.Name == "" { - return errors.E(op, errors.CodeNotFound, `invalid config name ""`) - } - - db := d.DB.WithContext(ctx) - if err := db.Where("name = ?", cfg.Name).First(&cfg).Error; err != nil { - err := dbError(err) - if errors.ErrorCode(err) == errors.CodeNotFound { - return errors.E(op, err, "controller config not found") - } - return errors.E(op, err) - } - return nil -} diff --git a/internal/db/controller_test.go b/internal/db/controller_test.go index 2f4797da5..4437942f7 100644 --- a/internal/db/controller_test.go +++ b/internal/db/controller_test.go @@ -342,63 +342,3 @@ func (s *dbSuite) TestForEachControllerModel(c *qt.C) { "00000002-0000-0000-0000-000000000004", }) } - -func TestUpsertControllerConfigUnconfiguredDatabase(t *testing.T) { - c := qt.New(t) - - var d db.Database - err := d.UpsertControllerConfig(context.Background(), &dbmodel.ControllerConfig{}) - c.Check(err, qt.ErrorMatches, `database not configured`) - c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeServerConfiguration) -} - -func (s *dbSuite) TestControllerConfig(c *qt.C) { - ctx := context.Background() - err := s.Database.Migrate(context.Background(), true) - c.Assert(err, qt.Equals, nil) - - config := dbmodel.ControllerConfig{ - Name: "jimm", - Config: map[string]interface{}{ - "key1": "value1", - "key2": "value2", - }, - } - err = s.Database.UpsertControllerConfig(ctx, &config) - c.Assert(err, qt.IsNil) - - config1 := dbmodel.ControllerConfig{ - Name: "jimm", - } - err = s.Database.GetControllerConfig(ctx, &config1) - c.Assert(err, qt.IsNil) - c.Assert(config1, qt.DeepEquals, config) - - config2 := config1 - config2.Config = map[string]interface{}{ - "key1": "value1.1", - "key2": "value2.1", - "key3": "value3", - } - err = s.Database.UpsertControllerConfig(ctx, &config2) - c.Assert(err, qt.IsNil) - - err = s.Database.GetControllerConfig(ctx, &config1) - c.Assert(err, qt.IsNil) - c.Assert(config1, qt.DeepEquals, config2) - - config3 := dbmodel.ControllerConfig{ - Name: "unknown", - } - err = s.Database.GetControllerConfig(ctx, &config3) - c.Assert(err, qt.ErrorMatches, "controller config not found") -} - -func TestGetControllerConfigUnconfiguredDatabase(t *testing.T) { - c := qt.New(t) - - var d db.Database - err := d.GetControllerConfig(context.Background(), &dbmodel.ControllerConfig{}) - c.Check(err, qt.ErrorMatches, `database not configured`) - c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeServerConfiguration) -} diff --git a/internal/dbmodel/controller.go b/internal/dbmodel/controller.go index 49a44d96c..4eab14dba 100644 --- a/internal/dbmodel/controller.go +++ b/internal/dbmodel/controller.go @@ -197,13 +197,3 @@ type CloudRegionControllerPriority struct { // chosen when deploying to a cloud-region. Priority uint } - -// ControllerConfig stores controller configuration. -type ControllerConfig struct { - gorm.Model - - // Name is the name given to this configuration. - Name string `gorm:"not null;uniqueIndex"` - // Config stores the controller configuration - Config Map -} diff --git a/internal/dbmodel/controller_test.go b/internal/dbmodel/controller_test.go index 8e3d92e83..5274e2090 100644 --- a/internal/dbmodel/controller_test.go +++ b/internal/dbmodel/controller_test.go @@ -228,25 +228,3 @@ func TestToJujuRedirectInfoResult(t *testing.T) { CACert: "ca-cert", }) } - -func TestControllerConfig(t *testing.T) { - c := qt.New(t) - db := gormDB(c) - - cfg := dbmodel.ControllerConfig{ - Name: "test-config", - Config: map[string]interface{}{ - "key1": float64(1), - "key2": "test-value-2", - "key3": 42.0, - }, - } - result := db.Create(&cfg) - c.Assert(result.Error, qt.IsNil) - - var cfg2 dbmodel.ControllerConfig - result = db.Where("name = ?", "test-config").First(&cfg2) - c.Assert(result.Error, qt.IsNil) - - c.Check(cfg2, qt.DeepEquals, cfg) -} diff --git a/internal/dbmodel/sql/postgres/1_15.sql b/internal/dbmodel/sql/postgres/1_15.sql new file mode 100644 index 000000000..40b991f9a --- /dev/null +++ b/internal/dbmodel/sql/postgres/1_15.sql @@ -0,0 +1,5 @@ +-- 1_15.sql is a migration to delete controller configs +DROP TABLE controller_configs; + +UPDATE versions SET major=1, minor=15 WHERE component='jimmdb'; + diff --git a/internal/dbmodel/version.go b/internal/dbmodel/version.go index 6ed209c4b..067ca5395 100644 --- a/internal/dbmodel/version.go +++ b/internal/dbmodel/version.go @@ -20,7 +20,7 @@ const ( // Minor is the minor version of the model described in the dbmodel // package. It should be incremented for any change made to the // database model from database model in a released JIMM. - Minor = 14 + Minor = 15 ) type Version struct { diff --git a/internal/jimm/controller.go b/internal/jimm/controller.go index db8673951..a985e664d 100644 --- a/internal/jimm/controller.go +++ b/internal/jimm/controller.go @@ -678,54 +678,6 @@ func (m *modelImporter) handleModelDeltas(ctx context.Context) error { return nil } -// SetControllerConfig changes the value of specified controller configuration -// settings. -func (j *JIMM) SetControllerConfig(ctx context.Context, user *openfga.User, args jujuparams.ControllerConfigSet) error { - const op = errors.Op("jimm.SetControllerConfig") - - if !user.JimmAdmin { - return errors.E(op, errors.CodeUnauthorized, "unauthorized") - } - - err := j.Database.Transaction(func(tx *db.Database) error { - config := dbmodel.ControllerConfig{ - Name: "jimm", - } - err := tx.GetControllerConfig(ctx, &config) - if err != nil && errors.ErrorCode(err) != errors.CodeNotFound { - return err - } - if config.Config == nil { - config.Config = make(map[string]interface{}) - } - for key, value := range args.Config { - config.Config[key] = value - } - return tx.UpsertControllerConfig(ctx, &config) - }) - if err != nil { - return errors.E(op, err) - } - return nil -} - -// GetControllerConfig returns jimm's controller config. -func (j *JIMM) GetControllerConfig(ctx context.Context, u *dbmodel.Identity) (*dbmodel.ControllerConfig, error) { - const op = errors.Op("jimm.GetControllerConfig") - config := dbmodel.ControllerConfig{ - Name: "jimm", - Config: make(map[string]interface{}), - } - err := j.Database.GetControllerConfig(ctx, &config) - if err != nil { - if errors.ErrorCode(err) == errors.CodeNotFound { - return &config, nil - } - return nil, errors.E(op, err) - } - return &config, nil -} - // UpdateMigratedModel asserts that the model has been migrated to the // specified controller and updates the internal model representation. func (j *JIMM) UpdateMigratedModel(ctx context.Context, user *openfga.User, modelTag names.ModelTag, targetControllerName string) error { diff --git a/internal/jimm/controller_test.go b/internal/jimm/controller_test.go index b4bf8acfc..d9dd21df0 100644 --- a/internal/jimm/controller_test.go +++ b/internal/jimm/controller_test.go @@ -1029,172 +1029,6 @@ func TestImportModel(t *testing.T) { } } -const testControllerConfigEnv = ` -users: -- username: alice@canonical.com -- username: eve@canonical.com -- username: fred@canonical.com -` - -func TestSetControllerConfig(t *testing.T) { - c := qt.New(t) - - tests := []struct { - about string - user string - args jujuparams.ControllerConfigSet - jimmAdmin bool - expectedError string - expectedConfig dbmodel.ControllerConfig - }{{ - about: "admin allowed to set config", - user: "alice@canonical.com", - args: jujuparams.ControllerConfigSet{ - Config: map[string]interface{}{ - "key1": "value1", - "key2": "value2", - "key3": "value3", - }, - }, - jimmAdmin: true, - expectedConfig: dbmodel.ControllerConfig{ - Name: "jimm", - Config: map[string]interface{}{ - "key1": "value1", - "key2": "value2", - "key3": "value3", - }, - }, - }, { - about: "add-model user - unauthorized", - user: "eve@canonical.com", - args: jujuparams.ControllerConfigSet{ - Config: map[string]interface{}{ - "key1": "value1", - "key2": "value2", - "key3": "value3", - }, - }, - expectedError: "unauthorized", - }, { - about: "login user - unauthorized", - user: "fred@canonical.com", - args: jujuparams.ControllerConfigSet{ - Config: map[string]interface{}{ - "key1": "value1", - "key2": "value2", - "key3": "value3", - }, - }, - expectedError: "unauthorized", - }} - - for _, test := range tests { - c.Run(test.about, func(c *qt.C) { - j := jimmtest.NewJIMM(c, nil) - - env := jimmtest.ParseEnvironment(c, testControllerConfigEnv) - env.PopulateDB(c, j.Database) - - dbUser := env.User(test.user).DBObject(c, j.Database) - user := openfga.NewUser(&dbUser, nil) - user.JimmAdmin = test.jimmAdmin - - ctx := context.Background() - - err := j.SetControllerConfig(ctx, user, test.args) - if test.expectedError == "" { - c.Assert(err, qt.IsNil) - - cfg := dbmodel.ControllerConfig{ - Name: "jimm", - } - err = j.Database.GetControllerConfig(ctx, &cfg) - c.Assert(err, qt.IsNil) - c.Assert(cfg, jimmtest.DBObjectEquals, test.expectedConfig) - } else { - c.Assert(err, qt.ErrorMatches, test.expectedError) - } - }) - } -} - -func TestGetControllerConfig(t *testing.T) { - c := qt.New(t) - - tests := []struct { - about string - user string - jimmAdmin bool - expectedError string - expectedConfig dbmodel.ControllerConfig - }{{ - about: "admin allowed to set config", - user: "alice@canonical.com", - jimmAdmin: true, - expectedConfig: dbmodel.ControllerConfig{ - Name: "jimm", - Config: map[string]interface{}{ - "key1": "value1", - }, - }, - }, { - about: "add-model user - unauthorized", - user: "eve@canonical.com", - jimmAdmin: false, - expectedConfig: dbmodel.ControllerConfig{ - Name: "jimm", - Config: map[string]interface{}{ - "key1": "value1", - }, - }, - }, { - about: "login user - unauthorized", - user: "fred@canonical.com", - jimmAdmin: false, - expectedConfig: dbmodel.ControllerConfig{ - Name: "jimm", - Config: map[string]interface{}{ - "key1": "value1", - }, - }, - }} - - for _, test := range tests { - c.Run(test.about, func(c *qt.C) { - j := jimmtest.NewJIMM(c, nil) - - env := jimmtest.ParseEnvironment(c, testImportModelEnv) - env.PopulateDB(c, j.Database) - - dbSuperuser := env.User("alice@canonical.com").DBObject(c, j.Database) - superuser := openfga.NewUser(&dbSuperuser, nil) - superuser.JimmAdmin = true - - dbUser := env.User(test.user).DBObject(c, j.Database) - user := openfga.NewUser(&dbUser, nil) - user.JimmAdmin = test.jimmAdmin - - ctx := context.Background() - - err := j.SetControllerConfig(ctx, superuser, jujuparams.ControllerConfigSet{ - Config: map[string]interface{}{ - "key1": "value1", - }, - }) - c.Assert(err, qt.Equals, nil) - - cfg, err := j.GetControllerConfig(ctx, user.Identity) - if test.expectedError == "" { - c.Assert(err, qt.IsNil) - c.Assert(cfg, jimmtest.DBObjectEquals, &test.expectedConfig) - } else { - c.Assert(err, qt.ErrorMatches, test.expectedError) - } - }) - } -} - const testUpdateMigratedModelEnv = ` users: - username: alice@canonical.com diff --git a/internal/jujuapi/controller.go b/internal/jujuapi/controller.go index 2549beaa5..d53ed9ad5 100644 --- a/internal/jujuapi/controller.go +++ b/internal/jujuapi/controller.go @@ -9,8 +9,6 @@ import ( jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" "github.com/juju/version" - "github.com/juju/zaputil/zapctx" - "go.uber.org/zap" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" @@ -57,8 +55,6 @@ type ControllerService interface { ControllerInfo(ctx context.Context, name string) (*dbmodel.Controller, error) EarliestControllerVersion(ctx context.Context) (version.Number, error) ListControllers(ctx context.Context, user *openfga.User) ([]dbmodel.Controller, error) - GetControllerConfig(ctx context.Context, user *dbmodel.Identity) (*dbmodel.ControllerConfig, error) - SetControllerConfig(ctx context.Context, user *openfga.User, args jujuparams.ControllerConfigSet) error RemoveController(ctx context.Context, user *openfga.User, controllerName string, force bool) error SetControllerDeprecated(ctx context.Context, user *openfga.User, controllerName string, deprecated bool) error } @@ -67,13 +63,7 @@ type ControllerService interface { // settings. Only some settings can be changed after bootstrap. // JIMM does not support changing settings via ConfigSet. func (r *controllerRoot) ConfigSet(ctx context.Context, args jujuparams.ControllerConfigSet) error { - const op = errors.Op("jujuapi.ConfigSet") - - err := r.jimm.SetControllerConfig(ctx, r.user, args) - if err != nil { - return errors.E(op, err) - } - return nil + return errors.E(errors.CodeNotSupported) } // MongoVersion allows the introspection of the mongo version per @@ -231,32 +221,7 @@ func (r *controllerRoot) ModelStatus(ctx context.Context, args jujuparams.Entiti // ControllerConfig returns the controller's configuration. func (r *controllerRoot) ControllerConfig(ctx context.Context) (jujuparams.ControllerConfigResult, error) { - const op = errors.Op("jujuapi.ControllerConfig") - - isAdmin := r.user.JimmAdmin - if !isAdmin { - isControllerAdmin, err := openfga.IsAdministrator(ctx, r.user, names.NewControllerTag(r.params.ControllerUUID)) - if err != nil { - zapctx.Error(ctx, "failed to check access rights", zap.Error(err)) - return jujuparams.ControllerConfigResult{}, errors.E(op, errors.CodeUnauthorized, "unauthorized") - } - isAdmin = isControllerAdmin - } - if !isAdmin { - return jujuparams.ControllerConfigResult{ - Config: jujuparams.ControllerConfig{}, - }, nil - } - - cfg, err := r.jimm.GetControllerConfig(ctx, r.user.Identity) - if err != nil { - return jujuparams.ControllerConfigResult{}, errors.E(op, err) - } - result := jujuparams.ControllerConfigResult{ - Config: jujuparams.ControllerConfig(cfg.Config), - } - - return result, nil + return jujuparams.ControllerConfigResult{}, errors.E(errors.CodeNotSupported) } // ModelConfig returns implements the controller facade's ModelConfig diff --git a/internal/jujuapi/controller_test.go b/internal/jujuapi/controller_test.go index ad622fe91..65e6b0a6a 100644 --- a/internal/jujuapi/controller_test.go +++ b/internal/jujuapi/controller_test.go @@ -13,7 +13,6 @@ import ( "github.com/juju/juju/api/base" "github.com/juju/juju/api/client/modelmanager" controllerapi "github.com/juju/juju/api/controller/controller" - "github.com/juju/juju/controller" "github.com/juju/juju/core/life" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -35,35 +34,20 @@ type controllerSuite struct { var _ = gc.Suite(&controllerSuite{}) -func (s *controllerSuite) TestControllerConfig(c *gc.C) { +func (s *controllerSuite) TestControllerConfigGetNotSupported(c *gc.C) { conn := s.open(c, nil, "test") defer conn.Close() client := controllerapi.NewClient(conn) - conf, err := client.ControllerConfig() - c.Assert(err, gc.Equals, nil) - c.Assert(conf, jc.DeepEquals, controller.Config(map[string]interface{}{})) - - adminConn := s.open(c, nil, "alice") - defer adminConn.Close() - err = adminConn.APICall("Controller", 11, "", "ConfigSet", jujuparams.ControllerConfigSet{ - Config: map[string]interface{}{ - "key1": "value1", - "key2": "value2", - "charmstore-url": "https://api.jujucharms.com/charmstore", - "metering-url": "https://api.jujucharms.com/omnibus", - }, - }, nil) - c.Assert(err, jc.ErrorIsNil) + _, err := client.ControllerConfig() + c.Assert(jujuparams.IsCodeNotSupported(err), gc.Equals, true) +} - client = controllerapi.NewClient(adminConn) - conf, err = client.ControllerConfig() - c.Assert(err, gc.Equals, nil) - c.Assert(conf, jc.DeepEquals, controller.Config(map[string]interface{}{ - "key1": "value1", - "key2": "value2", - "charmstore-url": "https://api.jujucharms.com/charmstore", - "metering-url": "https://api.jujucharms.com/omnibus", - })) +func (s *controllerSuite) TestControllerConfigSetNotSupported(c *gc.C) { + conn := s.open(c, nil, "test") + defer conn.Close() + client := controllerapi.NewClient(conn) + err := client.ConfigSet(nil) + c.Assert(jujuparams.IsCodeNotSupported(err), gc.Equals, true) } func (s *controllerSuite) TestModelConfig(c *gc.C) { @@ -145,20 +129,6 @@ func (s *controllerSuite) TestModelStatus(c *gc.C) { doTest(modelmanager.NewClient(conn)) } -func (s *controllerSuite) TestConfigSet(c *gc.C) { - conn := s.open(c, nil, "alice") - defer conn.Close() - - err := conn.APICall("Controller", 11, "", "ConfigSet", jujuparams.ControllerConfigSet{}, nil) - c.Assert(err, jc.ErrorIsNil) - - conn1 := s.open(c, nil, "bob") - defer conn1.Close() - - err = conn1.APICall("Controller", 11, "", "ConfigSet", jujuparams.ControllerConfigSet{}, nil) - c.Assert(err, gc.ErrorMatches, `unauthorized \(unauthorized access\)`) -} - func (s *controllerSuite) TestIdentityProviderURL(c *gc.C) { conn := s.open(c, nil, "bob") defer conn.Close() diff --git a/internal/testutils/jimmtest/mocks/jimm_controller_mock.go b/internal/testutils/jimmtest/mocks/jimm_controller_mock.go index a0f2aead6..501b5599f 100644 --- a/internal/testutils/jimmtest/mocks/jimm_controller_mock.go +++ b/internal/testutils/jimmtest/mocks/jimm_controller_mock.go @@ -4,7 +4,6 @@ package mocks import ( "context" - jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/version" "github.com/canonical/jimm/v3/internal/dbmodel" @@ -16,11 +15,9 @@ import ( type ControllerService struct { AddController_ func(ctx context.Context, u *openfga.User, ctl *dbmodel.Controller) error ControllerInfo_ func(ctx context.Context, name string) (*dbmodel.Controller, error) - GetControllerConfig_ func(ctx context.Context, u *dbmodel.Identity) (*dbmodel.ControllerConfig, error) EarliestControllerVersion_ func(ctx context.Context) (version.Number, error) ListControllers_ func(ctx context.Context, user *openfga.User) ([]dbmodel.Controller, error) RemoveController_ func(ctx context.Context, user *openfga.User, controllerName string, force bool) error - SetControllerConfig_ func(ctx context.Context, u *openfga.User, args jujuparams.ControllerConfigSet) error SetControllerDeprecated_ func(ctx context.Context, user *openfga.User, controllerName string, deprecated bool) error } @@ -45,13 +42,6 @@ func (j *ControllerService) EarliestControllerVersion(ctx context.Context) (vers return j.EarliestControllerVersion_(ctx) } -func (j *ControllerService) GetControllerConfig(ctx context.Context, u *dbmodel.Identity) (*dbmodel.ControllerConfig, error) { - if j.GetControllerConfig_ == nil { - return nil, errors.E(errors.CodeNotImplemented) - } - return j.GetControllerConfig_(ctx, u) -} - func (j *ControllerService) ListControllers(ctx context.Context, user *openfga.User) ([]dbmodel.Controller, error) { if j.ListControllers_ == nil { return nil, errors.E(errors.CodeNotImplemented) @@ -66,13 +56,6 @@ func (j *ControllerService) RemoveController(ctx context.Context, user *openfga. return j.RemoveController_(ctx, user, controllerName, force) } -func (j *ControllerService) SetControllerConfig(ctx context.Context, u *openfga.User, args jujuparams.ControllerConfigSet) error { - if j.SetControllerConfig_ == nil { - return errors.E(errors.CodeNotImplemented) - } - return j.SetControllerConfig_(ctx, u, args) -} - func (j *ControllerService) SetControllerDeprecated(ctx context.Context, user *openfga.User, controllerName string, deprecated bool) error { if j.SetControllerDeprecated_ == nil { return errors.E(errors.CodeNotImplemented)