From edc69525198f015b0891850ca7e283ad6d580d25 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Mon, 21 Aug 2023 21:23:13 +0200 Subject: [PATCH] Revert "backports changes back to feature-rebac (#1003)" This reverts commit 19d3e80af64db8baaf8226233eef8735d2ea4c09. --- internal/jimm/model_status_parser.go | 64 +++++++++++------- internal/jimm/model_status_parser_test.go | 79 +++++++++++------------ internal/jujuapi/jimm.go | 9 +-- 3 files changed, 80 insertions(+), 72 deletions(-) diff --git a/internal/jimm/model_status_parser.go b/internal/jimm/model_status_parser.go index de9eaeb5a..b1902a7d3 100644 --- a/internal/jimm/model_status_parser.go +++ b/internal/jimm/model_status_parser.go @@ -2,7 +2,9 @@ package jimm import ( "context" + "database/sql" "encoding/json" + "strings" "github.com/canonical/jimm/api/params" "github.com/canonical/jimm/internal/dbmodel" @@ -14,17 +16,16 @@ import ( "github.com/juju/juju/cmd/juju/status" "github.com/juju/juju/cmd/juju/storage" rpcparams "github.com/juju/juju/rpc/params" - "github.com/juju/names/v4" "github.com/juju/zaputil/zapctx" ) -// QueryModels queries every specified model in modelUUIDs. +// QueryModels queries every model available to a given user. // // The jqQuery must be a valid jq query and can return every result, even iterative listings. // If a result is erroneous, for example, bad data type parsing, the resulting struct field // Errors will contain a map from model UUID -> []error. Otherwise, the Results field // will contain model UUID -> []Jq result. -func (j *JIMM) QueryModelsJq(ctx context.Context, models []dbmodel.Model, jqQuery string) (params.CrossModelQueryResponse, error) { +func (j *JIMM) QueryModelsJq(ctx context.Context, modelUUIDs []string, jqQuery string) (params.CrossModelQueryResponse, error) { op := errors.Op("QueryModels") results := params.CrossModelQueryResponse{ Results: make(map[string][]any), @@ -33,19 +34,25 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, models []dbmodel.Model, jqQuer query, err := gojq.Parse(jqQuery) if err != nil { - return results, errors.E(op, "failed to parse jq query", err) + return results, errors.E(op, err) + } + + // We remove "model:" from the UUIDs, unfortunately that's what OpenFGA returns now after + // recent versions. + for i := range modelUUIDs { + modelUUIDs[i] = strings.Split(modelUUIDs[i], ":")[1] } // Set up a formatterParamsRetriever to handle the heavy lifting // of each facade call and type conversion. retriever := newFormatterParamsRetriever(j) - for _, model := range models { - modelUUID := model.UUID.String - params, err := retriever.GetParams(ctx, model) + for _, id := range modelUUIDs { + + params, err := retriever.GetParams(ctx, id) if err != nil { - zapctx.Error(ctx, "failed to get status formatter params", zap.String("model-uuid", modelUUID)) - results.Errors[modelUUID] = append(results.Errors[modelUUID], err.Error()) + zapctx.Error(ctx, "failed to get status formatter params", zap.String("model-uuid", id)) + results.Errors[id] = append(results.Errors[id], err.Error()) continue } @@ -55,8 +62,8 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, models []dbmodel.Model, jqQuer formattedStatus, err := formatter.Format() if err != nil { - zapctx.Error(ctx, "failed to format status", zap.String("model-uuid", modelUUID)) - results.Errors[modelUUID] = append(results.Errors[modelUUID], err.Error()) + zapctx.Error(ctx, "failed to format status", zap.String("model-uuid", id)) + results.Errors[id] = append(results.Errors[id], err.Error()) continue } // We could use output.NewFormatter() from 3.0+ juju/juju, but ultimately @@ -64,8 +71,8 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, models []dbmodel.Model, jqQuer // *should* be OK. But TODO: make sure this is fine. fb, err := json.Marshal(formattedStatus) if err != nil { - zapctx.Error(ctx, "failed to marshal formatted status", zap.String("model-uuid", modelUUID)) - results.Errors[modelUUID] = append(results.Errors[modelUUID], err.Error()) + zapctx.Error(ctx, "failed to marshal formatted status", zap.String("model-uuid", id)) + results.Errors[id] = append(results.Errors[id], err.Error()) continue } tempMap := make(map[string]any) @@ -84,11 +91,11 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, models []dbmodel.Model, jqQuer // query. As such, we simply append all to the errors field and continue to collect // both erreoneous and valid query results. if err, ok := v.(error); ok { - results.Errors[modelUUID] = append(results.Errors[modelUUID], "jq error: "+err.Error()) + results.Errors[id] = append(results.Errors[id], "jq error: "+err.Error()) continue } - results.Results[modelUUID] = append(results.Results[modelUUID], v) + results.Results[id] = append(results.Results[id], v) } } return results, nil @@ -117,8 +124,10 @@ func newFormatterParamsRetriever(j *JIMM) *formatterParamsRetriever { // GetParams retrieves the required parameters for the Juju status formatter from the currently // loaded model. See formatterParamsRetriever.LoadModel for more information. -func (f *formatterParamsRetriever) GetParams(ctx context.Context, model dbmodel.Model) (*status.NewStatusFormatterParams, error) { - f.model = &model +func (f *formatterParamsRetriever) GetParams(ctx context.Context, modelUUID string) (*status.NewStatusFormatterParams, error) { + if err := f.loadModel(ctx, modelUUID); err != nil { + return nil, err + } err := f.dialModel(ctx) if err != nil { @@ -145,13 +154,24 @@ func (f *formatterParamsRetriever) GetParams(ctx context.Context, model dbmodel. }, nil } +// LoadModel loads the model by UUID from the database into the formatterParamsRetriever. +// This MUST be called before attempting to GetParams(). +func (f *formatterParamsRetriever) loadModel(ctx context.Context, modelUUID string) error { + model := dbmodel.Model{ + UUID: sql.NullString{String: modelUUID, Valid: true}, + } + + if err := f.jimm.Database.GetModel(ctx, &model); err != nil { + zapctx.Error(ctx, "failed to retrieve model", zap.String("model-uuid", modelUUID)) + return err + } + f.model = &model + return nil +} + // dialModel dials the model currently loaded into the formatterParamsRetriever. func (f *formatterParamsRetriever) dialModel(ctx context.Context) error { - modelTag, ok := f.model.Tag().(names.ModelTag) - if !ok { - return errors.E(errors.Op("failed to parse model tag")) - } - api, err := f.jimm.dial(ctx, &f.model.Controller, modelTag) + api, err := f.jimm.dial(ctx, &f.model.Controller, f.model.ResourceTag()) if err != nil { zapctx.Error(ctx, "failed to dial controller for model", zap.String("controller-uuid", f.model.Controller.UUID), zap.String("model-uuid", f.model.UUID.String), zap.Error(err)) } diff --git a/internal/jimm/model_status_parser_test.go b/internal/jimm/model_status_parser_test.go index 097a0921c..1798a074a 100644 --- a/internal/jimm/model_status_parser_test.go +++ b/internal/jimm/model_status_parser_test.go @@ -57,9 +57,6 @@ models: status: available info: "OK!" since: 2020-02-20T20:02:20Z - users: - - user: alice@external - access: admin - name: model-2 type: iaas uuid: 20000000-0000-0000-0000-000000000000 @@ -74,9 +71,6 @@ models: status: available info: "OK!" since: 2020-02-20T20:02:20Z - users: - - user: alice@external - access: admin - name: model-3 type: iaas uuid: 30000000-0000-0000-0000-000000000000 @@ -91,9 +85,6 @@ models: status: available info: "OK!" since: 2020-02-20T20:02:20Z - users: - - user: alice@external - access: admin - name: model-5 type: iaas uuid: 50000000-0000-0000-0000-000000000000 @@ -120,8 +111,6 @@ func getFullStatus( modelName string, applications map[string]jujuparams.ApplicationStatus, remoteApps map[string]jujuparams.RemoteApplicationStatus, - modelRelations []jujuparams.RelationStatus, - ) jujuparams.FullStatus { return jujuparams.FullStatus{ Model: jujuparams.ModelStatusInfo{ @@ -143,7 +132,7 @@ func getFullStatus( Applications: applications, RemoteApplications: remoteApps, Offers: map[string]jujuparams.ApplicationOfferStatus{}, - Relations: modelRelations, + Relations: []jujuparams.RelationStatus(nil), Branches: map[string]jujuparams.BranchStatus{}, } } @@ -208,22 +197,6 @@ var model1 = getFullStatus("model-1", map[string]jujuparams.ApplicationStatus{ }, }, }, - []jujuparams.RelationStatus{ - { - Id: 0, - Key: "myapp", - Interface: "db", - Scope: "regular", - Endpoints: []jujuparams.EndpointStatus{ - { - ApplicationName: "myapp", - Name: "db", - Role: "myrole", - Subordinate: false, - }, - }, - }, - }, ) // Model2 holds a model that is running against a K8S controller and has two apps. @@ -309,19 +282,16 @@ var model2 = getFullStatus("model-2", map[string]jujuparams.ApplicationStatus{ }, }, nil, - nil, ) // Model3 holds an empty model var model3 = getFullStatus("model-3", map[string]jujuparams.ApplicationStatus{}, nil, - nil, ) // Model5 holds an empty model, but it's API returns an error for storage var model5 = getFullStatus("model-5", map[string]jujuparams.ApplicationStatus{}, nil, - nil, ) func TestQueryModelsJq(t *testing.T) { @@ -440,8 +410,6 @@ func TestQueryModelsJq(t *testing.T) { env := jimmtest.ParseEnvironment(c, crossModelQueryEnv) env.PopulateDB(c, j.Database, nil) - user := env.User("alice@external").DBObject(c, j.Database, nil) - modelUUIDs := []string{ "10000000-0000-0000-0000-000000000000", "20000000-0000-0000-0000-000000000000", @@ -494,17 +462,20 @@ func TestQueryModelsJq(t *testing.T) { }..., ), qt.Equals, nil) + alice := openfga.NewUser( + &dbmodel.User{ + Username: "alice@external", + }, + client, + ) + // Tests: // Query for all models only. - usersModels, err := j.Database.GetUserModels(ctx, &user) + userModelUUIDs, err := alice.ListModels(ctx) c.Assert(err, qt.IsNil) - models := make([]dbmodel.Model, len(usersModels)) - for i, m := range usersModels { - models[i] = m.Model_ - } - res, err := j.QueryModelsJq(ctx, models, ".model") + res, err := j.QueryModelsJq(ctx, userModelUUIDs, ".model") c.Assert(err, qt.IsNil) c.Assert(` { @@ -556,6 +527,9 @@ func TestQueryModelsJq(t *testing.T) { ] }, "errors": { + "40000000-0000-0000-0000-000000000000": [ + "model not found" + ], "50000000-0000-0000-0000-000000000000": [ "forcing an error on model 5" ] @@ -564,7 +538,10 @@ func TestQueryModelsJq(t *testing.T) { `, qt.JSONEquals, res) // Query all applications across all models. - res, err = j.QueryModelsJq(ctx, models, ".applications") + userModelUUIDs, err = alice.ListModels(ctx) + c.Assert(err, qt.IsNil) + + res, err = j.QueryModelsJq(ctx, userModelUUIDs, ".applications") c.Assert(err, qt.IsNil) c.Assert(` { @@ -700,6 +677,9 @@ func TestQueryModelsJq(t *testing.T) { ] }, "errors": { + "40000000-0000-0000-0000-000000000000": [ + "model not found" + ], "50000000-0000-0000-0000-000000000000": [ "forcing an error on model 5" ] @@ -708,7 +688,10 @@ func TestQueryModelsJq(t *testing.T) { `, qt.JSONEquals, res) // Query specifically for models including the app "nginx-ingress-integrator" - res, err = j.QueryModelsJq(ctx, models, ".applications | with_entries(select(.key==\"nginx-ingress-integrator\"))") + userModelUUIDs, err = alice.ListModels(ctx) + c.Assert(err, qt.IsNil) + + res, err = j.QueryModelsJq(ctx, userModelUUIDs, ".applications | with_entries(select(.key==\"nginx-ingress-integrator\"))") c.Assert(err, qt.IsNil) c.Assert(` { @@ -762,6 +745,9 @@ func TestQueryModelsJq(t *testing.T) { ] }, "errors": { + "40000000-0000-0000-0000-000000000000": [ + "model not found" + ], "50000000-0000-0000-0000-000000000000": [ "forcing an error on model 5" ] @@ -770,7 +756,10 @@ func TestQueryModelsJq(t *testing.T) { `, qt.JSONEquals, res) // Query specifically for storage on this model. - res, err = j.QueryModelsJq(ctx, models, ".storage") + userModelUUIDs, err = alice.ListModels(ctx) + c.Assert(err, qt.IsNil) + + res, err = j.QueryModelsJq(ctx, userModelUUIDs, ".storage") c.Assert(err, qt.IsNil) // Not the cleanest thing in the world, but this field needs ignoring, @@ -815,10 +804,14 @@ func TestQueryModelsJq(t *testing.T) { ] }, "errors": { + "40000000-0000-0000-0000-000000000000": [ + "model not found" + ], "50000000-0000-0000-0000-000000000000": [ "forcing an error on model 5" - ] + ] } } `, qt.JSONEquals, res) + } diff --git a/internal/jujuapi/jimm.go b/internal/jujuapi/jimm.go index 061682304..6a432affd 100644 --- a/internal/jujuapi/jimm.go +++ b/internal/jujuapi/jimm.go @@ -496,18 +496,13 @@ func (r *controllerRoot) RemoveCloudFromController(ctx context.Context, req apip // The query will run against output exactly like "juju status --format json", but for each of their models. func (r *controllerRoot) CrossModelQuery(ctx context.Context, req apiparams.CrossModelQueryRequest) (apiparams.CrossModelQueryResponse, error) { const op = errors.Op("jujuapi.CrossModelQuery") - - usersModels, err := r.jimm.Database.GetUserModels(ctx, r.user.User) + modelUUIDs, err := r.user.ListModels(ctx) if err != nil { return apiparams.CrossModelQueryResponse{}, errors.E(op, errors.Code("failed to get models for user")) } - models := make([]dbmodel.Model, len(usersModels)) - for i, m := range usersModels { - models[i] = m.Model_ - } switch strings.TrimSpace(strings.ToLower(req.Type)) { case "jq": - return r.jimm.QueryModelsJq(ctx, models, req.Query) + return r.jimm.QueryModelsJq(ctx, modelUUIDs, req.Query) case "jimmsql": return apiparams.CrossModelQueryResponse{}, errors.E(op, errors.CodeNotImplemented) default: