-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Enable importing models created by local users #1037
Changes from 14 commits
1185090
a202d94
7906d0d
ce8e457
3fc4b0e
e29c7c5
7840c4d
bd23c1b
e8b585d
5007d37
33f5db9
95d4492
4d3f1a7
cfa2d85
8782743
494eef5
5a0b317
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to call t.Parallel to have parallelism in tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe so, and I assume that's fine because our CI runs the test without that flag set. |
||
|
||
install: version/commit.txt version/version.txt | ||
go install -tags version $(INSTALL_FLAGS) -v $(PROJECT)/... | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -416,7 +416,7 @@ func (j *JIMM) GetControllerAccess(ctx context.Context, user *dbmodel.User, tag | |
} | ||
|
||
// ImportModel imports model with the specified uuid from the controller. | ||
func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName string, modelTag names.ModelTag) error { | ||
func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName string, modelTag names.ModelTag, newOwner string) error { | ||
const op = errors.Op("jimm.ImportModel") | ||
|
||
ale := dbmodel.AuditLogEntry{ | ||
|
@@ -461,7 +461,6 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName | |
if err != nil { | ||
return errors.E(op, err) | ||
} | ||
|
||
model := dbmodel.Model{} | ||
// fill in data from model info | ||
err = model.FromJujuModelInfo(modelInfo) | ||
|
@@ -471,31 +470,62 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName | |
model.ControllerID = controller.ID | ||
model.Controller = controller | ||
|
||
// fetch the model owner user | ||
ownerTag, err := names.ParseUserTag(modelInfo.OwnerTag) | ||
var ownerString string | ||
if newOwner != "" { | ||
// Switch the model to be owned by the specified user. | ||
ownerString = names.UserTagKind + "-" + newOwner | ||
} else { | ||
// Use the model owner user | ||
ownerString = modelInfo.OwnerTag | ||
} | ||
ownerTag, err := names.ParseUserTag(ownerString) | ||
kian99 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
return errors.E(op, err) | ||
} | ||
owner := dbmodel.User{} | ||
owner.SetTag(ownerTag) | ||
err = j.Database.GetUser(ctx, &owner) | ||
ownerUser := dbmodel.User{} | ||
ownerUser.SetTag(ownerTag) | ||
err = j.Database.GetUser(ctx, &ownerUser) | ||
if err != nil { | ||
return errors.E(op, err) | ||
} | ||
model.OwnerUsername = owner.Username | ||
model.Owner = owner | ||
model.SwitchOwner(&ownerUser) | ||
|
||
ownerHasModelAccess := false | ||
for _, user := range model.Users { | ||
if user.User.Username == ownerUser.Username { | ||
ownerHasModelAccess = true | ||
break | ||
} | ||
} | ||
if !ownerHasModelAccess { | ||
zapctx.Debug(ctx, "User doesn't have model access, adding it") | ||
// Ensure the current user gets access to the model | ||
// This will be applied to JIMM's access table lower down. | ||
model.Users = append(model.Users, dbmodel.UserModelAccess{User: *u, Access: string(jujuparams.ModelAdminAccess)}) | ||
} | ||
|
||
// TODO(CSS-5458): Remove the below section on cloud credentials once we no longer persist the relation between | ||
// cloud credentials and models | ||
|
||
// fetch cloud credential used by the model | ||
credentialTag, err := names.ParseCloudCredentialTag(modelInfo.CloudCredentialTag) | ||
cloudTag, err := names.ParseCloudTag(modelInfo.CloudTag) | ||
if err != nil { | ||
return errors.E(op, err) | ||
} | ||
cloudCredential := dbmodel.CloudCredential{} | ||
cloudCredential.SetTag(credentialTag) | ||
err = j.Database.GetCloudCredential(ctx, &cloudCredential) | ||
errors.E(op, 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 think they can create new | ||
// models on the incoming cloud. | ||
allCredentials, err := j.Database.GetUserCloudCredentials(ctx, &ownerUser, cloudTag.Id()) | ||
if err != nil { | ||
return errors.E(op, err) | ||
errors.E(op, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not returning the error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Big woops, good spot |
||
} | ||
if len(allCredentials) == 0 { | ||
return errors.E(op, errors.CodeNotFound, fmt.Sprintf("Failed to find cloud credential for user %s on cloud %s", ownerUser.Username, cloudTag.Id())) | ||
} | ||
cloudCredential := allCredentials[0] | ||
|
||
model.CloudCredentialID = cloudCredential.ID | ||
model.CloudCredential = cloudCredential | ||
|
||
|
@@ -505,6 +535,7 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName | |
} | ||
err = j.Database.GetCloud(ctx, &cloud) | ||
if err != nil { | ||
zapctx.Error(ctx, "failed to get cloud", zap.String("cloud", cloud.Name)) | ||
return errors.E(op, err) | ||
} | ||
|
||
|
@@ -521,10 +552,20 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName | |
return errors.E(op, "cloud region not found") | ||
} | ||
|
||
var usersExcludingLocalUsers []dbmodel.UserModelAccess | ||
for _, userAccess := range model.Users { | ||
if !strings.Contains(userAccess.User.Username, "@") { | ||
kian99 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// If the username doesn't contain an "@" the user is local | ||
// to the controller and we don't want to propagate it. | ||
continue | ||
} | ||
usersExcludingLocalUsers = append(usersExcludingLocalUsers, userAccess) | ||
} | ||
model.Users = usersExcludingLocalUsers | ||
|
||
for i, userAccess := range model.Users { | ||
u := userAccess.User | ||
err = j.Database.GetUser(ctx, &u) | ||
if err != nil { | ||
if err = j.Database.GetUser(ctx, &u); err != nil { | ||
return errors.E(op, err) | ||
} | ||
model.Users[i].Username = u.Username | ||
|
@@ -539,6 +580,19 @@ func (j *JIMM) ImportModel(ctx context.Context, u *dbmodel.User, controllerName | |
return errors.E(op, err) | ||
} | ||
|
||
if !ownerHasModelAccess { | ||
// Here we finally grant the model owner, access to the underlying model. | ||
err = j.doModelAdmin(ctx, u, modelTag, func(m *dbmodel.Model, api API) error { | ||
if err := api.GrantModelAccess(ctx, modelTag, ownerUser.Tag().(names.UserTag), jujuparams.ModelAdminAccess); err != nil { | ||
return err | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
return errors.E(op, err, fmt.Sprintf("Failed to grant user %s admin access on the model", ownerUser.Username)) | ||
} | ||
} | ||
|
||
modelAPI, err := j.dial(ctx, &controller, modelTag) | ||
if err != nil { | ||
return errors.E(op, err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a driveby change to add coverage information to the output of our tests