slim clouds #1512

SimoneDutto · 2025-01-08T13:51:49Z

Description

In this pr we slim clouds, removing non-essential fields.

alesstimec

The approach looks good to me.. a few comments..

alesstimec · 2025-01-09T12:55:02Z

internal/jimm/cloud.go

+	return jCl, nil
+}
+
+func (j *JIMM) GetClouds(ctx context.Context, user *openfga.User) (map[string]jujuparams.Cloud, error) {


godoc, please

alesstimec · 2025-01-09T12:57:14Z

internal/jimm/cloud.go

+	}, nil
+}
+
+func (j *JIMM) ListCloudsInfo(ctx context.Context, user *openfga.User, all bool) ([]jujuparams.ListCloudInfoResult, error) {


godoc, please

alesstimec · 2025-01-09T13:33:21Z

internal/jimmhttp/auth_handler_test.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.


alesstimec · 2025-01-09T13:33:33Z

internal/jimmhttp/rebac_admin/identities_integration_test.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.


alesstimec · 2025-01-09T13:34:21Z

cmd/jimmctl/cmd/purge_logs_test.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.


new line, please

alesstimec · 2025-01-09T13:35:38Z

internal/jimm/cloud.go

-		return cl, errors.E(op, err)
+		return jujuparams.CloudInfo{}, errors.E(op, err)
+	}
+	// TODO (SimoneDutto): refactor this to use `user.IsCloudAdmin()`


where do we require admin access to the cloud?

kian99

I like how it's slimmed things but I'm a bit hesitant on performance, even though it's not a huge deal. I'm wondering if we want/need to slim down clouds. The gains seem marginal but curious to hear what you think after doing this.

kian99 · 2025-01-09T13:43:03Z

internal/db/resource_test.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.
 package db_test


kian99 · 2025-01-09T13:43:22Z

internal/dbmodel/cloud.go

@@ -26,29 +26,8 @@ type Cloud struct {
 	// cloud is hosted.
 	HostCloudRegion string


Why do we need to keep this?

kian99 · 2025-01-09T13:44:06Z

internal/dbmodel/sql/postgres/019_simplify_cloud_table.up.sql

@@ -0,0 +1,3 @@
+-- remove non essential fields from cloud.
+ALTER TABLE clouds  DROP COLUMN auth_types, DROP COLUMN endpoint, DROP COLUMN identity_endpoint, 


nit: add a space after the comment

kian99 · 2025-01-09T13:46:08Z

internal/jimm/cloud.go

@@ -20,35 +20,173 @@ import (
 	ofganames "github.com/canonical/jimm/v3/internal/openfga/names"
 )

+// GetUserCloudAccess returns users access level for the specified cloud.
+func (j *JIMM) GetUserCloudAccess(ctx context.Context, user *openfga.User, cloud names.CloudTag) (string, error) {


I think there is already a method in the permission manager that does this. Can you use that instead?

kian99 · 2025-01-09T13:46:37Z

internal/jimm/cloud.go

+	if err := j.Database.GetCloud(ctx, &cl); err != nil {
+		return jujuparams.Cloud{}, errors.E(op, err)
+	}
+	// TODO (SimoneDutto): refactor this to use `user.IsCloudAdmin()`


Can we do that now?

kian99 · 2025-01-09T13:47:55Z

internal/jimm/cloud.go

+// contain the authentcated user.
+func (j *JIMM) GetCloud(ctx context.Context, user *openfga.User, tag names.CloudTag) (jujuparams.Cloud, error) {
+	const op = errors.Op("jimm.CloudInfo")
+	zapctx.Info(ctx, string(op))


Was thinking about this sometime last year and whether we want it. If we do want it then it might be better suited to go inside the jujuapi.FindMethod() class of functions so that we don't clutter up these functions.

kian99 · 2025-01-09T13:48:36Z

internal/jimm/cloud.go

+	if err != nil {
+		return jCl, errors.E(op, err)
+	}
+	jCl.IsControllerCloud = false // jimm doesn't know where is deployed, so none of the clouds should have this field set.


Suggested change

jCl.IsControllerCloud = false // jimm doesn't know where is deployed, so none of the clouds should have this field set.

jCl.IsControllerCloud = false // jimm doesn't know where it's deployed, so none of the clouds should have this field set.

kian99 · 2025-01-09T13:52:34Z

internal/jimm/cloud.go

+	for _, cl := range clouds {
+		cloud, err := j.getCloudFromController(ctx, cl)
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		results[cl.ResourceTag().String()] = cloud
+	}


I have a concern that this will be annoyingly slow. Not so slow that it's bad but since getting a cloud requires a sequential call to a controller... I've experienced running juju <command> to take several seconds sometimes so I hope we don't have that issue here.
Some caching in the future will of course help and then be similar in a way to what the database was doing.

kian99 · 2025-01-09T13:54:59Z

internal/jimm/cloud.go

+			controllers = append(controllers, c.Controller)
+		}
+	}
+	err := j.firstSuccessfulController(ctx, controllers, func(api API) error {


Here we will always be calling the first controller in the list most frequently. We could randomise the list to do some round-robin if we want.

kian99 · 2025-01-09T13:57:34Z

local/seed_db/main.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.


kian99 · 2025-01-10T09:18:16Z

Points we discussed on this:

Cloud fields are static and we don't get much gain here versus models where we were able to get rid of the watcher.
This might be annoyingly slow if latency to controllers is high and we need to list all clouds.
Returning the cloud info from the first controller that matches might not be accurate if different controllers are aware of different regions.
Making this more performant would introduce a cache and mirror what we're currently doing with the database.

SimoneDutto · 2025-01-10T09:34:00Z

Points we discussed on this:

Cloud fields are static and we don't get much gain here versus models where we were able to get rid of the watcher.

This might be annoyingly slow if latency to controllers is high and we need to list all clouds.

Returning the cloud info from the first controller that matches might not be accurate if different controllers are aware of different regions.

Making this more performant would introduce a cache and mirror what we're currently doing with the database.

After the discussion the decision is to close this pr. Moving forward we've decided not to remove from our db static fields.

SimoneDutto requested a review from a team as a code owner January 8, 2025 13:51

SimoneDutto requested a review from kian99 January 8, 2025 13:52

SimoneDutto force-pushed the JUJU-7135/slim-clouds branch 6 times, most recently from 8cc9a90 to 65b53fa Compare January 9, 2025 10:50

slim clouds

6f18463

SimoneDutto force-pushed the JUJU-7135/slim-clouds branch from 65b53fa to 6f18463 Compare January 9, 2025 12:47

alesstimec approved these changes Jan 9, 2025

View reviewed changes

kian99 reviewed Jan 9, 2025

View reviewed changes

SimoneDutto closed this Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

slim clouds #1512

slim clouds #1512

SimoneDutto commented Jan 8, 2025 •

edited

Loading

alesstimec left a comment

alesstimec Jan 9, 2025

alesstimec Jan 9, 2025

alesstimec Jan 9, 2025

alesstimec Jan 9, 2025

alesstimec Jan 9, 2025

alesstimec Jan 9, 2025

kian99 left a comment

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 Jan 9, 2025

kian99 commented Jan 10, 2025

SimoneDutto commented Jan 10, 2025

		@@ -1,4 +1,4 @@
		// Copyright 2024 Canonical.
		// Copyright 2025 Canonical.

		@@ -26,29 +26,8 @@ type Cloud struct {
		// cloud is hosted.
		HostCloudRegion string

		@@ -0,0 +1,3 @@
		-- remove non essential fields from cloud.
		ALTER TABLE clouds DROP COLUMN auth_types, DROP COLUMN endpoint, DROP COLUMN identity_endpoint,

	jCl.IsControllerCloud = false // jimm doesn't know where is deployed, so none of the clouds should have this field set.
	jCl.IsControllerCloud = false // jimm doesn't know where it's deployed, so none of the clouds should have this field set.

slim clouds #1512

slim clouds #1512

Conversation

SimoneDutto commented Jan 8, 2025 • edited Loading

Description

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

kian99 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

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 Jan 10, 2025

SimoneDutto commented Jan 10, 2025

SimoneDutto commented Jan 8, 2025 •

edited

Loading