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

CSS-5266 - Tweak length check to require at least 1 region and controller #1034

Merged
merged 3 commits into from
Aug 25, 2023
Merged

CSS-5266 - Tweak length check to require at least 1 region and controller #1034

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfddbce
Tweak length check to require at least 1 region and controller
kian99 Aug 24, 2023
2116a2f
Add tests
kian99 Aug 24, 2023
62e40b1
Updated error string to be more descriptive
kian99 Aug 24, 2023
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
12 changes: 8 additions & 4 deletions internal/jimm/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,11 +483,15 @@ func (j *JIMM) doCloudAdmin(ctx context.Context, u *dbmodel.User, ct names.Cloud
// an unauthorized error.
return errors.E(op, errors.CodeUnauthorized, "unauthorized")
}

if len(c.Regions) != 1 || len(c.Regions[0].Controllers) != 1 {
return errors.E(op, "cloud administration not available for %s", ct.Id())
// Ensure we always have at least 1 region for the cloud with at least 1 controller
// managing that region.
if len(c.Regions) < 1 || len(c.Regions[0].Controllers) < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was it != 1?

Copy link
Member

Choose a reason for hiding this comment

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

It was != 1, but that includes cases with > 1 elements which shouldn't result in an error.

zapctx.Error(ctx, "cloud regions check", zap.Int("regions", len(c.Regions)))
Copy link
Member

Choose a reason for hiding this comment

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

The error message is not clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more of a debug line actually, if I change it to Debug should I then leave it or make it more explanatory?

Copy link
Member

Choose a reason for hiding this comment

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

Although a clear message is good anyway, but if it's just for future debugging, leave it as is. Also, I'm not sure if changing the level to debug is going to help, because we may lose it due to the applied log-level config.

if len(c.Regions) > 0 {
zapctx.Error(ctx, "region controllers check", zap.Int("controllers", len(c.Regions[0].Controllers)))
Copy link
Member

Choose a reason for hiding this comment

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

This, too.

}
return errors.E(op, fmt.Sprintf("cloud administration not available for %s", ct.Id()))
}

api, err := j.dial(ctx, &c.Regions[0].Controllers[0].Controller, names.ModelTag{})
if err != nil {
return errors.E(op, err)
Expand Down
15 changes: 15 additions & 0 deletions internal/jimm/cloud_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,7 @@ const grantCloudAccessTestEnv = `clouds:
host-cloud-region: test-cloud/test-cloud-region
regions:
- name: default
- name: region2
users:
- user: alice@external
access: admin
Expand All @@ -1141,6 +1142,9 @@ controllers:
- cloud: test
region: default
priority: 1
- cloud: test
region: region2
priority: 1
`

var grantCloudAccessTests = []struct {
Expand Down Expand Up @@ -1197,6 +1201,17 @@ var grantCloudAccessTests = []struct {
},
Priority: 1,
}},
}, {
Name: "region2",
Controllers: []dbmodel.CloudRegionControllerPriority{{
Controller: dbmodel.Controller{
Name: "controller-1",
UUID: "00000001-0000-0000-0000-000000000001",
CloudName: "test-cloud",
CloudRegion: "test-cloud-region",
},
Priority: 1,
}},
}},
Users: []dbmodel.UserCloudAccess{{
Username: "alice@external",
Expand Down