From 647aa5496d9efa4e557ba037f4967a3bb038c7b2 Mon Sep 17 00:00:00 2001 From: ale8k Date: Wed, 11 Dec 2024 13:39:47 +0000 Subject: [PATCH 1/9] test(modelmanager): k8s tests fix The api was trying to call /version, but our mock didn't implement it. --- internal/jujuapi/modelmanager_test.go | 11 +------- internal/testutils/kubetest/kubetest.go | 35 ++++++++++++++++--------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/internal/jujuapi/modelmanager_test.go b/internal/jujuapi/modelmanager_test.go index c65d09a6a..a0b4ba367 100644 --- a/internal/jujuapi/modelmanager_test.go +++ b/internal/jujuapi/modelmanager_test.go @@ -1374,15 +1374,12 @@ func (s *caasModelManagerSuite) SetUpTest(c *gc.C) { } func (s *caasModelManagerSuite) TestCreateModelKubernetes(c *gc.C) { - // TODO (ashipika): remove skip when the issue is resolved - // Error message: enumerating features supported by environment: querying kubernetes API version: the server could not find the requested resource - c.Skip("k8s_issue") conn := s.open(c, nil, "bob") defer conn.Close() client := modelmanager.NewClient(conn) mi, err := client.CreateModel("k8s-model-1", "bob@canonical.com", "bob-cloud", "", s.cred, nil) - c.Assert(err, gc.Equals, nil) + c.Assert(err, gc.Equals, nil) // ERROR IS HERE c.Assert(mi.Name, gc.Equals, "k8s-model-1") c.Assert(mi.Type, gc.Equals, model.CAAS) @@ -1393,9 +1390,6 @@ func (s *caasModelManagerSuite) TestCreateModelKubernetes(c *gc.C) { } func (s *caasModelManagerSuite) TestListCAASModelSummaries(c *gc.C) { - // TODO (ashipika): remove skip when the issue is resolved - // Error message: enumerating features supported by environment: querying kubernetes API version: the server could not find the requested resource - c.Skip("k8s_issue") conn := s.open(c, nil, "bob") defer conn.Close() @@ -1489,9 +1483,6 @@ func (s *caasModelManagerSuite) TestListCAASModelSummaries(c *gc.C) { } func (s *caasModelManagerSuite) TestListCAASModels(c *gc.C) { - // TODO (ashipika): remove skip when the issue is resolved - // Error message: enumerating features supported by environment: querying kubernetes API version: the server could not find the requested resource - c.Skip("k8s_issue") conn := s.open(c, nil, "bob") defer conn.Close() diff --git a/internal/testutils/kubetest/kubetest.go b/internal/testutils/kubetest/kubetest.go index a57489f39..9e637d0a7 100644 --- a/internal/testutils/kubetest/kubetest.go +++ b/internal/testutils/kubetest/kubetest.go @@ -16,25 +16,34 @@ const ( Password = "test-kubernetes-password" ) -// NewFakeKubernetes creates a minimal kubernetes API server which -// response to just the API calls required by the tests. func NewFakeKubernetes(c *gc.C) *httptest.Server { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if req.URL.Path != "/api/v1/namespaces" { - w.WriteHeader(http.StatusNotFound) - return - } - if req.Method != "POST" { - w.WriteHeader(http.StatusMethodNotAllowed) - return - } if username, password, ok := req.BasicAuth(); !ok || username != Username || password != Password { w.WriteHeader(http.StatusUnauthorized) return } - w.Header().Set("Content-Type", req.Header.Get("Content-Type")) - _, err := io.Copy(w, req.Body) - c.Assert(err, gc.IsNil) + + switch req.URL.Path { + case "/version": + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"major":"1","minor":"21","gitVersion":"v1.21.0"}`)) + case "/api/v1/namespaces": + if req.Method != "POST" { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", req.Header.Get("Content-Type")) + _, err := io.Copy(w, req.Body) + c.Assert(err, gc.IsNil) + case "/api": + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"versions":["v1"]}`)) + case "/apis": + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"groups":[{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}]}]}`)) + default: + w.WriteHeader(http.StatusNotFound) + } })) return srv } From 3d8aa1bc13e8f89d99c3f77a7efcd5b46b640559 Mon Sep 17 00:00:00 2001 From: ale8k Date: Thu, 12 Dec 2024 13:54:44 +0000 Subject: [PATCH 2/9] feat(listmodels): enable listmodels within the jujuclient pkg --- internal/jujuclient/modelmanager.go | 13 ++++++++ internal/jujuclient/modelmanager_test.go | 39 ++++++++++++++++++++++++ internal/testutils/jimmtest/api.go | 8 +++++ 3 files changed, 60 insertions(+) diff --git a/internal/jujuclient/modelmanager.go b/internal/jujuclient/modelmanager.go index b19808df6..b97e9c733 100644 --- a/internal/jujuclient/modelmanager.go +++ b/internal/jujuclient/modelmanager.go @@ -7,6 +7,8 @@ import ( "time" jujuerrors "github.com/juju/errors" + "github.com/juju/juju/api/base" + "github.com/juju/juju/api/client/modelmanager" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" @@ -325,3 +327,14 @@ func (c Connection) ChangeModelCredential(ctx context.Context, model names.Model } return out.OneError() } + +// ListModels returns the models that the specified user +// has access to in the current server. Only that controller owner +// can list models for any user (at this stage). Other users +// can only ask about their own models. +// +// In our wrapper, we ask for the owner. So expect ALL models from +// the controller. +func (c Connection) ListModels(ctx context.Context) ([]base.UserModel, error) { + return modelmanager.NewClient(&c).ListModels("admin") +} diff --git a/internal/jujuclient/modelmanager_test.go b/internal/jujuclient/modelmanager_test.go index 3d7c7d964..b3a379b94 100644 --- a/internal/jujuclient/modelmanager_test.go +++ b/internal/jujuclient/modelmanager_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/juju/juju/api/base" "github.com/juju/juju/core/life" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -288,3 +289,41 @@ func (s *modelmanagerSuite) TestChangeModelCredential(c *gc.C) { err = s.API.ChangeModelCredential(ctx, names.NewModelTag(info.UUID), ct) c.Assert(err, gc.Equals, nil) } + +func (s *modelmanagerSuite) TestListModels(c *gc.C) { + ctx := context.Background() + + var info jujuparams.ModelInfo + err := s.API.CreateModel(ctx, &jujuparams.ModelCreateArgs{ + Name: "test-model", + OwnerTag: names.NewUserTag("test-user@canonical.com").String(), + }, &info) + c.Assert(err, gc.Equals, nil) + + models, err := s.API.ListModels(ctx) + c.Assert(err, gc.IsNil) + c.Assert( + models, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes( + &time.Time{}, + ), + ), + []base.UserModel{ + { + Name: "controller", + UUID: "deadbeef-0bad-400d-8000-4b1d0d06f00d", + Type: "iaas", + Owner: "admin", + LastConnection: nil, + }, + { + Name: "test-model", + UUID: info.UUID, + Type: "iaas", + Owner: "test-user@canonical.com", + LastConnection: nil, + }, + }, + ) +} diff --git a/internal/testutils/jimmtest/api.go b/internal/testutils/jimmtest/api.go index d2851027c..a88ef3e50 100644 --- a/internal/testutils/jimmtest/api.go +++ b/internal/testutils/jimmtest/api.go @@ -165,6 +165,7 @@ type API struct { ListFilesystems_ func(ctx context.Context, machines []string) ([]jujuparams.FilesystemDetailsListResult, error) ListVolumes_ func(ctx context.Context, machines []string) ([]jujuparams.VolumeDetailsListResult, error) ListStorageDetails_ func(ctx context.Context) ([]jujuparams.StorageDetails, error) + ListModels_ func(ctx context.Context) ([]base.UserModel, error) } func (a *API) AddCloud(ctx context.Context, tag names.CloudTag, cld jujuparams.Cloud, force bool) error { @@ -466,4 +467,11 @@ func (a *API) ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetai return a.ListStorageDetails_(ctx) } +func (a *API) ListModels(ctx context.Context) ([]base.UserModel, error) { + if a.ListModels_ == nil { + return nil, errors.E(errors.CodeNotImplemented) + } + return a.ListModels_(ctx) +} + var _ jimm.API = &API{} From abc0676373ba907b9afca0584eed9fbc8e6bf20e Mon Sep 17 00:00:00 2001 From: ale8k Date: Thu, 12 Dec 2024 13:55:39 +0000 Subject: [PATCH 3/9] fix(listmodels): enable list models via controller We now list models via contacting controllers directly. --- internal/jimm/jimm.go | 9 ++ internal/jimm/model.go | 66 ++++++++ internal/jimm/model_test.go | 194 +++++++++++++++++++++++ internal/jujuapi/controllerroot.go | 2 + internal/jujuapi/modelmanager.go | 29 +++- internal/jujuapi/modelmanager_test.go | 189 +++++++++++----------- internal/testutils/jimmtest/jimm_mock.go | 8 + internal/testutils/kubetest/kubetest.go | 9 +- 8 files changed, 404 insertions(+), 102 deletions(-) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 07a4c12e3..99fab9b4c 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -494,6 +494,15 @@ type API interface { // ListStorageDetails lists all storage. ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error) + + // ListModels returns the models that the specified user + // has access to in the current server. Only that controller owner + // can list models for any user (at this stage). Other users + // can only ask about their own models. + // + // In our wrapper, we ask for the owner. So expect ALL models from + // the controller. + ListModels(ctx context.Context) ([]base.UserModel, error) } // forEachController runs a given function on multiple controllers diff --git a/internal/jimm/model.go b/internal/jimm/model.go index e3c56e08d..fee0cd12d 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -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" @@ -779,10 +781,12 @@ func (j *JIMM) ModelSummaries(ctx context.Context, user *openfga.User, maskingCo 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 { @@ -829,6 +833,68 @@ func (j *JIMM) ModelSummaries(ctx context.Context, user *openfga.User, maskingCo }, nil } +func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) { + const op = errors.Op("jimm.ListModels") + zapctx.Info(ctx, string(op)) + + // Get models uuids user has access to + uuids, err := user.ListModels(ctx, ofganames.ReaderRelation) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + // Get the models themselves + models, err := j.DB().GetModelsByUUID(ctx, uuids) + if err != nil { + return nil, errors.E(op, err, "failed to get models by uuid") + } + + // Find the controllers these models reside on and remove duplicates + var controllers []dbmodel.Controller + seen := make(map[uint]bool) + for _, model := range models { + 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) + 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: We skip 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 _, m := range ums { + if slices.Contains(uuids, m.UUID) { + userModels = append(userModels, m) + } + } + return nil + }) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + return userModels, nil +} + // mergeModelInfo replaces fields on the juju model info object with // information from JIMM where JIMM specific information should be used. func (j *JIMM) mergeModelInfo(ctx context.Context, user *openfga.User, modelInfo *jujuparams.ModelInfo, jimmModel dbmodel.Model) (*jujuparams.ModelInfo, error) { diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index cd33831bd..35fcdfac2 100644 --- a/internal/jimm/model_test.go +++ b/internal/jimm/model_test.go @@ -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" @@ -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: alice@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + +users: +- username: alice@canonical.com + 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) +}{ + { + name: "Bob lists models across controllers 1 and 2", + env: listModelsTestEnv, + username: "bob@canonical.com", + expectedUserModels: []base.UserModel{ + {UUID: "00000002-0000-0000-0000-000000000001"}, + {UUID: "00000002-0000-0000-0000-000000000002"}, + {UUID: "00000002-0000-0000-0000-000000000003"}, + }, + 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: "alice@canonical.com", + expectedUserModels: []base.UserModel{ + {UUID: "00000002-0000-0000-0000-000000000001"}, + {UUID: "00000002-0000-0000-0000-000000000002"}, + {UUID: "00000002-0000-0000-0000-000000000003"}, + {UUID: "00000002-0000-0000-0000-000000000004"}, + }, + 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: "alice@canonical.com", + 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"], + }, + }, + }, + }) + + 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 } diff --git a/internal/jujuapi/controllerroot.go b/internal/jujuapi/controllerroot.go index acaf97daf..c0fe44465 100644 --- a/internal/jujuapi/controllerroot.go +++ b/internal/jujuapi/controllerroot.go @@ -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" @@ -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. diff --git a/internal/jujuapi/modelmanager.go b/internal/jujuapi/modelmanager.go index 65b0ea68e..91f5d9cd8 100644 --- a/internal/jujuapi/modelmanager.go +++ b/internal/jujuapi/modelmanager.go @@ -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) + 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. diff --git a/internal/jujuapi/modelmanager_test.go b/internal/jujuapi/modelmanager_test.go index 989e7db7b..dfecdcdc8 100644 --- a/internal/jujuapi/modelmanager_test.go +++ b/internal/jujuapi/modelmanager_test.go @@ -221,23 +221,35 @@ func (s *modelManagerSuite) TestListModelSummariesWithoutControllerUUIDMasking(c } func (s *modelManagerSuite) TestListModels(c *gc.C) { - conn := s.open(c, nil, "bob") + conn := s.open(c, nil, "charlie@canonical.com") defer conn.Close() client := modelmanager.NewClient(conn) - models, err := client.ListModels("bob") + models, err := client.ListModels("charlie@canonical.com") c.Assert(err, gc.Equals, nil) - c.Assert(models, jc.SameContents, []base.UserModel{{ - Name: "model-1", - UUID: s.Model.UUID.String, - Owner: "bob@canonical.com", - Type: "iaas", - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Owner: "charlie@canonical.com", - Type: "iaas", - }}) + c.Assert( + models, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes(&time.Time{}), + cmpopts.SortSlices(func(a, b base.UserModelSummary) bool { + return a.Name < b.Name + }), + ), + []base.UserModel{ + { + Name: "model-2", + UUID: s.Model2.UUID.String, + Owner: "charlie@canonical.com", + Type: "iaas", + }, { + Name: "model-3", + UUID: s.Model3.UUID.String, + Owner: "charlie@canonical.com", + Type: "iaas", + }, + }, + ) + } func (s *modelManagerSuite) TestModelInfo(c *gc.C) { @@ -1364,7 +1376,7 @@ func (s *caasModelManagerSuite) TestCreateModelKubernetes(c *gc.C) { client := modelmanager.NewClient(conn) mi, err := client.CreateModel("k8s-model-1", "bob@canonical.com", "bob-cloud", "", s.cred, nil) - c.Assert(err, gc.Equals, nil) // ERROR IS HERE + c.Assert(err, gc.Equals, nil) c.Assert(mi.Name, gc.Equals, "k8s-model-1") c.Assert(mi.Type, gc.Equals, model.CAAS) @@ -1384,87 +1396,56 @@ func (s *caasModelManagerSuite) TestListCAASModelSummaries(c *gc.C) { models, err := client.ListModelSummaries("bob", false) c.Assert(err, gc.Equals, nil) - c.Assert(models, jimmtest.CmpEquals( - cmpopts.IgnoreTypes(&time.Time{}), - cmpopts.SortSlices(func(a, b base.UserModelSummary) bool { - return a.Name < b.Name - }), - ), []base.UserModelSummary{{ + + var caasMS *base.UserModelSummary + for _, m := range models { + if m.Name == "k8s-model-1" { + caasMS = &m + } + } + if caasMS == nil { + c.Fail() + } + expectedCaas := &base.UserModelSummary{ Name: "k8s-model-1", UUID: mi.UUID, + Type: "caas", ControllerUUID: jimmtest.ControllerUUID, + IsController: false, ProviderType: "kubernetes", DefaultSeries: "jammy", Cloud: "bob-cloud", CloudRegion: "default", - CloudCredential: s.cred.Id(), - Owner: "bob@canonical.com", - Life: life.Value(state.Alive.String()), - Status: base.Status{ - Status: status.Available, - Data: map[string]interface{}{}, - }, - ModelUserAccess: "admin", - Counts: []base.EntityCount{{ - Entity: "machines", - Count: 0, - }, { - Entity: "cores", - Count: 0, - }, { - Entity: "units", - Count: 0, - }}, - AgentVersion: &jujuversion.Current, - Type: "caas", - SLA: &base.SLASummary{ - Level: "unsupported", - }, - }, { - Name: "model-1", - UUID: s.Model.UUID.String, - Type: "iaas", - ControllerUUID: jimmtest.ControllerUUID, - ProviderType: jimmtest.TestProviderType, - DefaultSeries: "jammy", - Cloud: jimmtest.TestCloudName, - CloudRegion: jimmtest.TestCloudRegionName, - CloudCredential: jimmtest.TestCloudName + "/bob@canonical.com/cred", + CloudCredential: "bob-cloud/bob@canonical.com/k8s", Owner: "bob@canonical.com", - Life: life.Value(state.Alive.String()), + Life: "alive", Status: base.Status{ - Status: status.Available, + Status: "available", + Info: "", Data: map[string]interface{}{}, + Since: nil, }, - ModelUserAccess: "admin", - Counts: []base.EntityCount{{Entity: "machines"}, {Entity: "cores"}, {Entity: "units"}}, - AgentVersion: &jujuversion.Current, + ModelUserAccess: "admin", + UserLastConnection: nil, + Counts: []base.EntityCount{}, + AgentVersion: &jujuversion.Current, + Error: nil, + Migration: nil, SLA: &base.SLASummary{ - Level: "unsupported", - }, - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Type: "iaas", - ControllerUUID: jimmtest.ControllerUUID, - ProviderType: jimmtest.TestProviderType, - DefaultSeries: "jammy", - Cloud: jimmtest.TestCloudName, - CloudRegion: jimmtest.TestCloudRegionName, - CloudCredential: jimmtest.TestCloudName + "/charlie@canonical.com/cred", - Owner: "charlie@canonical.com", - Life: life.Value(state.Alive.String()), - Status: base.Status{ - Status: status.Available, - Data: map[string]interface{}{}, - }, - ModelUserAccess: "read", - Counts: []base.EntityCount{{Entity: "machines"}, {Entity: "cores"}, {Entity: "units"}}, - AgentVersion: &jujuversion.Current, - SLA: &base.SLASummary{ - Level: "unsupported", + Level: "", + Owner: "bob@canonical.com", }, - }}) + } + c.Assert( + caasMS, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes( + &time.Time{}, + &base.MigrationSummary{}, + ), + ), + expectedCaas, + ) } func (s *caasModelManagerSuite) TestListCAASModels(c *gc.C) { @@ -1477,22 +1458,34 @@ func (s *caasModelManagerSuite) TestListCAASModels(c *gc.C) { models, err := client.ListModels("bob") c.Assert(err, gc.Equals, nil) - c.Assert(models, jc.SameContents, []base.UserModel{{ - Name: "k8s-model-1", - UUID: mi.UUID, - Owner: "bob@canonical.com", - Type: "caas", - }, { - Name: "model-1", - UUID: s.Model.UUID.String, - Owner: "bob@canonical.com", - Type: "iaas", - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Owner: "charlie@canonical.com", - Type: "iaas", - }}) + sort.Slice(models, func(i, j int) bool { return i < j }) + + c.Assert( + models, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes( + &time.Time{}, + ), + ), + []base.UserModel{ + { + Name: "k8s-model-1", + UUID: mi.UUID, + Owner: "bob@canonical.com", + Type: "caas", + }, { + Name: "model-1", + UUID: s.Model.UUID.String, + Owner: "bob@canonical.com", + Type: "iaas", + }, { + Name: "model-3", + UUID: s.Model3.UUID.String, + Owner: "charlie@canonical.com", + Type: "iaas", + }, + }, + ) } func assertModelInfo(c *gc.C, obtained, expected []jujuparams.ModelInfoResult) { diff --git a/internal/testutils/jimmtest/jimm_mock.go b/internal/testutils/jimmtest/jimm_mock.go index e59968d3d..7d264085f 100644 --- a/internal/testutils/jimmtest/jimm_mock.go +++ b/internal/testutils/jimmtest/jimm_mock.go @@ -8,6 +8,7 @@ import ( "github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery" "github.com/google/uuid" + "github.com/juju/juju/api/base" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" @@ -87,6 +88,7 @@ type JIMM struct { UpdateCloud_ func(ctx context.Context, u *openfga.User, ct names.CloudTag, cloud jujuparams.Cloud) error UpdateCloudCredential_ func(ctx context.Context, u *openfga.User, args jimm.UpdateCloudCredentialArgs) ([]jujuparams.UpdateCredentialModelResult, error) UserLogin_ func(ctx context.Context, identityName string) (*openfga.User, error) + ListModels_ func(ctx context.Context, user *openfga.User) ([]base.UserModel, error) } func (j *JIMM) AddAuditLogEntry(ale *dbmodel.AuditLogEntry) { @@ -426,3 +428,9 @@ func (j *JIMM) UserLogin(ctx context.Context, identityName string) (*openfga.Use } return j.UserLogin_(ctx, identityName) } +func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) { + if j.ListModels_ == nil { + return nil, errors.E(errors.CodeNotImplemented) + } + return j.ListModels_(ctx, user) +} diff --git a/internal/testutils/kubetest/kubetest.go b/internal/testutils/kubetest/kubetest.go index 9e637d0a7..c6901b17e 100644 --- a/internal/testutils/kubetest/kubetest.go +++ b/internal/testutils/kubetest/kubetest.go @@ -26,7 +26,8 @@ func NewFakeKubernetes(c *gc.C) *httptest.Server { switch req.URL.Path { case "/version": w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"major":"1","minor":"21","gitVersion":"v1.21.0"}`)) + _, err := w.Write([]byte(`{"major":"1","minor":"21","gitVersion":"v1.21.0"}`)) + c.Assert(err, gc.IsNil) case "/api/v1/namespaces": if req.Method != "POST" { w.WriteHeader(http.StatusMethodNotAllowed) @@ -37,10 +38,12 @@ func NewFakeKubernetes(c *gc.C) *httptest.Server { c.Assert(err, gc.IsNil) case "/api": w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"versions":["v1"]}`)) + _, err := w.Write([]byte(`{"versions":["v1"]}`)) + c.Assert(err, gc.IsNil) case "/apis": w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"groups":[{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}]}]}`)) + _, err := w.Write([]byte(`{"groups":[{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}]}]}`)) + c.Assert(err, gc.IsNil) default: w.WriteHeader(http.StatusNotFound) } From ddd07f30ab136e56da9fef3ed2188b8ddd2b7a96 Mon Sep 17 00:00:00 2001 From: ale8k Date: Thu, 12 Dec 2024 13:59:55 +0000 Subject: [PATCH 4/9] docs(listmodels): update jimm pkg listmodels godoc --- internal/jimm/model.go | 126 +++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 62 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index f79ccb34b..ab7d6381c 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -833,68 +833,6 @@ func (j *JIMM) ListModelSummaries(ctx context.Context, user *openfga.User, maski }, nil } -func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) { - const op = errors.Op("jimm.ListModels") - zapctx.Info(ctx, string(op)) - - // Get models uuids user has access to - uuids, err := user.ListModels(ctx, ofganames.ReaderRelation) - if err != nil { - return nil, errors.E(op, err, "failed to list models") - } - - // Get the models themselves - models, err := j.DB().GetModelsByUUID(ctx, uuids) - if err != nil { - return nil, errors.E(op, err, "failed to get models by uuid") - } - - // Find the controllers these models reside on and remove duplicates - var controllers []dbmodel.Controller - seen := make(map[uint]bool) - for _, model := range models { - 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) - 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: We skip 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 _, m := range ums { - if slices.Contains(uuids, m.UUID) { - userModels = append(userModels, m) - } - } - return nil - }) - if err != nil { - return nil, errors.E(op, err, "failed to list models") - } - - return userModels, nil -} - // mergeModelInfo replaces fields on the juju model info object with // information from JIMM where JIMM specific information should be used. func (j *JIMM) mergeModelInfo(ctx context.Context, user *openfga.User, modelInfo *jujuparams.ModelInfo, jimmModel dbmodel.Model) (*jujuparams.ModelInfo, error) { @@ -1413,3 +1351,67 @@ 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) { + const op = errors.Op("jimm.ListModels") + zapctx.Info(ctx, string(op)) + + // Get models uuids user has access to + uuids, err := user.ListModels(ctx, ofganames.ReaderRelation) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + // Get the models themselves + models, err := j.DB().GetModelsByUUID(ctx, uuids) + if err != nil { + return nil, errors.E(op, err, "failed to get models by uuid") + } + + // Find the controllers these models reside on and remove duplicates + var controllers []dbmodel.Controller + seen := make(map[uint]bool) + for _, model := range models { + 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) + 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: We skip 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 _, m := range ums { + if slices.Contains(uuids, m.UUID) { + userModels = append(userModels, m) + } + } + return nil + }) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + return userModels, nil +} From ade1505d4bee0585d7053b443b4b194073421611 Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 08:53:53 +0000 Subject: [PATCH 5/9] refactor(pr): pr comments --- internal/jimm/jimm.go | 2 +- internal/jimm/model.go | 6 +++--- internal/jujuclient/modelmanager.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 99fab9b4c..7690d78c8 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -500,7 +500,7 @@ type API interface { // can list models for any user (at this stage). Other users // can only ask about their own models. // - // In our wrapper, we ask for the owner. So expect ALL models from + // In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. ListModels(ctx context.Context) ([]base.UserModel, error) } diff --git a/internal/jimm/model.go b/internal/jimm/model.go index ab7d6381c..1b43da5a7 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1358,13 +1358,13 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM const op = errors.Op("jimm.ListModels") zapctx.Info(ctx, string(op)) - // Get models uuids user has access to + // 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 models") + return nil, errors.E(op, err, "failed to list user models") } - // Get the models themselves + // Get the models from the database models, err := j.DB().GetModelsByUUID(ctx, uuids) if err != nil { return nil, errors.E(op, err, "failed to get models by uuid") diff --git a/internal/jujuclient/modelmanager.go b/internal/jujuclient/modelmanager.go index b97e9c733..3b4cbee75 100644 --- a/internal/jujuclient/modelmanager.go +++ b/internal/jujuclient/modelmanager.go @@ -333,7 +333,7 @@ func (c Connection) ChangeModelCredential(ctx context.Context, model names.Model // can list models for any user (at this stage). Other users // can only ask about their own models. // -// In our wrapper, we ask for the owner. So expect ALL models from +// In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. func (c Connection) ListModels(ctx context.Context) ([]base.UserModel, error) { return modelmanager.NewClient(&c).ListModels("admin") From 0141ef3fed599954f454317a0701c871123630bb Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:21:12 +0000 Subject: [PATCH 6/9] chore: pr comments --- internal/db/model.go | 3 +++ internal/db/model_test.go | 1 + internal/jimm/jimm.go | 6 ++---- internal/jimm/model.go | 15 ++++++++++++--- internal/jimm/model_test.go | 14 +++++++------- internal/jujuclient/modelmanager.go | 6 ++---- 6 files changed, 27 insertions(+), 18 deletions(-) diff --git a/internal/db/model.go b/internal/db/model.go index f998ccb48..b78fda687 100644 --- a/internal/db/model.go +++ b/internal/db/model.go @@ -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") diff --git a/internal/db/model_test.go b/internal/db/model_test.go index f80816bb4..4b3a1f8b9 100644 --- a/internal/db/model_test.go +++ b/internal/db/model_test.go @@ -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") diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 7690d78c8..75cfa2abf 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -495,10 +495,8 @@ type API interface { // ListStorageDetails lists all storage. ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error) - // ListModels returns the models that the specified user - // has access to in the current server. Only that controller owner - // can list models for any user (at this stage). Other users - // can only ask about their own models. + // ListModels returns UserModel's for the user that is logged in. If the user logged + // in is "admin" they may specify another user's models. // // In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 1b43da5a7..21cb7204c 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1402,9 +1402,18 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // NOTE: We skip 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 _, m := range ums { - if slices.Contains(uuids, m.UUID) { - userModels = append(userModels, m) + for _, um := range ums { + // Filter models that match authorised uuids list + if slices.Contains(uuids, um.UUID) { + // Find that model in the db models + index := slices.IndexFunc(models, func(m dbmodel.Model) bool { + return m.UUID.String == um.UUID + }) + if index != -1 { + // Override owner and append to result + um.Owner = models[index].OwnerIdentityName + userModels = append(userModels, um) + } } } return nil diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index 33f2fe2f3..4b70e4855 100644 --- a/internal/jimm/model_test.go +++ b/internal/jimm/model_test.go @@ -3901,9 +3901,9 @@ var modelListTests = []struct { env: listModelsTestEnv, username: "bob@canonical.com", expectedUserModels: []base.UserModel{ - {UUID: "00000002-0000-0000-0000-000000000001"}, - {UUID: "00000002-0000-0000-0000-000000000002"}, - {UUID: "00000002-0000-0000-0000-000000000003"}, + {UUID: "00000002-0000-0000-0000-000000000001", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000002", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000003", Owner: "alice@canonical.com"}, }, listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){ "controller-1": func(ctx context.Context) ([]base.UserModel, error) { @@ -3924,10 +3924,10 @@ var modelListTests = []struct { env: listModelsTestEnv, username: "alice@canonical.com", expectedUserModels: []base.UserModel{ - {UUID: "00000002-0000-0000-0000-000000000001"}, - {UUID: "00000002-0000-0000-0000-000000000002"}, - {UUID: "00000002-0000-0000-0000-000000000003"}, - {UUID: "00000002-0000-0000-0000-000000000004"}, + {UUID: "00000002-0000-0000-0000-000000000001", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000002", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000003", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000004", Owner: "alice@canonical.com"}, }, listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){ "controller-1": func(ctx context.Context) ([]base.UserModel, error) { diff --git a/internal/jujuclient/modelmanager.go b/internal/jujuclient/modelmanager.go index 3b4cbee75..26acf5cde 100644 --- a/internal/jujuclient/modelmanager.go +++ b/internal/jujuclient/modelmanager.go @@ -328,10 +328,8 @@ func (c Connection) ChangeModelCredential(ctx context.Context, model names.Model return out.OneError() } -// ListModels returns the models that the specified user -// has access to in the current server. Only that controller owner -// can list models for any user (at this stage). Other users -// can only ask about their own models. +// ListModels returns UserModel's for the user that is logged in. If the user logged +// in is "admin" they may specify another user's models. // // In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. From 2ad39e0894a32230fca4c22147fdf2b8357eb78a Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:28:20 +0000 Subject: [PATCH 7/9] chore: pr comments --- internal/jimm/jimm.go | 6 +----- internal/jimm/model.go | 7 ++++--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 75cfa2abf..fdadd7977 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -495,11 +495,7 @@ type API interface { // ListStorageDetails lists all storage. ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error) - // ListModels returns UserModel's for the user that is logged in. If the user logged - // in is "admin" they may specify another user's models. - // - // In our wrapper, we ask as the controller admin. So expect ALL models from - // the controller. + // ListModels returns all UserModel's on the controller. ListModels(ctx context.Context) ([]base.UserModel, error) } diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 21cb7204c..e706c6f99 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1399,9 +1399,10 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // Filter the models returned according to the uuids // returned from OpenFGA for read access. // - // NOTE: We skip 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. + // NOTE: The controller models are skipped because we do not relate users + // 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) { From 69d2604b8d8f718567c8844b9d9a622df938fdbb Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:41:58 +0000 Subject: [PATCH 8/9] chore: pr comments --- internal/jimm/model.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index e706c6f99..04c9a7dba 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1370,6 +1370,12 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM return nil, errors.E(op, err, "failed to get models by uuid") } + // Create map for lookup later + modelsMap := make(map[string]dbmodel.Model) + for _, m := range models { + modelsMap[m.UUID.String] = m + } + // Find the controllers these models reside on and remove duplicates var controllers []dbmodel.Controller seen := make(map[uint]bool) @@ -1406,15 +1412,8 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM for _, um := range ums { // Filter models that match authorised uuids list if slices.Contains(uuids, um.UUID) { - // Find that model in the db models - index := slices.IndexFunc(models, func(m dbmodel.Model) bool { - return m.UUID.String == um.UUID - }) - if index != -1 { - // Override owner and append to result - um.Owner = models[index].OwnerIdentityName - userModels = append(userModels, um) - } + um.Owner = modelsMap[um.UUID].OwnerIdentityName + userModels = append(userModels, um) } } return nil From 5f82bc34020829dd99df51be0ae88081ad2b2d7a Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 12:11:18 +0000 Subject: [PATCH 9/9] fix(allmodels): jIMM does not need to support AllModels Removes the support for AllModels() as it is only used in listing,showing,killing or destroying controllers. If in the future we wish to support destroy-controller then we can implement it properly then. --- internal/jujuapi/controller.go | 6 ++++-- internal/jujuapi/controller_test.go | 21 ++++----------------- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/internal/jujuapi/controller.go b/internal/jujuapi/controller.go index d53ed9ad5..b7c904592 100644 --- a/internal/jujuapi/controller.go +++ b/internal/jujuapi/controller.go @@ -169,9 +169,11 @@ func (r *controllerRoot) WatchAllModelSummaries(ctx context.Context) (jujuparams }, nil } -// AllModels implments the AllModels command on the Controller facade. +// AllModels is used for list-controllers, show-controller, kill-controller and destroy-controller, +// we currently do not support this via JIMM. If in the future we do, this method will need implementing. func (r *controllerRoot) AllModels(ctx context.Context) (jujuparams.UserModelList, error) { - return r.allModels(ctx) + const op = errors.Op("jujuapi.AllModels") + return jujuparams.UserModelList{}, errors.E(op, errors.CodeNotSupported) } // allModels returns all the models the logged in user has access to. diff --git a/internal/jujuapi/controller_test.go b/internal/jujuapi/controller_test.go index 65e6b0a6a..53bd19556 100644 --- a/internal/jujuapi/controller_test.go +++ b/internal/jujuapi/controller_test.go @@ -76,25 +76,12 @@ func (s *controllerSuite) TestMongoVersion(c *gc.C) { } func (s *controllerSuite) TestAllModels(c *gc.C) { - conn := s.open(c, nil, "bob") + conn := s.open(c, nil, "alice") defer conn.Close() client := controllerapi.NewClient(conn) - - models, err := client.AllModels() - c.Assert(err, gc.Equals, nil) - c.Assert(models, jc.SameContents, []base.UserModel{{ - Name: "model-1", - UUID: s.Model.UUID.String, - Owner: "bob@canonical.com", - LastConnection: nil, - Type: "iaas", - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Owner: "charlie@canonical.com", - LastConnection: nil, - Type: "iaas", - }}) + _, err := client.AllModels() + c.Assert(err, gc.ErrorMatches, `not supported \(not supported\)`) + c.Assert(jujuparams.IsCodeNotSupported(err), gc.Equals, true) } func (s *controllerSuite) TestModelStatus(c *gc.C) {