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

feat(modelmanager): listmodels #1493

Merged
merged 17 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from 15 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
3 changes: 3 additions & 0 deletions internal/db/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ func (d *Database) ForEachModel(ctx context.Context, f func(m *dbmodel.Model) er

// GetModelsByUUID retrieves a list of models where the model UUIDs are in
// the provided modelUUIDs slice.
//
// If the UUID cannot be resolved to a model, it is skipped from the result and
// no error is returned.
func (d *Database) GetModelsByUUID(ctx context.Context, modelUUIDs []string) (_ []dbmodel.Model, err error) {
const op = errors.Op("db.GetModelsByUUID")

Expand Down
1 change: 1 addition & 0 deletions internal/db/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ func (s *dbSuite) TestGetModelsByUUID(c *qt.C) {
sort.Slice(models, func(i, j int) bool {
return models[i].UUID.String < models[j].UUID.String
})
c.Assert(models, qt.HasLen, 3)
c.Check(models[0].UUID.String, qt.Equals, "00000002-0000-0000-0000-000000000001")
c.Check(models[0].Controller.Name, qt.Not(qt.Equals), "")
c.Check(models[1].UUID.String, qt.Equals, "00000002-0000-0000-0000-000000000002")
Expand Down
3 changes: 3 additions & 0 deletions internal/jimm/jimm.go
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,9 @@ type API interface {

// ListStorageDetails lists all storage.
ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error)

// ListModels returns all UserModel's on the controller.
ListModels(ctx context.Context) ([]base.UserModel, error)
ale8k marked this conversation as resolved.
Show resolved Hide resolved
}

// forEachController runs a given function on multiple controllers
Expand Down
74 changes: 74 additions & 0 deletions internal/jimm/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import (
"database/sql"
"fmt"
"math/rand"
"slices"
"sort"
"strings"
"sync"
"time"

"github.com/juju/juju/api/base"
jujupermission "github.com/juju/juju/core/permission"
jujuparams "github.com/juju/juju/rpc/params"
"github.com/juju/juju/state"
Expand Down Expand Up @@ -779,10 +781,12 @@ func (j *JIMM) ListModelSummaries(ctx context.Context, user *openfga.User, maski
model *dbmodel.Model
userAccess jujuparams.UserAccessPermission
}{model: m, userAccess: uap})

if _, ok := uniqueControllerMap[m.Controller.UUID]; !ok {
uniqueControllers = append(uniqueControllers, m.Controller)
uniqueControllerMap[m.Controller.UUID] = struct{}{}
}

return nil
})
if err != nil {
Expand Down Expand Up @@ -1347,3 +1351,73 @@ func (j *JIMM) ChangeModelCredential(ctx context.Context, user *openfga.User, mo

return nil
}

// ListModels list the models that the user has access to. It intentionally excludes the
// controller model as this call is used within the context of login and register commands.
func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) {
ale8k marked this conversation as resolved.
Show resolved Hide resolved
const op = errors.Op("jimm.ListModels")
zapctx.Info(ctx, string(op))

// Get uuids of models the user has access to
uuids, err := user.ListModels(ctx, ofganames.ReaderRelation)
if err != nil {
return nil, errors.E(op, err, "failed to list user models")
}

// Get the models from the database
models, err := j.DB().GetModelsByUUID(ctx, uuids)
ale8k marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, errors.E(op, err, "failed to get models by uuid")
}

// Create map for lookup later
modelsMap := make(map[string]dbmodel.Model)
// Find the controllers these models reside on and remove duplicates
var controllers []dbmodel.Controller
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if there's a way we could write this bit with generics or something, because we seem to be looking for unique lists in a few places in jimm code. also not for this pr :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already on it :P

seen := make(map[uint]bool)
for _, model := range models {
ale8k marked this conversation as resolved.
Show resolved Hide resolved
modelsMap[model.UUID.String] = model // Set map for lookup
if seen[model.ControllerID] {
continue
}
seen[model.ControllerID] = true
controllers = append(controllers, model.Controller)
}

// Call controllers for their models. We always call as admin, and we're
// filtering ourselves. We do this rather than send the user to be 100%
// certain that the models do belong to user according to OpenFGA. We could
// in theory rely on Juju correctly returning the models (by owner), but this
// is more reliable.
var userModels []base.UserModel
var mutex sync.Mutex
err = j.forEachController(ctx, controllers, func(_ *dbmodel.Controller, api API) error {
ums, err := api.ListModels(ctx)
alesstimec marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
mutex.Lock()
defer mutex.Unlock()

// Filter the models returned according to the uuids
// returned from OpenFGA for read access.
//
// NOTE: The controller models are skipped because we do not relate users
ale8k marked this conversation as resolved.
Show resolved Hide resolved
// to controller models, we skip the controller models as ListModels is used for
// login and register - the models returned are stored locally and used for reference.
// In the case of JIMM, we do not want to show the controller models.
for _, um := range ums {
// Filter models that match authorised uuids list
if slices.Contains(uuids, um.UUID) {
ale8k marked this conversation as resolved.
Show resolved Hide resolved
um.Owner = modelsMap[um.UUID].OwnerIdentityName
ale8k marked this conversation as resolved.
Show resolved Hide resolved
userModels = append(userModels, um)
}
}
return nil
})
if err != nil {
return nil, errors.E(op, err, "failed to list models")
}

return userModels, nil
}
194 changes: 194 additions & 0 deletions internal/jimm/model_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
qt "github.com/frankban/quicktest"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/juju/juju/api/base"
"github.com/juju/juju/core/life"
"github.com/juju/juju/rpc/params"
jujuparams "github.com/juju/juju/rpc/params"
Expand Down Expand Up @@ -3804,6 +3805,199 @@ controllers:
c.Assert(model.Controller.Name, qt.Equals, "controller-3")
}

const listModelsTestEnv = `clouds:
- name: test-cloud
type: test-provider
regions:
- name: test-cloud-region
cloud-credentials:
- owner: [email protected]
name: cred-1
cloud: test-cloud

controllers:
- name: controller-1
uuid: 00000001-0000-0000-0000-000000000001
cloud: test-cloud
region: test-cloud-region

- name: controller-2
uuid: 00000001-0000-0000-0000-000000000001
cloud: test-cloud
region: test-cloud-region

models:
- name: model-1
uuid: 00000002-0000-0000-0000-000000000001
controller: controller-1
cloud: test-cloud
region: test-cloud-region
cloud-credential: cred-1
owner: [email protected]
life: alive
users:
- user: [email protected]
access: admin
- user: [email protected]
access: admin

- name: model-2
uuid: 00000002-0000-0000-0000-000000000002
controller: controller-1
cloud: test-cloud
region: test-cloud-region
cloud-credential: cred-1
owner: [email protected]
life: alive
users:
- user: [email protected]
access: admin
- user: [email protected]
access: write
sla:
level: unsupported

- name: model-3
uuid: 00000002-0000-0000-0000-000000000003
controller: controller-2
cloud: test-cloud
region: test-cloud-region
cloud-credential: cred-1
owner: [email protected]
life: alive
users:
- user: [email protected]
access: admin
- user: [email protected]
access: read

- name: model-4
uuid: 00000002-0000-0000-0000-000000000004
controller: controller-1
cloud: test-cloud
region: test-cloud-region
cloud-credential: cred-1
owner: [email protected]
life: alive
users:
- user: [email protected]
access: admin

users:
- username: [email protected]
controller-access: superuser
`

var modelListTests = []struct {
name string
env string
username string
expectedUserModels []base.UserModel
expectedError string
listModelsMockByControllerName map[string]func(context.Context) ([]base.UserModel, error)
ale8k marked this conversation as resolved.
Show resolved Hide resolved
}{
{
name: "Bob lists models across controllers 1 and 2",
env: listModelsTestEnv,
username: "[email protected]",
expectedUserModels: []base.UserModel{
{UUID: "00000002-0000-0000-0000-000000000001", Owner: "[email protected]"},
{UUID: "00000002-0000-0000-0000-000000000002", Owner: "[email protected]"},
{UUID: "00000002-0000-0000-0000-000000000003", Owner: "[email protected]"},
},
listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){
"controller-1": func(ctx context.Context) ([]base.UserModel, error) {
return []base.UserModel{
{UUID: "00000002-0000-0000-0000-000000000001"},
{UUID: "00000002-0000-0000-0000-000000000002"},
}, nil
},
"controller-2": func(ctx context.Context) ([]base.UserModel, error) {
return []base.UserModel{
{UUID: "00000002-0000-0000-0000-000000000003"},
}, nil
},
},
},
{
name: "Alice lists models across controllers 1 and 2",
env: listModelsTestEnv,
username: "[email protected]",
expectedUserModels: []base.UserModel{
{UUID: "00000002-0000-0000-0000-000000000001", Owner: "[email protected]"},
{UUID: "00000002-0000-0000-0000-000000000002", Owner: "[email protected]"},
{UUID: "00000002-0000-0000-0000-000000000003", Owner: "[email protected]"},
{UUID: "00000002-0000-0000-0000-000000000004", Owner: "[email protected]"},
},
listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){
"controller-1": func(ctx context.Context) ([]base.UserModel, error) {
return []base.UserModel{
{UUID: "00000002-0000-0000-0000-000000000001"},
{UUID: "00000002-0000-0000-0000-000000000002"},
{UUID: "00000002-0000-0000-0000-000000000004"},
}, nil
},
"controller-2": func(ctx context.Context) ([]base.UserModel, error) {
return []base.UserModel{
{UUID: "00000002-0000-0000-0000-000000000003"},
}, nil
},
},
},
{
name: "Alice lists models across controllers 1 and 2",
env: listModelsTestEnv,
username: "[email protected]",
expectedUserModels: []base.UserModel{},
expectedError: "failed to list models",
listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){
"controller-1": func(ctx context.Context) ([]base.UserModel, error) {
return []base.UserModel{}, errors.E("test error")
},
},
},
}

func TestListModels(t *testing.T) {
c := qt.New(t)

for _, test := range modelListTests {
c.Run(
test.name,
func(c *qt.C) {
j := jimmtest.NewJIMM(c, &jimm.Parameters{
Dialer: jimmtest.DialerMap{
"controller-1": &jimmtest.Dialer{
API: &jimmtest.API{
ListModels_: test.listModelsMockByControllerName["controller-1"],
},
},
"controller-2": &jimmtest.Dialer{
API: &jimmtest.API{
ListModels_: test.listModelsMockByControllerName["controller-2"],
},
},
SimoneDutto marked this conversation as resolved.
Show resolved Hide resolved
},
})

env := jimmtest.ParseEnvironment(c, test.env)
env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, j.OpenFGAClient)

dbUser, err := dbmodel.NewIdentity(test.username)
c.Assert(err, qt.IsNil)
user := openfga.NewUser(dbUser, j.OpenFGAClient)

models, err := j.ListModels(context.Background(), user)
if test.expectedError != "" {
c.Assert(err, qt.ErrorMatches, test.expectedError)
} else {
c.Assert(models, qt.ContentEquals, test.expectedUserModels)
}
},
)
}
}

func newBool(b bool) *bool {
return &b
}
Expand Down
2 changes: 2 additions & 0 deletions internal/jujuapi/controllerroot.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery"
"github.com/juju/juju/api/base"
jujuparams "github.com/juju/juju/rpc/params"
"github.com/juju/names/v5"
"github.com/rogpeppe/fastuuid"
Expand Down Expand Up @@ -84,6 +85,7 @@ type JIMM interface {
UpdateCloud(ctx context.Context, u *openfga.User, ct names.CloudTag, cloud jujuparams.Cloud) error
UpdateCloudCredential(ctx context.Context, u *openfga.User, args jimm.UpdateCloudCredentialArgs) ([]jujuparams.UpdateCredentialModelResult, error)
UserLogin(ctx context.Context, identityName string) (*openfga.User, error)
ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error)
}

// controllerRoot is the root for endpoints served on controller connections.
Expand Down
29 changes: 28 additions & 1 deletion internal/jujuapi/modelmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,34 @@ func (r *controllerRoot) ListModelSummaries(ctx context.Context, _ jujuparams.Mo
// ListModels returns the models that the authenticated user
// has access to. The user parameter is ignored.
func (r *controllerRoot) ListModels(ctx context.Context, _ jujuparams.Entity) (jujuparams.UserModelList, error) {
return r.allModels(ctx)
ale8k marked this conversation as resolved.
Show resolved Hide resolved
const op = errors.Op("jujuapi.ListModels")
zapctx.Info(ctx, string(op))

res := jujuparams.UserModelList{}

models, err := r.jimm.ListModels(ctx, r.user)
if err != nil {
return res, err
}

for _, m := range models {
if !names.IsValidUser(m.Owner) {
zapctx.Error(ctx, fmt.Sprintf("%s is not a valid user", m.Owner))
}
ownerTag := names.NewUserTag(m.Owner)

res.UserModels = append(res.UserModels, jujuparams.UserModel{
Model: jujuparams.Model{
Name: m.Name,
UUID: m.UUID,
Type: string(m.Type),
OwnerTag: ownerTag.String(),
},
LastConnection: m.LastConnection,
})
}

return res, nil
}

// ModelInfo implements the ModelManager facade's ModelInfo method.
Expand Down
Loading
Loading