diff --git a/internal/db/cloudcredential.go b/internal/db/cloudcredential.go index b72c0766f..14b83822e 100644 --- a/internal/db/cloudcredential.go +++ b/internal/db/cloudcredential.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package db @@ -35,7 +35,7 @@ func (d *Database) SetCloudCredential(ctx context.Context, cred *dbmodel.CloudCr {Name: "owner_identity_name"}, {Name: "name"}, }, - DoUpdates: clause.AssignmentColumns([]string{"auth_type", "label", "attributes_in_vault", "attributes", "valid"}), + DoUpdates: clause.AssignmentColumns([]string{"auth_type", "label", "valid"}), }).Create(&cred).Error; err != nil { return errors.E(op, dbError(err)) } diff --git a/internal/db/cloudcredential_test.go b/internal/db/cloudcredential_test.go index f91d2bdae..774fae361 100644 --- a/internal/db/cloudcredential_test.go +++ b/internal/db/cloudcredential_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package db_test @@ -119,11 +119,6 @@ func (s *dbSuite) TestSetCloudCredentialUpdate(c *qt.C) { cred.Cloud.Regions = nil cred.Label = "test label" - cred.Attributes = dbmodel.StringMap{ - "key1": "value1", - "key2": "value2", - } - cred.AttributesInVault = true cred.Valid = sql.NullBool{ Bool: true, Valid: true, @@ -139,12 +134,7 @@ func (s *dbSuite) TestSetCloudCredentialUpdate(c *qt.C) { err = s.Database.GetCloudCredential(context.Background(), &dbCred) c.Assert(err, qt.Equals, nil) c.Assert(dbCred, jimmtest.DBObjectEquals, cred) - c.Assert(dbCred.Attributes, qt.DeepEquals, dbmodel.StringMap{ - "key1": "value1", - "key2": "value2", - }) c.Assert(dbCred.Label, qt.Equals, "test label") - c.Assert(dbCred.AttributesInVault, qt.IsTrue) c.Assert(dbCred.Valid.Valid, qt.IsTrue) c.Assert(dbCred.Valid.Bool, qt.IsTrue) } diff --git a/internal/dbmodel/cloudcredential.go b/internal/dbmodel/cloudcredential.go index 5c70e7347..44d1fdc8f 100644 --- a/internal/dbmodel/cloudcredential.go +++ b/internal/dbmodel/cloudcredential.go @@ -31,13 +31,6 @@ type CloudCredential struct { // Label is an optional label for the credential. Label string - // AttributesInVault indicates whether the attributes are stored in - // the configured vault key-value store, rather than this database. - AttributesInVault bool - - // Attributes contains the attributes of the credential. - Attributes StringMap - // Valid stores whether the cloud-credential is known to be valid. Valid sql.NullBool diff --git a/internal/dbmodel/cloudcredential_test.go b/internal/dbmodel/cloudcredential_test.go index 560c7884a..1f5d3ec8a 100644 --- a/internal/dbmodel/cloudcredential_test.go +++ b/internal/dbmodel/cloudcredential_test.go @@ -40,10 +40,6 @@ func TestCloudCredential(t *testing.T) { Owner: *i, AuthType: "empty", Label: "test label", - Attributes: dbmodel.StringMap{ - "a": "b", - "c": "d", - }, } result := db.Create(&cred) c.Assert(result.Error, qt.IsNil) diff --git a/internal/dbmodel/controller.go b/internal/dbmodel/controller.go index 4eab14dba..ea9d77451 100644 --- a/internal/dbmodel/controller.go +++ b/internal/dbmodel/controller.go @@ -32,14 +32,6 @@ type Controller struct { // purposes. UUID string `gorm:"not null"` - // AdminIdentityName is the identity name that JIMM uses to connect to the - // controller. - AdminIdentityName string - - // AdminPassword is the password that JIMM uses to connect to the - // controller. - AdminPassword string - // CACertificate is the CA certificate required to access this // controller. This is only set if the controller endpoint's // certificate is not signed by a public CA. @@ -112,7 +104,6 @@ func (c Controller) ToAPIControllerInfo() apiparams.ControllerInfo { var ci apiparams.ControllerInfo ci.Name = c.Name ci.UUID = c.UUID - ci.Username = c.AdminIdentityName ci.PublicAddress = c.PublicAddress for _, hps := range c.Addresses { for _, hp := range hps { @@ -122,7 +113,6 @@ func (c Controller) ToAPIControllerInfo() apiparams.ControllerInfo { ci.CACertificate = c.CACertificate ci.CloudTag = names.NewCloudTag(c.CloudName).String() ci.CloudRegion = c.CloudRegion - ci.Username = c.AdminIdentityName ci.AgentVersion = c.AgentVersion switch { case c.UnavailableSince.Valid: diff --git a/internal/dbmodel/controller_test.go b/internal/dbmodel/controller_test.go index 5274e2090..1faa5f245 100644 --- a/internal/dbmodel/controller_test.go +++ b/internal/dbmodel/controller_test.go @@ -40,13 +40,11 @@ func TestController(t *testing.T) { c.Assert(result.Error, qt.IsNil) ctl := dbmodel.Controller{ - Name: "test-controller", - UUID: "00000000-0000-0000-0000-000000000001", - AdminIdentityName: "admin", - AdminPassword: "pw", - CACertificate: "ca-cert", - PublicAddress: "controller.example.com:443", - CloudName: "test-cloud", + Name: "test-controller", + UUID: "00000000-0000-0000-0000-000000000001", + CACertificate: "ca-cert", + PublicAddress: "controller.example.com:443", + CloudName: "test-cloud", Addresses: dbmodel.HostPorts([][]jujuparams.HostPort{{{ Address: jujuparams.Address{ Value: "1.1.1.1", @@ -166,7 +164,6 @@ func TestToAPIControllerInfo(t *testing.T) { CloudRegion: cl.Regions[0], Priority: dbmodel.CloudRegionControllerPriorityDeployed, }} - ctl.AdminIdentityName = "admin" ctl.AgentVersion = "1.2.3" ci := ctl.ToAPIControllerInfo() diff --git a/internal/dbmodel/sql/postgres/019_remove_secrets_in_db.up.sql b/internal/dbmodel/sql/postgres/019_remove_secrets_in_db.up.sql new file mode 100644 index 000000000..d82d456c9 --- /dev/null +++ b/internal/dbmodel/sql/postgres/019_remove_secrets_in_db.up.sql @@ -0,0 +1,6 @@ +-- deletes secrets that were directly stored in the database. + +ALTER TABLE cloud_credentials DROP COLUMN IF EXISTS attributes_in_vault; +ALTER TABLE cloud_credentials DROP COLUMN IF EXISTS attributes; +ALTER TABLE controllers DROP COLUMN IF EXISTS admin_identity_name; +ALTER TABLE controllers DROP COLUMN IF EXISTS admin_password; diff --git a/internal/jimm/cloudcredential.go b/internal/jimm/cloudcredential.go index 81b6590ce..2fc773090 100644 --- a/internal/jimm/cloudcredential.go +++ b/internal/jimm/cloudcredential.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm @@ -41,7 +41,6 @@ func (j *JIMM) GetCloudCredential(ctx context.Context, user *openfga.User, tag n if err != nil { return nil, errors.E(op, err) } - credential.Attributes = nil return &credential, nil } @@ -177,7 +176,6 @@ func (j *JIMM) UpdateCloudCredential(ctx context.Context, user *openfga.User, ar } credential.AuthType = args.Credential.AuthType - credential.Attributes = args.Credential.Attributes if !args.SkipCheck { err := j.forEachController(ctx, controllers, func(ctl *dbmodel.Controller, api API) error { @@ -204,7 +202,7 @@ func (j *JIMM) UpdateCloudCredential(ctx context.Context, user *openfga.User, ar return result, nil } - if err := j.updateCredential(ctx, &credential); err != nil { + if err := j.updateCredential(ctx, &credential, args.Credential.Attributes); err != nil { return result, errors.E(op, err) } @@ -227,32 +225,18 @@ func (j *JIMM) UpdateCloudCredential(ctx context.Context, user *openfga.User, ar } // updateCredential updates the credential stored in JIMM's database. -func (j *JIMM) updateCredential(ctx context.Context, credential *dbmodel.CloudCredential) error { +func (j *JIMM) updateCredential(ctx context.Context, credential *dbmodel.CloudCredential, attr map[string]string) error { const op = errors.Op("jimm.updateCredential") - if j.CredentialStore == nil { - zapctx.Debug(ctx, "credential store is nil!") - credential.AttributesInVault = false - if err := j.Database.SetCloudCredential(ctx, credential); err != nil { - return errors.E(op, err) - } - return nil - } - - credential1 := *credential - credential1.Attributes = nil - credential1.AttributesInVault = true - if err := j.Database.SetCloudCredential(ctx, &credential1); err != nil { + if err := j.Database.SetCloudCredential(ctx, credential); err != nil { zapctx.Error(ctx, "failed to store credential id", zap.Error(err)) return errors.E(op, err) } - if err := j.CredentialStore.Put(ctx, credential.ResourceTag(), credential.Attributes); err != nil { + if err := j.CredentialStore.Put(ctx, credential.ResourceTag(), attr); err != nil { zapctx.Error(ctx, "failed to store credentials", zap.Error(err)) return errors.E(op, err) } - zapctx.Info(ctx, "credential store location", zap.Bool("vault", credential.AttributesInVault)) - return nil } @@ -263,13 +247,10 @@ func (j *JIMM) updateControllerCloudCredential( ) ([]jujuparams.UpdateCredentialModelResult, error) { const op = errors.Op("jimm.updateControllerCloudCredential") - attr := cred.Attributes - if attr == nil { - var err error - attr, err = j.getCloudCredentialAttributes(ctx, cred) - if err != nil { - return nil, errors.E(op, err) - } + var err error + attr, err := j.getCloudCredentialAttributes(ctx, cred) + if err != nil { + return nil, errors.E(op, err) } models, err := f(ctx, jujuparams.TaggedCredential{ @@ -302,7 +283,6 @@ func (j *JIMM) ForEachUserCloudCredential(ctx context.Context, u *dbmodel.Identi errStop := errors.E("stop") var iterErr error err := j.Database.ForEachCloudCredential(ctx, u.Name, cloud, func(cred *dbmodel.CloudCredential) error { - cred.Attributes = nil iterErr = f(cred) if iterErr != nil { return errStop @@ -365,17 +345,6 @@ func (j *JIMM) GetCloudCredentialAttributes(ctx context.Context, user *openfga.U func (j *JIMM) getCloudCredentialAttributes(ctx context.Context, cred *dbmodel.CloudCredential) (map[string]string, error) { const op = errors.Op("jimm.getCloudCredentialAttributes") - if !cred.AttributesInVault { - if err := j.Database.GetCloudCredential(ctx, cred); err != nil { - return nil, errors.E(op, err) - } - return map[string]string(cred.Attributes), nil - } - - // Attributes have to be loaded from vault. - if j.CredentialStore == nil { - return nil, errors.E(op, errors.CodeServerConfiguration, "vault not configured") - } attr, err := j.CredentialStore.Get(ctx, cred.ResourceTag()) if err != nil { return nil, errors.E(op, err) diff --git a/internal/jimm/cloudcredential_test.go b/internal/jimm/cloudcredential_test.go index 48380b8e4..a0530785b 100644 --- a/internal/jimm/cloudcredential_test.go +++ b/internal/jimm/cloudcredential_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm_test @@ -31,6 +31,10 @@ func TestUpdateCloudCredential(t *testing.T) { arch := "amd64" mem := uint64(8096) cores := uint64(8) + testAttributes := map[string]string{ + "key1": "value1", + "key2": "value2", + } tests := []struct { about string @@ -99,6 +103,9 @@ func TestUpdateCloudCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + err = j.CredentialStore.Put(context.Background(), cred.ResourceTag(), testAttributes) + c.Assert(err, qt.Equals, nil) + cred.Cloud = dbmodel.Cloud{ Name: "test-cloud", Type: "test-provider", @@ -116,20 +123,13 @@ func TestUpdateCloudCredential(t *testing.T) { arg := jimm.UpdateCloudCredentialArgs{ CredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), Credential: jujuparams.CloudCredential{ - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", + Attributes: testAttributes, + AuthType: "test-auth-type", }, } expectedCredential := cred expectedCredential.AuthType = "test-auth-type" - expectedCredential.Attributes = map[string]string{ - "key1": "value1", - "key2": "value2", - } m := dbmodel.Model{ UUID: sql.NullString{ @@ -214,6 +214,9 @@ func TestUpdateCloudCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + err = j.CredentialStore.Put(context.Background(), cred.ResourceTag(), testAttributes) + c.Assert(err, qt.Equals, nil) + cred.Cloud = dbmodel.Cloud{ Name: "test-cloud", Type: "test-provider", @@ -231,11 +234,8 @@ func TestUpdateCloudCredential(t *testing.T) { arg := jimm.UpdateCloudCredentialArgs{ CredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), Credential: jujuparams.CloudCredential{ - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", + Attributes: testAttributes, + AuthType: "test-auth-type", }, } return u, arg, dbmodel.CloudCredential{}, "test error" @@ -305,6 +305,9 @@ func TestUpdateCloudCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + err = j.CredentialStore.Put(context.Background(), cred.ResourceTag(), testAttributes) + c.Assert(err, qt.Equals, nil) + _, err = j.AddModel(context.Background(), user, &jimm.ModelCreateArgs{ Name: "test-model", Owner: names.NewUserTag(u.Name), @@ -317,11 +320,8 @@ func TestUpdateCloudCredential(t *testing.T) { arg := jimm.UpdateCloudCredentialArgs{ CredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), Credential: jujuparams.CloudCredential{ - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", + Attributes: testAttributes, + AuthType: "test-auth-type", }, } return u, arg, dbmodel.CloudCredential{}, "test error" @@ -399,6 +399,9 @@ func TestUpdateCloudCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + err = j.CredentialStore.Put(context.Background(), cred.ResourceTag(), testAttributes) + c.Assert(err, qt.Equals, nil) + mi, err := j.AddModel(context.Background(), alice, &jimm.ModelCreateArgs{ Name: "test-model", Owner: names.NewUserTag("eve@canonical.com"), @@ -411,11 +414,8 @@ func TestUpdateCloudCredential(t *testing.T) { arg := jimm.UpdateCloudCredentialArgs{ CredentialTag: names.NewCloudCredentialTag("test-cloud/eve@canonical.com/test-credential-1"), Credential: jujuparams.CloudCredential{ - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", + Attributes: testAttributes, + AuthType: "test-auth-type", }, } m := dbmodel.Model{ @@ -441,12 +441,8 @@ func TestUpdateCloudCredential(t *testing.T) { Type: cloud.Type, }, OwnerIdentityName: eve.Name, - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", - Models: []dbmodel.Model{m}, + AuthType: "test-auth-type", + Models: []dbmodel.Model{m}, }, "" }, }, { @@ -513,6 +509,9 @@ func TestUpdateCloudCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + err = j.CredentialStore.Put(context.Background(), cred.ResourceTag(), testAttributes) + c.Assert(err, qt.Equals, nil) + cred.Cloud = dbmodel.Cloud{ Name: "test-cloud", Type: "test-provider", @@ -530,21 +529,14 @@ func TestUpdateCloudCredential(t *testing.T) { arg := jimm.UpdateCloudCredentialArgs{ CredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), Credential: jujuparams.CloudCredential{ - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", + Attributes: testAttributes, + AuthType: "test-auth-type", }, SkipCheck: true, } expectedCredential := cred expectedCredential.AuthType = "test-auth-type" - expectedCredential.Attributes = map[string]string{ - "key1": "value1", - "key2": "value2", - } m := dbmodel.Model{ UUID: sql.NullString{ String: mi.UUID, @@ -626,6 +618,9 @@ func TestUpdateCloudCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + err = j.CredentialStore.Put(context.Background(), cred.ResourceTag(), testAttributes) + c.Assert(err, qt.Equals, nil) + cred.Cloud = dbmodel.Cloud{ Name: "test-cloud", Type: "test-provider", @@ -642,11 +637,8 @@ func TestUpdateCloudCredential(t *testing.T) { arg := jimm.UpdateCloudCredentialArgs{ CredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), Credential: jujuparams.CloudCredential{ - Attributes: map[string]string{ - "key1": "value1", - "key2": "value2", - }, - AuthType: "test-auth-type", + Attributes: testAttributes, + AuthType: "test-auth-type", }, SkipUpdate: true, } @@ -779,7 +771,7 @@ func TestUpdateCloudCredential(t *testing.T) { API: api, }, }, - jimmtest.UnsetCredentialStore, // this test relies on credential attributes being stored in postgres + jimmtest.UsePostgresAsCredentialStore, // this test relies on credential attributes being stored in postgres ) u, arg, expectedCredential, expectedError := test.createEnv(c, j) @@ -804,6 +796,10 @@ func TestUpdateCloudCredential(t *testing.T) { c.Assert(err, qt.Equals, nil) c.Assert(credential, jimmtest.DBObjectEquals, expectedCredential) + + gotAttributes, err := j.CredentialStore.Get(ctx, credential.ResourceTag()) + c.Assert(err, qt.Equals, nil) + c.Assert(gotAttributes, qt.DeepEquals, testAttributes) } else { c.Assert(err, qt.ErrorMatches, expectedError) } @@ -1450,9 +1446,6 @@ func TestForEachUserCloudCredential(t *testing.T) { if test.f == nil { test.f = func(cred *dbmodel.CloudCredential) error { credentials = append(credentials, cred.Tag().String()) - if cred.Attributes != nil { - return errors.E("credential contains attributes") - } return nil } } @@ -1651,7 +1644,6 @@ func TestCloudCredentialAttributeStore(t *testing.T) { Name: "test", Type: "test-provider", }, - AttributesInVault: true, }) attr, _, err := j.GetCloudCredentialAttributes(ctx, user, &cred, true) c.Assert(err, qt.IsNil) diff --git a/internal/jimm/controller.go b/internal/jimm/controller.go index feb54eeb3..b3273c640 100644 --- a/internal/jimm/controller.go +++ b/internal/jimm/controller.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm @@ -218,7 +218,7 @@ func addControllerTx(ctx context.Context, j *JIMM, jujuClouds []dbmodel.Cloud, c // code of CodeAlreadyExists will be returned. If the controller cannot be // contacted then an error with a code of CodeConnectionFailed will be // returned. -func (j *JIMM) AddController(ctx context.Context, user *openfga.User, ctl *dbmodel.Controller) error { +func (j *JIMM) AddController(ctx context.Context, user *openfga.User, ctl *dbmodel.Controller, creds ControllerCreds) error { const op = errors.Op("jimm.AddController") if err := j.checkJimmAdmin(user); err != nil { @@ -252,20 +252,11 @@ func (j *JIMM) AddController(ctx context.Context, user *openfga.User, ctl *dbmod dbClouds := convertJujuCloudsToDbClouds(clouds) - // TODO(ale8k): This shouldn't be necessary to check, but tests need updating - // to set insecure credential store explicitly. - if j.CredentialStore != nil { - err := j.CredentialStore.PutControllerCredentials(ctx, ctl.Name, ctl.AdminIdentityName, ctl.AdminPassword) - if err != nil { - return errors.E(op, err, "failed to store controller credentials") - } + err = j.CredentialStore.PutControllerCredentials(ctx, ctl.Name, creds.AdminIdentityName, creds.AdminPassword) + if err != nil { + return errors.E(op, err, "failed to store controller credentials") } - // Credential store will always be set either to vault or explicitly insecure, - // no need to be persist in db. - ctl.AdminIdentityName = "" - ctl.AdminPassword = "" - if err := addControllerTx(ctx, j, dbClouds, ctl); err != nil { zapctx.Error(ctx, "failed to add controller", zaputil.Error(err)) if errors.ErrorCode(err) == errors.CodeAlreadyExists { diff --git a/internal/jimm/controller_test.go b/internal/jimm/controller_test.go index 474c92176..79f44b3d1 100644 --- a/internal/jimm/controller_test.go +++ b/internal/jimm/controller_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm_test @@ -144,12 +144,14 @@ func TestAddController(t *testing.T) { c.Assert(err, qt.IsNil) ctl1 := dbmodel.Controller{ - Name: "test-controller", - AdminIdentityName: "admin", - AdminPassword: "5ecret", - PublicAddress: "example.com:443", + Name: "test-controller", + PublicAddress: "example.com:443", + } + ctlCreds := jimm.ControllerCreds{ + AdminIdentityName: "user", + AdminPassword: "secret", } - err = j.AddController(context.Background(), alice, &ctl1) + err = j.AddController(context.Background(), alice, &ctl1, ctlCreds) c.Assert(err, qt.IsNil) ctl2 := dbmodel.Controller{ @@ -160,12 +162,10 @@ func TestAddController(t *testing.T) { c.Check(ctl2, qt.CmpEquals(cmpopts.EquateEmpty(), cmpopts.IgnoreTypes(dbmodel.CloudRegion{})), ctl1) ctl3 := dbmodel.Controller{ - Name: "test-controller-2", - AdminIdentityName: "admin", - AdminPassword: "5ecret", - PublicAddress: "example.com:443", + Name: "test-controller-2", + PublicAddress: "example.com:443", } - err = j.AddController(context.Background(), alice, &ctl3) + err = j.AddController(context.Background(), alice, &ctl3, ctlCreds) c.Assert(err, qt.IsNil) ctl4 := dbmodel.Controller{ @@ -301,15 +301,15 @@ func TestAddControllerWithVault(t *testing.T) { c.Assert(err, qt.IsNil) ctl1 := dbmodel.Controller{ - Name: "test-controller", + Name: "test-controller", + PublicAddress: "example.com:443", + } + ctlCreds := jimm.ControllerCreds{ AdminIdentityName: "admin", - AdminPassword: "5ecret", - PublicAddress: "example.com:443", + AdminPassword: "secret", } - err = j.AddController(context.Background(), alice, &ctl1) + err = j.AddController(context.Background(), alice, &ctl1, ctlCreds) c.Assert(err, qt.IsNil) - c.Assert(ctl1.AdminIdentityName, qt.Equals, "") - c.Assert(ctl1.AdminPassword, qt.Equals, "") ctl2 := dbmodel.Controller{ Name: "test-controller", @@ -321,18 +321,14 @@ func TestAddControllerWithVault(t *testing.T) { username, password, err := store.GetControllerCredentials(ctx, ctl1.Name) c.Assert(err, qt.IsNil) c.Assert(username, qt.Equals, "admin") - c.Assert(password, qt.Equals, "5ecret") + c.Assert(password, qt.Equals, "secret") ctl3 := dbmodel.Controller{ - Name: "test-controller-2", - AdminIdentityName: "admin", - AdminPassword: "5ecretToo", - PublicAddress: "example.com:443", + Name: "test-controller-2", + PublicAddress: "example.com:443", } - err = j.AddController(context.Background(), alice, &ctl3) + err = j.AddController(context.Background(), alice, &ctl3, ctlCreds) c.Assert(err, qt.IsNil) - c.Assert(ctl3.AdminIdentityName, qt.Equals, "") - c.Assert(ctl3.AdminPassword, qt.Equals, "") ctl4 := dbmodel.Controller{ Name: "test-controller-2", @@ -344,7 +340,7 @@ func TestAddControllerWithVault(t *testing.T) { username, password, err = store.GetControllerCredentials(ctx, ctl4.Name) c.Assert(err, qt.IsNil) c.Assert(username, qt.Equals, "admin") - c.Assert(password, qt.Equals, "5ecretToo") + c.Assert(password, qt.Equals, "secret") } const testEarliestControllerVersionEnv = `clouds: diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 58e5ad2b3..6cc809d4c 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -182,9 +182,7 @@ type Parameters struct { Dialer Dialer // CredentialStore is a store for the attributes of a - // cloud credential and controller credentials. If this is - // not configured then the attributes - // are stored in the standard database. + // cloud credential and controller credentials. CredentialStore credentials.CredentialStore // Pubsub is a pub-sub hub used for buffering model summaries. @@ -760,15 +758,9 @@ func fillMigrationTarget(db *db.Database, credStore credentials.CredentialStore, if err != nil { return jujuparams.MigrationTargetInfo{}, 0, err } - adminUser := dbController.AdminIdentityName - adminPass := dbController.AdminPassword - if adminPass == "" { - u, p, err := credStore.GetControllerCredentials(ctx, controllerName) - if err != nil { - return jujuparams.MigrationTargetInfo{}, 0, err - } - adminUser = u - adminPass = p + adminUser, adminPass, err := credStore.GetControllerCredentials(ctx, controllerName) + if err != nil { + return jujuparams.MigrationTargetInfo{}, 0, err } if adminUser == "" || adminPass == "" { return jujuparams.MigrationTargetInfo{}, 0, errors.E("missing target controller credentials") diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 482086160..162e2e6c8 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm @@ -538,13 +538,8 @@ func (b *modelBuilder) CreateControllerModel() *modelBuilder { func (b *modelBuilder) updateCredential(ctx context.Context, api API, cred *dbmodel.CloudCredential) error { var err error - cred1 := *cred - cred1.Attributes, err = b.jimm.getCloudCredentialAttributes(ctx, cred) - if err != nil { - return err - } - _, err = b.jimm.updateControllerCloudCredential(ctx, &cred1, api.UpdateCredential) + _, err = b.jimm.updateControllerCloudCredential(ctx, cred, api.UpdateCredential) return err } diff --git a/internal/jimm/types.go b/internal/jimm/types.go new file mode 100644 index 000000000..c11868864 --- /dev/null +++ b/internal/jimm/types.go @@ -0,0 +1,7 @@ +// Copyright 2025 Canonical. +package jimm + +type ControllerCreds struct { + AdminIdentityName string + AdminPassword string +} diff --git a/internal/jujuapi/controller.go b/internal/jujuapi/controller.go index d53ed9ad5..342514cc7 100644 --- a/internal/jujuapi/controller.go +++ b/internal/jujuapi/controller.go @@ -12,6 +12,7 @@ import ( "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm" "github.com/canonical/jimm/v3/internal/jujuapi/rpc" "github.com/canonical/jimm/v3/internal/openfga" jimmversion "github.com/canonical/jimm/v3/version" @@ -51,7 +52,7 @@ func init() { // ControllerService defines the methods used to manage controllers. type ControllerService interface { - AddController(ctx context.Context, user *openfga.User, ctl *dbmodel.Controller) error + AddController(ctx context.Context, user *openfga.User, ctl *dbmodel.Controller, creds jimm.ControllerCreds) error 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) diff --git a/internal/jujuapi/jimm.go b/internal/jujuapi/jimm.go index 14bca66a7..7ca770cac 100644 --- a/internal/jujuapi/jimm.go +++ b/internal/jujuapi/jimm.go @@ -18,6 +18,7 @@ import ( "github.com/canonical/jimm/v3/internal/db" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm" "github.com/canonical/jimm/v3/internal/jujuapi/rpc" ofganames "github.com/canonical/jimm/v3/internal/openfga/names" "github.com/canonical/jimm/v3/pkg/api/params" @@ -204,16 +205,18 @@ func (r *controllerRoot) AddController(ctx context.Context, req apiparams.AddCon // TODO(ale8k): Don't build dbmodel here, do it as params to AddController. ctl := dbmodel.Controller{ - UUID: req.UUID, - Name: req.Name, - PublicAddress: req.PublicAddress, - CACertificate: req.CACertificate, + UUID: req.UUID, + Name: req.Name, + PublicAddress: req.PublicAddress, + CACertificate: req.CACertificate, + TLSHostname: req.TLSHostname, + Addresses: dbmodel.HostPorts{jujuparams.FromProviderHostPorts(nphps)}, + } + ctlCreds := jimm.ControllerCreds{ AdminIdentityName: req.Username, AdminPassword: req.Password, - TLSHostname: req.TLSHostname, - Addresses: dbmodel.HostPorts{jujuparams.FromProviderHostPorts(nphps)}, } - if err := r.jimm.AddController(ctx, r.user, &ctl); err != nil { + if err := r.jimm.AddController(ctx, r.user, &ctl, ctlCreds); err != nil { zapctx.Error(ctx, "failed to add controller", zaputil.Error(err)) return apiparams.ControllerInfo{}, errors.E(op, err) } diff --git a/internal/jujuclient/client_test.go b/internal/jujuclient/client_test.go index b3e370349..d123bd0de 100644 --- a/internal/jujuclient/client_test.go +++ b/internal/jujuclient/client_test.go @@ -38,12 +38,10 @@ func (s *clientSuite) TestStatus(c *gc.C) { info := s.APIInfo(c) ctl := dbmodel.Controller{ - UUID: info.ControllerUUID, - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - PublicAddress: info.Addrs[0], + UUID: info.ControllerUUID, + Name: s.ControllerConfig.ControllerName(), + CACertificate: info.CACert, + PublicAddress: info.Addrs[0], } models, err := s.API.UpdateCredential(ctx, cred) diff --git a/internal/jujuclient/dial_test.go b/internal/jujuclient/dial_test.go index 0837f1cca..35bf7991e 100644 --- a/internal/jujuclient/dial_test.go +++ b/internal/jujuclient/dial_test.go @@ -71,12 +71,10 @@ var _ = gc.Suite(&dialSuite{}) func (s *dialSuite) TestDial(c *gc.C) { info := s.APIInfo(c) ctl := dbmodel.Controller{ - UUID: s.ControllerConfig.ControllerUUID(), - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - PublicAddress: info.Addrs[0], + UUID: s.ControllerConfig.ControllerUUID(), + Name: s.ControllerConfig.ControllerName(), + CACertificate: info.CACert, + PublicAddress: info.Addrs[0], } api, err := s.Dialer.Dial(context.Background(), &ctl, names.ModelTag{}, nil) c.Assert(err, gc.Equals, nil) diff --git a/internal/jujuclient/ping_test.go b/internal/jujuclient/ping_test.go index a87257498..7c2497b37 100644 --- a/internal/jujuclient/ping_test.go +++ b/internal/jujuclient/ping_test.go @@ -22,12 +22,10 @@ func (s *pingSuite) TestPing(c *gc.C) { info := s.APIInfo(c) ctl := dbmodel.Controller{ - UUID: s.ControllerConfig.ControllerUUID(), - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - PublicAddress: info.Addrs[0], + UUID: s.ControllerConfig.ControllerUUID(), + Name: s.ControllerConfig.ControllerName(), + CACertificate: info.CACert, + PublicAddress: info.Addrs[0], } api, err := s.Dialer.Dial(ctx, &ctl, names.ModelTag{}, nil) c.Assert(err, gc.Equals, nil) diff --git a/internal/jujuclient/storage_test.go b/internal/jujuclient/storage_test.go index 6aae59632..04032f0b3 100644 --- a/internal/jujuclient/storage_test.go +++ b/internal/jujuclient/storage_test.go @@ -35,12 +35,10 @@ func (s *storageSuite) TestListFilesystems(c *gc.C) { info := s.APIInfo(c) ctl := dbmodel.Controller{ - UUID: s.ControllerConfig.ControllerUUID(), - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - PublicAddress: info.Addrs[0], + UUID: s.ControllerConfig.ControllerUUID(), + Name: s.ControllerConfig.ControllerName(), + CACertificate: info.CACert, + PublicAddress: info.Addrs[0], } models, err := s.API.UpdateCredential(ctx, cred) @@ -81,12 +79,10 @@ func (s *storageSuite) TestListVolumes(c *gc.C) { info := s.APIInfo(c) ctl := dbmodel.Controller{ - UUID: s.ControllerConfig.ControllerUUID(), - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - PublicAddress: info.Addrs[0], + UUID: s.ControllerConfig.ControllerUUID(), + Name: s.ControllerConfig.ControllerName(), + CACertificate: info.CACert, + PublicAddress: info.Addrs[0], } models, err := s.API.UpdateCredential(ctx, cred) @@ -127,12 +123,10 @@ func (s *storageSuite) TestListStorageDetails(c *gc.C) { info := s.APIInfo(c) ctl := dbmodel.Controller{ - UUID: s.ControllerConfig.ControllerUUID(), - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - PublicAddress: info.Addrs[0], + UUID: s.ControllerConfig.ControllerUUID(), + Name: s.ControllerConfig.ControllerName(), + CACertificate: info.CACert, + PublicAddress: info.Addrs[0], } models, err := s.API.UpdateCredential(ctx, cred) diff --git a/internal/testutils/cmdtest/jimmsuite.go b/internal/testutils/cmdtest/jimmsuite.go index 349b043e4..ebd62deeb 100644 --- a/internal/testutils/cmdtest/jimmsuite.go +++ b/internal/testutils/cmdtest/jimmsuite.go @@ -196,12 +196,14 @@ func (s *JimmCmdSuite) RefreshControllerAddress(c *gc.C) { func (s *JimmCmdSuite) AddController(c *gc.C, name string, info *api.Info) { ctl := &dbmodel.Controller{ - UUID: info.ControllerUUID, - Name: name, + UUID: info.ControllerUUID, + Name: name, + CACertificate: info.CACert, + Addresses: nil, + } + ctlCreds := jimm.ControllerCreds{ AdminIdentityName: info.Tag.Id(), AdminPassword: info.Password, - CACertificate: info.CACert, - Addresses: nil, } ctl.Addresses = make(dbmodel.HostPorts, 0, len(info.Addrs)) for _, addr := range info.Addrs { @@ -214,7 +216,7 @@ func (s *JimmCmdSuite) AddController(c *gc.C, name string, info *api.Info) { } adminUser := openfga.NewUser(s.AdminUser, s.OFGAClient) adminUser.JimmAdmin = true - err := s.JIMM.AddController(context.Background(), adminUser, ctl) + err := s.JIMM.AddController(context.Background(), adminUser, ctl, ctlCreds) c.Assert(err, gc.Equals, nil) } diff --git a/internal/testutils/jimmtest/env.go b/internal/testutils/jimmtest/env.go index 4556fe402..81d22c43b 100644 --- a/internal/testutils/jimmtest/env.go +++ b/internal/testutils/jimmtest/env.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimmtest @@ -182,17 +182,6 @@ func (m Model) addModelRelations(c *qt.C, db *db.Database, client *openfga.OFGAC // addControllerRelations adds permissions the model should have and adds permissions for users to the controller. func (ctl Controller) addControllerRelations(c *qt.C, client *openfga.OFGAClient) { - if ctl.dbo.AdminIdentityName != "" { - userIdentity, err := dbmodel.NewIdentity(ctl.dbo.AdminIdentityName) - c.Assert(err, qt.IsNil) - - user := openfga.NewUser( - userIdentity, - client, - ) - err = user.SetControllerAccess(context.Background(), ctl.dbo.ResourceTag(), ofganames.AdministratorRelation) - c.Assert(err, qt.IsNil) - } err := client.AddCloudController(context.Background(), names.NewCloudTag(ctl.Cloud), ctl.dbo.ResourceTag()) c.Assert(err, qt.IsNil) } @@ -358,11 +347,10 @@ func (cl *Cloud) DBObject(c Tester, db *db.Database) dbmodel.Cloud { // A CloudCredential represents the definition of a cloud credential in a // test environment. type CloudCredential struct { - Owner string `json:"owner"` - Cloud string `json:"cloud"` - Name string `json:"name"` - AuthType string `json:"auth-type"` - Attributes map[string]string `json:"attributes"` + Owner string `json:"owner"` + Cloud string `json:"cloud"` + Name string `json:"name"` + AuthType string `json:"auth-type"` env *Environment dbo dbmodel.CloudCredential @@ -378,7 +366,6 @@ func (cc *CloudCredential) DBObject(c Tester, db *db.Database) dbmodel.CloudCred cc.dbo.Owner = cc.env.User(cc.Owner).DBObject(c, db) cc.dbo.OwnerIdentityName = cc.dbo.Owner.Name cc.dbo.AuthType = cc.AuthType - cc.dbo.Attributes = cc.Attributes err := db.SetCloudCredential(context.Background(), &cc.dbo) if err != nil { @@ -391,14 +378,12 @@ func (cc *CloudCredential) DBObject(c Tester, db *db.Database) dbmodel.CloudCred // A Controller represents the definition of a controller in a test // environment. type Controller struct { - Name string `json:"name"` - UUID string `json:"uuid"` - Cloud string `json:"cloud"` - CloudRegion string `json:"region"` - CloudRegions []CloudRegionControllerPriority `json:"cloud-regions"` - AgentVersion string `json:"agent-version"` - AdminUser string `json:"admin-user"` - AdminPassword string `json:"admin-password"` + Name string `json:"name"` + UUID string `json:"uuid"` + Cloud string `json:"cloud"` + CloudRegion string `json:"region"` + CloudRegions []CloudRegionControllerPriority `json:"cloud-regions"` + AgentVersion string `json:"agent-version"` env *Environment dbo dbmodel.Controller @@ -411,8 +396,6 @@ func (ctl *Controller) DBObject(c Tester, db *db.Database) dbmodel.Controller { ctl.dbo.Name = ctl.Name ctl.dbo.UUID = ctl.UUID ctl.dbo.AgentVersion = ctl.AgentVersion - ctl.dbo.AdminIdentityName = ctl.AdminUser - ctl.dbo.AdminPassword = ctl.AdminPassword ctl.dbo.CloudName = ctl.Cloud ctl.dbo.CloudRegion = ctl.CloudRegion ctl.dbo.CloudRegions = make([]dbmodel.CloudRegionControllerPriority, len(ctl.CloudRegions)) diff --git a/internal/testutils/jimmtest/gorm.go b/internal/testutils/jimmtest/gorm.go index f8a24cfd6..7da8d4e6e 100644 --- a/internal/testutils/jimmtest/gorm.go +++ b/internal/testutils/jimmtest/gorm.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. // Package jimmtest contains useful helpers for testing JIMM. package jimmtest @@ -65,7 +65,7 @@ func PostgresDBWithDbName(t Tester, nowFunc func() time.Time) (*gorm.DB, string) templateDatabaseName, _, err := getOrCreateTemplateDatabase() if err != nil { - t.Fatalf("template database does not exist") + t.Fatalf("template database does not exist: %s", err) } suggestedName := "jimm_test_" + t.Name() @@ -322,7 +322,7 @@ func getOrCreateTemplateDatabase() (string, string, error) { templateName, templateDSN, err := createTemplateDatabase() if err != nil { - return "", "", errors.E(err, "error creating template database") + return "", "", fmt.Errorf("error creating template database: %w", err) } templateDatabaseDSN = templateDSN diff --git a/internal/testutils/jimmtest/jimm.go b/internal/testutils/jimmtest/jimm.go index ca1fd8496..027e93173 100644 --- a/internal/testutils/jimmtest/jimm.go +++ b/internal/testutils/jimmtest/jimm.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimmtest @@ -21,6 +21,9 @@ var ( UnsetCredentialStore Option = func(j *jimm.JIMM) { j.CredentialStore = nil } + UsePostgresAsCredentialStore Option = func(j *jimm.JIMM) { + j.CredentialStore = j.Database + } ) func NewJIMM(t Tester, additionalParameters *jimm.Parameters, options ...Option) *jimm.JIMM { diff --git a/internal/testutils/jimmtest/mocks/jimm_controller_mock.go b/internal/testutils/jimmtest/mocks/jimm_controller_mock.go index 57bf530ac..9f54810b1 100644 --- a/internal/testutils/jimmtest/mocks/jimm_controller_mock.go +++ b/internal/testutils/jimmtest/mocks/jimm_controller_mock.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package mocks @@ -9,12 +9,13 @@ import ( "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm" "github.com/canonical/jimm/v3/internal/openfga" ) // ControllerService is an implementation of the jujuapi.ControllerService interface. type ControllerService struct { - AddController_ func(ctx context.Context, u *openfga.User, ctl *dbmodel.Controller) error + AddController_ func(ctx context.Context, u *openfga.User, ctl *dbmodel.Controller, creds jimm.ControllerCreds) error ControllerInfo_ func(ctx context.Context, name string) (*dbmodel.Controller, error) EarliestControllerVersion_ func(ctx context.Context) (version.Number, error) ListControllers_ func(ctx context.Context, user *openfga.User) ([]dbmodel.Controller, error) @@ -22,11 +23,11 @@ type ControllerService struct { SetControllerDeprecated_ func(ctx context.Context, user *openfga.User, controllerName string, deprecated bool) error } -func (j *ControllerService) AddController(ctx context.Context, u *openfga.User, ctl *dbmodel.Controller) error { +func (j *ControllerService) AddController(ctx context.Context, u *openfga.User, ctl *dbmodel.Controller, creds jimm.ControllerCreds) error { if j.AddController_ == nil { return errors.E(errors.CodeNotImplemented) } - return j.AddController_(ctx, u, ctl) + return j.AddController_(ctx, u, ctl, creds) } func (j *ControllerService) ControllerInfo(ctx context.Context, name string) (*dbmodel.Controller, error) { diff --git a/internal/testutils/jimmtest/suite.go b/internal/testutils/jimmtest/suite.go index 9f3691b8d..19ad641c1 100644 --- a/internal/testutils/jimmtest/suite.go +++ b/internal/testutils/jimmtest/suite.go @@ -278,12 +278,14 @@ func (s *JIMMSuite) NewUser(u *dbmodel.Identity) *openfga.User { func (s *JIMMSuite) AddController(c *gc.C, name string, info *api.Info) { ctl := &dbmodel.Controller{ - UUID: info.ControllerUUID, - Name: name, + UUID: info.ControllerUUID, + Name: name, + CACertificate: info.CACert, + Addresses: nil, + } + ctlCreds := jimm.ControllerCreds{ AdminIdentityName: info.Tag.Id(), AdminPassword: info.Password, - CACertificate: info.CACert, - Addresses: nil, } ctl.Addresses = make(dbmodel.HostPorts, 0, len(info.Addrs)) for _, addr := range info.Addrs { @@ -294,7 +296,7 @@ func (s *JIMMSuite) AddController(c *gc.C, name string, info *api.Info) { Port: hp.Port(), }}) } - err := s.JIMM.AddController(context.Background(), s.AdminUser, ctl) + err := s.JIMM.AddController(context.Background(), s.AdminUser, ctl, ctlCreds) c.Assert(err, gc.Equals, nil) }