Skip to content

Commit

Permalink
fix(internal/db/cloud.go): add virtual field to CloudRegion
Browse files Browse the repository at this point in the history
When we add a cloud reports no cloud regions we add a virtual "default" region to keep the
controller lookup logic the same when adding a model. Then only difference is that we do not send
the cloud region name in ModelCreateArgs to the controller.
  • Loading branch information
alesstimec committed Jan 10, 2025
1 parent 8c6df2a commit 5b8b195
Show file tree
Hide file tree
Showing 58 changed files with 215 additions and 70 deletions.
2 changes: 1 addition & 1 deletion cmd/jimmctl/cmd/purge_logs_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

import (
Expand Down
2 changes: 1 addition & 1 deletion internal/auth/oauth2_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package auth_test

Expand Down
2 changes: 1 addition & 1 deletion internal/db/applicationoffer_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
2 changes: 1 addition & 1 deletion internal/db/auditlog_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
5 changes: 3 additions & 2 deletions internal/db/cloud_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 @@ -35,7 +35,8 @@ func (s *dbSuite) TestAddCloud(c *qt.C) {
IdentityEndpoint: "https://identity.example.com",
StorageEndpoint: "https://storage.example.com",
Regions: []dbmodel.CloudRegion{{
Name: "test-cloud-region",
Name: "test-cloud-region",
Virtual: true,
}},
CACertificates: dbmodel.Strings{"CACERT 1", "CACERT 2"},
}
Expand Down
2 changes: 1 addition & 1 deletion internal/db/clouddefaults_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
2 changes: 1 addition & 1 deletion internal/db/controller_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
2 changes: 1 addition & 1 deletion internal/db/db.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

// Package db contains routines to store and retrieve data from a database.
package db
Expand Down
2 changes: 1 addition & 1 deletion internal/db/db_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
2 changes: 1 addition & 1 deletion internal/db/export_test.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
2 changes: 1 addition & 1 deletion internal/db/group_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
2 changes: 1 addition & 1 deletion internal/db/identity_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
2 changes: 1 addition & 1 deletion internal/db/model_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
2 changes: 1 addition & 1 deletion internal/db/resource_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

import (
Expand Down
2 changes: 1 addition & 1 deletion internal/db/role_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
2 changes: 1 addition & 1 deletion internal/db/rootkeys_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
2 changes: 1 addition & 1 deletion internal/db/secrets_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
10 changes: 9 additions & 1 deletion internal/dbmodel/cloud.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package dbmodel

Expand Down Expand Up @@ -181,6 +181,14 @@ type CloudRegion struct {
// Controllers contains any controllers that can provide service for
// this cloud-region.
Controllers []CloudRegionControllerPriority

// Virtual, if true, indicates that a cloud was reported to have
// no cloud regions so we created a virtual "default" regions for it.
// This is necessary because Juju seems inconsistent in how they manage
// clouds with no regions; e.g. maas or k8s clouds may not have regions,
// yet for some cloud juju creates a "default" region, while for others
// it does not.
Virtual bool
}

// ToJujuCloudRegion converts a CloudRegion into a jujuparams.CloudRegion.
Expand Down
2 changes: 1 addition & 1 deletion internal/dbmodel/cloudcredential.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package dbmodel

Expand Down
2 changes: 1 addition & 1 deletion internal/dbmodel/cloudcredential_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
2 changes: 1 addition & 1 deletion internal/dbmodel/controller.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package dbmodel

Expand Down
2 changes: 1 addition & 1 deletion internal/dbmodel/gorm_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
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
-- adds a boolean column to the cloud_regions table.

ALTER TABLE cloud_regions ADD COLUMN virtual BOOLEAN;
2 changes: 1 addition & 1 deletion internal/jimm/applicationoffer.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
2 changes: 1 addition & 1 deletion internal/jimm/cloud.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
2 changes: 1 addition & 1 deletion internal/jimm/cloud_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2024 Canonical.
// Copyright 2025 Canonical.

package jimm_test

Expand Down
1 change: 1 addition & 0 deletions internal/jimm/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ func (j *JIMM) AddController(ctx context.Context, user *openfga.User, ctl *dbmod
dbClouds[i].Regions = []dbmodel.CloudRegion{{
CloudName: cloud.Name,
Name: "default",
Virtual: true,
}}
if cloud.Name == ctl.CloudName {
ctl.CloudRegion = "default"
Expand Down
35 changes: 22 additions & 13 deletions internal/jimm/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,16 +103,17 @@ type modelBuilder struct {

jimm *JIMM

name string
config map[string]interface{}
owner *dbmodel.Identity
credential *dbmodel.CloudCredential
controller *dbmodel.Controller
cloud *dbmodel.Cloud
cloudRegion string
cloudRegionID uint
model *dbmodel.Model
modelInfo *jujuparams.ModelInfo
name string
config map[string]interface{}
owner *dbmodel.Identity
credential *dbmodel.CloudCredential
controller *dbmodel.Controller
cloud *dbmodel.Cloud
cloudRegion string
cloudRegionID uint
cloudRegionVirtual bool
model *dbmodel.Model
modelInfo *jujuparams.ModelInfo
}

// Error returns the error that occurred in the process
Expand All @@ -138,14 +139,21 @@ func (b *modelBuilder) jujuModelCreateArgs() (*jujuparams.ModelCreateArgs, error
return nil, errors.E("credentials not specified")
}

return &jujuparams.ModelCreateArgs{
args := &jujuparams.ModelCreateArgs{
Name: b.name,
OwnerTag: b.owner.Tag().String(),
Config: b.config,
CloudTag: b.cloud.Tag().String(),
CloudRegion: b.cloudRegion,
CloudCredentialTag: b.credential.Tag().String(),
}, nil
}
// if this cloud region is a virtual one (cloud did not report
// any regions and we added a "default" region), we will
// not send a cloud region to the controller.
if b.cloudRegionVirtual {
args.CloudRegion = ""
}
return args, nil
}

// WithOwner returns a builder with the specified owner.
Expand Down Expand Up @@ -268,7 +276,8 @@ func (b *modelBuilder) WithCloudRegion(region string) *modelBuilder {

// and select the first controller in the slice
b.cloudRegion = region
b.cloudRegionID = regionControllers[0].CloudRegionID
b.cloudRegionID = r.ID
b.cloudRegionVirtual = r.Virtual
b.controller = &regionControllers[0].Controller

break
Expand Down
123 changes: 122 additions & 1 deletion internal/jimm/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,106 @@ users:
OwnerTag: names.NewUserTag("[email protected]").String(),
},
expectError: "no cloud specified for model; please specify one",
}, {
name: "CreateModelOnACloudWithNoRegions",
// test-cloud has one virtual cloud region
env: `
clouds:
- name: test-cloud
type: test-provider
regions:
- name: default
virtual: true
users:
- user: [email protected]
access: add-model
cloud-credentials:
- name: test-credential-1
owner: [email protected]
cloud: test-cloud
auth-type: empty
controllers:
- name: controller-1
uuid: 00000000-0000-0000-0000-0000-0000000000001
cloud: test-cloud
region: default
cloud-regions:
- cloud: test-cloud
region: default
priority: 0
- name: controller-2
uuid: 00000000-0000-0000-0000-0000-0000000000002
cloud: test-cloud
region: default
cloud-regions:
- cloud: test-cloud
region: default
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: assertCreateModelArgs(&jujuparams.ModelCreateArgs{
Name: "test-model",
OwnerTag: names.NewUserTag("[email protected]").String(),
CloudTag: names.NewCloudTag("test-cloud").String(),
// we expect cloud region to be empty, because it is a virtual "default" region
CloudRegion: "",
CloudCredentialTag: names.NewCloudCredentialTag("test-cloud/[email protected]/test-credential-1").String(),
}, createModel(`
uuid: 00000001-0000-0000-0000-0000-000000000001
status:
status: started
info: running a test
life: alive
users:
- user: [email protected]
access: admin
- user: bob
access: read
`[1:])),
username: "[email protected]",
jimmAdmin: true,
cloudCredTag: names.NewCloudCredentialTag("test-cloud/[email protected]/test-credential-1"),
args: jujuparams.ModelCreateArgs{
Name: "test-model",
OwnerTag: names.NewUserTag("[email protected]").String(),
CloudTag: names.NewCloudTag("test-cloud").String(),
// Creating a model without specifying the cloud region
CloudRegion: "",
},
expectModel: dbmodel.Model{
Name: "test-model",
UUID: sql.NullString{
String: "00000001-0000-0000-0000-0000-000000000001",
Valid: true,
},
Owner: dbmodel.Identity{
Name: "[email protected]",
},
Controller: dbmodel.Controller{
Name: "controller-2",
UUID: "00000000-0000-0000-0000-0000-0000000000002",
CloudName: "test-cloud",
CloudRegion: "default",
},
CloudRegion: dbmodel.CloudRegion{
Cloud: dbmodel.Cloud{
Name: "test-cloud",
Type: "test-provider",
},
Name: "default",
Virtual: true,
},
CloudCredential: dbmodel.CloudCredential{
Name: "test-credential-1",
AuthType: "empty",
},
Life: state.Alive.String(),
},
}}

func TestAddModel(t *testing.T) {
Expand Down Expand Up @@ -1151,11 +1251,32 @@ func assertConfig(config map[string]interface{}, fnc func(context.Context, *juju
}
for k, v := range args.Config {
if config[k] != v {
return errors.E(fmt.Sprintf("config value mismatch for key %s", k))
return errors.E(fmt.Sprintf("config value mismatch for key %s: %s -> %s", k, config[k], v))
}
}
return fnc(ctx, args, mi)
}
}

func assertCreateModelArgs(expectedArgs *jujuparams.ModelCreateArgs, 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 expectedArgs.Name != args.Name {
return fmt.Errorf("name mismatch: expected %q, got %q", expectedArgs.Name, args.Name)
}
if expectedArgs.OwnerTag != args.OwnerTag {
return fmt.Errorf("owner mismatch: expected %q, got %q", expectedArgs.Name, args.Name)
}
if expectedArgs.CloudTag != args.CloudTag {
return fmt.Errorf("cloud mismatch: expected %q, got %q", expectedArgs.Name, args.Name)
}
if expectedArgs.CloudRegion != args.CloudRegion {
return fmt.Errorf("cloud region mismatch: expected %q, got %q", expectedArgs.Name, args.Name)
}
if expectedArgs.CloudCredentialTag != args.CloudCredentialTag {
return fmt.Errorf("credential mismatch: expected %q, got %q", expectedArgs.Name, args.Name)
}
return fnc(ctx, args, mi)
}

}

Expand Down
2 changes: 1 addition & 1 deletion internal/jimm/service_account.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
Loading

0 comments on commit 5b8b195

Please sign in to comment.