Add MySQL storage and tests #5

lucasmrod · 2022-07-21T16:48:44Z

Hi folks.

This PR implements a MySQL storage, issue #4.
I've added tests that can be run on any future storage (which caught a small bug in the FileStorage).

zwass

Stoked to see this up!

http/api/tokenpki.go

storage/mysql/mysql.go

storage/storagetest/storagetest.go

cmd/depsyncer/main.go

jessepeterson

This is an amazing contribution! Thanks so much for this! Here's some quick thoughts and I'll try to get time soon, hopefully early next week for more. :)

Bunch of comments inline. :)

I have some reservations about having an opinionated docker-compose front and center. Let me think more on that (maybe it's as simple as placing the compose file in storage/storagetest/?).

The cmd/cli.go -> parse/storage.go is interesting. What was the thinking there?

Also - don't forget to update the docs (docs/operations-guide.md) for the new storage backend. :)

go.mod

parse/storage.go

jessepeterson · 2022-07-21T20:29:11Z

storage/file/file.go

@@ -20,6 +21,8 @@ type FileStorage struct {
 	path string
 }

+var _ storage.AllStorage = (*FileStorage)(nil)


Curious what's the reason for this?

Convention that serves both as a "compile time check" and as a form of documentation for code readers/reviewers that FileStorage implements storage.AllStorage interface.

I think this creates an unnecessary coupling. How do you feel about removing it? As Go is statically typed an implementation must conform to it's use. AllStorage isn't particularly useful outside of main.go so I'd not like to couple these.

Let me know what you think. :)

jessepeterson · 2022-07-21T20:30:28Z

storage/file/file.go

-	return tokens, decodeJSONfile(s.tokensFilename(name), tokens)
+	err := decodeJSONfile(s.tokensFilename(name), tokens)
+	if err != nil {
+		return nil, err


Any problem with returning a non-nil empty token — most callers are going to check for errors anyway?

A convention I've seen many projects use is to try (when possible) to always return zero-values when error is non-nil. In this case, the zero-value of a pointer is nil.

So I'm fine with this specific code change, although its a bit verbose. But in general I don't think its necessary to add a bunch of boilerplate to assure a zero-values for an error. Discussed here: https://macadmins.slack.com/archives/C07ME1284/p1663867171949419

storage/file/file.go

storage/mysql/mysql.go

storage/mysql/mysql_test.go

storage/storagetest/storagetest.go

lucasmrod · 2022-07-21T23:48:20Z

I believe I've address all the comments.

I have some reservations about having an opinionated docker-compose front and center. Let me think more on that (maybe it's as simple as placing the compose file in storage/storagetest/?).

I've moved it to storage/mysql/. Let me know if that works.

The cmd/cli.go -> parse/storage.go is interesting. What was the thinking there?

Originally I didn't want mysql imports in storage/storage.go (where we define the abstract interface), thus I moved the one parsing method to its own package parse.

Also - don't forget to update the docs (docs/operations-guide.md) for the new storage backend. :)

Done.

I really appreciate the feedback!

jessepeterson

sorry for the unacceptably huge delay! just a few changes. would love your input on the general Go idiomatic/convention stuff — but it sounds like we might disagree on a few small things :). hopefully we can resolve!

jessepeterson · 2022-09-29T18:30:22Z

parse/storage.go

@@ -0,0 +1,28 @@
+package parse


I like the idea of moving this out of the cmd/ tree. But can we rename this package to just cli to match micromdm/nanomdm@ff8b7af?

jessepeterson · 2022-09-29T18:32:23Z

storage/file/file.go

@@ -20,6 +21,8 @@ type FileStorage struct {
 	path string
 }

+var _ storage.AllStorage = (*FileStorage)(nil)


I think this creates an unnecessary coupling. How do you feel about removing it? As Go is statically typed an implementation must conform to it's use. AllStorage isn't particularly useful outside of main.go so I'd not like to couple these.

Let me know what you think. :)

jessepeterson · 2022-09-29T18:34:24Z

storage/file/file.go

-	return tokens, decodeJSONfile(s.tokensFilename(name), tokens)
+	err := decodeJSONfile(s.tokensFilename(name), tokens)
+	if err != nil {
+		return nil, err


So I'm fine with this specific code change, although its a bit verbose. But in general I don't think its necessary to add a bunch of boilerplate to assure a zero-values for an error. Discussed here: https://macadmins.slack.com/archives/C07ME1284/p1663867171949419

jessepeterson · 2022-09-29T18:43:15Z

storage/mysql/mysql.go

+	if err != nil {
+		if errors.Is(err, sql.ErrNoRows) {
+			// an 'empty' config is valid
+			return &client.Config{}, nil


Hmm, good call-out. I think the file-storage should be changed to match actually:

No config (missing) = nil
Present config (including an empty BaseURL) = returned.

They're treated the same in the config retriever interface implementation, as it happens. This should probably be documented there:

nanodep/client/client.go

Lines 23 to 25 in a7538c5

type ConfigRetriever interface {

RetrieveConfig(context.Context, string) (*Config, error)

}

lucasmrod · 2022-10-03T13:04:15Z

Hmm, good call-out. I think the file-storage should be changed to match actually:
No config (missing) = nil
Present config (including an empty BaseURL) = returned.

I've amended the two storage implementations to behave this way, and added a test for DefaultConfigRetreiver.

lucasmrod · 2022-10-03T16:04:37Z

@jessepeterson One last change that can be added to this PR, or open a separate one if this one is merged-as is: mikermcneil#1

Your call.

jessepeterson · 2022-10-03T21:00:41Z

@lucasmrod let's do a separate PR and keep this contained :)

also I think you need to 'request review' or whatever - it's showing as changes still requested :)

thanks again!

lucasmrod · 2022-10-03T22:14:21Z

let's do a separate PR and keep this contained :)

Cool. I'll create another PR once this one is merged to main.

jessepeterson

thanks much for this contribution @lucasmrod !

jessepeterson · 2022-10-03T22:15:38Z

client/client.go

@@ -21,7 +21,11 @@ type Config struct {
 }

 type ConfigRetriever interface {
-	RetrieveConfig(context.Context, string) (*Config, error)
+	// RetrieveConfig reads the JSON DEP config of a DEP name.


Add MySQL storage and tests

cd04701

lucasmrod force-pushed the issue-4-add-mysql-storage-and-tests branch from 0aa680c to cd04701 Compare July 21, 2022 18:07

zwass reviewed Jul 21, 2022

View reviewed changes

http/api/tokenpki.go Outdated Show resolved Hide resolved

storage/mysql/mysql.go Outdated Show resolved Hide resolved

storage/storagetest/storagetest.go Outdated Show resolved Hide resolved

jessepeterson reviewed Jul 21, 2022

View reviewed changes

cmd/depsyncer/main.go Outdated Show resolved Hide resolved

jessepeterson reviewed Jul 21, 2022

View reviewed changes

lucasmrod added 12 commits July 21, 2022 19:15

go mod tidy

e5554fb

Move docker-compose-test.yml file to storage/mysql

37aed61

Fix flag storage default value

4aafd64

Remove defaultDays TODO comment

fa61974

Add mysql storage to docs/operations-guide.md

15a774b

Move mysql imports

f0dd734

Fix spacing

1c988d7

Remove config guard check

478cc3e

Use MySQL 8.0.19 for tests

2ef1eca

Use storage.ErrNotFound

ea941b4

Do not rely on parseTime in dsn

31fced2

Use MySQL 8 syntax for ON DUPLICATE KEY UPDATE

95a8f2b

lucasmrod mentioned this pull request Jul 22, 2022

Allow setting CN and validity days on certificate generation #6

Merged

lucasmrod requested a review from jessepeterson July 25, 2022 11:48

lucasmrod added 3 commits July 29, 2022 06:48

Export schema as Go variable

0a82006

Move RSAKeyFromPEM from package api to tokenpki

21464cb

Format time to string for storage

bb4cb0f

michalnicp force-pushed the issue-4-add-mysql-storage-and-tests branch from 6952196 to bb4cb0f Compare September 19, 2022 17:56

jessepeterson requested changes Sep 29, 2022

View reviewed changes

lucasmrod added 2 commits October 3, 2022 09:57

Address review comments

1bb90ee

Merge branch 'main' into issue-4-add-mysql-storage-and-tests

465df8b

Rename to cli package

63fe581

jessepeterson linked an issue Oct 3, 2022 that may be closed by this pull request

Implement MySQL backend #4

Closed

lucasmrod requested a review from jessepeterson October 3, 2022 21:54

jessepeterson approved these changes Oct 3, 2022

View reviewed changes

jessepeterson merged commit 9288f28 into micromdm:main Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MySQL storage and tests #5

Add MySQL storage and tests #5

lucasmrod commented Jul 21, 2022 •

edited

Loading

zwass left a comment

jessepeterson left a comment

jessepeterson Jul 21, 2022

lucasmrod Jul 21, 2022 •

edited

Loading

jessepeterson Sep 29, 2022

jessepeterson Jul 21, 2022

lucasmrod Jul 21, 2022

jessepeterson Sep 29, 2022

lucasmrod commented Jul 21, 2022 •

edited

Loading

jessepeterson left a comment

jessepeterson Sep 29, 2022

jessepeterson Sep 29, 2022

jessepeterson Sep 29, 2022

jessepeterson Sep 29, 2022

lucasmrod commented Oct 3, 2022

lucasmrod commented Oct 3, 2022 •

edited

Loading

jessepeterson commented Oct 3, 2022

lucasmrod commented Oct 3, 2022

jessepeterson left a comment

jessepeterson Oct 3, 2022

	type ConfigRetriever interface {
	RetrieveConfig(context.Context, string) (*Config, error)
	}

Add MySQL storage and tests #5

Add MySQL storage and tests #5

Conversation

lucasmrod commented Jul 21, 2022 • edited Loading

zwass left a comment

Choose a reason for hiding this comment

jessepeterson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmrod Jul 21, 2022 • 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

lucasmrod commented Jul 21, 2022 • edited Loading

jessepeterson 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

lucasmrod commented Oct 3, 2022

lucasmrod commented Oct 3, 2022 • edited Loading

jessepeterson commented Oct 3, 2022

lucasmrod commented Oct 3, 2022

jessepeterson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmrod commented Jul 21, 2022 •

edited

Loading

lucasmrod Jul 21, 2022 •

edited

Loading

lucasmrod commented Jul 21, 2022 •

edited

Loading

lucasmrod commented Oct 3, 2022 •

edited

Loading