refactor(internal/jimm/jimm.go): introduces a jimm constructor #1468

alesstimec · 2024-11-26T15:37:30Z

Description

Introduces a constructor for jimm in internal/jimm. With the lightest touch possible atm.

drive-by fix: the db.Database type holds a migrated (uint32) field. however we were passing the database around by value and every time we copied it, whoever tried to use it would get a CodeUpgradeInProgress error. In this PR we pass a pointer to the database around, hopefully avoiding further migration voes.

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

Test instructions

Notes for code reviewers

cmd/jimmsrv/service/service.go

internal/jimm/jimm.go

SimoneDutto · 2024-11-28T08:30:55Z

internal/jimm/jimm.go

+// connections therefore the JIMM object itself does not contain any per-
+// request state.
+type JIMM struct {
+	Parameters


since we are embedding the struct, it's not Parameters but more Services. Because we don't start the service in the constructor, we rather take the service reference and store it in the JIMM struct.

constructors have parameters. and the constructor will then instantiate all the managers/service provided by JIMM.

I wouldn't embed it, params you pass into the func, and on JIMM it filles in another struct. Don't couple them please?

They're only embedded for the time being. Once we have the domain objects, we can delete this.

SimoneDutto · 2024-11-28T08:33:37Z

internal/jimm/jimm.go

+func New(p Parameters, options ...Option) (*JIMM, error) {
+	j := &JIMM{
+		Parameters: p,
+	}
+
+	if err := j.Database.Migrate(context.Background(), false); err != nil {
+		return nil, errors.E(err)
+	}
+
+	roleManager, err := role.NewRoleManager(j.Database, p.OpenFGAClient)
+	if err != nil {
+		return nil, err
+	}
+	j.RoleManager = roleManager
+	for _, option := range options {
+		option(j)
+	}
+
+	return j, nil
+}


i thought this method would have been a method where you receive some parameters (ex. db connection string), and you create the Database, Openfgaclient.
So we'll end up with a central point where we validate jimm's config, start the jimm services and return the well defined jimm struct.

I think it's correct like this, the JIMM method sets up the business logic, so when we have more "managers", the JIMM struct won't need any of the fields in Params, it will just take the pieces of params like the DB, OpenFGA client, pubSub, etc and pass them to the relevant constructors for the managers. Whoever calls jimm.New() decides what they want to use as a DB, as an auth service, etc (even if you can't mock the DB or auth service today).

kian99

Looks good with some comments

cmd/jimmsrv/service/service.go

internal/jimm/admin_test.go

internal/jimm/cloud.go

internal/jimm/jimm.go

github-actions · 2024-11-28T15:03:25Z

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

cmd/jimmsrv/service/service.go

kian99 · 2024-12-03T08:34:08Z

cmd/jimmsrv/service/service.go

-	}
-
-	sqlDb, err := s.jimm.Database.DB.DB()
+	sqlDb, err := db.DB.DB()


Can we improve this at the moment?

improve how?

For a separate PR, and not for you per se, but I was just remarking that the naming isn't great. database.database.database() is crazy.

cmd/jimmsrv/service/service_test.go

internal/jimm/jimm.go

cmd/jimmsrv/service/service.go

SimoneDutto

few comments then LGTM

cmd/jimmsrv/service/service.go

SimoneDutto · 2024-12-03T11:58:25Z

cmd/jimmsrv/service/service.go


+	jimmParameters := jimm.Parameters{


i would still call the services. But I won't die on this hill

internal/jimm/jimm.go

ale8k · 2024-12-05T08:00:20Z

internal/jimm/jimm.go

+		return errors.E("missing jwks service")
+	}
+
+	if p.JWTService == nil {


I imagine this should only contain primitives? Not Serices?

ale8k · 2024-12-05T08:02:20Z

internal/jimm/jimm.go

+// connections therefore the JIMM object itself does not contain any per-
+// request state.
+type JIMM struct {
+	Parameters


I wouldn't embed it, params you pass into the func, and on JIMM it filles in another struct. Don't couple them please?

- adds jimm.New costructor - adds jimmtest.NewJIMM - refactors GetRoleManager and GetGroupManager

internal/jimm/access_test.go

SimoneDutto · 2024-12-05T10:58:15Z

internal/testutils/jimmtest/jimm.go

+	if p.Database == nil {
+		p.Database = &db.Database{
+			DB: PostgresDB(t, func() time.Time { return now }),
+		}
+	}
+	if p.CredentialStore == nil {
+		p.CredentialStore = p.Database
+	}
+	if p.OpenFGAClient == nil {
+		ofgaClient, _, _, err := SetupTestOFGAClient(t.Name())
+		if err != nil {
+			t.Fatalf("setting up openfga client: %v", err)
+		}
+		p.OpenFGAClient = ofgaClient


so, this is the default config for jimmtest now. Update the godoc please

SimoneDutto · 2024-12-05T10:58:22Z

internal/testutils/jimmtest/jimm.go

+	}
+)
+
+func NewJIMM(t Tester, additionalParameters *jimm.Parameters, options ...Option) *jimm.JIMM {


internal/testutils/jimmtest/jimm.go

SimoneDutto

lgtm, but please new time let's split this in multiple pr, with a feature branch.
Too many mechanical changes in the same pr

SimoneDutto · 2024-12-05T10:59:34Z

internal/testutils/jimmtest/jimm.go

+type Option func(j *jimm.JIMM)
+
+var (
+	UnsetCredentialStore Option = func(j *jimm.JIMM) {
+		j.CredentialStore = nil
+	}
+)


if we give default options to choose from, do we need to have the Option exported?

yes. we have a slight bug around cloud credentials. and there is a test that relies on the fact that the credential store is unset.. until we fix that bug, we need this option

SimoneDutto · 2024-12-05T11:02:23Z

internal/testutils/jimmtest/suite.go

@@ -47,7 +46,11 @@ func (t GocheckTester) Name() string {
 }

 func (t GocheckTester) Cleanup(f func()) {
-	t.C.Logf("warning: gocheck does not support Cleanup functions; make sure you're using suite's tear-down method")
+	if t.AddCleanup != nil {
+		t.AddCleanup(f)


why bother with adding the cleanup? Since we don't have any cleanup with all the ŧesting.t tests.

I think it's just enough to increase the conn limit on postgres

btw, where did the change go? Or now it's fine because we are not creating that many connections?

https://github.com/canonical/jimm/pull/1468/files#diff-3fde9d1a396e140fefc7676e1bd237d67b6864552b6f45af1ebcc27bcd0bb6e9R73

I think this link stops working when you force push btw.

kian99

I think it's almost okay. My biggest gripe is with all tests getting a DB and OpenFGA client.

kian99 · 2024-12-05T12:13:34Z

cmd/jimmsrv/service/service.go

-	}
-
-	sqlDb, err := s.jimm.Database.DB.DB()
+	sqlDb, err := db.DB.DB()


For a separate PR, and not for you per se, but I was just remarking that the naming isn't great. database.database.database() is crazy.

kian99 · 2024-12-05T12:16:19Z

docker-compose.yaml

@@ -70,7 +70,7 @@ services:
      POSTGRES_PASSWORD: jimm
    # Since it's mainly used for testing purposes, it's okay to set fsync=off for
    # improved performance.
-    command: -c fsync=off -c full_page_writes=off
+    command: -c fsync=off -c full_page_writes=off -c max_connections=200


This is a bit concerning. I wonder if we're going over the default 100 connections because we're leaking connections or because of tests running in parallel. Perhaps one way to check would be to add a global test cleanup methods that checks the number of open connections to Postgres. Or run tests with -p 1 and print the number of connections to Postgres between each test incovation.

kian99 · 2024-12-05T12:20:48Z

internal/jimm/jimm.go

+// connections therefore the JIMM object itself does not contain any per-
+// request state.
+type JIMM struct {
+	Parameters


They're only embedded for the time being. Once we have the domain objects, we can delete this.

kian99 · 2024-12-05T12:24:53Z

internal/testutils/jimmtest/jimm.go

+
+	if p.Database == nil {
+		p.Database = &db.Database{
+			DB: PostgresDB(t, func() time.Time { return now }),


I'm currently not in favour of this approach where every test that calls jimmtest.NewJIMM gets a whole database and OpenFGA store. Some tests need neither, some tests need one. That was nicely captured in the tests and when we move to managers we can use suites so that there is less duplication of that boilerplate test setup.

I think the default should be all mocks or all nil fields unless the test requests them.

kian99 · 2024-12-05T12:27:28Z

internal/testutils/jimmtest/suite.go

@@ -172,14 +190,14 @@ func (s *JIMMSuite) TearDownTest(c *gc.C) {
 	}
 }

-func (s *JIMMSuite) setupMacaroonDischarger(c *gc.C) *discharger.MacaroonDischarger {
+func setupMacaroonDischarger(c *gc.C, uuid string, db *db.Database, ofgaClient *openfga.OFGAClient) *discharger.MacaroonDischarger {


Why was this changed to a function?

ale8k · 2024-12-05T11:25:48Z

cmd/jimmsrv/service/service.go

 	}

 	if _, err := url.Parse(p.DashboardFinalRedirectURL); err != nil {
 		return nil, errors.E(op, err, "failed to parse final redirect url for the dashboard")
 	}

-	rebacBackend, err := rebac_admin.SetupBackend(ctx, &s.jimm)
+	// instantiate jimm
+	s.jimm, err = jimm.New(jimmParameters)


Happy with this as is, as we discussed, but we should definitely break this down further into primitives and application domains later on. I'd love to see primitive db, openfga, authsvc (as params) -> and within the constructor: rolemanager, identityservice, etc. all constructed returning a nicely complete jimm. And, if we want to override any application domain we pass an option.

ale8k · 2024-12-05T11:27:42Z

cmd/jimmsrv/service/service.go

 	)

+	if p.AuditLogRetentionPeriodInDays != "" {


I feel like we should have service params honestly, it's confusing / bit odd, to see params which re for both jimm and the service and they take effect when you start the service. @kian99 I think you mentioned this right?

ale8k · 2024-12-05T11:32:17Z

cmd/jimmsrv/service/service.go

 	return s, nil
 }

+func (s *Service) StartServices(ctx context.Context, svc *service.Service) {
+	// on the leader unit we start additional routines


I do like how we start all services here, but in main.go we're running this and then we start the jimm service. Would be nice if jimm was also started here? Could even setup the on shutdown etc all here.

ale8k · 2024-12-05T11:33:40Z

internal/db/cloud_test.go

@@ -244,7 +244,7 @@ controllers:
    region: test-region
    priority: 1
 `)
-	env.PopulateDB(c, *s.Database)
+	env.PopulateDB(c, s.Database)


Interesting we could handle ** here. Good spot.

ale8k · 2024-12-05T12:38:30Z

internal/testutils/jimmtest/jimm.go

+
+var now = (time.Time{}).UTC().Round(time.Millisecond)
+
+type Option func(j *jimm.JIMM)


I see, we use option pattern only in the context of tests... I think... It's more useful the have the pattern on the JIMM constructor for when we come to (or not to) initialise services. Kinda like juju's manifolds where they configure the binary in different ways - if we ever need to go down that path we can. It's also no harm I think rather than having the option pattern only apply to the jimmtest constructor, I think it's a lot clearer if we just use jimm.New() and pass test options.

nope.. in production we never expect to use options.. only for tests, so why pollute production code with options?

ale8k

lgtm, I've left some comments. Nothing too serious, but more about options and service. I'd much prefer we just use a single constructor for initialisng jimm in tests and in main.

alesstimec requested a review from a team as a code owner November 26, 2024 15:37

alesstimec force-pushed the internal-jimm-contructor branch 5 times, most recently from a730c54 to b910d95 Compare November 27, 2024 14:49

SimoneDutto reviewed Nov 28, 2024

View reviewed changes

kian99 reviewed Nov 28, 2024

View reviewed changes

alesstimec force-pushed the internal-jimm-contructor branch 2 times, most recently from f0bc5ed to cd1f2a2 Compare November 28, 2024 15:03

alesstimec force-pushed the internal-jimm-contructor branch 3 times, most recently from abc2bf1 to 30ce643 Compare December 3, 2024 07:53

kian99 reviewed Dec 3, 2024

View reviewed changes

SimoneDutto reviewed Dec 3, 2024

View reviewed changes

alesstimec force-pushed the internal-jimm-contructor branch 10 times, most recently from f0985ed to fbe23f4 Compare December 5, 2024 08:17

ale8k reviewed Dec 5, 2024

View reviewed changes

alesstimec force-pushed the internal-jimm-contructor branch from 40f3c7d to 61fea0d Compare December 5, 2024 09:51

refactor(internal/jimm/jimm.go): introduces a jimm constructor

8b40a37

- adds jimm.New costructor - adds jimmtest.NewJIMM - refactors GetRoleManager and GetGroupManager

alesstimec force-pushed the internal-jimm-contructor branch from 61fea0d to 8b40a37 Compare December 5, 2024 10:22

SimoneDutto reviewed Dec 5, 2024

View reviewed changes

internal/testutils/jimmtest/jimm.go Show resolved Hide resolved

SimoneDutto approved these changes Dec 5, 2024

View reviewed changes

kian99 reviewed Dec 5, 2024

View reviewed changes

ale8k approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'v3' into internal-jimm-contructor

313ec00

alesstimec merged commit 4b9eaf2 into canonical:v3 Dec 6, 2024
4 checks passed


		var now = (time.Time{}).UTC().Round(time.Millisecond)

		type Option func(j *jimm.JIMM)

refactor(internal/jimm/jimm.go): introduces a jimm constructor #1468

refactor(internal/jimm/jimm.go): introduces a jimm constructor #1468

Conversation

alesstimec commented Nov 26, 2024 • edited Loading

Description

Engineering checklist

Test instructions

Notes for code reviewers

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

github-actions bot commented Nov 28, 2024

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 • edited Loading

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

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

Choose a reason for hiding this comment

ale8k left a comment

Choose a reason for hiding this comment

alesstimec commented Nov 26, 2024 •

edited

Loading

SimoneDutto left a comment •

edited

Loading