chore: use the juju client for model related methods by kian99 · Pull Request #1633 · canonical/jimm

kian99 · 2025-06-26T15:19:55Z

Description

This change started off as a simple one, a continuation of #1611 where we slowly move our jujuclient package to use Juju's Go client so that we don't need to call facades directly and we get nicer types out of the client.
The downside of this change is that I had to replace all usages of jujuparams.* with base.* where were were using the RPC object in many places (i.e. an object with JSON tags used for marshalling/unmarshalling over the wire). We are now using a different but similar type that simplifies a few rough edges.

There was also a somewhat unrelated but necessary change in the internal/rpc package where I had previously introduced a dependency on internal/jimm/ that was incorrect and caused cyclic dependencies during this PR. That's now been addressed.

Finally, you'll see new functions in internal/jujuapi/types.go that convert from the base.* types to the API type. I haven't written any tests for these as our integration tests cover this behavior, but as discussed before, if we have any objections to this approach we can always revisit it. Also, confusingly, the types in Juju's base package are not the same types used by Juju's apiserver, this might be worth looking at further.

Fixes JUJU-9132

Engineering checklist

Documentation updated
Covered by unit tests
Covered by integration tests

kian99 · 2025-06-26T15:32:45Z

internal/jujuapi/types.go

This file was renamed and modified, see types.go for what existed previously.

alesstimec

1st pass review with a few comments

alesstimec · 2025-07-08T14:32:18Z

internal/errors/errors.go

 	// API Error types.
-	if ec, ok := e.Err.(interface{ ErrorCode() string }); ok {
-		e.Code = Code(ec.ErrorCode())
+	var errCode interface{ ErrorCode() string }


why this change?

The error that comes out of the Juju client is wrapped internally due to a errors.Trace(err) call where that original err has an error code. Then, when do errors.E(err) and try to extract an error code, we get nothing because the error with the code is 2 levels deep. So we can try to unwrap the error until we find an error that implement ErrorCode() or we reach the end of the list of wrapped errors.

alesstimec · 2025-07-08T14:35:36Z

internal/jimm/juju/interface.go


 	// ChangeModelCredential replaces cloud credential for a given model with the provided one.
-	ChangeModelCredential(context.Context, names.ModelTag, names.CloudCredentialTag) error
+	ChangeModelCredential(model names.ModelTag, credential names.CloudCredentialTag) error


i'm not fond of named arguments in interface definitions.. feels like the godoc above does not fully explain what the method is expected to do

Will remove the named arguments and update the godoc.

Removed the named arguments where unneccessary (some were useful and I've left them).
Left the docstring since I didn't touch that in this PR.

internal/jimm/juju/modelbuilder.go

alesstimec · 2025-07-08T14:39:16Z

internal/jujuapi/mapper.go

+	"gopkg.in/yaml.v3"
+)
+
+func cloudFromParams(cloudName string, p jujuparams.Cloud) cloud.Cloud {


it would be nice to have some consistency in naming.. XYToZW where X is a package, Y a type, Z a package and W a type.. e.g. paramsCloudToCloudCloud wdyt?

or have a type that embeds cloud.Cloud and has a FromParamsCloud method

I hear you, I'll take a look at this

Here are some thoughts, we either need to convert from an RPC struct to some type that we use internally or we need to convert to whatever type we use internally to an RPC type - speaking specifically about the jujuapi package.

For converting InternalType -> RPC, we can look at internal/jujuapi/cloud.go where we've got the API handler CloudInfo() which receives a dbmodel.Cloud and runs cloud.ToJujuCloudInfo() on the dbmodel object converting from the dbmodel type to the RPC type. I don't think that is great because it imports the RPC types into the dbmodel package for the purposes of putting the conversion logic in there.

Then for RPC -> InternalType we can look at AddCloud in the same file, which calls cloudFromParams to convert a jujuparams.Cloud to github.com/juju/juju/cloud.Cloud.

So ultimately, my idea is that it would be nice if the naming convention for the mapping functions easily indicated we are pushing data down vs surfacing it.
I.e. if we are converting to an internal type we could name the function toX where X is some type. And if we are converting to an RPC type we could name the function fromX.
The actual words don't matter to me but at least names that reflect which way the data is going. Another options might be rpcToX and xFromRPC.

exactly.. some naming convention where i can tell which way we're converting just by looking at the method name

alesstimec · 2025-07-08T14:41:00Z

internal/jujuclient/dial.go

 	return d.createLoginRequest1(ctx, ctl.ResourceTag(), user.ResourceTag(), p)
 }

+// toModelAccessString maps relation to a model access string.


hhmmm.. i vaguely remember we have a place for these conversion methods between openfga relations to juju access strings, don't we?

Yeah inside internal/jimm/permissions , and you'll see that's where it was used from initially, but I don't think the jujuclient should be reaching in there for things so I've copied it here. Especially because internal/jimm/juju imports this package, so you can get into a dependency cycle very easily (can't remember if this import did cause a cycle but better to avoid importing anything internal/jimm from internal/jujuclient).

The issue makes me think the conversion of OpenFGA types to Juju strings should happen in some kind of adapter package that like internal/openfgaToJuju or at least something along those lines is worth thinking about.

what do you think about a test to make sure these checks can't drift.

I've tried to add a test now but the package does blackbox testing so I can't without more changes,

perhaps the openfga.Relation type could implement ModelAccessString or ControllerAccessString as needed to convert them to juju access strings..

Yeah that could work. I'll keep it in mind but won't add it to this PR.

internal/jujuclient/modelmanager.go

alesstimec · 2025-07-08T14:43:40Z

internal/jujuclient/types.go

+// types preferably of the form base.ModelInfo. Until either that inconsitency
+// is fixed or we export the convertParamsModelInfo function, we copy it here
+// here have better consistency in our domain layer.
+func convertParamsModelInfo(modelInfo params.ModelInfo) (base.ModelInfo, error) {


again a comment about naming consistency, or having a type that wraps base.ModelInfo and has a method for conversion from params.ModelInfo and a method that returns the wrapped base.ModelInfo

This function and name was copied from Juju so maybe this one should stay the same?

internal/rpc/httpproxy.go

luci1900 · 2025-08-13T12:33:21Z

So this is a change to use the existing machinery that juju clients rely on for resolving RPC methods to specific ones in case of multiple versions or similar?

Looks reasonable overall, but I don't know if I understand it enough to approve.

internal/jimm/jimm.go

internal/jimm/juju/interface.go

internal/jimm/juju/modelbuilder.go

internal/dbmodel/model_test.go

kian99 · 2026-02-10T11:23:09Z

internal/jujuclient/types.go

+// a params.ModelInfo type is returned. We want consistent types preferably of
+// the form base.ModelInfo. Until the client returns a consistent set of types
+// we copy it here here to have better consistency in our domain layer.
+func convertParamsModelInfo(modelInfo params.ModelInfo) (ModelInfo, error) {


An important note here. The params.ModelInfo holds a few more fields than base.ModelInfo. The difference is below.
The following fields appear in params.ModelInfo but don't appear in base.ModelInfo (conversely there is nothing missing):

DefaultBase string `json:"default-base,omitempty"` // CloudCredentialValidity contains if model credential is valid, if known. CloudCredentialValidity *bool `json:"cloud-credential-validity,omitempty"` // SecretBackends contains information about the secret backends // currently in use by the model. SecretBackends []SecretBackendResult `json:"secret-backends"` // Migration contains information about the latest failed or // currently-running migration. It'll be nil if there isn't one. Migration *ModelMigrationStatus `json:"migration,omitempty"` // SLA contains the information about the SLA for the model, if set. SLA *ModelSLAInfo `json:"sla"` // SupportedFeatures provides information about the set of features // supported by this model. The feature set contains both Juju-specific // entries (e.g. juju version) and other features that depend on the // substrate the model is deployed to. SupportedFeatures []SupportedFeature `json:"supported-features,omitempty"`

Out of these fields I believe SLA is long deprecated and unused, SupportedFeatures I'm unsure on its usage, Migration is necessary and we've added it to our wrapper, SecretBackends seems useful but not currently parsed and DefaultBase is also not parsed. So I've gone and checked the output of juju show-model after this change via JIMM and via the backing Juju controller. Below is the output from JIMM with the fields from Juju added and marked. Note that the output doesn't include ommitted fields like migration status which is only present in certain circumstances.

foo: name: jimm-test@canonical.com/foo short-name: foo model-uuid: fe9cecbf-676e-4592-8c0c-effcaf981400 model-type: iaas controller-uuid: 3217dbc9-8ea9-4381-9e97-01eab0b3f6bb controller-name: jimm-dev is-controller: false owner: jimm-test@canonical.com cloud: localhost region: localhost type: lxd life: alive status: current: available since: 7 seconds ago users: jimm-test@canonical.com:\ display-name: jimm-test ---- missing access: admin last-connection: never connected sla: unsupported ---- missing agent-version: 3.6.12 credential: name: localhost owner: jimm-test@canonical.com cloud: localhost validity-check: invalid ---- backing Juju controller says valid supported-features: ---- this entire block is missing - name: juju description: the version of Juju used by the model version: 3.6.12

What I'll do is expand our ModelInfo struct to include the missing fields so that we can always return them and add a test that verifies the difference between juju show-model against JIMM vs Juju.

I'm thinking about doing the above in a follow-up PR.

Alternatively, maybe we should just pass around the rpc type when we call this method instead of the base type.

It doesn't seem like it's a problem to pass the RPC type.

mmm, that's not good. Why would this type be less than the rpc type? Should we fix it in juju?

To summarise the work done here,

I tried changing to use the RPC type and that proved challenging because there are functions used on code-paths that are common to both CreateModel and ModelInfo calls. If the one returns a base.ModelInfo (returned by CreateModel) and the other returns a rpc.ModelInfo, we create a lot more friction in the code for nothing.

I discussed making the change in Juju and there is an appetite for it, but I've decided to forego making the change in 3.6 as we'll move to 4.0 soon and it would be a breaking change in 3.6.

I've added the missing types to our jujuclient package and update our mapping logic in jujuapi. Now the response from JIMM and Juju are nearly identical.

SimoneDutto · 2026-02-10T13:48:55Z

internal/jimm/juju/cloudcredential_test.go

 						}}, nil
 					}
 				},
-				GrantJIMMModelAdmin_: func(_ context.Context, _ names.ModelTag) error {


leave the ctx even if it's useless rn because once we migrate to juju 4 client, all of the methods will require ctx

I was thinking about that, I don't mind adding it back afterwards, wdyt?

We'll definitely need it, I'd imagine it's simpler to not remove it now.

Yeah let me add them back

luci1900

Mostly mechanical changes, should be fine.

I do think we need to QA this somewhat more thoroughly than other changes, simply because it touches so many areas.

SimoneDutto

Some questions to be sure everything we're doing here is correct

SimoneDutto · 2026-02-12T11:24:56Z

internal/jimm/juju/interface.go

@@ -131,13 +132,13 @@ type API interface {
 	ListApplicationOffers(context.Context, []jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error)

 	// ListModelSummaries lists models summaries
-	ListModelSummaries(context.Context, jujuparams.ModelSummariesRequest) (jujuparams.ModelSummaryResults, error)
+	ListModelSummaries(context.Context, jujuparams.ModelSummariesRequest) ([]base.UserModelSummary, error)

 	// ModelInfo fetches a model's ModelInfo.
-	ModelInfo(context.Context, *jujuparams.ModelInfo) error
+	ModelInfo(context.Context, names.ModelTag) (jujuclient.ModelInfo, error)

 	// ModelStatus fetches a model's ModelStatus.
-	ModelStatus(context.Context, *jujuparams.ModelStatus) error
+	ModelStatus(context.Context, names.ModelTag) (base.ModelStatus, error)

 	// ModelSummaryWatcherNext returns the next set of model summaries from
 	// the watcher.
@@ -179,7 +180,7 @@ type API interface {
 	UpdateCredential(context.Context, jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error)

 	// ValidateModelUpgrade validates that a model can be upgraded.
-	ValidateModelUpgrade(context.Context, names.ModelTag, bool) error
+	ValidateModelUpgrade(ctx context.Context, model names.ModelTag, force bool) error


why do we need the named args?

I added them where it was unclear what the bool/string were. A names.ModelTag is clear, but bool, string, bool was very unclear.

SimoneDutto · 2026-02-12T11:26:30Z

internal/jimm/juju/model.go


 // AddModel adds the specified model to JIMM.
-func (j *JujuManager) AddModel(ctx context.Context, user *openfga.User, args *ModelCreateArgs) (_ *jujuparams.ModelInfo, err error) {
+func (j *JujuManager) AddModel(ctx context.Context, user *openfga.User, args *ModelCreateArgs) (base.ModelInfo, error) {


why did we have the named return here? I can't see any defer something, but double check please

Double checked the current code and looks fine without a named return.

SimoneDutto · 2026-02-12T11:27:04Z

internal/jimm/juju/model.go

 // access to the model then the returned error will have the code
 // CodeUnauthorized.
-func (j *JujuManager) ModelInfo(ctx context.Context, user *openfga.User, mt names.ModelTag) (*jujuparams.ModelInfo, error) {
+func (j *JujuManager) ModelInfo(ctx context.Context, user *openfga.User, mt names.ModelTag) (jujuclient.ModelInfo, error) {


what's the reason of using the struct instead of the pointer?

I believe a pointer was used to make the return values "simpler", i.e. return null, but there was no reason for a pointer.

SimoneDutto · 2026-02-12T11:28:55Z

internal/jimm/juju/model.go

 		models = append(models, struct {
 			model      *dbmodel.Model
-			userAccess jujuparams.UserAccessPermission
+			userAccess string


this was an "enum", converting it to a string isn't dangerous for when we do the permission checks?

So the struct value being a string is okay because that's just used for returning to the user later, the signature for the ForEachUserModel callback could be a better type but it's also only being used here so I believe it's okay.

SimoneDutto · 2026-02-12T11:30:38Z

internal/jimm/juju/model.go

-func (j *JujuManager) mergeModelInfo(ctx context.Context, user *openfga.User, modelInfo *jujuparams.ModelInfo, jimmModel dbmodel.Model) (*jujuparams.ModelInfo, error) {
-	modelInfo.CloudCredentialTag = jimmModel.CloudCredential.Tag().String()
+func (j *JujuManager) mergeModelInfo(ctx context.Context, user *openfga.User, modelInfo jujuclient.ModelInfo, jimmModel dbmodel.Model) (jujuclient.ModelInfo, error) {
+	modelInfo.CloudCredential = jimmModel.CloudCredential.ResourceTag().Id()


so now this is just the cloud credential name instead of the full tag?

Yip, similarly for ownerTag -> Owner and CloudTag -> Cloud

SimoneDutto · 2026-02-12T11:41:33Z

internal/jujuclient/controller.go

 }
+
+// CloudSpec retrieves the cloud spec of the model connected to.
+func (c Connection) CloudSpec(ctx context.Context) (cloudspec.CloudSpec, error) {


why do we need this now?

It replaces ControllerModelSummary which was being used to get the controller model's cloud and cloud-credential then we'd fetch the cloud-credential. If/when we remove the cloning of controllers we can remove this and simplify.

I left out a bit of info, there was no Juju API equivalent of ControllerModelSummary, but CloudSpec() exists and did what we needed.

SimoneDutto · 2026-02-12T11:42:50Z

internal/jujuclient/dial.go

 	return d.createLoginRequest1(ctx, ctl.ResourceTag(), user.ResourceTag(), p)
 }

+// toModelAccessString maps relation to a model access string.


what do you think about a test to make sure these checks can't drift.

internal/jujuclient/modelmanager_test.go

SimoneDutto · 2026-02-12T11:44:56Z

internal/jujuclient/modelmanager.go

-	err := c.Call(ctx, "ModelManager", 10, "", "ModelStatus", &args, &resp)
+// ModelStatus retrieves the status of a model from the controller.
+func (c Connection) ModelStatus(ctx context.Context, modelTag names.ModelTag) (base.ModelStatus, error) {
+	statuses, err := modelmanager.NewClient(&c).ModelStatus(modelTag)


here err is the first error in the Result or does it work in the client?
I'm kinda worried because they don't "type" errors when the result is of Result and it can't potentially mess with our NotFound checks.

I think our errors.E was typing the error, so this change you've made could be breaking

It should all work out once we get to the API layer and call mapError which will inspect the error with ErrorCode until it finds a code.

SimoneDutto · 2026-02-12T11:51:41Z

internal/jujuclient/types.go

+// a params.ModelInfo type is returned. We want consistent types preferably of
+// the form base.ModelInfo. Until the client returns a consistent set of types
+// we copy it here here to have better consistency in our domain layer.
+func convertParamsModelInfo(modelInfo params.ModelInfo) (ModelInfo, error) {


mmm, that's not good. Why would this type be less than the rpc type? Should we fix it in juju?

SimoneDutto

ty for the effort

alesstimec · 2026-02-16T08:39:57Z

internal/jujuclient/dial.go

 	return d.createLoginRequest1(ctx, ctl.ResourceTag(), user.ResourceTag(), p)
 }

+// toModelAccessString maps relation to a model access string.


perhaps the openfga.Relation type could implement ModelAccessString or ControllerAccessString as needed to convert them to juju access strings..

internal/jujuclient/modelmanager.go

kian99 force-pushed the replace-model-manager-client branch from 643ceb3 to fca0e39 Compare June 27, 2025 07:39

kian99 commented Jun 27, 2025

View reviewed changes

internal/jujuapi/types.go

Copy link

Contributor Author

kian99 Jun 26, 2025

Choose a reason for hiding this comment

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

This file was renamed and modified, see types.go for what existed previously.

kian99 force-pushed the replace-model-manager-client branch 2 times, most recently from b3b7330 to f7aa97d Compare July 8, 2025 07:51

kian99 marked this pull request as ready for review July 8, 2025 09:13

kian99 requested a review from a team as a code owner July 8, 2025 09:13

alesstimec reviewed Jul 8, 2025

View reviewed changes

kian99 force-pushed the replace-model-manager-client branch 3 times, most recently from ec13770 to 82493fa Compare July 23, 2025 09:16

luci1900 reviewed Aug 13, 2025

View reviewed changes

internal/jimm/jimm.go Show resolved Hide resolved

luci1900 reviewed Aug 13, 2025

View reviewed changes

internal/jimm/juju/interface.go Show resolved Hide resolved

luci1900 reviewed Aug 13, 2025

View reviewed changes

internal/jimm/juju/modelbuilder.go Show resolved Hide resolved

luci1900 reviewed Aug 13, 2025

View reviewed changes

internal/dbmodel/model_test.go Show resolved Hide resolved

kian99 force-pushed the replace-model-manager-client branch from 477808e to dbc97a1 Compare February 9, 2026 15:38

kian99 commented Feb 10, 2026

View reviewed changes

kian99 force-pushed the replace-model-manager-client branch from 573ef33 to 8e462f2 Compare February 10, 2026 12:43

SimoneDutto reviewed Feb 10, 2026

View reviewed changes

kian99 force-pushed the replace-model-manager-client branch from b9c5607 to 648d8c3 Compare February 10, 2026 14:24

kian99 added 6 commits February 11, 2026 09:41

feat: replace model manager with Juju API client

8656120

refactor: use jujuclient.ModelInfo over base.ModelInfo

f6935cf

fix: address failing tests

8b08ff5

fix: return error codes in jujuapi

30288bd

feat: keep GrantJIMMModelAdmin api method

da62fb0

feat: replace ControllerModelSummary with CloudSpec call

c40fa4c

kian99 force-pushed the replace-model-manager-client branch from 2704a9d to c40fa4c Compare February 11, 2026 07:42

chore: rename mock field to match expected value

9e9c9b3

luci1900 approved these changes Feb 11, 2026

View reviewed changes

chore: add ctx to api methods

f0bb1f6

SimoneDutto reviewed Feb 12, 2026

View reviewed changes

kian99 added 4 commits February 13, 2026 16:12

Merge branch 'v3' into replace-model-manager-client

b0c9a9e

chore: update usages of jujuparams.ModelInfo

997c666

feat: add missing fields to jujuclient.ModelInfo

455f8ff

chore: avoid unnecessary pointer

48fe1cd

SimoneDutto approved these changes Feb 16, 2026

View reviewed changes

alesstimec approved these changes Feb 16, 2026

View reviewed changes

ale8k approved these changes Feb 16, 2026

View reviewed changes

chore: extra check on model status API call

4cb8546

kian99 mentioned this pull request Feb 16, 2026

chore(jujuclient): remove Ping because it's unused #1866

Merged

kian99 merged commit ed0a281 into canonical:v3 Feb 16, 2026
8 checks passed

Conversation

kian99 commented Jun 26, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Engineering checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesstimec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kian99 Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kian99 Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kian99 Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luci1900 commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimoneDutto Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kian99 commented Jun 26, 2025 •

edited by atlassian bot

Loading

kian99 Jul 11, 2025 •

edited

Loading

kian99 Feb 13, 2026 •

edited

Loading

kian99 Jul 11, 2025 •

edited

Loading

SimoneDutto Feb 10, 2026 •

edited

Loading