Add Datastore layer #73

maxlambrecht · 2022-12-15T22:25:02Z

Signed-off-by: Max Lambrecht [email protected]

Pull request check list

Proper tests/regressions included?
Documentation updated?

Affected functionality
Added a new Datastore implementation that supports Postgres

Description of change

This PR adds migration SQLs to create the schema based on the new data model, and also adds the SQL Queries.
The interfaces, model structs, and lower level DB operations were auto generated using sqlc which uses the SQL files from migrations and queries as input.
A new Datastore struct was added, that defines CRUD methods for all the entities of Galadriel.
When the Datastore is created and it connects to the configured DB, the schema is validated and migrations applied if needed, using golang-migrate.
Tests for the Datastore were implemented using dockertest, which starts a container from a Postgres image, runs the migrations, and then run the test.

Which issue this pull requests fixes

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi

Great stuff! Did a first quick pass-through and got stuck thinking into two different things:

On a more high-level discussion, do you still see we having one interface to abstract the datastore layer, in which each of the different implementations (postgres, mysql, etc) will have to satisfy? I couldn't find that interface anymore, but the Datastore object looks like it would implement this interface.

and the 2nd topic is related to using a connection pool, can be discussed in the comment below.

jufantozzi · 2022-12-19T13:52:29Z

db/schema.go

+)
+
+//go:embed migrations/*.sql
+var fs embed.FS


this is new to me :D

Embed go feature allows embedding files in compile time. However, some errors can only be caught in runtime. (ie. If a SQL file is not well formatted or even when there is no SQL in an empty file). Another similar feature is go:generate which allows running code at compile time. Take a look:

https://blog.carlmjohnson.net/post/2016-11-27-how-to-use-go-generate/

https://blog.carlmjohnson.net/post/2021/how-to-use-go-embed/

The embedded SQLs (for migrations) are being exercised in the CRUD tests.

jufantozzi · 2022-12-19T14:04:38Z

db/datastore.go

+		return nil, fmt.Errorf("failed to parse Postgres Connection URL: %w", err)
+	}
+
+	db := stdlib.OpenDB(*c)


Just to confirm: this does not use a connection pool, does it?

This does use a connection pool:

// DB is a database handle representing a pool of zero or more // underlying connections. It's safe for concurrent use by multiple // goroutines.

Victorblsilveira

Amazing work!!! See if my comments make sense

db/datastore.go

db/datastore_test.go

Victorblsilveira · 2022-12-19T18:39:15Z

Great stuff! Did a first quick pass-through and got stuck thinking into two different things:

On a more high-level discussion, do you still see we having one interface to abstract the datastore layer, in which each of the different implementations (postgres, mysql, etc) will have to satisfy? I couldn't find that interface anymore, but the Datastore object looks like it would implement this interface.

and the 2nd topic is related to using a connection pool, can be discussed in the comment below.

About the abstract datastore layer, I think that is this one: querier.go

Makefile

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi · 2022-12-20T18:24:07Z

db/migrations/1_initialize_schema.up.sql

+    updated_at    TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now()
+);
+
+CREATE TABLE join_tokens


Will join tokens not be related to harvesters in any way? Thinking about HA, does it matter if we're able to validate if the join token was for a specific harvester?

I know this has some impacts we would also have to think about, like how to deal when a harvester is deleted. This specifically is also something I was thinking for a bit and considering: wouldn't it be a better idea to instead of deleting harvesters to just disable them? is this data really not worth keeping?

According to our data model, there is an implicit relation between join_token and harvesters through members relation, but not an explicit one. Would be enough for the validation to know that the join token is for a member that is related to the harvester, or you are thinking of a more restricted validation in which the token needs to be exactly issued for a harvester?

Would be enough for the validation to know that the join token is for a member that is related to the harvester?

As I understand it so far, I think it would be enough for the validation knowing which member a token belongs to, whether it was used or not, and whether it didn't expire.

According to our data model, there is an implicit relation between join_token and harvesters through members relation, but not an explicit one

my bad on the wording, that's what I actually meant 😅

maxlambrecht · 2022-12-20T19:41:15Z

Great stuff! Did a first quick pass-through and got stuck thinking into two different things:
On a more high-level discussion, do you still see we having one interface to abstract the datastore layer, in which each of the different implementations (postgres, mysql, etc) will have to satisfy? I couldn't find that interface anymore, but the Datastore object looks like it would implement this interface.
and the 2nd topic is related to using a connection pool, can be discussed in the comment below.

About the abstract datastore layer, I think that is this one: querier.go

We could still abstract the Datastore layer in an interface that uses entity objects from the API. I didn't think about that because this PR introduces a Datastore that just supports one dialect, and I still don't know if we'll need that abstraction anyway, even in the case of supporting multiple dialects.
The querier interface is auto-generated by the sqlc and it's a lower level interface that uses DB models.

jufantozzi · 2022-12-21T11:55:22Z

We could still abstract the Datastore layer in an interface that uses entity objects from the API. I didn't think about that because this PR introduces a Datastore that just supports one dialect, and I still don't know if we'll need that abstraction anyway, even in the case of supporting multiple dialects.
The querier interface is auto-generated by the sqlc and it's a lower level interface that uses DB models.

Right, it makes sense. Yes, the querier interface looks good enough, at least for now. Although this higher level abstraction will still be implemented in some form, (at the handlers, most likely?) by having an interface we're better segregating this logic for our handlers. Anyways, not something for right now

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi

LGTM!! Btw, you're not planning on incorporating these changes into galadriel on this PR, right? If not, I'm ready to approve.

maxlambrecht · 2023-01-11T20:48:43Z

LGTM!! Btw, you're not planning on incorporating these changes into galadriel on this PR, right? If not, I'm ready to approve.

No need to approve just yet, I'm expecting more feedback from the rest of the team.

jufantozzi · 2023-01-11T20:54:50Z

Btw, you're not planning on incorporating these changes into galadriel on this PR, right?

Just realized my last question was a bit ambiguous, I wanted to ask about replacing the current In memory datastore to use this implementation. This should happen in a separate PR right?

maxlambrecht · 2023-01-12T12:55:20Z

Btw, you're not planning on incorporating these changes into galadriel on this PR, right?

Just realized my last question was a bit ambiguous, I wanted to ask about replacing the current In memory datastore to use this implementation. This should happen in a separate PR right?

IMO migrating Galadriel to the new Datastore is not worth doing right now, since the whole implementation of Galadriel will be mostly re-visited while moving from the POC to the MVP. Each new (re)implemented functionality in Galadriel Server will need to use the new Datastore.

Signed-off-by: Max Lambrecht <[email protected]>

pkg/server/endpoints/management.go

 		e.handleError(w, errMsg)
 		return
 	}

-	e.Logger.Printf("Created member for trust domain: %s", memberReq.TrustDomain)
+	e.Logger.Printf("Created trustDomain for trust domain: %s", trustDomainReq.Name)


pkg/server/endpoints/management.go

Signed-off-by: Max Lambrecht <[email protected]>

pkg/server/endpoints/harvester.go

+			return err
+		}
+
+		e.Logger.Debugf("Trust domain %s has been successfully updated", harvesterReq.TrustDomainName)


Signed-off-by: Max Lambrecht <[email protected]>

pkg/server/endpoints/management.go

@@ -215,11 +283,15 @@
 	e.Logger.Errorf(errMsg)

 	errBytes := []byte(errMsg)
-	w.WriteHeader(len(errBytes))
+	w.WriteHeader(500)
+	_, err := w.Write(errBytes)


jufantozzi

Amazing work @maxlambrecht ❤️

Did a code pass-through and it looks impeccable, I'm just not approving it now cause I didn't have time to test yet.

One topic that came to my mind though, is that I noticed we're not supporting TLS for the postgres connection. IDK if this is something needed for now, I don't think we've discussed this yet, but I'm pretty sure we'll have to think about this at some point. WDYT?

pkg/server/datastore/README.md

jufantozzi · 2023-02-27T21:02:49Z

pkg/server/endpoints/harvester.go

-	if receivedHarvesterState.TrustDomain.Compare(token.TrustDomain) != 0 {
-		err := fmt.Errorf("authenticated trust domain {%s} does not match received trust domain {%s}", receivedHarvesterState.TrustDomain.String(), token.TrustDomain.String())
-
+	if harvesterReq.TrustDomainName != authenticatedTD.Name {


Just out of curiosity, was this change due to readability?

Yes, IMO it reads better that way.

jufantozzi · 2023-02-27T21:12:37Z

pkg/server/endpoints/harvester.go

+			return nil, nil, err
+		}
+
+		if b != nil {


Wondering if we might want to have a debug(?) log in case this is nil

maxlambrecht · 2023-02-27T21:41:22Z

Amazing work @maxlambrecht ❤️

Did a code pass-through and it looks impeccable, I'm just not approving it now cause I didn't have time to test yet.

One topic that came to my mind though, is that I noticed we're not supporting TLS for the postgres connection. IDK if this is something needed for now, I don't think we've discussed this yet, but I'm pretty sure we'll have to think about this at some point. WDYT?

Thanks Juliano! ❤️

The Postgres connection string allows TLS, something like:

"sslmode=require sslrootcert=server-ca.pem sslcert=client-cert.pem sslkey=client-key.pem hostaddr=your_host port=5432 user=your_user dbname=your_db"

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi

💯 💯

mgbcaio

this is a number 10 work, right here. Nice job, Max 🏅 Ready to approve as soon as others take a look into this.

Victorblsilveira

Hey @maxlambrecht, great work here.
A lot of changes, most of my comments are about naming.
Don't feel obliged to accept it, but let me know what you think about it.
Feel free to ignore them as well.

pkg/server/endpoints/harvester.go

pkg/server/endpoints/management.go

Signed-off-by: Max Lambrecht <[email protected]>

wibarre

Just focused on the pkg/server/datastore. Thank you Maxl for this. Looks great!

pkg/server/datastore/README.md

pkg/server/datastore/datastore.go

pkg/server/datastore/datastore_test.go

pkg/server/datastore/migrations/1_initialize_schema.down.sql

Signed-off-by: Max Lambrecht <[email protected]>

mchurichi

This is looking great, Max! 💯

cmd/server/cli/create.go

cmd/server/cli/list.go

pkg/server/datastore/datastore.go

pkg/server/datastore/migrations/1_initialize_schema.down.sql

pkg/server/datastore/migrations/1_initialize_schema.up.sql

pkg/server/endpoints/harvester.go

demo/README.md

Signed-off-by: Max Lambrecht <[email protected]>

sonarqubecloud · 2023-03-01T17:31:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
1.2% Duplication

mchurichi

🌭

Add datastore layer

128b9ba

Signed-off-by: Max Lambrecht <[email protected]>

maxlambrecht requested review from wibarre, mchurichi, jufantozzi, mgbcaio and Victorblsilveira as code owners December 15, 2022 22:25

maxlambrecht marked this pull request as draft December 15, 2022 22:25

maxlambrecht changed the title ~~Add Datastore layer~~ [DRAFT] Add Datastore layer Dec 15, 2022

Add unique constraint to membership table

fddd152

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi reviewed Dec 19, 2022

View reviewed changes

Victorblsilveira reviewed Dec 19, 2022

View reviewed changes

db/datastore.go Outdated Show resolved Hide resolved

db/datastore.go Outdated Show resolved Hide resolved

db/datastore_test.go Outdated Show resolved Hide resolved

jufantozzi reviewed Dec 20, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Max Lambrecht added 2 commits December 20, 2022 11:50

Fix typo

1d34267

Signed-off-by: Max Lambrecht <[email protected]>

Fix grammar

2b3f6fe

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi reviewed Dec 20, 2022

View reviewed changes

Refactor datastore methods

1010635

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi reviewed Jan 11, 2023

View reviewed changes

Merge branch 'main' into feature/datastore

a31cf6e

maxlambrecht marked this pull request as ready for review January 17, 2023 18:08

maxlambrecht changed the title ~~[DRAFT] Add Datastore layer~~ Add Datastore layer Jan 17, 2023

Max Lambrecht added 4 commits January 17, 2023 12:11

Add datastore layer

9e294cf

Signed-off-by: Max Lambrecht <[email protected]>

Add unique constraint to membership table

96e9bd7

Signed-off-by: Max Lambrecht <[email protected]>

Fix typo

8c6949e

Signed-off-by: Max Lambrecht <[email protected]>

Fix grammar

0c21cb1

Signed-off-by: Max Lambrecht <[email protected]>

maxlambrecht changed the title ~~[DRAFT] Add Datastore layer~~ Add Datastore layer Feb 27, 2023

maxlambrecht marked this pull request as ready for review February 27, 2023 14:48

github-advanced-security bot found potential problems Feb 27, 2023

View reviewed changes

Max Lambrecht added 2 commits February 27, 2023 10:02

Amend incorrect word

a61c275

Signed-off-by: Max Lambrecht <[email protected]>

Amend logs

b8490b1

Signed-off-by: Max Lambrecht <[email protected]>

github-advanced-security bot found potential problems Feb 27, 2023

View reviewed changes

pkg/server/endpoints/harvester.go

return err

}

e.Logger.Debugf("Trust domain %s has been successfully updated", harvesterReq.TrustDomainName)

Check failure

Code scanning / CodeQL

Log entries created from user input

This log write receives unsanitized user input from [here](1).

Fix lint issues

ced0583

Signed-off-by: Max Lambrecht <[email protected]>

github-advanced-security bot found potential problems Feb 27, 2023

View reviewed changes

jufantozzi reviewed Feb 27, 2023

View reviewed changes

Fix link

b7c2ecd

Signed-off-by: Max Lambrecht <[email protected]>

jufantozzi previously approved these changes Feb 27, 2023

View reviewed changes

mgbcaio reviewed Feb 28, 2023

View reviewed changes

Victorblsilveira previously approved these changes Feb 28, 2023

View reviewed changes

pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved

pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved

pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved

pkg/server/endpoints/management.go Outdated Show resolved Hide resolved

Max Lambrecht added 2 commits February 28, 2023 08:58

Merge remote-tracking branch 'upstream/main' into feature/datastore

2a34d77

Addressing PR comments

64d4d07

Signed-off-by: Max Lambrecht <[email protected]>

maxlambrecht dismissed stale reviews from Victorblsilveira and jufantozzi via 64d4d07 February 28, 2023 15:00

wibarre reviewed Feb 28, 2023

View reviewed changes

Max Lambrecht added 4 commits February 28, 2023 12:07

Addressing some linter errors

0b59ca5

Signed-off-by: Max Lambrecht <[email protected]>

Addressing some linter errors

65d2d21

Signed-off-by: Max Lambrecht <[email protected]>

Amend table name in sql

37b9d21

Signed-off-by: Max Lambrecht <[email protected]>

Exclude datastore_test from sonarcloud

355b56a

Signed-off-by: Max Lambrecht <[email protected]>

mchurichi reviewed Mar 1, 2023

View reviewed changes

Address PR comments

d131168

Signed-off-by: Max Lambrecht <[email protected]>

mchurichi approved these changes Mar 1, 2023

View reviewed changes

jufantozzi approved these changes Mar 1, 2023

View reviewed changes

maxlambrecht merged commit fe91db8 into HewlettPackard:main Mar 1, 2023

maxlambrecht deleted the feature/datastore branch March 1, 2023 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Datastore layer #73

Add Datastore layer #73

maxlambrecht commented Dec 15, 2022 •

edited

Loading

jufantozzi left a comment •

edited

Loading

jufantozzi Dec 19, 2022

Victorblsilveira Dec 20, 2022

maxlambrecht Dec 20, 2022

jufantozzi Dec 19, 2022

maxlambrecht Dec 20, 2022

Victorblsilveira left a comment

Victorblsilveira commented Dec 19, 2022

jufantozzi Dec 20, 2022

jufantozzi Dec 20, 2022

Victorblsilveira Dec 20, 2022

maxlambrecht Dec 20, 2022 •

edited

Loading

jufantozzi Dec 20, 2022

maxlambrecht commented Dec 20, 2022

jufantozzi commented Dec 21, 2022

jufantozzi left a comment

maxlambrecht commented Jan 11, 2023

jufantozzi commented Jan 11, 2023

maxlambrecht commented Jan 12, 2023

jufantozzi left a comment

jufantozzi Feb 27, 2023

maxlambrecht Feb 27, 2023

jufantozzi Feb 27, 2023 •

edited

Loading

maxlambrecht commented Feb 27, 2023

jufantozzi left a comment

mgbcaio left a comment

Victorblsilveira left a comment

wibarre left a comment

mchurichi left a comment

sonarqubecloud bot commented Mar 1, 2023

mchurichi left a comment

Add Datastore layer #73

Add Datastore layer #73

Conversation

maxlambrecht commented Dec 15, 2022 • edited Loading

jufantozzi 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

Victorblsilveira left a comment

Choose a reason for hiding this comment

Victorblsilveira commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlambrecht Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlambrecht commented Dec 20, 2022

jufantozzi commented Dec 21, 2022

jufantozzi left a comment

Choose a reason for hiding this comment

maxlambrecht commented Jan 11, 2023

jufantozzi commented Jan 11, 2023

maxlambrecht commented Jan 12, 2023

jufantozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jufantozzi Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

maxlambrecht commented Feb 27, 2023

jufantozzi left a comment

Choose a reason for hiding this comment

mgbcaio left a comment

Choose a reason for hiding this comment

Victorblsilveira left a comment

Choose a reason for hiding this comment

wibarre left a comment

Choose a reason for hiding this comment

mchurichi left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Mar 1, 2023

mchurichi left a comment

Choose a reason for hiding this comment

maxlambrecht commented Dec 15, 2022 •

edited

Loading

jufantozzi left a comment •

edited

Loading

maxlambrecht Dec 20, 2022 •

edited

Loading

jufantozzi Feb 27, 2023 •

edited

Loading