From adcb70b7fc96a19f07875936323d1006d109f4c7 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Mon, 26 Aug 2024 11:37:58 +0200 Subject: [PATCH] Allow adding a model with implicit cloud. If the user does not specify which cloud to add the model to and only one cloud is known to JIMM, we will select that cloud and continue instead of returning an error. --- internal/jimm/model.go | 59 ++++++-- internal/jimm/model_test.go | 203 +++++++++++++++++++++++++- internal/jujuapi/modelmanager_test.go | 12 +- 3 files changed, 246 insertions(+), 28 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 1d2ee823a..0b60539b3 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -55,14 +55,13 @@ func (a *ModelCreateArgs) FromJujuModelCreateArgs(args *jujuparams.ModelCreateAr a.Name = args.Name a.Config = args.Config a.CloudRegion = args.CloudRegion - if args.CloudTag == "" { - return errors.E("no cloud specified for model; please specify one") - } - ct, err := names.ParseCloudTag(args.CloudTag) - if err != nil { - return errors.E(err, errors.CodeBadRequest) + if args.CloudTag != "" { + ct, err := names.ParseCloudTag(args.CloudTag) + if err != nil { + return errors.E(err, errors.CodeBadRequest) + } + a.Cloud = ct } - a.Cloud = ct if args.OwnerTag == "" { return errors.E("owner tag not specified") @@ -175,10 +174,17 @@ func (b *modelBuilder) WithConfig(cfg map[string]interface{}) *modelBuilder { } // WithCloud returns a builder with the specified cloud. -func (b *modelBuilder) WithCloud(cloud names.CloudTag) *modelBuilder { +func (b *modelBuilder) WithCloud(user *openfga.User, cloud names.CloudTag) *modelBuilder { if b.err != nil { return b } + + // if cloud was not specified then we try to determine if + // JIMM knows of only one cloud and use that one + if cloud.Id() == "" { + return b.withImplicitCloud(user) + } + c := dbmodel.Cloud{ Name: cloud.Id(), } @@ -192,6 +198,34 @@ func (b *modelBuilder) WithCloud(cloud names.CloudTag) *modelBuilder { return b } +// withImplicitCloud returns a builder with the only cloud known to JIMM. Should JIMM +// know of multiple clouds an error will be raised. +func (b *modelBuilder) withImplicitCloud(user *openfga.User) *modelBuilder { + if b.err != nil { + return b + } + var clouds []*dbmodel.Cloud + err := b.jimm.ForEachUserCloud(b.ctx, user, func(c *dbmodel.Cloud) error { + clouds = append(clouds, c) + return nil + }) + if err != nil { + b.err = err + return b + } + if len(clouds) == 0 { + b.err = fmt.Errorf("no available clouds") + return b + } + if len(clouds) != 1 { + b.err = fmt.Errorf("no cloud specified for model; please specify one") + return b + } + b.cloud = clouds[0] + + return b +} + // WithCloudRegion returns a builder with the specified cloud region. func (b *modelBuilder) WithCloudRegion(region string) *modelBuilder { if b.err != nil { @@ -563,12 +597,11 @@ func (j *JIMM) AddModel(ctx context.Context, user *openfga.User, args *ModelCrea builder = builder.WithConfig(cloudDefaults.Defaults) } - if args.Cloud != (names.CloudTag{}) { - builder = builder.WithCloud(args.Cloud) - if err := builder.Error(); err != nil { - return nil, errors.E(op, err) - } + builder = builder.WithCloud(user, args.Cloud) + if err := builder.Error(); err != nil { + return nil, errors.E(op, err) } + builder = builder.WithCloudRegion(args.CloudRegion) if err := builder.Error(); err != nil { return nil, errors.E(op, err) diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index 57b7cbcc0..f7e256803 100644 --- a/internal/jimm/model_test.go +++ b/internal/jimm/model_test.go @@ -104,14 +104,6 @@ func TestModelCreateArgs(t *testing.T) { CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice/test-credential-1").String(), }, expectedError: "owner tag not specified", - }, { - about: "cloud tag not specified", - args: jujuparams.ModelCreateArgs{ - Name: "test-model", - OwnerTag: names.NewUserTag("alice@canonical.com").String(), - CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice/test-credential-1").String(), - }, - expectedError: "no cloud specified for model; please specify one", }} opts := []cmp.Option{ @@ -886,6 +878,198 @@ users: CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/alice@canonical.com/test-credential-1").String(), }, expectError: "unauthorized", +}, { + name: "CreateModelWithImplicitCloud", + env: ` +clouds: +- name: test-cloud + type: test-provider + regions: + - name: test-region-1 + users: + - user: alice@canonical.com + access: add-model +user-defaults: +- user: alice@canonical.com + defaults: + key4: value4 +cloud-defaults: +- user: alice@canonical.com + cloud: test-cloud + region: test-region-1 + defaults: + key1: value1 + key2: value2 +- user: alice@canonical.com + cloud: test-cloud + defaults: + key3: value3 +cloud-credentials: +- name: test-credential-1 + owner: alice@canonical.com + cloud: test-cloud + auth-type: empty +controllers: +- name: controller-1 + uuid: 00000000-0000-0000-0000-0000-0000000000001 + cloud: test-cloud + region: test-region-1 + cloud-regions: + - cloud: test-cloud + region: test-region-1 + priority: 0 +- name: controller-2 + uuid: 00000000-0000-0000-0000-0000-0000000000002 + cloud: test-cloud + region: test-region-1 + cloud-regions: + - cloud: test-cloud + region: test-region-1 + priority: 2 +`[1:], + updateCredential: func(_ context.Context, _ jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error) { + return nil, nil + }, + grantJIMMModelAdmin: func(_ context.Context, _ names.ModelTag) error { + return nil + }, + createModel: assertConfig(map[string]interface{}{ + "key4": "value4", + }, createModel(` +uuid: 00000001-0000-0000-0000-0000-000000000001 +status: + status: started + info: running a test +life: alive +users: +- user: alice@canonical.com + access: admin +- user: bob + access: read +`[1:])), + username: "alice@canonical.com", + jimmAdmin: true, + 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(), + }, + expectModel: dbmodel.Model{ + Name: "test-model", + UUID: sql.NullString{ + String: "00000001-0000-0000-0000-0000-000000000001", + Valid: true, + }, + Owner: dbmodel.Identity{ + Name: "alice@canonical.com", + }, + Controller: dbmodel.Controller{ + Name: "controller-2", + UUID: "00000000-0000-0000-0000-0000-0000000000002", + CloudName: "test-cloud", + CloudRegion: "test-region-1", + }, + CloudRegion: dbmodel.CloudRegion{ + Cloud: dbmodel.Cloud{ + Name: "test-cloud", + Type: "test-provider", + }, + Name: "test-region-1", + }, + CloudCredential: dbmodel.CloudCredential{ + Name: "test-credential-1", + AuthType: "empty", + }, + Life: state.Alive.String(), + Status: dbmodel.Status{ + Status: "started", + Info: "running a test", + }, + }, +}, { + name: "CreateModelWithImplicitCloudAndMultipleClouds", + env: ` +clouds: +- name: test-cloud + type: test-provider + regions: + - name: test-region-1 + users: + - user: alice@canonical.com + access: add-model +- name: test-cloud-2 + type: test-provider-2 + regions: + - name: test-region-2 + users: + - user: alice@canonical.com + access: add-model +user-defaults: +- user: alice@canonical.com + defaults: + key4: value4 +cloud-defaults: +- user: alice@canonical.com + cloud: test-cloud + region: test-region-1 + defaults: + key1: value1 + key2: value2 +- user: alice@canonical.com + cloud: test-cloud + defaults: + key3: value3 +cloud-credentials: +- name: test-credential-1 + owner: alice@canonical.com + cloud: test-cloud + auth-type: empty +controllers: +- name: controller-1 + uuid: 00000000-0000-0000-0000-0000-0000000000001 + cloud: test-cloud + region: test-region-1 + cloud-regions: + - cloud: test-cloud + region: test-region-1 + priority: 0 +- name: controller-2 + uuid: 00000000-0000-0000-0000-0000-0000000000002 + cloud: test-cloud + region: test-region-1 + cloud-regions: + - cloud: test-cloud + region: test-region-1 + priority: 2 +`[1:], + updateCredential: func(_ context.Context, _ jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error) { + return nil, nil + }, + grantJIMMModelAdmin: func(_ context.Context, _ names.ModelTag) error { + return nil + }, + createModel: assertConfig(map[string]interface{}{ + "key4": "value4", + }, createModel(` +uuid: 00000001-0000-0000-0000-0000-000000000001 +status: + status: started + info: running a test +life: alive +users: +- user: alice@canonical.com + access: admin +- user: bob + access: read +`[1:])), + username: "alice@canonical.com", + jimmAdmin: true, + 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(), + }, + expectError: "no cloud specified for model; please specify one", }} func TestAddModel(t *testing.T) { @@ -982,6 +1166,9 @@ func createModel(template string) func(context.Context, *jujuparams.ModelCreateA func assertConfig(config map[string]interface{}, fnc func(context.Context, *jujuparams.ModelCreateArgs, *jujuparams.ModelInfo) error) func(context.Context, *jujuparams.ModelCreateArgs, *jujuparams.ModelInfo) error { return func(ctx context.Context, args *jujuparams.ModelCreateArgs, mi *jujuparams.ModelInfo) error { + if args.CloudTag == "" { + return errors.E("cloud not specified") + } if len(config) != len(args.Config) { return errors.E(fmt.Sprintf("expected %d config settings, got %d", len(config), len(args.Config))) } diff --git a/internal/jujuapi/modelmanager_test.go b/internal/jujuapi/modelmanager_test.go index 4e2c915dd..402489578 100644 --- a/internal/jujuapi/modelmanager_test.go +++ b/internal/jujuapi/modelmanager_test.go @@ -860,19 +860,17 @@ var createModelTests = []struct { cloudTag: "not-a-cloud-tag", credentialTag: "cloudcred-" + jimmtest.TestCloudName + "_bob@canonical.com_cred1", expectError: `"not-a-cloud-tag" is not a valid tag \(bad request\)`, -}, { - about: "no cloud tag", - name: "model-8", - ownerTag: names.NewUserTag("bob@canonical.com").String(), - cloudTag: "", - credentialTag: "cloudcred-" + jimmtest.TestCloudName + "_bob@canonical.com_cred1", - expectError: `no cloud specified for model; please specify one`, }, { about: "no credential tag selects unambigous creds", name: "model-8", ownerTag: names.NewUserTag("bob@canonical.com").String(), cloudTag: names.NewCloudTag(jimmtest.TestCloudName).String(), region: jimmtest.TestCloudRegionName, +}, { + about: "success - without a cloud tag", + name: "model-9", + ownerTag: names.NewUserTag("bob@canonical.com").String(), + credentialTag: "cloudcred-" + jimmtest.TestCloudName + "_bob@canonical.com_cred", }} func (s *modelManagerSuite) TestCreateModel(c *gc.C) {