Enable importing models created by local users #1037

kian99 · 2023-08-28T11:00:34Z

Description

Add the ability for a JIMM admin to import a model that belongs to a local user of a controller.
Currently importing a model that already exists but belongs to a local user will fail because JIMM requires that all models have an associated cloud-credential. A cloud-credential for a local user will never be something that JIMM is aware of.

A good follow-up to this would be the removal of associating cloud-credentials with a model since a cloud-credential is only needed once, when the model is created so it doesn't make sense to persist that info in JIMM.

Fixes CSS-4612

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

internal/jimm/controller.go

local/candid/config.yaml

ale8k

LGTM, surprised this wasn't already there

internal/jimm/controller_test.go

alesstimec

Hmm.. not really fond of this approach because the user doing the import automatically becomes the model owner.

Here's what i propose:

whoever is doing the import must be JIMM admin
we introduce an additional parameter to the command which will specify who is to be set as owner of the model - this way we basically say "we know owner is a local user, but we want that user to map to this specific user"

internal/jimm/controller.go

kian99 · 2023-08-30T07:55:54Z

@alesstimec

It's already the case that the user doing the import must be a JIMM admin.
We could do that, but that breaks existing clients, not a biggie, we just need to release a new client version. But I think this approach is somewhat cleaner in that regard, the user doing the import should be the owner (we already check they're a JIMM admin) and they can grant access to whoever they want.

kian99 · 2023-09-05T07:09:51Z

@alesstimec @ale8k
I addressed Ales' comments and changed the import-model function to accept an owner parameter to allow the user to explicitly change the owner of the model. Please take another look.

kian99 · 2023-09-05T07:11:20Z

.github/workflows/ci.yaml

@@ -40,7 +40,7 @@ jobs:
      - name: Start test environment
        run: docker compose up -d
      - name: Build and Test
-        run: go test -mod readonly ./...
+        run: go test -mod readonly ./... -coverprofile cover.out


This is a driveby change to add coverage information to the output of our tests

kian99 · 2023-09-05T07:11:44Z

Makefile

@@ -34,7 +34,7 @@ build/server: version/commit.txt version/version.txt
 	go build -tags version ./cmd/jimmsrv

 check: version/commit.txt version/version.txt
-	go test -p 1 -timeout 30m -tags version $(PROJECT)/...
+	go test -timeout 30m $(PROJECT)/... -coverprofile cover.out


Also cleaned this up a bit, let me know if you notice anything wrong with these changes. I don't think we are using this make target anywhere though

Without -p 1 I expect the tests to be run in parallel. Right?

You need to call t.Parallel to have parallelism in tests

I believe so, and I assume that's fine because our CI runs the test without that flag set. ~~Though perhaps that's where the failing watcher tests come from?~~ Alex explained above re parallelism

alesstimec

LTGM with a few suggestions..

cmd/jimmctl/cmd/importmodel.go

internal/jimm/controller.go

babakks

Just to make sure things are traceable, does the audit log record contain both the commanding user and the new-owner (if provided with --owner option)?

babakks · 2023-09-06T08:25:38Z

Makefile

@@ -34,7 +34,7 @@ build/server: version/commit.txt version/version.txt
 	go build -tags version ./cmd/jimmsrv

 check: version/commit.txt version/version.txt
-	go test -p 1 -timeout 30m -tags version $(PROJECT)/...
+	go test -timeout 30m $(PROJECT)/... -coverprofile cover.out


Without -p 1 I expect the tests to be run in parallel. Right?

babakks · 2023-09-06T08:39:26Z

internal/jimm/controller.go

-	owner.SetTag(ownerTag)
-	err = j.Database.GetUser(ctx, &owner)
+	if ownerTag.IsLocal() {
+		return errors.E(op, "cannot import model from local user, try --owner to switch the model owner")


This message is a little confusing for me. Also, there was no test to reproduce it.

Good point, I'll add one

I think it would be better explained in our documentation, what are local users vs external users. I mentioned --owner to help nudge the user towards reading the jimmctl import-model --help text which explains further

babakks · 2023-09-06T08:40:40Z

internal/jimm/controller.go

 	if err != nil {
-		return errors.E(op, err)
+		errors.E(op, err)


Why not returning the error?

Big woops, good spot

kian99 · 2023-09-06T08:49:33Z

@babakks >Just to make sure things are traceable, does the audit log record contain both the commanding user and the new-owner (if provided with --owner option)?

The audit logs should contains all the params for the call so it would include the user and the new owner

kian99 added 9 commits August 24, 2023 15:52

Add switch-owner option

1185090

Ignore local users on the incoming model

a202d94

Tweaks to import logic

7906d0d

Update setup-controller.sh

ce8e457

Add new test

3fc4b0e

Ensure old test follows previous behavior

e29c7c5

Test tweaks

7840c4d

Tweaks removing unnecessary changes

bd23c1b

Merge branch 'main' into enable-importing-non-external-user-models

e8b585d

kian99 requested review from alesstimec, babakks, ale8k and mina1460 August 29, 2023 06:55

ale8k reviewed Aug 29, 2023

View reviewed changes

internal/jimm/controller.go Outdated Show resolved Hide resolved

ale8k reviewed Aug 29, 2023

View reviewed changes

local/candid/config.yaml Outdated Show resolved Hide resolved

ale8k approved these changes Aug 29, 2023

View reviewed changes

ale8k reviewed Aug 29, 2023

View reviewed changes

internal/jimm/controller_test.go Outdated Show resolved Hide resolved

alesstimec reviewed Aug 30, 2023

View reviewed changes

internal/jimm/controller.go Outdated Show resolved Hide resolved

kian99 added 4 commits September 4, 2023 18:25

Update to add "owner" flag

5007d37

PR comments

33f5db9

Test fixes

95d4492

Add TODO with jira ref

4d3f1a7

kian99 requested review from ale8k and alesstimec September 5, 2023 07:10

kian99 commented Sep 5, 2023

View reviewed changes

Fixes

cfa2d85

alesstimec approved these changes Sep 6, 2023

View reviewed changes

cmd/jimmctl/cmd/importmodel.go Show resolved Hide resolved

internal/jimm/controller.go Show resolved Hide resolved

internal/jimm/controller.go Outdated Show resolved Hide resolved

PR suggestions

8782743

babakks reviewed Sep 6, 2023

View reviewed changes

babakks approved these changes Sep 6, 2023

View reviewed changes

kian99 added 2 commits September 6, 2023 10:53

Extra test and tweaks

494eef5

Switch to User.Tag()

5a0b317

kian99 merged commit 30c6c9d into canonical:main Sep 6, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable importing models created by local users #1037

Enable importing models created by local users #1037

kian99 commented Aug 28, 2023 •

edited by jira bot

Loading

ale8k left a comment

alesstimec left a comment

kian99 commented Aug 30, 2023

kian99 commented Sep 5, 2023 •

edited

Loading

kian99 Sep 5, 2023

kian99 Sep 5, 2023

babakks Sep 6, 2023

ale8k Sep 6, 2023

kian99 Sep 6, 2023 •

edited

Loading

alesstimec left a comment

babakks left a comment

babakks Sep 6, 2023

babakks Sep 6, 2023

kian99 Sep 6, 2023

kian99 Sep 6, 2023

babakks Sep 6, 2023

kian99 Sep 6, 2023

kian99 commented Sep 6, 2023

Enable importing models created by local users #1037

Enable importing models created by local users #1037

Conversation

kian99 commented Aug 28, 2023 • edited by jira bot Loading

Description

Engineering checklist

ale8k left a comment

Choose a reason for hiding this comment

alesstimec left a comment

Choose a reason for hiding this comment

kian99 commented Aug 30, 2023

kian99 commented Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kian99 Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

alesstimec left a comment

Choose a reason for hiding this comment

babakks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kian99 commented Sep 6, 2023

kian99 commented Aug 28, 2023 •

edited by jira bot

Loading

kian99 commented Sep 5, 2023 •

edited

Loading

kian99 Sep 6, 2023 •

edited

Loading