import offers during model import #1432

kian99 · 2024-11-12T12:13:36Z

Description

This PR does two things:

Refactors the ImportModel method. I started getting linting errors on cognitive complexity when trying to add functionality so I took the time to refactor the method.
Adds functionality to the ImportModel method to import a model's application-offers.

Please Note: The first commit contains only the first change and the second commit contains the second change.

One issue that remains is how to handle offer URLs that already exist. This will be tackled in a separate PR.

Partially fixes JUJU-7011

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

alesstimec

We need to discuss how to set access rights to imported application offers.

alesstimec · 2024-11-13T07:11:34Z

internal/jimm/controller.go

 		}
+		return nil
+	})
+	if err != nil {


question: we run handleModelDeltas on L516, which will connect to the controller, create an all watcher and process a single set of deltas.. is that the intent?

I can't say why it's like that, but that does seem intentional and has been that way for some time, the tests also verify the delta functionality.
The tests show that the deltas are used to update some properties of the model like the status fields, etc.

alesstimec · 2024-11-13T07:16:08Z

internal/jimm/controller.go

+			return errors.E(op, err)
+		}
+		for _, offer := range offersToAdd {
+			var dbOffer dbmodel.ApplicationOffer


while this will add offers to JIMM's database, it will not grant anybody access to imported offers. At the very least, the user importing the model should be granted access.

this will require a bit of thought: each offer comes with a set of users that have access to the offer. If those are external users (those that JIMM cares about), they should be granted access to the offer when it is imported. But i guess in most cases these users would be local users - access to the offer will remain granted on the controller, but JIMM will not know/care about those rights. perhaps we should simply print this info and let the user importing the model handle it. e.g. we imported this model and these app offer and these are the users that have access to the app offer - handle it!

True, I think we need to create an OpenFGA relation between the offers and the model. That way the model owner will automatically get access. Then they can grant access to other users. The users on the controller should never (at least with the new JIMM) be external users, because nowadays those are never stored on Juju's end (only in JIMM) so I think we can safely discard the users coming from the controller.

But yes printing a message would be helpful.

+1, relation makes sense to me.

Can do in a follow up PR @alesstimec

SimoneDutto

lgtm

alesstimec · 2024-11-13T13:22:03Z

internal/jimm/controller.go


-	if err := j.checkJimmAdmin(user); err != nil {
-		return err
+func newModelImporter(jimm *JIMM) modelImporter {


pass in the new owner name so that you can immediately return an error if we fail to parse the owner tag or owner is a local user

func newModelImporter(jimm *JIMM, ownerTag names.UserTag) (error, *modelImporter)

alesstimec · 2024-11-13T14:33:24Z

internal/dbmodel/model.go

@@ -103,7 +103,7 @@ func (m *Model) SetTag(t names.ModelTag) {
 	m.UUID.Valid = true
 }

-// FromModelUpdate updates the model from the given ModelUpdate.
+// SwitchOwner updates the model owner.


nitpick: SetOwner seems a more appropriate name

alesstimec · 2024-11-13T14:45:10Z

internal/jimm/controller.go

+
+	// TODO(CSS-5458): Remove the below section on cloud credentials once we no longer persist the relation between
+	// cloud credentials and models.
+	// Update: We need to investigate this further, if a user updates their cloud-credential it will update the credential


hm. this might be a bit confusing. if a user updates their cloud-credential, sure, jimm will try and update the cloud credential on the controller responsible for this model, but the actual credential used by the model will not be affected. the user would need to use the juju set-credential command to set credentials first and then update them to get the actual model cloud credentials updated.

Yes this will need fixing I think. For a follow up PR.

ale8k · 2024-11-14T07:14:55Z

internal/jimm/controller.go

+	})
+}
+
+// ImportModel imports model with the specified UUID from the controller.


imports a model and the currently existing offers into JIMM. A new owner must be set to represent the external user who will own this model.

Something like this for the comment?

ale8k

lgtm

- drive by fix to openfga rebac test that incorrectly assumed ordering

- ensure model owner has access to the offer

- Rename SwitchOwner to SetOwner - Updated newModelImporter constructor - Updated godoc

kian99 requested a review from a team as a code owner November 12, 2024 12:13

alesstimec requested changes Nov 13, 2024

View reviewed changes

kian99 requested a review from alesstimec November 13, 2024 08:09

kian99 force-pushed the JUJU-7011-import-offers branch from 20f70d3 to 6bfb06d Compare November 13, 2024 11:20

kian99 requested review from SimoneDutto and ale8k November 13, 2024 11:22

kian99 enabled auto-merge November 13, 2024 12:36

SimoneDutto approved these changes Nov 13, 2024

View reviewed changes

kian99 mentioned this pull request Nov 13, 2024

Import model name collision #1433

Open

alesstimec reviewed Nov 13, 2024

View reviewed changes

ale8k reviewed Nov 14, 2024

View reviewed changes

ale8k approved these changes Nov 14, 2024

View reviewed changes

kian99 added 2 commits November 14, 2024 11:51

refactor import model function

f8a14ca

- drive by fix to openfga rebac test that incorrectly assumed ordering

import offers during model import

c57abf8

- ensure model owner has access to the offer

kian99 force-pushed the JUJU-7011-import-offers branch from 6bfb06d to 275eda1 Compare November 14, 2024 09:51

kian99 requested a review from alesstimec November 14, 2024 09:51

alesstimec disabled auto-merge November 14, 2024 12:50

PR comments

dd9588c

- Rename SwitchOwner to SetOwner - Updated newModelImporter constructor - Updated godoc

kian99 force-pushed the JUJU-7011-import-offers branch from 275eda1 to dd9588c Compare November 15, 2024 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import offers during model import #1432

import offers during model import #1432

kian99 commented Nov 12, 2024 •

edited

Loading

alesstimec left a comment

alesstimec Nov 13, 2024

kian99 Nov 13, 2024

alesstimec Nov 13, 2024

kian99 Nov 13, 2024

ale8k Nov 14, 2024

ale8k Nov 14, 2024

SimoneDutto left a comment

alesstimec Nov 13, 2024

alesstimec Nov 13, 2024

alesstimec Nov 13, 2024

kian99 Nov 14, 2024

ale8k Nov 14, 2024

ale8k left a comment

import offers during model import #1432

Are you sure you want to change the base?

import offers during model import #1432

Conversation

kian99 commented Nov 12, 2024 • edited Loading

Description

Engineering checklist

alesstimec 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

SimoneDutto 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

ale8k left a comment

Choose a reason for hiding this comment

kian99 commented Nov 12, 2024 •

edited

Loading