From 9c1b367bb14faa73454d39f4ecf44b798e714c04 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Thu, 9 Jan 2025 12:36:12 +0200 Subject: [PATCH] chore: fix tests that require credential attributes --- internal/db/cloudcredential_test.go | 6 - internal/db/secrets.go | 5 +- internal/dbmodel/controller_test.go | 3 +- internal/jimm/cloudcredential_test.go | 96 +++++++++------ internal/jimm/model_test.go | 167 ++++++++++++++------------ internal/jimm/service_account_test.go | 6 +- internal/testutils/jimmtest/jimm.go | 9 -- pkg/api/params/params.go | 6 +- 8 files changed, 160 insertions(+), 138 deletions(-) diff --git a/internal/db/cloudcredential_test.go b/internal/db/cloudcredential_test.go index 774fae361..c534a1806 100644 --- a/internal/db/cloudcredential_test.go +++ b/internal/db/cloudcredential_test.go @@ -209,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 diff --git a/internal/db/secrets.go b/internal/db/secrets.go index 7d73b1ccf..903546297 100644 --- a/internal/db/secrets.go +++ b/internal/db/secrets.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package db @@ -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 + } zapctx.Error(ctx, "failed to get secret data", zap.Error(err)) return nil, errors.E(op, err) } diff --git a/internal/dbmodel/controller_test.go b/internal/dbmodel/controller_test.go index 1faa5f245..4e620e571 100644 --- a/internal/dbmodel/controller_test.go +++ b/internal/dbmodel/controller_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package dbmodel_test @@ -179,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", diff --git a/internal/jimm/cloudcredential_test.go b/internal/jimm/cloudcredential_test.go index a0530785b..5615aefab 100644 --- a/internal/jimm/cloudcredential_test.go +++ b/internal/jimm/cloudcredential_test.go @@ -23,6 +23,7 @@ import ( "github.com/canonical/jimm/v3/internal/openfga" ofganames "github.com/canonical/jimm/v3/internal/openfga/names" "github.com/canonical/jimm/v3/internal/testutils/jimmtest" + "github.com/canonical/jimm/v3/internal/vault" ) func TestUpdateCloudCredential(t *testing.T) { @@ -771,7 +772,6 @@ func TestUpdateCloudCredential(t *testing.T) { API: api, }, }, - jimmtest.UsePostgresAsCredentialStore, // this test relies on credential attributes being stored in postgres ) u, arg, expectedCredential, expectedError := test.createEnv(c, j) @@ -1365,15 +1365,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 @@ -1474,11 +1468,6 @@ cloud-credentials: cloud: test-cloud owner: bob@canonical.com auth-type: oauth2 - attributes: - client-email: bob@example.com - client-id: 1234 - private-key: super-secret - project-id: 5678 - name: cred-2 cloud: test-cloud owner: bob@canonical.com @@ -1495,6 +1484,7 @@ var getCloudCredentialAttributesTests = []struct { hidden bool jimmAdmin bool cred string + skipAttributes bool expectAttributes map[string]string expectRedacted []string expectError string @@ -1515,6 +1505,7 @@ var getCloudCredentialAttributesTests = []struct { username: "bob@canonical.com", jimmAdmin: true, cred: "cred-2", + skipAttributes: true, expectAttributes: map[string]string{}, expectRedacted: nil, }, { @@ -1556,37 +1547,66 @@ var getCloudCredentialAttributesTests = []struct { }} func TestGetCloudCredentialAttributes(t *testing.T) { - c := qt.New(t) + attributes := map[string]string{ + "client-email": "bob@example.com", + "client-id": "1234", + "private-key": "super-secret", + "project-id": "5678", + } for _, test := range getCloudCredentialAttributesTests { - c.Run(test.name, func(c *qt.C) { - ctx := context.Background() - - j := jimmtest.NewJIMM(c, nil) - - env := jimmtest.ParseEnvironment(c, getCloudCredentialAttributesEnv) - env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, j.OpenFGAClient) - u := env.User("bob@canonical.com").DBObject(c, j.Database) - userBob := openfga.NewUser(&u, j.OpenFGAClient) - credTag := fmt.Sprintf("test-cloud/bob@canonical.com/%s", test.cred) - cred, err := j.GetCloudCredential(ctx, userBob, names.NewCloudCredentialTag(credTag)) - c.Assert(err, qt.IsNil) + c := qt.New(t) + // Run each test twice, once with Vault as a credential store + // and again with Postgres as a credential store. + client, path, roleID, roleSecretID, ok := jimmtest.VaultClient(c) + c.Assert(ok, qt.IsTrue) + vaultStore := &vault.VaultStore{ + Client: client, + RoleID: roleID, + RoleSecretID: roleSecretID, + KVPath: path, + } + jimmWithVault := jimm.Parameters{CredentialStore: vaultStore} + + testF := func(jp *jimm.Parameters) func(c *qt.C) { + return func(c *qt.C) { + ctx := context.Background() + + j := jimmtest.NewJIMM(c, jp) + + env := jimmtest.ParseEnvironment(c, getCloudCredentialAttributesEnv) + env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, j.OpenFGAClient) + + u := env.User("bob@canonical.com").DBObject(c, j.Database) + userBob := openfga.NewUser(&u, j.OpenFGAClient) + + credTag := names.NewCloudCredentialTag(fmt.Sprintf("test-cloud/bob@canonical.com/%s", test.cred)) + cred, err := j.GetCloudCredential(ctx, userBob, credTag) + c.Assert(err, qt.IsNil) + + if !test.skipAttributes { + err = j.CredentialStore.Put(ctx, credTag, attributes) + c.Assert(err, qt.IsNil) + } - u = env.User(test.username).DBObject(c, j.Database) - userTest := openfga.NewUser(&u, j.OpenFGAClient) - userTest.JimmAdmin = test.jimmAdmin - attr, redacted, err := j.GetCloudCredentialAttributes(ctx, userTest, cred, test.hidden) - if test.expectError != "" { - c.Check(err, qt.ErrorMatches, test.expectError) - if test.expectErrorCode != "" { - c.Check(errors.ErrorCode(err), qt.Equals, test.expectErrorCode) + u = env.User(test.username).DBObject(c, j.Database) + userTest := openfga.NewUser(&u, j.OpenFGAClient) + userTest.JimmAdmin = test.jimmAdmin + attr, redacted, err := j.GetCloudCredentialAttributes(ctx, userTest, cred, test.hidden) + if test.expectError != "" { + c.Check(err, qt.ErrorMatches, test.expectError) + if test.expectErrorCode != "" { + c.Check(errors.ErrorCode(err), qt.Equals, test.expectErrorCode) + } + return } - return + c.Assert(err, qt.IsNil) + c.Check(attr, qt.DeepEquals, test.expectAttributes) + c.Check(redacted, qt.DeepEquals, test.expectRedacted) } - c.Assert(err, qt.IsNil) - c.Check(attr, qt.DeepEquals, test.expectAttributes) - c.Check(redacted, qt.DeepEquals, test.expectRedacted) - }) + } + c.Run(test.name+"-postgres", testF(nil)) + c.Run(test.name+"-vault", testF(&jimmWithVault)) } } diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index f6a27ed46..8d39c962c 100644 --- a/internal/jimm/model_test.go +++ b/internal/jimm/model_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm_test @@ -139,9 +139,12 @@ var addModelTests = []struct { createModel func(ctx context.Context, args *jujuparams.ModelCreateArgs, mi *jujuparams.ModelInfo) error username string jimmAdmin bool - args jujuparams.ModelCreateArgs - expectModel dbmodel.Model - expectError string + // This cloudCredTag is used to manually populate a dummy cloud credential + // into JIMM's credential store and then applied onto args before adding a model. + cloudCredTag names.CloudCredentialTag + args jujuparams.ModelCreateArgs + expectModel dbmodel.Model + expectError string }{{ name: "CreateModelWithCloudRegion", env: ` @@ -209,14 +212,14 @@ users: - user: bob access: read `[1:])), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectModel: dbmodel.Model{ Name: "test-model", @@ -313,15 +316,15 @@ users: - user: bob access: read `[1:])), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ Name: "test-model", OwnerTag: names.NewUserTag("alice@canonical.com").String(), CloudTag: names.NewCloudTag("test-cloud").String(), // Creating a model without specifying the cloud region - CloudRegion: "", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + CloudRegion: "", }, expectModel: dbmodel.Model{ Name: "test-model", @@ -418,14 +421,14 @@ users: - user: bob access: read `[1:])), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectModel: dbmodel.Model{ Name: "test-model", @@ -514,14 +517,14 @@ users: - user: bob access: read `[1:]), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("bob@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("bob@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectModel: dbmodel.Model{ Name: "test-model", @@ -608,13 +611,13 @@ users: - user: bob access: read `[1:]), - username: "alice@canonical.com", + username: "alice@canonical.com", + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("bob@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("bob@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectError: "unauthorized", }, { @@ -660,14 +663,14 @@ controllers: createModel: func(ctx context.Context, args *jujuparams.ModelCreateArgs, mi *jujuparams.ModelInfo) error { return errors.E("a test error") }, - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectError: "a test error", }, { @@ -738,14 +741,14 @@ users: - user: bob access: read `[1:]), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectError: "model alice@canonical.com/test-model already exists", }, { @@ -800,14 +803,14 @@ users: - user: bob access: read `[1:]), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectError: "failed to update cloud credential: a silly error", }, { @@ -857,14 +860,14 @@ users: - user: alice@canonical.com access: admin `[1:]), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudTag: names.NewCloudTag("test-cloud").String(), - CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), + CloudTag: names.NewCloudTag("test-cloud").String(), + CloudRegion: "test-region-1", }, expectError: "unauthorized", }, { @@ -937,12 +940,12 @@ users: - user: bob access: read `[1:])), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), }, expectModel: dbmodel.Model{ Name: "test-model", @@ -1044,12 +1047,12 @@ users: - user: bob access: read `[1:])), - username: "alice@canonical.com", - jimmAdmin: true, + username: "alice@canonical.com", + jimmAdmin: true, + cloudCredTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1"), args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + Name: "test-model", + OwnerTag: names.NewUserTag("alice@canonical.com").String(), }, expectError: "no cloud specified for model; please specify one", }} @@ -1074,12 +1077,16 @@ func TestAddModel(t *testing.T) { env := jimmtest.ParseEnvironment(c, test.env) env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, j.OpenFGAClient) + err := j.CredentialStore.Put(ctx, test.cloudCredTag, map[string]string{"key": "value"}) + c.Assert(err, qt.IsNil) + dbUser := env.User(test.username).DBObject(c, j.Database) user := openfga.NewUser(&dbUser, j.OpenFGAClient) user.JimmAdmin = test.jimmAdmin + test.args.CloudCredentialTag = test.cloudCredTag.String() args := jimm.ModelCreateArgs{} - err := args.FromJujuModelCreateArgs(&test.args) + err = args.FromJujuModelCreateArgs(&test.args) c.Assert(err, qt.IsNil) _, err = j.AddModel(context.Background(), user, &args) @@ -3675,7 +3682,11 @@ func TestUpdateModelCredential(t *testing.T) { dbUser := env.User(test.username).DBObject(c, j.Database) user := openfga.NewUser(&dbUser, j.OpenFGAClient) - err := j.ChangeModelCredential( + testAttributes := map[string]string{"key": "value"} + err := j.CredentialStore.Put(ctx, names.NewCloudCredentialTag(test.credential), testAttributes) + c.Assert(err, qt.IsNil) + + err = j.ChangeModelCredential( ctx, user, names.NewModelTag(test.uuid), @@ -3791,13 +3802,17 @@ controllers: err = j.Database.DeleteController(ctx, &controller) c.Assert(err, qt.IsNil) + cloudCredTag := names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1") + err = j.CredentialStore.Put(ctx, cloudCredTag, map[string]string{"key": "value"}) + c.Assert(err, qt.IsNil) + args := jimm.ModelCreateArgs{} err = args.FromJujuModelCreateArgs(&jujuparams.ModelCreateArgs{ Name: "test-model", OwnerTag: names.NewUserTag("alice@canonical.com").String(), CloudTag: names.NewCloudTag("test-cloud").String(), CloudRegion: "test-region-1", - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), + CloudCredentialTag: cloudCredTag.String(), }) c.Assert(err, qt.IsNil) diff --git a/internal/jimm/service_account_test.go b/internal/jimm/service_account_test.go index 6e09ee628..4e1616ba8 100644 --- a/internal/jimm/service_account_test.go +++ b/internal/jimm/service_account_test.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package jimm_test @@ -116,6 +116,10 @@ func TestCopyServiceAccountCredential(t *testing.T) { err = j.Database.SetCloudCredential(context.Background(), &cred) c.Assert(err, qt.Equals, nil) + credAttr := map[string]string{"key": "value"} + err = j.CredentialStore.Put(ctx, cred.ResourceTag(), credAttr) + c.Assert(err, qt.Equals, nil) + _, res, err := j.CopyServiceAccountCredential(ctx, user, svcAcc, cred.ResourceTag()) c.Assert(err, qt.Equals, nil) newCred := dbmodel.CloudCredential{ diff --git a/internal/testutils/jimmtest/jimm.go b/internal/testutils/jimmtest/jimm.go index 027e93173..b58573828 100644 --- a/internal/testutils/jimmtest/jimm.go +++ b/internal/testutils/jimmtest/jimm.go @@ -17,15 +17,6 @@ var now = (time.Time{}).UTC().Round(time.Millisecond) type Option func(j *jimm.JIMM) -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 { auth := NewMockOAuthAuthenticator(t, nil) diff --git a/pkg/api/params/params.go b/pkg/api/params/params.go index b3a2ecf83..64d899df1 100644 --- a/pkg/api/params/params.go +++ b/pkg/api/params/params.go @@ -1,4 +1,4 @@ -// Copyright 2024 Canonical. +// Copyright 2025 Canonical. package params @@ -168,10 +168,6 @@ type ControllerInfo struct { // CloudRegion is the region that this controller is running in. CloudRegion string `json:"cloud-region,omitempty"` - // Username contains the username that JIMM uses to connect to the - // controller. - Username string `json:"username"` - // The version of the juju agent running on the controller. AgentVersion string `json:"agent-version"`