Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: don't allow nil credential store #1514

Merged
merged 4 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions cmd/jimmctl/cmd/listcontrollers_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package cmd_test

Expand All @@ -23,7 +23,6 @@ var (
-----END CERTIFICATE-----
cloudtag: cloud-` + jimmtest.TestCloudName + `
cloudregion: ` + jimmtest.TestCloudRegionName + `
username: admin
agentversion: .*
status:
status: available
Expand All @@ -41,7 +40,6 @@ var (
-----END CERTIFICATE-----
cloudtag: cloud-` + jimmtest.TestCloudName + `
cloudregion: ` + jimmtest.TestCloudRegionName + `
username: admin
agentversion: .*
status:
status: available
Expand All @@ -57,7 +55,6 @@ var (
cacertificate: ""
cloudtag: ""
cloudregion: ""
username: ""
agentversion: .*
status:
status: available
Expand Down
4 changes: 2 additions & 2 deletions internal/db/cloudcredential.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package db

Expand Down Expand Up @@ -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))
}
Expand Down
18 changes: 1 addition & 17 deletions internal/db/cloudcredential_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package db_test

Expand Down Expand Up @@ -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,
Expand All @@ -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)
}
Expand Down Expand Up @@ -219,15 +209,9 @@ cloud-credentials:
- name: cred-1
cloud: cloud-1
owner: alice@canonical.com
attributes:
k1: v1
k2: v2
- name: cred-2
cloud: cloud-1
owner: bob@canonical.com
attributes:
k1: v1
k2: v2
- name: cred-3
cloud: cloud-2
owner: alice@canonical.com
Expand Down
5 changes: 4 additions & 1 deletion internal/db/secrets.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package db

Expand Down Expand Up @@ -126,6 +126,9 @@ func (d *Database) Get(ctx context.Context, tag names.CloudCredentialTag) (_ map
secret := dbmodel.NewSecret(tag.Kind(), tag.String(), nil)
err = d.GetSecret(ctx, &secret)
if err != nil {
if errors.ErrorCode(err) == errors.CodeNotFound {
return nil, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we returning no error if it is not found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behaviour of our Vault client, that test where I set it to run against Postgres and Vault showed that our Postgres implementation was not consistent with our Vault implementation. So I've changed the Postgres implementation to match Vault.

zapctx.Error(ctx, "failed to get secret data", zap.Error(err))
return nil, errors.E(op, err)
}
Expand Down
7 changes: 0 additions & 7 deletions internal/dbmodel/cloudcredential.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 0 additions & 4 deletions internal/dbmodel/cloudcredential_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 0 additions & 10 deletions internal/dbmodel/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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:
Expand Down
16 changes: 6 additions & 10 deletions internal/dbmodel/controller_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package dbmodel_test

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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()
Expand All @@ -182,7 +179,6 @@ func TestToAPIControllerInfo(t *testing.T) {
CACertificate: "ca-cert",
CloudTag: names.NewCloudTag("test-cloud").String(),
CloudRegion: "test-region",
Username: "admin",
AgentVersion: "1.2.3",
Status: jujuparams.EntityStatus{
Status: "available",
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
48 changes: 8 additions & 40 deletions internal/jimm/cloudcredential.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package jimm

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}

Expand All @@ -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
}

Expand All @@ -263,13 +247,9 @@ 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)
}
attr, err := j.getCloudCredentialAttributes(ctx, cred)
if err != nil {
return nil, errors.E(op, err)
}

models, err := f(ctx, jujuparams.TaggedCredential{
Expand Down Expand Up @@ -302,7 +282,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
Expand Down Expand Up @@ -365,17 +344,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)
Expand Down
Loading
Loading