Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

import offers during model import #1432

Open
wants to merge 3 commits into
base: v3
Choose a base branch
from
Open

import offers during model import #1432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8a14ca
refactor import model function
kian99 Nov 6, 2024
c57abf8
import offers during model import
kian99 Nov 13, 2024
dd9588c
PR comments
kian99 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/dbmodel/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ func (m *Model) SetTag(t names.ModelTag) {
m.UUID.Valid = true
}

// FromModelUpdate updates the model from the given ModelUpdate.
func (m *Model) SwitchOwner(u *Identity) {
// SetOwner updates the model owner.
func (m *Model) SetOwner(u *Identity) {
m.OwnerIdentityName = u.Name
m.Owner = *u
}
Expand Down
243 changes: 177 additions & 66 deletions internal/jimm/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,133 +393,244 @@ func (j *JIMM) GetUserControllerAccess(ctx context.Context, user *openfga.User,
return ToControllerAccessString(accessLevel), nil
}

// ImportModel imports model with the specified UUID from the controller.
func (j *JIMM) ImportModel(ctx context.Context, user *openfga.User, controllerName string, modelTag names.ModelTag, newOwner string) error {
const op = errors.Op("jimm.ImportModel")
type modelImporter struct {
jimm *JIMM
model dbmodel.Model
modelInfo jujuparams.ModelInfo
// newOwner may be nil if the user wants to keep the original owner.
newOwner *names.UserTag
originalOwner names.UserTag
offersToAdd []jujuparams.ApplicationOfferAdminDetailsV5
}

if err := j.checkJimmAdmin(user); err != nil {
return err
func newModelImporter(jimm *JIMM, newOwner string) (modelImporter, error) {
modelImporter := modelImporter{
jimm: jimm,
}
if newOwner == "" {
return modelImporter, nil
}
if !names.IsValidUser(newOwner) {
return modelImporter, errors.E(errors.CodeBadRequest, "invalid new username for new model owner")
}
newOwnerTag := names.NewUserTag(newOwner)
modelImporter.newOwner = &newOwnerTag
return modelImporter, nil
}

controller, err := j.getControllerByName(ctx, controllerName)
func (m *modelImporter) fetchModelInfo(ctx context.Context, controllerName string, modelTag names.ModelTag) error {
controller, err := m.jimm.getControllerByName(ctx, controllerName)
if err != nil {
return errors.E(op, err)
return err
}

api, err := j.dialController(ctx, controller)
api, err := m.jimm.dialController(ctx, controller)
if err != nil {
return errors.E(op, "failed to dial the controller", err)
return errors.E("failed to dial the controller", err)
}
defer api.Close()

modelInfo := jujuparams.ModelInfo{
m.modelInfo = jujuparams.ModelInfo{
UUID: modelTag.Id(),
}
err = api.ModelInfo(ctx, &modelInfo)
err = api.ModelInfo(ctx, &m.modelInfo)
if err != nil {
return errors.E(op, err)
return err
}
model := dbmodel.Model{}

m.originalOwner, err = names.ParseUserTag(m.modelInfo.OwnerTag)
if err != nil {
return errors.E(fmt.Sprintf("invalid username %s from original model owner", m.modelInfo.OwnerTag))
}

m.offersToAdd, err = api.ListApplicationOffers(ctx, []jujuparams.OfferFilter{
{
OwnerName: m.originalOwner.Id(),
ModelName: m.modelInfo.Name,
},
})
if err != nil {
return err
}

// fill in data from model info
err = model.FromJujuModelInfo(modelInfo)
err = m.model.FromJujuModelInfo(m.modelInfo)
if err != nil {
return errors.E(op, err)
return err
}
model.ControllerID = controller.ID
model.Controller = *controller
m.model.ControllerID = controller.ID
m.model.Controller = *controller

return nil
}

func (m *modelImporter) setModelOwner(ctx context.Context) error {
var ownerTag names.UserTag
if newOwner != "" {
// Switch the model to be owned by the specified user.
if !names.IsValidUser(newOwner) {
return errors.E(op, errors.CodeBadRequest, "invalid new username for new model owner")
}
ownerTag = names.NewUserTag(newOwner)
if m.newOwner != nil {
ownerTag = *m.newOwner
} else {
// Use the model owner user
ownerTag, err = names.ParseUserTag(modelInfo.OwnerTag)
if err != nil {
return errors.E(op, fmt.Sprintf("invalid username %s from original model owner", modelInfo.OwnerTag))
}
ownerTag = m.originalOwner
}

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

err := m.jimm.Database.GetIdentity(ctx, &owner)
if err != nil {
return errors.E(op, err)
return errors.E(err)
}
model.SwitchOwner(&ownerUser)
m.model.SetOwner(&owner)

return nil
}

func (m *modelImporter) addPermissions(ctx context.Context) error {
// Note that only the new owner is given access. All previous users that had access according to Juju
// are discarded as access must now be governed by JIMM and OpenFGA.
ofgaUser := openfga.NewUser(&ownerUser, j.OpenFGAClient)
controllerTag := model.Controller.ResourceTag()
ofgaUser := openfga.NewUser(&m.model.Owner, m.jimm.OpenFGAClient)
controllerTag := m.model.Controller.ResourceTag()

if err := j.addModelPermissions(ctx, ofgaUser, modelTag, controllerTag); err != nil {
return errors.E(op, err)
if err := m.jimm.addModelPermissions(ctx, ofgaUser, m.model.ResourceTag(), controllerTag); err != nil {
return err
}

// TODO(CSS-5458): Remove the below section on cloud credentials once we no longer persist the relation between
// cloud credentials and models
for _, offer := range m.offersToAdd {
err := m.jimm.OpenFGAClient.AddModelApplicationOffer(ctx, m.model.ResourceTag(), names.NewApplicationOfferTag(offer.OfferUUID))
if err != nil {
return err
}
}

return nil
}

func (m *modelImporter) setCloudCredential(ctx context.Context) error {
// fetch cloud credential used by the model
cloudTag, err := names.ParseCloudTag(modelInfo.CloudTag)
cloudTag, err := names.ParseCloudTag(m.modelInfo.CloudTag)
if err != nil {
return errors.E(op, err)
return err
}

// Note that the model already has a cloud credential configured which it will use when deploying new
// applications. JIMM needs some cloud credential reference to be able to import the model so use any
// credential against the cloud the model is deployed against. Even using the correct cloud for the
// credential is not strictly necessary, but will help prevent the user thinking they can create new
// models on the incoming cloud.
allCredentials, err := j.Database.GetIdentityCloudCredentials(ctx, &ownerUser, cloudTag.Id())
allCredentials, err := m.jimm.Database.GetIdentityCloudCredentials(ctx, &m.model.Owner, cloudTag.Id())
if err != nil {
return errors.E(op, err)
return err
}
if len(allCredentials) == 0 {
return errors.E(op, errors.CodeNotFound, fmt.Sprintf("Failed to find cloud credential for user %s on cloud %s", ownerUser.Name, cloudTag.Id()))
return errors.E(errors.CodeNotFound, fmt.Sprintf("Failed to find cloud credential for user %s on cloud %s", m.model.Owner.Name, cloudTag.Id()))
}
cloudCredential := allCredentials[0]

model.CloudCredentialID = cloudCredential.ID
model.CloudCredential = cloudCredential
m.model.CloudCredentialID = cloudCredential.ID
m.model.CloudCredential = cloudCredential

return nil
}

func (m *modelImporter) setModelCloud(ctx context.Context) error {
// fetch the cloud used by the model
cloud := dbmodel.Cloud{
Name: cloudCredential.CloudName,
cloudTag, err := names.ParseCloudTag(m.modelInfo.CloudTag)
if err != nil {
return err
}
err = j.Database.GetCloud(ctx, &cloud)
cloud := dbmodel.Cloud{Name: cloudTag.Id()}
err = m.jimm.Database.GetCloud(ctx, &cloud)
if err != nil {
zapctx.Error(ctx, "failed to get cloud", zap.String("cloud", cloud.Name))
return errors.E(op, err)
return err
}

cr := cloud.Region(modelInfo.CloudRegion)
if cr.Name != modelInfo.CloudRegion {
return errors.E(op, "cloud region not found")
cr := cloud.Region(m.modelInfo.CloudRegion)
if cr.Name != m.modelInfo.CloudRegion {
return errors.E("cloud region not found")
}

model.CloudRegionID = cr.ID
model.CloudRegion = cr
m.model.CloudRegionID = cr.ID
m.model.CloudRegion = cr

err = j.Database.AddModel(ctx, &model)
if err != nil {
if errors.ErrorCode(err) == errors.CodeAlreadyExists {
return errors.E(op, err, "model already exists")
return nil
}

func (m *modelImporter) save(ctx context.Context) error {
return m.jimm.Database.Transaction(func(d *db.Database) error {
err := m.jimm.Database.AddModel(ctx, &m.model)
if err != nil {
if errors.ErrorCode(err) == errors.CodeAlreadyExists {
return fmt.Errorf("model (%s) already exists", m.model.Name)
}
return err
}
for _, offer := range m.offersToAdd {
var dbOffer dbmodel.ApplicationOffer
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, relation makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can do in a follow up PR @alesstimec

dbOffer.FromJujuApplicationOfferAdminDetailsV5(offer)
dbOffer.ModelID = m.model.ID
if err := m.jimm.Database.AddApplicationOffer(ctx, &dbOffer); err != nil {
if errors.ErrorCode(err) == errors.CodeAlreadyExists {
return fmt.Errorf("offer with URL %s already exists", offer.OfferURL)
}
return err
}
}
return nil
})
}

// ImportModel imports a model and existing offers into JIMM. A new owner must be set to
// represent the external user who will own this model (if the original owner is a local user).
func (j *JIMM) ImportModel(ctx context.Context, user *openfga.User, controllerName string, modelTag names.ModelTag, newOwner string) error {
const op = errors.Op("jimm.ImportModel")

if err := j.checkJimmAdmin(user); err != nil {
return err
}

importer, err := newModelImporter(j, newOwner)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return errors.E(op, err)
}

if err := importer.fetchModelInfo(ctx, controllerName, modelTag); err != nil {
return errors.E(op, err)
}

if err := importer.setModelOwner(ctx); err != nil {
return errors.E(op, err)
}

if err := importer.addPermissions(ctx); err != nil {
return errors.E(op, err)
}

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// on this model.
if err := importer.setCloudCredential(ctx); err != nil {
return errors.E(op, err)
}

if err := importer.setModelCloud(ctx); err != nil {
return errors.E(op, err)
}

if err := importer.save(ctx); err != nil {
return errors.E(op, err)
}

return j.handleModelDeltas(ctx, controller, modelTag, model)
return importer.handleModelDeltas(ctx)
}

func (j *JIMM) handleModelDeltas(ctx context.Context, controller *dbmodel.Controller, modelTag names.ModelTag, model dbmodel.Model) error {
func (m *modelImporter) handleModelDeltas(ctx context.Context) error {
const op = errors.Op("jimm.getModelDeltas")

modelAPI, err := j.dialModel(ctx, controller, modelTag)
modelAPI, err := m.jimm.dialModel(ctx, &m.model.Controller, m.model.ResourceTag())
if err != nil {
return errors.E(op, err)
}
Expand All @@ -541,9 +652,9 @@ func (j *JIMM) handleModelDeltas(ctx context.Context, controller *dbmodel.Contro
}

modelIDf := func(uuid string) *modelState {
if uuid == model.UUID.String {
if uuid == m.model.UUID.String {
return &modelState{
id: model.ID,
id: m.model.ID,
machines: make(map[string]int64),
units: make(map[string]bool),
}
Expand All @@ -552,7 +663,7 @@ func (j *JIMM) handleModelDeltas(ctx context.Context, controller *dbmodel.Contro
}

w := &Watcher{
Database: j.Database,
Database: m.jimm.Database,
}
for _, d := range deltas {
if err := w.handleDelta(ctx, modelIDf, d); err != nil {
Expand Down
Loading
Loading